Linux-RTC Archive on lore.kernel.org
 help / color / Atom feed
From: "Vaittinen, Matti" <Matti.Vaittinen@fi.rohmeurope.com>
To: "robh+dt@kernel.org" <robh+dt@kernel.org>
Cc: "sboyd@kernel.org" <sboyd@kernel.org>,
	"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>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"alexandre.belloni@bootlin.com" <alexandre.belloni@bootlin.com>,
	"mazziesaccount@gmail.com" <mazziesaccount@gmail.com>,
	"mturquette@baylibre.com" <mturquette@baylibre.com>,
	"lgirdwood@gmail.com" <lgirdwood@gmail.com>,
	"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
	"linus.walleij@linaro.org" <linus.walleij@linaro.org>,
	"a.zummo@towertech.it" <a.zummo@towertech.it>,
	"mark.rutland@arm.com" <mark.rutland@arm.com>,
	"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>,
	"jacek.anaszewski@gmail.com" <jacek.anaszewski@gmail.com>,
	"broonie@kernel.org" <broonie@kernel.org>
Subject: Re: [RFC PATCH 11/13] led: bd71828: Support LED outputs on ROHM BD71828 PMIC
Date: Fri, 25 Oct 2019 14:37:30 +0000
Message-ID: <4fcea7213ae9b3c0de775d1854f8e160ea0b178a.camel@fi.rohmeurope.com> (raw)
In-Reply-To: <CAL_JsqK7fYYdobOrgxFaMOy+uONCV-i0aOiBQ9oOc4OOPLR8cw@mail.gmail.com>

Hello Peeps,

On Fri, 2019-10-25 at 08:24 -0500, Rob Herring wrote:
> On Fri, Oct 25, 2019 at 2:07 AM Vaittinen, Matti
> <Matti.Vaittinen@fi.rohmeurope.com> wrote:
> > Hi Again Jacek,
> > 
> > This has been a nice conversation. I guess I have learned something
> > from this all but I think this is no longer going forward too much
> > :)
> > I'll cook up second version - where I add LEDs to DT (even if I
> > don't
> > see the value for it now). I won't add own compatible for the LED
> > (for
> > now) - as it is part of MFD - and I'll add the LEDs also to binding
> > docs. I think that will get the attention from Lee/Rob better than
> > the
> > LED driver discussion. We can continue discussion there. I hope
> > this is
> > Ok to you. (And then just few compulsory notes about my view on
> > your
> > replies - after all, I can't let you to have the final say xD - you
> > can
> > ignore them or respond just as you like)
> > 
> > On Fri, 2019-10-25 at 00:04 +0200, Jacek Anaszewski wrote:
> > > Hi Matti,
> > > 
> > > On 10/24/19 10:15 AM, Vaittinen, Matti wrote:
> > > > Hello Jacek,
> > > > 
> > > > On Wed, 2019-10-23 at 23:59 +0200, Jacek Anaszewski wrote:
> > > > > On 10/23/19 10:37 AM, Vaittinen, Matti wrote:
> > > > > > On Tue, 2019-10-22 at 19:40 +0200, Jacek Anaszewski wrote:
> > > > > > > On 10/22/19 2:40 PM, Vaittinen, Matti wrote:
> > > > > > > > On Mon, 2019-10-21 at 21:09 +0200, Jacek Anaszewski
> > > > > > > > wrote:
> > > > > > > > > On 10/21/19 10:00 AM, Vaittinen, Matti wrote:
> > > > > > > > > > Hello Dan,
> > > > > > > > > > 
> > > > > > > > > > Thanks for taking the time to check my driver :) I
> > > > > > > > > > truly
> > > > > > > > > > appreciate
> > > > > > > > > > all
> > > > > > > > > > the help!
> > > > > > > > > > 
> > > > > > > > > > A "fundamental question" regarding these review
> > > > > > > > > > comments is
> > > > > > > > > > whether
> > > > > > > > > > I
> > > > > > > > > > should add DT entries for these LEDs or not. I
> > > > > > > > > > thought
> > > > > > > > > > I
> > > > > > > > > > shouldn't
> > > > > > > > > > but
> > > > > > > > > > I would like to get a comment from Rob regarding
> > > > > > > > > > it.
> > > > > > > > > 
> > > > > > > > > If the LED controller is a part of MFD device probed
> > > > > > > > > from
> > > > > > > > > DT
> > > > > > > > > then
> > > > > > > > > there is no doubt it should have corresponding DT
> > > > > > > > > sub-
> > > > > > > > > node.
> 
> Agreed.

