Linux-RTC Archive on lore.kernel.org
 help / color / Atom feed
From: "Vaittinen, Matti" <Matti.Vaittinen@fi.rohmeurope.com>
To: "bgolaszewski@baylibre.com" <bgolaszewski@baylibre.com>
Cc: "dmurphy@ti.com" <dmurphy@ti.com>,
	"linux-leds@vger.kernel.org" <linux-leds@vger.kernel.org>,
	"linux-rtc@vger.kernel.org" <linux-rtc@vger.kernel.org>,
	"linux-gpio@vger.kernel.org" <linux-gpio@vger.kernel.org>,
	"alexandre.belloni@bootlin.com" <alexandre.belloni@bootlin.com>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"mturquette@baylibre.com" <mturquette@baylibre.com>,
	"lgirdwood@gmail.com" <lgirdwood@gmail.com>,
	"jacek.anaszewski@gmail.com" <jacek.anaszewski@gmail.com>,
	"mazziesaccount@gmail.com" <mazziesaccount@gmail.com>,
	"a.zummo@towertech.it" <a.zummo@towertech.it>,
	"linus.walleij@linaro.org" <linus.walleij@linaro.org>,
	"mark.rutland@arm.com" <mark.rutland@arm.com>,
	"robh+dt@kernel.org" <robh+dt@kernel.org>,
	"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
	"linux-clk@vger.kernel.org" <linux-clk@vger.kernel.org>,
	"pavel@ucw.cz" <pavel@ucw.cz>,
	"sboyd@kernel.org" <sboyd@kernel.org>,
	"broonie@kernel.org" <broonie@kernel.org>,
	"lee.jones@linaro.org" <lee.jones@linaro.org>
Subject: Re: [RFC PATCH 10/13] gpio: bd71828: Initial support for ROHM BD71828 PMIC GPIOs
Date: Mon, 21 Oct 2019 07:00:34 +0000
Message-ID: <3ae3507649f2e9a66053a99b4a71e29786fc3d34.camel@fi.rohmeurope.com> (raw)
In-Reply-To: <CAMpxmJWXQccY8HsM6MXYBW8KC0U+7iOk+Ve-4nk=cpa=Zuk1cg@mail.gmail.com>

Hello Bartosz,

Thanks for reading this through! I'll rework this patch during this
week :)

On Thu, 2019-10-17 at 14:45 +0200, Bartosz Golaszewski wrote:
> czw., 17 paź 2019 o 11:53 Matti Vaittinen
> <matti.vaittinen@fi.rohmeurope.com> napisał(a):
> > 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
> > rusts 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.
> > 
> > Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
> > 
> > +++ b/drivers/gpio/gpio-bd71828.c
> > @@ -0,0 +1,161 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +// Copyright (C) 2018 ROHM Semiconductors
> > +// gpio-bd71828.c ROHM BD71828 gpio driver
> 
> I don't think the name of the source file is needed here.

I Agree.

> 
> > +
> > +#include <linux/gpio/driver.h>
> > +#include <linux/mfd/rohm-bd71828.h>
> > +#include <linux/module.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/regmap.h>
> > +
> > +#define OUT 0
> > +#define IN 1
> 
> If you really want to define those, please use a common prefix for
> all
> symbols in the driver.

I prefer defining them because I always need to check the meaning of
these values. My brains just refuse from remembering which value is
used for in and which for out. I will add the prefix even though the
scope of these defines is limited to this file :)

