diff mbox series

[v9,05/10] power: supply: bd70528: use linear ranges

Message ID 8dcf498134adb09d88bd16138cdce65e9ffa0bb6.1586849281.git.matti.vaittinen@fi.rohmeurope.com (mailing list archive)
State New, archived
Headers show
Series Support ROHM BD99954 charger IC | expand

Commit Message

Vaittinen, Matti April 14, 2020, 8:04 a.m. UTC
Change the bd70528 to use common linear_range code instead of
implementing a copy of it in this driver.

Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.com>
---

Changes since v8 - rebased and reordered series.

 drivers/power/supply/Kconfig           |   1 +
 drivers/power/supply/bd70528-charger.c | 144 ++++++++++---------------
 2 files changed, 56 insertions(+), 89 deletions(-)

Comments

Andy Shevchenko April 14, 2020, 9:32 a.m. UTC | #1
On Tue, Apr 14, 2020 at 11:04:21AM +0300, Matti Vaittinen wrote:
> Change the bd70528 to use common linear_range code instead of
> implementing a copy of it in this driver.

Couple of nits below which you can take as TODO items for the future.
(Because maintainer is fine with it according to the tags)

...

>  config CHARGER_BD70528
>  	tristate "ROHM bd70528 charger driver"
>  	depends on MFD_ROHM_BD70528
> +	select LINEAR_RANGES

>  	default n

At some point you can remove this kind of defaults (see [1] for the details).

...

