Re: [PATCH 6/6] platform/x86: int3472/discrete: Get the polarity from the _DSM entry

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 11/30/22 12:06, Andy Shevchenko wrote:
> On Wed, Nov 30, 2022 at 11:39:42AM +0100, Hans de Goede wrote:
>> On 11/30/22 11:01, Andy Shevchenko wrote:
>>> On Wed, Nov 30, 2022 at 1:12 AM Hans de Goede <hdegoede@redhat.com> wrote:
>>>>
>>>> According to:
>>>> https://github.com/intel/ipu6-drivers/blob/master/patch/int3472-support-independent-clock-and-LED-gpios-5.17%2B.patch
>>>>
>>>> Bits 31-24 of the _DSM pin entry integer value codes the active-value,
>>>> that is the actual physical signal (0 or 1) which needs to be output on
>>>> the pin to turn the sensor chip on (to make it active).
>>>>
>>>> So if bits 31-24 are 0 for a reset pin, then the actual value of the reset
>>>> pin needs to be 0 to take the chip out of reset. IOW in this case the reset
>>>> signal is active-high rather then the default active-low.
>>>>
>>>> And if bits 31-24 are 0 for a clk-en pin then the actual value of the clk
>>>> pin needs to be 0 to enable the clk. So in this case the clk-en signal
>>>> is active-low rather then the default active-high.
>>>>
>>>> IOW if bits 31-24 are 0 for a pin, then the default polarity of the pin
>>>> is inverted.
>>>>
>>>> Add a check for this and also propagate this new polarity to the clock
>>>> registration.
>>>
>>> I like it in this form, thanks!
>>>
>>> ...
>>>
>>>> +               (polarity == GPIO_ACTIVE_HIGH) ? "high" : "low");
>>>
>>> Perhaps
>>>
>>> static inline str_high_low(bool v)
>>> {
>>>   return v ? "high" : "low";
>>> }
>>>
>>> In the string_helpers.h? If you are okay with the idea, you may use my
>>> tag ahead for that patch.
>>
>> That is an interesting idea. but IMHO best done in a follow-up series
>> after checking for similar code in the rest of the kernel.
>>
>> This is based on the assumption that "selling" this new helper to
>> the string_helpers.h maintainer will be easier if there are multiple
>> consumers of it right away.
> 
> strings_helper.h maintainer is any known kernel subsystem maintainer + me
> (as a reviewer / main contributor to that library). That's why I proposed
> the solution and my tag would be enough to route it via your tree.
> 
> So, offer still stays.

Ok I'll add your patch adding the str_high_low() helper to the next
version of this series (or when I merge it, depending on how things go)
and then move this over to use the str_high_low() helper.

Regards,

Hans