Linux-LEDs Archive on lore.kernel.org
 help / color / Atom feed
From: "Vaittinen, Matti" <Matti.Vaittinen@fi.rohmeurope.com>
To: "linus.walleij@linaro.org" <linus.walleij@linaro.org>
Cc: "corbet@lwn.net" <corbet@lwn.net>,
	"linux-doc@vger.kernel.org" <linux-doc@vger.kernel.org>,
	"phil.edworthy@renesas.com" <phil.edworthy@renesas.com>,
	"linux-rtc@vger.kernel.org" <linux-rtc@vger.kernel.org>,
	"dmurphy@ti.com" <dmurphy@ti.com>,
	"linux-leds@vger.kernel.org" <linux-leds@vger.kernel.org>,
	"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
	"jeffrey.t.kirsher@intel.com" <jeffrey.t.kirsher@intel.com>,
	"linux-gpio@vger.kernel.org" <linux-gpio@vger.kernel.org>,
	"mchehab+samsung@kernel.org" <mchehab+samsung@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>,
	"hkallweit1@gmail.com" <hkallweit1@gmail.com>,
	"wsa+renesas@sang-engineering.com"
	<wsa+renesas@sang-engineering.com>,
	"m.szyprowski@samsung.com" <m.szyprowski@samsung.com>,
	"mark.rutland@arm.com" <mark.rutland@arm.com>,
	"robh+dt@kernel.org" <robh+dt@kernel.org>,
	"hofrat@osadl.org" <hofrat@osadl.org>,
	"bgolaszewski@baylibre.com" <bgolaszewski@baylibre.com>,
	"linux-clk@vger.kernel.org" <linux-clk@vger.kernel.org>,
	"lee.jones@linaro.org" <lee.jones@linaro.org>,
	"pavel@ucw.cz" <pavel@ucw.cz>,
	"broonie@kernel.org" <broonie@kernel.org>,
	"sboyd@kernel.org" <sboyd@kernel.org>
Subject: Re: [PATCH v5 10/16] gpio: devres: Add devm_gpiod_get_parent_array
Date: Tue, 19 Nov 2019 17:54:24 +0000
Message-ID: <ece1ab1418e237d6f4968fc4cf59202c35f02ba7.camel@fi.rohmeurope.com> (raw)
In-Reply-To: <CACRpkdY_2WzAnK01bQdMF69KsDvHHu9TXuyRoBcmiQMziux=eQ@mail.gmail.com>

Hello Linus,

On Tue, 2019-11-19 at 15:43 +0100, Linus Walleij wrote:
> On Mon, Nov 18, 2019 at 7:58 AM Matti Vaittinen
> <matti.vaittinen@fi.rohmeurope.com> wrote:
> 
> > Bunch of MFD sub-devices which are instantiated by MFD do not have
> > own device-tree nodes but have (for example) the GPIO consumer
> > information in parent device's DT node. Add resource managed
> > devm_gpiod_get_array() for such devices so that they can get the
> > consumer information from parent DT while still binding the GPIO
> > reservation life-time to this sub-device life time.
> > 
> > If devm_gpiod_get_array is used as such - then unloading and then
> > re-loading the child device fails as the GPIOs reserved during
> > first
> > load are not freed when driver for sub-device is unload (if parent
> > stays there).
> > 
> > Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
> (...)
> > +static struct gpio_descs *__must_check
> > +__devm_gpiod_get_array(struct device *gpiodev,
> > +                      struct device *managed,
> > +                      const char *con_id,
> > +                      enum gpiod_flags flags)
> 
> I'm opposed to functions named __underscore_something()
> so find a proper name for this function.
> devm_gpiod_get_array_common() works if nothing else.

Ok. We all have our own pecularitie... I mean preferences :) But as
this is definitely your territory - I'll change the name :) No problem.

