Linux-RTC Archive on lore.kernel.org
 help / color / Atom feed
From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
To: "Vaittinen, Matti" <Matti.Vaittinen@fi.rohmeurope.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 16:36:47 +0200
Message-ID: <CAMpxmJVABg-UAzZtaQKu5ADBhi1P7CNArmstxHi5ZfdPiSKyYw@mail.gmail.com> (raw)
In-Reply-To: <3ae3507649f2e9a66053a99b4a71e29786fc3d34.camel@fi.rohmeurope.com>

pon., 21 paź 2019 o 09:00 Vaittinen, Matti
<Matti.Vaittinen@fi.rohmeurope.com> napisał(a):
>
> 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?
>

I looked at the code again and it seems I was wrong. If we don't have
direction_input() or direction_output() we check the actual direction
with get_direction() before emitting any warnings and if there's no
direction_output(), but line is in input mode then all's fine. In
other words: false alarm, and you can keep it this way.

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

If the gpio module is a sub-node on the device tree than you may need
to use a sub-compatible to get the module loaded by udev.

Bart

> 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
2019-10-21 14:36       ` Bartosz Golaszewski [this message]
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=CAMpxmJVABg-UAzZtaQKu5ADBhi1P7CNArmstxHi5ZfdPiSKyYw@mail.gmail.com \
    --to=bgolaszewski@baylibre.com \
    --cc=Matti.Vaittinen@fi.rohmeurope.com \
    --cc=a.zummo@towertech.it \
    --cc=alexandre.belloni@bootlin.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