Hi Daniel,
thanks for picking my suggestion and converting the driver :)
Unfortunately it is not that easy since we need to care about the
existing users. You will get the legacy users if you grep for
ads7846_platform_data. Those platforms needs to converted to.
Again, pls align the commit subject prefix with the existing patches.
On 20-05-04 19:37, Daniel Mack wrote:
> Use gpiod_* function to access the pendown GPIO line.
Maybe this is better:
Input: ads7846 - Convert to GPIO descriptors
This converts the ads7846 touchscreen driver to use GPIO
descriptors and switches all platform data boards over
to passing descriptors instead of global GPIO numbers.
We use the "pendown-gpio" name as found in the device
tree bindings for this touchscreen driver.
>
> Signed-off-by: Daniel Mack <daniel@xxxxxxxxxx>
> ---
> drivers/input/touchscreen/ads7846.c | 53 ++++++++++++++++-------------
> 1 file changed, 30 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/input/touchscreen/ads7846.c b/drivers/input/touchscreen/ads7846.c
> index 7f4ead542a73..b3e17ee4e499 100644
> --- a/drivers/input/touchscreen/ads7846.c
> +++ b/drivers/input/touchscreen/ads7846.c
> @@ -27,7 +27,7 @@
> #include <linux/of.h>
> #include <linux/of_gpio.h>
> #include <linux/of_device.h>
> -#include <linux/gpio.h>
> +#include <linux/gpio/consumer.h>
> #include <linux/spi/spi.h>
> #include <linux/spi/ads7846.h>
> #include <linux/regulator/consumer.h>
> @@ -137,7 +137,7 @@ struct ads7846 {
> void *filter_data;
> void (*filter_cleanup)(void *data);
> int (*get_pendown_state)(void);
> - int gpio_pendown;
> + struct gpio_desc *gpio_pendown;
>
> void (*wait_for_sync)(void);
> };
> @@ -598,7 +598,7 @@ static int get_pendown_state(struct ads7846 *ts)
> if (ts->get_pendown_state)
> return ts->get_pendown_state();
>
> - return !gpio_get_value(ts->gpio_pendown);
> + return !gpiod_get_value(ts->gpio_pendown);
The gpio_get_value() function return the raw value and the ! before
inverts the logic. Thanks to gpiod we don't need to care about this
inverting logic anymore.
> }
>
> static void null_wait_for_sync(void)
> @@ -919,6 +919,7 @@ static int ads7846_setup_pendown(struct spi_device *spi,
> struct ads7846 *ts,
> const struct ads7846_platform_data *pdata)
> {
> + struct device *dev = &spi->dev;
> int err;
>
> /*
> @@ -929,27 +930,33 @@ static int ads7846_setup_pendown(struct spi_device *spi,
>
> if (pdata->get_pendown_state) {
> ts->get_pendown_state = pdata->get_pendown_state;
> - } else if (gpio_is_valid(pdata->gpio_pendown)) {
> -
> - err = devm_gpio_request_one(&spi->dev, pdata->gpio_pendown,
> - GPIOF_IN, "ads7846_pendown");
> - if (err) {
> - dev_err(&spi->dev,
> - "failed to request/setup pendown GPIO%d: %d\n",
> - pdata->gpio_pendown, err);
> - return err;
> - }
> + return 0;
> + }
>
> - ts->gpio_pendown = pdata->gpio_pendown;
> + ts->gpio_pendown = devm_gpiod_get(dev, "pendown", GPIOD_IN);
> + if (IS_ERR(ts->gpio_pendown)) {
> + err = PTR_ERR(ts->gpio_pendown);
>
> - if (pdata->gpio_pendown_debounce)
> - gpio_set_debounce(pdata->gpio_pendown,
> - pdata->gpio_pendown_debounce);
> - } else {
> - dev_err(&spi->dev, "no get_pendown_state nor gpio_pendown?\n");
> - return -EINVAL;
> + if (gpio_is_valid(pdata->gpio_pendown)) {
> + err = devm_gpio_request_one(dev, pdata->gpio_pendown,
> + GPIOF_IN,
> + "ads7846_pendown");
> + if (err < 0)
> + return err;
> +
> + ts->gpio_pendown = gpio_to_desc(pdata->gpio_pendown);
> + if (!ts->gpio_pendown)
> + return -EINVAL;
> + }
> +
> + if (err < 0)
> + return err;
> }
>
> + if (pdata->gpio_pendown_debounce)
> + gpiod_set_debounce(ts->gpio_pendown,
> + pdata->gpio_pendown_debounce);
> +
> return 0;
> }
>
> @@ -1236,8 +1243,6 @@ static const struct ads7846_platform_data *ads7846_probe_dt(struct device *dev)
> pdata->wakeup = of_property_read_bool(node, "wakeup-source") ||
> of_property_read_bool(node, "linux,wakeup");
>
> - pdata->gpio_pendown = of_get_named_gpio(dev->of_node, "pendown-gpio", 0);
> -
> return pdata;
> }
It's a bit hard to follow the changes within ads7846_setup_pendown(). To
make it easier and IMHO cleaner to everyone we should convert the
ads7846_platform_data to gpio_desc too and request the gpio within the
ads7846_probe_dt() function. Pls take a look on this patchset [1] from
Linus. There you will get all informations who platform data based
machines can be changed usign gpiod_lookup_table's.
[1] https://patches.linaro.org/patch/185481/
Thanks for working on this =)
Regards,
Marco
> #else
> @@ -1340,8 +1345,10 @@ static int ads7846_probe(struct spi_device *spi)
> }
>
> err = ads7846_setup_pendown(spi, ts, pdata);
> - if (err)
> + if (err) {
> + dev_err(dev, "Unable to request pendown GPIO: %d", err);
> goto err_cleanup_filter;
> + }
>
> if (pdata->penirq_recheck_delay_usecs)
> ts->penirq_recheck_delay_usecs =
> --
> 2.26.2