> +static const struct linear_range current_limit_ranges[] = {
>  	{
>  		.min = 5,
>  		.step = 1,
> -		.vals = 36,
> -		.low_sel = 0,

> +		.min_sel = 0,

Perhaps it's better to have it aligned with max_sel, i.e. be 0x00.
Same applies to the rest of a such.

> +		.max_sel = 0x22,
>  	},

...

> +static const struct linear_range warm_charge_curr[] = {
>  	{
>  		.min = 10,
>  		.step = 10,
> -		.vals = 20,
> -		.low_sel = 0,
> +		.min_sel = 0,
> +		.max_sel = 0x12

Perhaps leaving comma is a good thing to avoid potential churn in the future
(if any of fields will be added here). Same applies to the reset of a such.

>  	},
Andy Shevchenko April 14, 2020, 9:35 a.m. UTC | #2
On Tue, Apr 14, 2020 at 12:32:43PM +0300, Andy Shevchenko wrote:
> On Tue, Apr 14, 2020 at 11:04:21AM +0300, Matti Vaittinen wrote:
> > Change the bd70528 to use common linear_range code instead of
> > implementing a copy of it in this driver.
> 
> Couple of nits below which you can take as TODO items for the future.
> (Because maintainer is fine with it according to the tags)
> 
> ...
> 
> >  config CHARGER_BD70528
> >  	tristate "ROHM bd70528 charger driver"
> >  	depends on MFD_ROHM_BD70528
> > +	select LINEAR_RANGES
> 
> >  	default n
> 
> At some point you can remove this kind of defaults (see [1] for the details).

Missed reference

[1]: commit 0192f17529fa ("clean up x86 platform driver default values")

> ...
> 
> > +static const struct linear_range current_limit_ranges[] = {
> >  	{
> >  		.min = 5,
> >  		.step = 1,
> > -		.vals = 36,
> > -		.low_sel = 0,
> 
> > +		.min_sel = 0,
> 
> Perhaps it's better to have it aligned with max_sel, i.e. be 0x00.
> Same applies to the rest of a such.
> 
> > +		.max_sel = 0x22,
> >  	},
> 
> ...
> 
> > +static const struct linear_range warm_charge_curr[] = {
> >  	{
> >  		.min = 10,
> >  		.step = 10,
> > -		.vals = 20,
> > -		.low_sel = 0,
> > +		.min_sel = 0,
> > +		.max_sel = 0x12
> 
> Perhaps leaving comma is a good thing to avoid potential churn in the future
> (if any of fields will be added here). Same applies to the reset of a such.
> 
> >  	},
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
>
Vaittinen, Matti April 14, 2020, 10:11 a.m. UTC | #3
Hello Andy,

I do agree with everything except perhaps the necessity of leading zero
in .min_sel initialization :) And I will fix these if I need to change
something else in these files. Thanks.

Best Regards,
    --Matti

On Tue, 2020-04-14 at 12:32 +0300, Andy Shevchenko wrote:
> On Tue, Apr 14, 2020 at 11:04:21AM +0300, Matti Vaittinen wrote:
> > Change the bd70528 to use common linear_range code instead of
> > implementing a copy of it in this driver.
> 
> Couple of nits below which you can take as TODO items for the future.
> (Because maintainer is fine with it according to the tags)
> 
> ...
> 
> >  config CHARGER_BD70528
> >  	tristate "ROHM bd70528 charger driver"
> >  	depends on MFD_ROHM_BD70528
> > +	select LINEAR_RANGES
> >  	default n
> 
> At some point you can remove this kind of defaults (see [1] for the
> details).
> 
> ...
> 
> > +static const struct linear_range current_limit_ranges[] = {
> >  	{
> >  		.min = 5,
> >  		.step = 1,
> > -		.vals = 36,
> > -		.low_sel = 0,
> > +		.min_sel = 0,
> 
> Perhaps it's better to have it aligned with max_sel, i.e. be 0x00.
> Same applies to the rest of a such.
> 
> > +		.max_sel = 0x22,
> >  	},
> 
> ...
> 
> > +static const struct linear_range warm_charge_curr[] = {
> >  	{
> >  		.min = 10,
> >  		.step = 10,
> > -		.vals = 20,
> > -		.low_sel = 0,
> > +		.min_sel = 0,
> > +		.max_sel = 0x12
> 
> Perhaps leaving comma is a good thing to avoid potential churn in the
> future
> (if any of fields will be added here). Same applies to the reset of a
> such.
> 
> >  	},
diff mbox series

Patch

diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig
index f3424fdce341..9f19636db922 100644
--- a/drivers/power/supply/Kconfig
+++ b/drivers/power/supply/Kconfig
@@ -695,6 +695,7 @@  config CHARGER_UCS1002
 config CHARGER_BD70528
 	tristate "ROHM bd70528 charger driver"
 	depends on MFD_ROHM_BD70528
+	select LINEAR_RANGES
 	default n
 	help
 	 Say Y here to enable support for getting battery status
diff --git a/drivers/power/supply/bd70528-charger.c b/drivers/power/supply/bd70528-charger.c
index 3b820110ecfa..7a805faeee83 100644
--- a/drivers/power/supply/bd70528-charger.c
+++ b/drivers/power/supply/bd70528-charger.c
@@ -72,6 +72,7 @@ 
 #include <linux/module.h>
 #include <linux/platform_device.h>
 #include <linux/power_supply.h>
+#include <linux/linear_range.h>
 
 #define CHG_STAT_SUSPEND	0x0
 #define CHG_STAT_TRICKLE	0x1
@@ -335,38 +336,37 @@  static int bd70528_get_present(struct bd70528_psy *bdpsy, int *val)
 	return 0;
 }
 
-struct bd70528_linear_range {
-	int min;
-	int step;
-	int vals;
-	int low_sel;
-};
-
-static const struct bd70528_linear_range current_limit_ranges[] = {
+static const struct linear_range current_limit_ranges[] = {
 	{
 		.min = 5,
 		.step = 1,
-		.vals = 36,
-		.low_sel = 0,
+		.min_sel = 0,
+		.max_sel = 0x22,
 	},
 	{
 		.min = 40,
 		.step = 5,
-		.vals = 5,
-		.low_sel = 0x23,
+		.min_sel = 0x23,
+		.max_sel = 0x26,
 	},
 	{
 		.min = 60,
 		.step = 20,
-		.vals = 8,
-		.low_sel = 0x27,
+		.min_sel = 0x27,
+		.max_sel = 0x2d,
 	},
 	{
 		.min = 200,
 		.step = 50,
-		.vals = 7,
-		.low_sel = 0x2e,
-	}
+		.min_sel = 0x2e,
+		.max_sel = 0x34,
+	},
+	{
+		.min = 500,
+		.step = 0,
+		.min_sel = 0x35,
+		.max_sel = 0x3f,
+	},
 };
 
 /*
@@ -374,18 +374,18 @@  static const struct bd70528_linear_range current_limit_ranges[] = {
  * voltage for low temperatures. The driver currently only reads
  * the charge current at room temperature. We do set both though.
  */
-static const struct bd70528_linear_range warm_charge_curr[] = {
+static const struct linear_range warm_charge_curr[] = {
 	{
 		.min = 10,
 		.step = 10,
-		.vals = 20,
-		.low_sel = 0,
+		.min_sel = 0,
+		.max_sel = 0x12
 	},
 	{
 		.min = 200,
 		.step = 25,
-		.vals = 13,
-		.low_sel = 0x13,
+		.min_sel = 0x13,
+		.max_sel = 0x1f
 	},
 };
 
@@ -398,56 +398,6 @@  static const struct bd70528_linear_range warm_charge_curr[] = {
 #define MAX_WARM_CHG_CURR_SEL 0x1f
 #define MIN_CHG_CURR_SEL 0x0
 
-static int find_value_for_selector_low(const struct bd70528_linear_range *r,
-				       int selectors, unsigned int sel,
-				       unsigned int *val)
-{
-	int i;
-
-	for (i = 0; i < selectors; i++) {
-		if (r[i].low_sel <= sel && r[i].low_sel + r[i].vals >= sel) {
-			*val = r[i].min + (sel - r[i].low_sel) * r[i].step;
-			return 0;
-		}
-	}
-	return -EINVAL;
-}
-
-/*
- * For BD70528 voltage/current limits we happily accept any value which
- * belongs the range. We could check if value matching the selector is
- * desired by computing the range min + (sel - sel_low) * range step - but
- * I guess it is enough if we use voltage/current which is closest (below)
- * the requested?
- */
-static int find_selector_for_value_low(const struct bd70528_linear_range *r,
-				       int selectors, unsigned int val,
-				       unsigned int *sel, bool *found)
-{
-	int i;
-	int ret = -EINVAL;
-
-	*found = false;
-	for (i = 0; i < selectors; i++) {
-		if (r[i].min <= val) {
-			if (r[i].min + r[i].step * r[i].vals >= val) {
-				*found = true;
-				*sel = r[i].low_sel + (val - r[i].min) /
-				       r[i].step;
-				ret = 0;
-				break;
-			}
-			/*
-			 * If the range max is smaller than requested
-			 * we can set the max supported value from range
-			 */
-			*sel = r[i].low_sel + r[i].vals;
-			ret = 0;
-		}
-	}
-	return ret;
-}
-
 static int get_charge_current(struct bd70528_psy *bdpsy, int *ma)
 {
 	unsigned int sel;
@@ -463,9 +413,9 @@  static int get_charge_current(struct bd70528_psy *bdpsy, int *ma)
 
 	sel &= BD70528_MASK_CHG_CHG_CURR;
 
-	ret = find_value_for_selector_low(&warm_charge_curr[0],
-					  ARRAY_SIZE(warm_charge_curr), sel,
-					  ma);
+	ret = linear_range_get_value_array(&warm_charge_curr[0],
+					   ARRAY_SIZE(warm_charge_curr),
+					   sel, ma);
 	if (ret) {
 		dev_err(bdpsy->dev,
 			"Unknown charge current value 0x%x\n",
@@ -491,10 +441,9 @@  static int get_current_limit(struct bd70528_psy *bdpsy, int *ma)
 
 	sel &= BD70528_MASK_CHG_DCIN_ILIM;
 
-	ret = find_value_for_selector_low(&current_limit_ranges[0],
-					  ARRAY_SIZE(current_limit_ranges), sel,
-					  ma);
-
+	ret = linear_range_get_value_array(&current_limit_ranges[0],
+					   ARRAY_SIZE(current_limit_ranges),
+					   sel, ma);
 	if (ret) {
 		/* Unspecified values mean 500 mA */
 		*ma = 500;
@@ -588,15 +537,28 @@  static int set_charge_current(struct bd70528_psy *bdpsy, int ma)
 		goto set;
 	}
 
-	ret = find_selector_for_value_low(&warm_charge_curr[0],
-					  ARRAY_SIZE(warm_charge_curr), ma,
-					  &reg, &found);
+/*
+ * For BD70528 voltage/current limits we happily accept any value which
+ * belongs the range. We could check if value matching the selector is
+ * desired by computing the range min + (sel - sel_low) * range step - but
+ * I guess it is enough if we use voltage/current which is closest (below)
+ * the requested?
+ */
+
+	ret = linear_range_get_selector_low_array(warm_charge_curr,
+						  ARRAY_SIZE(warm_charge_curr),
+						  ma, &reg, &found);
 	if (ret) {
+		dev_err(bdpsy->dev,
+			 "Unsupported charge current %u mA\n", ma);
 		reg = MIN_CHG_CURR_SEL;
 		goto set;
 	}
 	if (!found) {
-		/* There was a gap in supported values and we hit it */
+		/*
+		 * There was a gap in supported values and we hit it.
+		 * Yet a smaller value was found so we use it.
+		 */
 		dev_warn(bdpsy->dev,
 			 "Unsupported charge current %u mA\n", ma);
 	}
@@ -648,17 +610,21 @@  static int set_current_limit(struct bd70528_psy *bdpsy, int ma)
 		goto set;
 	}
 
-	ret = find_selector_for_value_low(&current_limit_ranges[0],
-					  ARRAY_SIZE(current_limit_ranges), ma,
-					  &reg, &found);
+	ret = linear_range_get_selector_low_array(current_limit_ranges,
+					ARRAY_SIZE(current_limit_ranges),
+					ma, &reg, &found);
 	if (ret) {
+		dev_err(bdpsy->dev, "Unsupported current limit %umA\n", ma);
 		reg = MIN_CURR_LIMIT_SEL;
 		goto set;
 	}
 	if (!found) {
-		/* There was a gap in supported values and we hit it ?*/
-		dev_warn(bdpsy->dev, "Unsupported current limit %umA\n",
-			 ma);
+		/*
+		 * There was a gap in supported values and we hit it.
+		 * We found a smaller value from ranges and use it.
+		 * Warn user though.
+		 */
+		dev_warn(bdpsy->dev, "Unsupported current limit %umA\n", ma);
 	}
 
 set: