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
