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

[Andy Shevchenko <andy@kernel.org>]

On Mon, Nov 28, 2022 at 12:24:59PM +0100, Hans de Goede wrote:
> 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.

I do not insist, but my objection here is the terminology (active state,
inactive state vs. 0, 1 or other way around) and inconsistency with what you
put as a value and what comment says taking into account / (or
negation) of the real signal.

Ideally yes, would be nice to have it indeed requested since it's in use even
from the Linux kernel perspective (one may debug its usage or see via user
space, note as well that non-requested pin maybe easily altered in the Linux).

But if you guarantee nothing of this happens, feel free to amend the comment
to make it more clear and proceed.

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

-- 
With Best Regards,
Andy Shevchenko