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

[Mirela Rabulea <mirela.rabulea@nxp.com>]

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
>