| Message ID | cover.1579249511.git.matti.vaittinen@fi.rohmeurope.com |
|---|---|
| Headers | show |
| Series | Support ROHM BD71828 PMIC | expand |
On Fri, 17 Jan 2020, Matti Vaittinen wrote: > ROHM BD71828 PMIC contains 4 pins which can be configured by OTP > to be used for general purposes. First 3 can be used as outputs > and 4.th pin can be used as input. Allow them to be controlled > via GPIO framework. > > The driver assumes all of the pins are configured as GPIOs and > trusts that the reserved pins in other OTP configurations are > excluded from control using "gpio-reserved-ranges" device tree > property (or left untouched by GPIO users). > > Typical use for 4.th pin (input) is to use it as HALL sensor > input so that this pin state is toggled when HALL sensor detects > LID position change (from close to open or open to close). PMIC > HW implements some extra logic which allows PMIC to power-up the > system when this pin is toggled. Please see the data sheet for > details of GPIO options which can be selected by OTP settings. > > Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> > Reviewed-by: Bartosz Golaszewski <bgolaszewski@baylibre.com> > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Linus, Is that an Ack? > --- > no changes since v9 > > drivers/gpio/Kconfig | 12 +++ > drivers/gpio/Makefile | 1 + > drivers/gpio/gpio-bd71828.c | 159 ++++++++++++++++++++++++++++++++++++ > 3 files changed, 172 insertions(+) > create mode 100644 drivers/gpio/gpio-bd71828.c
On Fri, 17 Jan 2020, Matti Vaittinen wrote: > When RTC is used in 24H mode (and it is by this driver) the maximum > hour value is 24 in BCD. This occupies bits [5:0] - which means > correct mask for HOUR register is 0x3f not 0x1f. Fix the mask > > Fixes: 32a4a4ebf768 ("rtc: bd70528: Initial support for ROHM bd70528 RTC") > > Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> > Acked-by: Alexandre Belloni <alexandre.belloni@bootlin.com> > --- > Changes: Splitted this fix into separate patch which can be applied to > 5.4 too > > include/linux/mfd/rohm-bd70528.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) For my own reference: Acked-for-MFD-by: Lee Jones <lee.jones@linaro.org>
On Fri, 17 Jan 2020, Matti Vaittinen wrote: > Few ROHM PMICs allow setting the voltage states for different system states > like RUN, IDLE, SUSPEND and LPSR. States are then changed via SoC specific > mechanisms. bd718x7 driver implemented device-tree parsing functions for > these state specific voltages. The parsing functions can be re-used by > other ROHM chip drivers like bd71828. Split the generic functions from > bd718x7-regulator.c to rohm-regulator.c and export them for other modules > to use. > > Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> > Acked-by: Mark Brown <broonie@kernel.org> > --- > no changes since v9 > > drivers/regulator/Kconfig | 4 + > drivers/regulator/Makefile | 1 + > drivers/regulator/bd718x7-regulator.c | 183 ++++++++------------------ > drivers/regulator/rohm-regulator.c | 95 +++++++++++++ > include/linux/mfd/rohm-generic.h | 66 ++++++++++ > 5 files changed, 221 insertions(+), 128 deletions(-) > create mode 100644 drivers/regulator/rohm-regulator.c [...] > diff --git a/include/linux/mfd/rohm-generic.h b/include/linux/mfd/rohm-generic.h > index ff3dd7578fd3..6cc5a0819959 100644 > --- a/include/linux/mfd/rohm-generic.h > +++ b/include/linux/mfd/rohm-generic.h > @@ -4,6 +4,9 @@ > #ifndef __LINUX_MFD_ROHM_H__ > #define __LINUX_MFD_ROHM_H__ > > +#include <linux/regmap.h> > +#include <linux/regulator/driver.h> > + > enum rohm_chip_type { > ROHM_CHIP_TYPE_BD71837 = 0, > ROHM_CHIP_TYPE_BD71847, > @@ -17,4 +20,67 @@ struct rohm_regmap_dev { > struct regmap *regmap; > }; > > +enum { > + ROHM_DVS_LEVEL_UNKNOWN, > + ROHM_DVS_LEVEL_RUN, > + ROHM_DVS_LEVEL_IDLE, > + ROHM_DVS_LEVEL_SUSPEND, > + ROHM_DVS_LEVEL_LPSR, > +#define ROHM_DVS_LEVEL_MAX ROHM_DVS_LEVEL_LPSR Haven't seen this before. Is it legit? What about: ROHM_DVS_LEVEL_MAX = ROHM_DVS_LEVEL_LPSR
On Fri, 17 Jan 2020, Matti Vaittinen wrote: > 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 v10: > - Split RTC patch to a BD70528 fix (which hopefully goes to 5.4) and to > BD71828 support Still missing LED Acks.
Hello Lee, On Fri, 2020-01-17 at 10:28 +0000, Lee Jones wrote: > On Fri, 17 Jan 2020, Matti Vaittinen wrote: > > > Few ROHM PMICs allow setting the voltage states for different > > system states > > like RUN, IDLE, SUSPEND and LPSR. States are then changed via SoC > > specific > > mechanisms. bd718x7 driver implemented device-tree parsing > > functions for > > these state specific voltages. The parsing functions can be re-used > > by > > other ROHM chip drivers like bd71828. Split the generic functions > > from > > bd718x7-regulator.c to rohm-regulator.c and export them for other > > modules > > to use. > > > > Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> > > Acked-by: Mark Brown <broonie@kernel.org> > > --- > > no changes since v9 > > > > drivers/regulator/Kconfig | 4 + > > drivers/regulator/Makefile | 1 + > > drivers/regulator/bd718x7-regulator.c | 183 ++++++++------------ > > ------ > > drivers/regulator/rohm-regulator.c | 95 +++++++++++++ > > include/linux/mfd/rohm-generic.h | 66 ++++++++++ > > 5 files changed, 221 insertions(+), 128 deletions(-) > > create mode 100644 drivers/regulator/rohm-regulator.c > > [...] > > > diff --git a/include/linux/mfd/rohm-generic.h > > b/include/linux/mfd/rohm-generic.h > > index ff3dd7578fd3..6cc5a0819959 100644 > > --- a/include/linux/mfd/rohm-generic.h > > +++ b/include/linux/mfd/rohm-generic.h > > @@ -4,6 +4,9 @@ > > #ifndef __LINUX_MFD_ROHM_H__ > > #define __LINUX_MFD_ROHM_H__ > > > > +#include <linux/regmap.h> > > +#include <linux/regulator/driver.h> > > + > > enum rohm_chip_type { > > ROHM_CHIP_TYPE_BD71837 = 0, > > ROHM_CHIP_TYPE_BD71847, > > @@ -17,4 +20,67 @@ struct rohm_regmap_dev { > > struct regmap *regmap; > > }; > > > > +enum { > > + ROHM_DVS_LEVEL_UNKNOWN, > > + ROHM_DVS_LEVEL_RUN, > > + ROHM_DVS_LEVEL_IDLE, > > + ROHM_DVS_LEVEL_SUSPEND, > > + ROHM_DVS_LEVEL_LPSR, > > +#define ROHM_DVS_LEVEL_MAX ROHM_DVS_LEVEL_LPSR > > Haven't seen this before. Is it legit? > I don't know why it wouldn't be :) I kind of grew used to that when I still did some networking stuff. It doesn't really matter in this case but for example the netlink headers do: enum { foo, #define foo foo bar, #define bar bar ... }; https://elixir.bootlin.com/linux/v5.5-rc6/source/include/uapi/linux/rtnetlink.h What is the good here is that this allows one to nicely exclude unsupported stuff using preprocessor: #include <header_with_or_without_foo_dependng_on_version.h> #ifdef foo use_foo(foo); #endif What about: > > ROHM_DVS_LEVEL_MAX = ROHM_DVS_LEVEL_LPSR Anyways, I don't see why define wouldn't be Ok here - but sure it can be changed if you insist ;) Just let me know if you can accept the define or not :) >
On Fri, 2020-01-17 at 10:30 +0000, Lee Jones wrote: > On Fri, 17 Jan 2020, Matti Vaittinen wrote: > > > 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 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... Br, Matti Vaittinen
On Fri, 17 Jan 2020, Vaittinen, Matti wrote: > Hello Lee, > > On Fri, 2020-01-17 at 10:28 +0000, Lee Jones wrote: > > On Fri, 17 Jan 2020, Matti Vaittinen wrote: > > > > > Few ROHM PMICs allow setting the voltage states for different > > > system states > > > like RUN, IDLE, SUSPEND and LPSR. States are then changed via SoC > > > specific > > > mechanisms. bd718x7 driver implemented device-tree parsing > > > functions for > > > these state specific voltages. The parsing functions can be re-used > > > by > > > other ROHM chip drivers like bd71828. Split the generic functions > > > from > > > bd718x7-regulator.c to rohm-regulator.c and export them for other > > > modules > > > to use. > > > > > > Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> > > > Acked-by: Mark Brown <broonie@kernel.org> > > > --- > > > no changes since v9 > > > > > > drivers/regulator/Kconfig | 4 + > > > drivers/regulator/Makefile | 1 + > > > drivers/regulator/bd718x7-regulator.c | 183 ++++++++------------ > > > ------ > > > drivers/regulator/rohm-regulator.c | 95 +++++++++++++ > > > include/linux/mfd/rohm-generic.h | 66 ++++++++++ > > > 5 files changed, 221 insertions(+), 128 deletions(-) > > > create mode 100644 drivers/regulator/rohm-regulator.c > > > > [...] > > > > > diff --git a/include/linux/mfd/rohm-generic.h > > > b/include/linux/mfd/rohm-generic.h > > > index ff3dd7578fd3..6cc5a0819959 100644 > > > --- a/include/linux/mfd/rohm-generic.h > > > +++ b/include/linux/mfd/rohm-generic.h > > > @@ -4,6 +4,9 @@ > > > #ifndef __LINUX_MFD_ROHM_H__ > > > #define __LINUX_MFD_ROHM_H__ > > > > > > +#include <linux/regmap.h> > > > +#include <linux/regulator/driver.h> > > > + > > > enum rohm_chip_type { > > > ROHM_CHIP_TYPE_BD71837 = 0, > > > ROHM_CHIP_TYPE_BD71847, > > > @@ -17,4 +20,67 @@ struct rohm_regmap_dev { > > > struct regmap *regmap; > > > }; > > > > > > +enum { > > > + ROHM_DVS_LEVEL_UNKNOWN, > > > + ROHM_DVS_LEVEL_RUN, > > > + ROHM_DVS_LEVEL_IDLE, > > > + ROHM_DVS_LEVEL_SUSPEND, > > > + ROHM_DVS_LEVEL_LPSR, > > > +#define ROHM_DVS_LEVEL_MAX ROHM_DVS_LEVEL_LPSR > > > > Haven't seen this before. Is it legit? > > > > I don't know why it wouldn't be :) I kind of grew used to that when I > still did some networking stuff. Networking it not a good example. It's full of odd little quirks to the standard styling. > It doesn't really matter in this case but for example the netlink > headers do: > > enum { > foo, > #define foo foo > bar, > #define bar bar > ... > }; > > https://elixir.bootlin.com/linux/v5.5-rc6/source/include/uapi/linux/rtnetlink.h > > What is the good here is that this allows one to nicely exclude > unsupported stuff using preprocessor: > > #include <header_with_or_without_foo_dependng_on_version.h> > > #ifdef foo > use_foo(foo); > #endif > > What about: > > > > ROHM_DVS_LEVEL_MAX = ROHM_DVS_LEVEL_LPSR > > Anyways, I don't see why define wouldn't be Ok here - but sure it can > be changed if you insist ;) Just let me know if you can accept the > define or not :) Let's go for not in this instance. :D
On Fri, 17 Jan 2020, Vaittinen, Matti wrote: > > On Fri, 2020-01-17 at 10:30 +0000, Lee Jones wrote: > > On Fri, 17 Jan 2020, Matti Vaittinen wrote: > > > > > 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 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... Sure. Give me a few days though.
Hello, On Fri, 2020-01-17 at 10:21 +0000, Lee Jones wrote: > On Fri, 17 Jan 2020, Matti Vaittinen wrote: > > > ROHM BD71828 PMIC contains 4 pins which can be configured by OTP > > to be used for general purposes. First 3 can be used as outputs > > and 4.th pin can be used as input. Allow them to be controlled > > via GPIO framework. > > > > The driver assumes all of the pins are configured as GPIOs and > > trusts that the reserved pins in other OTP configurations are > > excluded from control using "gpio-reserved-ranges" device tree > > property (or left untouched by GPIO users). > > > > Typical use for 4.th pin (input) is to use it as HALL sensor > > input so that this pin state is toggled when HALL sensor detects > > LID position change (from close to open or open to close). PMIC > > HW implements some extra logic which allows PMIC to power-up the > > system when this pin is toggled. Please see the data sheet for > > details of GPIO options which can be selected by OTP settings. > > > > Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> > > Reviewed-by: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> > > Linus, Is that an Ack? I have always thought that reviewed-by implies that reviewer is Ok with the patch (imples Ack). Maybe I have mistaken? Br, --Matti
Hello, On Fri, 2020-01-17 at 13:40 +0000, Lee Jones wrote: > On Fri, 17 Jan 2020, Vaittinen, Matti wrote: > > > > > > > > +enum { > > > > + ROHM_DVS_LEVEL_UNKNOWN, > > > > + ROHM_DVS_LEVEL_RUN, > > > > + ROHM_DVS_LEVEL_IDLE, > > > > + ROHM_DVS_LEVEL_SUSPEND, > > > > + ROHM_DVS_LEVEL_LPSR, > > > > +#define ROHM_DVS_LEVEL_MAX ROHM_DVS_LEVEL_LPSR > > > > > > Haven't seen this before. Is it legit? > > > > > > > I don't know why it wouldn't be :) I kind of grew used to that when > > I > > still did some networking stuff. > > Networking it not a good example. > > It's full of odd little quirks to the standard styling. That was quite a strong wording... Some people might disagree :) Anyways, as far as I know the preprocessor does not care about where the preprocessor directives are placed. It just goes through the file sequentially and macro definitions take effect at the place you write them. And actual compiler does not see the directive - just code which has been replaced. So from C point of view I see no problem here. From coding conventions or guidelines point of view - well, that's more of your territory ;) > > What about: > > > ROHM_DVS_LEVEL_MAX = ROHM_DVS_LEVEL_LPSR > > > > Anyways, I don't see why define wouldn't be Ok here - but sure it > > can > > be changed if you insist ;) Just let me know if you can accept the > > define or not :) > > Let's go for not in this instance. :D Ok. I sent v11 where this has been changed as you suggested :) Br, --Matti
On Mon, 20 Jan 2020, Vaittinen, Matti wrote: > Hello, > > On Fri, 2020-01-17 at 10:21 +0000, Lee Jones wrote: > > On Fri, 17 Jan 2020, Matti Vaittinen wrote: > > > > > ROHM BD71828 PMIC contains 4 pins which can be configured by OTP > > > to be used for general purposes. First 3 can be used as outputs > > > and 4.th pin can be used as input. Allow them to be controlled > > > via GPIO framework. > > > > > > The driver assumes all of the pins are configured as GPIOs and > > > trusts that the reserved pins in other OTP configurations are > > > excluded from control using "gpio-reserved-ranges" device tree > > > property (or left untouched by GPIO users). > > > > > > Typical use for 4.th pin (input) is to use it as HALL sensor > > > input so that this pin state is toggled when HALL sensor detects > > > LID position change (from close to open or open to close). PMIC > > > HW implements some extra logic which allows PMIC to power-up the > > > system when this pin is toggled. Please see the data sheet for > > > details of GPIO options which can be selected by OTP settings. > > > > > > Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> > > > Reviewed-by: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> > > > > Linus, Is that an Ack? > I have always thought that reviewed-by implies that reviewer is Ok with > the patch (imples Ack). Maybe I have mistaken? I would rather not assume.
On Fri, 2020-01-17 at 11:43 +0200, Matti Vaittinen wrote: > Quick grep for 'for_each' or 'linux,default-trigger' or > 'default-state' under drivers/leds can tell quite a lot. It seems > multiple LED controller drivers implement the very similar looping > through the child nodes in order to locate the LED nodes and read > and support the common LED dt bindings. Implementing this same > stuff for all LED controllers gets old pretty fast. > > This commit adds support for locating the LED node (based on known > node names - or given property name-value pair) and handling of > few common LED properties. > > linux,default-trigger, > default-state (with the exception of keep), > > (in addition to already handled > function-enumerator, > function, > color > and label). > > The handling of linux,default-trigger and default-state are omitted > if init_data->parse_fwnode is not set to true. Thus the existing > LED drivers are not impacted until they set this field to true. > > Regarding the node look-up: > > 1. If no init_data is given, then no node-lookup is done and cdev > name is used as such. (as was earlier) > > 2. If neither match_property nor of_match is given then passed > starting > point is used as such. (as was earlier - it might help in > few cases to use dev_fwnode() here but that would change existing > logic and possibly cause some existing setup to break). > > 3. If init_data is given but no starting point for node lookup - then > (parent) device's own DT node is used as starting point for look- > up. > > 4. The match_property has priority over of_match and is first used > for > look-up. If a node with matching property is found then the found > node > is used. > > 5. If no match_property is given or no mathicng property was found > then > of_macth is used. The node which name matches of_match is searched > for > and used if found. > > This ensures that existing drivers which do not populate of_match or > match_property in init_data wont be impacted bu node look-up. > > Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> > --- > no changes since v9 > > drivers/leds/led-class.c | 115 +++++++++++++++-- > drivers/leds/led-core.c | 258 ++++++++++++++++++++++++++++++++----- > -- > include/linux/leds.h | 94 ++++++++++++-- > 3 files changed, 401 insertions(+), 66 deletions(-) > > diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c > index 438774315e6c..170ab227eaa9 100644 > --- a/drivers/leds/led-class.c > +++ b/drivers/leds/led-class.c > @@ -235,6 +235,32 @@ static int led_classdev_next_name(const char > *init_name, char *name, > return i; > } > > +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")) { > + if (!ld->max_brightness) > + ld->brightness = LED_FULL; > + else > + ld->brightness = ld->max_brightness; This makes no sense as long as we unconditionally call the led_update_brightness at the end of led_classdev_register_ext. I misread the led_update_brightness - and thought it would update the brightness value from led classdev to hardware. But this is actually the opposite - it reads value from HW and sets it to classdev. So updating value in classdev won't help as the led_update_brightness will overwrite it. So for proper common DT parsing we need a flag to update the new default value from DT to HW - if the default is given. And spill out a warning if driver does not implement callback to set brightness. Anyways, I need to rework this patch to some extent (and I'll also try to see how existing drivers could be updated). So let's drop the patch 12 and 13 from this series - I try to rework them for next kernel release then. :) > + /* > + * We probably should not call the brightness_get prior calling > + * the of_parse_cb if one is provided. > + * Add a flag to advertice that state should be queried and > kept as-is. > + */ > + } else if (!strcmp(props->default_state, "keep")) > + props->brightness_keep = true; > + } > +} > + > /** > * led_classdev_register_ext - register a new object of led_classdev > class > * with init data. > @@ -251,22 +277,69 @@ int led_classdev_register_ext(struct device > *parent, > char final_name[LED_MAX_NAME_SIZE]; > const char *proposed_name = composed_name; > int ret; > - > + struct led_properties props = {0}; > + struct fwnode_handle *fw; > + > + /* > + * We don't try getting the name based on DT node if init-data > is not > + * given. We could see if we find LED properties from the > device's node > + * but that might change LED names for old users of > + * led_classdev_register who have been providing the LED name > in > + * cdev->name. So we seek fwnode for names only if init_data is > given > + */ > if (init_data) { > + led_cdev->init_data = init_data; > if (init_data->devname_mandatory && !init_data- > >devicename) { > dev_err(parent, "Mandatory device name is > missing"); > return -EINVAL; > } > - ret = led_compose_name(parent, init_data, > composed_name); > + > + fw = led_get_fwnode(parent, init_data); > + if (IS_ERR(fw)) > + return PTR_ERR(fw); > + > + if (fw) { > + /* > + * We did increase refcount for fwnode. Let's > set a flag > + * so we can decrease it during deregistration > + */ > + led_cdev->fwnode_owned = true; > + > + ret = led_parse_fwnode_props(parent, fw, > &props); > + if (ret) > + goto err_out; > + > + if (init_data->of_parse_cb) > + ret = init_data->of_parse_cb(led_cdev, > fw, > + &props); > + if (ret < 0) > + goto err_out; > + > + /* > + * Handle the common LED properties only for > drivers > + * that explicitly request it. This allows us > to test > + * and convert drivers to utilize common LED > parsing one > + * by one. > + */ > + if (init_data->parse_fwnode) > + led_add_props(led_cdev, &props); > + > + } else { > + led_cdev->fwnode_owned = false; > + } > + ret = led_compose_name(parent, init_data, &props, > + composed_name); > if (ret < 0) > - return ret; > + goto err_out; > } else { > proposed_name = led_cdev->name; > + led_cdev->fwnode_owned = false; > + fw = NULL; > } > > ret = led_classdev_next_name(proposed_name, final_name, > sizeof(final_name)); > if (ret < 0) > - return ret; > + goto err_out; > > mutex_init(&led_cdev->led_access); > mutex_lock(&led_cdev->led_access); > @@ -274,22 +347,28 @@ int led_classdev_register_ext(struct device > *parent, > led_cdev, led_cdev->groups, "%s", > final_name); > if (IS_ERR(led_cdev->dev)) { > mutex_unlock(&led_cdev->led_access); > - return PTR_ERR(led_cdev->dev); > + ret = PTR_ERR(led_cdev->dev); > + goto err_out; > } > - if (init_data && init_data->fwnode) > - led_cdev->dev->fwnode = init_data->fwnode; > + if (fw) > + led_cdev->dev->fwnode = fw; > > if (ret) > dev_warn(parent, "Led %s renamed to %s due to name > collision", > led_cdev->name, dev_name(led_cdev- > >dev)); > > + if (props.brightness_keep) > + if (led_cdev->brightness_get) > + led_cdev->brightness = > + led_cdev->brightness_get(led_cdev); > + > if (led_cdev->flags & LED_BRIGHT_HW_CHANGED) { > ret = led_add_brightness_hw_changed(led_cdev); > if (ret) { > device_unregister(led_cdev->dev); > led_cdev->dev = NULL; > mutex_unlock(&led_cdev->led_access); > - return ret; > + goto err_out; > } > } > > @@ -322,6 +401,17 @@ int led_classdev_register_ext(struct device > *parent, > led_cdev->name); > > return 0; > +err_out: > + if (led_cdev->fwnode_owned) { > + fwnode_handle_put(fw); > + /* > + * This might not be needed as 'fwnode_owned' is always > + * initialized in led_classdev_register_ext > + */ > + led_cdev->fwnode_owned = false; > + } > + > + return ret; > } > EXPORT_SYMBOL_GPL(led_classdev_register_ext); > > @@ -355,6 +445,15 @@ void led_classdev_unregister(struct led_classdev > *led_cdev) > if (led_cdev->flags & LED_BRIGHT_HW_CHANGED) > led_remove_brightness_hw_changed(led_cdev); > > + if (led_cdev->fwnode_owned) { > + fwnode_handle_put(led_cdev->dev->fwnode); > + /* > + * This might not be needed as 'fwnode_owned' is always > + * initialized in led_classdev_register_ext > + */ > + led_cdev->fwnode_owned = false; > + } > + > device_unregister(led_cdev->dev); > > down_write(&leds_list_lock); > diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c > index f1f718dbe0f8..3cd1784494f2 100644 > --- a/drivers/leds/led-core.c > +++ b/drivers/leds/led-core.c > @@ -365,70 +365,226 @@ void led_sysfs_enable(struct led_classdev > *led_cdev) > } > EXPORT_SYMBOL_GPL(led_sysfs_enable); > > -static void led_parse_fwnode_props(struct device *dev, > - struct fwnode_handle *fwnode, > - struct led_properties *props) > +static int fw_is_match(struct fwnode_handle *fw, > + struct led_fw_match_property *mp, void *val) > { > - int ret; > + void *cmp = mp->raw_val; > + int ret = -EINVAL; > + > + if (mp->raw_val) { > + ret = fwnode_property_read_u8_array(fw, mp->name, val, > + mp->size); > + } else if (mp->intval) { > + cmp = mp->intval; > + switch (mp->size) { > + case 1: > + ret = fwnode_property_read_u8_array(fw, mp- > >name, val, > + mp->size); > + break; > + case 2: > + ret = fwnode_property_read_u16_array(fw, mp- > >name, val, > + mp->size); > + break; > + case 4: > + ret = fwnode_property_read_u32_array(fw, mp- > >name, val, > + mp->size); > + break; > + case 8: > + ret = fwnode_property_read_u64_array(fw, mp- > >name, val, > + mp->size); > + break; > + default: > + return -EINVAL; > + } > + } > + if (!ret && cmp) > + if (!memcmp(val, cmp, mp->size)) > + return 1; > + > + return 0; > +} > +/** > + * led_get_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_get_fwnode(struct device *parent, > + struct led_init_data *init_data) > +{ > + struct fwnode_handle *fw; > + > + /* > + * This should never be called without init data. We could > always return > + * dev_fwnode() - but then we should pump-up the refcount > + */ > + if (!init_data) > + return NULL; > + > + /* > + * 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 not to > + * break the existing setups we keep the old behaviour and just > directly > + * use the given init_data->fwnode no matter if it is NULL or > not. > + */ > + > + if ((!init_data->match_property.name || > + !init_data->match_property.size) && !init_data->of_match) > + return fwnode_handle_get(init_data->fwnode); > + > + /* match information was given - do node look-up */ > + > + if (!init_data->fwnode) > + fw = dev_fwnode(parent); > + else > + fw = init_data->fwnode; > + > + if (!fw) > + return NULL; > + > + /* > + * 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 pickable. In order to this to work the binding > doc > + * for LED driver should define usable node names. > + * > + * If this is not working we can define specific match property > which > + * value we scan and use for matching for LEDs connected to the > + * controller. > + */ > + if (init_data->match_property.name && init_data- > >match_property.size) { > + u8 *val; > + int ret; > + struct fwnode_handle *child; > + struct led_fw_match_property *mp; > + > + mp = &init_data->match_property; > + > + val = kzalloc(mp->size, GFP_KERNEL); > + if (!val) > + return ERR_PTR(-ENOMEM); > + > + fwnode_for_each_child_node(fw, child) { > + ret = fw_is_match(child, mp, val); > + if (ret > 0) { > + kfree(val); > + return child; > + } > + if (ret < 0) { > + dev_err(parent, > + "invalid fw match. Use > raw_val?\n"); > + fwnode_handle_put(child); > + break; > + } > + } > + kfree(val); > + } > + if (init_data->of_match) > + fw = fwnode_get_named_child_node(fw, init_data- > >of_match); > + > + return fw; > +} > +EXPORT_SYMBOL(led_get_fwnode); > + > +int led_parse_fwnode_props(struct device *dev, struct fwnode_handle > *fwnode, > + struct led_properties *props) > +{ > + int ret = 0; > > if (!fwnode) > - return; > + return 0; > > if (fwnode_property_present(fwnode, "label")) { > ret = fwnode_property_read_string(fwnode, "label", > &props->label); > if (ret) > dev_err(dev, "Error parsing 'label' property > (%d)\n", ret); > - return; > + return ret; > } > > + /* > + * 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 :) > + */ > if (fwnode_property_present(fwnode, "color")) { > ret = fwnode_property_read_u32(fwnode, "color", &props- > >color); > - if (ret) > + if (ret) { > dev_err(dev, "Error parsing 'color' property > (%d)\n", ret); > - else if (props->color >= LED_COLOR_ID_MAX) > + return ret; > + } else if (props->color >= LED_COLOR_ID_MAX) { > dev_err(dev, "LED color identifier out of > range\n"); > - else > - props->color_present = true; > + return ret; > + } > + props->color_present = true; > } > > + if (fwnode_property_present(fwnode, "function")) { > + ret = fwnode_property_read_string(fwnode, "function", > + &props->function); > + if (ret) { > + dev_err(dev, > + "Error parsing 'function' property > (%d)\n", > + ret); > + return ret; > + } > + } > > - if (!fwnode_property_present(fwnode, "function")) > - return; > - > - ret = fwnode_property_read_string(fwnode, "function", &props- > >function); > - if (ret) { > - dev_err(dev, > - "Error parsing 'function' property (%d)\n", > - ret); > + if (fwnode_property_present(fwnode, "function-enumerator")) { > + ret = fwnode_property_read_u32(fwnode, "function- > enumerator", > + &props->func_enum); > + if (ret) { > + dev_err(dev, > + "Error parsing 'function-enumerator' > property (%d)\n", > + ret); > + return ret; > + } > + props->func_enum_present = true; > } > > - if (!fwnode_property_present(fwnode, "function-enumerator")) > - return; > + if (fwnode_property_present(fwnode, "default-state")) { > + ret = fwnode_property_read_string(fwnode, "default- > state", > + &props- > >default_state); > + if (ret) { > + dev_err(dev, > + "Error parsing 'default-state' property > (%d)\n", > + ret); > + return ret; > + } > + } > > - ret = fwnode_property_read_u32(fwnode, "function-enumerator", > - &props->func_enum); > - if (ret) { > - dev_err(dev, > - "Error parsing 'function-enumerator' property > (%d)\n", > - ret); > - } else { > - props->func_enum_present = true; > + if (fwnode_property_present(fwnode, "linux,default-trigger")) { > + ret = fwnode_property_read_string(fwnode, > + "linux,default- > trigger", > + &props- > >default_trigger); > + if (ret) > + dev_err(dev, > + "Error parsing 'linux,default-trigger' > property (%d)\n", > + ret); > } > + return ret; > } > +EXPORT_SYMBOL_GPL(led_parse_fwnode_props); > > int led_compose_name(struct device *dev, struct led_init_data > *init_data, > - char *led_classdev_name) > + struct led_properties *props, char > *led_classdev_name) > { > - struct led_properties props = {}; > - struct fwnode_handle *fwnode = init_data->fwnode; > const char *devicename = init_data->devicename; > > if (!led_classdev_name) > return -EINVAL; > > - led_parse_fwnode_props(dev, fwnode, &props); > - > - if (props.label) { > + if (props->label) { > /* > * If init_data.devicename is NULL, then it indicates > that > * DT label should be used as-is for LED class device > name. > @@ -436,23 +592,23 @@ int led_compose_name(struct device *dev, struct > led_init_data *init_data, > * the final LED class device name. > */ > if (!devicename) { > - strscpy(led_classdev_name, props.label, > + strscpy(led_classdev_name, props->label, > LED_MAX_NAME_SIZE); > } else { > snprintf(led_classdev_name, LED_MAX_NAME_SIZE, > "%s:%s", > - devicename, props.label); > + devicename, props->label); > } > - } else if (props.function || props.color_present) { > + } else if (props->function || props->color_present) { > char tmp_buf[LED_MAX_NAME_SIZE]; > > - if (props.func_enum_present) { > + if (props->func_enum_present) { > snprintf(tmp_buf, LED_MAX_NAME_SIZE, "%s:%s- > %d", > - props.color_present ? > led_colors[props.color] : "", > - props.function ?: "", > props.func_enum); > + props->color_present ? > led_colors[props->color] : "", > + props->function ?: "", props- > >func_enum); > } else { > snprintf(tmp_buf, LED_MAX_NAME_SIZE, "%s:%s", > - props.color_present ? > led_colors[props.color] : "", > - props.function ?: ""); > + props->color_present ? > led_colors[props->color] : "", > + props->function ?: ""); > } > if (init_data->devname_mandatory) { > snprintf(led_classdev_name, LED_MAX_NAME_SIZE, > "%s:%s", > @@ -468,11 +624,19 @@ int led_compose_name(struct device *dev, struct > led_init_data *init_data, > } > snprintf(led_classdev_name, LED_MAX_NAME_SIZE, "%s:%s", > devicename, init_data->default_label); > - } else if (is_of_node(fwnode)) { > - strscpy(led_classdev_name, to_of_node(fwnode)->name, > - LED_MAX_NAME_SIZE); > - } else > - return -EINVAL; > + } else { > + struct fwnode_handle *fwnode = led_get_fwnode(dev, > init_data); > + int ret = -EINVAL; > + > + if (is_of_node(fwnode)) { > + ret = 0; > + strscpy(led_classdev_name, to_of_node(fwnode)- > >name, > + LED_MAX_NAME_SIZE); > + } > + fwnode_handle_put(fwnode); > + > + return ret; > + } > > return 0; > } > diff --git a/include/linux/leds.h b/include/linux/leds.h > index 242258f7d837..af8ca4291d7d 100644 > --- a/include/linux/leds.h > +++ b/include/linux/leds.h > @@ -13,6 +13,7 @@ > #include <linux/kernfs.h> > #include <linux/list.h> > #include <linux/mutex.h> > +#include <linux/property.h> > #include <linux/rwsem.h> > #include <linux/spinlock.h> > #include <linux/timer.h> > @@ -20,6 +21,7 @@ > > struct device; > struct led_pattern; > +struct led_classdev; > /* > * LED Core > */ > @@ -31,8 +33,47 @@ enum led_brightness { > LED_FULL = 255, > }; > > +struct led_properties { > + u32 color; > + bool color_present; > + const char *function; > + u32 func_enum; > + bool func_enum_present; > + const char *label; > + const char *default_trigger; > + const char *default_state; > + bool brightness_keep; > +}; > + > +struct led_fw_match_property { > + const char *name; /* Name of the property to match */ > + void *raw_val; /* Raw property value as present in > fwnode */ > + void *intval; /* Property value if 8,16,32 or 64bit > integer */ > + size_t size; /* Size of value in bytes */ > +}; > + > struct led_init_data { > - /* device fwnode handle */ > + /* > + * If DT binding dictates the node name the driver can fill > of_match > + * corresponding to node name describing this LED. If fwnode is > given > + * the match is searched from it's child nodes. If not, the > match is > + * searched from device's own child nodes. > + */ > + const char *of_match; > + /* > + * If fwnode contains property with known value the driver can > specify > + * correct propertty-value pair here to do the matching. This > has higher > + * priority than of_match. If fwnode is given the match is > searched > + * from it's child nodes. If not match is searched from > device's > + * own child nodes. > + */ > + struct led_fw_match_property match_property; > + /* > + * device fwnode handle. If of_match or led_compatible are not > given > + * this is used for LED as given. If of_match or led_compatible > are > + * given then this is used as a parent node whose child nodes > are > + * scanned for given match. > + */ > struct fwnode_handle *fwnode; > /* > * default <color:function> tuple, for backward compatibility > @@ -53,9 +94,19 @@ struct led_init_data { > * set it to true > */ > bool devname_mandatory; > + /* > + * Callback which a LED driver can register if it has own non- > standard > + * DT properties. Core calls this with the located DT node > during > + * class_device registration > + */ > + int (*of_parse_cb)(struct led_classdev *ld, struct > fwnode_handle *fw, > + struct led_properties *props); > + /* LED core should parse and handle the common firmware > properties */ > + bool parse_fwnode; > }; > > struct led_classdev { > + struct led_init_data *init_data; > const char *name; > enum led_brightness brightness; > enum led_brightness max_brightness; > @@ -148,6 +199,7 @@ struct led_classdev { > > /* Ensures consistent access to the LED Flash Class device */ > struct mutex led_access; > + bool fwnode_owned; > }; > > /** > @@ -299,6 +351,7 @@ void led_sysfs_enable(struct led_classdev > *led_cdev); > * led_compose_name - compose LED class device name > * @dev: LED controller device object > * @init_data: the LED class device initialization data > + * @props: LED properties as parsed from fwnode. > * @led_classdev_name: composed LED class device name > * > * Create LED class device name basing on the provided init_data > argument. > @@ -308,7 +361,8 @@ void led_sysfs_enable(struct led_classdev > *led_cdev); > * Returns: 0 on success or negative error value on failure > */ > int led_compose_name(struct device *dev, struct led_init_data > *init_data, > - char *led_classdev_name); > + struct led_properties *props, > + char *led_classdev_name); > > /** > * led_sysfs_is_disabled - check if LED sysfs interface is disabled > @@ -321,6 +375,33 @@ static inline bool led_sysfs_is_disabled(struct > led_classdev *led_cdev) > return led_cdev->flags & LED_SYSFS_DISABLE; > } > > +/** > + * led_get_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_get_fwnode(struct device *parent, > + struct led_init_data *init_data); > + > +/** > + * led_parse_fwnode_props - parse LED common properties from fwnode > + * @dev: pointer to LED device. > + * @fwnode: LED node containing the properties > + * @props: structure where found property data is stored. > + * > + * Parse the common LED properties from fwnode. > + * > + * Returns: 0 on success or negative error value on failure > + */ > +int led_parse_fwnode_props(struct device *dev, struct fwnode_handle > *fwnode, > + struct led_properties *props); > + > /* > * LED Triggers > */ > @@ -478,15 +559,6 @@ struct led_platform_data { > struct led_info *leds; > }; > > -struct led_properties { > - u32 color; > - bool color_present; > - const char *function; > - u32 func_enum; > - bool func_enum_present; > - const char *label; > -}; > - > struct gpio_desc; > typedef int (*gpio_blink_set_t)(struct gpio_desc *desc, int state, > unsigned long *delay_on, > -- > 2.21.0 > >
On Fri, Jan 17, 2020 at 11:21 AM Lee Jones <lee.jones@linaro.org> wrote: > On Fri, 17 Jan 2020, Matti Vaittinen wrote: > > > ROHM BD71828 PMIC contains 4 pins which can be configured by OTP > > to be used for general purposes. First 3 can be used as outputs > > and 4.th pin can be used as input. Allow them to be controlled > > via GPIO framework. > > > > The driver assumes all of the pins are configured as GPIOs and > > trusts that the reserved pins in other OTP configurations are > > excluded from control using "gpio-reserved-ranges" device tree > > property (or left untouched by GPIO users). > > > > Typical use for 4.th pin (input) is to use it as HALL sensor > > input so that this pin state is toggled when HALL sensor detects > > LID position change (from close to open or open to close). PMIC > > HW implements some extra logic which allows PMIC to power-up the > > system when this pin is toggled. Please see the data sheet for > > details of GPIO options which can be selected by OTP settings. > > > > Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> > > Reviewed-by: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> > > Linus, Is that an Ack? Yes! Feel free to merge this. Yours, Linus Walleij
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 :-). Best regards, Pavel
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
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 v10: - Split RTC patch to a BD70528 fix (which hopefully goes to 5.4) and to BD71828 support Changelog v9: (Changes suggested by Lee) - Added kerneldoc to struct rohm_dvs_config - cleaned few comments - updated copyright date - renamed variable 'mfd' to 'parent'. Changelog v8: LEDs: - Fixed bunch of typos. - Corrected the commit message which errorneously contained the 'leds-compatible' which I dropped in favour of match_property. - use max_brightness instead of LED_FULL if it is given when handling the default_state, "on". - clean fwnode_owned at the end of unsuccessfull registration or at the end of deregistration. - fix the accidental linuxdoc comment. - rename led_find_fwnode to led_get_fwnode as it increases refcount. 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: Fix BD70528 RTC HOUR mask Patch 10: Support BD71828 RTC block using BD70528 RTC driver Patch 11: Allow control of GP(I)O pins on BD71828 via GPIO subsystem Patch 12: Add LED node lookup and common LED binding parsing support to LED class/core Patch 13: Support toggling the LEDs on BD71828. --- Matti Vaittinen (13): 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 mfd: bd70528: Fix hour register mask 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 | 115 ++++- 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 | 220 +++++++-- 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 | 70 ++- include/linux/mfd/rohm-shared.h | 21 + 29 files changed, 2248 insertions(+), 289 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: b3a987b0264d ("Linux 5.5-rc6")