> 
> > @@ -292,19 +284,62 @@ struct gpio_descs *__must_check
> > devm_gpiod_get_array(struct device *dev,
> >         if (!dr)
> >                 return ERR_PTR(-ENOMEM);
> > 
> > -       descs = gpiod_get_array(dev, con_id, flags);
> > +       descs = gpiod_get_array(gpiodev, con_id, flags);
> >         if (IS_ERR(descs)) {
> >                 devres_free(dr);
> >                 return descs;
> >         }
> > 
> >         *dr = descs;
> > -       devres_add(dev, dr);
> > +       if (managed)
> > +               devres_add(managed, dr);
> > +       else
> > +               devres_add(gpiodev, dr);
> 
> So we only get managed resources if the "managed" device is
> passed in.

Hmm. Actually, no if I am not misreading this. We get managed devices
in any case, but the lifetime is either bound to exitence of the gpio
consumer device (which is standard way) - or existence of specific
'managed' device.

In case of MFD sub-device (like BD71828 regulator subdev) which has
GPIO consumer properties in MFD node - the GPIO consumer information is
in parent device's (BD71828 MFD device) data - but the lifetime should
be bound to sub-devices (BD71828 regulator device) lifetime. Thus we
need two different devices here.

> 
> > +/**
> > + * devm_gpiod_get_array - Resource-managed gpiod_get_array()
> 
> And this function is supposed to be resource managed for sure.

Yes. This is the standard case where device which has the consumer data
is also the one who 'manages' the GPIO.

> 
> > + * @dev:       GPIO consumer
> > + * @con_id:    function within the GPIO consumer
> > + * @flags:     optional GPIO initialization flags
> > + *
> > + * Managed gpiod_get_array(). GPIO descriptors returned from this
> > function are
> > + * automatically disposed on driver detach. See gpiod_get_array()
> > for detailed
> > + * information about behavior and return values.
> > + */
> > +struct gpio_descs *__must_check devm_gpiod_get_array(struct device
> > *dev,
> > +                                                    const char
> > *con_id,
> > +                                                    enum
> > gpiod_flags flags)
> > +{
> > +       return __devm_gpiod_get_array(dev, NULL, con_id, flags);
> 
> So what is this? NULL?

Here we don't have separate manager device - thus the manager is NULL
-and the consumer device ("dev" here) is what we use to manage GPIO.

> 
> Doesn't that mean you just removed all resource management for this
> call?

No :)

> 
> Or am I reading it wrong?

Either you are reading it wrong or I am writing it wrong xD. In any
case this means I need to drop few comments in code :) Thanks.

Br,
	Matti Vaittinen


  reply index

