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

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

Hi,

On 12/16/22 15:57, Andy Shevchenko wrote:
> On Fri, Dec 16, 2022 at 12:30:13PM +0100, Hans de Goede 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,
> 
> Here and in the code you actually can refer to it as 3rd byte.

3th byte or 4th byte? There is no universal convention for numbering
bytes in a word, so just using Bits 31-24 is unambiguous.

> 
>> 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.
> 
> ...
> 
>> +	/* If bits 31-24 of the _DSM entry are all 0 then the signal is inverted */
> 
>> +	active_value = obj->integer.value >> 24;
>> +	if (!active_value)
> 
> Not sure why you need a temporary variable for this. Just use
> GENMASK()/GENMASK_ULL()?
> 
> 	if (obj->integer.value & GENMASK(31, 24));
> 
> In this case you even don't need to repeat bit numbers in the comment.

These bits contain the value to which the pin should be set when the
sensor is active (on), the active_value helper variable IMHO makes this
a lot more clear then directly checking the mask.

> 
>> +		polarity ^= GPIO_ACTIVE_LOW;
> 
>> +	dev_dbg(int3472->dev, "%s %s pin %d active-%s\n", func,
>> +		agpio->resource_source.string_ptr, agpio->pin_table[0],
>> +		(polarity == GPIO_ACTIVE_HIGH) ? "high" : "low");
> 
> Yet another high/low :-)

Nope, this is the same patch as last time (when you were fine with
the other bits...).

Regards,

Hans