Ouch. That annoying feeling when you notice you have been wrong...

> [...]
> 
> > > > Right. Or at first it might be enough (and simplest) to assume
> > > > that
> > > > if
> > > > LEDs are used via this driver, then colour for both LEDs is set
> > > > explicitly by user-space. I wouldn't try guessing if sibling
> > > > LED
> > > > state
> > > > changes to OFF when one LED is turned ON - or if LED states
> > > > change
> > > > to
> > > > ON if both are turned OFF. This would require exporting
> > > > interfaces
> > > > from
> > > > power-supply driver - and it would still be racy. The thing is
> > > > that
> > > > when both LEDs are on board they are both either under HW or SW
> > > > control. So it makes no sense to control only one LED in such
> > > > case.
> > > > Thus I think it is Ok if this LED driver is registering both
> > > > class
> > > > devices at one probe. No need to instantiate own platform
> > > > devices
> > > > for
> > > > both of the LEDs.
> > > 
> > > We always register all LEDs originating from the same device in
> > > one
> > > probe.
> > > 
> > 
> > Then I see little benefit from of_compatible or leds subnode for
> > MFD
> > devices with many LEDs. The driver or core must in any ways parse
> > the
> > DT in order to find the sub nodes with information for individual
> > LEDs.
> > I don't think that would be much different from just using the MFD
> > node
> > as controller node and walking through the MFD child nodes to
> > locate
> > LED sub nodes (at least for MFDs with simple LED controller).
> 
> The cases for not having child nodes are when you have child nodes
> with nothing more than a compatible and possibly provider properties
> (e.g. #gpio-cells for gpio providers). If you have other resource
> dependencies (e.g. clocks) or data to define (e.g. voltages for
> regulators), then child nodes absolutely make sense.

Thanks for telling the reasoning behind. Makes sense.

> Once we have
> child nodes, then generally it is easier for every function to be a
> child node and not mix the two. I'm sure I have told people
> incorrectly to not do child nodes because they define incomplete
> bindings.

Does this mean that if I add LED controlled node with LED nodes inside
- then I should actually add sub nodes for clk and GPIO too? I would
prefer still having the clk provider information in MFD node as adding
a sub-node for clk would probably require changes in the bd718x7_clk
driver. (Not big ones but avoidable if clk provider information can
still dwell in MFD node).

> I would group the led nodes under an led-controller node (with a
> compatible). The simple reason is each level only has one
> number/address space and you can't mix different ones. You're not
> numbering the leds here, but could you (with numbers that correspond
> to something in the h/w, not just 0..N)?

I don't know what that would be. The LED controller resides in MFD
device in I2C bus and has no meaningful numbers I can think of. The
actual LEDs (on my board) are dummy devices and I really don't know how
to invent meaningfull numbers for them either.

>  The other reason is modern
> LED bindings define a controller node for the controller and led
> nodes
> for the actual led on the board. While the MFD node could be the
> controller node, that gets into mixing.

My idea (if we use DT) was to use the MFD as controller - but that
really would be "mixing" then.

I'll see what I can prepare for v3.

Oh, and sorry Jacek for taking the time - I guess it was frustrating
for you - I really thought I knew how this should be done. Being wrong
is hard job at times, but so must be being right ;)


Br,
	Matti



  reply index

Thread overview: 43+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-10-17  9:40 [RFC PATCH 00/13] Support " 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
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 [this message]
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=4fcea7213ae9b3c0de775d1854f8e160ea0b178a.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