| Message ID | cover.1582617178.git.matti.vaittinen@fi.rohmeurope.com |
|---|---|
| Headers | show |
| Series | Support ROHM BD99954 charger IC | expand |
On 2/25/20 12:55 AM, Matti Vaittinen wrote: > diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig > index 8781c674ed07..0b3bad6fc736 100644 > --- a/drivers/power/supply/Kconfig > +++ b/drivers/power/supply/Kconfig > @@ -702,6 +702,16 @@ config CHARGER_BD70528 > information and altering charger configurations from charger > block of the ROHM BD70528 Power Management IC. > Hi, > +config CHARGER_BD99954 > + tristate "ROHM bd99954 charger driver" > + depends on I2C > + select LINEAR_RANGES > + default n Drop the "default n", since it is already the default. > + help > + Say Y here to enable support for getting battery and charger > + information and altering charger configurations from the ROHM > + BD99954 charger IC. Please indent the 3 lines of help text with one additional space (2 total). See Documentation/process/coding-style.rst: 10) Kconfig configuration files ------------------------------- For all of the Kconfig* configuration files throughout the source tree, the indentation is somewhat different. Lines under a ``config`` definition are indented with one tab, while help text is indented an additional two spaces. Example:: config AUDIT bool "Auditing support" depends on NET help Enable auditing infrastructure that can be used with another kernel subsystem, such as SELinux (which requires this for logging of avc messages output). Does not do system-call auditing without CONFIG_AUDITSYSCALL. > + > config CHARGER_WILCO > tristate "Wilco EC based charger for ChromeOS" > depends on WILCO_EC thanks.
Hello Randy, On Tue, 2020-02-25 at 08:21 -0800, Randy Dunlap wrote: > On 2/25/20 12:55 AM, Matti Vaittinen wrote: > > diff --git a/drivers/power/supply/Kconfig > > b/drivers/power/supply/Kconfig > > index 8781c674ed07..0b3bad6fc736 100644 > > --- a/drivers/power/supply/Kconfig > > +++ b/drivers/power/supply/Kconfig > > @@ -702,6 +702,16 @@ config CHARGER_BD70528 > > information and altering charger configurations from charger > > block of the ROHM BD70528 Power Management IC. > > > > Hi, > > > +config CHARGER_BD99954 > > + tristate "ROHM bd99954 charger driver" > > + depends on I2C > > + select LINEAR_RANGES > > + default n > > Drop the "default n", since it is already the default. > > > + help > > + Say Y here to enable support for getting battery and charger > > + information and altering charger configurations from the ROHM > > + BD99954 charger IC. > > Please indent the 3 lines of help text with one additional space (2 > total). > See Documentation/process/coding-style.rst: > > 10) Kconfig configuration files > ------------------------------- > > For all of the Kconfig* configuration files throughout the source > tree, > the indentation is somewhat different. Lines under a ``config`` > definition > are indented with one tab, while help text is indented an additional > two > spaces. Example:: > > config AUDIT > bool "Auditing support" > depends on NET > help > Enable auditing infrastructure that can be used with another > kernel subsystem, such as SELinux (which requires this for > logging of avc messages output). Does not do system-call > auditing without CONFIG_AUDITSYSCALL. > > > + > > config CHARGER_WILCO > > tristate "Wilco EC based charger for ChromeOS" > > depends on WILCO_EC > > thanks. Thanks again for the review. I'll fix these for the next version :) Br, Matti Vaittinen
On Tue, Feb 25, 2020 at 10:53:01AM +0200, Matti Vaittinen wrote: > Many devices have control registers which control some measurable > property. Often a register contains control field so that change in > this field causes linear change in the controlled property. It is not > a rare case that user wants to give 'meaningful' control values and > driver needs to convert them to register field values. Even more > often user wants to 'see' the currently set value - again in > meaningful units - and driver needs to convert the values it reads > from register to these meaningful units. Examples of this include: > > - regulators, voltage/current configurations > - power, voltage/current configurations > - clk(?) NCOs > > and maybe others I can't think of right now. > > Provide a linear_range helper which can do conversion from user value > to register value 'selector'. > > The idea here is stolen from regulator framework and patches refactoring > the regulator helpers to use this are following. > > Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> > --- > > Changes since rfc-v3: > - Kerneldoc fixes > - Corrected commit message typo meaningfull => meaningful > > drivers/base/Kconfig | 3 + > drivers/base/Makefile | 1 + > drivers/base/linear_ranges.c | 246 +++++++++++++++++++++++++++++++++++ Why in drivers/base/ ? Why not in lib/ ? > include/linux/linear_range.h | 48 +++++++ > 4 files changed, 298 insertions(+) > create mode 100644 drivers/base/linear_ranges.c > create mode 100644 include/linux/linear_range.h > > diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig > index 5f0bc74d2409..636b6fa8e499 100644 > --- a/drivers/base/Kconfig > +++ b/drivers/base/Kconfig > @@ -209,4 +209,7 @@ config GENERIC_ARCH_TOPOLOGY > appropriate scaling, sysfs interface for reading capacity values at > runtime. > > +config LINEAR_RANGES > + tristate No help text at all??? > +EXPORT_SYMBOL(linear_range_values_in_range); EXPORT_SYMBOL_GPL() for all of these? I have to ask... > @@ -0,0 +1,48 @@ > +/* SPDX-License-Identifier: GPL-2.0-or-later */ Are you sure about the "or later"? Again, I have to ask. > +/* Copyright (C) 2020 ROHM Semiconductors */ > + > +#ifndef LINEAR_RANGE_H > +#define LINEAR_RANGE_H > + > +#include <linux/types.h> Why is this needed? thanks, greg k-h
Hello Greg, On Wed, 2020-03-18 at 14:08 +0100, Greg Kroah-Hartman wrote: > On Tue, Feb 25, 2020 at 10:53:01AM +0200, Matti Vaittinen wrote: > > Many devices have control registers which control some measurable > > property. Often a register contains control field so that change in > > this field causes linear change in the controlled property. It is > > not > > a rare case that user wants to give 'meaningful' control values and > > driver needs to convert them to register field values. Even more > > often user wants to 'see' the currently set value - again in > > meaningful units - and driver needs to convert the values it reads > > from register to these meaningful units. Examples of this include: > > > > - regulators, voltage/current configurations > > - power, voltage/current configurations > > - clk(?) NCOs > > > > and maybe others I can't think of right now. > > > > Provide a linear_range helper which can do conversion from user > > value > > to register value 'selector'. > > > > The idea here is stolen from regulator framework and patches > > refactoring > > the regulator helpers to use this are following. > > > > Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> > > --- > > > > Changes since rfc-v3: > > - Kerneldoc fixes > > - Corrected commit message typo meaningfull => meaningful > > > > drivers/base/Kconfig | 3 + > > drivers/base/Makefile | 1 + > > drivers/base/linear_ranges.c | 246 > > +++++++++++++++++++++++++++++++++++ > > Why in drivers/base/ ? > > Why not in lib/ ? I was pondering which of these would be better. I decided to do with drivers/base because - in it's current form - this is really a driver related stuff. I see it somehow in same position as regmap code - although this is just a tiny helper compared to regmap. But this also has pretty driver specific audience :) And... I must admit I like things which I know. And I have been doing driver development and "know" a few of the driver related colleagues - hence working with them is easier for me ;) Getting to know the colleagues maintaining lib is a bit scary :] Yep, I'm Finnish if you happen to wonder why getting to know people is scary xD > > > include/linux/linear_range.h | 48 +++++++ > > 4 files changed, 298 insertions(+) > > create mode 100644 drivers/base/linear_ranges.c > > create mode 100644 include/linux/linear_range.h > > > > diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig > > index 5f0bc74d2409..636b6fa8e499 100644 > > --- a/drivers/base/Kconfig > > +++ b/drivers/base/Kconfig > > @@ -209,4 +209,7 @@ config GENERIC_ARCH_TOPOLOGY > > appropriate scaling, sysfs interface for reading capacity > > values at > > runtime. > > > > +config LINEAR_RANGES > > + tristate > > No help text at all??? Yes. The linear ranges has no meaning to be enabled alone. It only plays a role if it is used by some driver/subsystem. And drivers/subsystems should do select LINEAR_RANGES. So showing help in any config tool is not needed. This should actually not be visible in menuconfig or others. I think I have seen a few examples like this. Ayways, I have no obejctions to adding some text if absolutely needed. Any suggestions for a text politely saying - "please, pretend I am not here" - are welcome :) (Although, I think this really does not need help text). > > > +EXPORT_SYMBOL(linear_range_values_in_range); > > EXPORT_SYMBOL_GPL() for all of these? I have to ask... I personally have no objections towards EXPORT_SYMBOL_GPL. I guess regulator helpers and the power supply modules which use this are GPL modules. If no other (better) opinions, then I can change this in next version. Thanks for pointing it out - I didn't even think about it. > > > @@ -0,0 +1,48 @@ > > +/* SPDX-License-Identifier: GPL-2.0-or-later */ > > Are you sure about the "or later"? Again, I have to ask. No. If you want to educate me on this topic - or point a link to some nice explanation why to not use "later" - pretty please. I would like to learn :) > > > +/* Copyright (C) 2020 ROHM Semiconductors */ > > + > > +#ifndef LINEAR_RANGE_H > > +#define LINEAR_RANGE_H > > + > > +#include <linux/types.h> > > Why is this needed? I think it was the bool type which was missing without this. At least on my ARM gcc toolchain. Best Regards Matti Vaittinen
On Wed, Mar 18, 2020 at 01:42:26PM +0000, Vaittinen, Matti wrote: > Hello Greg, > > On Wed, 2020-03-18 at 14:08 +0100, Greg Kroah-Hartman wrote: > > On Tue, Feb 25, 2020 at 10:53:01AM +0200, Matti Vaittinen wrote: > > > Many devices have control registers which control some measurable > > > property. Often a register contains control field so that change in > > > this field causes linear change in the controlled property. It is > > > not > > > a rare case that user wants to give 'meaningful' control values and > > > driver needs to convert them to register field values. Even more > > > often user wants to 'see' the currently set value - again in > > > meaningful units - and driver needs to convert the values it reads > > > from register to these meaningful units. Examples of this include: > > > > > > - regulators, voltage/current configurations > > > - power, voltage/current configurations > > > - clk(?) NCOs > > > > > > and maybe others I can't think of right now. > > > > > > Provide a linear_range helper which can do conversion from user > > > value > > > to register value 'selector'. > > > > > > The idea here is stolen from regulator framework and patches > > > refactoring > > > the regulator helpers to use this are following. > > > > > > Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> > > > --- > > > > > > Changes since rfc-v3: > > > - Kerneldoc fixes > > > - Corrected commit message typo meaningfull => meaningful > > > > > > drivers/base/Kconfig | 3 + > > > drivers/base/Makefile | 1 + > > > drivers/base/linear_ranges.c | 246 > > > +++++++++++++++++++++++++++++++++++ > > > > Why in drivers/base/ ? > > > > Why not in lib/ ? > > I was pondering which of these would be better. I decided to do with > drivers/base because - in it's current form - this is really a driver > related stuff. I see it somehow in same position as regmap code - > although this is just a tiny helper compared to regmap. But this also > has pretty driver specific audience :) > > And... I must admit I like things which I know. And I have been doing > driver development and "know" a few of the driver related colleagues - > hence working with them is easier for me ;) Getting to know the > colleagues maintaining lib is a bit scary :] Yep, I'm Finnish if you > happen to wonder why getting to know people is scary xD > > > > > > include/linux/linear_range.h | 48 +++++++ > > > 4 files changed, 298 insertions(+) > > > create mode 100644 drivers/base/linear_ranges.c > > > create mode 100644 include/linux/linear_range.h > > > > > > diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig > > > index 5f0bc74d2409..636b6fa8e499 100644 > > > --- a/drivers/base/Kconfig > > > +++ b/drivers/base/Kconfig > > > @@ -209,4 +209,7 @@ config GENERIC_ARCH_TOPOLOGY > > > appropriate scaling, sysfs interface for reading capacity > > > values at > > > runtime. > > > > > > +config LINEAR_RANGES > > > + tristate > > > > No help text at all??? > > Yes. The linear ranges has no meaning to be enabled alone. It only > plays a role if it is used by some driver/subsystem. And > drivers/subsystems should do > select LINEAR_RANGES. So showing help in any config tool is not needed. > This should actually not be visible in menuconfig or others. I think I > have seen a few examples like this. > > Ayways, I have no obejctions to adding some text if absolutely needed. > Any suggestions for a text politely saying - "please, pretend I am not > here" - are welcome :) (Although, I think this really does not need > help text). This kind of implies it needs to be in lib/ that way the needed code links it and all should be fine. thanks, greg k-h
Hello Greg, On Wed, 2020-03-18 at 14:50 +0100, gregkh@linuxfoundation.org wrote: > On Wed, Mar 18, 2020 at 01:42:26PM +0000, Vaittinen, Matti wrote: > > Hello Greg, > > > > On Wed, 2020-03-18 at 14:08 +0100, Greg Kroah-Hartman wrote: > > > On Tue, Feb 25, 2020 at 10:53:01AM +0200, Matti Vaittinen wrote: > > > > Many devices have control registers which control some > > > > measurable > > > > property. Often a register contains control field so that > > > > change in > > > > this field causes linear change in the controlled property. It > > > > is > > > > not > > > > a rare case that user wants to give 'meaningful' control values > > > > and > > > > driver needs to convert them to register field values. Even > > > > more > > > > often user wants to 'see' the currently set value - again in > > > > meaningful units - and driver needs to convert the values it > > > > reads > > > > from register to these meaningful units. Examples of this > > > > include: > > > > > > > > - regulators, voltage/current configurations > > > > - power, voltage/current configurations > > > > - clk(?) NCOs > > > > > > > > and maybe others I can't think of right now. > > > > > > > > Provide a linear_range helper which can do conversion from user > > > > value > > > > to register value 'selector'. > > > > > > > > The idea here is stolen from regulator framework and patches > > > > refactoring > > > > the regulator helpers to use this are following. > > > > > > > > Signed-off-by: Matti Vaittinen < > > > > matti.vaittinen@fi.rohmeurope.com> > > > > --- > > > > > > > > Changes since rfc-v3: > > > > - Kerneldoc fixes > > > > - Corrected commit message typo meaningfull => meaningful > > > > > > > > drivers/base/Kconfig | 3 + > > > > drivers/base/Makefile | 1 + > > > > drivers/base/linear_ranges.c | 246 > > > > +++++++++++++++++++++++++++++++++++ > > > > > > Why in drivers/base/ ? > > > > > > Why not in lib/ ? > > > > I was pondering which of these would be better. I decided to do > > with > > drivers/base because - in it's current form - this is really a > > driver > > related stuff. I see it somehow in same position as regmap code - > > although this is just a tiny helper compared to regmap. But this > > also > > has pretty driver specific audience :) > > > > And... I must admit I like things which I know. And I have been > > doing > > driver development and "know" a few of the driver related > > colleagues - > > hence working with them is easier for me ;) Getting to know the > > colleagues maintaining lib is a bit scary :] Yep, I'm Finnish if > > you > > happen to wonder why getting to know people is scary xD > > > > > > include/linux/linear_range.h | 48 +++++++ > > > > 4 files changed, 298 insertions(+) > > > > create mode 100644 drivers/base/linear_ranges.c > > > > create mode 100644 include/linux/linear_range.h > > > > > > > > diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig > > > > index 5f0bc74d2409..636b6fa8e499 100644 > > > > --- a/drivers/base/Kconfig > > > > +++ b/drivers/base/Kconfig > > > > @@ -209,4 +209,7 @@ config GENERIC_ARCH_TOPOLOGY > > > > appropriate scaling, sysfs interface for reading > > > > capacity > > > > values at > > > > runtime. > > > > > > > > +config LINEAR_RANGES > > > > + tristate > > > > > > No help text at all??? > > > > Yes. The linear ranges has no meaning to be enabled alone. It only > > plays a role if it is used by some driver/subsystem. And > > drivers/subsystems should do > > select LINEAR_RANGES. So showing help in any config tool is not > > needed. > > This should actually not be visible in menuconfig or others. I > > think I > > have seen a few examples like this. > > > > Ayways, I have no obejctions to adding some text if absolutely > > needed. > > Any suggestions for a text politely saying - "please, pretend I am > > not > > here" - are welcome :) (Although, I think this really does not need > > help text). > > This kind of implies it needs to be in lib/ that way the needed code > links it and all should be fine. Sigh. I somehow guessed this was coming... Well, I had to try anyways :) Please just ignore v5 - I did send it prior receiving your comments. I am about to send v6 which will contain the changes you suggested. Let's see how people who look things that go under lib/ will see this. Br, Matti Vaittinen
Support ROHM BD99954 Battery Management IC ROHM BD99954 is a Battery Management IC for 1-4 cell Lithium-Ion secondary battery. BD99954 is intended to be used in space-constraint equipment such as Low profile Notebook PC, Tablets and other applications. Series extracts a "linear ranges" helper out of the regulator framework. Linear ranges helper is intended to help converting real-world values to register values when conversion is linear. I suspect this is useful also for power subsystem and possibly for clk. This version of series introduces new battry DT binding entries and adds the parsing in power_supply_get_battery_info(). Please note that regulator drivers have been compile tested (but not linked) after patch 5. Please also note that only the BD71847 based regulator testing has been performed after the patch 6 - all testing is highly appreciated. Changelog v4: generic: - rebase and drop RFC. DT-bindings: - add I2C node address-cells and size-cells to fix yaml check errors - uncomment multipleOf: bd70528: - add patch which renames driver internal linear_range struct to avoid collision when regulator/driver.h (which gets included from rohm generic header) introduces the linear_range struct. regulators: - rebase to v5.6-rc2 and convert also the two newly introduced drivers to use linear_range struct instead of regulator_linear_range. linear_ranges: - Fix kerneldoc. Changelog RFC-v3: DT-bindings: - fix the BD99954 binding (the *-microvolt Vs. *-microvolts issue is still there. Not sure which one is correct) - renabe tricklecharge-* binding to trickle-charge-* as suggested by Rob. - drop the linear-ranges helper which was written for BD70528 and extract the linear-range code from regulator framework instead. - refactor regulator framework to utilize extracted linear-ranges code. - change the struct regulator_linear_range to linear_range from regulator drivers. - refactor BD70528 to use regulator framework originated linear-ranges code. - change BD99954 to use linear-ranges code from regulator framework Changelog RFC-v2: DT-bindings: - Used the battery parameters described in battery.txt - Added few new parameters to battery.txt - Added ASCII art charging profile chart for BD99954 to explain states and limits. Linear ranges: - Fixed division by zero error from linear-ranges code if step 0 is used. Power-supply core: - Added parsing of new battery parameters. BD99954 driver: - converted to use battery parameters from battery node - Added step 0 ranges for reg values which do not change voltage - added dt-node to psy-config Patch 1: DT binding docs for the new battery parameters Patch 2: BD99954 charger DT binding docs Patch 3: Linear ranges helpers Patch 4: Rename driver internal struct linear_range from bd70528-power Patch 5: Rename struct regulator_linear_range to struct linear_range and convert regulator drivers to use renamed struct Patch 6: Use linear-ranges helpers in regulator framework Patch 7: Use linear-ranges helpers in bd70528 driver Patch 8: Parsing of new battery parameters Patch 9: ROHM BD99954 charger IC driver --- Matti Vaittinen (9): dt-bindings: battery: add new battery parameters dt_bindings: ROHM BD99954 Charger drivers: base: add linear ranges helpers power: supply: bd70528: rename linear_range to avoid collision regulator: rename regulator_linear_range to linear_range regulator: use linear_ranges helper power: supply: bd70528: use linear ranges power: supply: add battery parameters power: supply: Support ROHM bd99954 charger .../bindings/power/supply/battery.txt | 6 + .../bindings/power/supply/rohm,bd9995x.yaml | 155 +++ drivers/base/Kconfig | 3 + drivers/base/Makefile | 1 + drivers/base/linear_ranges.c | 246 ++++ drivers/power/supply/Kconfig | 11 + drivers/power/supply/Makefile | 1 + drivers/power/supply/bd70528-charger.c | 140 +- drivers/power/supply/bd99954-charger.c | 1171 +++++++++++++++++ drivers/power/supply/power_supply_core.c | 8 + drivers/regulator/88pg86x.c | 4 +- drivers/regulator/88pm800-regulator.c | 4 +- drivers/regulator/Kconfig | 1 + drivers/regulator/act8865-regulator.c | 4 +- drivers/regulator/act8945a-regulator.c | 2 +- drivers/regulator/arizona-ldo1.c | 2 +- drivers/regulator/arizona-micsupp.c | 4 +- drivers/regulator/as3711-regulator.c | 6 +- drivers/regulator/as3722-regulator.c | 4 +- drivers/regulator/axp20x-regulator.c | 16 +- drivers/regulator/bcm590xx-regulator.c | 8 +- drivers/regulator/bd70528-regulator.c | 8 +- drivers/regulator/bd71828-regulator.c | 10 +- drivers/regulator/bd718x7-regulator.c | 26 +- drivers/regulator/da903x.c | 2 +- drivers/regulator/helpers.c | 130 +- drivers/regulator/hi6421-regulator.c | 4 +- drivers/regulator/lochnagar-regulator.c | 4 +- drivers/regulator/lp873x-regulator.c | 4 +- drivers/regulator/lp87565-regulator.c | 2 +- drivers/regulator/lp8788-buck.c | 2 +- drivers/regulator/max77650-regulator.c | 2 +- drivers/regulator/mcp16502.c | 4 +- drivers/regulator/mp8859.c | 2 +- drivers/regulator/mt6323-regulator.c | 6 +- drivers/regulator/mt6358-regulator.c | 8 +- drivers/regulator/mt6380-regulator.c | 6 +- drivers/regulator/mt6397-regulator.c | 6 +- drivers/regulator/palmas-regulator.c | 4 +- drivers/regulator/qcom-rpmh-regulator.c | 2 +- drivers/regulator/qcom_rpm-regulator.c | 14 +- drivers/regulator/qcom_smd-regulator.c | 70 +- drivers/regulator/rk808-regulator.c | 10 +- drivers/regulator/s2mps11.c | 14 +- drivers/regulator/sky81452-regulator.c | 2 +- drivers/regulator/stpmic1_regulator.c | 18 +- drivers/regulator/tps65086-regulator.c | 10 +- drivers/regulator/tps65217-regulator.c | 4 +- drivers/regulator/tps65218-regulator.c | 6 +- drivers/regulator/tps65912-regulator.c | 4 +- drivers/regulator/twl-regulator.c | 4 +- drivers/regulator/twl6030-regulator.c | 2 +- drivers/regulator/wm831x-dcdc.c | 2 +- drivers/regulator/wm831x-ldo.c | 4 +- drivers/regulator/wm8350-regulator.c | 2 +- drivers/regulator/wm8400-regulator.c | 2 +- include/linux/linear_range.h | 48 + include/linux/power/bd99954-charger.h | 1075 +++++++++++++++ include/linux/power_supply.h | 4 + include/linux/regulator/driver.h | 27 +- 60 files changed, 3007 insertions(+), 344 deletions(-) create mode 100644 Documentation/devicetree/bindings/power/supply/rohm,bd9995x.yaml create mode 100644 drivers/base/linear_ranges.c create mode 100644 drivers/power/supply/bd99954-charger.c create mode 100644 include/linux/linear_range.h create mode 100644 include/linux/power/bd99954-charger.h base-commit 11a48a5a18c6 ("Linux 5.6-rc2")