Hi,
On 11/28/22 12:38, Andy Shevchenko wrote:
> 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.
Ok, I've amended the comment to make things more clear and merged this
into my review-hans branch now.
Regards,
Hans
>
>>>>>> + ret = x86_android_tablet_get_gpiod("INT33FF:03", 19, &gpiod);
>>>>>> + if (ret < 0)
>>>>>> + return ret;
>>>>>> +
>>>>>> + gpiod_set_value(gpiod, 0);
>
