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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





> -----Original Message-----
> From: Dmitry Torokhov [mailto:dmitry.torokhov@xxxxxxxxx]
> Sent: 12 October, 2015 19:48
> To: Tirdea, Irina
> Cc: Bastien Nocera; Aleksei Mamlin; Karsten Merker; linux-input@xxxxxxxxxxxxxxx; Mark Rutland; Purdila, Octavian; linux-
> kernel@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH v9 2/9] Input: goodix - reset device at init
> 
> On Mon, Oct 12, 2015 at 06:24:30PM +0300, 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 properly 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. This functionality will not be
> > available for devices that use indexed gpio pins declared in the _CRS
> > section (we need to provide backward compatibility with devices
> > that do not support using the interrupt gpio pin as output).
> >
> > For ACPI, the pins can be specified using ACPI 5.1:
> > Device (STAC)
> > {
> >     Name (_HID, "GDIX1001")
> >     ...
> >
> >     Method (_CRS, 0, Serialized)
> >     {
> >         Name (RBUF, ResourceTemplate ()
> >         {
> >             I2cSerialBus (0x0014, ControllerInitiated, 0x00061A80,
> >                 AddressingMode7Bit, "\\I2C0",
> >                 0x00, ResourceConsumer, ,
> >                 )
> >
> >             GpioInt (Edge, ActiveHigh, Exclusive, PullNone, 0x0000,
> >                 "\\I2C0", 0x00, ResourceConsumer, ,
> >                  )
> >                  {   // Pin list
> >                      0
> >                  }
> >
> >             GpioIo (Exclusive, PullDown, 0x0000, 0x0000,
> >                 IoRestrictionOutputOnly, "\\I2C0", 0x00,
> >                 ResourceConsumer, ,
> >                 )
> >                 {
> >                      1
> >                 }
> >         })
> >         Return (RBUF)
> >     }
> >
> >     Name (_DSD,  Package ()
> >     {
> >         ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> >         Package ()
> >         {
> >             Package (2) {"irq-gpio", Package() {^STAC, 0, 0, 0 }},
> >             Package (2) {"reset-gpio", Package() {^STAC, 1, 0, 0 }},
> >             ...
> >         }
> >     }
> >
> > Signed-off-by: Octavian Purdila <octavian.purdila@xxxxxxxxx>
> > Signed-off-by: Irina Tirdea <irina.tirdea@xxxxxxxxx>
> > Acked-by: Bastien Nocera <hadess@xxxxxxxxxx>
> > Tested-by: Bastien Nocera <hadess@xxxxxxxxxx>
> > Tested-by: Aleksei Mamlin <mamlinav@xxxxxxxxx>
> > ---
> >  .../bindings/input/touchscreen/goodix.txt          |   5 +
> >  drivers/input/touchscreen/Kconfig                  |   1 +
> >  drivers/input/touchscreen/goodix.c                 | 101 +++++++++++++++++++++
> >  3 files changed, 107 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
> b/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
> > index 8ba98ee..7137881 100644
> > --- a/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
> > +++ b/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
> > @@ -12,6 +12,8 @@ Required properties:
> >   - reg			: I2C address of the chip. Should be 0x5d or 0x14
> >   - interrupt-parent	: Interrupt controller to which the chip is connected
> >   - interrupts		: Interrupt to which the chip is connected
> > + - irq-gpio		: GPIO pin used for IRQ
> > + - reset-gpio		: GPIO pin used for reset
> >
> >  Example:
> >
> > @@ -23,6 +25,9 @@ Example:
> >  			reg = <0x5d>;
> >  			interrupt-parent = <&gpio>;
> >  			interrupts = <0 0>;
> > +
> > +			irq-gpio = <&gpio1 0 0>;
> > +			reset-gpio = <&gpio1 1 0>;
> >  		};
> >
> >  		/* ... */
> > diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
> > index 771d95c..76f5a9d 100644
> > --- a/drivers/input/touchscreen/Kconfig
> > +++ b/drivers/input/touchscreen/Kconfig
> > @@ -324,6 +324,7 @@ config TOUCHSCREEN_FUJITSU
> >  config TOUCHSCREEN_GOODIX
> >  	tristate "Goodix I2C touchscreen"
> >  	depends on I2C
> > +	depends on GPIOLIB
> >  	help
> >  	  Say Y here if you have the Goodix touchscreen (such as one
> >  	  installed in Onda v975w tablets) connected to your
> > diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
> > index 56d0330..87304ac 100644
> > --- a/drivers/input/touchscreen/goodix.c
> > +++ b/drivers/input/touchscreen/goodix.c
> > @@ -16,6 +16,7 @@
> >
> >  #include <linux/kernel.h>
> >  #include <linux/dmi.h>
> > +#include <linux/gpio.h>
> >  #include <linux/i2c.h>
> >  #include <linux/input.h>
> >  #include <linux/input/mt.h>
> > @@ -37,8 +38,13 @@ struct goodix_ts_data {
> >  	unsigned int int_trigger_type;
> >  	bool rotated_screen;
> >  	int cfg_len;
> > +	struct gpio_desc *gpiod_int;
> > +	struct gpio_desc *gpiod_rst;
> >  };
> >
> > +#define GOODIX_GPIO_INT_NAME		"irq"
> > +#define GOODIX_GPIO_RST_NAME		"reset"
> > +
> >  #define GOODIX_MAX_HEIGHT		4096
> >  #define GOODIX_MAX_WIDTH		4096
> >  #define GOODIX_INT_TRIGGER		1
> > @@ -237,6 +243,88 @@ static irqreturn_t goodix_ts_irq_handler(int irq, void *dev_id)
> >  	return IRQ_HANDLED;
> >  }
> >
> > +static int goodix_int_sync(struct goodix_ts_data *ts)
> > +{
> > +	int error;
> > +
> > +	error = gpiod_direction_output(ts->gpiod_int, 0);
> > +	if (error)
> > +		return error;
> > +	msleep(50);				/* T5: 50ms */
> > +
> > +	return gpiod_direction_input(ts->gpiod_int);
> > +}
> > +
> > +/**
> > + * goodix_reset - Reset device during power on
> > + *
> > + * @ts: goodix_ts_data pointer
> > + */
> > +static int goodix_reset(struct goodix_ts_data *ts)
> > +{
> > +	int error;
> > +
> > +	/* begin select I2C slave addr */
> > +	error = gpiod_direction_output(ts->gpiod_rst, 0);
> > +	if (error)
> > +		return error;
> > +	msleep(20);				/* T2: > 10ms */
> > +	/* HIGH: 0x28/0x29, LOW: 0xBA/0xBB */
> > +	error = gpiod_direction_output(ts->gpiod_int, ts->client->addr == 0x14);
> > +	if (error)
> > +		return error;
> > +	usleep_range(100, 2000);		/* T3: > 100us */
> > +	error = gpiod_direction_output(ts->gpiod_rst, 1);
> > +	if (error)
> > +		return error;
> > +	usleep_range(6000, 10000);		/* T4: > 5ms */
> > +	/* end select I2C slave addr */
> > +	error = gpiod_direction_input(ts->gpiod_rst);
> > +	if (error)
> > +		return error;
> > +	return goodix_int_sync(ts);
> > +}
> > +
> > +/**
> > + * goodix_get_gpio_config - Get GPIO config from ACPI/DT
> > + *
> > + * @ts: goodix_ts_data pointer
> > + */
> > +static int goodix_get_gpio_config(struct goodix_ts_data *ts)
> > +{
> > +	int error;
> > +	struct device *dev;
> > +	struct gpio_desc *gpiod;
> > +
> > +	if (!ts->client)
> > +		return -EINVAL;
> > +	dev = &ts->client->dev;
> > +
> > +	/* Get the interrupt GPIO pin number */
> > +	gpiod = devm_gpiod_get(dev, GOODIX_GPIO_INT_NAME, GPIOD_IN);
> 
> Why isn't this devm_gpiod_get_optional()? Then you would not need to
> clobber the return value down in goodix_ts_probe().
> 

I did not use devm_gpiod_get_optional() in order to ignore more errors
than -ENOENT. This is needed because the ACPI gpio core will fall back
to indexed gpios if named gpios are not found. In the common case of
having 2 indexed gpio pins declared in the ACPI table, the first
devm_gpiod_get() will successfully get indexed gpio pin 0 and the
second devm_gpiod_get() will try to get the same gpio pin 0 and return
-EBUSY. Considering this, I thought it is better to just ignore all errors in
order not to break any platforms currently using this driver.

> I can fix it up locally.
> 

Sure, you can replace devm_gpiod_get with devm_gpiod_get_optional,
but the error handling code should remain the same.

Thanks,
Irina

> Thanks.
> 
> --
> Dmitry
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]
  Powered by Linux