| 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: <7a05c500-b46d-e7c9-25d6-f787c01352bf@microchip.com>
Date: Tue, 11 Jan 2022 17:00:28 +0000
From: <Eugen.Hristev@...rochip.com>
To: <jacopo@...ndi.org>
CC: <linux-media@...r.kernel.org>, <robh+dt@...nel.org>,
<laurent.pinchart@...asonboard.com>, <sakari.ailus@....fi>,
<devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
<linux-arm-kernel@...ts.infradead.org>,
<Nicolas.Ferre@...rochip.com>
Subject: Re: [PATCH v3 12/23] media: atmel: atmel-isc: implement media
controller
On 1/11/22 6:51 PM, Jacopo Mondi wrote:
> On Tue, Jan 11, 2022 at 04:30:15PM +0000, Eugen.Hristev@...rochip.com wrote:
>> On 1/11/22 6:06 PM, Jacopo Mondi wrote:
>>> Hi Eugen,
>>>
>>> On Mon, Dec 13, 2021 at 03:49:29PM +0200, Eugen Hristev wrote:
>>>> Implement the support for media-controller.
>>>> This means that the capabilities of the driver have changed and now
>>>> it also advertises the IO_MC .
>>>> The driver will register it's media device, and add the video entity to this
>>>> media device. The subdevices are registered to the same media device.
>>>> The ISC will have a base entity which is auto-detected as atmel_isc_base.
>>>> It will also register a subdevice that allows cropping of the incoming frame
>>>> to the maximum frame size supported by the ISC.
>>>> The ISC will create a link between the subdevice that is asynchronously
>>>> registered and the atmel_isc_scaler entity.
>>>> Then, the atmel_isc_scaler and atmel_isc_base are connected through another
>>>> link.
>>>>
>>>> Signed-off-by: Eugen Hristev <eugen.hristev@...rochip.com>
>>>> ---
>>>> Changes in v2:
>>>> - implement try formats
>>>>
>>>> drivers/media/platform/atmel/Makefile | 2 +-
>>>> drivers/media/platform/atmel/atmel-isc-base.c | 27 +-
>>>> drivers/media/platform/atmel/atmel-isc-mc.c | 287 ++++++++++++++++++
>>>> drivers/media/platform/atmel/atmel-isc.h | 36 +++
>>>> .../media/platform/atmel/atmel-sama5d2-isc.c | 14 +-
>>>> .../media/platform/atmel/atmel-sama7g5-isc.c | 12 +-
>>>> 6 files changed, 370 insertions(+), 8 deletions(-)
>>>> create mode 100644 drivers/media/platform/atmel/atmel-isc-mc.c
>>>>
>>>> diff --git a/drivers/media/platform/atmel/Makefile b/drivers/media/platform/atmel/Makefile
>>>> index 794e8f739287..aa7a18b428f6 100644
>>>> --- a/drivers/media/platform/atmel/Makefile
>>>> +++ b/drivers/media/platform/atmel/Makefile
>>>> @@ -1,7 +1,7 @@
>>>> # SPDX-License-Identifier: GPL-2.0-only
>>>> atmel-isc-objs = atmel-sama5d2-isc.o
>>>> atmel-xisc-objs = atmel-sama7g5-isc.o
>>>> -atmel-isc-common-objs = atmel-isc-base.o atmel-isc-clk.o
>>>> +atmel-isc-common-objs = atmel-isc-base.o atmel-isc-clk.o atmel-isc-mc.o
>>>>
>>>> obj-$(CONFIG_VIDEO_ATMEL_ISI) += atmel-isi.o
>>>> obj-$(CONFIG_VIDEO_ATMEL_ISC_BASE) += atmel-isc-common.o
>>>> diff --git a/drivers/media/platform/atmel/atmel-isc-base.c b/drivers/media/platform/atmel/atmel-isc-base.c
>>>> index d0542b97a391..b0c3ed21f372 100644
>>>> --- a/drivers/media/platform/atmel/atmel-isc-base.c
>>>> +++ b/drivers/media/platform/atmel/atmel-isc-base.c
>>>> @@ -1726,6 +1726,7 @@ static int isc_async_bound(struct v4l2_async_notifier *notifier,
>>>> struct isc_device, v4l2_dev);
>>>> struct isc_subdev_entity *subdev_entity =
>>>> container_of(notifier, struct isc_subdev_entity, notifier);
>>>> + int pad;
>>>>
>>>> if (video_is_registered(&isc->video_dev)) {
>>>> v4l2_err(&isc->v4l2_dev, "only supports one sub-device.\n");
>>>> @@ -1734,6 +1735,16 @@ static int isc_async_bound(struct v4l2_async_notifier *notifier,
>>>>
>>>> subdev_entity->sd = subdev;
>>>>
>>>> + pad = media_entity_get_fwnode_pad(&subdev->entity, asd->match.fwnode,
>>>> + MEDIA_PAD_FL_SOURCE);
>>>> + if (pad < 0) {
>>>> + v4l2_err(&isc->v4l2_dev, "failed to find pad for %s\n",
>>>> + subdev->name);
>>>> + return pad;
>>>> + }
>>>> +
>>>> + isc->remote_pad = pad;
>>>> +
>>>> return 0;
>>>> }
>>>>
>>>> @@ -1748,8 +1759,8 @@ static void isc_async_unbind(struct v4l2_async_notifier *notifier,
>>>> v4l2_ctrl_handler_free(&isc->ctrls.handler);
>>>> }
>>>>
>>>> -static struct isc_format *find_format_by_code(struct isc_device *isc,
>>>> - unsigned int code, int *index)
>>>> +struct isc_format *isc_find_format_by_code(struct isc_device *isc,
>>>> + unsigned int code, int *index)
>>>> {
>>>> struct isc_format *fmt = &isc->formats_list[0];
>>>> unsigned int i;
>>>> @@ -1765,6 +1776,7 @@ static struct isc_format *find_format_by_code(struct isc_device *isc,
>>>>
>>>> return NULL;
>>>> }
>>>> +EXPORT_SYMBOL_GPL(isc_find_format_by_code);
>>>>
>>>> static int isc_formats_init(struct isc_device *isc)
>>>> {
>>>> @@ -1781,7 +1793,7 @@ static int isc_formats_init(struct isc_device *isc)
>>>> NULL, &mbus_code)) {
>>>> mbus_code.index++;
>>>>
>>>> - fmt = find_format_by_code(isc, mbus_code.code, &i);
>>>> + fmt = isc_find_format_by_code(isc, mbus_code.code, &i);
>>>> if (!fmt) {
>>>> v4l2_warn(&isc->v4l2_dev, "Mbus code %x not supported\n",
>>>> mbus_code.code);
>>>> @@ -1909,7 +1921,8 @@ static int isc_async_complete(struct v4l2_async_notifier *notifier)
>>>> vdev->queue = q;
>>>> vdev->lock = &isc->lock;
>>>> vdev->ctrl_handler = &isc->ctrls.handler;
>>>> - vdev->device_caps = V4L2_CAP_STREAMING | V4L2_CAP_VIDEO_CAPTURE;
>>>> + vdev->device_caps = V4L2_CAP_STREAMING | V4L2_CAP_VIDEO_CAPTURE |
>>>> + V4L2_CAP_IO_MC;
>>>> video_set_drvdata(vdev, isc);
>>>>
>>>> ret = video_register_device(vdev, VFL_TYPE_VIDEO, -1);
>>>> @@ -1919,8 +1932,14 @@ static int isc_async_complete(struct v4l2_async_notifier *notifier)
>>>> goto isc_async_complete_err;
>>>> }
>>>>
>>>> + ret = isc_mc_register(isc);
>>>> + if (ret < 0)
>>>> + goto isc_async_complete_unregister_device;
>>>> return 0;
>>>>
>>>> +isc_async_complete_unregister_device:
>>>> + video_unregister_device(vdev);
>>>> +
>>>> isc_async_complete_err:
>>>> mutex_destroy(&isc->lock);
>>>> return ret;
>>>> diff --git a/drivers/media/platform/atmel/atmel-isc-mc.c b/drivers/media/platform/atmel/atmel-isc-mc.c
>>>> new file mode 100644
>>>> index 000000000000..2aea81d77b2f
>>>> --- /dev/null
>>>> +++ b/drivers/media/platform/atmel/atmel-isc-mc.c
>>>> @@ -0,0 +1,287 @@
>>>> +// SPDX-License-Identifier: GPL-2.0-only
>>>> +/*
>>>> + * Microchip Image Sensor Controller (ISC) Media Controller support
>>>> + *
>>>> + * Copyright (C) 2021 Microchip Technology, Inc.
>>>> + *
>>>> + * Author: Eugen Hristev <eugen.hristev@...rochip.com>
>>>> + *
>>>> + */
>>>> +
>>>> +#include <media/v4l2-device.h>
>>>> +#include <media/v4l2-subdev.h>
>>>
>>> Should you include <media/media-entity.h> and <media/media-device.h> ?
>>
>> Hi Jacopo,
>>
>> They are probably being included from another header. I will add them.
>>
>
> Thank you :)
>
>>>
>>> Also, should you select MEDIA_CONTROLLER and V4L2_SUBDEV_API for all
>>> the Kconfig symbols ?
>>
>>
>> VIDEO_ATMEL_ISC directly selects:
>> select MEDIA_CONTROLLER
>> select VIDEO_V4L2_SUBDEV_API
>>
>> and VIDEO_ATMEL_XISC
>> depends on VIDEO_V4L2 && COMMON_CLK && VIDEO_V4L2_SUBDEV_API
>
> And selects VIDEO_ATMEL_ISC_BASE so it's fine!
>
> VIDEO_ATMEL_ISI does not. I'm fine if this is intended, I might have
> got lost in the several pieces this driver is composed of :)
The ISI is a totally different driver with no code shared with the ISC.
It's a different hardware, on some older platforms.
It does not support media controller and will not in the near future.
Initially there was ISI, then ISC appeared and a new driver was created.
Then a newer version of the ISC appeared named XISC, but a lot was
common with the old ISC, so I created a new driver XISC with shared
common base with the old ISC.
So now we have ISI, and ISC/XISC who share ATMEL_ISC_BASE
>
>>
>> and VIDEO_V4L2_SUBDEV_API depends on MEDIA_CONTROLLER
>>
>>>
>>>> +
>>>> +#include "atmel-isc-regs.h"
>>>> +#include "atmel-isc.h"
>>>> +
>>>> +static const struct media_device_ops isc_media_ops = {
>>>> +};
>>>
>>> Do you need this even if it's empty ?
>>>
>>
>> Not sure. I can remove it and pass NULL, but I haven't checked what the
>> subsystem is doing if there is no ops. I will try.
>>
>
> Seems like it is safe as the core checks for if (mdev->ops) before
> accessing it (except for mc-request for which operations are
> mandatory)
>
>>>> +
>>>> +static int isc_scaler_get_fmt(struct v4l2_subdev *sd,
>>>> + struct v4l2_subdev_state *sd_state,
>>>> + struct v4l2_subdev_format *format)
>>>> +{
>>>> + struct isc_device *isc = container_of(sd, struct isc_device, scaler_sd);
>>>> + struct v4l2_mbus_framefmt *v4l2_try_fmt;
>>>> +
>>>> + if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
>>>> + v4l2_try_fmt = v4l2_subdev_get_try_format(sd, sd_state,
>>>> + format->pad);
>>>> + format->format = *v4l2_try_fmt;
>>>> +
>>>> + return 0;
>>>> + }
>>>> +
>>>> + format->format = isc->scaler_format;
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static int isc_scaler_set_fmt(struct v4l2_subdev *sd,
>>>> + struct v4l2_subdev_state *sd_state,
>>>> + struct v4l2_subdev_format *req_fmt)
>>>> +{
>>>> + struct isc_device *isc = container_of(sd, struct isc_device, scaler_sd);
>>>> + struct v4l2_mbus_framefmt *v4l2_try_fmt;
>>>> + struct isc_format *fmt;
>>>> + unsigned int i;
>>>> +
>>>> + if (req_fmt->pad == ISC_SCALER_PAD_SOURCE)
>>>> + v4l_bound_align_image
>>>> + (&req_fmt->format.width, 16, isc->max_width, 0,
>>>> + &req_fmt->format.height, 16, isc->max_height, 0, 0);
>>>> + else
>>>> + v4l_bound_align_image
>>>> + (&req_fmt->format.width, 16, 10000, 0,
>>>> + &req_fmt->format.height, 16, 10000, 0, 0);
>>>> +
>>>> + req_fmt->format.colorspace = V4L2_COLORSPACE_SRGB;
>>>> + req_fmt->format.field = V4L2_FIELD_NONE;
>>>> + req_fmt->format.ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
>>>> + req_fmt->format.quantization = V4L2_QUANTIZATION_DEFAULT;
>>>> + req_fmt->format.xfer_func = V4L2_XFER_FUNC_DEFAULT;
>>>> +
>>>> + fmt = isc_find_format_by_code(isc, req_fmt->format.code, &i);
>>>> +
>>>> + if (!fmt)
>>>> + fmt = &isc->formats_list[0];
>>>> +
>>>> + req_fmt->format.code = fmt->mbus_code;
>>>> +
>>>> + if (req_fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
>>>> + v4l2_try_fmt = v4l2_subdev_get_try_format(sd, sd_state,
>>>> + req_fmt->pad);
>>>> + *v4l2_try_fmt = req_fmt->format;
>>>> + /* Trying on the pad sink makes the source sink change too */
>>>> + if (req_fmt->pad == ISC_SCALER_PAD_SINK) {
>>>> + v4l2_try_fmt =
>>>> + v4l2_subdev_get_try_format(sd, sd_state,
>>>> + ISC_SCALER_PAD_SOURCE);
>>>> + *v4l2_try_fmt = req_fmt->format;
>>>> +
>>>> + v4l_bound_align_image(&v4l2_try_fmt->width,
>>>> + 16, isc->max_width, 0,
>>>> + &v4l2_try_fmt->height,
>>>> + 16, isc->max_height, 0, 0);
>>>> + }
>>>> + /* if we are just trying, we are done */
>>>> + return 0;
>>>> + }
>>>> +
>>>> + isc->scaler_format = req_fmt->format;
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static int isc_scaler_enum_mbus_code(struct v4l2_subdev *sd,
>>>> + struct v4l2_subdev_state *sd_state,
>>>> + struct v4l2_subdev_mbus_code_enum *code)
>>>> +{
>>>> + struct isc_device *isc = container_of(sd, struct isc_device, scaler_sd);
>>>> + int supported_index = 0;
>>>> + int i;
>>>> +
>>>> + for (i = 0; i < isc->formats_list_size; i++) {
>>>> + if (!isc->formats_list[i].sd_support)
>>>> + continue;
>>>> + if (supported_index == code->index) {
>>>> + code->code = isc->formats_list[i].mbus_code;
>>>> + return 0;
>>>> + }
>>>> + supported_index++;
>>>> + }
>>>> +
>>>> + return -EINVAL;
>>>> +}
>>>> +
>>>> +static int isc_scaler_g_sel(struct v4l2_subdev *sd,
>>>> + struct v4l2_subdev_state *sd_state,
>>>> + struct v4l2_subdev_selection *sel)
>>>> +{
>>>> + struct isc_device *isc = container_of(sd, struct isc_device, scaler_sd);
>>>> +
>>>> + if (sel->pad == ISC_SCALER_PAD_SOURCE)
>>>> + return -EINVAL;
>>>> +
>>>> + if (sel->target != V4L2_SEL_TGT_CROP_BOUNDS &&
>>>> + sel->target != V4L2_SEL_TGT_CROP)
>>>> + return -EINVAL;
>>>> +
>>>> + sel->r.height = isc->max_height;
>>>> + sel->r.width = isc->max_width;
>>>> +
>>>> + sel->r.left = 0;
>>>> + sel->r.top = 0;
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static int isc_scaler_init_cfg(struct v4l2_subdev *sd,
>>>> + struct v4l2_subdev_state *sd_state)
>>>> +{
>>>> + struct v4l2_mbus_framefmt *v4l2_try_fmt =
>>>> + v4l2_subdev_get_try_format(sd, sd_state, 0);
>>>> + struct isc_device *isc = container_of(sd, struct isc_device, scaler_sd);
>>>> +
>>>> + *v4l2_try_fmt = isc->scaler_format;
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static const struct v4l2_subdev_pad_ops isc_scaler_pad_ops = {
>>>> + .enum_mbus_code = isc_scaler_enum_mbus_code,
>>>> + .set_fmt = isc_scaler_set_fmt,
>>>> + .get_fmt = isc_scaler_get_fmt,
>>>> + .get_selection = isc_scaler_g_sel,
>>>> + .init_cfg = isc_scaler_init_cfg,
>>>> +};
>>>> +
>>>> +static const struct v4l2_subdev_ops xisc_scaler_subdev_ops = {
>>>> + .pad = &isc_scaler_pad_ops,
>>>> +};
>>>> +
>>>> +static int isc_init_own_sd(struct isc_device *isc)
>>>> +{
>>>> + int ret;
>>>> +
>>>> + v4l2_subdev_init(&isc->scaler_sd, &xisc_scaler_subdev_ops);
>>>> +
>>>> + isc->scaler_sd.owner = THIS_MODULE;
>>>> + isc->scaler_sd.dev = isc->dev;
>>>> + snprintf(isc->scaler_sd.name, sizeof(isc->scaler_sd.name),
>>>> + "atmel_isc_scaler");
>>>> +
>>>> + isc->scaler_sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
>>>> + isc->scaler_sd.entity.function = MEDIA_ENT_F_PROC_VIDEO_SCALER;
>>>> + isc->scaler_pads[ISC_SCALER_PAD_SINK].flags = MEDIA_PAD_FL_SINK;
>>>> + isc->scaler_pads[ISC_SCALER_PAD_SOURCE].flags = MEDIA_PAD_FL_SOURCE;
>>>> +
>>>> + isc->scaler_format.height = isc->max_height;
>>>> + isc->scaler_format.width = isc->max_width;
>>>> + isc->scaler_format.code = isc->formats_list[0].mbus_code;
>>>> + isc->scaler_format.colorspace = V4L2_COLORSPACE_SRGB;
>>>> + isc->scaler_format.field = V4L2_FIELD_NONE;
>>>> + isc->scaler_format.ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
>>>> + isc->scaler_format.quantization = V4L2_QUANTIZATION_DEFAULT;
>>>> + isc->scaler_format.xfer_func = V4L2_XFER_FUNC_DEFAULT;
>>>> +
>>>> + ret = media_entity_pads_init(&isc->scaler_sd.entity,
>>>> + ISC_SCALER_PADS_NUM,
>>>> + isc->scaler_pads);
>>>> + if (ret < 0) {
>>>> + dev_err(isc->dev, "scaler sd media entity init failed\n");
>>>> + return ret;
>>>> + }
>>>> + ret = v4l2_device_register_subdev(&isc->v4l2_dev, &isc->scaler_sd);
>>>> + if (ret < 0) {
>>>> + dev_err(isc->dev, "scaler sd failed to register subdev\n");
>>>> + return ret;
>>>> + }
>>>> +
>>>> + return ret;
>>>> +}
>>>> +
>>>> +int isc_mc_init(struct isc_device *isc, u32 ver)
>>>> +{
>>>> + const struct of_device_id *match;
>>>> + int ret;
>>>> +
>>>> + isc->video_dev.entity.function = MEDIA_ENT_F_IO_V4L;
>>>> + isc->video_dev.entity.flags = MEDIA_ENT_FL_DEFAULT;
>>>> + isc->pads[ISC_PAD_SINK].flags = MEDIA_PAD_FL_SINK;
>>>> +
>>>> + ret = media_entity_pads_init(&isc->video_dev.entity, ISC_PADS_NUM,
>>>> + isc->pads);
>>>> + if (ret < 0) {
>>>> + dev_err(isc->dev, "media entity init failed\n");
>>>> + return ret;
>>>> + }
>>>> +
>>>> + isc->mdev.dev = isc->dev;
>>>> + isc->mdev.ops = &isc_media_ops;
>>>> +
>>>> + match = of_match_node(isc->dev->driver->of_match_table,
>>>> + isc->dev->of_node);
>>>> +
>>>> + strscpy(isc->mdev.driver_name, KBUILD_MODNAME,
>>>> + sizeof(isc->mdev.driver_name));
>>>> + strscpy(isc->mdev.model, match->compatible, sizeof(isc->mdev.model));
>>>> + snprintf(isc->mdev.bus_info, sizeof(isc->mdev.bus_info), "platform:%s",
>>>> + isc->v4l2_dev.name);
>>>> + isc->mdev.hw_revision = ver;
>>>> +
>>>> + media_device_init(&isc->mdev);
>>>> +
>>>> + isc->v4l2_dev.mdev = &isc->mdev;
>>>> +
>>>> + return isc_init_own_sd(isc);
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(isc_mc_init);
>>>> +
>>>> +int isc_mc_register(struct isc_device *isc)
>>>> +{
>>>> + int ret;
>>>> +
>>>> + ret = media_create_pad_link(&isc->current_subdev->sd->entity,
>>>> + isc->remote_pad, &isc->scaler_sd.entity,
>>>> + ISC_SCALER_PAD_SINK,
>>>> + MEDIA_LNK_FL_ENABLED |
>>>> + MEDIA_LNK_FL_IMMUTABLE);
>>>> +
>>>> + if (ret < 0) {
>>>> + v4l2_err(&isc->v4l2_dev,
>>>> + "Failed to create pad link: %s to %s\n",
>>>> + isc->current_subdev->sd->entity.name,
>>>> + isc->scaler_sd.entity.name);
>>>> + return ret;
>>>> + }
>>>> +
>>>> + dev_dbg(isc->dev, "link with %s pad: %d\n",
>>>> + isc->current_subdev->sd->name, isc->remote_pad);
>>>> +
>>>> + ret = media_create_pad_link(&isc->scaler_sd.entity,
>>>> + ISC_SCALER_PAD_SOURCE,
>>>> + &isc->video_dev.entity, ISC_PAD_SINK,
>>>> + MEDIA_LNK_FL_ENABLED |
>>>> + MEDIA_LNK_FL_IMMUTABLE);
>>>> +
>>>> + if (ret < 0) {
>>>> + v4l2_err(&isc->v4l2_dev,
>>>> + "Failed to create pad link: %s to %s\n",
>>>> + isc->scaler_sd.entity.name,
>>>> + isc->video_dev.entity.name);
>>>> + return ret;
>>>> + }
>>>> +
>>>> + dev_dbg(isc->dev, "link with %s pad: %d\n", isc->scaler_sd.name,
>>>> + ISC_SCALER_PAD_SOURCE);
>>>> +
>>>> + return media_device_register(&isc->mdev);
>>>
>>> I feel like the isc-base driver shold instead register the
>>> media-controller and this file shuld actually be only about
>>> isc-scaler or something. I might be missing why it is not possible
>>> though. It doesn't seem to me that mc support is conditional, so it
>>> can happen in the base driver directly ?
>>
>> This actually is the base driver. Just that the atmel-isc-base.c is
>> pretty crowded. So I moved the MC part in this file. But it's the same
>> base driver.
>>
>
> I understand. isc-base is pretty big, you're right.
> However breaking mc apart in a file that mixes registering an entity
> for the scaler and initializing mc for the whole driver feels weird,
> at least to me, as I don't immediately get why the mc part is handled
> by the scaler file...
>
>>>
>>> Links creation can still happen at async_complete() (I would have
>>> suggested _bound() time, but you have no asyn subdev for the scaler,
>>> so I guess the only async subdev here is the sensor).
>>
>> Yes, the sensor or any kind of subdevice (XISC normally is connected to
>> a bridge, ISC to a sensor directly )
>>
>>>
>>> All in all: can this function be removed and:
>>> 1) Base driver registers the media controller device in probe
>>
>> Should we register the media device even if it's not operational ? and
>> no video device has been registered ?
>> I thought it would be safer to have it registered with all the
>> links/entities/etc once the video node has been completed.
>>
>> But if you say this is the proper way, I can do that.
>> I wanted to keep things separated and not overcrowd the base file /probe
>> function, but if it's just a media_device_register call, I can do it there.
>
> You are right. Calling media_device_register() at async_complete time
> is the right thing to do.
>
> Thanks
> j
>
>
>>
>>> 2) links are created at async_complete time ?
>>
>> That works too. I think I could move them there.
>>
>> Thanks for reviewing,
>>
>> Eugen
>>
>>>
>>> Thanks
>>> j
>>>
>>>
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(isc_mc_register);
>>>> +
>>>> +void isc_mc_cleanup(struct isc_device *isc)
>>>> +{
>>>> + media_entity_cleanup(&isc->video_dev.entity);
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(isc_mc_cleanup);
>>>> diff --git a/drivers/media/platform/atmel/atmel-isc.h b/drivers/media/platform/atmel/atmel-isc.h
>>>> index 07fa6dbf8460..0b6370d7775f 100644
>>>> --- a/drivers/media/platform/atmel/atmel-isc.h
>>>> +++ b/drivers/media/platform/atmel/atmel-isc.h
>>>> @@ -183,6 +183,17 @@ struct isc_reg_offsets {
>>>> u32 his_entry;
>>>> };
>>>>
>>>> +enum isc_mc_pads {
>>>> + ISC_PAD_SINK = 0,
>>>> + ISC_PADS_NUM = 1,
>>>> +};
>>>> +
>>>> +enum isc_scaler_pads {
>>>> + ISC_SCALER_PAD_SINK = 0,
>>>> + ISC_SCALER_PAD_SOURCE = 1,
>>>> + ISC_SCALER_PADS_NUM = 2,
>>>> +};
>>>> +
>>>> /*
>>>> * struct isc_device - ISC device driver data/config struct
>>>> * @regmap: Register map
>>>> @@ -258,6 +269,12 @@ struct isc_reg_offsets {
>>>> * be used as an input to the controller
>>>> * @controller_formats_size: size of controller_formats array
>>>> * @formats_list_size: size of formats_list array
>>>> + * @pads: media controller pads for isc video entity
>>>> + * @mdev: media device that is registered by the isc
>>>> + * @remote_pad: remote pad on the connected subdevice
>>>> + * @scaler_sd: subdevice for the scaler that isc registers
>>>> + * @scaler_pads: media controller pads for the scaler subdevice
>>>> + * @scaler_format: current format for the scaler subdevice
>>>> */
>>>> struct isc_device {
>>>> struct regmap *regmap;
>>>> @@ -346,6 +363,19 @@ struct isc_device {
>>>> struct isc_format *formats_list;
>>>> u32 controller_formats_size;
>>>> u32 formats_list_size;
>>>> +
>>>> + struct {
>>>> + struct media_pad pads[ISC_PADS_NUM];
>>>> + struct media_device mdev;
>>>> +
>>>> + u32 remote_pad;
>>>> + };
>>>> +
>>>> + struct {
>>>> + struct v4l2_subdev scaler_sd;
>>>> + struct media_pad scaler_pads[ISC_SCALER_PADS_NUM];
>>>> + struct v4l2_mbus_framefmt scaler_format;
>>>> + };
>>>> };
>>>>
>>>> extern const struct regmap_config isc_regmap_config;
>>>> @@ -357,4 +387,10 @@ int isc_clk_init(struct isc_device *isc);
>>>> void isc_subdev_cleanup(struct isc_device *isc);
>>>> void isc_clk_cleanup(struct isc_device *isc);
>>>>
>>>> +int isc_mc_init(struct isc_device *isc, u32 ver);
>>>> +int isc_mc_register(struct isc_device *isc);
>>>> +void isc_mc_cleanup(struct isc_device *isc);
>>>> +
>>>> +struct isc_format *isc_find_format_by_code(struct isc_device *isc,
>>>> + unsigned int code, int *index);
>>>> #endif
>>>> diff --git a/drivers/media/platform/atmel/atmel-sama5d2-isc.c b/drivers/media/platform/atmel/atmel-sama5d2-isc.c
>>>> index 1b2063cce0f7..a3e4c32a66c4 100644
>>>> --- a/drivers/media/platform/atmel/atmel-sama5d2-isc.c
>>>> +++ b/drivers/media/platform/atmel/atmel-sama5d2-isc.c
>>>> @@ -529,6 +529,12 @@ static int atmel_isc_probe(struct platform_device *pdev)
>>>> break;
>>>> }
>>>>
>>>> + regmap_read(isc->regmap, ISC_VERSION + isc->offsets.version, &ver);
>>>> +
>>>> + ret = isc_mc_init(isc, ver);
>>>> + if (ret < 0)
>>>> + goto isc_probe_mc_init_err;
>>>> +
>>>> pm_runtime_set_active(dev);
>>>> pm_runtime_enable(dev);
>>>> pm_request_idle(dev);
>>>> @@ -538,7 +544,7 @@ static int atmel_isc_probe(struct platform_device *pdev)
>>>> ret = clk_prepare_enable(isc->ispck);
>>>> if (ret) {
>>>> dev_err(dev, "failed to enable ispck: %d\n", ret);
>>>> - goto cleanup_subdev;
>>>> + goto isc_probe_mc_init_err;
>>>> }
>>>>
>>>> /* ispck should be greater or equal to hclock */
>>>> @@ -548,7 +554,6 @@ static int atmel_isc_probe(struct platform_device *pdev)
>>>> goto unprepare_clk;
>>>> }
>>>>
>>>> - regmap_read(isc->regmap, ISC_VERSION + isc->offsets.version, &ver);
>>>> dev_info(dev, "Microchip ISC version %x\n", ver);
>>>>
>>>> return 0;
>>>> @@ -556,6 +561,9 @@ static int atmel_isc_probe(struct platform_device *pdev)
>>>> unprepare_clk:
>>>> clk_disable_unprepare(isc->ispck);
>>>>
>>>> +isc_probe_mc_init_err:
>>>> + isc_mc_cleanup(isc);
>>>> +
>>>> cleanup_subdev:
>>>> isc_subdev_cleanup(isc);
>>>>
>>>> @@ -576,6 +584,8 @@ static int atmel_isc_remove(struct platform_device *pdev)
>>>>
>>>> pm_runtime_disable(&pdev->dev);
>>>>
>>>> + isc_mc_cleanup(isc);
>>>> +
>>>> isc_subdev_cleanup(isc);
>>>>
>>>> v4l2_device_unregister(&isc->v4l2_dev);
>>>> diff --git a/drivers/media/platform/atmel/atmel-sama7g5-isc.c b/drivers/media/platform/atmel/atmel-sama7g5-isc.c
>>>> index 2b1082295c13..d2ac80b1ccc3 100644
>>>> --- a/drivers/media/platform/atmel/atmel-sama7g5-isc.c
>>>> +++ b/drivers/media/platform/atmel/atmel-sama7g5-isc.c
>>>> @@ -523,15 +523,23 @@ static int microchip_xisc_probe(struct platform_device *pdev)
>>>> break;
>>>> }
>>>>
>>>> + regmap_read(isc->regmap, ISC_VERSION + isc->offsets.version, &ver);
>>>> +
>>>> + ret = isc_mc_init(isc, ver);
>>>> + if (ret < 0)
>>>> + goto isc_probe_mc_init_err;
>>>> +
>>>> pm_runtime_set_active(dev);
>>>> pm_runtime_enable(dev);
>>>> pm_request_idle(dev);
>>>>
>>>> - regmap_read(isc->regmap, ISC_VERSION + isc->offsets.version, &ver);
>>>> dev_info(dev, "Microchip XISC version %x\n", ver);
>>>>
>>>> return 0;
>>>>
>>>> +isc_probe_mc_init_err:
>>>> + isc_mc_cleanup(isc);
>>>> +
>>>> cleanup_subdev:
>>>> isc_subdev_cleanup(isc);
>>>>
>>>> @@ -552,6 +560,8 @@ static int microchip_xisc_remove(struct platform_device *pdev)
>>>>
>>>> pm_runtime_disable(&pdev->dev);
>>>>
>>>> + isc_mc_cleanup(isc);
>>>> +
>>>> isc_subdev_cleanup(isc);
>>>>
>>>> v4l2_device_unregister(&isc->v4l2_dev);
>>>> --
>>>> 2.25.1
>>>>
>>
Powered by blists - more mailing lists