| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <2504080f-86af-161f-5c0d-284e89e33ce1@starfivetech.com>
Date: Wed, 2 Aug 2023 17:57:57 +0800
From: Jack Zhu <jack.zhu@...rfivetech.com>
To: Laurent Pinchart <laurent.pinchart@...asonboard.com>
CC: Mauro Carvalho Chehab <mchehab@...nel.org>,
Robert Foss <rfoss@...nel.org>,
Todor Tomov <todor.too@...il.com>,
<bryan.odonoghue@...aro.org>, Rob Herring <robh+dt@...nel.org>,
"Krzysztof Kozlowski" <krzysztof.kozlowski+dt@...aro.org>,
Conor Dooley <conor+dt@...nel.org>,
Philipp Zabel <p.zabel@...gutronix.de>,
Hans Verkuil <hverkuil-cisco@...all.nl>,
Eugen Hristev <eugen.hristev@...labora.com>,
Ezequiel Garcia <ezequiel@...guardiasur.com.ar>,
<linux-media@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
<devicetree@...r.kernel.org>, <changhuang.liang@...rfivetech.com>
Subject: Re: [PATCH v7 5/6] media: starfive: camss: Add ISP driver
Hi Laurent,
On 2023/7/28 4:41, Laurent Pinchart wrote:
> Hi Jack,
>
> Thank you for the patch.
>
> On Mon, Jun 19, 2023 at 07:28:37PM +0800, Jack Zhu wrote:
>> Add ISP driver for StarFive Camera Subsystem.
>>
>> Signed-off-by: Jack Zhu <jack.zhu@...rfivetech.com>
>> ---
>> .../media/platform/starfive/camss/Makefile | 2 +
>> .../media/platform/starfive/camss/stf_camss.c | 76 ++-
>> .../media/platform/starfive/camss/stf_camss.h | 3 +
>> .../media/platform/starfive/camss/stf_isp.c | 519 ++++++++++++++++++
>> .../media/platform/starfive/camss/stf_isp.h | 479 ++++++++++++++++
>> .../platform/starfive/camss/stf_isp_hw_ops.c | 468 ++++++++++++++++
>> 6 files changed, 1544 insertions(+), 3 deletions(-)
>> create mode 100644 drivers/media/platform/starfive/camss/stf_isp.c
>> create mode 100644 drivers/media/platform/starfive/camss/stf_isp.h
>> create mode 100644 drivers/media/platform/starfive/camss/stf_isp_hw_ops.c
>>
>> diff --git a/drivers/media/platform/starfive/camss/Makefile b/drivers/media/platform/starfive/camss/Makefile
>> index eb457917a914..cdf57e8c9546 100644
>> --- a/drivers/media/platform/starfive/camss/Makefile
>> +++ b/drivers/media/platform/starfive/camss/Makefile
>> @@ -5,6 +5,8 @@
>>
>> starfive-camss-objs += \
>> stf_camss.o \
>> + stf_isp.o \
>> + stf_isp_hw_ops.o \
>> stf_video.o
>>
>> obj-$(CONFIG_VIDEO_STARFIVE_CAMSS) += starfive-camss.o
>> diff --git a/drivers/media/platform/starfive/camss/stf_camss.c b/drivers/media/platform/starfive/camss/stf_camss.c
>> index dc2b5dba7bd4..6f56b45f57db 100644
>> --- a/drivers/media/platform/starfive/camss/stf_camss.c
>> +++ b/drivers/media/platform/starfive/camss/stf_camss.c
>> @@ -115,12 +115,65 @@ static int stfcamss_of_parse_ports(struct stfcamss *stfcamss)
>> return ret;
>> }
>>
>> +/*
>> + * stfcamss_init_subdevices - Initialize subdev structures and resources
>> + * @stfcamss: STFCAMSS device
>> + *
>> + * Return 0 on success or a negative error code on failure
>> + */
>> +static int stfcamss_init_subdevices(struct stfcamss *stfcamss)
>> +{
>> + int ret;
>> +
>> + ret = stf_isp_subdev_init(stfcamss);
>> + if (ret < 0) {
>> + dev_err(stfcamss->dev, "Failed to init isp subdev: %d\n", ret);
>> + return ret;
>> + }
>> +
>> + return ret;
>> +}
>> +
>> +static int stfcamss_register_subdevices(struct stfcamss *stfcamss)
>> +{
>> + int ret;
>> + struct stf_isp_dev *isp_dev = &stfcamss->isp_dev;
>> +
>> + ret = stf_isp_register(isp_dev, &stfcamss->v4l2_dev);
>> + if (ret < 0) {
>> + dev_err(stfcamss->dev,
>> + "Failed to register stf isp%d entity: %d\n", 0, ret);
>> + return ret;
>> + }
>> +
>> + return ret;
>> +}
>> +
>> +static void stfcamss_unregister_subdevices(struct stfcamss *stfcamss)
>> +{
>> + stf_isp_unregister(&stfcamss->isp_dev);
>> +}
>> +
>> static int stfcamss_subdev_notifier_bound(struct v4l2_async_notifier *async,
>> struct v4l2_subdev *subdev,
>> struct v4l2_async_subdev *asd)
>> {
>> + struct stfcamss *stfcamss =
>> + container_of(async, struct stfcamss, notifier);
>> + struct stfcamss_async_subdev *csd =
>> + container_of(asd, struct stfcamss_async_subdev, asd);
>> + enum stf_port_num port = csd->port;
>> + struct stf_isp_dev *isp_dev = &stfcamss->isp_dev;
>> struct media_pad *pad[STF_PADS_NUM];
>> - unsigned int i, pad_num = 0;
>> + unsigned int i, pad_num;
>> +
>> + if (port == STF_PORT_CSI2RX) {
>> + pad[0] = &isp_dev->pads[STF_PAD_SINK];
>> + pad_num = 1;
>> + } else if (port == STF_PORT_DVP) {
>> + dev_err(stfcamss->dev, "Not support DVP sensor\n");
>> + return -EPERM;
>> + }
>>
>> for (i = 0; i < pad_num; ++i) {
>> int ret;
>> @@ -223,12 +276,18 @@ static int stfcamss_probe(struct platform_device *pdev)
>> goto err_cleanup_notifier;
>> }
>>
>> + ret = stfcamss_init_subdevices(stfcamss);
>> + if (ret < 0) {
>> + dev_err(dev, "Failed to init subdevice: %d\n", ret);
>> + goto err_cleanup_notifier;
>> + }
>> +
>> stfcamss_mc_init(pdev, stfcamss);
>>
>> ret = v4l2_device_register(stfcamss->dev, &stfcamss->v4l2_dev);
>> if (ret < 0) {
>> dev_err(dev, "Failed to register V4L2 device: %d\n", ret);
>> - goto err_cleanup_notifier;
>> + goto err_cleanup_media_device;
>> }
>>
>> ret = media_device_register(&stfcamss->media_dev);
>> @@ -237,6 +296,12 @@ static int stfcamss_probe(struct platform_device *pdev)
>> goto err_unregister_device;
>> }
>>
>> + ret = stfcamss_register_subdevices(stfcamss);
>> + if (ret < 0) {
>> + dev_err(dev, "Failed to register subdevice: %d\n", ret);
>> + goto err_unregister_media_dev;
>> + }
>> +
>> pm_runtime_enable(dev);
>>
>> stfcamss->notifier.ops = &stfcamss_subdev_notifier_ops;
>> @@ -244,15 +309,19 @@ static int stfcamss_probe(struct platform_device *pdev)
>> if (ret) {
>> dev_err(dev, "Failed to register async subdev nodes: %d\n",
>> ret);
>> - goto err_unregister_media_dev;
>> + goto err_unregister_subdevs;
>> }
>>
>> return 0;
>>
>> +err_unregister_subdevs:
>> + stfcamss_unregister_subdevices(stfcamss);
>> err_unregister_media_dev:
>> media_device_unregister(&stfcamss->media_dev);
>> err_unregister_device:
>> v4l2_device_unregister(&stfcamss->v4l2_dev);
>> +err_cleanup_media_device:
>> + media_device_cleanup(&stfcamss->media_dev);
>> err_cleanup_notifier:
>> v4l2_async_nf_cleanup(&stfcamss->notifier);
>> return ret;
>> @@ -268,6 +337,7 @@ static int stfcamss_remove(struct platform_device *pdev)
>> {
>> struct stfcamss *stfcamss = platform_get_drvdata(pdev);
>>
>> + stfcamss_unregister_subdevices(stfcamss);
>> v4l2_device_unregister(&stfcamss->v4l2_dev);
>> media_device_cleanup(&stfcamss->media_dev);
>> pm_runtime_disable(&pdev->dev);
>> diff --git a/drivers/media/platform/starfive/camss/stf_camss.h b/drivers/media/platform/starfive/camss/stf_camss.h
>> index 15c4f34b9054..9482081288fa 100644
>> --- a/drivers/media/platform/starfive/camss/stf_camss.h
>> +++ b/drivers/media/platform/starfive/camss/stf_camss.h
>> @@ -18,6 +18,8 @@
>> #include <media/v4l2-async.h>
>> #include <media/v4l2-device.h>
>>
>> +#include "stf_isp.h"
>> +
>> #define STF_DVP_NAME "stf_dvp"
>> #define STF_CSI_NAME "cdns_csi2rx"
>> #define STF_ISP_NAME "stf_isp"
>> @@ -65,6 +67,7 @@ struct stfcamss {
>> struct media_device media_dev;
>> struct media_pipeline pipe;
>> struct device *dev;
>> + struct stf_isp_dev isp_dev;
>> struct v4l2_async_notifier notifier;
>> void __iomem *syscon_base;
>> void __iomem *isp_base;
>> diff --git a/drivers/media/platform/starfive/camss/stf_isp.c b/drivers/media/platform/starfive/camss/stf_isp.c
>> new file mode 100644
>> index 000000000000..933a583b398c
>> --- /dev/null
>> +++ b/drivers/media/platform/starfive/camss/stf_isp.c
>> @@ -0,0 +1,519 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * stf_isp.c
>> + *
>> + * StarFive Camera Subsystem - ISP Module
>> + *
>> + * Copyright (C) 2021-2023 StarFive Technology Co., Ltd.
>> + */
>> +#include <linux/firmware.h>
>
> This doesn't seem needed.
>
>> +#include <media/v4l2-event.h>
>> +
>> +#include "stf_camss.h"
>> +
>> +#define SINK_FORMATS_INDEX 0
>> +#define UO_FORMATS_INDEX 1
>
> What does "UO" stand for ?
>
"UO" is Usual Out, just represents output. :-)
>> +
>> +static int isp_set_selection(struct v4l2_subdev *sd,
>> + struct v4l2_subdev_state *state,
>> + struct v4l2_subdev_selection *sel);
>> +
>> +static const struct isp_format isp_formats_sink[] = {
>> + { MEDIA_BUS_FMT_SRGGB10_1X10, 10 },
>> + { MEDIA_BUS_FMT_SGRBG10_1X10, 10 },
>> + { MEDIA_BUS_FMT_SGBRG10_1X10, 10 },
>> + { MEDIA_BUS_FMT_SBGGR10_1X10, 10 },
>> +};
>> +
>> +static const struct isp_format isp_formats_uo[] = {
>> + { MEDIA_BUS_FMT_Y12_1X12, 8 },
>
> Y12 is a greyscale format, I don't think that's what you need here.
>
>> +};
>> +
>> +static const struct isp_format_table isp_formats_st7110[] = {
>> + { isp_formats_sink, ARRAY_SIZE(isp_formats_sink) },
>> + { isp_formats_uo, ARRAY_SIZE(isp_formats_uo) },
>> +};
>> +
>> +int stf_isp_subdev_init(struct stfcamss *stfcamss)
>
> This function doesn't initialize the subdev, I'd call it stf_isp_init().
> I would also create a stf_isp_cleanup() function to be consistent, and
> move the mutex_destroy() call there.
>
>> +{
>> + struct stf_isp_dev *isp_dev = &stfcamss->isp_dev;
>> +
>> + isp_dev->stfcamss = stfcamss;
>> + isp_dev->formats = isp_formats_st7110;
>> + isp_dev->nformats = ARRAY_SIZE(isp_formats_st7110);
>> +
>> + mutex_init(&isp_dev->stream_lock);
>> + return 0;
>> +}
>> +
>> +static struct v4l2_mbus_framefmt *
>> +__isp_get_format(struct stf_isp_dev *isp_dev,
>> + struct v4l2_subdev_state *state,
>> + unsigned int pad,
>> + enum v4l2_subdev_format_whence which)
>> +{
>> + if (which == V4L2_SUBDEV_FORMAT_TRY)
>> + return v4l2_subdev_get_try_format(&isp_dev->subdev, state, pad);
>> +
>> + return &isp_dev->fmt[pad];
>
> Use the subdev active state API to store the active format on subdev
> pads, it will simplify the driver. See commit a2514b9a634a ("media: i2c:
> imx290: Use V4L2 subdev active state") for an example of a subdev driver
> being converted to that API, it should help understanding how to use it.
> You will be able to drop the stf_isp_dev fmt and rect fields. For the
> bpp value stored in the rect structure, replace it with a const struct
> isp_format pointer in stf_isp_dev that you set in .set_fmt() (as well as
> initialization time) to the active format information.
>
>> +}
>> +
>> +static int isp_set_stream(struct v4l2_subdev *sd, int enable)
>> +{
>> + struct stf_isp_dev *isp_dev = v4l2_get_subdevdata(sd);
>> + int ret = 0;
>> + struct v4l2_mbus_framefmt *fmt;
>> + struct v4l2_event src_ch = { 0 };
>> +
>> + fmt = __isp_get_format(isp_dev, NULL, STF_ISP_PAD_SINK,
>> + V4L2_SUBDEV_FORMAT_ACTIVE);
>> + mutex_lock(&isp_dev->stream_lock);
>> + if (enable) {
>> + if (isp_dev->stream_count == 0) {
>> + stf_isp_clk_enable(isp_dev);
>> + stf_isp_reset(isp_dev);
>> + stf_isp_init_cfg(isp_dev);
>> + stf_isp_settings(isp_dev, isp_dev->rect, fmt->code);
>> + stf_isp_stream_set(isp_dev);
>> + }
>> + isp_dev->stream_count++;
>
> The subdev .s_stream() operation isn't supposed to be called multiple
> times. If you need to count how many video nodes have started streaming,
> that should be handled in the vb2 queue .start_streaming() and
> .stop_streaming() operations.
>
>> + } else {
>> + if (isp_dev->stream_count == 0)
>> + goto exit;
>> +
>> + if (isp_dev->stream_count == 1)
>> + stf_isp_clk_disable(isp_dev);
>> +
>> + isp_dev->stream_count--;
>> + }
>> + src_ch.type = V4L2_EVENT_SOURCE_CHANGE,
>
> ; instead of , at the end of the line. Same for the next line.
>
>> + src_ch.u.src_change.changes = isp_dev->stream_count,
>> +
>> + v4l2_subdev_notify_event(sd, &src_ch);
>
> This doesn't seem right. V4L2_EVENT_SOURCE_CHANGE is meant to report
> changes to the source format. I would drop support for events
> completely from this patch, it doesn't seem to be needed.
>
>> +exit:
>> + mutex_unlock(&isp_dev->stream_lock);
>> +
>> + return ret;
>
> ret is never set to a value other than 9, return 0 and drop the
> variable.
>
>> +}
>> +
>> +static void isp_try_format(struct stf_isp_dev *isp_dev,
>> + struct v4l2_subdev_state *state,
>> + unsigned int pad,
>> + struct v4l2_mbus_framefmt *fmt,
>> + enum v4l2_subdev_format_whence which)
>> +{
>> + const struct isp_format_table *formats;
>> + struct stf_isp_crop *rect;
>> + unsigned int i;
>> +
>> + if (pad == STF_ISP_PAD_SINK) {
>> + /* Set format on sink pad */
>> + formats = &isp_dev->formats[SINK_FORMATS_INDEX];
>> + rect = &isp_dev->rect[SINK_FORMATS_INDEX];
>> + } else if (pad == STF_ISP_PAD_SRC) {
>> + formats = &isp_dev->formats[UO_FORMATS_INDEX];
>> + rect = &isp_dev->rect[UO_FORMATS_INDEX];
>> + }
>> +
>> + fmt->width = clamp_t(u32, fmt->width, STFCAMSS_FRAME_MIN_WIDTH,
>> + STFCAMSS_FRAME_MAX_WIDTH);
>> + fmt->height = clamp_t(u32, fmt->height, STFCAMSS_FRAME_MIN_HEIGHT,
>> + STFCAMSS_FRAME_MAX_HEIGHT);
>> + fmt->height &= ~0x1;
>> + fmt->field = V4L2_FIELD_NONE;
>> + fmt->colorspace = V4L2_COLORSPACE_SRGB;
>> + fmt->flags = 0;
>> +
>> + for (i = 0; i < formats->nfmts; i++) {
>> + if (fmt->code == formats->fmts[i].code)
>> + break;
>> + }
>> +
>> + if (i >= formats->nfmts) {
>> + fmt->code = formats->fmts[0].code;
>> + rect->bpp = formats->fmts[0].bpp;
>> + } else {
>> + rect->bpp = formats->fmts[i].bpp;
>> + }
>> +}
>> +
>> +static int isp_enum_mbus_code(struct v4l2_subdev *sd,
>> + struct v4l2_subdev_state *state,
>> + struct v4l2_subdev_mbus_code_enum *code)
>> +{
>> + struct stf_isp_dev *isp_dev = v4l2_get_subdevdata(sd);
>> + const struct isp_format_table *formats;
>> +
>> + if (code->index >= isp_dev->nformats)
>
> That doesn't seem right, isp_dev->nformats isn't the number of formats
> supported on the ISP, but the number of format types (sink and uo).
>
>> + return -EINVAL;
>
> Add a blank line here, it makes code easier to read.
>
>> + if (code->pad == STF_ISP_PAD_SINK) {
>> + formats = &isp_dev->formats[SINK_FORMATS_INDEX];
>> + code->code = formats->fmts[code->index].code;
>> + } else {
>> + struct v4l2_mbus_framefmt *sink_fmt;
>> +
>> + sink_fmt = __isp_get_format(isp_dev, state, STF_ISP_PAD_SINK,
>> + code->which);
>> +
>> + code->code = sink_fmt->code;
>
> This doesn't seem right, you need to enumerate the ISP output media bus
> codes here, and the function returns the sink media bus code instead.
>
>> + if (!code->code)
>> + return -EINVAL;
>> + }
>> + code->flags = 0;
>> +
>> + return 0;
>> +}
>> +
>> +static int isp_enum_frame_size(struct v4l2_subdev *sd,
>> + struct v4l2_subdev_state *state,
>> + struct v4l2_subdev_frame_size_enum *fse)
>> +{
>> + struct stf_isp_dev *isp_dev = v4l2_get_subdevdata(sd);
>> + struct v4l2_mbus_framefmt format;
>> +
>> + if (fse->index != 0)
>> + return -EINVAL;
>> +
>> + format.code = fse->code;
>> + format.width = 1;
>> + format.height = 1;
>> + isp_try_format(isp_dev, state, fse->pad, &format, fse->which);
>> + fse->min_width = format.width;
>> + fse->min_height = format.height;
>> +
>> + if (format.code != fse->code)
>> + return -EINVAL;
>> +
>> + format.code = fse->code;
>> + format.width = -1;
>> + format.height = -1;
>> + isp_try_format(isp_dev, state, fse->pad, &format, fse->which);
>> + fse->max_width = format.width;
>> + fse->max_height = format.height;
>
> This seems unnecessarily complicated, you can just set the min and max
> width and height to STFCAMSS_FRAME_MIN_WIDTH, STFCAMSS_FRAME_MAX_WIDTH,
> STFCAMSS_FRAME_MIN_HEIGHT and STFCAMSS_FRAME_MAX_HEIGHT here.
>
>> +
>> + return 0;
>> +}
>> +
>> +static int isp_get_format(struct v4l2_subdev *sd,
>> + struct v4l2_subdev_state *state,
>> + struct v4l2_subdev_format *fmt)
>> +{
>> + struct stf_isp_dev *isp_dev = v4l2_get_subdevdata(sd);
>> + struct v4l2_mbus_framefmt *format;
>> +
>> + format = __isp_get_format(isp_dev, state, fmt->pad, fmt->which);
>> + if (!format)
>> + return -EINVAL;
>> +
>> + fmt->format = *format;
>> +
>> + return 0;
>> +}
>> +
>> +static int isp_set_format(struct v4l2_subdev *sd,
>> + struct v4l2_subdev_state *state,
>> + struct v4l2_subdev_format *fmt)
>> +{
>> + struct stf_isp_dev *isp_dev = v4l2_get_subdevdata(sd);
>> + struct v4l2_mbus_framefmt *format;
>> +
>> + format = __isp_get_format(isp_dev, state, fmt->pad, fmt->which);
>> + if (!format)
>> + return -EINVAL;
>> +
>> + mutex_lock(&isp_dev->stream_lock);
>> +
>> + isp_try_format(isp_dev, state, fmt->pad, &fmt->format, fmt->which);
>> + *format = fmt->format;
>> +
>> + mutex_unlock(&isp_dev->stream_lock);
>> +
>> + /* Propagate to in crop */
>> + if (fmt->pad == STF_ISP_PAD_SINK) {
>> + struct v4l2_subdev_selection sel = { 0 };
>> + int ret;
>> +
>> + /* Reset sink pad compose selection */
>> + sel.which = fmt->which;
>> + sel.pad = STF_ISP_PAD_SINK;
>> + sel.target = V4L2_SEL_TGT_CROP;
>> + sel.r.width = fmt->format.width;
>> + sel.r.height = fmt->format.height;
>> + ret = isp_set_selection(sd, state, &sel);
>> + if (ret < 0)
>> + return ret;
>
> You're setting the which, pad and target values manually, so they're
> guaranteed to be correct. isp_set_selection() will thus never return an
> error. You can drop the error check.
>
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static struct v4l2_rect *
>> +__isp_get_crop(struct stf_isp_dev *isp_dev,
>> + struct v4l2_subdev_state *state,
>> + unsigned int pad,
>> + enum v4l2_subdev_format_whence which)
>> +{
>> + if (which == V4L2_SUBDEV_FORMAT_TRY)
>> + return v4l2_subdev_get_try_crop(&isp_dev->subdev, state,
>> + STF_ISP_PAD_SINK);
>> +
>> + return &isp_dev->rect[pad].rect;
>> +}
>> +
>> +static void isp_try_crop(struct stf_isp_dev *isp_dev,
>> + struct v4l2_subdev_state *state,
>> + struct v4l2_rect *rect,
>> + enum v4l2_subdev_format_whence which)
>> +{
>> + struct v4l2_mbus_framefmt *fmt;
>> +
>> + fmt = __isp_get_format(isp_dev, state, STF_ISP_PAD_SINK, which);
>> +
>> + if (rect->width > fmt->width)
>> + rect->width = fmt->width;
>> +
>> + if (rect->width + rect->left > fmt->width)
>> + rect->left = fmt->width - rect->width;
>> +
>> + if (rect->height > fmt->height)
>> + rect->height = fmt->height;
>> +
>> + if (rect->height + rect->top > fmt->height)
>> + rect->top = fmt->height - rect->height;
>> +
>> + if (rect->width < STFCAMSS_FRAME_MIN_WIDTH) {
>> + rect->left = 0;
>> + rect->width = STFCAMSS_FRAME_MAX_WIDTH;
>> + }
>> +
>> + if (rect->height < STFCAMSS_FRAME_MIN_HEIGHT) {
>> + rect->top = 0;
>> + rect->height = STFCAMSS_FRAME_MAX_HEIGHT;
>> + }
>> + rect->height &= ~0x1;
>
> The v4l2_rect_set_min_size() and v4l2_rect_map_inside() functions can
> simplify this code.
>
>> +}
>> +
>> +static int isp_get_selection(struct v4l2_subdev *sd,
>> + struct v4l2_subdev_state *state,
>> + struct v4l2_subdev_selection *sel)
>> +{
>> + struct stf_isp_dev *isp_dev = v4l2_get_subdevdata(sd);
>> + struct v4l2_subdev_format fmt = { 0 };
>> + struct v4l2_rect *rect;
>> + int ret;
>> +
>> + switch (sel->target) {
>> + case V4L2_SEL_TGT_CROP_BOUNDS:
>> + if (sel->pad == STF_ISP_PAD_SINK) {
>> + fmt.pad = sel->pad;
>> + fmt.which = sel->which;
>> + ret = isp_get_format(sd, state, &fmt);
>> + if (ret < 0)
>> + return ret;
>> +
>> + sel->r.left = 0;
>> + sel->r.top = 0;
>> + sel->r.width = fmt.format.width;
>> + sel->r.height = fmt.format.height;
>> + } else if (sel->pad == STF_ISP_PAD_SRC) {
>> + rect = __isp_get_crop(isp_dev, state,
>> + sel->pad, sel->which);
>> + sel->r = *rect;
>> + }
>> + break;
>> +
>> + case V4L2_SEL_TGT_CROP:
>> + rect = __isp_get_crop(isp_dev, state, sel->pad, sel->which);
>> + if (!rect)
>> + return -EINVAL;
>> +
>> + sel->r = *rect;
>> + break;
>> +
>> + default:
>> + return -EINVAL;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int isp_set_selection(struct v4l2_subdev *sd,
>> + struct v4l2_subdev_state *state,
>> + struct v4l2_subdev_selection *sel)
>> +{
>> + struct stf_isp_dev *isp_dev = v4l2_get_subdevdata(sd);
>> + struct v4l2_rect *rect;
>> + int ret = 0;
>> +
>
> if (sel->target != V4L2_SEL_TGT_CROP)
> return -EINVAL;
>
>> + if (sel->target == V4L2_SEL_TGT_CROP &&
>> + sel->pad == STF_ISP_PAD_SINK) {
>> + struct v4l2_subdev_selection crop = { 0 };
>> +
>> + rect = __isp_get_crop(isp_dev, state, sel->pad, sel->which);
>> + if (!rect)
>> + return -EINVAL;
>> +
>> + mutex_lock(&isp_dev->stream_lock);
>> + isp_try_crop(isp_dev, state, &sel->r, sel->which);
>> + *rect = sel->r;
>> + mutex_unlock(&isp_dev->stream_lock);
>> +
>> + /* Reset source crop selection */
>> + crop.which = sel->which;
>> + crop.pad = STF_ISP_PAD_SRC;
>> + crop.target = V4L2_SEL_TGT_CROP;
>> + crop.r = *rect;
>> + ret = isp_set_selection(sd, state, &crop);
>> + } else if (sel->target == V4L2_SEL_TGT_CROP &&
>> + sel->pad == STF_ISP_PAD_SRC) {
>> + struct v4l2_subdev_format fmt = { 0 };
>> +
>> + rect = __isp_get_crop(isp_dev, state, sel->pad, sel->which);
>> + if (!rect)
>> + return -EINVAL;
>> +
>> + mutex_lock(&isp_dev->stream_lock);
>> + isp_try_crop(isp_dev, state, &sel->r, sel->which);
>> + *rect = sel->r;
>> + mutex_unlock(&isp_dev->stream_lock);
>> +
>> + /* Reset source pad format width and height */
>> + fmt.which = sel->which;
>> + fmt.pad = STF_ISP_PAD_SRC;
>> + fmt.format.width = rect->width;
>> + fmt.format.height = rect->height;
>> + ret = isp_set_format(sd, state, &fmt);
>> + if (ret < 0)
>> + return ret;
>
> You can drop the error check here too.
>
>> + }
>> +
>> + dev_dbg(isp_dev->stfcamss->dev, "pad: %d sel(%d,%d)/%dx%d\n",
>> + sel->pad, sel->r.left, sel->r.top, sel->r.width, sel->r.height);
>> +
>> + return 0;
>> +}
>> +
>> +static int isp_init_formats(struct v4l2_subdev *sd,
>> + struct v4l2_subdev_fh *fh)
>> +{
>> + struct v4l2_subdev_format format = {
>> + .pad = STF_ISP_PAD_SINK,
>> + .which =
>> + fh ? V4L2_SUBDEV_FORMAT_TRY : V4L2_SUBDEV_FORMAT_ACTIVE,
>> + .format = {
>> + .code = MEDIA_BUS_FMT_RGB565_2X8_LE,
>
> Pick a format that the driver supports.
>
>> + .width = 1920,
>> + .height = 1080
>> + }
>> + };
>> +
>> + return isp_set_format(sd, fh ? fh->state : NULL, &format);
>> +}
>> +
>> +static int isp_link_setup(struct media_entity *entity,
>> + const struct media_pad *local,
>> + const struct media_pad *remote, u32 flags)
>> +{
>> + if (flags & MEDIA_LNK_FL_ENABLED)
>> + if (media_pad_remote_pad_first(local))
>> + return -EBUSY;
>> + return 0;
>
> Is this check really needed ?
>
>> +}
>> +
>> +static int stf_isp_subscribe_event(struct v4l2_subdev *sd,
>> + struct v4l2_fh *fh,
>> + struct v4l2_event_subscription *sub)
>> +{
>> + switch (sub->type) {
>> + case V4L2_EVENT_SOURCE_CHANGE:
>> + return v4l2_src_change_event_subdev_subscribe(sd, fh, sub);
>> + default:
>> + return -EINVAL;
>> + }
>> +}
>> +
>> +static const struct v4l2_subdev_core_ops isp_core_ops = {
>> + .subscribe_event = stf_isp_subscribe_event,
>> + .unsubscribe_event = v4l2_event_subdev_unsubscribe,
>> +};
>
> Drop events support.
>
>> +
>> +static const struct v4l2_subdev_video_ops isp_video_ops = {
>> + .s_stream = isp_set_stream,
>> +};
>> +
>> +static const struct v4l2_subdev_pad_ops isp_pad_ops = {
>> + .enum_mbus_code = isp_enum_mbus_code,
>> + .enum_frame_size = isp_enum_frame_size,
>> + .get_fmt = isp_get_format,
>> + .set_fmt = isp_set_format,
>> + .get_selection = isp_get_selection,
>> + .set_selection = isp_set_selection,
>> +};
>> +
>> +static const struct v4l2_subdev_ops isp_v4l2_ops = {
>> + .core = &isp_core_ops,
>> + .video = &isp_video_ops,
>> + .pad = &isp_pad_ops,
>> +};
>> +
>> +static const struct v4l2_subdev_internal_ops isp_v4l2_internal_ops = {
>> + .open = isp_init_formats,
>> +};
>
> Replace this with the .init_cfg() pad operation.
>
>> +
>> +static const struct media_entity_operations isp_media_ops = {
>> + .link_setup = isp_link_setup,
>> + .link_validate = v4l2_subdev_link_validate,
>> +};
>> +
>> +int stf_isp_register(struct stf_isp_dev *isp_dev, struct v4l2_device *v4l2_dev)
>> +{
>> + struct v4l2_subdev *sd = &isp_dev->subdev;
>> + struct media_pad *pads = isp_dev->pads;
>> + int ret;
>> +
>> + v4l2_subdev_init(sd, &isp_v4l2_ops);
>> + sd->internal_ops = &isp_v4l2_internal_ops;
>> + sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE | V4L2_SUBDEV_FL_HAS_EVENTS;
>
> Drop events here too.
>
>> + snprintf(sd->name, ARRAY_SIZE(sd->name), "%s%d", STF_ISP_NAME, 0);
>> + v4l2_set_subdevdata(sd, isp_dev);
>> +
>> + ret = isp_init_formats(sd, NULL);
>> + if (ret < 0) {
>> + dev_err(isp_dev->stfcamss->dev, "Failed to init format: %d\n",
>> + ret);
>> + return ret;
>> + }
>> +
>> + pads[STF_ISP_PAD_SINK].flags = MEDIA_PAD_FL_SINK;
>> + pads[STF_ISP_PAD_SRC].flags = MEDIA_PAD_FL_SOURCE;
>> +
>> + sd->entity.function = MEDIA_ENT_F_PROC_VIDEO_PIXEL_FORMATTER;
>
> MEDIA_ENT_F_PROC_VIDEO_ISP seems a better match.
>
>> + sd->entity.ops = &isp_media_ops;
>> + ret = media_entity_pads_init(&sd->entity, STF_ISP_PAD_MAX, pads);
>> + if (ret < 0) {
>> + dev_err(isp_dev->stfcamss->dev,
>> + "Failed to init media entity: %d\n", ret);
>> + return ret;
>> + }
>> +
>> + ret = v4l2_device_register_subdev(v4l2_dev, sd);
>> + if (ret < 0) {
>> + dev_err(isp_dev->stfcamss->dev,
>> + "Failed to register subdev: %d\n", ret);
>> + goto err_sreg;
>> + }
>> +
>> + return 0;
>> +
>> +err_sreg:
>> + media_entity_cleanup(&sd->entity);
>> + return ret;
>> +}
>> +
>> +int stf_isp_unregister(struct stf_isp_dev *isp_dev)
>> +{
>> + v4l2_device_unregister_subdev(&isp_dev->subdev);
>> + media_entity_cleanup(&isp_dev->subdev.entity);
>> + mutex_destroy(&isp_dev->stream_lock);
>> + return 0;
>> +}
>> diff --git a/drivers/media/platform/starfive/camss/stf_isp.h b/drivers/media/platform/starfive/camss/stf_isp.h
>> new file mode 100644
>> index 000000000000..1e5c98482350
>> --- /dev/null
>> +++ b/drivers/media/platform/starfive/camss/stf_isp.h
>> @@ -0,0 +1,479 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/*
>> + * stf_isp.h
>> + *
>> + * StarFive Camera Subsystem - ISP Module
>> + *
>> + * Copyright (C) 2021-2023 StarFive Technology Co., Ltd.
>> + */
>> +
>> +#ifndef STF_ISP_H
>> +#define STF_ISP_H
>> +
>> +#include <media/v4l2-subdev.h>
>> +
>> +#include "stf_video.h"
>> +
>> +#define ISP_RAW_DATA_BITS 12
>> +#define SCALER_RATIO_MAX 1
>> +#define STF_ISP_REG_OFFSET_MAX 0x0fff
>> +#define STF_ISP_REG_DELAY_MAX 100
>> +
>> +/* isp registers */
>> +#define ISP_REG_CSI_INPUT_EN_AND_STATUS 0x000
>> +#define CSI_SCD_ERR BIT(6)
>> +#define CSI_ITU656_ERR BIT(4)
>> +#define CSI_ITU656_F BIT(3)
>> +#define CSI_SCD_DONE BIT(2)
>> +#define CSI_BUSY_S BIT(1)
>> +#define CSI_EN_S BIT(0)
>
> Could you align the macros values consistently in the whole file ? It
> would be easier to read. For the lines above, that would be
>
> #define ISP_RAW_DATA_BITS 12
> #define SCALER_RATIO_MAX 1
> #define STF_ISP_REG_OFFSET_MAX 0x0fff
> #define STF_ISP_REG_DELAY_MAX 100
>
> /* isp registers */
> #define ISP_REG_CSI_INPUT_EN_AND_STATUS 0x000
> #define CSI_SCD_ERR BIT(6)
> #define CSI_ITU656_ERR BIT(4)
> #define CSI_ITU656_F BIT(3)
> #define CSI_SCD_DONE BIT(2)
> #define CSI_BUSY_S BIT(1)
> #define CSI_EN_S BIT(0)
>
> (possibly with additional indentation if some of the lines below require
> that)
>
> You could also move the register definitions to a stf_isp_regs.h header
> if desired, as they're only needed by stf_isp_hw_ops.c, not by the other
> files that include stf_isp.h. up to you.
>
>> +
>> +#define ISP_REG_CSIINTS 0x008
>> +#define CSI_INTS(n) ((n) << 16)
>> +#define CSI_SHA_M(n) ((n) << 0)
>> +#define CSI_INTS_MASK GENMASK(17, 16)
>> +
>> +#define ISP_REG_CSI_MODULE_CFG 0x010
>> +#define CSI_DUMP_EN BIT(19)
>> +#define CSI_VS_EN BIT(18)
>> +#define CSI_SC_EN BIT(17)
>> +#define CSI_OBA_EN BIT(16)
>> +#define CSI_AWB_EN BIT(7)
>> +#define CSI_LCCF_EN BIT(6)
>> +#define CSI_OECFHM_EN BIT(5)
>> +#define CSI_OECF_EN BIT(4)
>> +#define CSI_LCBQ_EN BIT(3)
>> +#define CSI_OBC_EN BIT(2)
>> +#define CSI_DEC_EN BIT(1)
>> +#define CSI_DC_EN BIT(0)
>> +
>> +#define ISP_REG_SENSOR 0x014
>> +#define DVP_SYNC_POL(n) ((n) << 2)
>> +#define ITU656_EN(n) ((n) << 1)
>> +#define IMAGER_SEL(n) ((n) << 0)
>> +
>> +#define ISP_REG_RAW_FORMAT_CFG 0x018
>> +#define SMY13(n) ((n) << 14)
>> +#define SMY12(n) ((n) << 12)
>> +#define SMY11(n) ((n) << 10)
>> +#define SMY10(n) ((n) << 8)
>> +#define SMY3(n) ((n) << 6)
>> +#define SMY2(n) ((n) << 4)
>> +#define SMY1(n) ((n) << 2)
>> +#define SMY0(n) ((n) << 0)
>> +
>> +#define ISP_REG_PIC_CAPTURE_START_CFG 0x01c
>> +#define VSTART_CAP(n) ((n) << 16)
>> +#define HSTART_CAP(n) ((n) << 0)
>> +
>> +#define ISP_REG_PIC_CAPTURE_END_CFG 0x020
>> +#define VEND_CAP(n) ((n) << 16)
>> +#define HEND_CAP(n) ((n) << 0)
>> +
>> +#define ISP_REG_DUMP_CFG_0 0x024
>> +#define ISP_REG_DUMP_CFG_1 0x028
>> +#define DUMP_ID(n) ((n) << 24)
>> +#define DUMP_SHT(n) ((n) << 20)
>> +#define DUMP_BURST_LEN(n) ((n) << 16)
>> +#define DUMP_SD(n) ((n) << 0)
>> +#define DUMP_BURST_LEN_MASK GENMASK(17, 16)
>> +#define DUMP_SD_MASK GENMASK(15, 0)
>> +
>> +#define ISP_REG_DEC_CFG 0x030
>> +#define DEC_V_KEEP(n) ((n) << 24)
>> +#define DEC_V_PERIOD(n) ((n) << 16)
>> +#define DEC_H_KEEP(n) ((n) << 8)
>> +#define DEC_H_PERIOD(n) ((n) << 0)
>> +
>> +#define ISP_REG_OBC_CFG 0x034
>> +#define OBC_W_H(y) ((y) << 4)
>> +#define OBC_W_W(x) ((x) << 0)
>> +
>> +#define ISP_REG_DC_CFG_1 0x044
>> +#define DC_AXI_ID(n) ((n) << 0)
>> +
>> +#define ISP_REG_LCCF_CFG_0 0x050
>> +#define Y_DISTANCE(y) ((y) << 16)
>> +#define X_DISTANCE(x) ((x) << 0)
>> +
>> +#define ISP_REG_LCCF_CFG_1 0x058
>> +#define LCCF_MAX_DIS(n) ((n) << 0)
>> +
>> +#define ISP_REG_LCBQ_CFG_0 0x074
>> +#define H_LCBQ(y) ((y) << 12)
>> +#define W_LCBQ(x) ((x) << 8)
>> +
>> +#define ISP_REG_LCBQ_CFG_1 0x07c
>> +#define Y_COOR(y) ((y) << 16)
>> +#define X_COOR(x) ((x) << 0)
>> +
>> +#define ISP_REG_LCCF_CFG_2 0x0e0
>> +#define ISP_REG_LCCF_CFG_3 0x0e4
>> +#define ISP_REG_LCCF_CFG_4 0x0e8
>> +#define ISP_REG_LCCF_CFG_5 0x0ec
>> +#define LCCF_F2_PAR(n) ((n) << 16)
>> +#define LCCF_F1_PAR(n) ((n) << 0)
>> +
>> +#define ISP_REG_OECF_X0_CFG0 0x100
>> +#define ISP_REG_OECF_X0_CFG1 0x104
>> +#define ISP_REG_OECF_X0_CFG2 0x108
>> +#define ISP_REG_OECF_X0_CFG3 0x10c
>> +#define ISP_REG_OECF_X0_CFG4 0x110
>> +#define ISP_REG_OECF_X0_CFG5 0x114
>> +#define ISP_REG_OECF_X0_CFG6 0x118
>> +#define ISP_REG_OECF_X0_CFG7 0x11c
>> +
>> +#define ISP_REG_OECF_Y3_CFG0 0x1e0
>> +#define ISP_REG_OECF_Y3_CFG1 0x1e4
>> +#define ISP_REG_OECF_Y3_CFG2 0x1e8
>> +#define ISP_REG_OECF_Y3_CFG3 0x1ec
>> +#define ISP_REG_OECF_Y3_CFG4 0x1f0
>> +#define ISP_REG_OECF_Y3_CFG5 0x1f4
>> +#define ISP_REG_OECF_Y3_CFG6 0x1f8
>> +#define ISP_REG_OECF_Y3_CFG7 0x1fc
>> +
>> +#define ISP_REG_OECF_S0_CFG0 0x200
>> +#define ISP_REG_OECF_S3_CFG7 0x27c
>> +#define OCEF_PAR_H(n) ((n) << 16)
>> +#define OCEF_PAR_L(n) ((n) << 0)
>> +
>> +#define ISP_REG_AWB_X0_CFG_0 0x280
>> +#define ISP_REG_AWB_X0_CFG_1 0x284
>> +#define ISP_REG_AWB_X1_CFG_0 0x288
>> +#define ISP_REG_AWB_X1_CFG_1 0x28c
>> +#define ISP_REG_AWB_X2_CFG_0 0x290
>> +#define ISP_REG_AWB_X2_CFG_1 0x294
>> +#define ISP_REG_AWB_X3_CFG_0 0x298
>> +#define ISP_REG_AWB_X3_CFG_1 0x29c
>> +#define AWB_X_SYMBOL_H(n) ((n) << 16)
>> +#define AWB_X_SYMBOL_L(n) ((n) << 0)
>> +
>> +#define ISP_REG_AWB_Y0_CFG_0 0x2a0
>> +#define ISP_REG_AWB_Y0_CFG_1 0x2a4
>> +#define ISP_REG_AWB_Y1_CFG_0 0x2a8
>> +#define ISP_REG_AWB_Y1_CFG_1 0x2ac
>> +#define ISP_REG_AWB_Y2_CFG_0 0x2b0
>> +#define ISP_REG_AWB_Y2_CFG_1 0x2b4
>> +#define ISP_REG_AWB_Y3_CFG_0 0x2b8
>> +#define ISP_REG_AWB_Y3_CFG_1 0x2bc
>> +#define AWB_Y_SYMBOL_H(n) ((n) << 16)
>> +#define AWB_Y_SYMBOL_L(n) ((n) << 0)
>> +
>> +#define ISP_REG_AWB_S0_CFG_0 0x2c0
>> +#define ISP_REG_AWB_S0_CFG_1 0x2c4
>> +#define ISP_REG_AWB_S1_CFG_0 0x2c8
>> +#define ISP_REG_AWB_S1_CFG_1 0x2cc
>> +#define ISP_REG_AWB_S2_CFG_0 0x2d0
>> +#define ISP_REG_AWB_S2_CFG_1 0x2d4
>> +#define ISP_REG_AWB_S3_CFG_0 0x2d8
>> +#define ISP_REG_AWB_S3_CFG_1 0x2dc
>> +#define AWB_S_SYMBOL_H(n) ((n) << 16)
>> +#define AWB_S_SYMBOL_L(n) ((n) << 0)
>> +
>> +#define ISP_REG_OBCG_CFG_0 0x2e0
>> +#define ISP_REG_OBCG_CFG_1 0x2e4
>> +#define ISP_REG_OBCG_CFG_2 0x2e8
>> +#define ISP_REG_OBCG_CFG_3 0x2ec
>> +#define ISP_REG_OBCO_CFG_0 0x2f0
>> +#define ISP_REG_OBCO_CFG_1 0x2f4
>> +#define ISP_REG_OBCO_CFG_2 0x2f8
>> +#define ISP_REG_OBCO_CFG_3 0x2fc
>> +#define GAIN_D_POINT(x) ((x) << 24)
>> +#define GAIN_C_POINT(x) ((x) << 16)
>> +#define GAIN_B_POINT(x) ((x) << 8)
>> +#define GAIN_A_POINT(x) ((x) << 0)
>> +#define OFFSET_D_POINT(x) ((x) << 24)
>> +#define OFFSET_C_POINT(x) ((x) << 16)
>> +#define OFFSET_B_POINT(x) ((x) << 8)
>> +#define OFFSET_A_POINT(x) ((x) << 0)
>> +
>> +#define ISP_REG_ISP_CTRL_0 0xa00
>> +#define ISPC_SCFEINT BIT(27)
>> +#define ISPC_VSFWINT BIT(26)
>> +#define ISPC_VSINT BIT(25)
>> +#define ISPC_INTS BIT(24)
>> +#define ISPC_ENUO BIT(20)
>> +#define ISPC_ENLS BIT(17)
>> +#define ISPC_ENSS1 BIT(12)
>> +#define ISPC_ENSS0 BIT(11)
>> +#define ISPC_RST BIT(1)
>> +#define ISPC_EN BIT(0)
>> +#define ISPC_RST_MASK BIT(1)
>> +
>> +#define ISP_REG_ISP_CTRL_1 0xa08
>> +#define CTRL_SAT(n) ((n) << 28)
>> +#define CTRL_DBC BIT(22)
>> +#define CTRL_CTC BIT(21)
>> +#define CTRL_YHIST BIT(20)
>> +#define CTRL_YCURVE BIT(19)
>> +#define CTRL_CTM BIT(18)
>> +#define CTRL_BIYUV BIT(17)
>> +#define CTRL_SCE BIT(8)
>> +#define CTRL_EE BIT(7)
>> +#define CTRL_CCE BIT(5)
>> +#define CTRL_RGE BIT(4)
>> +#define CTRL_CME BIT(3)
>> +#define CTRL_AE BIT(2)
>> +#define CTRL_CE BIT(1)
>> +#define CTRL_SAT_MASK GENMASK(31, 28)
>> +
>> +#define ISP_REG_PIPELINE_XY_SIZE 0xa0c
>> +#define H_ACT_CAP(n) ((n) << 16)
>> +#define W_ACT_CAP(n) ((n) << 0)
>> +
>> +#define ISP_REG_ICTC 0xa10
>> +#define GF_MODE(n) ((n) << 30)
>> +#define MAXGT(n) ((n) << 16)
>> +#define MINGT(n) ((n) << 0)
>> +
>> +#define ISP_REG_IDBC 0xa14
>> +#define BADGT(n) ((n) << 16)
>> +#define BADXT(n) ((n) << 0)
>> +
>> +#define ISP_REG_ICFAM 0xa1c
>> +#define CROSS_COV(n) ((n) << 4)
>> +#define HV_W(n) ((n) << 0)
>> +
>> +#define ISP_REG_CS_GAIN 0xa30
>> +#define CMAD(n) ((n) << 16)
>> +#define CMAB(n) ((n) << 0)
>> +
>> +#define ISP_REG_CS_THRESHOLD 0xa34
>> +#define CMD(n) ((n) << 16)
>> +#define CMB(n) ((n) << 0)
>> +
>> +#define ISP_REG_CS_OFFSET 0xa38
>> +#define VOFF(n) ((n) << 16)
>> +#define UOFF(n) ((n) << 0)
>> +
>> +#define ISP_REG_CS_HUE_F 0xa3c
>> +#define SIN(n) ((n) << 16)
>> +#define COS(n) ((n) << 0)
>> +
>> +#define ISP_REG_CS_SCALE 0xa40
>> +#define CMSF(n) ((n) << 0)
>> +
>> +#define ISP_REG_IESHD 0xa50
>> +#define SHAD_UP_M BIT(1)
>> +#define SHAD_UP_EN BIT(0)
>> +
>> +#define ISP_REG_YADJ0 0xa54
>> +#define YOIR(n) ((n) << 16)
>> +#define YIMIN(n) ((n) << 0)
>> +
>> +#define ISP_REG_YADJ1 0xa58
>> +#define YOMAX(n) ((n) << 16)
>> +#define YOMIN(n) ((n) << 0)
>> +
>> +#define ISP_REG_Y_PLANE_START_ADDR 0xa80
>> +#define ISP_REG_UV_PLANE_START_ADDR 0xa84
>> +
>> +#define ISP_REG_STRIDE 0xa88
>> +#define IMG_STR(n) ((n) << 0)
>
> For registers that contain a single field at offset 0, you don't need to
> define a macro, you can pass the value directly to the register write
> function (IMG_STR is actually not used :-)). Same above and/or below
> where applicable.
>
>> +
>> +#define ISP_REG_ITIIWSR 0xb20
>> +#define ITI_HSIZE(n) ((n) << 16)
>> +#define ITI_WSIZE(n) ((n) << 0)
>> +
>> +#define ISP_REG_ITIDWLSR 0xb24
>> +#define ITI_WSTRIDE(n) ((n) << 0)
>> +
>> +#define ISP_REG_ITIPDFR 0xb38
>> +#define ITI_PACKAGE_FMT(n) ((n) << 0)
>> +
>> +#define ISP_REG_ITIDRLSR 0xb3C
>> +#define ITI_STRIDE_L(n) ((n) << 0)
>> +
>> +#define ISP_REG_DNYUV_YSWR0 0xc00
>> +#define ISP_REG_DNYUV_YSWR1 0xc04
>> +#define ISP_REG_DNYUV_CSWR0 0xc08
>> +#define ISP_REG_DNYUV_CSWR1 0xc0c
>> +#define YUVSW5(n) ((n) << 20)
>> +#define YUVSW4(n) ((n) << 16)
>> +#define YUVSW3(n) ((n) << 12)
>> +#define YUVSW2(n) ((n) << 8)
>> +#define YUVSW1(n) ((n) << 4)
>> +#define YUVSW0(n) ((n) << 0)
>> +
>> +#define ISP_REG_DNYUV_YDR0 0xc10
>> +#define ISP_REG_DNYUV_YDR1 0xc14
>> +#define ISP_REG_DNYUV_YDR2 0xc18
>> +#define ISP_REG_DNYUV_CDR0 0xc1c
>> +#define ISP_REG_DNYUV_CDR1 0xc20
>> +#define ISP_REG_DNYUV_CDR2 0xc24
>> +#define CURVE_D_H(n) ((n) << 16)
>> +#define CURVE_D_L(n) ((n) << 0)
>> +
>> +#define ISP_REG_ICAMD_0 0xc40
>> +#define DNRM_F(n) ((n) << 16)
>> +#define ISP_REG_ICAMD_12 0xc70
>> +#define ISP_REG_ICAMD_20 0xc90
>> +#define ISP_REG_ICAMD_24 0xca0
>> +#define ISP_REG_ICAMD_25 0xca4
>> +#define CCM_M_DAT(n) ((n) << 0)
>> +
>> +#define ISP_REG_GAMMA_VAL0 0xe00
>> +#define ISP_REG_GAMMA_VAL1 0xe04
>> +#define ISP_REG_GAMMA_VAL2 0xe08
>> +#define ISP_REG_GAMMA_VAL3 0xe0c
>> +#define ISP_REG_GAMMA_VAL4 0xe10
>> +#define ISP_REG_GAMMA_VAL5 0xe14
>> +#define ISP_REG_GAMMA_VAL6 0xe18
>> +#define ISP_REG_GAMMA_VAL7 0xe1c
>> +#define ISP_REG_GAMMA_VAL8 0xe20
>> +#define ISP_REG_GAMMA_VAL9 0xe24
>> +#define ISP_REG_GAMMA_VAL10 0xe28
>> +#define ISP_REG_GAMMA_VAL11 0xe2c
>> +#define ISP_REG_GAMMA_VAL12 0xe30
>> +#define ISP_REG_GAMMA_VAL13 0xe34
>> +#define ISP_REG_GAMMA_VAL14 0xe38
>> +#define GAMMA_S_VAL(n) ((n) << 16)
>> +#define GAMMA_VAL(n) ((n) << 0)
>> +
>> +#define ISP_REG_R2Y_0 0xe40
>> +#define ISP_REG_R2Y_1 0xe44
>> +#define ISP_REG_R2Y_2 0xe48
>> +#define ISP_REG_R2Y_3 0xe4c
>> +#define ISP_REG_R2Y_4 0xe50
>> +#define ISP_REG_R2Y_5 0xe54
>> +#define ISP_REG_R2Y_6 0xe58
>> +#define ISP_REG_R2Y_7 0xe5c
>> +#define ISP_REG_R2Y_8 0xe60
>> +#define CSC_M(n) ((n) << 0)
>> +
>> +#define ISP_REG_SHARPEN0 0xe80
>> +#define ISP_REG_SHARPEN1 0xe84
>> +#define ISP_REG_SHARPEN2 0xe88
>> +#define ISP_REG_SHARPEN3 0xe8c
>> +#define ISP_REG_SHARPEN4 0xe90
>> +#define ISP_REG_SHARPEN5 0xe94
>> +#define ISP_REG_SHARPEN6 0xe98
>> +#define ISP_REG_SHARPEN7 0xe9c
>> +#define ISP_REG_SHARPEN8 0xea0
>> +#define ISP_REG_SHARPEN9 0xea4
>> +#define ISP_REG_SHARPEN10 0xea8
>> +#define ISP_REG_SHARPEN11 0xeac
>> +#define ISP_REG_SHARPEN12 0xeb0
>> +#define ISP_REG_SHARPEN13 0xeb4
>> +#define ISP_REG_SHARPEN14 0xeb8
>> +#define S_DELTA(n) ((n) << 16)
>> +#define S_WEIGHT(n) ((n) << 8)
>> +
>> +#define ISP_REG_SHARPEN_FS0 0xebc
>> +#define ISP_REG_SHARPEN_FS1 0xec0
>> +#define ISP_REG_SHARPEN_FS2 0xec4
>> +#define ISP_REG_SHARPEN_FS3 0xec8
>> +#define ISP_REG_SHARPEN_FS4 0xecc
>> +#define ISP_REG_SHARPEN_FS5 0xed0
>> +#define S_FACTOR(n) ((n) << 24)
>> +#define S_SLOPE(n) ((n) << 0)
>> +
>> +#define ISP_REG_SHARPEN_WN 0xed4
>> +#define PDIRF(n) ((n) << 28)
>> +#define NDIRF(n) ((n) << 24)
>> +#define WSUM(n) ((n) << 0)
>> +
>> +#define ISP_REG_IUVS1 0xed8
>> +#define UVDIFF2(n) ((n) << 16)
>> +#define UVDIFF1(n) ((n) << 0)
>> +
>> +#define ISP_REG_IUVS2 0xedc
>> +#define UVF(n) ((n) << 24)
>> +#define UVSLOPE(n) ((n) << 0)
>> +
>> +#define ISP_REG_IUVCKS1 0xee0
>> +#define UVCKDIFF2(n) ((n) << 16)
>> +#define UVCKDIFF1(n) ((n) << 0)
>> +
>> +#define ISP_REG_IUVCKS2 0xee4
>> +#define UVCKSLOPE(n) ((n) << 0)
>> +
>> +#define ISP_REG_ISHRPET 0xee8
>> +#define TH(n) ((n) << 8)
>> +#define EN(n) ((n) << 0)
>> +
>> +#define ISP_REG_YCURVE_0 0xf00
>> +#define ISP_REG_YCURVE_63 0xffc
>> +#define L_PARAM(n) ((n) << 0)
>> +
>> +#define IMAGE_MAX_WIDTH 1920
>> +#define IMAGE_MAX_HEIGH 1080
>> +
>> +/* The output line of ISP */
>
> What is an ISP "line" ?
>
A pipeline contains ISP.
>> +enum isp_line_id {
>> + STF_ISP_LINE_INVALID = -1,
>> + STF_ISP_LINE_SRC = 1,
>> + STF_ISP_LINE_MAX = STF_ISP_LINE_SRC
>> +};
>> +
>> +/* pad id for media framework */
>> +enum isp_pad_id {
>> + STF_ISP_PAD_SINK = 0,
>> + STF_ISP_PAD_SRC,
>> + STF_ISP_PAD_MAX
>> +};
>> +
>> +enum {
>> + EN_INT_NONE = 0,
>> + EN_INT_ISP_DONE = (0x1 << 24),
>> + EN_INT_CSI_DONE = (0x1 << 25),
>> + EN_INT_SC_DONE = (0x1 << 26),
>> + EN_INT_LINE_INT = (0x1 << 27),
>> + EN_INT_ALL = (0xF << 24),
>
> Please use lowercase for hex constants.
>
> But I'm wondering if this is really needed, it seems to duplicate the
> ISPC_* bits defined above. I would use those directly.
>
>> +};
>> +
>> +enum {
>> + INTERFACE_DVP = 0,
>> + INTERFACE_CSI,
>> +};
>
> This isn't used.
>
>> +
>> +struct isp_format {
>> + u32 code;
>> + u8 bpp;
>> +};
>> +
>> +struct isp_format_table {
>> + const struct isp_format *fmts;
>> + int nfmts;
>> +};
>> +
>> +struct regval_t {
>> + u32 addr;
>> + u32 val;
>> + u32 delay_ms;
>> +};
>> +
>> +struct reg_table {
>> + const struct regval_t *regval;
>> + int regval_num;
>> +};
>
> Those two structures are not used.
>
>> +
>> +struct stf_isp_crop {
>> + struct v4l2_rect rect;
>> + u32 bpp;
>> +};
>> +
>> +struct stf_isp_dev {
>> + struct stfcamss *stfcamss;
>> + struct v4l2_subdev subdev;
>> + struct media_pad pads[STF_ISP_PAD_MAX];
>> + struct v4l2_mbus_framefmt fmt[STF_ISP_PAD_MAX];
>> + struct stf_isp_crop rect[STF_ISP_PAD_MAX];
>> + const struct isp_format_table *formats;
>> + unsigned int nformats;
>> + struct mutex stream_lock; /* serialize stream control */
>
> You should explicitly list the fields protected by the mutex.
>
>> + int stream_count;
>> +};
>> +
>> +int stf_isp_clk_enable(struct stf_isp_dev *isp_dev);
>> +int stf_isp_clk_disable(struct stf_isp_dev *isp_dev);
>> +int stf_isp_reset(struct stf_isp_dev *isp_dev);
>> +void stf_isp_init_cfg(struct stf_isp_dev *isp_dev);
>> +void stf_isp_settings(struct stf_isp_dev *isp_dev,
>> + struct stf_isp_crop *crop_array, u32 mcode);
>> +void stf_isp_stream_set(struct stf_isp_dev *isp_dev);
>> +int stf_isp_subdev_init(struct stfcamss *stfcamss);
>> +int stf_isp_register(struct stf_isp_dev *isp_dev, struct v4l2_device *v4l2_dev);
>> +int stf_isp_unregister(struct stf_isp_dev *isp_dev);
>> +
>> +#endif /* STF_ISP_H */
>> diff --git a/drivers/media/platform/starfive/camss/stf_isp_hw_ops.c b/drivers/media/platform/starfive/camss/stf_isp_hw_ops.c
>> new file mode 100644
>> index 000000000000..2088b6bd0d61
>> --- /dev/null
>> +++ b/drivers/media/platform/starfive/camss/stf_isp_hw_ops.c
>> @@ -0,0 +1,468 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * stf_isp_hw_ops.c
>> + *
>> + * Register interface file for StarFive ISP driver
>> + *
>> + * Copyright (C) 2021-2023 StarFive Technology Co., Ltd.
>> + *
>> + */
>> +
>> +#include "stf_camss.h"
>> +
>> +static void stf_isp_config_obc(struct stfcamss *stfcamss)
>> +{
>> + u32 reg_val, reg_add;
>> +
>> + stf_isp_reg_write(stfcamss, ISP_REG_OBC_CFG, OBC_W_H(11) | OBC_W_W(11));
>> +
>> + reg_val = GAIN_D_POINT(0x40) | GAIN_C_POINT(0x40) |
>> + GAIN_B_POINT(0x40) | GAIN_A_POINT(0x40);
>> + for (reg_add = ISP_REG_OBCG_CFG_0; reg_add <= ISP_REG_OBCG_CFG_3;) {
>> + stf_isp_reg_write(stfcamss, reg_add, reg_val);
>> + reg_add += 4;
>> + }
>> +
>> + reg_val = OFFSET_D_POINT(0) | OFFSET_C_POINT(0) |
>> + OFFSET_B_POINT(0) | OFFSET_A_POINT(0);
>> + for (reg_add = ISP_REG_OBCO_CFG_0; reg_add <= ISP_REG_OBCO_CFG_3;) {
>> + stf_isp_reg_write(stfcamss, reg_add, reg_val);
>> + reg_add += 4;
>> + }
>> +}
>> +
>> +static void stf_isp_config_oecf(struct stfcamss *stfcamss)
>> +{
>> + u32 reg_add, par_val;
>> + u16 par_h, par_l;
>> +
>> + par_h = 0x10; par_l = 0;
>> + par_val = OCEF_PAR_H(par_h) | OCEF_PAR_L(par_l);
>> + for (reg_add = ISP_REG_OECF_X0_CFG0; reg_add <= ISP_REG_OECF_Y3_CFG0;) {
>> + stf_isp_reg_write(stfcamss, reg_add, par_val);
>> + reg_add += 0x20;
>> + }
>> +
>> + par_h = 0x40; par_l = 0x20;
>> + par_val = OCEF_PAR_H(par_h) | OCEF_PAR_L(par_l);
>> + for (reg_add = ISP_REG_OECF_X0_CFG1; reg_add <= ISP_REG_OECF_Y3_CFG1;) {
>> + stf_isp_reg_write(stfcamss, reg_add, par_val);
>> + reg_add += 0x20;
>> + }
>> +
>> + par_h = 0x80; par_l = 0x60;
>> + par_val = OCEF_PAR_H(par_h) | OCEF_PAR_L(par_l);
>> + for (reg_add = ISP_REG_OECF_X0_CFG2; reg_add <= ISP_REG_OECF_Y3_CFG2;) {
>> + stf_isp_reg_write(stfcamss, reg_add, par_val);
>> + reg_add += 0x20;
>> + }
>> +
>> + par_h = 0xc0; par_l = 0xa0;
>> + par_val = OCEF_PAR_H(par_h) | OCEF_PAR_L(par_l);
>> + for (reg_add = ISP_REG_OECF_X0_CFG3; reg_add <= ISP_REG_OECF_Y3_CFG3;) {
>> + stf_isp_reg_write(stfcamss, reg_add, par_val);
>> + reg_add += 0x20;
>> + }
>> +
>> + par_h = 0x100; par_l = 0xe0;
>> + par_val = OCEF_PAR_H(par_h) | OCEF_PAR_L(par_l);
>> + for (reg_add = ISP_REG_OECF_X0_CFG4; reg_add <= ISP_REG_OECF_Y3_CFG4;) {
>> + stf_isp_reg_write(stfcamss, reg_add, par_val);
>> + reg_add += 0x20;
>> + }
>> +
>> + par_h = 0x200; par_l = 0x180;
>> + par_val = OCEF_PAR_H(par_h) | OCEF_PAR_L(par_l);
>> + for (reg_add = ISP_REG_OECF_X0_CFG5; reg_add <= ISP_REG_OECF_Y3_CFG5;) {
>> + stf_isp_reg_write(stfcamss, reg_add, par_val);
>> + reg_add += 0x20;
>> + }
>> +
>> + par_h = 0x300; par_l = 0x280;
>> + par_val = OCEF_PAR_H(par_h) | OCEF_PAR_L(par_l);
>> + for (reg_add = ISP_REG_OECF_X0_CFG6; reg_add <= ISP_REG_OECF_Y3_CFG6;) {
>> + stf_isp_reg_write(stfcamss, reg_add, par_val);
>> + reg_add += 0x20;
>> + }
>> +
>> + par_h = 0x3fe; par_l = 0x380;
>> + par_val = OCEF_PAR_H(par_h) | OCEF_PAR_L(par_l);
>> + for (reg_add = ISP_REG_OECF_X0_CFG7; reg_add <= ISP_REG_OECF_Y3_CFG7;) {
>> + stf_isp_reg_write(stfcamss, reg_add, par_val);
>> + reg_add += 0x20;
>> + }
>> +
>> + par_h = 0x80; par_l = 0x80;
>> + par_val = OCEF_PAR_H(par_h) | OCEF_PAR_L(par_l);
>> + for (reg_add = ISP_REG_OECF_S0_CFG0; reg_add <= ISP_REG_OECF_S3_CFG7;) {
>> + stf_isp_reg_write(stfcamss, reg_add, par_val);
>> + reg_add += 4;
>> + }
>> +}
>> +
>> +static void stf_isp_config_lccf(struct stfcamss *stfcamss)
>> +{
>> + u32 reg_add;
>> +
>> + stf_isp_reg_write(stfcamss, ISP_REG_LCCF_CFG_0,
>> + Y_DISTANCE(0x21C) | X_DISTANCE(0x3C0));
>> + stf_isp_reg_write(stfcamss, ISP_REG_LCCF_CFG_1, LCCF_MAX_DIS(0xb));
>> +
>> + for (reg_add = ISP_REG_LCCF_CFG_2; reg_add <= ISP_REG_LCCF_CFG_5;) {
>> + stf_isp_reg_write(stfcamss, reg_add,
>> + LCCF_F2_PAR(0x0) | LCCF_F1_PAR(0x0));
>> + reg_add += 4;
>> + }
>> +}
>> +
>> +static void stf_isp_config_awb(struct stfcamss *stfcamss)
>> +{
>> + u32 reg_val, reg_add;
>> + u16 symbol_h, symbol_l;
>> +
>> + symbol_h = 0x0; symbol_l = 0x0;
>> + reg_val = AWB_X_SYMBOL_H(symbol_h) | AWB_X_SYMBOL_L(symbol_l);
>> +
>> + for (reg_add = ISP_REG_AWB_X0_CFG_0; reg_add <= ISP_REG_AWB_X3_CFG_1;) {
>> + stf_isp_reg_write(stfcamss, reg_add, reg_val);
>> + reg_add += 4;
>> + }
>> +
>> + symbol_h = 0x0, symbol_l = 0x0;
>> + reg_val = AWB_Y_SYMBOL_H(symbol_h) | AWB_Y_SYMBOL_L(symbol_l);
>> +
>> + for (reg_add = ISP_REG_AWB_Y0_CFG_0; reg_add <= ISP_REG_AWB_Y3_CFG_1;) {
>> + stf_isp_reg_write(stfcamss, reg_add, reg_val);
>> + reg_add += 4;
>> + }
>> +
>> + symbol_h = 0x80, symbol_l = 0x80;
>> + reg_val = AWB_S_SYMBOL_H(symbol_h) | AWB_S_SYMBOL_L(symbol_l);
>> +
>> + for (reg_add = ISP_REG_AWB_S0_CFG_0; reg_add <= ISP_REG_AWB_S3_CFG_1;) {
>> + stf_isp_reg_write(stfcamss, reg_add, reg_val);
>> + reg_add += 4;
>> + }
>> +}
>> +
>> +static void stf_isp_config_grgb(struct stfcamss *stfcamss)
>> +{
>> + stf_isp_reg_write(stfcamss, ISP_REG_ICTC,
>> + GF_MODE(1) | MAXGT(0x140) | MINGT(0x40));
>> + stf_isp_reg_write(stfcamss, ISP_REG_IDBC, BADGT(0x200) | BADXT(0x200));
>> +}
>> +
>> +static void stf_isp_config_cfa(struct stfcamss *stfcamss)
>> +{
>> + stf_isp_reg_write(stfcamss, ISP_REG_RAW_FORMAT_CFG,
>> + SMY13(0) | SMY12(1) | SMY11(0) | SMY10(1) | SMY3(2) |
>> + SMY2(3) | SMY1(2) | SMY0(3));
>> + stf_isp_reg_write(stfcamss, ISP_REG_ICFAM, CROSS_COV(3) | HV_W(2));
>> +}
>> +
>> +static void stf_isp_config_ccm(struct stfcamss *stfcamss)
>> +{
>> + u32 reg_add;
>> +
>> + stf_isp_reg_write(stfcamss, ISP_REG_ICAMD_0, DNRM_F(6) | CCM_M_DAT(0));
>> +
>> + for (reg_add = ISP_REG_ICAMD_12; reg_add <= ISP_REG_ICAMD_20;) {
>> + stf_isp_reg_write(stfcamss, reg_add, CCM_M_DAT(0x80));
>> + reg_add += 0x10;
>> + }
>> +
>> + stf_isp_reg_write(stfcamss, ISP_REG_ICAMD_24, CCM_M_DAT(0x700));
>> + stf_isp_reg_write(stfcamss, ISP_REG_ICAMD_25, CCM_M_DAT(0x200));
>> +}
>> +
>> +static void stf_isp_config_gamma(struct stfcamss *stfcamss)
>> +{
>> + u32 reg_val, reg_add;
>> + u16 gamma_slope_v, gamma_v;
>> +
>> + gamma_slope_v = 0x2400; gamma_v = 0x0;
>> + reg_val = GAMMA_S_VAL(gamma_slope_v) | GAMMA_VAL(gamma_v);
>> + stf_isp_reg_write(stfcamss, ISP_REG_GAMMA_VAL0, reg_val);
>> +
>> + gamma_slope_v = 0x800; gamma_v = 0x20;
>> + for (reg_add = ISP_REG_GAMMA_VAL1; reg_add <= ISP_REG_GAMMA_VAL7;) {
>> + reg_val = GAMMA_S_VAL(gamma_slope_v) | GAMMA_VAL(gamma_v);
>> + stf_isp_reg_write(stfcamss, reg_add, reg_val);
>> + reg_add += 4;
>> + gamma_v += 0x20;
>> + }
>> +
>> + gamma_v = 0x100;
>> + for (reg_add = ISP_REG_GAMMA_VAL8; reg_add <= ISP_REG_GAMMA_VAL13;) {
>> + reg_val = GAMMA_S_VAL(gamma_slope_v) | GAMMA_VAL(gamma_v);
>> + stf_isp_reg_write(stfcamss, reg_add, reg_val);
>> + reg_add += 4;
>> + gamma_v += 0x80;
>> + }
>> +
>> + gamma_v = 0x3fe;
>> + reg_val = GAMMA_S_VAL(gamma_slope_v) | GAMMA_VAL(gamma_v);
>> + stf_isp_reg_write(stfcamss, ISP_REG_GAMMA_VAL14, reg_val);
>> +}
>> +
>> +static void stf_isp_config_r2y(struct stfcamss *stfcamss)
>> +{
>> + stf_isp_reg_write(stfcamss, ISP_REG_R2Y_0, CSC_M(0x4C));
>> + stf_isp_reg_write(stfcamss, ISP_REG_R2Y_1, CSC_M(0x97));
>> + stf_isp_reg_write(stfcamss, ISP_REG_R2Y_2, CSC_M(0x1d));
>> + stf_isp_reg_write(stfcamss, ISP_REG_R2Y_3, CSC_M(0x1d5));
>> + stf_isp_reg_write(stfcamss, ISP_REG_R2Y_4, CSC_M(0x1ac));
>> + stf_isp_reg_write(stfcamss, ISP_REG_R2Y_5, CSC_M(0x80));
>> + stf_isp_reg_write(stfcamss, ISP_REG_R2Y_6, CSC_M(0x80));
>> + stf_isp_reg_write(stfcamss, ISP_REG_R2Y_7, CSC_M(0x194));
>> + stf_isp_reg_write(stfcamss, ISP_REG_R2Y_8, CSC_M(0x1ec));
>> +}
>> +
>> +static void stf_isp_config_y_curve(struct stfcamss *stfcamss)
>> +{
>> + u32 reg_add;
>> + u16 y_curve;
>> +
>> + y_curve = 0x0;
>> + for (reg_add = ISP_REG_YCURVE_0; reg_add <= ISP_REG_YCURVE_63;) {
>> + stf_isp_reg_write(stfcamss, reg_add, y_curve);
>> + reg_add += 4;
>> + y_curve += 0x10;
>> + }
>> +}
>> +
>> +static void stf_isp_config_sharpen(struct stfcamss *sc)
>> +{
>> + u32 reg_add;
>> +
>> + stf_isp_reg_write(sc, ISP_REG_SHARPEN0, S_DELTA(0x7) | S_WEIGHT(0xf));
>> + stf_isp_reg_write(sc, ISP_REG_SHARPEN1, S_DELTA(0x18) | S_WEIGHT(0xf));
>> + stf_isp_reg_write(sc, ISP_REG_SHARPEN2, S_DELTA(0x80) | S_WEIGHT(0xf));
>> + stf_isp_reg_write(sc, ISP_REG_SHARPEN3, S_DELTA(0x100) | S_WEIGHT(0xf));
>> + stf_isp_reg_write(sc, ISP_REG_SHARPEN4, S_DELTA(0x10) | S_WEIGHT(0xf));
>> + stf_isp_reg_write(sc, ISP_REG_SHARPEN5, S_DELTA(0x60) | S_WEIGHT(0xf));
>> + stf_isp_reg_write(sc, ISP_REG_SHARPEN6, S_DELTA(0x100) | S_WEIGHT(0xf));
>> + stf_isp_reg_write(sc, ISP_REG_SHARPEN7, S_DELTA(0x190) | S_WEIGHT(0xf));
>> + stf_isp_reg_write(sc, ISP_REG_SHARPEN8, S_DELTA(0x0) | S_WEIGHT(0xf));
>> +
>> + for (reg_add = ISP_REG_SHARPEN9; reg_add <= ISP_REG_SHARPEN14;) {
>> + stf_isp_reg_write(sc, reg_add, S_WEIGHT(0xf));
>> + reg_add += 4;
>> + }
>> +
>> + for (reg_add = ISP_REG_SHARPEN_FS0; reg_add <= ISP_REG_SHARPEN_FS5;) {
>> + stf_isp_reg_write(sc, reg_add, S_FACTOR(0x10) | S_SLOPE(0x0));
>> + reg_add += 4;
>> + }
>> +
>> + stf_isp_reg_write(sc, ISP_REG_SHARPEN_WN,
>> + PDIRF(0x8) | NDIRF(0x8) | WSUM(0xd7c));
>> + stf_isp_reg_write(sc, ISP_REG_IUVS1, UVDIFF2(0xC0) | UVDIFF1(0x40));
>> + stf_isp_reg_write(sc, ISP_REG_IUVS2, UVF(0xff) | UVSLOPE(0x0));
>> + stf_isp_reg_write(sc, ISP_REG_IUVCKS1,
>> + UVCKDIFF2(0xa0) | UVCKDIFF1(0x40));
>> +}
>> +
>> +static void stf_isp_config_dnyuv(struct stfcamss *stfcamss)
>> +{
>> + u32 reg_val;
>> +
>> + reg_val = YUVSW5(7) | YUVSW4(7) | YUVSW3(7) | YUVSW2(7) |
>> + YUVSW1(7) | YUVSW0(7);
>> + stf_isp_reg_write(stfcamss, ISP_REG_DNYUV_YSWR0, reg_val);
>> + stf_isp_reg_write(stfcamss, ISP_REG_DNYUV_CSWR0, reg_val);
>> +
>> + reg_val = YUVSW3(7) | YUVSW2(7) | YUVSW1(7) | YUVSW0(7);
>> + stf_isp_reg_write(stfcamss, ISP_REG_DNYUV_YSWR1, reg_val);
>> + stf_isp_reg_write(stfcamss, ISP_REG_DNYUV_CSWR1, reg_val);
>> +
>> + reg_val = CURVE_D_H(0x60) | CURVE_D_L(0x40);
>> + stf_isp_reg_write(stfcamss, ISP_REG_DNYUV_YDR0, reg_val);
>> + stf_isp_reg_write(stfcamss, ISP_REG_DNYUV_CDR0, reg_val);
>> +
>> + reg_val = CURVE_D_H(0xd8) | CURVE_D_L(0x90);
>> + stf_isp_reg_write(stfcamss, ISP_REG_DNYUV_YDR1, reg_val);
>> + stf_isp_reg_write(stfcamss, ISP_REG_DNYUV_CDR1, reg_val);
>> +
>> + reg_val = CURVE_D_H(0x1e6) | CURVE_D_L(0x144);
>> + stf_isp_reg_write(stfcamss, ISP_REG_DNYUV_YDR2, reg_val);
>> + stf_isp_reg_write(stfcamss, ISP_REG_DNYUV_CDR2, reg_val);
>> +}
>> +
>> +static void stf_isp_config_sat(struct stfcamss *stfcamss)
>> +{
>> + stf_isp_reg_write(stfcamss, ISP_REG_CS_GAIN, CMAD(0x0) | CMAB(0x100));
>> + stf_isp_reg_write(stfcamss, ISP_REG_CS_THRESHOLD, CMD(0x1f) | CMB(0x1));
>> + stf_isp_reg_write(stfcamss, ISP_REG_CS_OFFSET, VOFF(0x0) | UOFF(0x0));
>> + stf_isp_reg_write(stfcamss, ISP_REG_CS_HUE_F, SIN(0x0) | COS(0x100));
>> + stf_isp_reg_write(stfcamss, ISP_REG_CS_SCALE, CMSF(0x8));
>> + stf_isp_reg_write(stfcamss, ISP_REG_YADJ0, YOIR(0x401) | YIMIN(0x1));
>> + stf_isp_reg_write(stfcamss, ISP_REG_YADJ1, YOMAX(0x3ff) | YOMIN(0x1));
>> +}
>> +
>> +int stf_isp_clk_enable(struct stf_isp_dev *isp_dev)
>> +{
>> + struct stfcamss *stfcamss = isp_dev->stfcamss;
>> +
>> + clk_prepare_enable(stfcamss->sys_clk[STF_CLK_WRAPPER_CLK_C].clk);
>> + reset_control_deassert(stfcamss->sys_rst[STF_RST_WRAPPER_C].rstc);
>> + reset_control_deassert(stfcamss->sys_rst[STF_RST_WRAPPER_P].rstc);
>
> Missing error handling.
>
> Is there a specific hardware requirement to split the clock enabling and
> reset deassertion in two, with some of them handled in the runtime PM
> resume handler and the rest here ? If not, moving all clock and reset
> control to the runtime PM handlers would simplify the code.
>
>> +
>> + return 0;
>> +}
>> +
>> +int stf_isp_clk_disable(struct stf_isp_dev *isp_dev)
>> +{
>> + struct stfcamss *stfcamss = isp_dev->stfcamss;
>> +
>> + reset_control_assert(stfcamss->sys_rst[STF_RST_WRAPPER_C].rstc);
>> + reset_control_assert(stfcamss->sys_rst[STF_RST_WRAPPER_P].rstc);
>> + clk_disable_unprepare(stfcamss->sys_clk[STF_CLK_WRAPPER_CLK_C].clk);
>> +
>> + return 0;
>> +}
>> +
>> +int stf_isp_reset(struct stf_isp_dev *isp_dev)
>> +{
>> + stf_isp_reg_set_bit(isp_dev->stfcamss, ISP_REG_ISP_CTRL_0,
>> + ISPC_RST_MASK, ISPC_RST);
>> + stf_isp_reg_set_bit(isp_dev->stfcamss, ISP_REG_ISP_CTRL_0,
>> + ISPC_RST_MASK, 0);
>> +
>> + return 0;
>> +}
>> +
>> +void stf_isp_init_cfg(struct stf_isp_dev *isp_dev)
>> +{
>> + stf_isp_reg_write(isp_dev->stfcamss, ISP_REG_DC_CFG_1, DC_AXI_ID(0x0));
>> + stf_isp_reg_write(isp_dev->stfcamss, ISP_REG_DEC_CFG,
>> + DEC_V_KEEP(0x0) |
>> + DEC_V_PERIOD(0x0) |
>> + DEC_H_KEEP(0x0) |
>> + DEC_H_PERIOD(0x0));
>> +
>> + stf_isp_config_obc(isp_dev->stfcamss);
>> + stf_isp_config_oecf(isp_dev->stfcamss);
>> + stf_isp_config_lccf(isp_dev->stfcamss);
>> + stf_isp_config_awb(isp_dev->stfcamss);
>> + stf_isp_config_grgb(isp_dev->stfcamss);
>> + stf_isp_config_cfa(isp_dev->stfcamss);
>> + stf_isp_config_ccm(isp_dev->stfcamss);
>> + stf_isp_config_gamma(isp_dev->stfcamss);
>> + stf_isp_config_r2y(isp_dev->stfcamss);
>> + stf_isp_config_y_curve(isp_dev->stfcamss);
>> + stf_isp_config_sharpen(isp_dev->stfcamss);
>> + stf_isp_config_dnyuv(isp_dev->stfcamss);
>> + stf_isp_config_sat(isp_dev->stfcamss);
>
> All these parameters are hardcoded, why are they not exposed to
> userspace ?
>
Here is a basic startup configuration for the ISP registers. The
function name is confusing, as if it is configuring a specific
function. In fact, it is just a basic init configuration.
>> +
>> + stf_isp_reg_write(isp_dev->stfcamss, ISP_REG_CSI_MODULE_CFG,
>> + CSI_DUMP_EN | CSI_SC_EN | CSI_AWB_EN |
>> + CSI_LCCF_EN | CSI_OECF_EN | CSI_OBC_EN | CSI_DEC_EN);
>> + stf_isp_reg_write(isp_dev->stfcamss, ISP_REG_ISP_CTRL_1,
>> + CTRL_SAT(1) | CTRL_DBC | CTRL_CTC | CTRL_YHIST |
>> + CTRL_YCURVE | CTRL_BIYUV | CTRL_SCE | CTRL_EE |
>> + CTRL_CCE | CTRL_RGE | CTRL_CME | CTRL_AE | CTRL_CE);
>> +}
>> +
>> +static void stf_isp_config_crop(struct stfcamss *stfcamss,
>> + struct stf_isp_crop *crop)
>> +{
>> + struct v4l2_rect *rect = &crop[STF_ISP_PAD_SRC].rect;
>> + u32 bpp = crop[STF_ISP_PAD_SRC].bpp;
>> + u32 val;
>> +
>> + val = VSTART_CAP(rect->top) | HSTART_CAP(rect->left);
>> + stf_isp_reg_write(stfcamss, ISP_REG_PIC_CAPTURE_START_CFG, val);
>> +
>> + val = VEND_CAP(rect->height + rect->top - 1) |
>> + HEND_CAP(rect->width + rect->left - 1);
>> + stf_isp_reg_write(stfcamss, ISP_REG_PIC_CAPTURE_END_CFG, val);
>> +
>> + val = H_ACT_CAP(rect->height) | W_ACT_CAP(rect->width);
>> + stf_isp_reg_write(stfcamss, ISP_REG_PIPELINE_XY_SIZE, val);
>> +
>> + val = ALIGN(rect->width * bpp / 8, STFCAMSS_FRAME_WIDTH_ALIGN_8);
>> + stf_isp_reg_write(stfcamss, ISP_REG_STRIDE, val);
>> +}
>> +
>> +static void stf_isp_config_raw_fmt(struct stfcamss *stfcamss, u32 mcode)
>> +{
>> + u32 val, val1;
>> +
>> + switch (mcode) {
>> + case MEDIA_BUS_FMT_SRGGB10_1X10:
>> + case MEDIA_BUS_FMT_SRGGB8_1X8:
>> + /* 3 2 3 2 1 0 1 0 B Gb B Gb Gr R Gr R */
>> + val = SMY13(3) | SMY12(2) | SMY11(3) | SMY10(2) |
>> + SMY3(1) | SMY2(0) | SMY1(1) | SMY0(0);
>> + val1 = CTRL_SAT(0x0);
>> + break;
>> + case MEDIA_BUS_FMT_SGRBG10_1X10:
>> + case MEDIA_BUS_FMT_SGRBG8_1X8:
>> + /* 2 3 2 3 0 1 0 1, Gb B Gb B R Gr R Gr */
>> + val = SMY13(2) | SMY12(3) | SMY11(2) | SMY10(3) |
>> + SMY3(0) | SMY2(1) | SMY1(0) | SMY0(1);
>> + val1 = CTRL_SAT(0x2);
>> + break;
>> + case MEDIA_BUS_FMT_SGBRG10_1X10:
>> + case MEDIA_BUS_FMT_SGBRG8_1X8:
>> + /* 1 0 1 0 3 2 3 2, Gr R Gr R B Gb B Gb */
>> + val = SMY13(1) | SMY12(0) | SMY11(1) | SMY10(0) |
>> + SMY3(3) | SMY2(2) | SMY1(3) | SMY0(2);
>> + val1 = CTRL_SAT(0x3);
>> + break;
>> + case MEDIA_BUS_FMT_SBGGR10_1X10:
>> + case MEDIA_BUS_FMT_SBGGR8_1X8:
>> + /* 0 1 0 1 2 3 2 3 R Gr R Gr Gb B Gb B */
>> + val = SMY13(0) | SMY12(1) | SMY11(0) | SMY10(1) |
>> + SMY3(2) | SMY2(3) | SMY1(2) | SMY0(3);
>> + val1 = CTRL_SAT(0x1);
>> + break;
>> + default:
>> + val = SMY13(0) | SMY12(1) | SMY11(0) | SMY10(1) |
>> + SMY3(2) | SMY2(3) | SMY1(2) | SMY0(3);
>> + val1 = CTRL_SAT(0x1);
>> + break;
>> + }
>> + stf_isp_reg_write(stfcamss, ISP_REG_RAW_FORMAT_CFG, val);
>> + stf_isp_reg_set_bit(stfcamss, ISP_REG_ISP_CTRL_1, CTRL_SAT_MASK, val1);
>> +}
>> +
>> +void stf_isp_settings(struct stf_isp_dev *isp_dev,
>> + struct stf_isp_crop *crop, u32 mcode)
>> +{
>> + struct stfcamss *stfcamss = isp_dev->stfcamss;
>> +
>> + stf_isp_config_crop(stfcamss, crop);
>> + stf_isp_config_raw_fmt(stfcamss, mcode);
>> +
>> + stf_isp_reg_set_bit(stfcamss, ISP_REG_DUMP_CFG_1,
>> + DUMP_BURST_LEN_MASK | DUMP_SD_MASK,
>> + DUMP_BURST_LEN(3));
>> +
>> + stf_isp_reg_write(stfcamss, ISP_REG_ITIIWSR,
>> + ITI_HSIZE(IMAGE_MAX_HEIGH) |
>> + ITI_WSIZE(IMAGE_MAX_WIDTH));
>> + stf_isp_reg_write(stfcamss, ISP_REG_ITIDWLSR, ITI_WSTRIDE(0x960));
>> + stf_isp_reg_write(stfcamss, ISP_REG_ITIDRLSR, ITI_STRIDE_L(0x960));
>> + stf_isp_reg_write(stfcamss, ISP_REG_SENSOR, 0x1);
>
> Should 0x1 be IMAGER_SEL(1) or are those unrelated ?
>
>> +}
>> +
>> +void stf_isp_stream_set(struct stf_isp_dev *isp_dev)
>> +{
>> + struct stfcamss *stfcamss = isp_dev->stfcamss;
>> +
>> + stf_isp_reg_write_delay(stfcamss, ISP_REG_ISP_CTRL_0,
>> + ISPC_ENUO | ISPC_ENLS | ISPC_RST, 10);
>> + stf_isp_reg_write_delay(stfcamss, ISP_REG_ISP_CTRL_0,
>> + ISPC_ENUO | ISPC_ENLS, 10);
>> + stf_isp_reg_write(stfcamss, ISP_REG_IESHD, SHAD_UP_M);
>> + stf_isp_reg_write_delay(stfcamss, ISP_REG_ISP_CTRL_0,
>> + ISPC_ENUO | ISPC_ENLS | ISPC_EN, 10);
>> + stf_isp_reg_write_delay(stfcamss, ISP_REG_CSIINTS,
>> + CSI_INTS(1) | CSI_SHA_M(4), 10);
>> + stf_isp_reg_write_delay(stfcamss, ISP_REG_CSIINTS,
>> + CSI_INTS(2) | CSI_SHA_M(4), 10);
>> + stf_isp_reg_write_delay(stfcamss, ISP_REG_CSI_INPUT_EN_AND_STATUS,
>> + CSI_EN_S, 10);
>> +}
>
Regards,
Jack Zhu
Powered by blists - more mailing lists