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);
>
