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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAHp75VeJaQjWb1834Nt7178GL72rcym=d3zV8tPpvJ5qKDuM_Q@mail.gmail.com>
Date:   Sun, 11 Jun 2017 16:40:16 +0300
From:   Andy Shevchenko <andy.shevchenko@...il.com>
To:     Sakari Ailus <sakari.ailus@....fi>
Cc:     "Mani, Rajmohan" <rajmohan.mani@...el.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-gpio@...r.kernel.org" <linux-gpio@...r.kernel.org>,
        "linux-acpi@...r.kernel.org" <linux-acpi@...r.kernel.org>,
        Lee Jones <lee.jones@...aro.org>,
        Linus Walleij <linus.walleij@...aro.org>,
        Alexandre Courbot <gnurou@...il.com>,
        "Rafael J. Wysocki" <rjw@...ysocki.net>,
        Len Brown <lenb@...nel.org>
Subject: Re: [PATCH v1 2/3] gpio: Add support for TPS68470 GPIOs

On Sun, Jun 11, 2017 at 2:30 PM, Sakari Ailus <sakari.ailus@....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@...el.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