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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <56fe323f-ee09-4a60-ab57-be2baab3af30@nxp.com>
Date: Mon, 3 Feb 2025 10:43:50 +0200
From: Mirela Rabulea <mirela.rabulea@....com>
To: Krzysztof Kozlowski <krzk@...nel.org>
Cc: mchehab@...nel.org, sakari.ailus@...ux.intel.com,
 hverkuil-cisco@...all.nl, laurent.pinchart+renesas@...asonboard.com,
 robh@...nel.org, krzk+dt@...nel.org, bryan.odonoghue@...aro.org,
 laurentiu.palcu@....com, robert.chiras@....com, linux-media@...r.kernel.org,
 linux-kernel@...r.kernel.org, LnxRevLi@....com,
 kieran.bingham@...asonboard.com, hdegoede@...hat.com,
 dave.stevenson@...pberrypi.com, mike.rudenko@...il.com,
 alain.volmat@...s.st.com, devicetree@...r.kernel.org, conor+dt@...nel.org,
 alexander.stein@...tq-group.com, umang.jain@...asonboard.com,
 zhi.mao@...iatek.com, festevam@...x.de, julien.vuillaumier@....com,
 alice.yuan@....com
Subject: Re: Re: [PATCH v3 4/4] media: ox05b1s: Add support for Omnivision
 OS08A20 raw sensor

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
>