[1/4] media: i2c: imx415: Add get_mbus_config() pad operation support

Allow the driver to report static media bus configuration using
pad get_mbus_config() operation.

Signed-off-by: Alexander Shiyan <eagle.alexander923@gmail.com>
---
 drivers/media/i2c/imx415.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

Hi Alexander

On Wed, 19 Feb 2025 at 09:48, Alexander Shiyan
<eagle.alexander923@gmail.com> wrote:
>
> Not all CSI configurations are suitable for both 2-lane and 4-lane mode.
> To solve this, let's use a zero value in the hmax_min[] field of the
> supported_modes[] structure to indicate which CSI configuration can not
> be used for 2-lane or 4-lane mode.
> Now that we have done that, let's add the remaining CSI configurations
> that can be used for 4-lane mode.
>
> Signed-off-by: Alexander Shiyan <eagle.alexander923@gmail.com>
> ---
>  drivers/media/i2c/imx415.c | 46 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 46 insertions(+)
>
> diff --git a/drivers/media/i2c/imx415.c b/drivers/media/i2c/imx415.c
> index 83b7929455b2..5f18d3f38ded 100644
> --- a/drivers/media/i2c/imx415.c
> +++ b/drivers/media/i2c/imx415.c
> @@ -505,6 +505,19 @@ static const struct cci_reg_sequence imx415_linkrate_1440mbps[] = {
>         { IMX415_TLPX, 0x004F },
>  };
>
> +/* 1485 Mbps CSI configuration */
> +static const struct cci_reg_sequence imx415_linkrate_1485mbps[] = {
> +       { IMX415_TCLKPOST, 0x00A7 },
> +       { IMX415_TCLKPREPARE, 0x0057 },
> +       { IMX415_TCLKTRAIL, 0x005F },
> +       { IMX415_TCLKZERO, 0x0197 },
> +       { IMX415_THSPREPARE, 0x005F },
> +       { IMX415_THSZERO, 0x00AF },
> +       { IMX415_THSTRAIL, 0x005F },
> +       { IMX415_THSEXIT, 0x009F },
> +       { IMX415_TLPX, 0x004F },
> +};
> +
>  /* 1782 Mbps CSI configuration */
>  static const struct cci_reg_sequence imx415_linkrate_1782mbps[] = {
>         { IMX415_TCLKPOST, 0x00B7 },
> @@ -531,6 +544,19 @@ static const struct cci_reg_sequence imx415_linkrate_2079mbps[] = {
>         { IMX415_TLPX, 0x006F },
>  };
>
> +/* 2376 Mbps CSI configuration */
> +static const struct cci_reg_sequence imx415_linkrate_2376mbps[] = {
> +       { IMX415_TCLKPOST, 0x00E7 },
> +       { IMX415_TCLKPREPARE, 0x008F },
> +       { IMX415_TCLKTRAIL, 0x008F },
> +       { IMX415_TCLKZERO, 0x027F },
> +       { IMX415_THSPREPARE, 0x0097 },
> +       { IMX415_THSZERO, 0x010F },
> +       { IMX415_THSTRAIL, 0x0097 },
> +       { IMX415_THSEXIT, 0x00F7 },
> +       { IMX415_TLPX, 0x007F },
> +};
> +
>  struct imx415_mode_reg_list {
>         u32 num_of_regs;
>         const struct cci_reg_sequence *regs;
> @@ -576,6 +602,14 @@ static const struct imx415_mode supported_modes[] = {
>                         .regs = imx415_linkrate_1440mbps,
>                 },
>         },
> +       {
> +               .lane_rate = 1485000000,
> +               .hmax_min = { 0, 550 },
> +               .reg_list = {
> +                       .num_of_regs = ARRAY_SIZE(imx415_linkrate_1485mbps),
> +                       .regs = imx415_linkrate_1485mbps,
> +               },
> +       },
>         {
>                 .lane_rate = 1782000000,
>                 .hmax_min = { 1100, 550 },
> @@ -592,6 +626,14 @@ static const struct imx415_mode supported_modes[] = {
>                         .regs = imx415_linkrate_2079mbps,
>                 },
>         },
> +       {
> +               .lane_rate = 2376000000,
> +               .hmax_min = { 0, 366 },
> +               .reg_list = {
> +                       .num_of_regs = ARRAY_SIZE(imx415_linkrate_2376mbps),
> +                       .regs = imx415_linkrate_2376mbps,
> +               },
> +       },
>  };
>
>  static const char *const imx415_test_pattern_menu[] = {
> @@ -1375,9 +1417,13 @@ static int imx415_parse_hw_config(struct imx415 *sensor)
>                 }
>
>                 for (j = 0; j < ARRAY_SIZE(supported_modes); ++j) {
> +                       int lanes_idx = sensor->num_data_lanes == 2 ? 0 : 1;
> +
>                         if (bus_cfg.link_frequencies[i] * 2 !=
>                             supported_modes[j].lane_rate)
>                                 continue;
> +                       if (!supported_modes[j].hmax_min[lanes_idx])
> +                               continue;

You could lose the local variable by checking
if (!supported_modes[j].hmax_min[0] && sensor->num_data_lanes == 2)

Either way:

Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>

I've checked the register settings against the datasheet, and they all
appear to match.

>                         sensor->cur_mode = j;
>                         break;
>                 }
> --
> 2.39.1
>
>

