| Message ID | 1435595304-4840-2-git-send-email-irina.tirdea@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, 2015-06-29 at 19:28 +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 declared, the functionality depending > on these pins will not be available, but the device can still be used > with basic functionality. I'm having a little trouble with this first patch, on a 4.2 "pre" Linus tree and on a 4.1 kernel. [ 6.720214] ------------[ cut here ]------------ [ 6.720230] WARNING: CPU: 2 PID: 475 at drivers/pinctrl/intel/pinctrl-baytrail.c:338 byt_gpio_direction_output+0x97/0xa0() [ 6.720234] Potential Error: Setting GPIO with direct_irq_en to output [ 6.720238] Modules linked in: [ 6.720241] regmap_i2c intel_soc_dts_iosf int340x_thermal_zone soundcore industrialio battery iosf_mbi acpi_thermal_rel dell_smo8800 snd_soc_sst_acpi goodix_backport(OE+) i2c_hid acpi_pad ac rfkill_gpio i2c_designware_platform rfkill pwm_lpss_platform pwm_lpss i2c_designware_core nfsd auth_rpcgss nfs_acl lockd grace sunrpc i915 mmc_block i2c_algo_bit drm_kms_helper drm video sdhci_acpi sdhci mmc_core [ 6.720292] CPU: 2 PID: 475 Comm: systemd-udevd Tainted: G W OE 4.2.0-0.rc0.git2.2.fc22.i686 #1 [ 6.720295] Hardware name: To be filled by O.E.M. To be filled by O.E.M./Aptio CRB, BIOS 5.6.5 07/25/2014 [ 6.720299] c0d39967 11197204 00000000 f6d37bc8 c0a9ce3c f6d37c08 f6d37bf8 c045c677 [ 6.720311] c0cb62a4 f6d37c28 000001db c0cb62e0 00000152 c073bfc7 c073bfc7 f7c580b8 [ 6.720321] f441409c f7c580b0 f6d37c14 c045c6ee 00000009 f6d37c08 c0cb62a4 f6d37c28 [ 6.720331] Call Trace: [ 6.720342] [<c0a9ce3c>] dump_stack+0x41/0x52 [ 6.720349] [<c045c677>] warn_slowpath_common+0x87/0xc0 [ 6.720355] [<c073bfc7>] ? byt_gpio_direction_output+0x97/0xa0 [ 6.720360] [<c073bfc7>] ? byt_gpio_direction_output+0x97/0xa0 [ 6.720365] [<c045c6ee>] warn_slowpath_fmt+0x3e/0x60 [ 6.720370] [<c073bfc7>] byt_gpio_direction_output+0x97/0xa0 [ 6.720376] [<c073bf30>] ? byt_gpio_irq_handler+0xc0/0xc0 [ 6.720382] [<c073e939>] _gpiod_direction_output_raw+0x59/0x1c0 [ 6.720388] [<c0aa202d>] ? _raw_spin_unlock_irqrestore+0xd/0x10 [ 6.720393] [<c073bb73>] ? byt_gpio_direction_input+0x43/0x50 [ 6.720398] [<c073bb30>] ? byt_gpio_set+0x70/0x70 [ 6.720404] [<c073eb0a>] gpiod_direction_output+0x2a/0x50 [ 6.720413] [<f7fe768b>] goodix_ts_probe+0x2fb/0x5fd [goodix_backport] [ 6.720422] [<c0909691>] i2c_device_probe+0x101/0x1b0 [ 6.720428] [<c0612e95>] ? sysfs_create_link+0x25/0x50 [ 6.720436] [<f7fe7390>] ? goodix_ts_irq_handler+0x1f0/0x1f0 [goodix_backport] [ 6.720442] [<c0819c52>] ? driver_sysfs_add+0x62/0x80 [ 6.720448] [<c081a56a>] driver_probe_device+0x1ca/0x460 [ 6.720454] [<c081a800>] ? driver_probe_device+0x460/0x460 [ 6.720461] [<c078d0b1>] ? acpi_driver_match_device+0x36/0x3f [ 6.720467] [<c081a879>] __driver_attach+0x79/0x80 [ 6.720473] [<c081a800>] ? driver_probe_device+0x460/0x460 [ 6.720478] [<c0818467>] bus_for_each_dev+0x57/0xa0 [ 6.720484] [<c0819dfe>] driver_attach+0x1e/0x20 [ 6.720489] [<c081a800>] ? driver_probe_device+0x460/0x460 [ 6.720494] [<c08199bf>] bus_add_driver+0x1ef/0x290 [ 6.720501] [<f7ca7000>] ? 0xf7ca7000 [ 6.720506] [<f7ca7000>] ? 0xf7ca7000 [ 6.720512] [<c081b07d>] driver_register+0x5d/0xf0 [ 6.720518] [<c090a3ca>] i2c_register_driver+0x2a/0xa0 [ 6.720524] [<c040046f>] ? do_one_initcall+0x9f/0x200 [ 6.720531] [<f7ca7012>] goodix_ts_driver_init+0x12/0x1000 [goodix_backport] [ 6.720536] [<c040047a>] do_one_initcall+0xaa/0x200 [ 6.720541] [<f7ca7000>] ? 0xf7ca7000 [ 6.720547] [<c05801c8>] ? free_vmap_area_noflush+0x38/0x80 [ 6.720554] [<c0594b95>] ? kmem_cache_alloc_trace+0x175/0x1f0 [ 6.720560] [<c0a9c64f>] ? do_init_module+0x21/0x1a1 [ 6.720565] [<c0a9c64f>] ? do_init_module+0x21/0x1a1 [ 6.720572] [<c0a9c67e>] do_init_module+0x50/0x1a1 [ 6.720578] [<c04d917b>] load_module+0x1ceb/0x22f0 [ 6.720585] [<c04d6159>] ? copy_module_from_fd.isra.47+0xf9/0x190 [ 6.720592] [<c04d99a5>] SyS_finit_module+0xa5/0xf0 [ 6.720598] [<c056490b>] ? vm_mmap_pgoff+0x9b/0xc0 [ 6.720605] [<c0aa26df>] sysenter_do_call+0x12/0x12 [ 6.720610] ---[ end trace d5183b3e60f0f675 ]--- -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2015-07-30 at 11:27 +0000, Tirdea, Irina wrote: > I can send some additional patches that will simplify testing the > configuration update to the Goodix device. I think this feature is > the easiest > to test so we can determine if writing to the interrupt pin actually > works. > However, even if it is a BIOS problem and the code will work, the > warning > will still be printed in dmesg. Somehow missed this mail before replying to the current patchset. I'd be fine with that, though it's still not clear to me whether the BIOS/hardware is at fault, or the code that's being added to the driver ;) Cheers -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2015-09-10 at 14:04 +0000, Tirdea, Irina wrote: > > > -----Original Message----- > > From: Bastien Nocera [mailto:hadess@hadess.net] > > Sent: 09 September, 2015 20:03 > > To: Tirdea, Irina; linux-input@vger.kernel.org > > Cc: linux-kernel@vger.kernel.org; Rob Herring; Pawel Moll; Ian > > Campbell; Kumar Gala; Purdila, Octavian; Dmitry Torokhov; Mark > > Rutland; devicetree@vger.kernel.org > > Subject: Re: [PATCH v3 1/5] Input: goodix - reset device at init > > > > On Thu, 2015-07-30 at 11:27 +0000, Tirdea, Irina wrote: > > > I can send some additional patches that will simplify testing the > > > configuration update to the Goodix device. I think this feature > > > is > > > the easiest > > > to test so we can determine if writing to the interrupt pin > > > actually > > > works. > > > However, even if it is a BIOS problem and the code will work, the > > > warning > > > will still be printed in dmesg. > > > > > > Somehow missed this mail before replying to the current patchset. > > I'd > > be fine with that, though it's still not clear to me whether the > > BIOS/hardware is at fault, or the code that's being added to the > > driver > > ;) > > > > The reset procedure is described in the Goodix GT911 datasheet [1] > and is > used for power-on reset and power management. The power-on sequence > is described in chapter 6.1. I2C Timing, in the Power-on Timing > diagram. > The sequence for putting the device to sleep is described in chapter > 7.1. Operating Modes, c) Sleep mode. These sequences use the > interrupt > pin as output. > > The warning you mentioned comes from the following code in the goodix > driver, which sets the interrupt to be used as output: > > + error = gpiod_direction_output(ts->gpiod_int, ts->client- > >addr == 0x14); > > The gpiod_direction_output() call ends up in > drivers/pinctrl/intel/pinctrl-baytrail.c: > /* > * Before making any direction modifications, do a check if gpio > * is set for direct IRQ. On baytrail, setting GPIO to output does > * not make sense, so let's at least warn the caller before they > shoot > * themselves in the foot. > */ > WARN(readl(conf_reg) & BYT_DIRECT_IRQ_EN, > "Potential Error: Setting GPIO with direct_irq_en to output"); > > So the problem comes from using the gpio interrupt pin as output, > which > should not work on Baytrail if BYT_DIRECT_IRQ_EN is set by BIOS. > The above warning is introduced and discussed in [2] and [3]. As I > mentioned, > this could be a real HW issue or the BIOS sets BYT_DIRECT_IRQ_EN when > it should not. I have also tested these patches on a Baytrail > plarform > (that uses the same pinctrl driver), but I did not see any issues > since > BYT_DIRECT_IRQ_EN is not set in my case for the interrupt gpio pin. Do we have a way to work-around this in the GPIO driver? > To determine if using the interrupt pin as output works, you can test > updating > the goodix configuration [4]. Right, the problem being that it's a later patch in the branch, and that the driver will fail to probe if there's an error from the GPIO call. Would you have a patch for me to test that would bypass this error, or at least fallback gracefully to not resetting, not probing GPIOs if they're badly setup? Cheers -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/devicetree/bindings/input/touchscreen/goodix.txt b/Documentation/devicetree/bindings/input/touchscreen/goodix.txt index 8ba98ee..c0715f8 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 + - gpios : GPIOS the chip is connected to: first one is the + interrupt gpio and second one the reset gpio. Example: @@ -23,6 +25,9 @@ Example: reg = <0x5d>; interrupt-parent = <&gpio>; interrupts = <0 0>; + + gpios = <&gpio1 0 0>, /* INT */ + <&gpio1 1 0>; /* RST */ }; /* ... */ diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c index b4d12e2..80100f9 100644 --- a/drivers/input/touchscreen/goodix.c +++ b/drivers/input/touchscreen/goodix.c @@ -24,6 +24,7 @@ #include <linux/interrupt.h> #include <linux/slab.h> #include <linux/acpi.h> +#include <linux/gpio.h> #include <linux/of.h> #include <asm/unaligned.h> @@ -34,6 +35,13 @@ struct goodix_ts_data { int abs_y_max; unsigned int max_touch_num; unsigned int int_trigger_type; + struct gpio_desc *gpiod_int; + struct gpio_desc *gpiod_rst; +}; + +struct goodix_gpio_data { + u8 irq_idx; + u8 rst_idx; }; #define GOODIX_MAX_HEIGHT 4096 @@ -60,6 +68,16 @@ static const unsigned long goodix_irq_flags[] = { IRQ_TYPE_LEVEL_HIGH, }; +static const struct goodix_gpio_data goodix_gpio_irq_rst = { + .irq_idx = 0, + .rst_idx = 1, +}; + +static const struct goodix_gpio_data goodix_gpio_rst_irq = { + .irq_idx = 1, + .rst_idx = 0, +}; + /** * goodix_i2c_read - read data from a register of the i2c slave device. * @@ -186,6 +204,102 @@ 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 ret; + + ret = gpiod_direction_output(ts->gpiod_int, 0); + if (ret) + return ret; + 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 ret; + + /* begin select I2C slave addr */ + ret = gpiod_direction_output(ts->gpiod_rst, 0); + if (ret) + return ret; + msleep(20); /* T2: > 10ms */ + /* HIGH: 0x28/0x29, LOW: 0xBA/0xBB */ + ret = gpiod_direction_output(ts->gpiod_int, ts->client->addr == 0x14); + if (ret) + return ret; + usleep_range(100, 2000); /* T3: > 100us */ + ret = gpiod_direction_output(ts->gpiod_rst, 1); + if (ret) + return ret; + usleep_range(6000, 10000); /* T4: > 5ms */ + /* end select I2C slave addr */ + ret = gpiod_direction_input(ts->gpiod_rst); + if (ret) + return ret; + 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, + const struct i2c_device_id *i2c_id) +{ + int ret; + struct device *dev; + const struct acpi_device_id *acpi_id; + struct gpio_desc *gpiod; + /* Default gpio pin order: irq, rst */ + const struct goodix_gpio_data *gpio = &goodix_gpio_irq_rst; + + if (!ts->client) + return -EINVAL; + dev = &ts->client->dev; + + /* Set gpio pin order if specified in chip data */ + if (i2c_id) { + if (i2c_id->driver_data) + gpio = (struct goodix_gpio_data *)i2c_id->driver_data; + } else if (ACPI_HANDLE(dev)) { + acpi_id = acpi_match_device(dev->driver->acpi_match_table, dev); + if (acpi_id) + gpio = (struct goodix_gpio_data *)acpi_id->driver_data; + } + + /* Get interrupt GPIO pin number */ + gpiod = devm_gpiod_get_index(dev, NULL, gpio->irq_idx, GPIOD_IN); + if (IS_ERR(gpiod)) { + ret = PTR_ERR(gpiod); + if (ret != -EPROBE_DEFER) + dev_warn(dev, "Failed to get GPIO %d: %d\n", + gpio->irq_idx, ret); + return ret; + } + ts->gpiod_int = gpiod; + + /* Get the reset line GPIO pin number */ + gpiod = devm_gpiod_get_index(dev, NULL, gpio->rst_idx, GPIOD_IN); + if (IS_ERR(gpiod)) { + ret = PTR_ERR(gpiod); + if (ret != -EPROBE_DEFER) + dev_warn(dev, "Failed to get GPIO %d: %d\n", + gpio->rst_idx, ret); + return ret; + } + ts->gpiod_rst = gpiod; + + return 0; +} + /** * goodix_read_config - Read the embedded configuration of the panel * @@ -362,6 +476,19 @@ static int goodix_ts_probe(struct i2c_client *client, return error; } + error = goodix_get_gpio_config(ts, id); + if (error && error != -ENOENT) + return error; + + if (ts->gpiod_int && ts->gpiod_rst) { + /* reset the controller */ + error = goodix_reset(ts); + if (error) { + dev_err(&client->dev, "Controller reset failed.\n"); + return error; + } + } + goodix_read_config(ts); error = goodix_request_input_dev(ts, version_info, id_info); @@ -381,13 +508,13 @@ static int goodix_ts_probe(struct i2c_client *client, } static const struct i2c_device_id goodix_ts_id[] = { - { "GDIX1001:00", 0 }, + { "GDIX1001:00", (kernel_ulong_t)&goodix_gpio_rst_irq }, { } }; #ifdef CONFIG_ACPI static const struct acpi_device_id goodix_acpi_match[] = { - { "GDIX1001", 0 }, + { "GDIX1001", (kernel_ulong_t)&goodix_gpio_rst_irq }, { } }; MODULE_DEVICE_TABLE(acpi, goodix_acpi_match);