Thread overview: 51+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-11-18  6:53 [PATCH v5 00/16] Support ROHM BD71828 PMIC Matti Vaittinen
2019-11-18  6:53 ` [PATCH v5 01/16] dt-bindings: regulator: Document ROHM BD71282 regulator bindings Matti Vaittinen
2019-11-18 16:25   ` Mark Brown
2019-11-18 18:03     ` Vaittinen, Matti
2019-11-19 18:13       ` Mark Brown
2019-11-19 18:51         ` Vaittinen, Matti
2019-11-19 19:36           ` Mark Brown
2019-11-29  7:48             ` Vaittinen, Matti
2019-11-29 12:09               ` Mark Brown
2019-12-02  7:57                 ` Vaittinen, Matti
2019-12-02 13:11                   ` Mark Brown
2019-12-02 14:02                     ` Vaittinen, Matti
2019-12-04 12:47                       ` Mark Brown
2019-12-04 13:13                         ` Vaittinen, Matti
2019-12-04 14:14                           ` Mark Brown
2019-12-10 10:39                             ` Vaittinen, Matti
2019-12-10 11:14                 ` Vaittinen, Matti
2019-12-10 12:11                   ` Mark Brown
2019-12-10 12:41                     ` Vaittinen, Matti
2019-12-10 12:45                       ` Mark Brown
2019-12-10 13:07                         ` Vaittinen, Matti
2019-11-22 22:48   ` Rob Herring
2019-11-18  6:54 ` [PATCH v5 02/16] dt-bindings: leds: ROHM BD71282 PMIC LED driver Matti Vaittinen
2019-11-22 23:00   ` Rob Herring
2019-11-18  6:54 ` [PATCH v5 03/16] dt-bindings: mfd: Document ROHM BD71828 bindings Matti Vaittinen
2019-11-22 23:05   ` Rob Herring
2019-11-18  6:55 ` [PATCH v5 04/16] mfd: rohm PMICs - use platform_device_id to match MFD sub-devices Matti Vaittinen
2019-11-18  6:55 ` [PATCH v5 05/16] mfd: bd71828: Support ROHM BD71828 PMIC - core Matti Vaittinen
2019-11-18  6:56 ` [PATCH v5 06/16] mfd: input: bd71828: Add power-key support Matti Vaittinen
2019-11-18  6:56 ` [PATCH v5 07/16] clk: bd718x7: Support ROHM BD71828 clk block Matti Vaittinen
2019-11-18  6:57 ` [PATCH v5 08/16] regulator: bd718x7: Split driver to common and bd718x7 specific parts Matti Vaittinen
2019-11-18  6:57 ` [PATCH v5 09/16] regulator: bd71828: Basic support for ROHM bd71828 PMIC regulators Matti Vaittinen
2019-11-18 16:20   ` Mark Brown
2019-11-19  9:12     ` Vaittinen, Matti
2019-11-18  6:58 ` [PATCH v5 10/16] gpio: devres: Add devm_gpiod_get_parent_array Matti Vaittinen
2019-11-19 14:43   ` Linus Walleij
2019-11-19 17:54     ` Vaittinen, Matti [this message]
2019-11-21 14:13       ` Linus Walleij
2019-11-18  6:59 ` [PATCH v5 11/16] docs: driver-model: Add missing managed GPIO array get functions Matti Vaittinen
2019-11-18  6:59 ` [PATCH v5 12/16] regulator: bd71828: Add GPIO based run-level control for regulators Matti Vaittinen
2019-11-18  7:00 ` [PATCH v5 13/16] rtc: bd70528 add BD71828 support Matti Vaittinen
2019-12-10 13:24   ` Alexandre Belloni
2019-11-18  7:01 ` [PATCH v5 14/16] gpio: bd71828: Initial support for ROHM BD71828 PMIC GPIOs Matti Vaittinen
2019-11-18  9:22   ` Bartosz Golaszewski
2019-11-18  7:03 ` [PATCH v5 15/16] leds: Add common LED binding parsing support to LED class/core Matti Vaittinen
2019-11-18 21:55   ` Jacek Anaszewski
2019-11-19  7:21     ` Vaittinen, Matti
2019-11-19 19:30       ` Jacek Anaszewski
2019-11-20  7:31         ` Vaittinen, Matti
2019-11-19 14:23   ` Vaittinen, Matti
2019-11-18  7:03 ` [PATCH v5 16/16] led: bd71828: Support LED outputs on ROHM BD71828 PMIC 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=ece1ab1418e237d6f4968fc4cf59202c35f02ba7.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=corbet@lwn.net \
    --cc=devicetree@vger.kernel.org \
    --cc=dmurphy@ti.com \
    --cc=hkallweit1@gmail.com \
    --cc=hofrat@osadl.org \
    --cc=jacek.anaszewski@gmail.com \
    --cc=jeffrey.t.kirsher@intel.com \
    --cc=lee.jones@linaro.org \
    --cc=lgirdwood@gmail.com \
    --cc=linus.walleij@linaro.org \
    --cc=linux-clk@vger.kernel.org \
    --cc=linux-doc@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=m.szyprowski@samsung.com \
    --cc=mark.rutland@arm.com \
    --cc=mazziesaccount@gmail.com \
    --cc=mchehab+samsung@kernel.org \
    --cc=mturquette@baylibre.com \
    --cc=pavel@ucw.cz \
    --cc=phil.edworthy@renesas.com \
    --cc=robh+dt@kernel.org \
    --cc=sboyd@kernel.org \
    --cc=wsa+renesas@sang-engineering.com \
    /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-LEDs Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-leds/0 linux-leds/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-leds linux-leds/ https://lore.kernel.org/linux-leds \
		linux-leds@vger.kernel.org
	public-inbox-index linux-leds

Example config snippet for mirrors

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


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