RE: [PATCH] Input: elants_i2c - wire up regulator support

From: ELAN ååé
Date: Thu Aug 06 2015 - 23:52:26 EST

Next message: Shubhrajyoti Datta: "Re: [PATCH v3] iio: adc: xilinx-xadc: Push interrupts into threaded context"
Previous message: Viresh Kumar: "Re: [PATCH v4 0/6] cpufreq: use generic cpufreq drivers for Exynos4x12 platform"
In reply to: Dmitry Torokhov: "Re: [PATCH] Input: elants_i2c - wire up regulator support"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi Dmitry,
Thanks for your explanation, I understand.

Reviewed-by: Scott Liu <scott.liu@xxxxxxxxxx>

Best Regards,
--
Scott

> -----Original Message-----
> From: Dmitry Torokhov [mailto:dmitry.torokhov@xxxxxxxxx]
> Sent: Friday, August 07, 2015 8:25 AM
> To: ELAN ååé
> Cc: Dmitry Torokhov; linux-input@xxxxxxxxxxxxxxx; Rob Herring; Mark Rutland;
> Benson Leung; Duson Lin; James Chen; devicetree@xxxxxxxxxxxxxxx; lkml
> Subject: Re: [PATCH] Input: elants_i2c - wire up regulator support
>
> Hi Scott,
>
> On Thu, Aug 6, 2015 at 12:03 AM, ELAN ååé <scott.liu@xxxxxxxxxx>
> wrote:
> > Hi Dmitry
> >
> > I am curious about how reset gpio works and I think the reset
> > gpio at low level after elants_i2c_power_on(),
> > So that the controller won't response anyway, please see below
> > my question and correct me if I am wrong.
> >
> >> -----Original Message-----
> >> From: Dmitry Torokhov [mailto:dtor@xxxxxxxxxxxx]
> >> Sent: Thursday, August 06, 2015 2:54 AM
> >> To: linux-input@xxxxxxxxxxxxxxx
> >> Cc: Rob Herring; Mark Rutland; Benson Leung; Duson Lin; Scott Liu;
> >> James Chen; devicetree@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> >> linux-input@xxxxxxxxxxxxxxx
> >> Subject: [PATCH] Input: elants_i2c - wire up regulator support
> >>
> >> Elan touchscreen controllers use two power supplies, vcc33 and vccio,
> >> and we need to enable them before trying to access the device. On X86
> >> firmware usually does this, but on ARM it is usually left to the kernel.
> >>
> >> Signed-off-by: Dmitry Torokhov <dtor@xxxxxxxxxxxx>
> >> Reviewed-by: Benson Leung <bleung@xxxxxxxxxxxx>
> >> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx>
> >> ---
> >> .../devicetree/bindings/input/elants_i2c.txt | 3 +
> >> drivers/input/touchscreen/elants_i2c.c | 184
> >> ++++++++++++++++++---
> >> 2 files changed, 165 insertions(+), 22 deletions(-)
> >>
> >> diff --git a/Documentation/devicetree/bindings/input/elants_i2c.txt
> >> b/Documentation/devicetree/bindings/input/elants_i2c.txt
> >> index a765232..8a71038 100644
> >> --- a/Documentation/devicetree/bindings/input/elants_i2c.txt
> >> +++ b/Documentation/devicetree/bindings/input/elants_i2c.txt
> >> @@ -13,6 +13,9 @@ Optional properties:
> >> - pinctrl-names: should be "default" (see pinctrl binding [1]).
> >> - pinctrl-0: a phandle pointing to the pin settings for the device (see
> >> pinctrl binding [1]).
> >> +- reset-gpios: reset gpio the chip is connected to.
> >> +- vcc33-supply: a phandle for the regulator supplying 3.3V power.
> >> +- vccio-supply: a phandle for the regulator supplying IO power.
> >>
> >> [0]:
> > Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
> >> [1]: Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
> >> diff --git a/drivers/input/touchscreen/elants_i2c.c
> >> b/drivers/input/touchscreen/elants_i2c.c
> >> index 42e43f1..1912265 100644
> >> --- a/drivers/input/touchscreen/elants_i2c.c
> >> +++ b/drivers/input/touchscreen/elants_i2c.c
> >> @@ -38,6 +38,8 @@
> >> #include <linux/input/mt.h>
> >> #include <linux/acpi.h>
> >> #include <linux/of.h>
> >> +#include <linux/gpio/consumer.h>
> >> +#include <linux/regulator/consumer.h>
> >> #include <asm/unaligned.h>
> >>
> >> /* Device, Driver information */
> >> @@ -102,6 +104,9 @@
> >> /* calibration timeout definition */
> >> #define ELAN_CALI_TIMEOUT_MSEC 10000
> >>
> >> +#define ELAN_POWERON_DELAY_USEC 500
> >> +#define ELAN_RESET_DELAY_MSEC 20
> >> +
> >> enum elants_state {
> >> ELAN_STATE_NORMAL,
> >> ELAN_WAIT_QUEUE_HEADER,
> >> @@ -118,6 +123,10 @@ struct elants_data {
> >> struct i2c_client *client;
> >> struct input_dev *input;
> >>
> >> + struct regulator *vcc33;
> >> + struct regulator *vccio;
> >> + struct gpio_desc *reset_gpio;
> >> +
> >> u16 fw_version;
> >> u8 test_version;
> >> u8 solution_version;
> >> @@ -141,6 +150,7 @@ struct elants_data {
> >> u8 buf[MAX_PACKET_SIZE];
> >>
> >> bool wake_irq_enabled;
> >> + bool keep_power_in_suspend;
> >> };
> >>
> >> static int elants_i2c_send(struct i2c_client *client, @@ -1058,6
> >> +1068,67 @@ static void elants_i2c_remove_sysfs_group(void *_data)
> >> sysfs_remove_group(&ts->client->dev.kobj,
> >> &elants_attribute_group); }
> >>
> >> +static int elants_i2c_power_on(struct elants_data *ts) {
> >> + int error;
> >> +
> >> + /*
> >> + * If we do not have reset gpio assume platform firmware
> >> + * controls regulators and does power them on for us.
> >> + */
> >> + if (IS_ERR_OR_NULL(ts->reset_gpio))
> >> + return 0;
> >> +
> >> + gpiod_set_value_cansleep(ts->reset_gpio, 1);
> >> +
> >> + error = regulator_enable(ts->vcc33);
> >> + if (error) {
> >> + dev_err(&ts->client->dev,
> >> + "failed to enable vcc33 regulator: %d\n",
> >> + error);
> >> + goto release_reset_gpio;
> >> + }
> >> +
> >> + error = regulator_enable(ts->vccio);
> >> + if (error) {
> >> + dev_err(&ts->client->dev,
> >> + "failed to enable vccio regulator: %d\n",
> >> + error);
> >> + regulator_disable(ts->vcc33);
> >> + goto release_reset_gpio;
> >> + }
> >> +
> >> + /*
> >> + * We need to wait a bit after powering on controller before
> >> + * we are allowed to release reset GPIO.
> >> + */
> >> + udelay(ELAN_POWERON_DELAY_USEC);
> >> +
> >> +release_reset_gpio:
> >> + gpiod_set_value_cansleep(ts->reset_gpio, 0);
> >> + if (error)
> >> + return error;
> >> +
> >> + msleep(ELAN_RESET_DELAY_MSEC);
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static void elants_i2c_power_off(void *_data) {
> >> + struct elants_data *ts = _data;
> >> +
> >> + if (!IS_ERR_OR_NULL(ts->reset_gpio)) {
> >> + /*
> >> + * Activate reset gpio to prevent leakage through the
> >> + * pin once we shut off power to the controller.
> >> + */
> >> + gpiod_set_value_cansleep(ts->reset_gpio, 1);
> >> + regulator_disable(ts->vccio);
> >> + regulator_disable(ts->vcc33);
> >> + }
> >> +}
> >> +
> >> static int elants_i2c_probe(struct i2c_client *client,
> >> const struct i2c_device_id *id) { @@
> >> -1072,13 +1143,6 @@ static int elants_i2c_probe(struct i2c_client
> >> *client,
> >> return -ENXIO;
> >> }
> >>
> >> - /* Make sure there is something at this address */
> >> - if (i2c_smbus_xfer(client->adapter, client->addr, 0,
> >> - I2C_SMBUS_READ, 0, I2C_SMBUS_BYTE,
> &dummy) < 0) {
> >> - dev_err(&client->dev, "nothing at this address\n");
> >> - return -ENXIO;
> >> - }
> >> -
> >> ts = devm_kzalloc(&client->dev, sizeof(struct elants_data),
> >> GFP_KERNEL);
> >> if (!ts)
> >> return -ENOMEM;
> >> @@ -1089,6 +1153,70 @@ static int elants_i2c_probe(struct i2c_client
> >> *client,
> >> ts->client = client;
> >> i2c_set_clientdata(client, ts);
> >>
> >> + ts->vcc33 = devm_regulator_get(&client->dev, "vcc33");
> >> + if (IS_ERR(ts->vcc33)) {
> >> + error = PTR_ERR(ts->vcc33);
> >> + if (error != -EPROBE_DEFER)
> >> + dev_err(&client->dev,
> >> + "Failed to get 'vcc33' regulator:
> %d\n",
> >> + error);
> >> + return error;
> >> + }
> >> +
> >> + ts->vccio = devm_regulator_get(&client->dev, "vccio");
> >> + if (IS_ERR(ts->vccio)) {
> >> + error = PTR_ERR(ts->vccio);
> >> + if (error != -EPROBE_DEFER)
> >> + dev_err(&client->dev,
> >> + "Failed to get 'vccio' regulator:
> %d\n",
> >> + error);
> >> + return error;
> >> + }
> >> +
> >> + ts->reset_gpio = devm_gpiod_get(&client->dev, "reset");
> >> + if (IS_ERR(ts->reset_gpio)) {
> >> + error = PTR_ERR(ts->reset_gpio);
> >> +
> >> + if (error == -EPROBE_DEFER)
> >> + return error;
> >> +
> >> + if (error != -ENOENT && error != -ENOSYS) {
> >> + dev_err(&client->dev,
> >> + "failed to get reset gpio: %d\n",
> >> + error);
> >> + return error;
> >> + }
> >> +
> >> + ts->keep_power_in_suspend = true;
> >> + } else {
> >> + error = gpiod_direction_output(ts->reset_gpio, 0);
> >
> > Does it mean to reset our controller? (reset gpio pull low).
>
> No, gpiod takes into account the declared polarity of the GPIO signal and we
> declare the reset GPIOs as "active low" in DTS. So here we configure GPIO for
> output and make it inactive (which translates to HIGH).
>
> >
> >> + if (error) {
> >> + dev_err(&client->dev,
> >> + "failed to configure reset gpio as
> output:
> > %d\n",
> >> + error);
> >> + return error;
> >> + }
> >> + }
> >> +
> >> + error = elants_i2c_power_on(ts);
> >
> > If reset gpio is at low, controller won't response initialize flow.
> > I am curious that does reset pin be at low level after
> > elants_i2c_power_on(ts)?
>
> The reset gpio will be "inactive", or logical 0. Given that we expect these
> gpios be declared as GPIO_ACTIVE_LOW this translates to line going
> HIGH->LOW->HIGH in elants_i2c_power_on() and stay HIGH while the driver
> is bound to the device.
>
> Thanks.
>
> --
> Dmitry

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

Next message: Shubhrajyoti Datta: "Re: [PATCH v3] iio: adc: xilinx-xadc: Push interrupts into threaded context"
Previous message: Viresh Kumar: "Re: [PATCH v4 0/6] cpufreq: use generic cpufreq drivers for Exynos4x12 platform"
In reply to: Dmitry Torokhov: "Re: [PATCH] Input: elants_i2c - wire up regulator support"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]