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
[off-list ref] wrote:

quoted

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.

quoted

Signed-off-by: Matti Vaittinen <redacted>

Overall looks very good.

quoted

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

quoted

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