Re: [PATCH] platform/x86: x86-android-tablets: Add Lenovo Yoga Tab 3 (YT3-X90F) charger + fuel-gauge data

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

Hi,

On 11/28/22 12:03, Andy Shevchenko wrote:
> On Mon, Nov 28, 2022 at 11:44:36AM +0100, Hans de Goede wrote:
>> On 11/28/22 11:20, Andy Shevchenko wrote:
>>> On Sun, Nov 27, 2022 at 07:24:58PM +0100, Hans de Goede wrote:
> 
> ...
> 
>>>> +	/*
>>>> +	 * The "bq25892_0" charger IC has its /CE (Charge-Enable) and OTG pins
>>>> +	 * connected to GPIOs, rather then having them hardwired to the correct
>>>> +	 * values as is normally done.
>>>> +	 *
>>>> +	 * The bq25890_charger driver controls these through I2C, but this only
>>>> +	 * works if not overridden by the pins. Set these pins here:
>>>> +	 * 1. Set /CE to 0 to allow charging.
>>>
>>> If I read this correctly then the /CE is an active low pin and setting to 0
>>> means active state
>>
>> Correct.
>>
>>> which...
>>>
>>>> +	 * 2. Set OTG to 0 disable V5 boost output since the 5V boost output of
>>>> +	 *    the main "bq25892_1" charger is used when necessary.
>>>> +	 */
>>>> +
>>>> +	/* /CE pin */
>>>> +	ret = x86_android_tablet_get_gpiod("INT33FF:02", 22, &gpiod);
>>>> +	if (ret < 0)
>>>> +		return ret;
>>>
>>>> +	gpiod_set_value(gpiod, 0);
>>>
>>> ...contradicts with the virtual state here. Perhaps you missed the
>>> corresponding flag to enable negation?
>>
>> x86_android_tablet_get_gpiod() gets the GPIO directly from
>> the gpio-chip using gpiochip_get_desc() since these GPIOs are
>> not described in ACPI. There is no option to pass a gpio_lookup_flags
>> flag like GPIO_ACTIVE_LOW this way since we are not doing an actual lookup.
> 
> gpiod_toggle_active_low() is your friend, no?

Note that the GPIO is never actually requested and doing
gpiod_toggle_active_low() on a non requested gpio_desc is not nice.

Normally the GPIO_ACTIVE_LOW flag gets cleared on gpiod_free() to
leave it in a clean state for any future users, so we would need to
do something like:

gpiod_toggle_active_low(gpiod);
gpiod_set_value(gpiod, 1);
gpiod_toggle_active_low(gpiod);

or actually request the GPIO, which means adding an lenovo_yt3_exit()
to unrequest them; and adding a global lenovo_yt3_gpios[] variable
to store the descs in between init + exit.

This is something which I did consider, since it would also list
the GPIOs in /sys/kernel/debug/gpio which would be somewhat nice,
otoh it is a bunch of extra code just for getting the GPIOs
listed in  debugfs file.

Still if you really want me to mark it as active-low I believe
that doing a proper request of the GPIO + free on exit() is
the right way to go here.  That or just leave this as is in
this version 1 of this patch.

Please let me know how you want to proceed with this.

Regards,

Hans







> 
>>>> +	ret = x86_android_tablet_get_gpiod("INT33FF:03", 19, &gpiod);
>>>> +	if (ret < 0)
>>>> +		return ret;
>>>> +
>>>> +	gpiod_set_value(gpiod, 0);
>