Re: [PATCH v2 2/2] hwmon: (pmbus/tda38640) Add workaround for bug in SVID mode

[Guenter Roeck <linux@roeck-us.net>]

On 8/1/23 11:46, Naresh Solanki wrote:
> Hi Guenter,
> 
>>
>> On 8/1/23 05:34, Naresh Solanki wrote:
>>
>> [ ... ]
>>
>>>>> +     if (IS_ENABLED(CONFIG_SENSORS_TDA38640_REGULATOR) || svid) {
>>>>
>>>> If you hide this behind IS_ENABLED(CONFIG_SENSORS_TDA38640_REGULATOR), reading
>>>> svid outside the if() statement has no value.
>>> svid mode check is needed only when regulator is enabled for on/off
>>> control later.
>>> Will align the code such that if svid_mode check is done only when
>>> REGULATOR config is enabled
>>> & if it is in svid mode then apply the WA.
>>>
>>>>
>>>>> +             /*
>>>>> +              * Apply ON_OFF_CONFIG workaround as enabling the regulator using the
>>>>> +              * OPERATION register doesn't work in SVID mode.
>>>>> +              *
>>>>> +              * One should configure PMBUS_ON_OFF_CONFIG here, but
>>>>> +              * PB_ON_OFF_CONFIG_POWERUP_CONTROL and PB_ON_OFF_CONFIG_EN_PIN_REQ
>>>>> +              * are ignored by the device.
>>>>> +              * Only PB_ON_OFF_CONFIG_POLARITY_HIGH has an effect.
>>>>
>>>> Hmm, maybe I start to understand. This is really weird, since it changes
>>>> the polarity of the EN input pin, effectively reverting its value.
>>>> In other words, what really happens is that it is not possible to disable
>>>> the chip with PMBUS_ON_OFF_CONFIG in SVID mode, and that reverting
>>>> the EN pin polarity effectively simulates turning the chip on or off by
>>>> software. Maybe software enable is disabled on purpose in VID mode.
>>>> Is that really a bug or is it a feature, and is it really a good idea to
>>>> override it ?
>>> By design, SVID mode only has HW control enabled.
>>> This was with the assumption that PGOOD will be used for controlling
>>> Enable of another rail in Hardware.
>>>
>>> Since my use case needs the complete PMBUS based control,
>>> EN pin polarity flipping can be used for controlling output.
>>>
>>
>> So, effectively, this is not really a bug. It is working around chip functionality.
>>
>> That means we can not just enable this unconditionally in SVID mode after all.
>> Sorry, but it has to be configurable after all, with appropriate explanation.
> By 'configurable' you mean add a dt-property like 'en-svid-control' to have this
> enabled ?
> 

Yes. After all, not everyone may want this, and it is quite obvious to me that
it is not a bug but normal operation for the chip in SVID mode. Besides, it is
difficult to determine how the chip performs when manipulating the EN input
in SVID mode; I don't understand SVID well enough to be able to determine if doing
that is even valid in that mode. So we definitely don't want to enable this
for other users of the chip.

Sorry, my initial understanding was that this is a bug. I am now convinced
that it isn't.

Guenter

> Regards,
> Naresh
>>
>> Guenter
>>
>>>>
>>>> AN_2203_PL12_2204_184108 might really help here.
>>>>
>>>> Guenter
>>>>
>>>>> +              */
>>>>> +             data->info.read_byte_data = tda38640_read_byte_data;
>>>>> +             data->info.write_byte_data = tda38640_write_byte_data;
>>>>> +     }
>>>>> +     return pmbus_do_probe(client, &data->info);
>>>>>     }
>>>>>
>>>>>     static const struct i2c_device_id tda38640_id[] = {
>>>>
>>