Re: [PATCH v1 3/8] MT9M114: Add get_mbus_config

Dave Stevenson
2025-02-26T17:13:54Z

Hi Mathis

On Wed, 26 Feb 2025 at 15:45, Mathis Foerst <mathis.foerst@mt.com> wrote:
>
> The i.MX6 camera frameworks requires get_mbus_config to be implemented.
> See [0].
>
> [0] drivers/staging/media/imx/imx-media-csi.c - line 211..216

The docs for get_mbus_config say
* @get_mbus_config: get the media bus configuration of a remote sub-device.
* The media bus configuration is usually retrieved from the
* firmware interface at sub-device probe time, immediately
* applied to the hardware and eventually adjusted by the
* driver.
https://github.com/torvalds/linux/blob/master/include/media/v4l2-subdev.h#L814

All other receiver drivers (including imx6-mipi-csi2.c) that call
get_mbus_config handle it returning -ENOIOCTLCMD by reverting to the
static configuration or the receiver node from device tree / ACPI.

I may be missing something, but as imx-media-csi.c appears to be the
exception, isn't it better to fix that up rather than having to fix up
all the sensor drivers that ever might get attached to it?

> Signed-off-by: Mathis Foerst <mathis.foerst@mt.com>
> ---
> drivers/media/i2c/mt9m114.c | 19 +++++++++++++++++++
> 1 file changed, 19 insertions(+)
>
> diff --git a/drivers/media/i2c/mt9m114.c b/drivers/media/i2c/mt9m114.c
> index 5f0b0ad8f885..fa64d6d315a1 100644
> --- a/drivers/media/i2c/mt9m114.c
> +++ b/drivers/media/i2c/mt9m114.c
> @@ -1977,6 +1977,24 @@ static int mt9m114_ifp_registered(struct v4l2_subdev *sd)
> v4l2_device_unregister_subdev(&sensor->pa.sd);
> return ret;
> }
> + return 0;
> +}
> +
> +static int mt9m114_ifp_get_mbus_config(struct v4l2_subdev *sd,
> + unsigned int pad,
> + struct v4l2_mbus_config *cfg)
> +{
> + struct mt9m114 *sensor = ifp_to_mt9m114(sd);
> +
> + if (sensor->bus_cfg.bus_type == V4L2_MBUS_CSI2_DPHY) {
> + cfg->type = V4L2_MBUS_CSI2_DPHY;

Not setting cfg->bus.mipi_csi2.num_data_lanes is going to cause some
confusion. What does an assumed 0 data lanes mean?

Likewise it would be sensible to set cfg->bus.mipi_csi2.flags so as to
avoid any ambiguities (did the caller memset all fields before
calling?)

Dave

> + } else {
> + cfg->type = V4L2_MBUS_PARALLEL;
> + cfg->bus.parallel.flags = V4L2_MBUS_MASTER |
> + V4L2_MBUS_PCLK_SAMPLE_RISING |
> + V4L2_MBUS_DATA_ACTIVE_HIGH;
> + cfg->bus.parallel.bus_width = 8;
> + }
>
> return 0;
> }
> @@ -1993,6 +2011,7 @@ static const struct v4l2_subdev_pad_ops mt9m114_ifp_pad_ops = {
> .set_fmt = mt9m114_ifp_set_fmt,
> .get_selection = mt9m114_ifp_get_selection,
> .set_selection = mt9m114_ifp_set_selection,
> + .get_mbus_config = mt9m114_ifp_get_mbus_config,
> .get_frame_interval = mt9m114_ifp_get_frame_interval,
> .set_frame_interval = mt9m114_ifp_set_frame_interval,
> };
> --
> 2.34.1
>
>