RE: [PATCH v1 2/3] gpio: Add support for TPS68470 GPIOs

[Mani, Rajmohan]

Hi Sakari,

> Subject: Re: [PATCH v1 2/3] gpio: Add support for TPS68470 GPIOs
> 
> Hi Rajmohan and Andy,
> 
> On Sun, Jun 11, 2017 at 03:49:18AM +, Mani, Rajmohan wrote:
> > Hi Andy,
> >
> > Thanks for the reviews and patience.
> >
> > >
> > > On Tue, Jun 6, 2017 at 2:55 PM, Rajmohan Mani
> > > <rajmohan.m...@intel.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 
> > > > +#include 
> > >
> > > These shouldn't be in the driver.
> > > Instead use
> > > #include 
> > >
> >
> > Ack
> >
> > > > +#include 
> > > > +#include 
> > > > +#include 
> > >
> > > > +   if (offset >= TPS68470_N_REGULAR_GPIO) {
> > > > +   offset -= TPS68470_N_REGULAR_GPIO;
> > > > +   reg = TPS68470_REG_SGPO;
> > > > +   }
> > >
> > > Two GPIO chips makes this gone.
> 
> Again, I'm not really worried about this driver, but the ACPI tables. How does
> the difference show there?
> 
> The outputs (s_enable, s_idle and s_resetn) are not numbered in the
> documentation. There grouped, though, but the order in that grouping varies.
> 
> > > > +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
> 
> This information should come from the ACPI tables, it should not be present in
> drivers. Why do you need it?
> 

I have removed the GPIO lookup table code for now.