Hello Dave.

> > The imx415 sensor has one more register for setting the output
> > format (10/12 bit), which is currently not in the driver.
>
> The datasheet states it is the "internal A/D conversion bits" setting,
> not output format.
> Output format is set via MDBIT (reg 0x3032).
>
> The sensor may well happily truncate 12bit A/D readout to RAW10
> output, or left shift 10bit A/D values to RAW12 output.
>
> I'll defer to Wolfvision on this one as I would expect them as the
> original authors to have been given a register list by Sony for the
> readout modes that they were interested in. Sony may therefore have
> recommended this apparent mismatch in A/D depth vs output format.

As far as I can see from the datasheet, the "Operating mode" table (page 48)
clearly states that the AS-conversion and output bit width values are equal.

Thanks!

Hi Alexander,

Thanks for the patches. Next time, a cover letter that describes the
overall goal of the changes would be nice for a series like this.

On 2/19/25 10:46, Alexander Shiyan wrote:
> Allow the driver to report static media bus configuration using
> pad get_mbus_config() operation.
> 
> Signed-off-by: Alexander Shiyan <eagle.alexander923@gmail.com>

Reviewed-by: Michael Riesch <michael.riesch@wolfvision.net>

Best regards,
Michael

> ---
>  drivers/media/i2c/imx415.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/drivers/media/i2c/imx415.c b/drivers/media/i2c/imx415.c
> index 9f37779bd611..16a52900c61c 100644
> --- a/drivers/media/i2c/imx415.c
> +++ b/drivers/media/i2c/imx415.c
> @@ -1076,6 +1076,18 @@ static int imx415_init_state(struct v4l2_subdev *sd,
>  	return 0;
>  }
>  
> +static int imx415_get_mbus_config(struct v4l2_subdev *sd, unsigned int pad_id,
> +				  struct v4l2_mbus_config *config)
> +{
> +	struct imx415 *sensor = to_imx415(sd);
> +
> +	config->type = V4L2_MBUS_CSI2_DPHY;
> +	config->bus.mipi_csi2.flags = 0;
> +	config->bus.mipi_csi2.num_data_lanes = sensor->num_data_lanes;
> +
> +	return 0;
> +}
> +
>  static const struct v4l2_subdev_video_ops imx415_subdev_video_ops = {
>  	.s_stream = imx415_s_stream,
>  };
> @@ -1086,6 +1098,7 @@ static const struct v4l2_subdev_pad_ops imx415_subdev_pad_ops = {
>  	.get_fmt = v4l2_subdev_get_fmt,
>  	.set_fmt = imx415_set_format,
>  	.get_selection = imx415_get_selection,
> +	.get_mbus_config = imx415_get_mbus_config,
>  };
>  
>  static const struct v4l2_subdev_ops imx415_subdev_ops = {

Hi Alexander, Dave,

On 2/20/25 19:47, Alexander Shiyan wrote:
> Hello Dave.
> 
>>> The imx415 sensor has one more register for setting the output
>>> format (10/12 bit), which is currently not in the driver.
>>
>> The datasheet states it is the "internal A/D conversion bits" setting,
>> not output format.
>> Output format is set via MDBIT (reg 0x3032).
>>
>> The sensor may well happily truncate 12bit A/D readout to RAW10
>> output, or left shift 10bit A/D values to RAW12 output.
>>
>> I'll defer to Wolfvision on this one as I would expect them as the
>> original authors to have been given a register list by Sony for the
>> readout modes that they were interested in. Sony may therefore have
>> recommended this apparent mismatch in A/D depth vs output format.

Cc: Gerald who is already looking into this

As far as I can gather, the register is not crucial for the correct
operation. The chip works fine for us with the current driver and ADBIT1
is not mentioned in the register lists for the different modes.

However, we have successfully set the register in a different project
(MCU + IMX415).

The patch may not be essential, but it is clearly correct IMHO.

Best regards,
Michael

> 
> As far as I can see from the datasheet, the "Operating mode" table (page 48)
> clearly states that the AS-conversion and output bit width values are equal.
> 
> Thanks!

Hi Alexander,

> -----Ursprüngliche Nachricht-----
> Von: Michael Riesch <Michael.Riesch@wolfvision.net>
> Gesendet: Dienstag, 25. Februar 2025 09:44
> An: Alexander Shiyan <eagle.alexander923@gmail.com>; Dave Stevenson
> <dave.stevenson@raspberrypi.com>
> Cc: linux-media@vger.kernel.org; Sakari Ailus <sakari.ailus@linux.intel.com>;
> Mauro Carvalho Chehab <mchehab@kernel.org>; Gerald Loacker
> <Gerald.Loacker@wolfvision.net>
> Betreff: Re: [PATCH 2/4] media: i2c: imx415: Add missing ADBIT1 register for setup
> output format
> 
> Hi Alexander, Dave,
> 
> On 2/20/25 19:47, Alexander Shiyan wrote:
> > Hello Dave.
> >
> >>> The imx415 sensor has one more register for setting the output
> >>> format (10/12 bit), which is currently not in the driver.
> >>
> >> The datasheet states it is the "internal A/D conversion bits" setting,
> >> not output format.
> >> Output format is set via MDBIT (reg 0x3032).
> >>
> >> The sensor may well happily truncate 12bit A/D readout to RAW10
> >> output, or left shift 10bit A/D values to RAW12 output.
> >>
> >> I'll defer to Wolfvision on this one as I would expect them as the
> >> original authors to have been given a register list by Sony for the
> >> readout modes that they were interested in. Sony may therefore have
> >> recommended this apparent mismatch in A/D depth vs output format.
> 
> Cc: Gerald who is already looking into this
> 
> As far as I can gather, the register is not crucial for the correct
> operation. The chip works fine for us with the current driver and ADBIT1
> is not mentioned in the register lists for the different modes.
> 
> However, we have successfully set the register in a different project
> (MCU + IMX415).
> 
> The patch may not be essential, but it is clearly correct IMHO.
> 

It's correct that this register needs to be set to zero for 10-bit operation 
according to the datasheet, although there are no visible differences.

Reviewed-by: Gerald Loacker <gerald.loacker@wolfvision.net>
Tested-by: Gerald Loacker <gerald.loacker@wolfvision.net>

Best regards,
Gerald

> Best regards,
> Michael
> 
> >
> > As far as I can see from the datasheet, the "Operating mode" table (page 48)
> > clearly states that the AS-conversion and output bit width values are equal.
> >
> > Thanks!