Message ID | cover.1576745635.git.matti.vaittinen@fi.rohmeurope.com |
---|---|
Headers | show |
Series | Support ROHM BD71828 PMIC | expand |
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@fi.rohmeurope.com> > --- > > 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. > @@ -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? > +/** > + * 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 > + * 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. > + /* > + * 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? > + /** > + * 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? > +/** > + * 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"? Best regards, Pavel
Hi! > ROHM BD71828 power management IC has two LED outputs for charge status > and button pressing indications. The LED outputs can also be forced > by SW so add driver allowing to use these LEDs for other indications > as well. > > Leds are controlled by SW using 'Force ON' bits. Please note the > constrains mentioned in data-sheet: > 1. If one LED is forced ON - then also the other LED is forced. > => You can't use SW control to force ON one LED and allow HW > to control the other. > 2. You can't force both LEDs OFF. If the FORCE bit for both LED's is > zero, then LEDs are controlled by HW and indicate button/charger > states as explained in data-sheet. That's really quite sad, is it? All the effort and all we got is ... one working LED. Because hardware does not allow you to control both LEDs... ...and we don't even have support selecting if the LED should be sw or hw controlled in the mainline, yet... Best regards, Pavel
Hello Pavel, 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@fi.rohmeurope.com> > > --- > > > > 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? I believe we do. Besides bunch of the lines are comments. I don't think I actually added much of new things here. And one thing we should not overlook is the drivers to come. I believe amount of LED devices we will be getting drivers for will increase. 320 lines is peanuts if we get 5 new drivers all implementing the same DT parsing loop. Anyways, I will look all of the comments thoroughly during next Friday. I am currently having a vacation and I might get strangled by my family if I spend it staring at the computer xD Thanks for taking the time to look at this! I do appreciate the effort! Br, Matti Vaittinen
Quoting Matti Vaittinen (2019-12-19 01:52:13) > BD71828GW is a single-chip power management IC for battery-powered portable > devices. Add support for controlling BD71828 clk using bd718x7 driver. > > Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> > Acked-for-MFD-by: Lee Jones <lee.jones@linaro.org> > --- Acked-by: Stephen Boyd <sboyd@kernel.org> I guess we can't win and break the build dependency on the "generic" header file :/
On Sat, 2019-12-21 at 20:48 +0100, Pavel Machek wrote: > Hi! > > > ROHM BD71828 power management IC has two LED outputs for charge > > status > > and button pressing indications. The LED outputs can also be forced > > by SW so add driver allowing to use these LEDs for other > > indications > > as well. > > > > Leds are controlled by SW using 'Force ON' bits. Please note the > > constrains mentioned in data-sheet: > > 1. If one LED is forced ON - then also the other LED is forced. > > => You can't use SW control to force ON one LED and > > allow HW > > to control the other. > > 2. You can't force both LEDs OFF. If the FORCE bit for both > > LED's is > > zero, then LEDs are controlled by HW and indicate > > button/charger > > states as explained in data-sheet. > > That's really quite sad, is it? > > All the effort and all we got is ... one working LED. Because > hardware > does not allow you to control both LEDs... Yes and no. I do fully agree that it would be much nicer if the LEDs could be set to be fully controlled by SW. OTOH, knowing the LEDs are usually OFF, using them to indicate something else is still doable. I think you know typical LED use-cases better than I do - but I guess that some blink pattern in order to indicate errors is well doable with these LEDs. > ...and we don't even have support selecting if the LED should be sw > or > hw controlled in the mainline, yet... Which sounds like we have such support somewhere - and hopefully in mainline one day ;) Anyways, Thanks for taking a look at this! :) Br, Matti Vaittinen
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@fi.rohmeurope.com> > > --- > > > > 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
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@fi.rohmeurope.com> > > --- > > //snip > > @@ -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? I added this although with the current patch it should not be required. The led_cdev->fwnode_owned is anyways re-initialized at the beginning of the 'led_classdev_register_ext'. It won't eat many cycles to zero it here though so perhaps it's safer to just do it. I am not sure I can finish and test the patch v7 today. So it may be next year when I am able to send it... Sorry for the delay! Br, Matti Vaittinen
Hello Again Pavel, On Sat, 2019-12-21 at 20:37 +0100, Pavel Machek wrote: > > +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. I took another look at this and changed this to: if (!strcmp(props->default_state, "on")) { if (!ld->max_brightness) ld->brightness = LED_FULL; else ld->brightness = ld->max_brightness; } I hope this is what you were suggesting. I'll send the v8 (hopefully) soon(ish). Best Regards Matti