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@xxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxx> > > 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
