Re: [PATCH v6 3/4] platform: int3472: discrete: con_id vana for Sony IMX471 as power enable

[Hans de Goede <hansg@kernel.org>]

Hi,

On 1-Jul-26 14:33, Tarang Raval wrote:
> Hi Hans, Sakari
> 
>> On Wed, Jul 01, 2026 at 01:01:58PM +0200, Hans de Goede wrote:
>>> Hi,
>>>
>>> On 1-Jul-26 08:19, Tarang Raval wrote:
>>>> Hi Hans,
>>>>
>>>>> On 30-Jun-26 09:32, Tarang Raval wrote:
>>>>>> Hi Kate,
>>>>>>
>>>>>>> Update the con_id for the Sony IMX471 sensor to "vana" to serve as the
>>>>>>> power enable. Additionally, the HID values SONY471A and TBE20A0, both
>>>>>>> associated with the IMX471 image sensor, have been identified on Lenovo
>>>>>>> laptops.
>>>>>>>
>>>>>>> Signed-off-by: Kate Hsuan <hpa@redhat.com>
>>>>>>
>>>>>> Thanks, looks good.
>>>>>>
>>>>>> Reviewed-by: Tarang Raval <tarang.raval@siliconsignals.io>
>>>>>
>>>>> Hmm, the imx471 driver is still pending upstream:
>>>>>
>>>>> https://lore.kernel.org/linux-media/20260629074026.35490-5-hpa@redhat.com/
>>>>>
>>>>> As part of this series.
>>>>>
>>>>> Please just use the standardized "avdd" in that driver instead
>>>>> of "vana" (which also seems to refer to the analog supply vdd,
>>>>> which is what avdd stands for).
>>>>>
>>>>> Then this whole patch is unnecessary and can be dropped from
>>>>> this series.
>>>>
>>>> The regulator name "vana" comes directly from the Sony IMX471 sensor
>>>> datasheet, which typically refers to the analog supply voltage. Using the
>>>> datasheet name helps keep the driver consistent with the hardware
>>>> documentation and makes it easier to cross-reference.
>>>>
>>>> as per my understanding, the more standardized way is to use the regulator
>>>> name as per the sensor datasheet. Therefore, I respectfully disagree with
>>>> your suggestion.
>>>
>>> As shown by the need for this patch on x86 at least because there
>>> is no devicetree it greatly helps if all Linux sensor drivers use
>>> standardized names for their regulators rather then using the exact name
>>> from the datasheet which often is not very consistent.
>>>
>>> And "avdd" is the name we've standardized on for this, so lets use that:
>>>
>>> hans@shalem:~/projects/linux$ grep -l '"vana"' drivers/media/i2c/*.c | wc -l
>>> 4
>>> hans@shalem:~/projects/linux$ grep -l '"avdd"' drivers/media/i2c/*.c | wc -l
>>> 36
>>>
>>> The alternative is needing to add more and more quirks as different
>>> sensors are used, which is not great.
>>
>> I do agree that having a constant name for the regulators would be
>> beneficial for the int3472 driver. Still, if, and presumably, when that
>> sensor gets DT support, the bindings will use the regulator name from the
>> datasheet.
>>
>> Let's just use the datasheet name now and add the few lines needed to the
>> int3472 driver and avoid the churn in the future. There's a limited number
>> of sensor drivers that need this after all.
>>
>> I'd be more concerned of what's going on in tps68470_board_data.c for
>> instance.
> 
> I went through the INT3472 driver and would like to propose a generic     
> approach that satisfies both sides without per-HID quirks or sensor driver
> changes.                                                                  
>                                                                           
> The problem is:                                                      
>  - INT3472 standardizes on "avdd" internally                             
>  - Sony IMX sensor drivers use "vana" per datasheet, and all existing    
>    Sony DT bindings (imx219, imx290, imx415) already use vana-supply     
>  - Changing imx471 to "avdd" now will create inconsistency with those    
>    bindings, or require a rename later

Ack, as mentioned in my reply to Sakari from 1 minute ago I'm ok
with sticking with vana for the imx* case,
                                  
> Instead of fixing this per-sensor (either by changing the driver or adding
> a per-HID entry to int3472_gpio_map), we can add a small vendor alias     
> table inside skl_int3472_register_regulator().                            
>                                                                           
> Currently that function registers two consumer supply entries per sensor: 
> lowercase ("avdd") and uppercase ("AVDD"). We can extend it to also       
> register vendor datasheet aliases from a static table, e.g.:              
>                                                                           
>   avdd -> vana   (Sony IMX series: imx219, imx290, imx415, imx471, ...)   
>                                                                           
> This way, when a sensor driver requests "vana", the regulator framework   
> finds it in the supply map without any extra quirks.                      

Interesting proposal. For now I think just going with the quirk from
this patch is fine. But if we get more Sony sensors doing what you
suggests might make sense.

IIRC currently all regulators get an upper-cased alias to avoid needing
quirks just for the case where drivers only differ in case.

Special casing avdd is going to require adding special code to the generic
bits. Or maybe just an alt_name parameter ? Either way I think for just
the one sensor the quirk is fine. But if we get more (which is somewhat
likely) then your suggestion is probably a good idea.

Regards,

Hans