RE: [PATCH v7 2/9] Input: goodix - reset device at init

[Tirdea, Irina]

> -----Original Message-----
> From: Arnd Bergmann [mailto:arnd@xxxxxxxx]
> Sent: 08 October, 2015 16:01
> To: Tirdea, Irina
> Cc: Dmitry Torokhov; Bastien Nocera; Aleksei Mamlin; linux-input@xxxxxxxxxxxxxxx; Mark Rutland; Purdila, Octavian; linux-
> kernel@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH v7 2/9] Input: goodix - reset device at init
> 
> On Thursday 08 October 2015 12:18:37 Tirdea, Irina wrote:
> > > From: Arnd Bergmann [mailto:arnd@xxxxxxxx]
> > > Sent: 08 October, 2015 13:54
> > > To: Tirdea, Irina
> > > Cc: Dmitry Torokhov; Bastien Nocera; Aleksei Mamlin; linux-input@xxxxxxxxxxxxxxx; Mark Rutland; Purdila, Octavian; linux-
> > > kernel@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx
> > > Subject: Re: [PATCH v7 2/9] Input: goodix - reset device at init
> > >
> > > On Thursday 08 October 2015 13:19:28 Irina Tirdea wrote:
> > > > After power on, it is recommended that the driver resets the device.
> > > > The reset procedure timing is described in the datasheet and is used
> > > > at device init (before writing device configuration) and
> > > > for power management. It is a sequence of setting the interrupt
> > > > and reset pins high/low at specific timing intervals. This procedure
> > > > also includes setting the slave address to the one specified in the
> > > > ACPI/device tree.
> > > >
> > > > This is based on Goodix datasheets for GT911 and GT9271 and on Goodix
> > > > driver gt9xx.c for Android (publicly available in Android kernel
> > > > trees for various devices).
> > > >
> > > > For reset the driver needs to control the interrupt and
> > > > reset gpio pins (configured through ACPI/device tree). For devices
> > > > that do not have the gpio pins declared, the functionality depending
> > > > on these pins will not be available, but the device can still be used
> > > > with basic functionality.
> > > >
> > > > For both device tree and ACPI, the interrupt gpio pin configuration is
> > > > read from the "irq-gpio" property and the reset pin configuration is
> > > > read from the "reset-gpio" property. For ACPI 5.1, named properties
> > > > can be specified using the _DSD section. If there is no _DSD section
> > > > in the ACPI table, the driver will fall back to using indexed gpio
> > > > pins declared in the _CRS section.
> > >
> > > Would it help to use a plain "gpios" property here to always look
> > > up the lines by index?
> > >
> >
> > The problem with ACPI indexed gpios is that platforms declare the
> > pins in random order. In this case we have some platforms that declare
> > the interrupt pin first and others that declare the reset pin first.
> > There is no way to differentiate between them so the only way to support
> > these platforms is to pick a default and list all exceptions in the driver.
> > My previous implementation did that with indexed gpios and dmi quirks. [1]
> >
> > This can be solved by using named gpios, which are available starting with ACPI 5.1.
> > In this way we know exactly which is the interrupt pin and which is the reset pin
> > and we do not need to add any additional exceptions to the driver.
> > However, we still need to support the platforms that are already out there so
> > we fall back to indexed gpios.
> >
> > [1] https://lkml.org/lkml/2015/9/15/609
> 
> Right.
> 
> > > > +/*
> > > > + * Some platforms specify the gpio pins for interrupt and reset properly
> > > > + * in ACPI, but cannot use the interrupt pin as output due to their specific
> > > > + * HW configuration.
> > > > + */
> > > > +static const struct dmi_system_id goodix_no_gpio_pins_support[] = {
> > > > +#if defined(CONFIG_DMI) && defined(CONFIG_X86)
> > > > +	{
> > > > +		.ident = "Onda v975w",
> > > > +		.matches = {
> > > > +			DMI_MATCH(DMI_BIOS_VENDOR, "American Megatrends Inc."),
> > > > +			DMI_MATCH(DMI_PRODUCT_UUID,
> > > > +				  "03000200-0400-0500-0006-000700080009"),
> > > > +			DMI_MATCH(DMI_BOARD_VENDOR, "AMI Corporation"),
> > > > +			DMI_MATCH(DMI_BOARD_NAME, "Aptio CRB"),
> > > > +		}
> > > > +	},
> > >
> > > I think lists like this in drivers should be avoided if at all possible,
> > > it just leads to other people adding their platform in the lists as
> > > opposed to fixing their boot loaders.
> > >
> > > Can you find another way to detect at runtime whether it works, and
> > > print a warning if it doesn't?
> >
> > I agree with you on this, but unfortunately I have not found a better way to do it.
> >
> > The main problem comes from the interrupt pin. This device uses the interrupt pin
> > as output, which some platforms do not support (either due to the HW configuration
> > or due to flagging it wrong in BIOS) [2] [3]. There is no error returned, just a warning
> > in dmesg.
> >
> > [2] https://lkml.org/lkml/2015/9/28/851
> > [3] https://lkml.org/lkml/2015/9/30/607
> 
> Would it be possible to combine those two and require future firmware to
> use the named gpios if they want the proper reset, but fall back to not
> doing it if they use an anonymous list of GPIOs?
> 

Yes, that would work.
I will send a new version that includes this change.

Thanks,
Irina

> > > > +	/* HIGH: 0x28/0x29, LOW: 0xBA/0xBB */
> > > > +	error = gpiod_direction_output(ts->gpiod_int, ts->client->addr == 0x14);
> > > > +	if (error)
> > > > +		return error;
> > >
> > > If the "interrupt" gpio is used as an output, maybe it has the wrong
> > > name? Is that the name from the goodix datasheet (that would be ok)
> > > or something you picked?
> > >
> >
> > This is from the goodix datasheet [4]. The pin that is used for receiving interrupts
> > is also used as output (for reset and suspend procedures).
> >
> > [4]
> https://drive.google.com/folderview?id=0BxCVOQS3ZymGfmJyY2RKbE5XbVlKNlktVTlwV0lxNEdxd2dzeWZER094cmJPVnMxN1F0Yzg&;
> usp=sharing
> >
> 
> Ok.
> 
> 	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/