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

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

[Hans de Goede]

The Lenovo Yoga Tab 3 (YT3-X90F) is an Intel Cherry Trail based tablet
which ships with Android as Factory OS. Its DSDT contains a bunch of I2C
devices which are not actually there, causing various resource conflicts.
Use acpi_quirk_skip_i2c_client_enumeration() to not enumerate these.

The YT3-X90F has quite a bit of exotic hardware, this adds initial
support by manually instantiating the i2c-clients for the 2 charger +
2 fuel-gauge chips used for the 2 batteries.

Support for other parts of the hw will be added by follow-up patches.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/platform/x86/x86-android-tablets.c | 129 ++++++++++++++++++++-
 1 file changed, 128 insertions(+), 1 deletion(-)

diff --git a/drivers/platform/x86/x86-android-tablets.c b/drivers/platform/x86/x86-android-tablets.c
index f04e06eeb958..dd933cf32b38 100644
--- a/drivers/platform/x86/x86-android-tablets.c
+++ b/drivers/platform/x86/x86-android-tablets.c
@@ -5,7 +5,7 @@
  * devices typically have a bunch of things hardcoded, rather than specified
  * in their DSDT.
  *
- * Copyright (C) 2021 Hans de Goede <hdegoede@redhat.com>
+ * Copyright (C) 2021-2022 Hans de Goede <hdegoede@redhat.com>
  */
 
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
@@ -987,6 +987,124 @@ static void lenovo_yoga_tab2_830_1050_exit(void)
 	}
 }
 
+/* Lenovo Yoga Tab 3 Pro YT3-X90F */
+
+/*
+ * There are 2 batteries, with 2 bq27500 fuel-gauges and 2 bq25892 chargers,
+ * "bq25890-charger-1" is instantiated from: drivers/i2c/busses/i2c-cht-wc.c.
+ */
+static const char * const lenovo_yt3_bq25892_0_suppliers[] = { "cht_wcove_pwrsrc" };
+static const char * const bq25890_1_psy[] = { "bq25890-charger-1" };
+
+static const struct property_entry fg_bq25890_1_supply_props[] = {
+	PROPERTY_ENTRY_STRING_ARRAY("supplied-from", bq25890_1_psy),
+	{ }
+};
+
+static const struct software_node fg_bq25890_1_supply_node = {
+	.properties = fg_bq25890_1_supply_props,
+};
+
+/* bq25892 charger settings for the flat lipo battery behind the screen */
+static const struct property_entry lenovo_yt3_bq25892_0_props[] = {
+	PROPERTY_ENTRY_STRING_ARRAY("supplied-from", lenovo_yt3_bq25892_0_suppliers),
+	PROPERTY_ENTRY_STRING("linux,power-supply-name", "bq25892-second-chrg"),
+	PROPERTY_ENTRY_U32("linux,iinlim-percentage", 40),
+	PROPERTY_ENTRY_BOOL("linux,skip-reset"),
+	/* Values taken from Android Factory Image */
+	PROPERTY_ENTRY_U32("ti,charge-current", 2048000),
+	PROPERTY_ENTRY_U32("ti,battery-regulation-voltage", 4352000),
+	PROPERTY_ENTRY_U32("ti,termination-current", 128000),
+	PROPERTY_ENTRY_U32("ti,precharge-current", 128000),
+	PROPERTY_ENTRY_U32("ti,minimum-sys-voltage", 3700000),
+	PROPERTY_ENTRY_U32("ti,boost-voltage", 4998000),
+	PROPERTY_ENTRY_U32("ti,boost-max-current", 500000),
+	PROPERTY_ENTRY_BOOL("ti,use-ilim-pin"),
+	{ }
+};
+
+static const struct software_node lenovo_yt3_bq25892_0_node = {
+	.properties = lenovo_yt3_bq25892_0_props,
+};
+
+static const struct x86_i2c_client_info lenovo_yt3_i2c_clients[] __initconst = {
+	{
+		/* bq27500 fuel-gauge for the flat lipo battery behind the screen */
+		.board_info = {
+			.type = "bq27500",
+			.addr = 0x55,
+			.dev_name = "bq27500_0",
+			.swnode = &fg_bq25890_supply_node,
+		},
+		.adapter_path = "\\_SB_.PCI0.I2C1",
+	}, {
+		/* bq25892 charger for the flat lipo battery behind the screen */
+		.board_info = {
+			.type = "bq25892",
+			.addr = 0x6b,
+			.dev_name = "bq25892_0",
+			.swnode = &lenovo_yt3_bq25892_0_node,
+		},
+		.adapter_path = "\\_SB_.PCI0.I2C1",
+		.irq_data = {
+			.type = X86_ACPI_IRQ_TYPE_GPIOINT,
+			.chip = "INT33FF:01",
+			.index = 5,
+			.trigger = ACPI_EDGE_SENSITIVE,
+			.polarity = ACPI_ACTIVE_LOW,
+		},
+	}, {
+		/* bq27500 fuel-gauge for the round li-ion cells in the hinge */
+		.board_info = {
+			.type = "bq27500",
+			.addr = 0x55,
+			.dev_name = "bq27500_1",
+			.swnode = &fg_bq25890_1_supply_node,
+		},
+		.adapter_path = "\\_SB_.PCI0.I2C2",
+	}
+};
+
+static int __init lenovo_yt3_init(void)
+{
+	struct gpio_desc *gpiod;
+	int ret;
+
+	/*
+	 * 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.
+	 * 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);
+
+	/* OTG pin */
+	ret = x86_android_tablet_get_gpiod("INT33FF:03", 19, &gpiod);
+	if (ret < 0)
+		return ret;
+
+	gpiod_set_value(gpiod, 0);
+
+	return 0;
+}
+
+static const struct x86_dev_info lenovo_yt3_info __initconst = {
+	.i2c_client_info = lenovo_yt3_i2c_clients,
+	.i2c_client_count = ARRAY_SIZE(lenovo_yt3_i2c_clients),
+	.init = lenovo_yt3_init,
+};
+
 /* Medion Lifetab S10346 tablets have an Android factory img with everything hardcoded */
 static const char * const medion_lifetab_s10346_accel_mount_matrix[] = {
 	"0", "1", "0",
@@ -1327,6 +1445,15 @@ static const struct dmi_system_id x86_android_tablet_ids[] __initconst = {
 		},
 		.driver_data = (void *)&lenovo_yoga_tab2_830_1050_info,
 	},
+	{
+		/* Lenovo Yoga Tab 3 Pro YT3-X90F */
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "Intel Corporation"),
+			DMI_MATCH(DMI_PRODUCT_NAME, "CHERRYVIEW D1 PLATFORM"),
+			DMI_MATCH(DMI_PRODUCT_VERSION, "Blade3-10A-001"),
+		},
+		.driver_data = (void *)&lenovo_yt3_info,
+	},
 	{
 		/* Medion Lifetab S10346 */
 		.matches = {
-- 
2.38.1

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

[Andy Shevchenko]

On Sun, Nov 27, 2022 at 07:24:58PM +0100, Hans de Goede wrote:
> The Lenovo Yoga Tab 3 (YT3-X90F) is an Intel Cherry Trail based tablet
> which ships with Android as Factory OS. Its DSDT contains a bunch of I2C
> devices which are not actually there, causing various resource conflicts.
> Use acpi_quirk_skip_i2c_client_enumeration() to not enumerate these.
> 
> The YT3-X90F has quite a bit of exotic hardware, this adds initial
> support by manually instantiating the i2c-clients for the 2 charger +
> 2 fuel-gauge chips used for the 2 batteries.
> 
> Support for other parts of the hw will be added by follow-up patches.

...

> +	/*
> +	 * 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 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?

> +	/* OTG pin */
> +	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

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

[Hans de Goede]

Hi Andy,

On 11/28/22 11:20, Andy Shevchenko wrote:
> On Sun, Nov 27, 2022 at 07:24:58PM +0100, Hans de Goede wrote:
>> The Lenovo Yoga Tab 3 (YT3-X90F) is an Intel Cherry Trail based tablet
>> which ships with Android as Factory OS. Its DSDT contains a bunch of I2C
>> devices which are not actually there, causing various resource conflicts.
>> Use acpi_quirk_skip_i2c_client_enumeration() to not enumerate these.
>>
>> The YT3-X90F has quite a bit of exotic hardware, this adds initial
>> support by manually instantiating the i2c-clients for the 2 charger +
>> 2 fuel-gauge chips used for the 2 batteries.
>>
>> Support for other parts of the hw will be added by follow-up patches.
> 
> ...
> 
>> +	/*
>> +	 * 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.

Regards,

Hans




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

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

[Andy Shevchenko]

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?

> >> +	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

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

[Hans de Goede]

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

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

[Andy Shevchenko]

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

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

[Hans de Goede]

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