Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1663033 > unrolled thread

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

Started by"Mani, Rajmohan" <rajmohan.mani@intel.com>
First post2017-06-11 05:50 +0200
Last post2017-06-12 12:00 +0200
Articles 8 — 3 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  RE: [PATCH v1 2/3] gpio: Add support for TPS68470 GPIOs "Mani, Rajmohan" <rajmohan.mani@intel.com> - 2017-06-11 05:50 +0200
    Re: [PATCH v1 2/3] gpio: Add support for TPS68470 GPIOs Sakari Ailus <sakari.ailus@iki.fi> - 2017-06-11 13:40 +0200
      Re: [PATCH v1 2/3] gpio: Add support for TPS68470 GPIOs Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-06-11 16:20 +0200
        Re: [PATCH v1 2/3] gpio: Add support for TPS68470 GPIOs Sakari Ailus <sakari.ailus@iki.fi> - 2017-06-11 19:00 +0200
          Re: [PATCH v1 2/3] gpio: Add support for TPS68470 GPIOs Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-06-11 21:50 +0200
            RE: [PATCH v1 2/3] gpio: Add support for TPS68470 GPIOs "Mani, Rajmohan" <rajmohan.mani@intel.com> - 2017-06-12 11:20 +0200
              Re: [PATCH v1 2/3] gpio: Add support for TPS68470 GPIOs Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-06-12 11:30 +0200
      RE: [PATCH v1 2/3] gpio: Add support for TPS68470 GPIOs "Mani, Rajmohan" <rajmohan.mani@intel.com> - 2017-06-12 12:00 +0200

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

From"Mani, Rajmohan" <rajmohan.mani@intel.com>
Date2017-06-11 05:50 +0200
SubjectRE: [PATCH v1 2/3] gpio: Add support for TPS68470 GPIOs
Message-ID<tR22Z-8m8-1@gated-at.bofh.it>
Hi Andy,

Thanks for the reviews and patience.

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

[toc] | [next] | [standalone]

#1663093

FromSakari Ailus <sakari.ailus@iki.fi>
Date2017-06-11 13:40 +0200
Message-ID<tR9nP-4uK-1@gated-at.bofh.it>
In reply to#1663033
Hi Rajmohan and Andy,

On Sun, Jun 11, 2017 at 03:49:18AM +0000, Mani, Rajmohan wrote:
> Hi Andy,
> 
> Thanks for the reviews and patience.
> 
> > 
> > On Tue, Jun 6, 2017 at 2:55 PM, Rajmohan Mani <rajmohan.mani@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 <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.

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.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

[toc] | [prev] | [next] | [standalone]

#1663109

FromAndy Shevchenko <andy.shevchenko@gmail.com>
Date2017-06-11 16:20 +0200
Message-ID<tRbSF-65f-1@gated-at.bofh.it>
In reply to#1663093
On Sun, Jun 11, 2017 at 2:30 PM, Sakari Ailus <sakari.ailus@iki.fi> wrote:
> On Sun, Jun 11, 2017 at 03:49:18AM +0000, Mani, Rajmohan wrote:
>> > On Tue, Jun 6, 2017 at 2:55 PM, Rajmohan Mani <rajmohan.mani@intel.com>
>> > 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 <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.
>
> 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

[toc] | [prev] | [next] | [standalone]

#1663123

FromSakari Ailus <sakari.ailus@iki.fi>
Date2017-06-11 19:00 +0200
Message-ID<tRenv-7qC-9@gated-at.bofh.it>
In reply to#1663109
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 <sakari.ailus@iki.fi> wrote:
> > On Sun, Jun 11, 2017 at 03:49:18AM +0000, Mani, Rajmohan wrote:
> >> > On Tue, Jun 6, 2017 at 2:55 PM, Rajmohan Mani <rajmohan.mani@intel.com>
> >> > 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 <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.
> >
> > 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.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

[toc] | [prev] | [next] | [standalone]

#1663137

FromAndy Shevchenko <andy.shevchenko@gmail.com>
Date2017-06-11 21:50 +0200
Message-ID<tRh21-Dj-1@gated-at.bofh.it>
In reply to#1663123
On Sun, Jun 11, 2017 at 7:50 PM, Sakari Ailus <sakari.ailus@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.ailus@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).

-- 
With Best Regards,
Andy Shevchenko

[toc] | [prev] | [next] | [standalone]

#1663350

From"Mani, Rajmohan" <rajmohan.mani@intel.com>
Date2017-06-12 11:20 +0200
Message-ID<tRtFU-fr-25@gated-at.bofh.it>
In reply to#1663137
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.ailus@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.ailus@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

[toc] | [prev] | [next] | [standalone]

#1663362

FromAndy Shevchenko <andy.shevchenko@gmail.com>
Date2017-06-12 11:30 +0200
Message-ID<tRtPA-iC-7@gated-at.bofh.it>
In reply to#1663350
On Mon, Jun 12, 2017 at 12:17 PM, Mani, Rajmohan
<rajmohan.mani@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.ailus@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.ailus@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

[toc] | [prev] | [next] | [standalone]

#1663391

From"Mani, Rajmohan" <rajmohan.mani@intel.com>
Date2017-06-12 12:00 +0200
Message-ID<tRuiC-sU-17@gated-at.bofh.it>
In reply to#1663093
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 +0000, Mani, Rajmohan wrote:
> > Hi Andy,
> >
> > Thanks for the reviews and patience.
> >
> > >
> > > On Tue, Jun 6, 2017 at 2:55 PM, Rajmohan Mani
> > > <rajmohan.mani@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 <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.
> 
> 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.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web