| Message ID | cover.1574059625.git.matti.vaittinen@fi.rohmeurope.com |
|---|---|
| Headers | show |
| Series | Support ROHM BD71828 PMIC | expand |
pon., 18 lis 2019 o 08:01 Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> napisał(a): > > 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> > --- > > No changes from v4 > > 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 > > diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig > index 38e096e6925f..b4089096f7f2 100644 > --- a/drivers/gpio/Kconfig > +++ b/drivers/gpio/Kconfig > @@ -994,6 +994,18 @@ config GPIO_BD70528 > This driver can also be built as a module. If so, the module > will be called gpio-bd70528. > > +config GPIO_BD71828 > + tristate "ROHM BD71828 GPIO support" > + depends on MFD_ROHM_BD71828 > + help > + Support for GPIOs on ROHM BD71828 PMIC. There are three GPIOs > + available on the ROHM PMIC in total. The GPIOs are limited to > + outputs only and pins must be configured to GPIO outputs by > + OTP. Enable this only if you want to use these pins as outputs. > + > + This driver can also be built as a module. If so, the module > + will be called gpio-bd71828. > + > config GPIO_BD9571MWV > tristate "ROHM BD9571 GPIO support" > depends on MFD_BD9571MWV > diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile > index d2fd19c15bae..034b38996579 100644 > --- a/drivers/gpio/Makefile > +++ b/drivers/gpio/Makefile > @@ -35,6 +35,7 @@ obj-$(CONFIG_GPIO_ASPEED) += gpio-aspeed.o > obj-$(CONFIG_GPIO_ATH79) += gpio-ath79.o > obj-$(CONFIG_GPIO_BCM_KONA) += gpio-bcm-kona.o > obj-$(CONFIG_GPIO_BD70528) += gpio-bd70528.o > +obj-$(CONFIG_GPIO_BD71828) += gpio-bd71828.o > obj-$(CONFIG_GPIO_BD9571MWV) += gpio-bd9571mwv.o > obj-$(CONFIG_GPIO_BRCMSTB) += gpio-brcmstb.o > obj-$(CONFIG_GPIO_BT8XX) += gpio-bt8xx.o > diff --git a/drivers/gpio/gpio-bd71828.c b/drivers/gpio/gpio-bd71828.c > new file mode 100644 > index 000000000000..04aade9e0a4d > --- /dev/null > +++ b/drivers/gpio/gpio-bd71828.c > @@ -0,0 +1,159 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +// Copyright (C) 2018 ROHM Semiconductors > + > +#include <linux/gpio/driver.h> > +#include <linux/mfd/rohm-bd71828.h> > +#include <linux/module.h> > +#include <linux/platform_device.h> > +#include <linux/regmap.h> > + > +#define GPIO_OUT_REG(off) (BD71828_REG_GPIO_CTRL1 + (off)) > +#define HALL_GPIO_OFFSET 3 > + > +/* > + * These defines can be removed when > + * "gpio: Add definition for GPIO direction" > + * (9208b1e77d6e8e9776f34f46ef4079ecac9c3c25 in GPIO tree) gets merged, > + */ > +#ifndef GPIO_LINE_DIRECTION_IN > + #define GPIO_LINE_DIRECTION_IN 1 > + #define GPIO_LINE_DIRECTION_OUT 0 > +#endif > + > +struct bd71828_gpio { > + struct rohm_regmap_dev chip; > + struct gpio_chip gpio; > +}; > + > +static void bd71828_gpio_set(struct gpio_chip *chip, unsigned int offset, > + int value) > +{ > + int ret; > + struct bd71828_gpio *bdgpio = gpiochip_get_data(chip); > + u8 val = (value) ? BD71828_GPIO_OUT_HI : BD71828_GPIO_OUT_LO; > + > + /* > + * The HALL input pin can only be used as input. If this is the pin > + * we are dealing with - then we are done > + */ > + if (offset == HALL_GPIO_OFFSET) > + return; > + > + ret = regmap_update_bits(bdgpio->chip.regmap, GPIO_OUT_REG(offset), > + BD71828_GPIO_OUT_MASK, val); > + if (ret) > + dev_err(bdgpio->chip.dev, "Could not set gpio to %d\n", value); > +} > + > +static int bd71828_gpio_get(struct gpio_chip *chip, unsigned int offset) > +{ > + int ret; > + unsigned int val; > + struct bd71828_gpio *bdgpio = gpiochip_get_data(chip); > + > + if (offset == HALL_GPIO_OFFSET) > + ret = regmap_read(bdgpio->chip.regmap, BD71828_REG_IO_STAT, > + &val); > + else > + ret = regmap_read(bdgpio->chip.regmap, GPIO_OUT_REG(offset), > + &val); > + if (!ret) > + ret = (val & BD71828_GPIO_OUT_MASK); > + > + return ret; > +} > + > +static int bd71828_gpio_set_config(struct gpio_chip *chip, unsigned int offset, > + unsigned long config) > +{ > + struct bd71828_gpio *bdgpio = gpiochip_get_data(chip); > + > + if (offset == HALL_GPIO_OFFSET) > + return -ENOTSUPP; > + > + switch (pinconf_to_config_param(config)) { > + case PIN_CONFIG_DRIVE_OPEN_DRAIN: > + return regmap_update_bits(bdgpio->chip.regmap, > + GPIO_OUT_REG(offset), > + BD71828_GPIO_DRIVE_MASK, > + BD71828_GPIO_OPEN_DRAIN); > + case PIN_CONFIG_DRIVE_PUSH_PULL: > + return regmap_update_bits(bdgpio->chip.regmap, > + GPIO_OUT_REG(offset), > + BD71828_GPIO_DRIVE_MASK, > + BD71828_GPIO_PUSH_PULL); > + default: > + break; > + } > + return -ENOTSUPP; > +} > + > +static int bd71828_get_direction(struct gpio_chip *chip, unsigned int offset) > +{ > + /* > + * Pin usage is selected by OTP data. We can't read it runtime. Hence > + * we trust that if the pin is not excluded by "gpio-reserved-ranges" > + * the OTP configuration is set to OUT. (Other pins but HALL input pin > + * on BD71828 can't really be used for general purpose input - input > + * states are used for specific cases like regulator control or > + * PMIC_ON_REQ. > + */ > + if (offset == HALL_GPIO_OFFSET) > + return GPIO_LINE_DIRECTION_IN; > + > + return GPIO_LINE_DIRECTION_OUT; > +} > + > +static int bd71828_probe(struct platform_device *pdev) > +{ > + struct bd71828_gpio *bdgpio; > + struct rohm_regmap_dev *bd71828; > + > + bd71828 = dev_get_drvdata(pdev->dev.parent); > + if (!bd71828) { > + dev_err(&pdev->dev, "No MFD driver data\n"); > + return -EINVAL; > + } > + > + bdgpio = devm_kzalloc(&pdev->dev, sizeof(*bdgpio), > + GFP_KERNEL); > + if (!bdgpio) > + return -ENOMEM; > + > + bdgpio->chip.dev = &pdev->dev; > + bdgpio->gpio.parent = pdev->dev.parent; > + bdgpio->gpio.label = "bd71828-gpio"; > + bdgpio->gpio.owner = THIS_MODULE; > + bdgpio->gpio.get_direction = bd71828_get_direction; > + bdgpio->gpio.set_config = bd71828_gpio_set_config; > + bdgpio->gpio.can_sleep = true; > + bdgpio->gpio.get = bd71828_gpio_get; > + bdgpio->gpio.set = bd71828_gpio_set; > + bdgpio->gpio.base = -1; > + > + /* > + * See if we need some implementation to mark some PINs as > + * not controllable based on DT info or if core can handle > + * "gpio-reserved-ranges" and exclude them from control > + */ > + bdgpio->gpio.ngpio = 4; > + bdgpio->gpio.of_node = pdev->dev.parent->of_node; > + bdgpio->chip.regmap = bd71828->regmap; > + > + return devm_gpiochip_add_data(&pdev->dev, &bdgpio->gpio, > + bdgpio); > +} > + > +static struct platform_driver bd71828_gpio = { > + .driver = { > + .name = "bd71828-gpio" > + }, > + .probe = bd71828_probe, > +}; > + > +module_platform_driver(bd71828_gpio); > + > +MODULE_AUTHOR("Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>"); > +MODULE_DESCRIPTION("BD71828 voltage regulator driver"); > +MODULE_LICENSE("GPL"); > +MODULE_ALIAS("platform:bd71828-gpio"); > -- > 2.21.0 > > > -- > Matti Vaittinen, Linux device drivers > ROHM Semiconductors, Finland SWDC > Kiviharjunlenkki 1E > 90220 OULU > FINLAND > > ~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~ > Simon says - in Latin please. > ~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~ > Thanks to Simon Glass for the translation =] Reviewed-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
On Mon, Nov 18, 2019 at 08:57:57AM +0200, Matti Vaittinen wrote: > +static int ramp_delay_supported(struct regulator_dev *rdev) > +{ > + switch (rdev->desc->id) { > + case BD71828_BUCK1: > + case BD71828_BUCK2: > + case BD71828_BUCK6: > + case BD71828_BUCK7: > + return 1; > + default: > + break; > + } > + return 0; > +} > + > +static int bd71828_set_ramp_delay(struct regulator_dev *rdev, int ramp_delay) > +{ > + unsigned int val; > + > + if (!ramp_delay_supported(rdev)) { > + dev_err(&rdev->dev, "%s: can't set ramp-delay\n", > + rdev->desc->name); > + return -EINVAL; Rather than doing this it's better to just not provide the operation for devices that don't support it, that makes the handling in the core easier.
Hi Matti, Thank you for the patch. If your driver does not depend on it then please send is separately. Besides, we would require to convert many of current LED drivers to verify how the proposed parsing mechanism will work with them. I've been testing my LED name composition series using QEMU and stubbing things in drivers where necessary and I propose to use the same approach in this case. Best regards, Jacek Anaszewski On 11/18/19 8:03 AM, Matti Vaittinen wrote: > Qucik 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 linux,led-compatible property) 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). > > Regarding the node look-up: If no init_data is given, then no > node-lookup is done and cdev name is used as such. > > If init_data is goven but no starting point for node lookup - then > (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 > 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> > --- > > Changes from v4: > Fixed issues reported by Dan Carpenter and kbuild-bot. > (leftover kfree and uninitialized return value) > > drivers/leds/led-class.c | 88 ++++++++++++-- > drivers/leds/led-core.c | 246 +++++++++++++++++++++++++++++++-------- > include/linux/leds.h | 90 ++++++++++++-- > 3 files changed, 359 insertions(+), 65 deletions(-) >
Hello Jacek, On Mon, 2019-11-18 at 22:55 +0100, Jacek Anaszewski wrote: > Hi Matti, > > Thank you for the patch. If your driver does not depend > on it then please send is separately. The BD71828 depends on device-tree node look-up. It does not utilize the common property parsing. I could of course do the child dt-node walking in BD71828 driver - but it kind of breaks my motivation to do the LED core improvement if I anyways need to do the parsing in BD71828 driver ;) > Besides, we would require > to convert many of current LED drivers to verify how the > proposed parsing mechanism will work with them. I see the risk you are pointing out. And I actually think we could default to old mechanism if of_match or match_property is not given (for now). I could then see the existing drivers who use init_data - and ensure those are initializing the new match_property and of_match in init_data with 0. That would be quite trivial task. That would allow us to convert and test existing drivers one-by-one while allowing new drivers to offload the LED node look-up and common property parsing to LED core. No risk, but less drivers to convert in the future - and simpler drivers to maintain when all of them do not need to duplicate node look-up or basic property parsing ;) To make this more concrete: We can only do the new DT node look-up if either if (init_data->match_property.name && init_data->match_property.size) or if (init_data->of_match) That would keep the node-lookup same for all existing drivers. Eg, led_find_fwnode could for now just do: struct fwnode_handle *led_find_fwnode(struct device *parent, struct led_init_data *init_data) { /* * This should never be called W/O init data. */ if (!init_data) return NULL; /* * For old drivers which do not populate new match information * just directly use the given init_data->fwnode no matter if * it is NULL or not. - as old functionality did. */ if ( (!init_data->match_property.name || !init_data->match_property.size) && !init_data->of_match) return init_data->fwnode; /* match information was given - do node look-up */ ... } Furthermore, the common property parsing could also be (for now) done only if match data is given: /* * For now only allow core to parse DT properties if * parsing is explicitly requested by driver or if core has * found new match data from init_data and then set the flag */ if (INVENT_A_COOL_NEW_FLAG_NAME_HERE) led_add_props(led_cdev, &props); or just simply: if ((init_data->match_property.name && init_data->match_property.size) || init_data->of_match) led_add_props(led_cdev, &props); (but this won't allow driver to ask for common parsing even if it was verified for this drv to work - hence I like the flag better) And if you don't feel confident I can even drop the "common property parsing" from the series and leave only the "node look-up if match-data was given" to it. Anyways, I would like to introduce this support while I am working with the BD71828 driver which really has the LEDs - but I can modify the patch series so that it only impacts to drivers which implement the new match data in init_data and leave old drivers to be converted one-by- one when they can be tested. > I've been testing > my LED name composition series using QEMU and stubbing things in > drivers where necessary and I propose to use the same approach > in this case. I don't plan to do any mass-conversion as it is somewhat risky. I can do conversion to some of the drivers (simple ones which I can understand without too much of pain) - and ask if anyone having access to actual HW with LEDs could be kind enough to test the patch for the device. Tested drivers can then be taken in-tree as examples. And who knows, maybe there is some developers looking for a hobby project with access to LED controller to help with the rest ;) I don't have the ambition to change all of the LED drivers but I think I can give my 10 cents by contributing the mechanism and doing few examples :) Anyways, please let me know if you think you could accept patch which won't change existing driver functionality - but allows new drivers to not duplicate the code. Else I'll just duplicate the lookup code in one more driver and hope I don't have another PMIC with LED controller on my table too soon... (I am having "some" pressure to do few other tasks I recently got. So I am afraid I won't have too much time to invest on LEDs this year :( Thus setting up the qemu and starting with stubbing is really not an option for me at this phase). Br, Matti Vaittinen > > Best regards, > Jacek Anaszewski > > On 11/18/19 8:03 AM, Matti Vaittinen wrote: > > Qucik 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 linux,led-compatible property) 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). > > > > Regarding the node look-up: If no init_data is given, then no > > node-lookup is done and cdev name is used as such. > > > > If init_data is goven but no starting point for node lookup - then > > (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 > > 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> > > --- > > > > Changes from v4: > > Fixed issues reported by Dan Carpenter and kbuild-bot. > > (leftover kfree and uninitialized return value) > > > > drivers/leds/led-class.c | 88 ++++++++++++-- > > drivers/leds/led-core.c | 246 +++++++++++++++++++++++++++++++-- > > ------ > > include/linux/leds.h | 90 ++++++++++++-- > > 3 files changed, 359 insertions(+), 65 deletions(-) > >
Thanks Mark, On Mon, 2019-11-18 at 16:20 +0000, Mark Brown wrote: > On Mon, Nov 18, 2019 at 08:57:57AM +0200, Matti Vaittinen wrote: > > > +static int ramp_delay_supported(struct regulator_dev *rdev) > > +{ > > + switch (rdev->desc->id) { > > + case BD71828_BUCK1: > > + case BD71828_BUCK2: > > + case BD71828_BUCK6: > > + case BD71828_BUCK7: > > + return 1; > > + default: > > + break; > > + } > > + return 0; > > +} > > + > > +static int bd71828_set_ramp_delay(struct regulator_dev *rdev, int > > ramp_delay) > > +{ > > + unsigned int val; > > + > > + if (!ramp_delay_supported(rdev)) { > > + dev_err(&rdev->dev, "%s: can't set ramp-delay\n", > > + rdev->desc->name); > > + return -EINVAL; > > Rather than doing this it's better to just not provide the operation > for > devices that don't support it, that makes the handling in the core > easier. Makes sense. I'll change this in next version. Br, Matti Vaittinen
On Mon, 2019-11-18 at 09:03 +0200, Matti Vaittinen wrote: > Qucik 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 linux,led-compatible property) and handling of > few common LED properties. This is actually not completely true. I originally thought of adding led-compatible - but I changed my mind after I saw that at least some of the existing drivers used value of "reg"-property to do the matching in-driver. So, to make it simple for them, I did add property name/value pair in init data for matching. This allows one to do led- compatible, or to use existing fixed reg (or other) property value. I'd better to change the commit message to reflect this too. > > linux,default-trigger, > default-state (with the exception of keep), > > (in addition to already handled > function-enumerator, > function, > color > and label). > > Regarding the node look-up: If no init_data is given, then no > node-lookup is done and cdev name is used as such. > > If init_data is goven but no starting point for node lookup - then > (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 > is given then device's own node or passed starting point are used as > such. And same here. > > Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> > --- > > Changes from v4: > Fixed issues reported by Dan Carpenter and kbuild-bot. > (leftover kfree and uninitialized return value) > > drivers/leds/led-class.c | 88 ++++++++++++-- > drivers/leds/led-core.c | 246 +++++++++++++++++++++++++++++++---- > ---- > include/linux/leds.h | 90 ++++++++++++-- > 3 files changed, 359 insertions(+), 65 deletions(-) > > diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c > index 647b1263c579..98173b64a597 100644 > --- a/drivers/leds/led-class.c > +++ b/drivers/leds/led-class.c > @@ -235,6 +235,29 @@ 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")) > + ld->brightness = LED_FULL; > + /* > + * 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 +274,58 @@ 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_find_fwnode(parent, init_data); > + if (!fw) { > + dev_err(parent, "No fwnode found\n"); > + return -ENOENT; > + } > + /* > + * 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) > - return ret; > + goto err_out; > + > + led_add_props(led_cdev, &props); > + > + ret = led_compose_name(parent, init_data, &props, > + composed_name); > + if (ret < 0) > + 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 +333,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 +387,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; > } > EXPORT_SYMBOL_GPL(led_classdev_register_ext); > > @@ -355,6 +424,9 @@ 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); > + > 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..9369f03ee540 100644 > --- a/drivers/leds/led-core.c > +++ b/drivers/leds/led-core.c > @@ -365,70 +365,214 @@ 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_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 > + * dev_fwnode() - but then we should pump-up the refcount > + */ > + if (!init_data) > + return NULL; > + > + 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 selectible. 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); > + else > + fw = fwnode_handle_get(fw); > + > + return fw; > +} > +EXPORT_SYMBOL(led_find_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 +580,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 +612,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_find_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 efb309dba914..aea7d6bc7294 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,17 @@ 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); > }; > > struct led_classdev { > + struct led_init_data *init_data; > const char *name; > enum led_brightness brightness; > enum led_brightness max_brightness; > @@ -148,6 +197,7 @@ struct led_classdev { > > /* Ensures consistent access to the LED Flash Class device */ > struct mutex led_access; > + bool fwnode_owned; > }; > > /** > @@ -302,6 +352,7 @@ extern 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. > @@ -311,6 +362,7 @@ extern void led_sysfs_enable(struct led_classdev > *led_cdev); > * Returns: 0 on success or negative error value on failure > */ > extern int led_compose_name(struct device *dev, struct led_init_data > *init_data, > + struct led_properties *props, > char *led_classdev_name); > > /** > @@ -324,6 +376,33 @@ static inline bool led_sysfs_is_disabled(struct > led_classdev *led_cdev) > return led_cdev->flags & LED_SYSFS_DISABLE; > } > > +/** > + * 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); > + > +/** > + * 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 > */ > @@ -490,15 +569,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 Mon, Nov 18, 2019 at 7:58 AM Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> wrote: > Bunch of MFD sub-devices which are instantiated by MFD do not have > own device-tree nodes but have (for example) the GPIO consumer > information in parent device's DT node. Add resource managed > devm_gpiod_get_array() for such devices so that they can get the > consumer information from parent DT while still binding the GPIO > reservation life-time to this sub-device life time. > > If devm_gpiod_get_array is used as such - then unloading and then > re-loading the child device fails as the GPIOs reserved during first > load are not freed when driver for sub-device is unload (if parent > stays there). > > Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> (...) > +static struct gpio_descs *__must_check > +__devm_gpiod_get_array(struct device *gpiodev, > + struct device *managed, > + const char *con_id, > + enum gpiod_flags flags) I'm opposed to functions named __underscore_something() so find a proper name for this function. devm_gpiod_get_array_common() works if nothing else. > @@ -292,19 +284,62 @@ struct gpio_descs *__must_check devm_gpiod_get_array(struct device *dev, > if (!dr) > return ERR_PTR(-ENOMEM); > > - descs = gpiod_get_array(dev, con_id, flags); > + descs = gpiod_get_array(gpiodev, con_id, flags); > if (IS_ERR(descs)) { > devres_free(dr); > return descs; > } > > *dr = descs; > - devres_add(dev, dr); > + if (managed) > + devres_add(managed, dr); > + else > + devres_add(gpiodev, dr); So we only get managed resources if the "managed" device is passed in. > +/** > + * devm_gpiod_get_array - Resource-managed gpiod_get_array() And this function is supposed to be resource managed for sure. > + * @dev: GPIO consumer > + * @con_id: function within the GPIO consumer > + * @flags: optional GPIO initialization flags > + * > + * Managed gpiod_get_array(). GPIO descriptors returned from this function are > + * automatically disposed on driver detach. See gpiod_get_array() for detailed > + * information about behavior and return values. > + */ > +struct gpio_descs *__must_check devm_gpiod_get_array(struct device *dev, > + const char *con_id, > + enum gpiod_flags flags) > +{ > + return __devm_gpiod_get_array(dev, NULL, con_id, flags); So what is this? NULL? Doesn't that mean you just removed all resource management for this call? Or am I reading it wrong? Yours, Linus Walleij
Hello Linus, On Tue, 2019-11-19 at 15:43 +0100, Linus Walleij wrote: > On Mon, Nov 18, 2019 at 7:58 AM Matti Vaittinen > <matti.vaittinen@fi.rohmeurope.com> wrote: > > > Bunch of MFD sub-devices which are instantiated by MFD do not have > > own device-tree nodes but have (for example) the GPIO consumer > > information in parent device's DT node. Add resource managed > > devm_gpiod_get_array() for such devices so that they can get the > > consumer information from parent DT while still binding the GPIO > > reservation life-time to this sub-device life time. > > > > If devm_gpiod_get_array is used as such - then unloading and then > > re-loading the child device fails as the GPIOs reserved during > > first > > load are not freed when driver for sub-device is unload (if parent > > stays there). > > > > Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> > (...) > > +static struct gpio_descs *__must_check > > +__devm_gpiod_get_array(struct device *gpiodev, > > + struct device *managed, > > + const char *con_id, > > + enum gpiod_flags flags) > > I'm opposed to functions named __underscore_something() > so find a proper name for this function. > devm_gpiod_get_array_common() works if nothing else. Ok. We all have our own pecularitie... I mean preferences :) But as this is definitely your territory - I'll change the name :) No problem. > > > @@ -292,19 +284,62 @@ struct gpio_descs *__must_check > > devm_gpiod_get_array(struct device *dev, > > if (!dr) > > return ERR_PTR(-ENOMEM); > > > > - descs = gpiod_get_array(dev, con_id, flags); > > + descs = gpiod_get_array(gpiodev, con_id, flags); > > if (IS_ERR(descs)) { > > devres_free(dr); > > return descs; > > } > > > > *dr = descs; > > - devres_add(dev, dr); > > + if (managed) > > + devres_add(managed, dr); > > + else > > + devres_add(gpiodev, dr); > > So we only get managed resources if the "managed" device is > passed in. Hmm. Actually, no if I am not misreading this. We get managed devices in any case, but the lifetime is either bound to exitence of the gpio consumer device (which is standard way) - or existence of specific 'managed' device. In case of MFD sub-device (like BD71828 regulator subdev) which has GPIO consumer properties in MFD node - the GPIO consumer information is in parent device's (BD71828 MFD device) data - but the lifetime should be bound to sub-devices (BD71828 regulator device) lifetime. Thus we need two different devices here. > > > +/** > > + * devm_gpiod_get_array - Resource-managed gpiod_get_array() > > And this function is supposed to be resource managed for sure. Yes. This is the standard case where device which has the consumer data is also the one who 'manages' the GPIO. > > > + * @dev: GPIO consumer > > + * @con_id: function within the GPIO consumer > > + * @flags: optional GPIO initialization flags > > + * > > + * Managed gpiod_get_array(). GPIO descriptors returned from this > > function are > > + * automatically disposed on driver detach. See gpiod_get_array() > > for detailed > > + * information about behavior and return values. > > + */ > > +struct gpio_descs *__must_check devm_gpiod_get_array(struct device > > *dev, > > + const char > > *con_id, > > + enum > > gpiod_flags flags) > > +{ > > + return __devm_gpiod_get_array(dev, NULL, con_id, flags); > > So what is this? NULL? Here we don't have separate manager device - thus the manager is NULL -and the consumer device ("dev" here) is what we use to manage GPIO. > > Doesn't that mean you just removed all resource management for this > call? No :) > > Or am I reading it wrong? Either you are reading it wrong or I am writing it wrong xD. In any case this means I need to drop few comments in code :) Thanks. Br, Matti Vaittinen
Hi Matti, On 11/19/19 8:21 AM, Vaittinen, Matti wrote: > Hello Jacek, > > On Mon, 2019-11-18 at 22:55 +0100, Jacek Anaszewski wrote: >> Hi Matti, >> >> Thank you for the patch. If your driver does not depend >> on it then please send is separately. > > The BD71828 depends on device-tree node look-up. It does not utilize > the common property parsing. I could of course do the child dt-node > walking in BD71828 driver - but it kind of breaks my motivation to do > the LED core improvement if I anyways need to do the parsing in BD71828 > driver ;) If you do not plan on spending too much time on contributing this set then I propose adhering to the currently used parsing schema :-) And you have to know that from this development cycle I handed over LED tree maintenance to Pavel Machek, so you will require to have his acceptance in the first place. >> Besides, we would require >> to convert many of current LED drivers to verify how the >> proposed parsing mechanism will work with them. > > I see the risk you are pointing out. And I actually think we could > default to old mechanism if of_match or match_property is not given > (for now). I could then see the existing drivers who use init_data - > and ensure those are initializing the new match_property and of_match > in init_data with 0. That would be quite trivial task. > > That would allow us to convert and test existing drivers one-by-one > while allowing new drivers to offload the LED node look-up and common > property parsing to LED core. No risk, but less drivers to convert in > the future - and simpler drivers to maintain when all of them do not > need to duplicate node look-up or basic property parsing ;) I personally would prefer to do the massive driver update to using the new mechanism. I know that this is time consuming but we are not in a hurry. > To make this more concrete: > > We can only do the new DT node look-up if either > if (init_data->match_property.name && init_data->match_property.size) > or > if (init_data->of_match) > That would keep the node-lookup same for all existing drivers. > > Eg, > led_find_fwnode could for now just do: > > struct fwnode_handle *led_find_fwnode(struct device *parent, > struct led_init_data *init_data) > { > /* > * This should never be called W/O init data. > */ > if (!init_data) > return NULL; > > /* > * For old drivers which do not populate new match information > * just directly use the given init_data->fwnode no matter if > * it is NULL or not. - as old functionality did. > */ > if ( (!init_data->match_property.name || > !init_data->match_property.size) && !init_data->of_match) > return init_data->fwnode; > > /* match information was given - do node look-up */ > ... > } > > Furthermore, the common property parsing could also be (for now) done > only if match data is given: > > /* > * For now only allow core to parse DT properties if > * parsing is explicitly requested by driver or if core has > * found new match data from init_data and then set the flag > */ > if (INVENT_A_COOL_NEW_FLAG_NAME_HERE) > led_add_props(led_cdev, &props); > > or just simply: > if ((init_data->match_property.name && > init_data->match_property.size) || init_data->of_match) > led_add_props(led_cdev, &props); > > (but this won't allow driver to ask for common parsing even if it was > verified for this drv to work - hence I like the flag better) > > And if you don't feel confident I can even drop the "common property > parsing" from the series and leave only the "node look-up if match-data > was given" to it. > > Anyways, I would like to introduce this support while I am working with > the BD71828 driver which really has the LEDs - but I can modify the > patch series so that it only impacts to drivers which implement the new > match data in init_data and leave old drivers to be converted one-by- > one when they can be tested. > >> I've been testing >> my LED name composition series using QEMU and stubbing things in >> drivers where necessary and I propose to use the same approach >> in this case. > > I don't plan to do any mass-conversion as it is somewhat risky. I can You do not need hardware to test DT parsing as I mentioned before, so I don't see too much risk involved. > do conversion to some of the drivers (simple ones which I can > understand without too much of pain) - and ask if anyone having access > to actual HW with LEDs could be kind enough to test the patch for the > device. Tested drivers can then be taken in-tree as examples. And who > knows, maybe there is some developers looking for a hobby project with > access to LED controller to help with the rest ;) I don't have the > ambition to change all of the LED drivers but I think I can give my 10 > cents by contributing the mechanism and doing few examples :) If you want to introduce good, robust mechanism, then it should be tested against widest possible spectrum of use cases. > Anyways, please let me know if you think you could accept patch which > won't change existing driver functionality - but allows new drivers to > not duplicate the code. Else I'll just duplicate the lookup code in one > more driver and hope I don't have another PMIC with LED controller on > my table too soon... > > (I am having "some" pressure to do few other tasks I recently got. So I > am afraid I won't have too much time to invest on LEDs this year :( > Thus setting up the qemu and starting with stubbing is really not an > option for me at this phase). As mentioned before - I no longer apply patches so you will need to consult Pavel, but I bet he will have similar opinion.
On Tue, 2019-11-19 at 20:30 +0100, Jacek Anaszewski wrote: > Hi Matti, > > On 11/19/19 8:21 AM, Vaittinen, Matti wrote: > > Hello Jacek, > > > > On Mon, 2019-11-18 at 22:55 +0100, Jacek Anaszewski wrote: > > > Hi Matti, > > > > > > Thank you for the patch. If your driver does not depend > > > on it then please send is separately. > > > > The BD71828 depends on device-tree node look-up. It does not > > utilize > > the common property parsing. I could of course do the child dt-node > > walking in BD71828 driver - but it kind of breaks my motivation to > > do > > the LED core improvement if I anyways need to do the parsing in > > BD71828 > > driver ;) > > If you do not plan on spending too much time on contributing this > set then I propose adhering to the currently used parsing schema :-) I have no objections on doing few iterations of the patches. And I tend to take care of problems my changes cause. So I am prepared to spend the required time fixing the node look-up and common property parsing for drivers I do break. What I am not prepared is to change and test all of the existing drivers - so it's better to not promise such :) > And you have to know that from this development cycle I handed > over LED tree maintenance to Pavel Machek, so you will require > to have his acceptance in the first place. Well, then I for sure wait for Pavel's take on this. In general I have had some positive feedback about doing the DT node look-up and common property parsing in a centralized manner in LED core. So maybe also Pavel sees the value of adding this now for new drivers - instead of adding one more driver with copy-paste node look-up code. I want to thank you for all the comments though, it's nice that you have been active on this topic! > > > Besides, we would require > > > to convert many of current LED drivers to verify how the > > > proposed parsing mechanism will work with them. > > > > I see the risk you are pointing out. And I actually think we could > > default to old mechanism if of_match or match_property is not given > > (for now). I could then see the existing drivers who use init_data > > - > > and ensure those are initializing the new match_property and > > of_match > > in init_data with 0. That would be quite trivial task. > > > > That would allow us to convert and test existing drivers one-by-one > > while allowing new drivers to offload the LED node look-up and > > common > > property parsing to LED core. No risk, but less drivers to convert > > in > > the future - and simpler drivers to maintain when all of them do > > not > > need to duplicate node look-up or basic property parsing ;) > > I personally would prefer to do the massive driver update to using > the new mechanism. I know that this is time consuming but we are not > in a hurry. I understand the preference of massive update - but I also know that if we wait for someone to do a massive update and neglect improvements done in small steps, then there is a risk that there won't be any updates at all... > > To make this more concrete: > > > > We can only do the new DT node look-up if either > > if (init_data->match_property.name && init_data- > > >match_property.size) > > or > > if (init_data->of_match) > > That would keep the node-lookup same for all existing drivers. > > > > Eg, > > led_find_fwnode could for now just do: > > > > struct fwnode_handle *led_find_fwnode(struct device *parent, > > struct led_init_data *init_data) > > { > > /* > > * This should never be called W/O init data. > > */ > > if (!init_data) > > return NULL; > > > > /* > > * For old drivers which do not populate new match information > > * just directly use the given init_data->fwnode no matter if > > * it is NULL or not. - as old functionality did. > > */ > > if ( (!init_data->match_property.name || > > !init_data->match_property.size) && !init_data->of_match) > > return init_data->fwnode; > > > > /* match information was given - do node look-up */ > > ... > > } > > > > Furthermore, the common property parsing could also be (for now) > > done > > only if match data is given: > > > > /* > > * For now only allow core to parse DT properties if > > * parsing is explicitly requested by driver or if core has > > * found new match data from init_data and then set the flag > > */ > > if (INVENT_A_COOL_NEW_FLAG_NAME_HERE) > > led_add_props(led_cdev, &props); > > > > or just simply: > > if ((init_data->match_property.name && > > init_data->match_property.size) || init_data->of_match) > > led_add_props(led_cdev, &props); > > > > (but this won't allow driver to ask for common parsing even if it > > was > > verified for this drv to work - hence I like the flag better) > > > > And if you don't feel confident I can even drop the "common > > property > > parsing" from the series and leave only the "node look-up if match- > > data > > was given" to it. > > > > Anyways, I would like to introduce this support while I am working > > with > > the BD71828 driver which really has the LEDs - but I can modify the > > patch series so that it only impacts to drivers which implement the > > new > > match data in init_data and leave old drivers to be converted one- > > by- > > one when they can be tested. > > > > > I've been testing > > > my LED name composition series using QEMU and stubbing things in > > > drivers where necessary and I propose to use the same approach > > > in this case. > > > > I don't plan to do any mass-conversion as it is somewhat risky. I > > can > > You do not need hardware to test DT parsing as I mentioned before, > so I don't see too much risk involved. Yes - if you have the time to test all the drivers at once - and assuming you don't do some silly mistake there. Final verification should always be done in HW. But as I said, I want to ensure all the drivers I convert to new mechanism will work (the best I can) - and as I can't test all the drivers I won't do mass-conversion. I have offered a initial solution in the patch (and suggested reduced version to mitigate the risk of breaking anything in this email) - and I see this beneficial and as a good starting point enabling the rest of the improvements. But as you said, we need also Pavel's take on this. > > do conversion to some of the drivers (simple ones which I can > > understand without too much of pain) - and ask if anyone having > > access > > to actual HW with LEDs could be kind enough to test the patch for > > the > > device. Tested drivers can then be taken in-tree as examples. And > > who > > knows, maybe there is some developers looking for a hobby project > > with > > access to LED controller to help with the rest ;) I don't have the > > ambition to change all of the LED drivers but I think I can give my > > 10 > > cents by contributing the mechanism and doing few examples :) > > If you want to introduce good, robust mechanism, then it should be > tested against widest possible spectrum of use cases. Yes. OTOH, if the mechanism is sub-optimal, then the beauty of open source is that it can be improved. Preparing in advance for something that never happens is also a waste. But I guess we don't need to discuss this philosphy here :) > > Anyways, please let me know if you think you could accept patch > > which > > won't change existing driver functionality - but allows new drivers > > to > > not duplicate the code. Else I'll just duplicate the lookup code in > > one > > more driver and hope I don't have another PMIC with LED controller > > on > > my table too soon... > > > > (I am having "some" pressure to do few other tasks I recently got. > > So I > > am afraid I won't have too much time to invest on LEDs this year :( > > Thus setting up the qemu and starting with stubbing is really not > > an > > option for me at this phase). > > As mentioned before - I no longer apply patches so you will need to > consult Pavel, but I bet he will have similar opinion. Who knows, maybe he can see this differently :) Thanks anyways! Br, Matti Vaittinen
On Tue, Nov 19, 2019 at 6:54 PM Vaittinen, Matti <Matti.Vaittinen@fi.rohmeurope.com> wrote: > [Me] > > So what is this? NULL? > > Here we don't have separate manager device - thus the manager is NULL > -and the consumer device ("dev" here) is what we use to manage GPIO. > > > > > Doesn't that mean you just removed all resource management for this > > call? > > No :) > > > > > Or am I reading it wrong? > > Either you are reading it wrong or I am writing it wrong xD. In any > case this means I need to drop few comments in code :) Thanks. I was reading it wrong, so not your bad. I guess lack of focus on my side, this part is fine! Yours, Linus Walleij
Hi, On 18/11/2019 09:00:47+0200, Matti Vaittinen wrote: > @@ -468,26 +596,35 @@ static int bd70528_probe(struct platform_device *pdev) > * leave them enabled as irq-controller should disable irqs > * from sub-registers when IRQ is disabled or freed. > */ > - ret = regmap_update_bits(mfd->regmap, > + if (enable_main_irq) { > + ret = regmap_update_bits(mfd->regmap, > BD70528_REG_INT_MAIN_MASK, > BD70528_INT_RTC_MASK, 0); > - if (ret) { > - dev_err(&pdev->dev, "Failed to enable RTC interrupts\n"); > - return ret; > + if (ret) { > + dev_err(&pdev->dev, "Failed to enable RTC interrupts\n"); > + return ret; > + } > } > > return rtc_register_device(rtc); > } Missing blank line here. > +static const struct platform_device_id bd718x7_rtc_id[] = { > + { "bd70528-rtc", ROHM_CHIP_TYPE_BD70528 }, > + { "bd71828-rtc", ROHM_CHIP_TYPE_BD71828 }, > + { }, > +}; > +MODULE_DEVICE_TABLE(platform, bd718x7_rtc_id); > Else, Acked-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
Patch series introducing support for ROHM BD71828 PMIC ROHM BD71828 is a power management IC containing 7 bucks and 7 LDOs. All regulators can either 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. Run level changes can then be initiated wither via I2C writes or GPIO. and when run-level is changed, state of all bucks which are set to be controlled via run-levels are changed accrdingly. This control mechanism selection (I2C or GPIO) is selected by data in one time programmable PMIC memory area (during production) and can't be changed later. 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 "under construction" and 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. In GPIO framework this series adds devm-support for gpio_array getting for MFD sub-devices whose GPIO consumer information may be in parent device's DT node. And while I was at it I also added few missing GPIO devm functions to the documentaton listing. 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 regulators on BD71828 PMIC Patch 2: dt-bindings for LEDs on BD71828 PMIC Patch 3: dt-bindings for BD71828 PMIC Patch 4: Convert rohm PMICs with common sub-devices to use platform_ device_id to match MFD sub-devices 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: Basic regulator support (individual control via I2C). This should be pretty standard stuff. Patch 10: Add devm_gpiod_get_parent_array Patch 11: Add missing managed GPIO array get functions to documentation Patch 12: Add support for getting regulator voltages when run-levels are used. Allow specifying voltages for run-levels via DT and in-kernel API. Support changing run-levels via in-kernel API. Patch 13: Support BD71828 RTC block using BD70528 RTC driver Patch 14: Allow control of GP(I)O pins on BD71828 via GPIO subsystem Patch 15: Add LED node lookup and common LED binding parsing support to LED class/core Patch 16: Support toggling the LEDs on BD71828. This patch series is based on v5.4-rc7 --- Matti Vaittinen (16): dt-bindings: regulator: Document ROHM BD71282 regulator bindings 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: 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 regulator: bd71828: Basic support for ROHM bd71828 PMIC regulators gpio: devres: Add devm_gpiod_get_parent_array docs: driver-model: Add missing managed GPIO array get functions regulator: bd71828: Add GPIO based run-level control for regulators 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 | 49 + .../bindings/mfd/rohm,bd71828-pmic.yaml | 249 +++ .../regulator/rohm,bd71828-regulator.yaml | 122 ++ .../driver-api/driver-model/devres.rst | 3 + 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/gpio/gpiolib-devres.c | 65 +- drivers/leds/Kconfig | 10 + drivers/leds/Makefile | 1 + drivers/leds/led-class.c | 88 +- drivers/leds/led-core.c | 246 ++- drivers/leds/leds-bd71828.c | 104 ++ drivers/mfd/Kconfig | 15 + drivers/mfd/Makefile | 2 +- drivers/mfd/rohm-bd70528.c | 3 +- drivers/mfd/rohm-bd71828.c | 345 +++++ drivers/mfd/rohm-bd718x7.c | 39 +- drivers/regulator/Kconfig | 16 + drivers/regulator/Makefile | 2 + drivers/regulator/bd71828-regulator.c | 1374 +++++++++++++++++ drivers/regulator/bd718x7-regulator.c | 200 +-- drivers/regulator/rohm-regulator.c | 95 ++ drivers/rtc/Kconfig | 3 +- drivers/rtc/rtc-bd70528.c | 167 +- include/linux/gpio/consumer.h | 5 + include/linux/leds.h | 90 +- include/linux/mfd/rohm-bd70528.h | 19 +- include/linux/mfd/rohm-bd71828.h | 428 +++++ include/linux/mfd/rohm-bd718x7.h | 6 - include/linux/mfd/rohm-generic.h | 48 +- include/linux/mfd/rohm-shared.h | 27 + 34 files changed, 3771 insertions(+), 278 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 Documentation/devicetree/bindings/regulator/rohm,bd71828-regulator.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/bd71828-regulator.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: 31f4f5b495a62c9a8b15b1c3581acd5efeb9af8c