> > > > +   /*
> > > > +* 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.
> 
> If that was the case, I bet you could expect interesting behaviour from the
> hardware connected to these pins.
> 
> For what it's worth, the chip documentation states that the reset value for 
> the
> SGPO and GPDO registers is zero, as well as that GPIOs are configured as input
> and the reset value of the s_* outputs is low.
> 
> In other words, I don't think that explicitly setting the values to zero has 
> an
> effect.
>

For now, I have removed this code that sets the GPIOs to zeros. If there are 
enough findings / reasons, this code may come back.

RE: [PATCH v1 2/3] gpio: Add support for TPS68470 GPIOs

[Mani, Rajmohan]

Hi Sakari,

> Subject: Re: [PATCH v1 2/3] gpio: Add support for TPS68470 GPIOs
> 
> Hi Rajmohan and Andy,
> 
> On Sun, Jun 11, 2017 at 03:49:18AM +, Mani, Rajmohan wrote:
> > Hi Andy,
> >
> > Thanks for the reviews and patience.
> >
> > >
> > > On Tue, Jun 6, 2017 at 2:55 PM, Rajmohan Mani
> > > 
> > > 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 
> > > > +#include 
> > >
> > > These shouldn't be in the driver.
> > > Instead use
> > > #include 
> > >
> >
> > Ack
> >
> > > > +#include 
> > > > +#include 
> > > > +#include 
> > >
> > > > +   if (offset >= TPS68470_N_REGULAR_GPIO) {
> > > > +   offset -= TPS68470_N_REGULAR_GPIO;
> > > > +   reg = TPS68470_REG_SGPO;
> > > > +   }
> > >
> > > Two GPIO chips makes this gone.
> 
> Again, I'm not really worried about this driver, but the ACPI tables. How does
> the difference show there?
> 
> The outputs (s_enable, s_idle and s_resetn) are not numbered in the
> documentation. There grouped, though, but the order in that grouping varies.
> 
> > > > +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
> 
> This information should come from the ACPI tables, it should not be present in
> drivers. Why do you need it?
> 

I have removed the GPIO lookup table code for now.

> > > > +   /*
> > > > +* 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.
> 
> If that was the case, I bet you could expect interesting behaviour from the
> hardware connected to these pins.
> 
> For what it's worth, the chip documentation states that the reset value for 
> the
> SGPO and GPDO registers is zero, as well as that GPIOs are configured as input
> and the reset value of the s_* outputs is low.
> 
> In other words, I don't think that explicitly setting the values to zero has 
> an
> effect.
>

For now, I have removed this code that sets the GPIOs to zeros. If there are 
enough findings / reasons, this code may come back.

Re: [PATCH v1 2/3] gpio: Add support for TPS68470 GPIOs

[Andy Shevchenko]

On Mon, Jun 12, 2017 at 12:17 PM, Mani, Rajmohan
<rajmohan.m...@intel.com> wrote:
>> Subject: Re: [PATCH v1 2/3] gpio: Add support for TPS68470 GPIOs
>> On Sun, Jun 11, 2017 at 7:50 PM, Sakari Ailus <sakari.ai...@iki.fi> wrote:
>> > On Sun, Jun 11, 2017 at 04:40:16PM +0300, Andy Shevchenko wrote:
>> >> On Sun, Jun 11, 2017 at 2:30 PM, Sakari Ailus <sakari.ai...@iki.fi> wrote:

>> >> > Again, I'm not really worried about this driver, but the ACPI
>> >> > tables. How does the difference show there?
>> >>
>> >> Same way. You will have common numbering over the chip [0, 9]. It
>> >> will be just an abstraction inside the driver.

>> > Sounds fine to me, taken that this does not add complications to ACPI
>> > tables.
>>
>> They just need to share the same ACPI_HANDLE (it might require to do this in
>> generic way in gpiolib) and have a continuous numbering (easy to achieve with
>> carefully chosen bases).

> Few clarifications...
>
> Are you implying new kernel changes are needed in gpiolib to accommodate 2 
> GPIO chips?

Might be needed. It should be investigated first.
In any case it would be somelike oneliner.

> Does it need changes in platform firmware or is it expected to work just with 
> the gpiolib changes that you described above?

No firmware changes are implied.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 2/3] gpio: Add support for TPS68470 GPIOs

[Andy Shevchenko]

On Mon, Jun 12, 2017 at 12:17 PM, Mani, Rajmohan
 wrote:
>> Subject: Re: [PATCH v1 2/3] gpio: Add support for TPS68470 GPIOs
>> On Sun, Jun 11, 2017 at 7:50 PM, Sakari Ailus  wrote:
>> > On Sun, Jun 11, 2017 at 04:40:16PM +0300, Andy Shevchenko wrote:
>> >> On Sun, Jun 11, 2017 at 2:30 PM, Sakari Ailus  wrote:

>> >> > Again, I'm not really worried about this driver, but the ACPI
>> >> > tables. How does the difference show there?
>> >>
>> >> Same way. You will have common numbering over the chip [0, 9]. It
>> >> will be just an abstraction inside the driver.

>> > Sounds fine to me, taken that this does not add complications to ACPI
>> > tables.
>>
>> They just need to share the same ACPI_HANDLE (it might require to do this in
>> generic way in gpiolib) and have a continuous numbering (easy to achieve with
>> carefully chosen bases).

> Few clarifications...
>
> Are you implying new kernel changes are needed in gpiolib to accommodate 2 
> GPIO chips?

Might be needed. It should be investigated first.
In any case it would be somelike oneliner.

> Does it need changes in platform firmware or is it expected to work just with 
> the gpiolib changes that you described above?

No firmware changes are implied.

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH v1 2/3] gpio: Add support for TPS68470 GPIOs

[Mani, Rajmohan]

Hi Andy, Sakari,

> Subject: Re: [PATCH v1 2/3] gpio: Add support for TPS68470 GPIOs
> 
> On Sun, Jun 11, 2017 at 7:50 PM, Sakari Ailus <sakari.ai...@iki.fi> wrote:
> > On Sun, Jun 11, 2017 at 04:40:16PM +0300, Andy Shevchenko wrote:
> >> On Sun, Jun 11, 2017 at 2:30 PM, Sakari Ailus <sakari.ai...@iki.fi> wrote:
> 
> >> > Again, I'm not really worried about this driver, but the ACPI
> >> > tables. How does the difference show there?
> >>
> >> Same way. You will have common numbering over the chip [0, 9]. It
> >> will be just an abstraction inside the driver.
> >
> > Oh, in that case that should be a non-issue.
> 
> >> Above states the opposite, so, it's clear to me that abstraction of 2
> >> GPIO chips over 1 can be utilized here.
> >
> > Sounds fine to me, taken that this does not add complications to ACPI
> > tables.
> 
> They just need to share the same ACPI_HANDLE (it might require to do this in
> generic way in gpiolib) and have a continuous numbering (easy to achieve with
> carefully chosen bases).
> 

Few clarifications...

Are you implying new kernel changes are needed in gpiolib to accommodate 2 GPIO 
chips?
Does it need changes in platform firmware or is it expected to work just with 
the gpiolib changes that you described above?

Thanks
Raj

RE: [PATCH v1 2/3] gpio: Add support for TPS68470 GPIOs

[Mani, Rajmohan]

Hi Andy, Sakari,

> Subject: Re: [PATCH v1 2/3] gpio: Add support for TPS68470 GPIOs
> 
> On Sun, Jun 11, 2017 at 7:50 PM, Sakari Ailus  wrote:
> > On Sun, Jun 11, 2017 at 04:40:16PM +0300, Andy Shevchenko wrote:
> >> On Sun, Jun 11, 2017 at 2:30 PM, Sakari Ailus  wrote:
> 
> >> > Again, I'm not really worried about this driver, but the ACPI
> >> > tables. How does the difference show there?
> >>
> >> Same way. You will have common numbering over the chip [0, 9]. It
> >> will be just an abstraction inside the driver.
> >
> > Oh, in that case that should be a non-issue.
> 
> >> Above states the opposite, so, it's clear to me that abstraction of 2
> >> GPIO chips over 1 can be utilized here.
> >
> > Sounds fine to me, taken that this does not add complications to ACPI
> > tables.
> 
> They just need to share the same ACPI_HANDLE (it might require to do this in
> generic way in gpiolib) and have a continuous numbering (easy to achieve with
> carefully chosen bases).
> 

Few clarifications...

Are you implying new kernel changes are needed in gpiolib to accommodate 2 GPIO 
chips?
Does it need changes in platform firmware or is it expected to work just with 
the gpiolib changes that you described above?

Thanks
Raj

RE: [PATCH v1 2/3] gpio: Add support for TPS68470 GPIOs

[Mani, Rajmohan]

Hi Linus,

> Subject: Re: [PATCH v1 2/3] gpio: Add support for TPS68470 GPIOs

> > +static inline struct tps68470_gpio_data *to_gpio_data(struct
> > +gpio_chip *gpiochp) {
> > +   return container_of(gpiochp, struct tps68470_gpio_data, gc); }
> 
> Please use the data pointe inside gpio_chip.
> 

My bad. I completely missed the following line. Will fix it v3.

> struct tps68470_gpio_data *foo = gpiochip_get_data(gc);
> 

RE: [PATCH v1 2/3] gpio: Add support for TPS68470 GPIOs

[Mani, Rajmohan]

Hi Linus,

> Subject: Re: [PATCH v1 2/3] gpio: Add support for TPS68470 GPIOs

> > +static inline struct tps68470_gpio_data *to_gpio_data(struct
> > +gpio_chip *gpiochp) {
> > +   return container_of(gpiochp, struct tps68470_gpio_data, gc); }
> 
> Please use the data pointe inside gpio_chip.
> 

My bad. I completely missed the following line. Will fix it v3.

> struct tps68470_gpio_data *foo = gpiochip_get_data(gc);
> 

Re: [PATCH v1 2/3] gpio: Add support for TPS68470 GPIOs

[Andy Shevchenko]

On Sun, Jun 11, 2017 at 7:50 PM, Sakari Ailus  wrote:
> On Sun, Jun 11, 2017 at 04:40:16PM +0300, Andy Shevchenko wrote:
>> On Sun, Jun 11, 2017 at 2:30 PM, Sakari Ailus  wrote:

>> > Again, I'm not really worried about this driver, but the ACPI tables. How
>> > does the difference show there?
>>
>> Same way. You will have common numbering over the chip [0, 9]. It will
>> be just an abstraction inside the driver.
>
> Oh, in that case that should be a non-issue.

>> Above states the opposite, so, it's clear to me that abstraction of 2
>> GPIO chips over 1 can be utilized here.
>
> Sounds fine to me, taken that this does not add complications to ACPI
> tables.

They just need to share the same ACPI_HANDLE (it might require to do
this in generic way in gpiolib) and have a continuous numbering (easy
to achieve with carefully chosen bases).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 2/3] gpio: Add support for TPS68470 GPIOs

[Andy Shevchenko]

On Sun, Jun 11, 2017 at 7:50 PM, Sakari Ailus  wrote:
> On Sun, Jun 11, 2017 at 04:40:16PM +0300, Andy Shevchenko wrote:
>> On Sun, Jun 11, 2017 at 2:30 PM, Sakari Ailus  wrote:

>> > Again, I'm not really worried about this driver, but the ACPI tables. How
>> > does the difference show there?
>>
>> Same way. You will have common numbering over the chip [0, 9]. It will
>> be just an abstraction inside the driver.
>
> Oh, in that case that should be a non-issue.

>> Above states the opposite, so, it's clear to me that abstraction of 2
>> GPIO chips over 1 can be utilized here.
>
> Sounds fine to me, taken that this does not add complications to ACPI
> tables.

They just need to share the same ACPI_HANDLE (it might require to do
this in generic way in gpiolib) and have a continuous numbering (easy
to achieve with carefully chosen bases).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 2/3] gpio: Add support for TPS68470 GPIOs

[Sakari Ailus]

Hi Andy,

On Sun, Jun 11, 2017 at 04:40:16PM +0300, Andy Shevchenko wrote:
> On Sun, Jun 11, 2017 at 2:30 PM, Sakari Ailus  wrote:
> > On Sun, Jun 11, 2017 at 03:49:18AM +, Mani, Rajmohan wrote:
> >> > On Tue, Jun 6, 2017 at 2:55 PM, Rajmohan Mani 
> >> > wrote:
> 
> >> > 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.
> 
> >> 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 
> >> > > +#include 
> >> > > +#include 
> >> >
> >> > > +   if (offset >= TPS68470_N_REGULAR_GPIO) {
> >> > > +   offset -= TPS68470_N_REGULAR_GPIO;
> >> > > +   reg = TPS68470_REG_SGPO;
> >> > > +   }
> >> >
> >> > Two GPIO chips makes this gone.
> >
> > Again, I'm not really worried about this driver, but the ACPI tables. How
> > does the difference show there?
> 
> Same way. You will have common numbering over the chip [0, 9]. It will
> be just an abstraction inside the driver.

Oh, in that case that should be a non-issue.

> 
> > The outputs (s_enable, s_idle and s_resetn) are not numbered in the
> > documentation. There grouped, though, but the order in that grouping varies.
> 
> I don't get this. You are telling that the property of "always output"
> can be assigned to any 3 out of 10?

No, I'm telling you that the three (s_enable, s_idle and s_resetn) cannot be
configured as inputs --- instead they're always outputs. That's how the
hardware is implemented.

> Above states the opposite, so, it's clear to me that abstraction of 2
> GPIO chips over 1 can be utilized here.

Sounds fine to me, taken that this does not add complications to ACPI
tables.

-- 
Regards,

Sakari Ailus
e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk

Re: [PATCH v1 2/3] gpio: Add support for TPS68470 GPIOs

[Sakari Ailus]

Hi Andy,

On Sun, Jun 11, 2017 at 04:40:16PM +0300, Andy Shevchenko wrote:
> On Sun, Jun 11, 2017 at 2:30 PM, Sakari Ailus  wrote:
> > On Sun, Jun 11, 2017 at 03:49:18AM +, Mani, Rajmohan wrote:
> >> > On Tue, Jun 6, 2017 at 2:55 PM, Rajmohan Mani 
> >> > wrote:
> 
> >> > 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.
> 
> >> 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 
> >> > > +#include 
> >> > > +#include 
> >> >
> >> > > +   if (offset >= TPS68470_N_REGULAR_GPIO) {
> >> > > +   offset -= TPS68470_N_REGULAR_GPIO;
> >> > > +   reg = TPS68470_REG_SGPO;
> >> > > +   }
> >> >
> >> > Two GPIO chips makes this gone.
> >
> > Again, I'm not really worried about this driver, but the ACPI tables. How
> > does the difference show there?
> 
> Same way. You will have common numbering over the chip [0, 9]. It will
> be just an abstraction inside the driver.

Oh, in that case that should be a non-issue.

> 
> > The outputs (s_enable, s_idle and s_resetn) are not numbered in the
> > documentation. There grouped, though, but the order in that grouping varies.
> 
> I don't get this. You are telling that the property of "always output"
> can be assigned to any 3 out of 10?

No, I'm telling you that the three (s_enable, s_idle and s_resetn) cannot be
configured as inputs --- instead they're always outputs. That's how the
hardware is implemented.

> Above states the opposite, so, it's clear to me that abstraction of 2
> GPIO chips over 1 can be utilized here.

Sounds fine to me, taken that this does not add complications to ACPI
tables.

-- 
Regards,

Sakari Ailus
e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk

Re: [PATCH v1 2/3] gpio: Add support for TPS68470 GPIOs

[Andy Shevchenko]

On Sun, Jun 11, 2017 at 2:30 PM, Sakari Ailus  wrote:
> On Sun, Jun 11, 2017 at 03:49:18AM +, Mani, Rajmohan wrote:
>> > On Tue, Jun 6, 2017 at 2:55 PM, Rajmohan Mani 
>> > wrote:

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

>> 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 
>> > > +#include 
>> > > +#include 
>> >
>> > > +   if (offset >= TPS68470_N_REGULAR_GPIO) {
>> > > +   offset -= TPS68470_N_REGULAR_GPIO;
>> > > +   reg = TPS68470_REG_SGPO;
>> > > +   }
>> >
>> > Two GPIO chips makes this gone.
>
> Again, I'm not really worried about this driver, but the ACPI tables. How
> does the difference show there?

Same way. You will have common numbering over the chip [0, 9]. It will
be just an abstraction inside the driver.

> The outputs (s_enable, s_idle and s_resetn) are not numbered in the
> documentation. There grouped, though, but the order in that grouping varies.

I don't get this. You are telling that the property of "always output"
can be assigned to any 3 out of 10?
Above states the opposite, so, it's clear to me that abstraction of 2
GPIO chips over 1 can be utilized here.

>> > > +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.

>> I have moved this code to the MFD driver
>
> This information should come from the ACPI tables, it should not be present
> in drivers. Why do you need it?

+1 (asked same in review of new version)

>> > > +   /*
>> > > +* 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.
>
> If that was the case, I bet you could expect interesting behaviour from the
> hardware connected to these pins.

+1. I don't believe the hardware / firmware doesn't care about clear
and always predictable state.

> For what it's worth, the chip documentation states that the reset value for
> the SGPO and GPDO registers is zero, as well as that GPIOs are configured as
> input and the reset value of the s_* outputs is low.
>
> In other words, I don't think that explicitly setting the values to zero has
> an effect.


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 2/3] gpio: Add support for TPS68470 GPIOs

[Andy Shevchenko]

On Sun, Jun 11, 2017 at 2:30 PM, Sakari Ailus  wrote:
> On Sun, Jun 11, 2017 at 03:49:18AM +, Mani, Rajmohan wrote:
>> > On Tue, Jun 6, 2017 at 2:55 PM, Rajmohan Mani 
>> > wrote:

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

>> 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 
>> > > +#include 
>> > > +#include 
>> >
>> > > +   if (offset >= TPS68470_N_REGULAR_GPIO) {
>> > > +   offset -= TPS68470_N_REGULAR_GPIO;
>> > > +   reg = TPS68470_REG_SGPO;
>> > > +   }
>> >
>> > Two GPIO chips makes this gone.
>
> Again, I'm not really worried about this driver, but the ACPI tables. How
> does the difference show there?

Same way. You will have common numbering over the chip [0, 9]. It will
be just an abstraction inside the driver.

> The outputs (s_enable, s_idle and s_resetn) are not numbered in the
> documentation. There grouped, though, but the order in that grouping varies.

I don't get this. You are telling that the property of "always output"
can be assigned to any 3 out of 10?
Above states the opposite, so, it's clear to me that abstraction of 2
GPIO chips over 1 can be utilized here.

>> > > +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.

>> I have moved this code to the MFD driver
>
> This information should come from the ACPI tables, it should not be present
> in drivers. Why do you need it?

+1 (asked same in review of new version)

>> > > +   /*
>> > > +* 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.
>
> If that was the case, I bet you could expect interesting behaviour from the
> hardware connected to these pins.

+1. I don't believe the hardware / firmware doesn't care about clear
and always predictable state.

> For what it's worth, the chip documentation states that the reset value for
> the SGPO and GPDO registers is zero, as well as that GPIOs are configured as
> input and the reset value of the s_* outputs is low.
>
> In other words, I don't think that explicitly setting the values to zero has
> an effect.


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 2/3] gpio: Add support for TPS68470 GPIOs

[Sakari Ailus]

Hi Rajmohan and Andy,

On Sun, Jun 11, 2017 at 03:49:18AM +, Mani, Rajmohan wrote:
> Hi Andy,
> 
> Thanks for the reviews and patience.
> 
> > 
> > On Tue, Jun 6, 2017 at 2:55 PM, Rajmohan Mani 
> > 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 
> > > +#include 
> > 
> > These shouldn't be in the driver.
> > Instead use
> > #include 
> > 
> 
> Ack
> 
> > > +#include 
> > > +#include 
> > > +#include 
> > 
> > > +   if (offset >= TPS68470_N_REGULAR_GPIO) {
> > > +   offset -= TPS68470_N_REGULAR_GPIO;
> > > +   reg = TPS68470_REG_SGPO;
> > > +   }
> > 
> > Two GPIO chips makes this gone.

Again, I'm not really worried about this driver, but the ACPI tables. How
does the difference show there?

The outputs (s_enable, s_idle and s_resetn) are not numbered in the
documentation. There grouped, though, but the order in that grouping varies.

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

This information should come from the ACPI tables, it should not be present
in drivers. Why do you need it?

> > > +   /*
> > > +* 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.

If that was the case, I bet you could expect interesting behaviour from the
hardware connected to these pins.

For what it's worth, the chip documentation states that the reset value for
the SGPO and GPDO registers is zero, as well as that GPIOs are configured as
input and the reset value of the s_* outputs is low.

In other words, I don't think that explicitly setting the values to zero has
an effect.

-- 
Regards,

Sakari Ailus
e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk

Re: [PATCH v1 2/3] gpio: Add support for TPS68470 GPIOs

[Sakari Ailus]

Hi Rajmohan and Andy,

On Sun, Jun 11, 2017 at 03:49:18AM +, Mani, Rajmohan wrote:
> Hi Andy,
> 
> Thanks for the reviews and patience.
> 
> > 
> > On Tue, Jun 6, 2017 at 2:55 PM, Rajmohan Mani 
> > 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 
> > > +#include 
> > 
> > These shouldn't be in the driver.
> > Instead use
> > #include 
> > 
> 
> Ack
> 
> > > +#include 
> > > +#include 
> > > +#include 
> > 
> > > +   if (offset >= TPS68470_N_REGULAR_GPIO) {
> > > +   offset -= TPS68470_N_REGULAR_GPIO;
> > > +   reg = TPS68470_REG_SGPO;
> > > +   }
> > 
> > Two GPIO chips makes this gone.

Again, I'm not really worried about this driver, but the ACPI tables. How
does the difference show there?

The outputs (s_enable, s_idle and s_resetn) are not numbered in the
documentation. There grouped, though, but the order in that grouping varies.

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

This information should come from the ACPI tables, it should not be present
in drivers. Why do you need it?

> > > +   /*
> > > +* 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.

If that was the case, I bet you could expect interesting behaviour from the
hardware connected to these pins.

For what it's worth, the chip documentation states that the reset value for
the SGPO and GPDO registers is zero, as well as that GPIOs are configured as
input and the reset value of the s_* outputs is low.

In other words, I don't think that explicitly setting the values to zero has
an effect.

-- 
Regards,

Sakari Ailus
e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk

RE: [PATCH v1 2/3] gpio: Add support for TPS68470 GPIOs

[Mani, Rajmohan]

Hi Linus,

Thanks for the reviews.

> -Original Message-
> From: Linus Walleij [mailto:linus.wall...@linaro.org]
> Sent: Friday, June 09, 2017 4:15 AM
> To: Mani, Rajmohan <rajmohan.m...@intel.com>
> Cc: linux-kernel@vger.kernel.org; linux-g...@vger.kernel.org; ACPI Devel
> Maling List <linux-a...@vger.kernel.org>; Lee Jones <lee.jo...@linaro.org>;
> Alexandre Courbot <gnu...@gmail.com>; Rafael J. Wysocki
> <r...@rjwysocki.net>; Len Brown <l...@kernel.org>
> Subject: Re: [PATCH v1 2/3] gpio: Add support for TPS68470 GPIOs
> 
> On Tue, Jun 6, 2017 at 1:55 PM, Rajmohan Mani <rajmohan.m...@intel.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.
> >
> > Signed-off-by: Rajmohan Mani <rajmohan.m...@intel.com>
> 
> Same comments as Andy already sent, plus:
> 
> > +static inline struct tps68470_gpio_data *to_gpio_data(struct
> > +gpio_chip *gpiochp) {
> > +   return container_of(gpiochp, struct tps68470_gpio_data, gc); }
> 
> Please use the data pointe inside gpio_chip.
> 

This is not clear to me.
The driver already gets the data pointer inside gpio_chip (which is gpiochp)
Is the name gpiochp misleading? 
I had to get rid of "i" in gpiochip, to meet 80 char limit.

> struct tps68470_gpio_data *foo = gpiochip_get_data(gc);
> 
> > +   ret = tps68470_reg_read(tps, reg, );
> (...)
> > +   tps68470_update_bits(tps, reg, BIT(offset), value ?
> > + BIT(offset) : 0);
> (...)
> > +   return tps68470_update_bits(tps, TPS68470_GPIO_CTL_REG_A(offset),
> > +TPS68470_GPIO_MODE_MASK,
> > +TPS68470_GPIO_MODE_OUT_CMOS);
> (...)
> > +   return tps68470_update_bits(tps, TPS68470_GPIO_CTL_REG_A(offset),
> > +  TPS68470_GPIO_MODE_MASK, 0x00);
> 
> This looks like a reimplementation of regmap. Is it not possible to create a
> regmap in the MFD driver and pass that around instead?
> 

Ack
Will be addressed in v2 of this patch series.

> > +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),
> > + {},
> > +   },
> > +};
> 
> Hm that's why you include  I guess.
> 

Ack

> This warrants a big comment above it explaining why this is done like that and
> what the use is inside any system with this chip mounted.
> 

Ack
I have added comments in the MFD driver, inside which the lookup table has 
moved.

> > +   tps68470_gpio->gc.label = "tps68470-gpio";
> > +   tps68470_gpio->gc.owner = THIS_MODULE;
> > +   tps68470_gpio->gc.direction_input = tps68470_gpio_input;
> > +   tps68470_gpio->gc.direction_output = tps68470_gpio_output;
> 
> Please implement .get_direction()
> 

Ack

> > +   tps68470_gpio->gc.get = tps68470_gpio_get;
> > +   tps68470_gpio->gc.set = tps68470_gpio_set;
> > +   tps68470_gpio->gc.can_sleep = true;
> > +   tps68470_gpio->gc.ngpio = TPS68470_N_GPIO;
> > +   tps68470_gpio->gc.base = -1;
> > +   tps68470_gpio->gc.parent = >dev;
> 
> Consider assigning the .names() array some sensible line names.
> 

Ack

> > +static int tps68470_gpio_remove(struct platform_device *pdev) {
> > +   struct tps68470_gpio_data *tps68470_gpio =
> > +platform_get_drvdata(pdev);
> > +
> > +   gpiod_remove_lookup_table(_table);
> > +   gpiochip_remove(_gpio->gc);
> 
> You can't use devm_gpiochip_add()?
> 

Originally I couldn't, because the driver can not remove the lookup table upon 
exit/removal. I moved this code inside the MFD driver, which is modified to use 
.shutdown() to remove the lookup table, so it can use dev_mfd_* call.

RE: [PATCH v1 2/3] gpio: Add support for TPS68470 GPIOs

[Mani, Rajmohan]

Hi Linus,

Thanks for the reviews.

> -Original Message-
> From: Linus Walleij [mailto:linus.wall...@linaro.org]
> Sent: Friday, June 09, 2017 4:15 AM
> To: Mani, Rajmohan 
> Cc: linux-kernel@vger.kernel.org; linux-g...@vger.kernel.org; ACPI Devel
> Maling List ; Lee Jones ;
> Alexandre Courbot ; Rafael J. Wysocki
> ; Len Brown 
> Subject: Re: [PATCH v1 2/3] gpio: Add support for TPS68470 GPIOs
> 
> On Tue, Jun 6, 2017 at 1:55 PM, Rajmohan Mani 
> 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.
> >
> > Signed-off-by: Rajmohan Mani 
> 
> Same comments as Andy already sent, plus:
> 
> > +static inline struct tps68470_gpio_data *to_gpio_data(struct
> > +gpio_chip *gpiochp) {
> > +   return container_of(gpiochp, struct tps68470_gpio_data, gc); }
> 
> Please use the data pointe inside gpio_chip.
> 

This is not clear to me.
The driver already gets the data pointer inside gpio_chip (which is gpiochp)
Is the name gpiochp misleading? 
I had to get rid of "i" in gpiochip, to meet 80 char limit.

> struct tps68470_gpio_data *foo = gpiochip_get_data(gc);
> 
> > +   ret = tps68470_reg_read(tps, reg, );
> (...)
> > +   tps68470_update_bits(tps, reg, BIT(offset), value ?
> > + BIT(offset) : 0);
> (...)
> > +   return tps68470_update_bits(tps, TPS68470_GPIO_CTL_REG_A(offset),
> > +TPS68470_GPIO_MODE_MASK,
> > +TPS68470_GPIO_MODE_OUT_CMOS);
> (...)
> > +   return tps68470_update_bits(tps, TPS68470_GPIO_CTL_REG_A(offset),
> > +  TPS68470_GPIO_MODE_MASK, 0x00);
> 
> This looks like a reimplementation of regmap. Is it not possible to create a
> regmap in the MFD driver and pass that around instead?
> 

Ack
Will be addressed in v2 of this patch series.

> > +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),
> > + {},
> > +   },
> > +};
> 
> Hm that's why you include  I guess.
> 

Ack

> This warrants a big comment above it explaining why this is done like that and
> what the use is inside any system with this chip mounted.
> 

Ack
I have added comments in the MFD driver, inside which the lookup table has 
moved.

> > +   tps68470_gpio->gc.label = "tps68470-gpio";
> > +   tps68470_gpio->gc.owner = THIS_MODULE;
> > +   tps68470_gpio->gc.direction_input = tps68470_gpio_input;
> > +   tps68470_gpio->gc.direction_output = tps68470_gpio_output;
> 
> Please implement .get_direction()
> 

Ack

> > +   tps68470_gpio->gc.get = tps68470_gpio_get;
> > +   tps68470_gpio->gc.set = tps68470_gpio_set;
> > +   tps68470_gpio->gc.can_sleep = true;
> > +   tps68470_gpio->gc.ngpio = TPS68470_N_GPIO;
> > +   tps68470_gpio->gc.base = -1;
> > +   tps68470_gpio->gc.parent = >dev;
> 
> Consider assigning the .names() array some sensible line names.
> 

Ack

> > +static int tps68470_gpio_remove(struct platform_device *pdev) {
> > +   struct tps68470_gpio_data *tps68470_gpio =
> > +platform_get_drvdata(pdev);
> > +
> > +   gpiod_remove_lookup_table(_table);
> > +   gpiochip_remove(_gpio->gc);
> 
> You can't use devm_gpiochip_add()?
> 

Originally I couldn't, because the driver can not remove the lookup table upon 
exit/removal. I moved this code inside the MFD driver, which is modified to use 
.shutdown() to remove the lookup table, so it can use dev_mfd_* call.

RE: [PATCH v1 2/3] gpio: Add support for TPS68470 GPIOs

[Mani, Rajmohan]

Hi Andy,

Thanks for the reviews and patience.

> 
> On Tue, Jun 6, 2017 at 2:55 PM, Rajmohan Mani 
> 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 
> > +#include 
> 
> These shouldn't be in the driver.
> Instead use
> #include 
> 

Ack

> > +#include 
> > +#include 
> > +#include 
> 
> > +   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(_gpio->gc);
> 
> devm_ ?
> 

Ack

> > +   gpiod_add_lookup_table(_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(_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(_table);
> > +   gpiochip_remove(_gpio->gc);
> > +
> > +   return 0;
> > +}
> 
> Should gone after devm_ in use.
> 

Ack

RE: [PATCH v1 2/3] gpio: Add support for TPS68470 GPIOs

[Mani, Rajmohan]

Hi Andy,

Thanks for the reviews and patience.

> 
> On Tue, Jun 6, 2017 at 2:55 PM, Rajmohan Mani 
> 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 
> > +#include 
> 
> These shouldn't be in the driver.
> Instead use
> #include 
> 

Ack

> > +#include 
> > +#include 
> > +#include 
> 
> > +   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(_gpio->gc);
> 
> devm_ ?
> 

Ack

> > +   gpiod_add_lookup_table(_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(_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(_table);
> > +   gpiochip_remove(_gpio->gc);
> > +
> > +   return 0;
> > +}
> 
> Should gone after devm_ in use.
> 

Ack

Re: [PATCH v1 2/3] gpio: Add support for TPS68470 GPIOs

[Linus Walleij]

On Tue, Jun 6, 2017 at 1:55 PM, Rajmohan Mani  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.
>
> Signed-off-by: Rajmohan Mani 

Same comments as Andy already sent, plus:

> +static inline struct tps68470_gpio_data *to_gpio_data(struct gpio_chip 
> *gpiochp)
> +{
> +   return container_of(gpiochp, struct tps68470_gpio_data, gc);
> +}

Please use the data pointe inside gpio_chip.

struct tps68470_gpio_data *foo = gpiochip_get_data(gc);

> +   ret = tps68470_reg_read(tps, reg, );
(...)
> +   tps68470_update_bits(tps, reg, BIT(offset), value ? BIT(offset) : 0);
(...)
> +   return tps68470_update_bits(tps, TPS68470_GPIO_CTL_REG_A(offset),
> +TPS68470_GPIO_MODE_MASK,
> +TPS68470_GPIO_MODE_OUT_CMOS);
(...)
> +   return tps68470_update_bits(tps, TPS68470_GPIO_CTL_REG_A(offset),
> +  TPS68470_GPIO_MODE_MASK, 0x00);

This looks like a reimplementation of regmap. Is it not possible
to create a regmap in the MFD driver and pass that around instead?

> +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),
> + {},
> +   },
> +};

Hm that's why you include  I guess.

This warrants a big comment above it explaining why this is done like
that and what the use is inside any system with this chip mounted.

> +   tps68470_gpio->gc.label = "tps68470-gpio";
> +   tps68470_gpio->gc.owner = THIS_MODULE;
> +   tps68470_gpio->gc.direction_input = tps68470_gpio_input;
> +   tps68470_gpio->gc.direction_output = tps68470_gpio_output;

Please implement .get_direction()

> +   tps68470_gpio->gc.get = tps68470_gpio_get;
> +   tps68470_gpio->gc.set = tps68470_gpio_set;
> +   tps68470_gpio->gc.can_sleep = true;
> +   tps68470_gpio->gc.ngpio = TPS68470_N_GPIO;
> +   tps68470_gpio->gc.base = -1;
> +   tps68470_gpio->gc.parent = >dev;

Consider assigning the .names() array some sensible line names.

> +static int tps68470_gpio_remove(struct platform_device *pdev)
> +{
> +   struct tps68470_gpio_data *tps68470_gpio = platform_get_drvdata(pdev);
> +
> +   gpiod_remove_lookup_table(_table);
> +   gpiochip_remove(_gpio->gc);

You can't use devm_gpiochip_add()?

Yours,
Linus Walleij

Re: [PATCH v1 2/3] gpio: Add support for TPS68470 GPIOs

[Linus Walleij]

On Tue, Jun 6, 2017 at 1:55 PM, Rajmohan Mani  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.
>
> Signed-off-by: Rajmohan Mani 

Same comments as Andy already sent, plus:

> +static inline struct tps68470_gpio_data *to_gpio_data(struct gpio_chip 
> *gpiochp)
> +{
> +   return container_of(gpiochp, struct tps68470_gpio_data, gc);
> +}

Please use the data pointe inside gpio_chip.

struct tps68470_gpio_data *foo = gpiochip_get_data(gc);

> +   ret = tps68470_reg_read(tps, reg, );
(...)
> +   tps68470_update_bits(tps, reg, BIT(offset), value ? BIT(offset) : 0);
(...)
> +   return tps68470_update_bits(tps, TPS68470_GPIO_CTL_REG_A(offset),
> +TPS68470_GPIO_MODE_MASK,
> +TPS68470_GPIO_MODE_OUT_CMOS);
(...)
> +   return tps68470_update_bits(tps, TPS68470_GPIO_CTL_REG_A(offset),
> +  TPS68470_GPIO_MODE_MASK, 0x00);

This looks like a reimplementation of regmap. Is it not possible
to create a regmap in the MFD driver and pass that around instead?

> +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),
> + {},
> +   },
> +};

Hm that's why you include  I guess.

This warrants a big comment above it explaining why this is done like
that and what the use is inside any system with this chip mounted.

> +   tps68470_gpio->gc.label = "tps68470-gpio";
> +   tps68470_gpio->gc.owner = THIS_MODULE;
> +   tps68470_gpio->gc.direction_input = tps68470_gpio_input;
> +   tps68470_gpio->gc.direction_output = tps68470_gpio_output;

Please implement .get_direction()

> +   tps68470_gpio->gc.get = tps68470_gpio_get;
> +   tps68470_gpio->gc.set = tps68470_gpio_set;
> +   tps68470_gpio->gc.can_sleep = true;
> +   tps68470_gpio->gc.ngpio = TPS68470_N_GPIO;
> +   tps68470_gpio->gc.base = -1;
> +   tps68470_gpio->gc.parent = >dev;

Consider assigning the .names() array some sensible line names.

> +static int tps68470_gpio_remove(struct platform_device *pdev)
> +{
> +   struct tps68470_gpio_data *tps68470_gpio = platform_get_drvdata(pdev);
> +
> +   gpiod_remove_lookup_table(_table);
> +   gpiochip_remove(_gpio->gc);

You can't use devm_gpiochip_add()?

Yours,
Linus Walleij

Re: [PATCH v1 2/3] gpio: Add support for TPS68470 GPIOs

[Andy Shevchenko]

On Tue, Jun 6, 2017 at 2:55 PM, Rajmohan Mani  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.

> +#include 
> +#include 

These shouldn't be in the driver.
Instead use
#include 

> +#include 
> +#include 
> +#include 

> +   if (offset >= TPS68470_N_REGULAR_GPIO) {
> +   offset -= TPS68470_N_REGULAR_GPIO;
> +   reg = TPS68470_REG_SGPO;
> +   }

Two GPIO chips makes this gone.

> +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.

> +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(_gpio->gc);

devm_ ?

> +   gpiod_add_lookup_table(_table);

Doesn't belong to the driver either.
I suppose it's a part of MFD (patch 1)

> +   /*
> +* 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?

> +*/
> +   for (i = 0; i < tps68470_gpio->gc.ngpio; i++)
> +   tps68470_gpio_set(_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(_table);
> +   gpiochip_remove(_gpio->gc);
> +
> +   return 0;
> +}

Should gone after devm_ in use.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 2/3] gpio: Add support for TPS68470 GPIOs

[Andy Shevchenko]

On Tue, Jun 6, 2017 at 2:55 PM, Rajmohan Mani  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.

> +#include 
> +#include 

These shouldn't be in the driver.
Instead use
#include 

> +#include 
> +#include 
> +#include 

> +   if (offset >= TPS68470_N_REGULAR_GPIO) {
> +   offset -= TPS68470_N_REGULAR_GPIO;
> +   reg = TPS68470_REG_SGPO;
> +   }

Two GPIO chips makes this gone.

> +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.

> +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(_gpio->gc);

devm_ ?

> +   gpiod_add_lookup_table(_table);

Doesn't belong to the driver either.
I suppose it's a part of MFD (patch 1)

> +   /*
> +* 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?

> +*/
> +   for (i = 0; i < tps68470_gpio->gc.ngpio; i++)
> +   tps68470_gpio_set(_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(_table);
> +   gpiochip_remove(_gpio->gc);
> +
> +   return 0;
> +}

Should gone after devm_ in use.

-- 
With Best Regards,
Andy Shevchenko