Re: [PATCH v3 1/3] dt-bindings: hwmon: Add Infineon TDA38640

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

On 16/08/2023 10:51, Naresh Solanki wrote:
> Hi Krzysztof,
> 
> On Tue, 15 Aug 2023 at 01:02, Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
>>
>> On 11/08/2023 18:00, Naresh Solanki wrote:
>>> Hi,
>>>
>>> On Tue, 8 Aug 2023 at 19:58, Conor Dooley <conor@kernel.org> wrote:
>>>>
>>>> On Tue, Aug 08, 2023 at 07:10:08AM -0700, Guenter Roeck wrote:
>>>>> On 8/8/23 04:46, Conor Dooley wrote:
>>>>>> On Wed, Aug 02, 2023 at 09:31:51PM +0200, Naresh Solanki wrote:
>>>>>>> From: Patrick Rudolph <patrick.rudolph@9elements.com>
>>>>>>>
>>>>>>> The TDA38640 chip has different output control mechanisms depending on
>>>>>>> its mode of operation. When the chip is in SVID mode, only
>>>>>>> hardware-based output control is supported via ENABLE pin. However, when
>>>>>>> it operates in PMBus mode, software control works perfectly.
>>>>>>>
>>>>>>> To enable software control as a workaround in SVID mode, add the DT
>>>>>>> property 'infineon,en-svid-control'. This property will enable the
>>>>>>> workaround, which utilizes ENABLE pin polarity flipping for output when
>>>>>>> the chip is in SVID mode.
>>>>>>
>>>>>> Why do you need a custom property for this? How come it is not possible
>>>>>> to determine what bus you are on?
>>>>>>
>>>>>
>>>>> That is not the point. Yes, it can be detected if the control method is
>>>>> PMBus or SVID. However, in SVID mode, SVID is supposed to control the
>>>>> output, not PMBUs. This is bypassed by controlling the polarity of the
>>>>> (physical) output enable signal. We do _not_ want this enabled automatically
>>>>> in SVID mode. Its side effects on random boards using this chip are unknown.
>>>>> Thus, this needs a property which specifically enables this functionality
>>>>> for users who _really_ need to use it and (hopefully) know what they are
>>>>> doing.
>>>>
>>>> Hmm, reading this it makes a lot more sense why this is a property - I
>>>> guess I just struggled to understand the commit message here,
>>>> particularly what the benefit of using the workaround is. I'm still
>>>> having difficulty parsing the commit & property text though - its
>>>> unclear to me when you would need to use it - so I will stay out
>>>> of the way & let Rob or Krzysztof handle things.
>>>
>>> To provide context, my system employs a unique power sequence
>>> strategy utilizing a BMC (Baseboard Management Controller),
>>> rendering the reliance on the ENABLE pin unnecessary.
>>> In this configuration, the ENABLE pin is grounded in the hardware.
>>> While most regulators facilitate PMBus Operation for output control,
>>> the TDA38640 chip, when in SVID mode, is constrained by the
>>> ENABLE pin to align with Intel specifications.
>>> My communication with Infineon confirmed that the recommended
>>> approach is to invert the Enable Pin for my use case.
>>>
>>> Since this is not typically the use case for most setup & hence DT property
>>> is must for enabling the special case.
>>>
>>> For further insight into my setup's power sequence strategy, you can
>>> refer to the following link: https://github.com/9elements/pwrseqd
>>>
>>
>> This justifies to me the property, but still you described desired
>> driver behavior, not the hardware characteristic. Don't describe what
>> you want to control, but describe the entire system.
> I guess by entire system you mean how the regulators(including
> TDA38640) connected & operated in our setup ?

I mean, property name and description should say what is the
characteristic of the hardware/firmware/entire system.


Best regards,
Krzysztof