linux-kernel - Re: [PATCH v10 00/13] Support ROHM BD71828 PMIC

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Wed, 26 Feb 2020 14:25:31 +0000
From:   "Vaittinen, Matti" <Matti.Vaittinen@...rohmeurope.com>
To:     "pavel@....cz" <pavel@....cz>
CC:     "dmurphy@...com" <dmurphy@...com>,
        "mazziesaccount@...il.com" <mazziesaccount@...il.com>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linus.walleij@...aro.org" <linus.walleij@...aro.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "lee.jones@...aro.org" <lee.jones@...aro.org>,
        "jacek.anaszewski@...il.com" <jacek.anaszewski@...il.com>,
        "linux-leds@...r.kernel.org" <linux-leds@...r.kernel.org>,
        "robh+dt@...nel.org" <robh+dt@...nel.org>,
        "bgolaszewski@...libre.com" <bgolaszewski@...libre.com>
Subject: Re: [PATCH v10 00/13] Support ROHM BD71828 PMIC

Hello Pavel,

I dropped some of the recipients as I don't think this is interesting
for everybody :)

On Wed, 2020-02-26 at 14:42 +0100, Pavel Machek wrote:
> Hi!
> 
> > > > Changelog v10:
> > > >   - Split RTC patch to a BD70528 fix (which hopefully goes to
> > > > 5.4)
> > > > and to
> > > >     BD71828 support
> > > 
> > > Still missing LED Acks.
> > > 
> > 
> > Yep. I know. I haven't heard from Pavel recently and the patch 12
> > definitely requires his ack. Can you take the other patches in and
> > leave 12 and 13 out for now? I can continue work on LEDs with Pavel
> > but
> > I would really like to have the regulators working and BD70528 RTC
> > fixed in next release...
> 
> Going through my backlogs now. You taking patches up-to 11 so
> that we have two left sounds good :-).

Nice to hear you're back in the business :)
This series (except patches 12 and 13) was merged to linux 5.6-rc1.

I have not continued work with the patch 12 as I am not entirely happy
with that approach even myself.

I still think the led core should parse common led bindings.

What I am wondering is if LED core should provide interface which could
register an array of LEDs at one go - by taking an array of LED descs
from the driver and by scanning through the child nodes in given LED
controller's root node. Or if we should stick to the approach
introduced in the patch 12 - which requires own 'register call' per
LED.

Problem with latter approach is that it requires the LED driver to know
how many LEDs there is attached - or then to ignore the errors from
register function assuming error is caused by missing LED. So currently
(after I looked through more of the existing drivers) it seems to me
the individual drivers need to either hard-code number of LEDs (which
might not be a real problem though) or anyways check the DT for LED
nodes and call the registration for each LED based on the DT nodes.

Ideally I would like to see approach where the LED controller driver
would only fill LED descriptors for possible LED connections - and give
that to a registration API. The registration API would see the
descriptors and check which of the descs match to a LED given in DT &&
register those LED devices. That would help LED drivers with no special
DT properties to truly get rid of DT parsing.

Eg, as a quickly written pseudo code to explain the idea:
driver would fill some array like:

struct const led_descriptors my_leds[] = {
	{
		.id = MY_LED1,
		.ops = led_control_functions,
		.match_data = &dt_match_data,
		.is_optional = true,
		.of_match_cb = my_controller_specific_dt_parser,
	},
	{
		.id = MY_LED2,
		.ops = led_control_functions,
		.match_
data = &dt_match_data,
		.is_optional = true,
		.of_match_cb =
my_controller_specific_dt_parser,
	},
};

where:
 - led_control_functions would be function pointers to set the LED
states etc.
 - id would be a LED ID which the led control functions could use to
ide1ntify LED in controller
 - match_data would contain DT node match information the LED core
could use when searching the LED from DT
 - is_optional would tell whether the core should treat missing LED DT
node as error
 - of_match_cb would be a callback to call when core has parsed common
DT properties so that the driver can parse any driver specific dt
stuff.

and the driver could pass this and the LED controller DT node to
registration API which would "mass register" all found LEDs.

I think it would not be a huge thing to implement - but it definitely
is some work. And if this idea is strongly objected - then I may not
have the energy to push it through... So.. I would like to know what
people think of it? Is it really worth of trying? Or should I stick
with the approach presented in this series? Or should I just forget it
and add yet one more LED drivers implementing the same DT parsing loops
as others?

Best Regards
	Matti