From: <Eugen.Hristev@microchip.com>
To: <jacopo@jmondi.org>
Cc: <linux-media@vger.kernel.org>, <robh+dt@kernel.org>,
<laurent.pinchart@ideasonboard.com>, <sakari.ailus@iki.fi>,
<devicetree@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
<linux-arm-kernel@lists.infradead.org>,
<Nicolas.Ferre@microchip.com>
Subject: Re: [PATCH v3 12/23] media: atmel: atmel-isc: implement media controller
Date: Tue, 11 Jan 2022 17:00:28 +0000 [thread overview]
Message-ID: <7a05c500-b46d-e7c9-25d6-f787c01352bf@microchip.com> (raw)
In-Reply-To: <20220111165132.g62ykrjjk3jh4eyd@uno.localdomain>
On 1/11/22 6:51 PM, Jacopo Mondi wrote:
> On Tue, Jan 11, 2022 at 04:30:15PM +0000, Eugen.Hristev@microchip.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@microchip.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@microchip.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
>>>>
>>
WARNING: multiple messages have this Message-ID (diff)
From: <Eugen.Hristev@microchip.com>
To: <jacopo@jmondi.org>
Cc: devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
robh+dt@kernel.org, sakari.ailus@iki.fi,
laurent.pinchart@ideasonboard.com,
linux-arm-kernel@lists.infradead.org,
linux-media@vger.kernel.org
Subject: Re: [PATCH v3 12/23] media: atmel: atmel-isc: implement media controller
Date: Tue, 11 Jan 2022 17:00:28 +0000 [thread overview]
Message-ID: <7a05c500-b46d-e7c9-25d6-f787c01352bf@microchip.com> (raw)
In-Reply-To: <20220111165132.g62ykrjjk3jh4eyd@uno.localdomain>
On 1/11/22 6:51 PM, Jacopo Mondi wrote:
> On Tue, Jan 11, 2022 at 04:30:15PM +0000, Eugen.Hristev@microchip.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@microchip.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@microchip.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
>>>>
>>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2022-01-11 17:00 UTC|newest]
Thread overview: 94+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-12-13 13:49 [PATCH v3 00/23] media: atmel: atmel-isc: implement media controller Eugen Hristev
2021-12-13 13:49 ` Eugen Hristev
2021-12-13 13:49 ` [PATCH v3 01/23] MAINTAINERS: add microchip csi2dc Eugen Hristev
2021-12-13 13:49 ` Eugen Hristev
2021-12-13 13:49 ` [PATCH v3 02/23] dt-bindings: media: atmel: csi2dc: add bindings for " Eugen Hristev
2021-12-13 13:49 ` Eugen Hristev
2021-12-15 20:16 ` Rob Herring
2021-12-15 20:16 ` Rob Herring
2021-12-13 13:49 ` [PATCH v3 03/23] media: atmel: introduce microchip csi2dc driver Eugen Hristev
2021-12-13 13:49 ` Eugen Hristev
2022-01-11 15:43 ` Jacopo Mondi
2022-01-11 15:43 ` Jacopo Mondi
2021-12-13 13:49 ` [PATCH v3 04/23] media: atmel: atmel-isc: split the clock code into separate source file Eugen Hristev
2021-12-13 13:49 ` Eugen Hristev
2022-01-11 15:43 ` Jacopo Mondi
2022-01-11 15:43 ` Jacopo Mondi
2021-12-13 13:49 ` [PATCH v3 05/23] media: atmel: atmel-isc: replace video device name with module name Eugen Hristev
2021-12-13 13:49 ` Eugen Hristev
2021-12-13 13:49 ` [PATCH v3 06/23] media: atmel: atmel-sama7g5-isc: fix ispck leftover Eugen Hristev
2021-12-13 13:49 ` Eugen Hristev
2021-12-13 13:49 ` [PATCH v3 07/23] media: atmel: atmel-isc-base: use streaming status when queueing buffers Eugen Hristev
2021-12-13 13:49 ` Eugen Hristev
2022-01-11 15:53 ` Hans Verkuil
2022-01-11 15:53 ` Hans Verkuil
2022-01-11 16:38 ` Eugen.Hristev
2022-01-11 16:38 ` Eugen.Hristev
2022-01-12 9:52 ` Hans Verkuil
2022-01-12 9:52 ` Hans Verkuil
2021-12-13 13:49 ` [PATCH v3 08/23] media: atmel: atmel-isc-base: remove frameintervals VIDIOC Eugen Hristev
2021-12-13 13:49 ` Eugen Hristev
2021-12-13 13:49 ` [PATCH v3 09/23] media: atmel: atmel-isc-base: report frame sizes as full supported range Eugen Hristev
2021-12-13 13:49 ` Eugen Hristev
2021-12-13 13:49 ` [PATCH v3 10/23] media: atmel: atmel-isc-base: implement mbus_code support in enumfmt Eugen Hristev
2021-12-13 13:49 ` Eugen Hristev
2021-12-13 13:49 ` [PATCH v3 11/23] media: atmel: atmel-isc-base: fix bytesperline value for planar formats Eugen Hristev
2021-12-13 13:49 ` Eugen Hristev
2021-12-13 13:49 ` [PATCH v3 12/23] media: atmel: atmel-isc: implement media controller Eugen Hristev
2021-12-13 13:49 ` Eugen Hristev
2022-01-11 16:06 ` Jacopo Mondi
2022-01-11 16:06 ` Jacopo Mondi
2022-01-11 16:30 ` Eugen.Hristev
2022-01-11 16:30 ` Eugen.Hristev
2022-01-11 16:51 ` Jacopo Mondi
2022-01-11 16:51 ` Jacopo Mondi
2022-01-11 17:00 ` Eugen.Hristev [this message]
2022-01-11 17:00 ` Eugen.Hristev
2021-12-13 13:49 ` [PATCH v3 13/23] ARM: dts: at91: sama7g5: add nodes for video capture Eugen Hristev
2021-12-13 13:49 ` Eugen Hristev
2021-12-13 13:49 ` [PATCH v3 14/23] ARM: configs: at91: sama7: add xisc and csi2dc Eugen Hristev
2021-12-13 13:49 ` Eugen Hristev
2021-12-13 13:49 ` [PATCH v3 15/23] ARM: multi_v7_defconfig: add atmel video pipeline modules Eugen Hristev
2021-12-13 13:49 ` Eugen Hristev
2021-12-13 13:49 ` [PATCH v3 16/23] media: atmel: atmel-sama5d2-isc: fix wrong mask in YUYV format check Eugen Hristev
2021-12-13 13:49 ` Eugen Hristev
2021-12-13 13:49 ` [PATCH v3 17/23] media: atmel: atmel-isc-base: use mutex to lock awb workqueue from streaming Eugen Hristev
2021-12-13 13:49 ` Eugen Hristev
2022-01-12 8:58 ` Jacopo Mondi
2022-01-12 8:58 ` Jacopo Mondi
2022-01-19 15:40 ` Eugen.Hristev
2022-01-19 15:40 ` Eugen.Hristev
2021-12-13 13:49 ` [PATCH v3 18/23] media: atmel: atmel-isc-base: add wb debug messages Eugen Hristev
2021-12-13 13:49 ` Eugen Hristev
2022-01-12 9:00 ` Jacopo Mondi
2022-01-12 9:00 ` Jacopo Mondi
2021-12-13 13:49 ` [PATCH v3 19/23] media: atmel: atmel-isc-base: clamp wb gain coefficients Eugen Hristev
2021-12-13 13:49 ` Eugen Hristev
2022-01-12 9:04 ` Jacopo Mondi
2022-01-12 9:04 ` Jacopo Mondi
2021-12-13 13:49 ` [PATCH v3 20/23] media: atmel: atmel-sama7g5-isc: fix UYVY input format mbus_code typo Eugen Hristev
2021-12-13 13:49 ` Eugen Hristev
2022-01-12 9:04 ` Jacopo Mondi
2022-01-12 9:04 ` Jacopo Mondi
2021-12-13 13:49 ` [PATCH v3 21/23] media: atmel: atmel-isc: add raw Bayer 8bit 10bit output formats Eugen Hristev
2021-12-13 13:49 ` Eugen Hristev
2022-01-12 9:05 ` Jacopo Mondi
2022-01-12 9:05 ` Jacopo Mondi
2021-12-13 13:49 ` [PATCH v3 22/23] media: atmel: atmel-isc: compact the controller formats list Eugen Hristev
2021-12-13 13:49 ` Eugen Hristev
2022-01-12 9:06 ` Jacopo Mondi
2022-01-12 9:06 ` Jacopo Mondi
2021-12-13 13:49 ` [PATCH v3 23/23] media: atmel: atmel-isc: change format propagation to subdev into only verification Eugen Hristev
2021-12-13 13:49 ` Eugen Hristev
2022-01-12 9:21 ` Jacopo Mondi
2022-01-12 9:21 ` Jacopo Mondi
2022-01-20 8:43 ` Eugen.Hristev
2022-01-20 8:43 ` Eugen.Hristev
2022-01-20 8:48 ` Eugen.Hristev
2022-01-20 8:48 ` Eugen.Hristev
2022-01-20 8:58 ` Jacopo Mondi
2022-01-20 8:58 ` Jacopo Mondi
2022-01-12 12:07 ` [PATCH v3 00/23] media: atmel: atmel-isc: implement media controller Hans Verkuil
2022-01-12 12:07 ` Hans Verkuil
2022-01-12 12:46 ` Eugen.Hristev
2022-01-12 12:46 ` Eugen.Hristev
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=7a05c500-b46d-e7c9-25d6-f787c01352bf@microchip.com \
--to=eugen.hristev@microchip.com \
--cc=Nicolas.Ferre@microchip.com \
--cc=devicetree@vger.kernel.org \
--cc=jacopo@jmondi.org \
--cc=laurent.pinchart@ideasonboard.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-media@vger.kernel.org \
--cc=robh+dt@kernel.org \
--cc=sakari.ailus@iki.fi \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.