Date:   Sun, 11 Jun 2017 03:49:18 +0000
From:   "Mani, Rajmohan" <rajmohan.mani@...el.com>
To:     Andy Shevchenko <andy.shevchenko@...il.com>
CC:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-gpio@...r.kernel.org" <linux-gpio@...r.kernel.org>,
        "linux-acpi@...r.kernel.org" <linux-acpi@...r.kernel.org>,
        Lee Jones <lee.jones@...aro.org>,
        Linus Walleij <linus.walleij@...aro.org>,
        "Alexandre Courbot" <gnurou@...il.com>,
        "Rafael J. Wysocki" <rjw@...ysocki.net>,
        "Len Brown" <lenb@...nel.org>
Subject: RE: [PATCH v1 2/3] gpio: Add support for TPS68470 GPIOs

Hi Andy,

Thanks for the reviews and patience.

> 
> On Tue, Jun 6, 2017 at 2:55 PM, Rajmohan Mani <rajmohan.mani@...el.com>
> wrote:
> > This patch adds support for TPS68470 GPIOs.
> > There are 7 GPIOs and a few sensor related GPIOs.
> > These GPIOs can be requested and configured as appropriate.
> 
> Besides my below comments, just put it here that I recommended earlier to
> provide 2 GPIO chips (one per bank of GPIOs).
> It's up to Linus to decide since you didn't follow the recommendation.
> 

Ack.
Did you mean to add this in Kconfig or this source file?

Here's some more details on these GPIOs.
Each of these 7 GPIOs has 2 registers to control the mode, level, drive strength, polarity, hysteresis control among other things. Also there are GPDI and GPDO registers to control the input and output values of these 7 GPIOs. These GPIOs are numbered 0 through 6.
The remaining 3 GPIOs are more of special purpose GPIOs that are output only, with one register to control all of their output values and drive strengths. These GPIOs are named with a special purpose (ENABLE, IDLE and RESET of the sensor).

> > +#include <linux/gpio.h>
> > +#include <linux/gpio/machine.h>
> 
> These shouldn't be in the driver.
> Instead use
> #include <linux/gpio/driver.h>
> 

Ack

> > +#include <linux/mfd/tps68470.h>
> > +#include <linux/module.h>
> > +#include <linux/platform_device.h>
> 
> > +       if (offset >= TPS68470_N_REGULAR_GPIO) {
> > +               offset -= TPS68470_N_REGULAR_GPIO;
> > +               reg = TPS68470_REG_SGPO;
> > +       }
> 
> Two GPIO chips makes this gone.
> 

Ack.
On a closer look, creating two GPIO chips makes things clearer. 
But, this comes at the cost of a new set of gpio_get/set, 
gpio_output/input functions for the new GPIO chip. This results in 
new code for the second GPIO chip, which is pretty much 
going to be the copy of first GPIO chip, except for initializing 
the "reg" variable with GPDO or SGPO register. If we decide to 
reuse the existing code of the first GPIO chip for the new/second 
GPIO chip, we would still need to add a check, which would be 
effectively the same as the existing code, with the only advantage 
of not having to initialize the "offset" variable (which holds the 
GPIO offset). Given the above, it seems ok to retain the existing 
model of a single chip with the adjustments for offset, reg 
variables per the GPIO offset, to keep the whole picture simple.

> > +struct gpiod_lookup_table gpios_table = {
> > +       .dev_id = NULL,
> > +       .table = {
> > +                 GPIO_LOOKUP("tps68470-gpio", 0, "gpio.0", GPIO_ACTIVE_HIGH),
> > +                 GPIO_LOOKUP("tps68470-gpio", 1, "gpio.1", GPIO_ACTIVE_HIGH),
> > +                 GPIO_LOOKUP("tps68470-gpio", 2, "gpio.2", GPIO_ACTIVE_HIGH),
> > +                 GPIO_LOOKUP("tps68470-gpio", 3, "gpio.3", GPIO_ACTIVE_HIGH),
> > +                 GPIO_LOOKUP("tps68470-gpio", 4, "gpio.4", GPIO_ACTIVE_HIGH),
> > +                 GPIO_LOOKUP("tps68470-gpio", 5, "gpio.5", GPIO_ACTIVE_HIGH),
> > +                 GPIO_LOOKUP("tps68470-gpio", 6, "gpio.6", GPIO_ACTIVE_HIGH),
> > +                 GPIO_LOOKUP("tps68470-gpio", 7, "s_enable",
> GPIO_ACTIVE_HIGH),
> > +                 GPIO_LOOKUP("tps68470-gpio", 8, "s_idle", GPIO_ACTIVE_HIGH),
> > +                 GPIO_LOOKUP("tps68470-gpio", 9, "s_resetn",
> GPIO_ACTIVE_HIGH),
> > +                 {},
> > +       },
> > +};
> 
> This doesn't belong to the driver.
> 

Ack
I have moved this code to the MFD driver

> > +static int tps68470_gpio_probe(struct platform_device *pdev) {
> > +       struct tps68470 *tps68470 = dev_get_drvdata(pdev->dev.parent);
> > +       struct tps68470_gpio_data *tps68470_gpio;
> 
> > +       int i, ret;
> 
> unsingned int i;
> 
> > +       ret = gpiochip_add(&tps68470_gpio->gc);
> 
> devm_ ?
> 

Ack

> > +       gpiod_add_lookup_table(&gpios_table);
> 
> Doesn't belong to the driver either.
> I suppose it's a part of MFD (patch 1)
> 

Ack

> > +       /*
> > +        * Initialize all the GPIOs to 0, just to make sure all
> > +        * GPIOs start with known default values. This protects against
> > +        * any GPIOs getting set with a value of 1, after TPS68470
> > + reset
> 
> So, this is hardware bug. Right? Or misconfiguration of the chip we may avoid?
> 

The tps68470 PMIC upon reset, does not have the "power on reset" values.
We just initialize the GPIOs with their known default values.

> > +        */
> > +       for (i = 0; i < tps68470_gpio->gc.ngpio; i++)
> > +               tps68470_gpio_set(&tps68470_gpio->gc, i, 0);
> > +
> > +       return ret;
> > +}
> 
> > +
> > +static int tps68470_gpio_remove(struct platform_device *pdev) {
> > +       struct tps68470_gpio_data *tps68470_gpio =
> > +platform_get_drvdata(pdev);
> > +
> > +       gpiod_remove_lookup_table(&gpios_table);
> > +       gpiochip_remove(&tps68470_gpio->gc);
> > +
> > +       return 0;
> > +}
> 
> Should gone after devm_ in use.
> 

Ack

Powered by blists - more mailing lists