| 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
Patch series introducing support for ROHM BD71828 PMIC ROHM BD71828 is a power management IC containing 7 bucks and 7 LDOs. All regulators can be controlled individually via I2C. Bucks 1,2,6 and 7 can also be assigned to a "regulator group" controlled by run-levels. Eg. Run level specific voltages and enable/disable statuses for each of these bucks can be set via register interface. The buck run-level group assignment (selection if buck is to be controlled individually or via run-levels) can be changed at run-time via I2C. This patch series brings only the basic support for controlling regulators individually via I2C. In addition to the bucks and LDOs there are: - The usual clk gate - 4 IO pins (mostly usable as GPO or tied to specific purpose) - power button support - RTC - two LEDs - battery charger - HALL sensor input This patch series adds support to regulators, clk, RTC, GPIOs and LEDs. Power-supply driver for charger is not included in this series. The series also adds LED DT-node lookup based on node name or given property name/value pair in LED core. It also adds generic default-state and default-trigger property handling to LED core. Follow-up patches simplifying few other LED drivers should follow. Changelog v7: - Rebased on top of v5.5-rc2 - Dropped already applied patches - basic regulator support (in regulator tree now) - regulator dt-bindings (in regulator tree now) - gpio devm function documentation (in GPIO tree now) - Dropped new devm-variant of gpio_array getting for MFD sub-devices who have consumer information in parent DT node as gpio consumer was dropped from the series - removed extra line-breaks from MFD driver and Makefile - fixed RTC patch subject line (added missing colon) - included a patch which adds compatible for ROHM BD71850 PMIC Changelog v6: Rebased on top of v5.5-rc1 LED core: - Do new fw-node look-up only if the new match data is given. That way behaviour for existing drivers is not changed - Handle generic LED properties by core only if explisitly requested in init-data. That way behaviour for existing drivers is not changed until they are verified to work. BD71828 LEDs: - Fix module loading by adding "dummy" of_device_id table. DT bindings: All: - Remove regulator run-level properties as run-level support was dropped for now. - Change SPDX to dual lisence LED: - added select: false - replace oneOf + const by enum Regulator: - remove forgotten comments - comment indenting MFD: - remove unnecessary descriptions Regulators: - Dropped patch 12 with run-level controls - Dropped unnecessary ramp_delay_supported() - ram_delay ops were already only filled for DVS bucks. GPIO: - rename internal function. RTC: - Added missing blank line Changelog v5: Only LED patch (patch 15) changed, rest as in v4. LED: - Fixed issues reported by Dan Carpenter and kbuild-bot static analysis. Changelog v4 (first non RFC): General: - Changed subdevice loading and chip version identification to use platform ID. - License identifiers changed to GPL-2.0-only MFD: - Styling fixes mostly DT-Bindings: - a few more checks as suggested by Rob Herring. - Order of DT patches changed. - me as maintainer - standard units to new properties (microvolts, ohms) - runlevel values in an array LED: - BD71828 driver added (back) - Added DT support - Added LED DT node lookup in led framework when init_data is given with DT node match information. - Added common property parsing for default-state and default-trigger. Regulators: - dropped sysfs interfaces - fixed module unload/reload by binding gpio consumer information to regulator device not to MFD. GPIO: - Added devm_gpiod_get_parent_array - added few missing devm functions to documentation Changelog v3: DT-Bindings: - yamlify - add LED binding doc CLK: - Move clk register definitions from MFD headers to clk driver GPIO: - Add generic direction define and use it. LED: - Drop LED driver from the series (for now). Changelog v2: Mainly RTC and GPIO fixes suggested by Alexandre and Bartosz General: -Patch ordering changed to provide dt binding documents right after the MFD core. DT-Bindings for regulators (Patch 3) -Fix typo in PMIC model number RTC (patch 11) -Reverted renaming in order to reduce patch size. -Reworded commit message BD71828 regulator (patch 7) -Add MODULE_ALIAS GPIO (patch 12) -Remove file-name from comment -prefix IN and OUT defines with chip type -improved documentation for the INPUT only pin. -removed empty left-over function -removed unnecessary #ifdef CONFIG_OF_GPIO -removed unnecessary error print -Add MODULE_ALIAS Patch 1: dt-bindings for LEDs on BD71828 PMIC Patch 2: dt-bindings for BD71828 PMIC Patch 3: Convert rohm PMICs with common sub-devices to use platform_ device_id to match MFD sub-devices Patch 4: Add compatible for BD71850 Patch 5: BD71828 MFD core. Patch 6: Power button support using GPIO keys. Patch 7: CLK gate support using existing clk-bd718x7 Patch 8: Split existing bd718x7 regulator driver to generic ROHM dt parsing portion (used by more than one ROHM drivers) and bd718x8 specific parts Patch 9: Support BD71828 RTC block using BD70528 RTC driver Patch 10: Allow control of GP(I)O pins on BD71828 via GPIO subsystem Patch 11: Add LED node lookup and common LED binding parsing support to LED class/core Patch 12: Support toggling the LEDs on BD71828. This patch series is based on v5.5-rc2 --- Matti Vaittinen (12): dt-bindings: leds: ROHM BD71282 PMIC LED driver dt-bindings: mfd: Document ROHM BD71828 bindings mfd: rohm PMICs - use platform_device_id to match MFD sub-devices mfd: bd718x7: Add compatible for BD71850 mfd: bd71828: Support ROHM BD71828 PMIC - core mfd: input: bd71828: Add power-key support clk: bd718x7: Support ROHM BD71828 clk block regulator: bd718x7: Split driver to common and bd718x7 specific parts rtc: bd70528: add BD71828 support gpio: bd71828: Initial support for ROHM BD71828 PMIC GPIOs leds: Add common LED binding parsing support to LED class/core led: bd71828: Support LED outputs on ROHM BD71828 PMIC .../bindings/leds/rohm,bd71828-leds.yaml | 52 +++ .../bindings/mfd/rohm,bd71828-pmic.yaml | 193 ++++++++ drivers/clk/Kconfig | 6 +- drivers/clk/clk-bd718x7.c | 50 ++- drivers/gpio/Kconfig | 12 + drivers/gpio/Makefile | 1 + drivers/gpio/gpio-bd71828.c | 159 +++++++ drivers/leds/Kconfig | 10 + drivers/leds/Makefile | 1 + drivers/leds/led-class.c | 99 +++- drivers/leds/led-core.c | 258 +++++++++-- drivers/leds/leds-bd71828.c | 118 +++++ drivers/mfd/Kconfig | 15 + drivers/mfd/Makefile | 1 + drivers/mfd/rohm-bd70528.c | 3 +- drivers/mfd/rohm-bd71828.c | 344 ++++++++++++++ drivers/mfd/rohm-bd718x7.c | 43 +- drivers/regulator/Kconfig | 4 + drivers/regulator/Makefile | 1 + drivers/regulator/bd718x7-regulator.c | 200 +++------ drivers/regulator/rohm-regulator.c | 95 ++++ drivers/rtc/Kconfig | 3 +- drivers/rtc/rtc-bd70528.c | 168 ++++++- include/linux/leds.h | 94 +++- include/linux/mfd/rohm-bd70528.h | 19 +- include/linux/mfd/rohm-bd71828.h | 423 ++++++++++++++++++ include/linux/mfd/rohm-bd718x7.h | 6 - include/linux/mfd/rohm-generic.h | 48 +- include/linux/mfd/rohm-shared.h | 27 ++ 29 files changed, 2190 insertions(+), 263 deletions(-) create mode 100644 Documentation/devicetree/bindings/leds/rohm,bd71828-leds.yaml create mode 100644 Documentation/devicetree/bindings/mfd/rohm,bd71828-pmic.yaml create mode 100644 drivers/gpio/gpio-bd71828.c create mode 100644 drivers/leds/leds-bd71828.c create mode 100644 drivers/mfd/rohm-bd71828.c create mode 100644 drivers/regulator/rohm-regulator.c create mode 100644 include/linux/mfd/rohm-bd71828.h create mode 100644 include/linux/mfd/rohm-shared.h base-commit: d1eef1c619749b2a57e514a3fa67d9a516ffa919