Re: Re: [PATCH v3 4/4] media: ox05b1s: Add support for Omnivision OS08A20 raw sensor

[Mirela Rabulea]

Hi Krzysztof,

thanks again for feedback.

On 24.01.2025 10:09, Krzysztof Kozlowski wrote:
> On Fri, Jan 24, 2025 at 02:12:43AM +0200, Mirela Rabulea wrote:
> > @@ -758,6 +914,9 @@ static int ox05b1s_read_chip_id(struct ox05b1s *sensor)
> >        }
> >
> >        switch (chip_id) {
> > +     case 0x530841:
> > +             camera_name = "os08a20";
> > +             break;
> Ah, so here I see missing second device support.
>
> It is still confusing to see that you use here some sort of
> autodetection, but in the same time not.

The two sensors that I included in this driver have some similarities, 
but also differences, for which I used the platform data. I made 
separate patches for the two sensors, such that it is visible how much 
is common/different.  The chip_id reading is for validating that the 
sensor that is actually attached matches the device tree. It happens to 
me sometimes, that I switch the sensors, but forget to switch the dtb, 
and it helps to see which sensor is actually attached. Also, it helps a 
lot when the evaluation board is in some remote lab and I am unsure what 
sensor is attached to it.

I saw most sensor drivers have some kind of identification of the sensor 
module by the means of reading the chip_id register. Some examples with 
multiple compatibles supported and chip_id validation: imx296, ov9650, 
ov772x.

Please let me know what you suggest further.

For your other comments, I did prepare more structures to become const, 
hope I did not miss any, it will be in the next version.

Thanks,

Mirela

> >        case 0x580542:
> >                camera_name = "ox05b1s";
> >                break;
> > @@ -900,6 +1059,24 @@ static void ox05b1s_remove(struct i2c_client *client)
> >   static DEFINE_RUNTIME_DEV_PM_OPS(ox05b1s_pm_ops, ox05b1s_runtime_suspend,
> >                                 ox05b1s_runtime_resume, NULL);
> >
> > +static const struct ox05b1s_plat_data os08a20_data = {
> > +     .name                   = "os08a20",
> > +     .chip_id                = 0x530841,
> > +     .native_width           = 3872, /* 16 dummy + 3840 active pixels + 16 dummy */
> > +     .native_height          = 2192, /* 16 dummy + 2160 active lines + 16 dummy */
> > +     .active_top             = 16,
> > +     .active_left            = 16,
> > +     .active_width           = 3840,
> > +     .active_height          = 2160,
> > +     .supported_modes        = os08a20_supported_modes,
> > +     .default_mode_index     = 0,
> > +     .supported_codes        = os08a20_supported_codes,
> > +     .hdr_modes              = os08a20_hdr_modes,
> > +     .hdr_modes_count        = ARRAY_SIZE(os08a20_hdr_modes),
> > +     .set_hdr_mode           = os08a20_set_hdr_mode,
> > +
> > +};
> > +
> >   static const struct ox05b1s_plat_data ox05b1s_data = {
> >        .name                   = "ox05b1s",
> >        .chip_id                = 0x580542,
> > @@ -912,9 +1089,16 @@ static const struct ox05b1s_plat_data ox05b1s_data = {
> >        .supported_modes        = ox05b1s_supported_modes,
> >        .default_mode_index     = 0,
> >        .supported_codes        = ox05b1s_supported_codes,
> > +     .hdr_modes              = NULL,
> > +     .hdr_modes_count        = 0,
> > +     .set_hdr_mode           = NULL,
> >   };
> >
> >   static const struct of_device_id ox05b1s_of_match[] = {
> > +     {
> > +             .compatible = "ovti,os08a20",
> > +             .data = &os08a20_data,
> And here static configuration of model, not autodetection.
>
>
> Best regards,
> Krzysztof