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 <sakari.ailus@xxxxxx> 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@xxxxxxxxx>
> >> > 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@xxxxxx	XMPP: sailus@xxxxxxxxxxxxxx