> -----Original Message----- > From: Dmitry Torokhov [mailto:dmitry.torok...@gmail.com] > Sent: 12 October, 2015 19:48 > To: Tirdea, Irina > Cc: Bastien Nocera; Aleksei Mamlin; Karsten Merker; > linux-in...@vger.kernel.org; Mark Rutland; Purdila, Octavian; linux- > ker...@vger.kernel.org; devicet...@vger.kernel.org > 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.purd...@intel.com> > > Signed-off-by: Irina Tirdea <irina.tir...@intel.com> > > Acked-by: Bastien Nocera <had...@hadess.net> > > Tested-by: Bastien Nocera <had...@hadess.net> > > Tested-by: Aleksei Mamlin <mamli...@gmail.com> > > --- > > .../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 linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
