linux-kernel - Re: [RFC PATCH v2 12/13] gpio: bd71828: Initial support for ROHM BD71828 PMIC 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: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 24 Oct 2019 13:34:27 +0000
From:   "Vaittinen, Matti" <Matti.Vaittinen@...rohmeurope.com>
To:     "linus.walleij@...aro.org" <linus.walleij@...aro.org>
CC:     "dmurphy@...com" <dmurphy@...com>,
        "linux-leds@...r.kernel.org" <linux-leds@...r.kernel.org>,
        "linux-rtc@...r.kernel.org" <linux-rtc@...r.kernel.org>,
        "linux-gpio@...r.kernel.org" <linux-gpio@...r.kernel.org>,
        "alexandre.belloni@...tlin.com" <alexandre.belloni@...tlin.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "mturquette@...libre.com" <mturquette@...libre.com>,
        "lgirdwood@...il.com" <lgirdwood@...il.com>,
        "jacek.anaszewski@...il.com" <jacek.anaszewski@...il.com>,
        "mazziesaccount@...il.com" <mazziesaccount@...il.com>,
        "a.zummo@...ertech.it" <a.zummo@...ertech.it>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "mark.rutland@....com" <mark.rutland@....com>,
        "robh+dt@...nel.org" <robh+dt@...nel.org>,
        "bgolaszewski@...libre.com" <bgolaszewski@...libre.com>,
        "linux-clk@...r.kernel.org" <linux-clk@...r.kernel.org>,
        "pavel@....cz" <pavel@....cz>,
        "sboyd@...nel.org" <sboyd@...nel.org>,
        "broonie@...nel.org" <broonie@...nel.org>,
        "lee.jones@...aro.org" <lee.jones@...aro.org>
Subject: Re: [RFC PATCH v2 12/13] gpio: bd71828: Initial support for ROHM
 BD71828 PMIC GPIOs

Hello Linus,

Thanks again for the review :) Highly appreciated!

On Thu, 2019-10-24 at 13:59 +0200, Linus Walleij wrote:
> On Thu, Oct 24, 2019 at 1:51 PM Matti Vaittinen
> <matti.vaittinen@...rohmeurope.com> wrote:
> 
> > ROHM BD71828 PMIC contains 4 pins which can be configured by OTP
> > to be used for general purposes. First 3 can be used as outputs
> > and 4.th pin can be used as input. Allow them to be controlled
> > via GPIO framework.
> > 
> > The driver assumes all of the pins are configured as GPIOs and
> > trusts that the reserved pins in other OTP configurations are
> > excluded from control using "gpio-reserved-ranges" device tree
> > property (or left untouched by GPIO users).
> > 
> > Typical use for 4.th pin (input) is to use it as HALL sensor
> > input so that this pin state is toggled when HALL sensor detects
> > LID position change (from close to open or open to close). PMIC
> > HW implements some extra logic which allows PMIC to power-up the
> > system when this pin is toggled. Please see the data sheet for
> > details of GPIO options which can be selcted by OTP settings.
> 
> spelling of selected

Right, thanks.

> > Signed-off-by: Matti Vaittinen <matti.vaittinen@...rohmeurope.com>
> 
> Overall looks very good.
> 
> > +// SPDX-License-Identifier: GPL-2.0
> 
> I think they want you to use GPL-2.0-only these days.

Hmm. Is this documented somewhere? The LICENSES/preferred/GPL-2.0
still states:
Valid-License-Identifier: GPL-2.0
Valid-License-Identifier: GPL-2.0-only
Valid-License-Identifier: GPL-2.0+
Valid-License-Identifier: GPL-2.0-or-later
SPDX-URL: https://spdx.org/licenses/GPL-2.0.html
Usage-Guide:
  To use this license in source code, put one of the following SPDX
  tag/value pairs into a comment according to the placement
  guidelines in the licensing rules documentation.
  For 'GNU General Public License (GPL) version 2 only' use:
    SPDX-License-Identifier: GPL-2.0
  or
    SPDX-License-Identifier: GPL-2.0-only
  For 'GNU General Public License (GPL) version 2 or any later version'
use:
    SPDX-License-Identifier: GPL-2.0+
  or
    SPDX-License-Identifier: GPL-2.0-or-later

(at least for 5.4-rc2)

This looks like "GPL-2.0" should still be valid. Not that I have
anything against the "GPL-2.0-only" - I am just a tiny bit frustrated
what comes to polishing the SPDX format :/

> > +#define BD71828_OUT 0
> > +#define BD71828_IN 1
> 
> These have nothing to do with BD71828, just skip these defines
> and hardcode 0/1 in the code called from gpiolib.

I personally don't like using 0/1 as I _always_ need to check which is
IN and which is OUT. It must be somehow related to my brain chemistry
but this just does not stay in my mind over a weekend... But you are
right - having these defines in bd71828 driver does not help much. When
I start with next GPIO chip I must once again dig the values from these
defines or from somewhere else. Thus...

>  If we want defines
> for this they should be generically named and put in
> <linux/gpio/driver.h>

...placing the defines in this header which is visible also in next
driver would be "the right thing to do"(tm).

Do you think we should add the defines there then? I was not trying to
do that as I thought that someone really loves the raw numbers and
dislikes defines - as these hard-coded values 1 and 0 seem to be used
everywhere... :] Maybe the plain numbers are only difficult for me?

> Nice use of the config API!

Thanks =) I admit I have stolen that from some other driver. So I don't
really deserve the credits =) It's easy to build when the foundations
are done well ;)

Br,
	Matti Vaittinen