Re: [PATCH v7 11/12] leds: Add common LED binding parsing support to LED class/core

[Vaittinen, Matti]

On Sat, 2019-12-21 at 20:37 +0100, Pavel Machek wrote:
> Hi!
> 
> > Qucik grep for 'for_each' or 'linux,default-trigger' or
> 
> quick.
> 
> > If init_data is goven but no starting point for node lookup - then
> 
> is given.
> 
> > (parent) device's own DT node is used. If no led-compatible is
> > given,
> > then of_match is searched for. If neither led-compatible not
> > of_match
> 
> nor of_match.
> 
> > is given then device's own node or passed starting point are used
> > as
> > such.
> > 
> > Signed-off-by: Matti Vaittinen <matti.vaittinen@xxxxxxxxxxxxxxxxx>
> > ---
> > 
> > No changes since v6
> > 
> >  drivers/leds/led-class.c |  99 +++++++++++++--
> >  drivers/leds/led-core.c  | 258 ++++++++++++++++++++++++++++++++---
> > ----
> >  include/linux/leds.h     |  94 ++++++++++++--
> >  3 files changed, 385 insertions(+), 66 deletions(-)
> 
> Quite a lot of code added here. Can I trust you that we we'll delete
> 320 lines by converting driver or two?
> 
> > +static void led_add_props(struct led_classdev *ld, struct
> > led_properties *props)
> > +{
> > +	if (props->default_trigger)
> > +		ld->default_trigger = props->default_trigger;
> > +	/*
> > +	 * According to binding docs the LED is by-default turned OFF
> > unless
> > +	 * default_state is used to indicate it should be ON or that
> > state
> > +	 * should be kept as is
> > +	 */
> > +	if (props->default_state) {
> > +		ld->brightness = LED_OFF;
> > +		if (!strcmp(props->default_state, "on"))
> > +			ld->brightness = LED_FULL;
> 
> Max brightness is not always == LED_FULL these days.

Hmm. That sounds like having LED_FULL is pretty pointless then, right?
I mean, if LED_FULL may not be LED_FULL, why we have LED_FULL then?
Anyways, I don't know what would be better value for the default state
"on"? I am willing to rework the patch here but I need some guidance.
Other option is to use the LED_FULL here and leave drivers using
something else to use own property parsing - or convert them to use
LED_FULL too. (Sorry, I don't know these drivers or why they don't use
LED_FULL so I can't say if this makes sense or not). Can you give me a
nudge as how to improve this?

> 
> > @@ -322,6 +398,10 @@ int led_classdev_register_ext(struct device
> > *parent,
> >  			led_cdev->name);
> >  
> >  	return 0;
> > +err_out:
> > +	if (led_cdev->fwnode_owned)
> > +		fwnode_handle_put(fw);
> > +	return ret;
> >  }
> 
> led_cdev->fwnode_owned = false here?

Hmm. Thanks. I didn't think that the cdev is not freed here and could
be re-used. So yes. I think we could set the led_cdev->fwnode_owned to
false here. I'll fix this. Good catch :)

> 
> 
> > +/**
> > + * led_find_fwnode - find fwnode for led
> > + * @parent	LED controller device
> > + * @init_data	led init data with match information
> > + *
> > + * Scans the firmware nodes and returns node matching the given
> > init_data.
> > + * NOTE: Function increases refcount for found node. Caller must
> > decrease
> > + * refcount using fwnode_handle_put when finished with node.
> > + */
> > +struct fwnode_handle *led_find_fwnode(struct device *parent,
> > +				      struct led_init_data *init_data)
> > +{
> > +	struct fwnode_handle *fw;
> > +
> > +	/*
> > +	 * This should never be called W/O init data. We could always
> > return
> 
> without

Right.

> > +	 * For now we do only do node look-up for drivers which
> > populate
> > +	 * the new match properties. We could and perhaps should do
> > +	 * fw = dev_fwnode(parent); if given fwnode is NULL. But in
> > order to
> > +	 * not break existing setups we keep the old behaviour and just
> > directly
> 
> not to break.

Indeed, thanks!

> > +	/*
> > +	 * Simple things are pretty. I think simplest is to use DT
> > node-name
> > +	 * for matching the node with LED - same way regulators use the
> > node
> > +	 * name to match with desc.
> > +	 *
> > +	 * This may not work with existing LED DT entries if the node
> > name has
> > +	 * been freely selectible. In order to this to work the binding
> > doc
> 
> selectable?

Ah. Again the same problem I had with regulator voltage ranges support.
English is hard. Google told me that selectible or selectable are not
really good words to use - hence I ended up using 'pickable' ranges. I
think this could also be "if the node name has been freely pickable.
I'll switch to that.

> > +	/**
> > +	 * Please note, logic changed - if invalid property is found we
> > bail
> > +	 * early out without parsing the rest of the properties.
> > Originally
> > +	 * this was the case only for 'label' property. I don't know
> > the
> > +	 * rationale behind original logic allowing invalid properties
> > to be
> > +	 * given. If there is a reason then we should reconsider this.
> > +	 * Intuitively it feels correct to just yell and quit if we hit
> > value we
> > +	 * don't understand - but intuition may be wrong at times :)
> > +	 */
> 
> Is this supposed to be linuxdoc?

definitely not. Thanks! I'll remove the extra *

> 
> > +/**
> > + * led_find_fwnode - find fwnode matching given LED init data
> > + * @parent: LED controller device this LED is driven by
> > + * @init_data: the LED class device initialization data
> > + *
> > + * Find the fw node matching given LED init data.
> > + * NOTE: Function increases refcount for found node. Caller must
> > decrease
> > + * refcount using fwnode_handle_put when finished with node.
> > + *
> > + * Returns: node handle or NULL if matching fw node was not found
> > + */
> > +struct fwnode_handle *led_find_fwnode(struct device *parent,
> > +				      struct led_init_data *init_data);
> > +
> 
> If function _gets_ the node and increments its usage count, is it
> normally called "get"?

Ok, thanks for the guidance. I didn't know that. I'll change this to
led_get_fwnode :)

Thanks a bunch!

Br,
	Matti Vaittinen