> 
> > +#define GPIO_OUT_REG(off) (BD71828_REG_GPIO_CTRL1 + (off))
> > +#define HALL_GPIO_OFFSET 3
> > +
> > +struct bd71828_gpio {
> > +       struct rohm_regmap_dev chip;
> > +       struct gpio_chip gpio;
> > +};
> > +
> > +static void bd71828_gpio_set(struct gpio_chip *chip, unsigned int
> > offset,
> > +                            int value)
> > +{
> > +       int ret;
> > +       struct bd71828_gpio *bdgpio = gpiochip_get_data(chip);
> > +       u8 val = (value) ? BD71828_GPIO_OUT_HI :
> > BD71828_GPIO_OUT_LO;
> > +
> > +       if (offset == HALL_GPIO_OFFSET)
> > +               return;
> 
> Can you add a comment here saying that this pin can only be used as
> input? Otherwise this information is only available in the commit
> message.

Sure thing. I thought the comment in get_direction was sufficient but
you are correct - it's nice to see this limitation also here.

> > +
> > +       ret = regmap_update_bits(bdgpio->chip.regmap,
> > GPIO_OUT_REG(offset),
> > +                                BD71828_GPIO_OUT_MASK, val);
> > +       if (ret)
> > +               dev_err(bdgpio->chip.dev, "Could not set gpio to
> > %d\n", value);
> > +}
> > +
> > +static int bd71828_gpio_get(struct gpio_chip *chip, unsigned int
> > offset)
> > +{
> > +       int ret;
> > +       unsigned int val;
> > +       struct bd71828_gpio *bdgpio = gpiochip_get_data(chip);
> > +
> > +       if (offset == HALL_GPIO_OFFSET)
> > +               ret = regmap_read(bdgpio->chip.regmap,
> > BD71828_REG_IO_STAT,
> > +                                 &val);
> > +       else
> > +               ret = regmap_read(bdgpio->chip.regmap,
> > GPIO_OUT_REG(offset),
> > +                                 &val);
> > +       if (!ret)
> > +               ret = (val & BD71828_GPIO_OUT_MASK);
> > +
> > +       return ret;
> > +}
> > +
> > +static int bd71828_gpio_set_config(struct gpio_chip *chip,
> > unsigned int offset,
> > +                                  unsigned long config)
> > +{
> > +       struct bd71828_gpio *bdgpio = gpiochip_get_data(chip);
> > +
> > +       if (offset == HALL_GPIO_OFFSET)
> > +               return -ENOTSUPP;
> > +
> > +       switch (pinconf_to_config_param(config)) {
> > +       case PIN_CONFIG_DRIVE_OPEN_DRAIN:
> > +               return regmap_update_bits(bdgpio->chip.regmap,
> > +                                         GPIO_OUT_REG(offset),
> > +                                         BD71828_GPIO_DRIVE_MASK,
> > +                                         BD71828_GPIO_OPEN_DRAIN);
> > +       case PIN_CONFIG_DRIVE_PUSH_PULL:
> > +               return regmap_update_bits(bdgpio->chip.regmap,
> > +                                         GPIO_OUT_REG(offset),
> > +                                         BD71828_GPIO_DRIVE_MASK,
> > +                                         BD71828_GPIO_PUSH_PULL);
> > +       default:
> > +               break;
> > +       }
> > +       return -ENOTSUPP;
> +}+static int bd71828_get_direction(struct gpio_chip *chip, unsigned
> int offset)
> > +{
> > +       /*
> > +        * Pin usage is selected by OTP data. We can't read it
> > runtime. Hence
> > +        * we trust that if the pin is not excluded by "gpio-
> > reserved-ranges"
> > +        * the OTP configuration is set to OUT. (Other pins but
> > HALL input pin
> > +        * on BD71828 can't really be used for general purpose
> > input - input
> > +        * states are used for specific cases like regulator
> > control or
> > +        * PMIC_ON_REQ.
> > +        */
> > +       if (offset == HALL_GPIO_OFFSET)
> > +               return IN;
> > +
> > +       return OUT;
> > +}
> > +
> > +static int bd71828_gpio_parse_dt(struct device *dev,
> > +                                struct bd71828_gpio *bdgpio)
> > +{
> > +       /*
> > +        * TBD: See if we need some implementation to mark some
> > PINs as
> > +        * not controllable based on DT info or if core can handle
> > +        * "gpio-reserved-ranges" and exclude them from control
> > +        */
> > +       return 0;
> > +}
> 
> Please don't implement empty functions. Just add this comment next to
> gpiochip's initialization...

Yep. I should have cleaned this before sending even this. Thanks for
pointing it out!

> 
> 
> > +
> > +       bdgpio->chip.dev = &pdev->dev;
> > +       bdgpio->gpio.parent = pdev->dev.parent;
> > +       bdgpio->gpio.label = "bd71828-gpio";
> > +       bdgpio->gpio.owner = THIS_MODULE;
> > +       bdgpio->gpio.get_direction = bd71828_get_direction;
> > +       bdgpio->gpio.set_config = bd71828_gpio_set_config;
> > +       bdgpio->gpio.can_sleep = true;
> > +       bdgpio->gpio.get = bd71828_gpio_get;
> > +       bdgpio->gpio.set = bd71828_gpio_set;
> 
> Not implementing direction_output() and direction_input() here will
> results in warnings from the GPIO framework: for instance you
> implement set() but not direction_output(). I'd say: just add those
> callbacks and return an error if they're called for invalid lines
> (for
> instance: direction_output() being called for line 3).

Ok. I will implement dummy functions.

But out of the curiosity - why the GPIO core emits the warnings if
these are not implemented? I think the core should not require "no-
operation" functions to be implemented for pins which don't support
both of the directions. GPIO core could only emit warning if it needs
to set direction to something the HW does not support. That would avoid
adding the dummy functions to all of the drivers, right?

> 
> > +       bdgpio->gpio.base = -1;
> > +       bdgpio->gpio.ngpio = 4;
> > +#ifdef CONFIG_OF_GPIO
> 
> This is not needed - for CONFIG_OF_GPIO disabled the parent of_node
> will be NULL.

Right. Thanks.

> > +       bdgpio->gpio.of_node = pdev->dev.parent->of_node;
> > +#endif
> > +       bdgpio->chip.regmap = bd71828->regmap;
> > +
> > +       ret = devm_gpiochip_add_data(&pdev->dev, &bdgpio->gpio,
> > +                                    bdgpio);
> > +       if (ret)
> > +               dev_err(&pdev->dev, "gpio_init: Failed to add
> > bd71828-gpio\n");
> 
> Since there aren't many places where this function can fail, you can
> directly return devm_gpiochip_add_data() here.

Ok.

> > +
> > +       return ret;
> > +}
> > +
> > +static struct platform_driver bd71828_gpio = {
> > +       .driver = {
> > +               .name = "bd71828-gpio"
> > +       },
> > +       .probe = bd71828_probe,
> > +};
> > +
> > +module_platform_driver(bd71828_gpio);
> > +
> > +MODULE_AUTHOR("Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
> > ");
> > +MODULE_DESCRIPTION("BD71828 voltage regulator driver");
> > +MODULE_LICENSE("GPL");
> 
> Don't you need a MODULE_ALIAS() here since this is an MFD sub-module?

I must admit I don't know the details of how module loading is done. I
used system where modules are load by scripts. (I guess the module
alias could be used to allow automatic module loading [by udev?])

Can you please educate me - If I add module aliases matching the sub-
device name given in in MFD cell - should the sub module loading be
automatic when MFD driver gets probed? For some reason I didn't get
that working on my test bed. Or maybe I misunderstood something.

Eg, this should be enough for GPIO sub-module to be also load:

MFD:
static struct mfd_cell bd71828_mfd_cells[] = {
        { .name = "bd71828-pmic", },
        { .name = "bd71828-gpio", },
...
ret = devm_mfd_add_devices(&i2c->dev, PLATFORM_DEVID_AUTO,
                           bd71828_mfd_cells,
                           ARRAY_SIZE(bd71828_mfd_cells), NULL, 0,
                           regmap_irq_get_domain(irq_data)); 

GPIO driver:
MODULE_ALIAS("platform:bd71828-gpio");

I had the sub-devices probed even without the MODULE_ALIAS - but manual
loading is required. I will gladly add the alias if it should enable
the automatic module loading.

Br,
	Matti Vaittinen


  reply index

Thread overview: 43+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-10-17  9:40 [RFC PATCH 00/13] Support ROHM BD71828 PMIC Matti Vaittinen
2019-10-17  9:41 ` [RFC PATCH 01/13] mfd: bd71828: Support ROHM BD71828 PMIC - core Matti Vaittinen
2019-10-17  9:42 ` [RFC PATCH 02/13] mfd: input: bd71828: Add power-key support Matti Vaittinen
2019-10-17  9:43 ` [RFC PATCH 03/13] clk: bd718x7: Support ROHM BD71828 clk block Matti Vaittinen
2019-10-17  9:44 ` [RFC PATCH 04/13] regulator: bd718x7: Split driver to common and bd718x7 specific parts Matti Vaittinen
2019-10-17  9:44 ` [RFC PATCH 05/13] regulator: bd71828: Basic support for ROHM bd71828 PMIC regulators Matti Vaittinen
2019-10-17  9:48 ` [RFC PATCH 06/13] regulator: bd71828: Add GPIO based run-level control for regulators Matti Vaittinen
2019-10-17  9:50 ` [RFC PATCH 07/13] regulator: bd71828: enhanced run-level support Matti Vaittinen
2019-10-17  9:51 ` [RFC PATCH 08/13] regulator: bd71828: Support in-kernel APIs to change run-level Matti Vaittinen
2019-10-17  9:52 ` [RFC PATCH 09/13] mfd: rtc: support RTC on ROHM BD71828 with BD70528 driver Matti Vaittinen
2019-10-17 10:12   ` Alexandre Belloni
2019-10-17 10:36     ` Vaittinen, Matti
2019-10-17 10:48       ` Alexandre Belloni
2019-10-21  5:29         ` Vaittinen, Matti
2019-10-23 10:27         ` Vaittinen, Matti
2019-10-29 13:50           ` Alexandre Belloni
2019-10-29 14:08             ` Vaittinen, Matti
2019-10-17  9:53 ` [RFC PATCH 10/13] gpio: bd71828: Initial support for ROHM BD71828 PMIC GPIOs Matti Vaittinen
2019-10-17 12:45   ` Bartosz Golaszewski
2019-10-21  7:00     ` Vaittinen, Matti [this message]
2019-10-21 14:36       ` Bartosz Golaszewski
2019-10-21 14:56         ` Vaittinen, Matti
2019-10-22 13:19         ` Vaittinen, Matti
2019-10-17  9:53 ` [RFC PATCH 11/13] led: bd71828: Support LED outputs on ROHM BD71828 PMIC Matti Vaittinen
2019-10-17 14:04   ` Dan Murphy
2019-10-17 14:28     ` Alexandre Belloni
2019-10-21  8:00     ` Vaittinen, Matti
2019-10-21 19:09       ` Jacek Anaszewski
2019-10-22 12:40         ` Vaittinen, Matti
2019-10-22 17:40           ` Jacek Anaszewski
2019-10-23  8:37             ` Vaittinen, Matti
2019-10-23 21:59               ` Jacek Anaszewski
2019-10-24  8:15                 ` Vaittinen, Matti
2019-10-24 22:04                   ` Jacek Anaszewski
2019-10-25  7:07                     ` Vaittinen, Matti
2019-10-25 13:24                       ` Rob Herring
2019-10-25 14:37                         ` Vaittinen, Matti
2019-10-25 15:47                           ` Rob Herring
2019-10-29 13:29                             ` Vaittinen, Matti
2019-10-17  9:55 ` [RFC PATCH 12/13] dt-bindings: mfd: Document ROHM BD71282 bindings Matti Vaittinen
2019-10-17 14:18   ` Dan Murphy
2019-10-21  8:03     ` Vaittinen, Matti
2019-10-17  9:57 ` [RFC PATCH 13/13] dt-bindings: regulator: Document ROHM BD71282 regulator bindings Matti Vaittinen

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=3ae3507649f2e9a66053a99b4a71e29786fc3d34.camel@fi.rohmeurope.com \
    --to=matti.vaittinen@fi.rohmeurope.com \
    --cc=a.zummo@towertech.it \
    --cc=alexandre.belloni@bootlin.com \
    --cc=bgolaszewski@baylibre.com \
    --cc=broonie@kernel.org \
    --cc=devicetree@vger.kernel.org \
    --cc=dmurphy@ti.com \
    --cc=jacek.anaszewski@gmail.com \
    --cc=lee.jones@linaro.org \
    --cc=lgirdwood@gmail.com \
    --cc=linus.walleij@linaro.org \
    --cc=linux-clk@vger.kernel.org \
    --cc=linux-gpio@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-leds@vger.kernel.org \
    --cc=linux-rtc@vger.kernel.org \
    --cc=mark.rutland@arm.com \
    --cc=mazziesaccount@gmail.com \
    --cc=mturquette@baylibre.com \
    --cc=pavel@ucw.cz \
    --cc=robh+dt@kernel.org \
    --cc=sboyd@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-RTC Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-rtc/0 linux-rtc/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-rtc linux-rtc/ https://lore.kernel.org/linux-rtc \
		linux-rtc@vger.kernel.org
	public-inbox-index linux-rtc

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-rtc


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git