| Message ID | cover.1617020713.git.matti.vaittinen@fi.rohmeurope.com |
|---|---|
| Headers | show |
| Series | Support ROHM BD71815 PMIC | expand |
On Mon, Mar 29, 2021 at 3:58 PM Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> wrote: > > Support GPO(s) found from ROHM BD71815 power management IC. The IC has two > GPO pins but only one is properly documented in data-sheet. The driver in the datasheet > exposes by default only the documented GPO. The second GPO is connected to > E5 pin and is marked as GND in data-sheet. Control for this undocumented in the datasheet > pin can be enabled using a special DT property. > > This driver is derived from work by Peter Yang <yanglsh@embest-tech.com> > although not so much of original is left. of the original It seems you ignored my comments about the commit message. :-( > +struct bd71815_gpio { > + struct gpio_chip chip; > + struct device *dev; Wondering why you need this. Is it the same as chip.parent? > + struct regmap *regmap; > +}; ... > + int ret, bit; > + > + bit = BIT(offset); I prefer int bit = BIT(offset); int ret; but I think we already discussed that. OK. ... > + default: > + break; > + } > + return -ENOTSUPP; Here is a waste of line. Why break instead of direct return? ... > +/* Template for GPIO chip */ > +static const struct gpio_chip bd71815gpo_chip = { > + .label = "bd71815", > + .owner = THIS_MODULE, > + .get = bd71815gpo_get, > + .get_direction = bd71815gpo_direction_get, > + .set = bd71815gpo_set, > + .set_config = bd71815_gpio_set_config, > + .can_sleep = 1, Strictly speaking this should be true (boolean type value). > +}; ... > +#define BD71815_TWO_GPIOS 0x3UL > +#define BD71815_ONE_GPIO 0x1UL Are they masks? Can you use BIT() and GENMASK()? ... > +/* > + * Sigh. The BD71815 and BD71817 were originally designed to support two GPO > + * pins. At some point it was noticed the second GPO pin which is the E5 pin > + * located at the center of IC is hard to use on PCB (due to the location). It > + * was decided to not promote this second GPO and pin is marked as GND in the and the pin > + * datasheet. The functionality is still there though! I guess driving a GPO > + * connected to the ground is a bad idea. Thus we do not support it by default. > + * OTOH - the original driver written by colleagues at Embest did support > + * controlling this second GPO. It is thus possible this is used in some of the > + * products. > + * > + * This driver does not by default support configuring this second GPO > + * but allows using it by providing the DT property > + * "rohm,enable-hidden-gpo". > + */ ... > + /* > + * As writing of this the sysfs interface for GPIO control does not > + * respect the valid_mask. Do not trust it but rather set the ngpios > + * to 1 if "rohm,enable-hidden-gpo" is not given. > + * > + * This check can be removed later if the sysfs export is fixed and > + * if the fix is backported. So, mark this comment with the TODO/FIXME keyword? > + * > + * For now it is safest to just set the ngpios though. > + */ ... > + ret = devm_gpiochip_add_data(dev, &g->chip, g); > + if (ret < 0) { > + dev_err(dev, "could not register gpiochip, %d\n", ret); > + return ret; > + } > + > + return ret; This entire piece can be simplified by return devm_gpiochip_add_data(...); ... > +static struct platform_driver gpo_bd71815_driver = { > + .driver = { > + .name = "bd71815-gpo", > + .owner = THIS_MODULE, Seems I commented on this. The module_*_driver() macro(s) will take care of it. > + }, > + .probe = gpo_bd71815_probe, > +}; > + Extra blank line. Drop it. > +module_platform_driver(gpo_bd71815_driver);
Hi Andy, On Tue, 2021-03-30 at 13:11 +0300, Andy Shevchenko wrote: > On Mon, Mar 29, 2021 at 3:58 PM Matti Vaittinen > <matti.vaittinen@fi.rohmeurope.com> wrote: > > Support GPO(s) found from ROHM BD71815 power management IC. The IC > > has two > > GPO pins but only one is properly documented in data-sheet. The > > driver > > in the datasheet > > > exposes by default only the documented GPO. The second GPO is > > connected to > > E5 pin and is marked as GND in data-sheet. Control for this > > undocumented > > in the datasheet > > > pin can be enabled using a special DT property. > > > > This driver is derived from work by Peter Yang < > > yanglsh@embest-tech.com> > > although not so much of original is left. > > of the original > > It seems you ignored my comments about the commit message. :-( Sorry. I didn't do that by purpose. I forgot to reword commit. Completely my bad. > > +struct bd71815_gpio { > > + struct gpio_chip chip; > > + struct device *dev; > > Wondering why you need this. Is it the same as chip.parent? > > > + struct regmap *regmap; > > +}; > > ... > > > + int ret, bit; > > + > > + bit = BIT(offset); > > I prefer > int bit = BIT(offset); > int ret; > but I think we already discussed that. OK. Yes, we did. > ... > > > + default: > > + break; > > + } > > + return -ENOTSUPP; > > Here is a waste of line. Why break instead of direct return? As we discussed last time, I do prefer functions which are supposed to return a value, do so at the end of function. It's easier to read and does not cause issues if someone changes switch to if-else or does other modifications. IMO original is safer, reads better and does not cause issues even with old compilers. > ... > > > +/* Template for GPIO chip */ > > +static const struct gpio_chip bd71815gpo_chip = { > > + .label = "bd71815", > > + .owner = THIS_MODULE, > > + .get = bd71815gpo_get, > > + .get_direction = bd71815gpo_direction_get, > > + .set = bd71815gpo_set, > > + .set_config = bd71815_gpio_set_config, > > + .can_sleep = 1, > > Strictly speaking this should be true (boolean type value). true. > > > +}; > > ... > > > +#define BD71815_TWO_GPIOS 0x3UL > > +#define BD71815_ONE_GPIO 0x1UL > > Are they masks? Can you use BIT() and GENMASK()? Yes and yes. I personally prefer 0x3 over GENMASK() as for me the value 3 as bitmask is perfectly readable. But I know others may prefer using GENMASK(). So yes, your comment is valid. > > +/* > > + * Sigh. The BD71815 and BD71817 were originally designed to > > support two GPO > > + * pins. At some point it was noticed the second GPO pin which is > > the E5 pin > > + * located at the center of IC is hard to use on PCB (due to the > > location). It > > + * was decided to not promote this second GPO and pin is marked as > > GND in the > > and the pin > > > + * datasheet. The functionality is still there though! I guess > > driving a GPO > > + * connected to the ground is a bad idea. Thus we do not support > > it by default. > > + * OTOH - the original driver written by colleagues at Embest did > > support > > + * controlling this second GPO. It is thus possible this is used > > in some of the > > + * products. > > + * > > + * This driver does not by default support configuring this second > > GPO > > + * but allows using it by providing the DT property > > + * "rohm,enable-hidden-gpo". > > + */ > I am sorry. I think I missed this one too. > ... > > > + /* > > + * As writing of this the sysfs interface for GPIO control > > does not > > + * respect the valid_mask. Do not trust it but rather set > > the ngpios > > + * to 1 if "rohm,enable-hidden-gpo" is not given. > > + * > > + * This check can be removed later if the sysfs export is > > fixed and > > + * if the fix is backported. > > So, mark this comment with the TODO/FIXME keyword? I haven't used to use keywords like TODO/FIXME. Now that I think of it I've seen a few FIXME comments in sources so perhaps I should start using them where appropriate. I don't think it makes a big difference here though as I expect to be reworking this in near future (I'll revise ROHM PMIC GPIO drivers for regmap_gpio usage during this spring). I added this comment so I can revise this at that point. > > > + * > > + * For now it is safest to just set the ngpios though. > > + */ > > ... > > > + ret = devm_gpiochip_add_data(dev, &g->chip, g); > > + if (ret < 0) { > > + dev_err(dev, "could not register gpiochip, %d\n", > > ret); > > + return ret; > > + } > > + > > + return ret; > > Sorry again. I somehow overlooked this comment as well. > ... > > > +static struct platform_driver gpo_bd71815_driver = { > > + .driver = { > > + .name = "bd71815-gpo", > > + .owner = THIS_MODULE, > > Seems I commented on this. The module_*_driver() macro(s) will take > care of it. Yes you did. I missed this too. Sorry. Andy, how fatal do you think these issues are? I did put these comments on my 'things to clean-up' list. If you don't see them as fatal, then I rather not resend whole series of 19 patches just for these. I am anyway going to rework the ROHM PMIC GPIO drivers which I have authored during the next couple of months for regmap_gpio usage. This series has most of the acks except for the regulator part - so I was about to suggest to Lee that perhaps he could apply other but regulator stuff to MFD so I could squeeze the recipient list and amount of patches in series. Best Regards Matti Vaittinen
On Tue, Mar 30, 2021 at 1:43 PM Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> wrote: > On Tue, 2021-03-30 at 13:11 +0300, Andy Shevchenko wrote: ... > Andy, how fatal do you think these issues are? I did put these comments > on my 'things to clean-up' list. > > If you don't see them as fatal, then I rather not resend whole series > of 19 patches just for these. I am anyway going to rework the ROHM PMIC > GPIO drivers which I have authored during the next couple of months for > regmap_gpio usage. This series has most of the acks except for the > regulator part - so I was about to suggest to Lee that perhaps he could > apply other but regulator stuff to MFD so I could squeeze the recipient > list and amount of patches in series. I understand that. I'm not a maintainer, but my personal view is that it can be fixed in follow ups. The problem as usual here is that people often forget to cook / send follow up. That's why lately I'm more insisting on changes to be done as soon as possible. -- With Best Regards, Andy Shevchenko
On Tue, 2021-03-30 at 13:54 +0300, Andy Shevchenko wrote: > On Tue, Mar 30, 2021 at 1:43 PM Matti Vaittinen > <matti.vaittinen@fi.rohmeurope.com> wrote: > > On Tue, 2021-03-30 at 13:11 +0300, Andy Shevchenko wrote: > > ... > > > Andy, how fatal do you think these issues are? I did put these > > comments > > on my 'things to clean-up' list. > > > > If you don't see them as fatal, then I rather not resend whole > > series > > of 19 patches just for these. I am anyway going to rework the ROHM > > PMIC > > GPIO drivers which I have authored during the next couple of months > > for > > regmap_gpio usage. This series has most of the acks except for the > > regulator part - so I was about to suggest to Lee that perhaps he > > could > > apply other but regulator stuff to MFD so I could squeeze the > > recipient > > list and amount of patches in series. > > I understand that. I'm not a maintainer, but my personal view is that > it can be fixed in follow ups. Thanks Andy. The series already had acks from Bartosz and Linus so I hope they are also Ok with fixing these when reworking for regmap_gpio (I intend to do that during 5.13-rc cycle). Best Regards Matti Vaittinen
On Mon, 2021-03-29 at 15:52 +0300, Matti Vaittinen wrote: > Patch series introducing support for ROHM BD71815 PMIC > > ROHM BD71815 is a power management IC used in some battery powered > systems. It contains regulators, GPO(s), charger + coulomb counter, > RTC > and a clock gate. Lee, Mark, (Linus, Bartosz, all) I think all other parts except the regulator have relevant acks. (Well, I changed GPIO in last version so Bart/Linus may want changes but those will follow to GPIO in near future anyways). Do you think Lee could merge other but the regulator parts to MFD if Mark is busy? I'd like to be able to squeeze the amount of patches and recipients for future iterations. It might be easier to work directly on regulator tree if regulator part gets delayed to next cycle. (I do also plan further working with the GPIO part during 5.13-rc cycle to utilize the regmap_gpio. That could be done in the GPIO tree then). I think the other portions are in a pretty stable shape now. Best Regards Matti Vaittinen
On Tue, 2021-03-30 at 13:11 +0300, Andy Shevchenko wrote: > On Mon, Mar 29, 2021 at 3:58 PM Matti Vaittinen > <matti.vaittinen@fi.rohmeurope.com> wrote: > > > > +struct bd71815_gpio { > > + struct gpio_chip chip; > > + struct device *dev; > > Wondering why you need this. Is it the same as chip.parent? > This is exactly the reason why I had the comments you objected in the probe. dev is pointer to the platform device - which should be used for prints and any potential devm stuff. chip.parent is the MFD device which provides the regmap access and DT node. Best Regards Matti Vaittinen
On Tue, Mar 30, 2021 at 3:06 PM Vaittinen, Matti <Matti.Vaittinen@fi.rohmeurope.com> wrote: > On Tue, 2021-03-30 at 13:11 +0300, Andy Shevchenko wrote: > > On Mon, Mar 29, 2021 at 3:58 PM Matti Vaittinen > > <matti.vaittinen@fi.rohmeurope.com> wrote: > > > > > > +struct bd71815_gpio { > > > + struct gpio_chip chip; > > > + struct device *dev; > > > > Wondering why you need this. Is it the same as chip.parent? > > > > This is exactly the reason why I had the comments you objected in the > probe. dev is pointer to the platform device - which should be used for > prints and any potential devm stuff. > > chip.parent is the MFD device which provides the regmap access and DT > node. We have a kernel doc for such things. If you commented it in the first place around this structure, it will be obvious. Now you have dangling comment somewhere and no clue for reader why you have struct device pointer here. -- With Best Regards, Andy Shevchenko
On Tue, Mar 30, 2021 at 11:06:53AM +0000, Vaittinen, Matti wrote: > Do you think Lee could merge other but the regulator parts to MFD if > Mark is busy? I'd like to be able to squeeze the amount of patches and > recipients for future iterations. It might be easier to work directly > on regulator tree if regulator part gets delayed to next cycle. (I do > also plan further working with the GPIO part during 5.13-rc cycle to > utilize the regmap_gpio. That could be done in the GPIO tree then). I > think the other portions are in a pretty stable shape now. This wouldn't be a bad idea in general for these serieses, especially the bigger ones or the ones that get a lot of review comments on some patches. In any case, here's a pull request for the helpers that are added The following changes since commit 0d02ec6b3136c73c09e7859f0d0e4e2c4c07b49b: Linux 5.12-rc4 (2021-03-21 14:56:43 -0700) are available in the Git repository at: https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git tags/regulator-list-ramp-helpers for you to fetch changes up to fb8fee9efdcf084d9e31ba14cc4734d97e5dd972: regulator: Add regmap helper for ramp-delay setting (2021-04-02 18:33:59 +0100) ---------------------------------------------------------------- regulator: Add a new helper and export an existing one For new drivers. ---------------------------------------------------------------- Matti Vaittinen (2): regulator: helpers: Export helper voltage listing regulator: Add regmap helper for ramp-delay setting drivers/regulator/helpers.c | 101 +++++++++++++++++++++++++++++++++++---- include/linux/regulator/driver.h | 7 +++ 2 files changed, 100 insertions(+), 8 deletions(-)
On Mon, 29 Mar 2021 15:52:38 +0300, Matti Vaittinen wrote: > Patch series introducing support for ROHM BD71815 PMIC > > ROHM BD71815 is a power management IC used in some battery powered > systems. It contains regulators, GPO(s), charger + coulomb counter, RTC > and a clock gate. > > All regulators can be controlled via I2C. LDO4 can additionally be set to > be enabled/disabled by a GPIO. LDO3 voltage could be selected from two > voltages written into separate VSEL reisters using GPIO but this mode is > not supported by driver. On top of that the PMIC has the typical HW > state machine which is present also on many other ROHM PMICs. > > [...] Applied to https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git for-next Thanks! [10/19] regulator: helpers: Export helper voltage listing commit: e3baacf54275647a018ee35bff3bc775a8a2a01a [13/19] regulator: Add regmap helper for ramp-delay setting commit: fb8fee9efdcf084d9e31ba14cc4734d97e5dd972 All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted. You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed. If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced. Please add any relevant lists and maintainers to the CCs when replying to this mail. Thanks, Mark
On Fri, 2021-04-02 at 20:19 +0100, Mark Brown wrote: > On Tue, Mar 30, 2021 at 11:06:53AM +0000, Vaittinen, Matti wrote: > > > Do you think Lee could merge other but the regulator parts to MFD > > if > > Mark is busy? I'd like to be able to squeeze the amount of patches > > and > > recipients for future iterations. It might be easier to work > > directly > > on regulator tree if regulator part gets delayed to next cycle. (I > > do > > also plan further working with the GPIO part during 5.13-rc cycle > > to > > utilize the regmap_gpio. That could be done in the GPIO tree then). > > I > > think the other portions are in a pretty stable shape now. > > This wouldn't be a bad idea in general for these serieses, especially > the bigger ones or the ones that get a lot of review comments on some > patches. > > In any case, here's a pull request for the helpers that are added Thanks Mark. > Matti Vaittinen (2): > regulator: helpers: Export helper voltage listing > regulator: Add regmap helper for ramp-delay setting > If I understand this correctly, the idea is that Lee could pull these changes to his tree? So, I will drop these two patches from the series when I resend it. Helpers are needed for the regulator part of the series to apply. Lee, Mark, please let me know if I misunderstood. Best Regards Matti Vaittinen
On Mon, Apr 05, 2021 at 05:23:41AM +0000, Vaittinen, Matti wrote: > On Fri, 2021-04-02 at 20:19 +0100, Mark Brown wrote: > > Matti Vaittinen (2): > > regulator: helpers: Export helper voltage listing > > regulator: Add regmap helper for ramp-delay setting > If I understand this correctly, the idea is that Lee could pull these > changes to his tree? So, I will drop these two patches from the series > when I resend it. Helpers are needed for the regulator part of the > series to apply. Lee, Mark, please let me know if I misunderstood. Yes.
Patch series introducing support for ROHM BD71815 PMIC ROHM BD71815 is a power management IC used in some battery powered systems. It contains regulators, GPO(s), charger + coulomb counter, RTC and a clock gate. All regulators can be controlled via I2C. LDO4 can additionally be set to be enabled/disabled by a GPIO. LDO3 voltage could be selected from two voltages written into separate VSEL reisters using GPIO but this mode is not supported by driver. On top of that the PMIC has the typical HW state machine which is present also on many other ROHM PMICs. IC contains two GPOs - but one of the GPOs is marked as GND in data-sheet. Thus the driver by default only exposes one GPO. The second GPO can be enabled by special DT property. RTC is almost similar to what is on BD71828. For currently used features only the register address offset to RTC block differs. The charger driver is not included in this series. ROHM has a charger driver with some fuel-gauging logig written in but this is not included here. I am working on separating the logic from HW specific driver and supporting both BD71815 and BD71828 chargers in separate patch series. Changelog v5: Regulator: - Added regmap helper for regulator ramp-delay and taken it in use (patches 13, 14, 16 - they can be just dropped if ramp-delay helper is not a good idea. Patch 15 implements old-fashioned ramp-delay) GPIO: - styling changes to GPIO (Mostly suggested by Andy) - implemented init_valid_mask (but can't count on it yet) Changelog v4: - Sorted ROHM chip ID enum - Statcized DVS structures in regulator driver - Minor styling for regulator driver - rebased on v5.12-rc4 Changelog v3: - GPIO clean-up as suggested by Bartosz - MFD clean-up as suggested by Lee - clk-mode dt-binding handling in MFD driver corrected to reflect new property values. - Dropped already applied patches - Rebased on v5.12-rc2 Changelog v2: - Rebased on top of v5.11-rc3 - Added another "preliminary patch" which fixes HW-dvs voltage handling (patch 1) - split regulator patch to two. - changed dt-binding patch ordering. regulators: - staticized probe - removed some unnecessary defines - updated comments - split rohm-regulator patch adding SNVS and supporting simple linear mapping into two - one adding support for mapping, other adding SNVS. GPIO: - removed unnecessary headers - clarified dev/parent->dev usage - removed forgotten #define DEBUG dt-bindings: - changed patch order to meet ref-dependencies - added missing regulator nodes - changed string property for clk mode to tristated MFD: - header cleanups. CLK: - fixed commit message -- Matti Vaittinen (19): rtc: bd70528: Do not require parent data mfd: bd718x7: simplify by cleaning unnecessary device data dt_bindings: bd71828: Add clock output mode dt_bindings: regulator: Add ROHM BD71815 PMIC regulators dt_bindings: mfd: Add ROHM BD71815 PMIC mfd: Add ROHM BD71815 ID mfd: Sort ROHM chip ID list for better readability mfd: Support for ROHM BD71815 PMIC core gpio: support ROHM BD71815 GPOs regulator: helpers: Export helper voltage listing regulator: rohm-regulator: linear voltage support regulator: rohm-regulator: Support SNVS HW state. regulator: Add regmap helper for ramp-delay setting regulator: bd718x7, bd71828: Use ramp-delay helper regulator: Support ROHM BD71815 regulators regulator: bd71815: use ramp-delay helper clk: bd718x7: Add support for clk gate on ROHM BD71815 PMIC rtc: bd70528: Support RTC on ROHM BD71815 MAINTAINERS: Add ROHM BD71815AGW .../bindings/mfd/rohm,bd71815-pmic.yaml | 201 ++++++ .../bindings/mfd/rohm,bd71828-pmic.yaml | 6 + .../regulator/rohm,bd71815-regulator.yaml | 116 ++++ MAINTAINERS | 3 + drivers/clk/clk-bd718x7.c | 9 +- drivers/gpio/Kconfig | 10 + drivers/gpio/Makefile | 1 + drivers/gpio/gpio-bd71815.c | 193 ++++++ drivers/mfd/Kconfig | 15 +- drivers/mfd/rohm-bd71828.c | 486 +++++++++---- drivers/mfd/rohm-bd718x7.c | 43 +- drivers/regulator/Kconfig | 11 + drivers/regulator/Makefile | 1 + drivers/regulator/bd71815-regulator.c | 651 ++++++++++++++++++ drivers/regulator/bd71828-regulator.c | 51 +- drivers/regulator/bd718x7-regulator.c | 60 +- drivers/regulator/helpers.c | 101 ++- drivers/regulator/rohm-regulator.c | 23 +- drivers/rtc/Kconfig | 6 +- drivers/rtc/rtc-bd70528.c | 104 +-- include/linux/mfd/rohm-bd71815.h | 562 +++++++++++++++ include/linux/mfd/rohm-bd71828.h | 3 + include/linux/mfd/rohm-bd718x7.h | 13 - include/linux/mfd/rohm-generic.h | 15 +- include/linux/regulator/driver.h | 7 + 25 files changed, 2393 insertions(+), 298 deletions(-) create mode 100644 Documentation/devicetree/bindings/mfd/rohm,bd71815-pmic.yaml create mode 100644 Documentation/devicetree/bindings/regulator/rohm,bd71815-regulator.yaml create mode 100644 drivers/gpio/gpio-bd71815.c create mode 100644 drivers/regulator/bd71815-regulator.c create mode 100644 include/linux/mfd/rohm-bd71815.h base-commit: 0d02ec6b3136c73c09e7859f0d0e4e2c4c07b49b