[PATCH 1/7] iio: inkern: API for reading available iio channel attribute values

[PATCH 1/7] iio: inkern: API for reading available iio channel attribute values

[Artur Rojek]

Extend the inkern API with a function for reading available
attribute values of iio channels.

Signed-off-by: Artur Rojek <contact@artur-rojek.eu>
---
 drivers/iio/inkern.c         | 20 ++++++++++++++++++++
 include/linux/iio/consumer.h | 14 ++++++++++++++
 2 files changed, 34 insertions(+)

diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
index 06ca3f7fcc44..f19dbde3c945 100644
--- a/drivers/iio/inkern.c
+++ b/drivers/iio/inkern.c
@@ -733,6 +733,26 @@ static int iio_channel_read_avail(struct iio_channel *chan,
 						 vals, type, length, info);
 }
 
+int iio_read_avail_channel_attribute(struct iio_channel *chan,
+				     const int **vals, int *type, int *length,
+				     enum iio_chan_info_enum attribute)
+{
+	int ret;
+
+	mutex_lock(&chan->indio_dev->info_exist_lock);
+	if (!chan->indio_dev->info) {
+		ret = -ENODEV;
+		goto err_unlock;
+	}
+
+	ret = iio_channel_read_avail(chan, vals, type, length, attribute);
+err_unlock:
+	mutex_unlock(&chan->indio_dev->info_exist_lock);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(iio_read_avail_channel_attribute);
+
 int iio_read_avail_channel_raw(struct iio_channel *chan,
 			       const int **vals, int *length)
 {
diff --git a/include/linux/iio/consumer.h b/include/linux/iio/consumer.h
index 9887f4f8e2a8..b2d34831ed7c 100644
--- a/include/linux/iio/consumer.h
+++ b/include/linux/iio/consumer.h
@@ -290,6 +290,20 @@ int iio_read_max_channel_raw(struct iio_channel *chan, int *val);
 int iio_read_avail_channel_raw(struct iio_channel *chan,
 			       const int **vals, int *length);
 
+/**
+ * iio_read_avail_channel_attribute() - read available channel attribute values
+ * @chan:		The channel being queried.
+ * @vals:		Available values read back.
+ * @type:		Type of values read back.
+ * @length:		Number of entries in vals.
+ * @attribute:		info attribute to be read back.
+ *
+ * Returns an error code, IIO_AVAIL_RANGE or IIO_AVAIL_LIST.
+ */
+int iio_read_avail_channel_attribute(struct iio_channel *chan,
+				     const int **vals, int *type, int *length,
+				     enum iio_chan_info_enum attribute);
+
 /**
  * iio_get_channel_type() - get the type of a channel
  * @channel:		The channel being queried.
-- 
2.20.1

[PATCH 2/7] power: supply: core: Add a field to support battery max voltage

[Artur Rojek]

Add a field for "voltage_max_design_uv" to present fully charged
battery voltage.

Signed-off-by: Artur Rojek <contact@artur-rojek.eu>
---
 drivers/power/supply/power_supply_core.c | 3 +++
 include/linux/power_supply.h             | 1 +
 2 files changed, 4 insertions(+)

diff --git a/drivers/power/supply/power_supply_core.c b/drivers/power/supply/power_supply_core.c
index 569790ea6917..702a2b17ff78 100644
--- a/drivers/power/supply/power_supply_core.c
+++ b/drivers/power/supply/power_supply_core.c
@@ -575,6 +575,7 @@ int power_supply_get_battery_info(struct power_supply *psy,
 	info->energy_full_design_uwh         = -EINVAL;
 	info->charge_full_design_uah         = -EINVAL;
 	info->voltage_min_design_uv          = -EINVAL;
+	info->voltage_max_design_uv          = -EINVAL;
 	info->precharge_current_ua           = -EINVAL;
 	info->charge_term_current_ua         = -EINVAL;
 	info->constant_charge_current_max_ua = -EINVAL;
@@ -615,6 +616,8 @@ int power_supply_get_battery_info(struct power_supply *psy,
 			     &info->charge_full_design_uah);
 	of_property_read_u32(battery_np, "voltage-min-design-microvolt",
 			     &info->voltage_min_design_uv);
+	of_property_read_u32(battery_np, "voltage-max-design-microvolt",
+			     &info->voltage_max_design_uv);
 	of_property_read_u32(battery_np, "precharge-current-microamp",
 			     &info->precharge_current_ua);
 	of_property_read_u32(battery_np, "charge-term-current-microamp",
diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
index 57b2ab82b951..2f9c201a54d1 100644
--- a/include/linux/power_supply.h
+++ b/include/linux/power_supply.h
@@ -332,6 +332,7 @@ struct power_supply_battery_info {
 	int energy_full_design_uwh;	    /* microWatt-hours */
 	int charge_full_design_uah;	    /* microAmp-hours */
 	int voltage_min_design_uv;	    /* microVolts */
+	int voltage_max_design_uv;	    /* microVolts */
 	int precharge_current_ua;	    /* microAmps */
 	int charge_term_current_ua;	    /* microAmps */
 	int constant_charge_current_max_ua; /* microAmps */
-- 
2.20.1

[PATCH 3/7] dt-bindings: power: supply: Add voltage-max-design-microvolt property

[Artur Rojek]

Add documentation for the "voltage-max-design-microvolt" property.

Signed-off-by: Artur Rojek <contact@artur-rojek.eu>
---
 Documentation/devicetree/bindings/power/supply/battery.txt | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/power/supply/battery.txt b/Documentation/devicetree/bindings/power/supply/battery.txt
index 89871ab8c704..5c913d4cf36c 100644
--- a/Documentation/devicetree/bindings/power/supply/battery.txt
+++ b/Documentation/devicetree/bindings/power/supply/battery.txt
@@ -16,6 +16,7 @@ Required Properties:
 
 Optional Properties:
  - voltage-min-design-microvolt: drained battery voltage
+ - voltage-max-design-microvolt: fully charged battery voltage
  - energy-full-design-microwatt-hours: battery design energy
  - charge-full-design-microamp-hours: battery design capacity
  - precharge-current-microamp: current for pre-charge phase
@@ -48,6 +49,7 @@ Example:
 	bat: battery {
 		compatible = "simple-battery";
 		voltage-min-design-microvolt = <3200000>;
+		voltage-max-design-microvolt = <4200000>;
 		energy-full-design-microwatt-hours = <5290000>;
 		charge-full-design-microamp-hours = <1430000>;
 		precharge-current-microamp = <256000>;
-- 
2.20.1

[PATCH 4/7] dt-bindings: power: supply: Add docs for Ingenic JZ47xx SoCs battery.

[Artur Rojek]

Add documentation for the ingenic-battery driver, used on JZ47xx SoCs.

Signed-off-by: Artur Rojek <contact@artur-rojek.eu>
---
 .../bindings/power/supply/ingenic,battery.txt | 31 +++++++++++++++++++
 1 file changed, 31 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/power/supply/ingenic,battery.txt

diff --git a/Documentation/devicetree/bindings/power/supply/ingenic,battery.txt b/Documentation/devicetree/bindings/power/supply/ingenic,battery.txt
new file mode 100644
index 000000000000..66430bf73815
--- /dev/null
+++ b/Documentation/devicetree/bindings/power/supply/ingenic,battery.txt
@@ -0,0 +1,31 @@
+* Ingenic JZ47xx battery bindings
+
+Required properties:
+
+- compatible: Must be "ingenic,jz4740-battery".
+- io-channels: phandle and IIO specifier pair to the IIO device.
+  Format described in iio-bindings.txt.
+- monitored-battery: phandle to a "simple-battery" compatible node.
+
+The "monitored-battery" property must be a phandle to a node using the format
+described in battery.txt, with the following properties being required:
+
+- voltage-min-design-microvolt: Drained battery voltage.
+- voltage-max-design-microvolt: Fully charged battery voltage.
+
+Example:
+
+#include <dt-bindings/iio/adc/ingenic,adc.h>
+
+simple_battery: battery {
+	compatible = "simple-battery";
+	voltage-min-design-microvolt = <3600000>;
+	voltage-max-design-microvolt = <4200000>;
+};
+
+ingenic_battery {
+	compatible = "ingenic,jz4740-battery";
+	io-channels = <&adc INGENIC_ADC_BATTERY>;
+	io-channel-names = "battery";
+	monitored-battery = <&simple_battery>;
+};
-- 
2.20.1

[PATCH 5/7] power: supply: add Ingenic JZ47xx battery driver.

[Artur Rojek]

Add a driver for battery present on Ingenic JZ47xx SoCs.

Signed-off-by: Artur Rojek <contact@artur-rojek.eu>
---
 drivers/power/supply/Kconfig           |  11 ++
 drivers/power/supply/Makefile          |   1 +
 drivers/power/supply/ingenic-battery.c | 182 +++++++++++++++++++++++++
 3 files changed, 194 insertions(+)
 create mode 100644 drivers/power/supply/ingenic-battery.c

diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig
index e901b9879e7e..9bfb1637ec28 100644
--- a/drivers/power/supply/Kconfig
+++ b/drivers/power/supply/Kconfig
@@ -169,6 +169,17 @@ config BATTERY_COLLIE
 	  Say Y to enable support for the battery on the Sharp Zaurus
 	  SL-5500 (collie) models.
 
+config BATTERY_INGENIC
+	tristate "Ingenic JZ47xx SoCs battery driver"
+	depends on MIPS || COMPILE_TEST
+	depends on INGENIC_ADC
+	help
+	  Choose this option if you want to monitor battery status on
+	  Ingenic JZ47xx SoC based devices.
+
+	  This driver can also be built as a module. If so, the module will be
+	  called ingenic-battery.
+
 config BATTERY_IPAQ_MICRO
 	tristate "iPAQ Atmel Micro ASIC battery driver"
 	depends on MFD_IPAQ_MICRO
diff --git a/drivers/power/supply/Makefile b/drivers/power/supply/Makefile
index b731c2a9b695..9e2c481d0187 100644
--- a/drivers/power/supply/Makefile
+++ b/drivers/power/supply/Makefile
@@ -34,6 +34,7 @@ obj-$(CONFIG_BATTERY_PMU)	+= pmu_battery.o
 obj-$(CONFIG_BATTERY_OLPC)	+= olpc_battery.o
 obj-$(CONFIG_BATTERY_TOSA)	+= tosa_battery.o
 obj-$(CONFIG_BATTERY_COLLIE)	+= collie_battery.o
+obj-$(CONFIG_BATTERY_INGENIC)	+= ingenic-battery.o
 obj-$(CONFIG_BATTERY_IPAQ_MICRO) += ipaq_micro_battery.o
 obj-$(CONFIG_BATTERY_WM97XX)	+= wm97xx_battery.o
 obj-$(CONFIG_BATTERY_SBS)	+= sbs-battery.o
diff --git a/drivers/power/supply/ingenic-battery.c b/drivers/power/supply/ingenic-battery.c
new file mode 100644
index 000000000000..4ee75c1ac241
--- /dev/null
+++ b/drivers/power/supply/ingenic-battery.c
@@ -0,0 +1,182 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Battery driver for the Ingenic JZ47xx SoCs
+ * Copyright (c) 2019 Artur Rojek <contact@artur-rojek.eu>
+ *
+ * based on drivers/power/supply/jz4740-battery.c
+ */
+
+#include <linux/iio/consumer.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/power_supply.h>
+#include <linux/property.h>
+
+struct ingenic_battery {
+	struct device *dev;
+	struct iio_channel *channel;
+	struct power_supply_desc desc;
+	struct power_supply *battery;
+	struct power_supply_battery_info info;
+};
+
+static int ingenic_battery_get_property(struct power_supply *psy,
+					enum power_supply_property psp,
+					union power_supply_propval *val)
+{
+	struct ingenic_battery *bat = power_supply_get_drvdata(psy);
+	struct power_supply_battery_info *info = &bat->info;
+	int ret = 0;
+
+	switch (psp) {
+	case POWER_SUPPLY_PROP_HEALTH:
+		ret = iio_read_channel_processed(bat->channel, &val->intval);
+		val->intval *= 1000;
+		if (val->intval < info->voltage_min_design_uv)
+			val->intval = POWER_SUPPLY_HEALTH_DEAD;
+		else if (val->intval > info->voltage_max_design_uv)
+			val->intval = POWER_SUPPLY_HEALTH_OVERVOLTAGE;
+		else
+			val->intval = POWER_SUPPLY_HEALTH_GOOD;
+	break;
+	case POWER_SUPPLY_PROP_VOLTAGE_NOW:
+		ret = iio_read_channel_processed(bat->channel, &val->intval);
+		val->intval *= 1000;
+	break;
+	case POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN:
+		val->intval = info->voltage_min_design_uv;
+	break;
+	case POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN:
+		val->intval = info->voltage_max_design_uv;
+	break;
+	default:
+		return -EINVAL;
+	};
+
+	return ret;
+}
+
+/* Set the most appropriate IIO channel voltage reference scale
+ * based on the battery's max voltage.
+ */
+static int ingenic_battery_set_scale(struct ingenic_battery *bat)
+{
+	const int *scale_raw;
+	int scale_len, scale_type, best_idx = -1, best_mV, max_raw, i, ret;
+	u64 max_mV;
+
+	ret = iio_read_max_channel_raw(bat->channel, &max_raw);
+	if (ret) {
+		dev_err(bat->dev, "Unable to read max raw channel value\n");
+		return ret;
+	}
+
+	ret = iio_read_avail_channel_attribute(bat->channel, &scale_raw,
+					       &scale_type, &scale_len,
+					       IIO_CHAN_INFO_SCALE);
+	if (ret < 0) {
+		dev_err(bat->dev, "Unable to read channel avail scale\n");
+		return ret;
+	}
+	if (ret != IIO_AVAIL_LIST || scale_type != IIO_VAL_FRACTIONAL_LOG2)
+		return -EINVAL;
+
+	max_mV = bat->info.voltage_max_design_uv / 1000;
+
+	for (i = 1; i < scale_len; i += 2) {
+		u64 scale_mV = (max_raw * scale_raw[i - 1]) >> scale_raw[i];
+
+		if (scale_mV < max_mV)
+			continue;
+
+		if (best_idx >= 0 && scale_mV > best_mV)
+			continue;
+
+		best_mV = scale_mV;
+		best_idx = i - 1;
+	}
+
+	if (best_idx < 0) {
+		dev_err(bat->dev, "Unable to find matching voltage scale\n");
+		return -EINVAL;
+	}
+
+	return iio_write_channel_attribute(bat->channel,
+					   scale_raw[best_idx],
+					   scale_raw[best_idx+1],
+					   IIO_CHAN_INFO_SCALE);
+}
+
+static enum power_supply_property ingenic_battery_properties[] = {
+	POWER_SUPPLY_PROP_HEALTH,
+	POWER_SUPPLY_PROP_VOLTAGE_NOW,
+	POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN,
+	POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN,
+};
+
+static int ingenic_battery_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct ingenic_battery *bat;
+	struct power_supply_config psy_cfg = {};
+	struct power_supply_desc *desc;
+	int ret;
+
+	bat = devm_kzalloc(dev, sizeof(*bat), GFP_KERNEL);
+	if (!bat)
+		return -ENOMEM;
+
+	bat->dev = dev;
+	bat->channel = devm_iio_channel_get(dev, "battery");
+	if (IS_ERR(bat->channel))
+		return PTR_ERR(bat->channel);
+
+	desc = &bat->desc;
+	desc->name = "jz-battery";
+	desc->type = POWER_SUPPLY_TYPE_BATTERY;
+	desc->properties = ingenic_battery_properties;
+	desc->num_properties = ARRAY_SIZE(ingenic_battery_properties);
+	desc->get_property = ingenic_battery_get_property;
+	psy_cfg.drv_data = bat;
+	psy_cfg.of_node = dev->of_node;
+
+	bat->battery = devm_power_supply_register(dev, desc, &psy_cfg);
+	if (IS_ERR(bat->battery)) {
+		dev_err(dev, "Unable to register battery\n");
+		return PTR_ERR(bat->battery);
+	}
+
+	ret = power_supply_get_battery_info(bat->battery, &bat->info);
+	if (ret) {
+		dev_err(dev, "Unable to get battery info: %d\n", ret);
+		return ret;
+	}
+	if (bat->info.voltage_min_design_uv < 0) {
+		dev_err(dev, "Unable to get voltage min design\n");
+		return bat->info.voltage_min_design_uv;
+	}
+	if (bat->info.voltage_max_design_uv < 0) {
+		dev_err(dev, "Unable to get voltage max design\n");
+		return bat->info.voltage_max_design_uv;
+	}
+
+	return ingenic_battery_set_scale(bat);
+}
+
+#ifdef CONFIG_OF
+static const struct of_device_id ingenic_battery_of_match[] = {
+	{ .compatible = "ingenic,jz4740-battery", },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, ingenic_battery_of_match);
+#endif
+
+static struct platform_driver ingenic_battery_driver = {
+	.driver = {
+		.name = "ingenic-battery",
+		.of_match_table = of_match_ptr(ingenic_battery_of_match),
+	},
+	.probe = ingenic_battery_probe,
+};
+module_platform_driver(ingenic_battery_driver);
-- 
2.20.1

Re: [PATCH 1/7] iio: inkern: API for reading available iio channel attribute values

[Artur Rojek]

Hi,

This patchset is supposed to contain 5 patches, not 7. So this patch is 
actually 1/5. My apologies for the mistake.

Artur

On 2019-02-17 15:29, Artur Rojek wrote:
> Extend the inkern API with a function for reading available
> attribute values of iio channels.
> 
> Signed-off-by: Artur Rojek <contact@artur-rojek.eu>
> ---
>  drivers/iio/inkern.c         | 20 ++++++++++++++++++++
>  include/linux/iio/consumer.h | 14 ++++++++++++++
>  2 files changed, 34 insertions(+)
> 
> diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
> index 06ca3f7fcc44..f19dbde3c945 100644
> --- a/drivers/iio/inkern.c
> +++ b/drivers/iio/inkern.c
> @@ -733,6 +733,26 @@ static int iio_channel_read_avail(struct 
> iio_channel *chan,
>  						 vals, type, length, info);
>  }
> 
> +int iio_read_avail_channel_attribute(struct iio_channel *chan,
> +				     const int **vals, int *type, int *length,
> +				     enum iio_chan_info_enum attribute)
> +{
> +	int ret;
> +
> +	mutex_lock(&chan->indio_dev->info_exist_lock);
> +	if (!chan->indio_dev->info) {
> +		ret = -ENODEV;
> +		goto err_unlock;
> +	}
> +
> +	ret = iio_channel_read_avail(chan, vals, type, length, attribute);
> +err_unlock:
> +	mutex_unlock(&chan->indio_dev->info_exist_lock);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(iio_read_avail_channel_attribute);
> +
>  int iio_read_avail_channel_raw(struct iio_channel *chan,
>  			       const int **vals, int *length)
>  {
> diff --git a/include/linux/iio/consumer.h 
> b/include/linux/iio/consumer.h
> index 9887f4f8e2a8..b2d34831ed7c 100644
> --- a/include/linux/iio/consumer.h
> +++ b/include/linux/iio/consumer.h
> @@ -290,6 +290,20 @@ int iio_read_max_channel_raw(struct iio_channel
> *chan, int *val);
>  int iio_read_avail_channel_raw(struct iio_channel *chan,
>  			       const int **vals, int *length);
> 
> +/**
> + * iio_read_avail_channel_attribute() - read available channel 
> attribute values
> + * @chan:		The channel being queried.
> + * @vals:		Available values read back.
> + * @type:		Type of values read back.
> + * @length:		Number of entries in vals.
> + * @attribute:		info attribute to be read back.
> + *
> + * Returns an error code, IIO_AVAIL_RANGE or IIO_AVAIL_LIST.
> + */
> +int iio_read_avail_channel_attribute(struct iio_channel *chan,
> +				     const int **vals, int *type, int *length,
> +				     enum iio_chan_info_enum attribute);
> +
>  /**
>   * iio_get_channel_type() - get the type of a channel
>   * @channel:		The channel being queried.

Re: [PATCH 5/7] power: supply: add Ingenic JZ47xx battery driver.

[Jonathan Cameron]

On Sun, 17 Feb 2019 15:29:14 +0100
Artur Rojek <contact@artur-rojek.eu> wrote:

> Add a driver for battery present on Ingenic JZ47xx SoCs.
> 
> Signed-off-by: Artur Rojek <contact@artur-rojek.eu>
A few things inline.

thanks,

Jonathan

> ---
>  drivers/power/supply/Kconfig           |  11 ++
>  drivers/power/supply/Makefile          |   1 +
>  drivers/power/supply/ingenic-battery.c | 182 +++++++++++++++++++++++++
>  3 files changed, 194 insertions(+)
>  create mode 100644 drivers/power/supply/ingenic-battery.c
> 
> diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig
> index e901b9879e7e..9bfb1637ec28 100644
> --- a/drivers/power/supply/Kconfig
> +++ b/drivers/power/supply/Kconfig
> @@ -169,6 +169,17 @@ config BATTERY_COLLIE
>  	  Say Y to enable support for the battery on the Sharp Zaurus
>  	  SL-5500 (collie) models.
>  
> +config BATTERY_INGENIC
> +	tristate "Ingenic JZ47xx SoCs battery driver"
> +	depends on MIPS || COMPILE_TEST
> +	depends on INGENIC_ADC
> +	help
> +	  Choose this option if you want to monitor battery status on
> +	  Ingenic JZ47xx SoC based devices.
> +
> +	  This driver can also be built as a module. If so, the module will be
> +	  called ingenic-battery.
> +
>  config BATTERY_IPAQ_MICRO
>  	tristate "iPAQ Atmel Micro ASIC battery driver"
>  	depends on MFD_IPAQ_MICRO
> diff --git a/drivers/power/supply/Makefile b/drivers/power/supply/Makefile
> index b731c2a9b695..9e2c481d0187 100644
> --- a/drivers/power/supply/Makefile
> +++ b/drivers/power/supply/Makefile
> @@ -34,6 +34,7 @@ obj-$(CONFIG_BATTERY_PMU)	+= pmu_battery.o
>  obj-$(CONFIG_BATTERY_OLPC)	+= olpc_battery.o
>  obj-$(CONFIG_BATTERY_TOSA)	+= tosa_battery.o
>  obj-$(CONFIG_BATTERY_COLLIE)	+= collie_battery.o
> +obj-$(CONFIG_BATTERY_INGENIC)	+= ingenic-battery.o
>  obj-$(CONFIG_BATTERY_IPAQ_MICRO) += ipaq_micro_battery.o
>  obj-$(CONFIG_BATTERY_WM97XX)	+= wm97xx_battery.o
>  obj-$(CONFIG_BATTERY_SBS)	+= sbs-battery.o
> diff --git a/drivers/power/supply/ingenic-battery.c b/drivers/power/supply/ingenic-battery.c
> new file mode 100644
> index 000000000000..4ee75c1ac241
> --- /dev/null
> +++ b/drivers/power/supply/ingenic-battery.c
> @@ -0,0 +1,182 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Battery driver for the Ingenic JZ47xx SoCs
> + * Copyright (c) 2019 Artur Rojek <contact@artur-rojek.eu>
> + *
> + * based on drivers/power/supply/jz4740-battery.c

What is the relationship between this driver and the older one?

> + */
> +
> +#include <linux/iio/consumer.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/power_supply.h>
> +#include <linux/property.h>
> +
> +struct ingenic_battery {
> +	struct device *dev;
> +	struct iio_channel *channel;
> +	struct power_supply_desc desc;
> +	struct power_supply *battery;
> +	struct power_supply_battery_info info;
> +};
> +
> +static int ingenic_battery_get_property(struct power_supply *psy,
> +					enum power_supply_property psp,
> +					union power_supply_propval *val)
> +{
> +	struct ingenic_battery *bat = power_supply_get_drvdata(psy);
> +	struct power_supply_battery_info *info = &bat->info;
> +	int ret = 0;
> +
> +	switch (psp) {
> +	case POWER_SUPPLY_PROP_HEALTH:
> +		ret = iio_read_channel_processed(bat->channel, &val->intval);
> +		val->intval *= 1000;
> +		if (val->intval < info->voltage_min_design_uv)
> +			val->intval = POWER_SUPPLY_HEALTH_DEAD;
> +		else if (val->intval > info->voltage_max_design_uv)
> +			val->intval = POWER_SUPPLY_HEALTH_OVERVOLTAGE;
> +		else
> +			val->intval = POWER_SUPPLY_HEALTH_GOOD;
> +	break;
> +	case POWER_SUPPLY_PROP_VOLTAGE_NOW:
> +		ret = iio_read_channel_processed(bat->channel, &val->intval);
> +		val->intval *= 1000;
> +	break;
> +	case POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN:
> +		val->intval = info->voltage_min_design_uv;
> +	break;
> +	case POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN:
> +		val->intval = info->voltage_max_design_uv;
> +	break;
> +	default:
> +		return -EINVAL;
> +	};
> +
Can't get here, so drop.

> +	return ret;
> +}
> +
> +/* Set the most appropriate IIO channel voltage reference scale
> + * based on the battery's max voltage.
> + */
> +static int ingenic_battery_set_scale(struct ingenic_battery *bat)
> +{
> +	const int *scale_raw;
> +	int scale_len, scale_type, best_idx = -1, best_mV, max_raw, i, ret;
> +	u64 max_mV;
> +
> +	ret = iio_read_max_channel_raw(bat->channel, &max_raw);
> +	if (ret) {
> +		dev_err(bat->dev, "Unable to read max raw channel value\n");
> +		return ret;
> +	}
> +
> +	ret = iio_read_avail_channel_attribute(bat->channel, &scale_raw,
> +					       &scale_type, &scale_len,
> +					       IIO_CHAN_INFO_SCALE);
> +	if (ret < 0) {
> +		dev_err(bat->dev, "Unable to read channel avail scale\n");
> +		return ret;
> +	}
This works under the constraints of knowing what ADC we are talking to, but
this isn't generic.  The relationship between scale and the range isn't always
direct.  i.e. there are devices where you sample the same range at a lower
scale to be able to read it faster (can be thought of as oversampling, or
just not running the last stages of a pipelined ADC).
Anyhow, doesn't matter here as I assume this is fine on this part.

> +	if (ret != IIO_AVAIL_LIST || scale_type != IIO_VAL_FRACTIONAL_LOG2)
> +		return -EINVAL;
> +
> +	max_mV = bat->info.voltage_max_design_uv / 1000;
> +
> +	for (i = 1; i < scale_len; i += 2) {
> +		u64 scale_mV = (max_raw * scale_raw[i - 1]) >> scale_raw[i];
Not keen on the offset in the index being backwards.

This would be clearer I think as

for (i = 0; i < scale_len; i+= 2) {
	u64 scale_mv = (max_raw * scale_raw[i]) >> scale_raw[i + 1];
	...
	best_idx = i;
}

> +
> +		if (scale_mV < max_mV)
> +			continue;
> +
> +		if (best_idx >= 0 && scale_mV > best_mV)
> +			continue;
> +
> +		best_mV = scale_mV;
> +		best_idx = i - 1;
> +	}
> +
> +	if (best_idx < 0) {
> +		dev_err(bat->dev, "Unable to find matching voltage scale\n");
> +		return -EINVAL;
> +	}
> +
> +	return iio_write_channel_attribute(bat->channel,
> +					   scale_raw[best_idx],
> +					   scale_raw[best_idx+1],

Spacing around that +.

> +					   IIO_CHAN_INFO_SCALE);
> +}
> +
> +static enum power_supply_property ingenic_battery_properties[] = {
> +	POWER_SUPPLY_PROP_HEALTH,
> +	POWER_SUPPLY_PROP_VOLTAGE_NOW,
> +	POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN,
> +	POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN,
> +};
> +
> +static int ingenic_battery_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct ingenic_battery *bat;
> +	struct power_supply_config psy_cfg = {};
> +	struct power_supply_desc *desc;
> +	int ret;
> +
> +	bat = devm_kzalloc(dev, sizeof(*bat), GFP_KERNEL);
> +	if (!bat)
> +		return -ENOMEM;
> +
> +	bat->dev = dev;
> +	bat->channel = devm_iio_channel_get(dev, "battery");
> +	if (IS_ERR(bat->channel))
> +		return PTR_ERR(bat->channel);
> +
> +	desc = &bat->desc;
> +	desc->name = "jz-battery";
> +	desc->type = POWER_SUPPLY_TYPE_BATTERY;
> +	desc->properties = ingenic_battery_properties;
> +	desc->num_properties = ARRAY_SIZE(ingenic_battery_properties);
> +	desc->get_property = ingenic_battery_get_property;
> +	psy_cfg.drv_data = bat;
> +	psy_cfg.of_node = dev->of_node;
> +
> +	bat->battery = devm_power_supply_register(dev, desc, &psy_cfg);
> +	if (IS_ERR(bat->battery)) {
> +		dev_err(dev, "Unable to register battery\n");
> +		return PTR_ERR(bat->battery);
> +	}
> +
> +	ret = power_supply_get_battery_info(bat->battery, &bat->info);
> +	if (ret) {
> +		dev_err(dev, "Unable to get battery info: %d\n", ret);
> +		return ret;
> +	}
> +	if (bat->info.voltage_min_design_uv < 0) {
> +		dev_err(dev, "Unable to get voltage min design\n");
> +		return bat->info.voltage_min_design_uv;
> +	}
> +	if (bat->info.voltage_max_design_uv < 0) {
> +		dev_err(dev, "Unable to get voltage max design\n");
> +		return bat->info.voltage_max_design_uv;
> +	}
> +
> +	return ingenic_battery_set_scale(bat);
> +}
> +
> +#ifdef CONFIG_OF
> +static const struct of_device_id ingenic_battery_of_match[] = {
> +	{ .compatible = "ingenic,jz4740-battery", },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, ingenic_battery_of_match);
> +#endif
> +
> +static struct platform_driver ingenic_battery_driver = {
> +	.driver = {
> +		.name = "ingenic-battery",
> +		.of_match_table = of_match_ptr(ingenic_battery_of_match),
> +	},
> +	.probe = ingenic_battery_probe,
> +};
> +module_platform_driver(ingenic_battery_driver);

Re: [PATCH 1/7] iio: inkern: API for reading available iio channel attribute values

[Jonathan Cameron]

On Sun, 17 Feb 2019 15:29:10 +0100
Artur Rojek <contact@artur-rojek.eu> wrote:

> Extend the inkern API with a function for reading available
> attribute values of iio channels.
> 
> Signed-off-by: Artur Rojek <contact@artur-rojek.eu>

Hmm. It would have been cleaner if we'd done the generic version
in the first place rather than just having the raw value
equivalent of this.

Would you mind sending a follow up patch to convert that call
over to a simple stub around this function?
The alternative would be to convert the few users over to this
function.  Hmm. Probably best to just share the code as much as
possible and for now leave the old function in place.

Anyhow, this looks good ;)

Given the battery driver is tied up with this, it will make sense
for them to go together.  Probably a case for an immutable branch
either in iio or the relevant battery tree.

Assuming it'll be the battery tree.

Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

The cleanup suggested above can follow at some later time.

Thanks,

Jonathan


> ---
>  drivers/iio/inkern.c         | 20 ++++++++++++++++++++
>  include/linux/iio/consumer.h | 14 ++++++++++++++
>  2 files changed, 34 insertions(+)
> 
> diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
> index 06ca3f7fcc44..f19dbde3c945 100644
> --- a/drivers/iio/inkern.c
> +++ b/drivers/iio/inkern.c
> @@ -733,6 +733,26 @@ static int iio_channel_read_avail(struct iio_channel *chan,
>  						 vals, type, length, info);
>  }
>  
> +int iio_read_avail_channel_attribute(struct iio_channel *chan,
> +				     const int **vals, int *type, int *length,
> +				     enum iio_chan_info_enum attribute)
> +{
> +	int ret;
> +
> +	mutex_lock(&chan->indio_dev->info_exist_lock);
> +	if (!chan->indio_dev->info) {
> +		ret = -ENODEV;
> +		goto err_unlock;
> +	}
> +
> +	ret = iio_channel_read_avail(chan, vals, type, length, attribute);
> +err_unlock:
> +	mutex_unlock(&chan->indio_dev->info_exist_lock);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(iio_read_avail_channel_attribute);
> +
>  int iio_read_avail_channel_raw(struct iio_channel *chan,
>  			       const int **vals, int *length)
>  {
> diff --git a/include/linux/iio/consumer.h b/include/linux/iio/consumer.h
> index 9887f4f8e2a8..b2d34831ed7c 100644
> --- a/include/linux/iio/consumer.h
> +++ b/include/linux/iio/consumer.h
> @@ -290,6 +290,20 @@ int iio_read_max_channel_raw(struct iio_channel *chan, int *val);
>  int iio_read_avail_channel_raw(struct iio_channel *chan,
>  			       const int **vals, int *length);
>  
> +/**
> + * iio_read_avail_channel_attribute() - read available channel attribute values
> + * @chan:		The channel being queried.
> + * @vals:		Available values read back.
> + * @type:		Type of values read back.
> + * @length:		Number of entries in vals.
> + * @attribute:		info attribute to be read back.
> + *
> + * Returns an error code, IIO_AVAIL_RANGE or IIO_AVAIL_LIST.
> + */
> +int iio_read_avail_channel_attribute(struct iio_channel *chan,
> +				     const int **vals, int *type, int *length,
> +				     enum iio_chan_info_enum attribute);
> +
>  /**
>   * iio_get_channel_type() - get the type of a channel
>   * @channel:		The channel being queried.

Re: [PATCH 5/7] power: supply: add Ingenic JZ47xx battery driver.

[Artur Rojek]

On 2019-02-20 12:14, Jonathan Cameron wrote:
> On Sun, 17 Feb 2019 15:29:14 +0100
> Artur Rojek <contact@artur-rojek.eu> wrote:
> 
>> Add a driver for battery present on Ingenic JZ47xx SoCs.
>> 
>> Signed-off-by: Artur Rojek <contact@artur-rojek.eu>
> A few things inline.
> 
> thanks,
> 
> Jonathan
> 

Hi Jonathan,

Thanks for the review.

>> ---
>>  drivers/power/supply/Kconfig           |  11 ++
>>  drivers/power/supply/Makefile          |   1 +
>>  drivers/power/supply/ingenic-battery.c | 182 
>> +++++++++++++++++++++++++
>>  3 files changed, 194 insertions(+)
>>  create mode 100644 drivers/power/supply/ingenic-battery.c
>> 
>> diff --git a/drivers/power/supply/Kconfig 
>> b/drivers/power/supply/Kconfig
>> index e901b9879e7e..9bfb1637ec28 100644
>> --- a/drivers/power/supply/Kconfig
>> +++ b/drivers/power/supply/Kconfig
>> @@ -169,6 +169,17 @@ config BATTERY_COLLIE
>>  	  Say Y to enable support for the battery on the Sharp Zaurus
>>  	  SL-5500 (collie) models.
>> 
>> +config BATTERY_INGENIC
>> +	tristate "Ingenic JZ47xx SoCs battery driver"
>> +	depends on MIPS || COMPILE_TEST
>> +	depends on INGENIC_ADC
>> +	help
>> +	  Choose this option if you want to monitor battery status on
>> +	  Ingenic JZ47xx SoC based devices.
>> +
>> +	  This driver can also be built as a module. If so, the module will 
>> be
>> +	  called ingenic-battery.
>> +
>>  config BATTERY_IPAQ_MICRO
>>  	tristate "iPAQ Atmel Micro ASIC battery driver"
>>  	depends on MFD_IPAQ_MICRO
>> diff --git a/drivers/power/supply/Makefile 
>> b/drivers/power/supply/Makefile
>> index b731c2a9b695..9e2c481d0187 100644
>> --- a/drivers/power/supply/Makefile
>> +++ b/drivers/power/supply/Makefile
>> @@ -34,6 +34,7 @@ obj-$(CONFIG_BATTERY_PMU)	+= pmu_battery.o
>>  obj-$(CONFIG_BATTERY_OLPC)	+= olpc_battery.o
>>  obj-$(CONFIG_BATTERY_TOSA)	+= tosa_battery.o
>>  obj-$(CONFIG_BATTERY_COLLIE)	+= collie_battery.o
>> +obj-$(CONFIG_BATTERY_INGENIC)	+= ingenic-battery.o
>>  obj-$(CONFIG_BATTERY_IPAQ_MICRO) += ipaq_micro_battery.o
>>  obj-$(CONFIG_BATTERY_WM97XX)	+= wm97xx_battery.o
>>  obj-$(CONFIG_BATTERY_SBS)	+= sbs-battery.o
>> diff --git a/drivers/power/supply/ingenic-battery.c 
>> b/drivers/power/supply/ingenic-battery.c
>> new file mode 100644
>> index 000000000000..4ee75c1ac241
>> --- /dev/null
>> +++ b/drivers/power/supply/ingenic-battery.c
>> @@ -0,0 +1,182 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Battery driver for the Ingenic JZ47xx SoCs
>> + * Copyright (c) 2019 Artur Rojek <contact@artur-rojek.eu>
>> + *
>> + * based on drivers/power/supply/jz4740-battery.c
> 
> What is the relationship between this driver and the older one?
This driver intends to replace the older one.

Artur
> 
>> + */
>> +
>> +#include <linux/iio/consumer.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/power_supply.h>
>> +#include <linux/property.h>
>> +
>> +struct ingenic_battery {
>> +	struct device *dev;
>> +	struct iio_channel *channel;
>> +	struct power_supply_desc desc;
>> +	struct power_supply *battery;
>> +	struct power_supply_battery_info info;
>> +};
>> +
>> +static int ingenic_battery_get_property(struct power_supply *psy,
>> +					enum power_supply_property psp,
>> +					union power_supply_propval *val)
>> +{
>> +	struct ingenic_battery *bat = power_supply_get_drvdata(psy);
>> +	struct power_supply_battery_info *info = &bat->info;
>> +	int ret = 0;
>> +
>> +	switch (psp) {
>> +	case POWER_SUPPLY_PROP_HEALTH:
>> +		ret = iio_read_channel_processed(bat->channel, &val->intval);
>> +		val->intval *= 1000;
>> +		if (val->intval < info->voltage_min_design_uv)
>> +			val->intval = POWER_SUPPLY_HEALTH_DEAD;
>> +		else if (val->intval > info->voltage_max_design_uv)
>> +			val->intval = POWER_SUPPLY_HEALTH_OVERVOLTAGE;
>> +		else
>> +			val->intval = POWER_SUPPLY_HEALTH_GOOD;
>> +	break;
>> +	case POWER_SUPPLY_PROP_VOLTAGE_NOW:
>> +		ret = iio_read_channel_processed(bat->channel, &val->intval);
>> +		val->intval *= 1000;
>> +	break;
>> +	case POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN:
>> +		val->intval = info->voltage_min_design_uv;
>> +	break;
>> +	case POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN:
>> +		val->intval = info->voltage_max_design_uv;
>> +	break;
>> +	default:
>> +		return -EINVAL;
>> +	};
>> +
> Can't get here, so drop.
> 
>> +	return ret;
>> +}
>> +
>> +/* Set the most appropriate IIO channel voltage reference scale
>> + * based on the battery's max voltage.
>> + */
>> +static int ingenic_battery_set_scale(struct ingenic_battery *bat)
>> +{
>> +	const int *scale_raw;
>> +	int scale_len, scale_type, best_idx = -1, best_mV, max_raw, i, ret;
>> +	u64 max_mV;
>> +
>> +	ret = iio_read_max_channel_raw(bat->channel, &max_raw);
>> +	if (ret) {
>> +		dev_err(bat->dev, "Unable to read max raw channel value\n");
>> +		return ret;
>> +	}
>> +
>> +	ret = iio_read_avail_channel_attribute(bat->channel, &scale_raw,
>> +					       &scale_type, &scale_len,
>> +					       IIO_CHAN_INFO_SCALE);
>> +	if (ret < 0) {
>> +		dev_err(bat->dev, "Unable to read channel avail scale\n");
>> +		return ret;
>> +	}
> This works under the constraints of knowing what ADC we are talking to, 
> but
> this isn't generic.  The relationship between scale and the range isn't 
> always
> direct.  i.e. there are devices where you sample the same range at a 
> lower
> scale to be able to read it faster (can be thought of as oversampling, 
> or
> just not running the last stages of a pipelined ADC).
> Anyhow, doesn't matter here as I assume this is fine on this part.
> 
>> +	if (ret != IIO_AVAIL_LIST || scale_type != IIO_VAL_FRACTIONAL_LOG2)
>> +		return -EINVAL;
>> +
>> +	max_mV = bat->info.voltage_max_design_uv / 1000;
>> +
>> +	for (i = 1; i < scale_len; i += 2) {
>> +		u64 scale_mV = (max_raw * scale_raw[i - 1]) >> scale_raw[i];
> Not keen on the offset in the index being backwards.
> 
> This would be clearer I think as
> 
> for (i = 0; i < scale_len; i+= 2) {
> 	u64 scale_mv = (max_raw * scale_raw[i]) >> scale_raw[i + 1];
> 	...
> 	best_idx = i;
> }
> 
>> +
>> +		if (scale_mV < max_mV)
>> +			continue;
>> +
>> +		if (best_idx >= 0 && scale_mV > best_mV)
>> +			continue;
>> +
>> +		best_mV = scale_mV;
>> +		best_idx = i - 1;
>> +	}
>> +
>> +	if (best_idx < 0) {
>> +		dev_err(bat->dev, "Unable to find matching voltage scale\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	return iio_write_channel_attribute(bat->channel,
>> +					   scale_raw[best_idx],
>> +					   scale_raw[best_idx+1],
> 
> Spacing around that +.
> 
>> +					   IIO_CHAN_INFO_SCALE);
>> +}
>> +
>> +static enum power_supply_property ingenic_battery_properties[] = {
>> +	POWER_SUPPLY_PROP_HEALTH,
>> +	POWER_SUPPLY_PROP_VOLTAGE_NOW,
>> +	POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN,
>> +	POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN,
>> +};
>> +
>> +static int ingenic_battery_probe(struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	struct ingenic_battery *bat;
>> +	struct power_supply_config psy_cfg = {};
>> +	struct power_supply_desc *desc;
>> +	int ret;
>> +
>> +	bat = devm_kzalloc(dev, sizeof(*bat), GFP_KERNEL);
>> +	if (!bat)
>> +		return -ENOMEM;
>> +
>> +	bat->dev = dev;
>> +	bat->channel = devm_iio_channel_get(dev, "battery");
>> +	if (IS_ERR(bat->channel))
>> +		return PTR_ERR(bat->channel);
>> +
>> +	desc = &bat->desc;
>> +	desc->name = "jz-battery";
>> +	desc->type = POWER_SUPPLY_TYPE_BATTERY;
>> +	desc->properties = ingenic_battery_properties;
>> +	desc->num_properties = ARRAY_SIZE(ingenic_battery_properties);
>> +	desc->get_property = ingenic_battery_get_property;
>> +	psy_cfg.drv_data = bat;
>> +	psy_cfg.of_node = dev->of_node;
>> +
>> +	bat->battery = devm_power_supply_register(dev, desc, &psy_cfg);
>> +	if (IS_ERR(bat->battery)) {
>> +		dev_err(dev, "Unable to register battery\n");
>> +		return PTR_ERR(bat->battery);
>> +	}
>> +
>> +	ret = power_supply_get_battery_info(bat->battery, &bat->info);
>> +	if (ret) {
>> +		dev_err(dev, "Unable to get battery info: %d\n", ret);
>> +		return ret;
>> +	}
>> +	if (bat->info.voltage_min_design_uv < 0) {
>> +		dev_err(dev, "Unable to get voltage min design\n");
>> +		return bat->info.voltage_min_design_uv;
>> +	}
>> +	if (bat->info.voltage_max_design_uv < 0) {
>> +		dev_err(dev, "Unable to get voltage max design\n");
>> +		return bat->info.voltage_max_design_uv;
>> +	}
>> +
>> +	return ingenic_battery_set_scale(bat);
>> +}
>> +
>> +#ifdef CONFIG_OF
>> +static const struct of_device_id ingenic_battery_of_match[] = {
>> +	{ .compatible = "ingenic,jz4740-battery", },
>> +	{ },
>> +};
>> +MODULE_DEVICE_TABLE(of, ingenic_battery_of_match);
>> +#endif
>> +
>> +static struct platform_driver ingenic_battery_driver = {
>> +	.driver = {
>> +		.name = "ingenic-battery",
>> +		.of_match_table = of_match_ptr(ingenic_battery_of_match),
>> +	},
>> +	.probe = ingenic_battery_probe,
>> +};
>> +module_platform_driver(ingenic_battery_driver);

Re: [PATCH 5/7] power: supply: add Ingenic JZ47xx battery driver.

[Artur Rojek]

On 2019-02-20 12:14, Jonathan Cameron wrote:
> On Sun, 17 Feb 2019 15:29:14 +0100
> Artur Rojek <contact@artur-rojek.eu> wrote:
> 
>> Add a driver for battery present on Ingenic JZ47xx SoCs.
>> 
>> Signed-off-by: Artur Rojek <contact@artur-rojek.eu>
> A few things inline.
> 
> thanks,
> 
> Jonathan
Hi Jonathan.

One thing inline.

Artur
> 
>> ---
>>  drivers/power/supply/Kconfig           |  11 ++
>>  drivers/power/supply/Makefile          |   1 +
>>  drivers/power/supply/ingenic-battery.c | 182 
>> +++++++++++++++++++++++++
>>  3 files changed, 194 insertions(+)
>>  create mode 100644 drivers/power/supply/ingenic-battery.c
>> 
>> diff --git a/drivers/power/supply/Kconfig 
>> b/drivers/power/supply/Kconfig
>> index e901b9879e7e..9bfb1637ec28 100644
>> --- a/drivers/power/supply/Kconfig
>> +++ b/drivers/power/supply/Kconfig
>> @@ -169,6 +169,17 @@ config BATTERY_COLLIE
>>  	  Say Y to enable support for the battery on the Sharp Zaurus
>>  	  SL-5500 (collie) models.
>> 
>> +config BATTERY_INGENIC
>> +	tristate "Ingenic JZ47xx SoCs battery driver"
>> +	depends on MIPS || COMPILE_TEST
>> +	depends on INGENIC_ADC
>> +	help
>> +	  Choose this option if you want to monitor battery status on
>> +	  Ingenic JZ47xx SoC based devices.
>> +
>> +	  This driver can also be built as a module. If so, the module will 
>> be
>> +	  called ingenic-battery.
>> +
>>  config BATTERY_IPAQ_MICRO
>>  	tristate "iPAQ Atmel Micro ASIC battery driver"
>>  	depends on MFD_IPAQ_MICRO
>> diff --git a/drivers/power/supply/Makefile 
>> b/drivers/power/supply/Makefile
>> index b731c2a9b695..9e2c481d0187 100644
>> --- a/drivers/power/supply/Makefile
>> +++ b/drivers/power/supply/Makefile
>> @@ -34,6 +34,7 @@ obj-$(CONFIG_BATTERY_PMU)	+= pmu_battery.o
>>  obj-$(CONFIG_BATTERY_OLPC)	+= olpc_battery.o
>>  obj-$(CONFIG_BATTERY_TOSA)	+= tosa_battery.o
>>  obj-$(CONFIG_BATTERY_COLLIE)	+= collie_battery.o
>> +obj-$(CONFIG_BATTERY_INGENIC)	+= ingenic-battery.o
>>  obj-$(CONFIG_BATTERY_IPAQ_MICRO) += ipaq_micro_battery.o
>>  obj-$(CONFIG_BATTERY_WM97XX)	+= wm97xx_battery.o
>>  obj-$(CONFIG_BATTERY_SBS)	+= sbs-battery.o
>> diff --git a/drivers/power/supply/ingenic-battery.c 
>> b/drivers/power/supply/ingenic-battery.c
>> new file mode 100644
>> index 000000000000..4ee75c1ac241
>> --- /dev/null
>> +++ b/drivers/power/supply/ingenic-battery.c
>> @@ -0,0 +1,182 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Battery driver for the Ingenic JZ47xx SoCs
>> + * Copyright (c) 2019 Artur Rojek <contact@artur-rojek.eu>
>> + *
>> + * based on drivers/power/supply/jz4740-battery.c
> 
> What is the relationship between this driver and the older one?
> 
>> + */
>> +
>> +#include <linux/iio/consumer.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/power_supply.h>
>> +#include <linux/property.h>
>> +
>> +struct ingenic_battery {
>> +	struct device *dev;
>> +	struct iio_channel *channel;
>> +	struct power_supply_desc desc;
>> +	struct power_supply *battery;
>> +	struct power_supply_battery_info info;
>> +};
>> +
>> +static int ingenic_battery_get_property(struct power_supply *psy,
>> +					enum power_supply_property psp,
>> +					union power_supply_propval *val)
>> +{
>> +	struct ingenic_battery *bat = power_supply_get_drvdata(psy);
>> +	struct power_supply_battery_info *info = &bat->info;
>> +	int ret = 0;
>> +
>> +	switch (psp) {
>> +	case POWER_SUPPLY_PROP_HEALTH:
>> +		ret = iio_read_channel_processed(bat->channel, &val->intval);
>> +		val->intval *= 1000;
>> +		if (val->intval < info->voltage_min_design_uv)
>> +			val->intval = POWER_SUPPLY_HEALTH_DEAD;
>> +		else if (val->intval > info->voltage_max_design_uv)
>> +			val->intval = POWER_SUPPLY_HEALTH_OVERVOLTAGE;
>> +		else
>> +			val->intval = POWER_SUPPLY_HEALTH_GOOD;
>> +	break;
>> +	case POWER_SUPPLY_PROP_VOLTAGE_NOW:
>> +		ret = iio_read_channel_processed(bat->channel, &val->intval);
>> +		val->intval *= 1000;
>> +	break;
>> +	case POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN:
>> +		val->intval = info->voltage_min_design_uv;
>> +	break;
>> +	case POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN:
>> +		val->intval = info->voltage_max_design_uv;
>> +	break;
>> +	default:
>> +		return -EINVAL;
>> +	};
>> +
> Can't get here, so drop.
Execution actually does reach this point.
Would you rather have me return in individual switch cases instead?
> 
>> +	return ret;
>> +}
>> +
>> +/* Set the most appropriate IIO channel voltage reference scale
>> + * based on the battery's max voltage.
>> + */
>> +static int ingenic_battery_set_scale(struct ingenic_battery *bat)
>> +{
>> +	const int *scale_raw;
>> +	int scale_len, scale_type, best_idx = -1, best_mV, max_raw, i, ret;
>> +	u64 max_mV;
>> +
>> +	ret = iio_read_max_channel_raw(bat->channel, &max_raw);
>> +	if (ret) {
>> +		dev_err(bat->dev, "Unable to read max raw channel value\n");
>> +		return ret;
>> +	}
>> +
>> +	ret = iio_read_avail_channel_attribute(bat->channel, &scale_raw,
>> +					       &scale_type, &scale_len,
>> +					       IIO_CHAN_INFO_SCALE);
>> +	if (ret < 0) {
>> +		dev_err(bat->dev, "Unable to read channel avail scale\n");
>> +		return ret;
>> +	}
> This works under the constraints of knowing what ADC we are talking to, 
> but
> this isn't generic.  The relationship between scale and the range isn't 
> always
> direct.  i.e. there are devices where you sample the same range at a 
> lower
> scale to be able to read it faster (can be thought of as oversampling, 
> or
> just not running the last stages of a pipelined ADC).
> Anyhow, doesn't matter here as I assume this is fine on this part.
> 
>> +	if (ret != IIO_AVAIL_LIST || scale_type != IIO_VAL_FRACTIONAL_LOG2)
>> +		return -EINVAL;
>> +
>> +	max_mV = bat->info.voltage_max_design_uv / 1000;
>> +
>> +	for (i = 1; i < scale_len; i += 2) {
>> +		u64 scale_mV = (max_raw * scale_raw[i - 1]) >> scale_raw[i];
> Not keen on the offset in the index being backwards.
> 
> This would be clearer I think as
> 
> for (i = 0; i < scale_len; i+= 2) {
> 	u64 scale_mv = (max_raw * scale_raw[i]) >> scale_raw[i + 1];
> 	...
> 	best_idx = i;
> }
> 
>> +
>> +		if (scale_mV < max_mV)
>> +			continue;
>> +
>> +		if (best_idx >= 0 && scale_mV > best_mV)
>> +			continue;
>> +
>> +		best_mV = scale_mV;
>> +		best_idx = i - 1;
>> +	}
>> +
>> +	if (best_idx < 0) {
>> +		dev_err(bat->dev, "Unable to find matching voltage scale\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	return iio_write_channel_attribute(bat->channel,
>> +					   scale_raw[best_idx],
>> +					   scale_raw[best_idx+1],
> 
> Spacing around that +.
> 
>> +					   IIO_CHAN_INFO_SCALE);
>> +}
>> +
>> +static enum power_supply_property ingenic_battery_properties[] = {
>> +	POWER_SUPPLY_PROP_HEALTH,
>> +	POWER_SUPPLY_PROP_VOLTAGE_NOW,
>> +	POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN,
>> +	POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN,
>> +};
>> +
>> +static int ingenic_battery_probe(struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	struct ingenic_battery *bat;
>> +	struct power_supply_config psy_cfg = {};
>> +	struct power_supply_desc *desc;
>> +	int ret;
>> +
>> +	bat = devm_kzalloc(dev, sizeof(*bat), GFP_KERNEL);
>> +	if (!bat)
>> +		return -ENOMEM;
>> +
>> +	bat->dev = dev;
>> +	bat->channel = devm_iio_channel_get(dev, "battery");
>> +	if (IS_ERR(bat->channel))
>> +		return PTR_ERR(bat->channel);
>> +
>> +	desc = &bat->desc;
>> +	desc->name = "jz-battery";
>> +	desc->type = POWER_SUPPLY_TYPE_BATTERY;
>> +	desc->properties = ingenic_battery_properties;
>> +	desc->num_properties = ARRAY_SIZE(ingenic_battery_properties);
>> +	desc->get_property = ingenic_battery_get_property;
>> +	psy_cfg.drv_data = bat;
>> +	psy_cfg.of_node = dev->of_node;
>> +
>> +	bat->battery = devm_power_supply_register(dev, desc, &psy_cfg);
>> +	if (IS_ERR(bat->battery)) {
>> +		dev_err(dev, "Unable to register battery\n");
>> +		return PTR_ERR(bat->battery);
>> +	}
>> +
>> +	ret = power_supply_get_battery_info(bat->battery, &bat->info);
>> +	if (ret) {
>> +		dev_err(dev, "Unable to get battery info: %d\n", ret);
>> +		return ret;
>> +	}
>> +	if (bat->info.voltage_min_design_uv < 0) {
>> +		dev_err(dev, "Unable to get voltage min design\n");
>> +		return bat->info.voltage_min_design_uv;
>> +	}
>> +	if (bat->info.voltage_max_design_uv < 0) {
>> +		dev_err(dev, "Unable to get voltage max design\n");
>> +		return bat->info.voltage_max_design_uv;
>> +	}
>> +
>> +	return ingenic_battery_set_scale(bat);
>> +}
>> +
>> +#ifdef CONFIG_OF
>> +static const struct of_device_id ingenic_battery_of_match[] = {
>> +	{ .compatible = "ingenic,jz4740-battery", },
>> +	{ },
>> +};
>> +MODULE_DEVICE_TABLE(of, ingenic_battery_of_match);
>> +#endif
>> +
>> +static struct platform_driver ingenic_battery_driver = {
>> +	.driver = {
>> +		.name = "ingenic-battery",
>> +		.of_match_table = of_match_ptr(ingenic_battery_of_match),
>> +	},
>> +	.probe = ingenic_battery_probe,
>> +};
>> +module_platform_driver(ingenic_battery_driver);

Re: [PATCH 4/7] dt-bindings: power: supply: Add docs for Ingenic JZ47xx SoCs battery.

[Rob Herring]

On Sun, Feb 17, 2019 at 03:29:13PM +0100, Artur Rojek wrote:
> Add documentation for the ingenic-battery driver, used on JZ47xx SoCs.
> 
> Signed-off-by: Artur Rojek <contact@artur-rojek.eu>
> ---
>  .../bindings/power/supply/ingenic,battery.txt | 31 +++++++++++++++++++
>  1 file changed, 31 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/power/supply/ingenic,battery.txt
> 
> diff --git a/Documentation/devicetree/bindings/power/supply/ingenic,battery.txt b/Documentation/devicetree/bindings/power/supply/ingenic,battery.txt
> new file mode 100644
> index 000000000000..66430bf73815
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/power/supply/ingenic,battery.txt
> @@ -0,0 +1,31 @@
> +* Ingenic JZ47xx battery bindings
> +
> +Required properties:
> +
> +- compatible: Must be "ingenic,jz4740-battery".
> +- io-channels: phandle and IIO specifier pair to the IIO device.
> +  Format described in iio-bindings.txt.
> +- monitored-battery: phandle to a "simple-battery" compatible node.
> +
> +The "monitored-battery" property must be a phandle to a node using the format
> +described in battery.txt, with the following properties being required:
> +
> +- voltage-min-design-microvolt: Drained battery voltage.
> +- voltage-max-design-microvolt: Fully charged battery voltage.
> +
> +Example:
> +
> +#include <dt-bindings/iio/adc/ingenic,adc.h>
> +
> +simple_battery: battery {
> +	compatible = "simple-battery";
> +	voltage-min-design-microvolt = <3600000>;
> +	voltage-max-design-microvolt = <4200000>;
> +};
> +
> +ingenic_battery {
> +	compatible = "ingenic,jz4740-battery";
> +	io-channels = <&adc INGENIC_ADC_BATTERY>;
> +	io-channel-names = "battery";
> +	monitored-battery = <&simple_battery>;

Isn't this just an ADC monitored battery? What's specific to JZ4740?

Maybe a generic binding is appropriate here?

Rob

Re: [PATCH 4/7] dt-bindings: power: supply: Add docs for Ingenic JZ47xx SoCs battery.

[Paul Cercueil]

Hi Rob,

Le jeu. 28 févr. 2019 à 15:48, Rob Herring <robh@kernel.org> a écrit 
:
> On Sun, Feb 17, 2019 at 03:29:13PM +0100, Artur Rojek wrote:
>>  Add documentation for the ingenic-battery driver, used on JZ47xx 
>> SoCs.
>> 
>>  Signed-off-by: Artur Rojek <contact@artur-rojek.eu>
>>  ---
>>   .../bindings/power/supply/ingenic,battery.txt | 31 
>> +++++++++++++++++++
>>   1 file changed, 31 insertions(+)
>>   create mode 100644 
>> Documentation/devicetree/bindings/power/supply/ingenic,battery.txt
>> 
>>  diff --git 
>> a/Documentation/devicetree/bindings/power/supply/ingenic,battery.txt 
>> b/Documentation/devicetree/bindings/power/supply/ingenic,battery.txt
>>  new file mode 100644
>>  index 000000000000..66430bf73815
>>  --- /dev/null
>>  +++ 
>> b/Documentation/devicetree/bindings/power/supply/ingenic,battery.txt
>>  @@ -0,0 +1,31 @@
>>  +* Ingenic JZ47xx battery bindings
>>  +
>>  +Required properties:
>>  +
>>  +- compatible: Must be "ingenic,jz4740-battery".
>>  +- io-channels: phandle and IIO specifier pair to the IIO device.
>>  +  Format described in iio-bindings.txt.
>>  +- monitored-battery: phandle to a "simple-battery" compatible node.
>>  +
>>  +The "monitored-battery" property must be a phandle to a node using 
>> the format
>>  +described in battery.txt, with the following properties being 
>> required:
>>  +
>>  +- voltage-min-design-microvolt: Drained battery voltage.
>>  +- voltage-max-design-microvolt: Fully charged battery voltage.
>>  +
>>  +Example:
>>  +
>>  +#include <dt-bindings/iio/adc/ingenic,adc.h>
>>  +
>>  +simple_battery: battery {
>>  +	compatible = "simple-battery";
>>  +	voltage-min-design-microvolt = <3600000>;
>>  +	voltage-max-design-microvolt = <4200000>;
>>  +};
>>  +
>>  +ingenic_battery {
>>  +	compatible = "ingenic,jz4740-battery";
>>  +	io-channels = <&adc INGENIC_ADC_BATTERY>;
>>  +	io-channel-names = "battery";
>>  +	monitored-battery = <&simple_battery>;
> 
> Isn't this just an ADC monitored battery? What's specific to JZ4740?
> 
> Maybe a generic binding is appropriate here?
> 
> Rob

There is no generic driver, is there? The generic-adc-battery driver
isn't OF-compatible and looks like dead code (nothing uses it).

-Paul

Re: [PATCH 4/7] dt-bindings: power: supply: Add docs for Ingenic JZ47xx SoCs battery.

[Artur Rojek]

Hi Rob,

thanks for the review.

On 2019-02-28 19:48, Rob Herring wrote:
> On Sun, Feb 17, 2019 at 03:29:13PM +0100, Artur Rojek wrote:
>> Add documentation for the ingenic-battery driver, used on JZ47xx SoCs.
>> 
>> Signed-off-by: Artur Rojek <contact@artur-rojek.eu>
>> ---
>>  .../bindings/power/supply/ingenic,battery.txt | 31 
>> +++++++++++++++++++
>>  1 file changed, 31 insertions(+)
>>  create mode 100644 
>> Documentation/devicetree/bindings/power/supply/ingenic,battery.txt
>> 
>> diff --git 
>> a/Documentation/devicetree/bindings/power/supply/ingenic,battery.txt 
>> b/Documentation/devicetree/bindings/power/supply/ingenic,battery.txt
>> new file mode 100644
>> index 000000000000..66430bf73815
>> --- /dev/null
>> +++ 
>> b/Documentation/devicetree/bindings/power/supply/ingenic,battery.txt
>> @@ -0,0 +1,31 @@
>> +* Ingenic JZ47xx battery bindings
>> +
>> +Required properties:
>> +
>> +- compatible: Must be "ingenic,jz4740-battery".
>> +- io-channels: phandle and IIO specifier pair to the IIO device.
>> +  Format described in iio-bindings.txt.
>> +- monitored-battery: phandle to a "simple-battery" compatible node.
>> +
>> +The "monitored-battery" property must be a phandle to a node using 
>> the format
>> +described in battery.txt, with the following properties being 
>> required:
>> +
>> +- voltage-min-design-microvolt: Drained battery voltage.
>> +- voltage-max-design-microvolt: Fully charged battery voltage.
>> +
>> +Example:
>> +
>> +#include <dt-bindings/iio/adc/ingenic,adc.h>
>> +
>> +simple_battery: battery {
>> +	compatible = "simple-battery";
>> +	voltage-min-design-microvolt = <3600000>;
>> +	voltage-max-design-microvolt = <4200000>;
>> +};
>> +
>> +ingenic_battery {
>> +	compatible = "ingenic,jz4740-battery";
>> +	io-channels = <&adc INGENIC_ADC_BATTERY>;
>> +	io-channel-names = "battery";
>> +	monitored-battery = <&simple_battery>;
> 
> Isn't this just an ADC monitored battery? What's specific to JZ4740?
> 
> Maybe a generic binding is appropriate here?
This isn't quite a generic driver; see Jonathan's reply to patch 5/7.

- Artur
> 
> Rob

Re: [PATCH 5/7] power: supply: add Ingenic JZ47xx battery driver.

[Jonathan Cameron]

On Thu, 21 Feb 2019 20:18:36 +0100
Artur Rojek <contact@artur-rojek.eu> wrote:

> On 2019-02-20 12:14, Jonathan Cameron wrote:
> > On Sun, 17 Feb 2019 15:29:14 +0100
> > Artur Rojek <contact@artur-rojek.eu> wrote:
> >   
> >> Add a driver for battery present on Ingenic JZ47xx SoCs.
> >> 
> >> Signed-off-by: Artur Rojek <contact@artur-rojek.eu>  
> > A few things inline.
> > 
> > thanks,
> > 
> > Jonathan  
> Hi Jonathan.
> 
> One thing inline.
> 
> Artur
> >   
> >> ---
> >>  drivers/power/supply/Kconfig           |  11 ++
> >>  drivers/power/supply/Makefile          |   1 +
> >>  drivers/power/supply/ingenic-battery.c | 182 
> >> +++++++++++++++++++++++++
> >>  3 files changed, 194 insertions(+)
> >>  create mode 100644 drivers/power/supply/ingenic-battery.c
> >> 
> >> diff --git a/drivers/power/supply/Kconfig 
> >> b/drivers/power/supply/Kconfig
> >> index e901b9879e7e..9bfb1637ec28 100644
> >> --- a/drivers/power/supply/Kconfig
> >> +++ b/drivers/power/supply/Kconfig
> >> @@ -169,6 +169,17 @@ config BATTERY_COLLIE
> >>  	  Say Y to enable support for the battery on the Sharp Zaurus
> >>  	  SL-5500 (collie) models.
> >> 
> >> +config BATTERY_INGENIC
> >> +	tristate "Ingenic JZ47xx SoCs battery driver"
> >> +	depends on MIPS || COMPILE_TEST
> >> +	depends on INGENIC_ADC
> >> +	help
> >> +	  Choose this option if you want to monitor battery status on
> >> +	  Ingenic JZ47xx SoC based devices.
> >> +
> >> +	  This driver can also be built as a module. If so, the module will 
> >> be
> >> +	  called ingenic-battery.
> >> +
> >>  config BATTERY_IPAQ_MICRO
> >>  	tristate "iPAQ Atmel Micro ASIC battery driver"
> >>  	depends on MFD_IPAQ_MICRO
> >> diff --git a/drivers/power/supply/Makefile 
> >> b/drivers/power/supply/Makefile
> >> index b731c2a9b695..9e2c481d0187 100644
> >> --- a/drivers/power/supply/Makefile
> >> +++ b/drivers/power/supply/Makefile
> >> @@ -34,6 +34,7 @@ obj-$(CONFIG_BATTERY_PMU)	+= pmu_battery.o
> >>  obj-$(CONFIG_BATTERY_OLPC)	+= olpc_battery.o
> >>  obj-$(CONFIG_BATTERY_TOSA)	+= tosa_battery.o
> >>  obj-$(CONFIG_BATTERY_COLLIE)	+= collie_battery.o
> >> +obj-$(CONFIG_BATTERY_INGENIC)	+= ingenic-battery.o
> >>  obj-$(CONFIG_BATTERY_IPAQ_MICRO) += ipaq_micro_battery.o
> >>  obj-$(CONFIG_BATTERY_WM97XX)	+= wm97xx_battery.o
> >>  obj-$(CONFIG_BATTERY_SBS)	+= sbs-battery.o
> >> diff --git a/drivers/power/supply/ingenic-battery.c 
> >> b/drivers/power/supply/ingenic-battery.c
> >> new file mode 100644
> >> index 000000000000..4ee75c1ac241
> >> --- /dev/null
> >> +++ b/drivers/power/supply/ingenic-battery.c
> >> @@ -0,0 +1,182 @@
> >> +// SPDX-License-Identifier: GPL-2.0
> >> +/*
> >> + * Battery driver for the Ingenic JZ47xx SoCs
> >> + * Copyright (c) 2019 Artur Rojek <contact@artur-rojek.eu>
> >> + *
> >> + * based on drivers/power/supply/jz4740-battery.c  
> > 
> > What is the relationship between this driver and the older one?
> >   
> >> + */
> >> +
> >> +#include <linux/iio/consumer.h>
> >> +#include <linux/module.h>
> >> +#include <linux/of.h>
> >> +#include <linux/platform_device.h>
> >> +#include <linux/power_supply.h>
> >> +#include <linux/property.h>
> >> +
> >> +struct ingenic_battery {
> >> +	struct device *dev;
> >> +	struct iio_channel *channel;
> >> +	struct power_supply_desc desc;
> >> +	struct power_supply *battery;
> >> +	struct power_supply_battery_info info;
> >> +};
> >> +
> >> +static int ingenic_battery_get_property(struct power_supply *psy,
> >> +					enum power_supply_property psp,
> >> +					union power_supply_propval *val)
> >> +{
> >> +	struct ingenic_battery *bat = power_supply_get_drvdata(psy);
> >> +	struct power_supply_battery_info *info = &bat->info;
> >> +	int ret = 0;
> >> +
> >> +	switch (psp) {
> >> +	case POWER_SUPPLY_PROP_HEALTH:
> >> +		ret = iio_read_channel_processed(bat->channel, &val->intval);
> >> +		val->intval *= 1000;
> >> +		if (val->intval < info->voltage_min_design_uv)
> >> +			val->intval = POWER_SUPPLY_HEALTH_DEAD;
> >> +		else if (val->intval > info->voltage_max_design_uv)
> >> +			val->intval = POWER_SUPPLY_HEALTH_OVERVOLTAGE;
> >> +		else
> >> +			val->intval = POWER_SUPPLY_HEALTH_GOOD;
> >> +	break;
> >> +	case POWER_SUPPLY_PROP_VOLTAGE_NOW:
> >> +		ret = iio_read_channel_processed(bat->channel, &val->intval);
> >> +		val->intval *= 1000;
> >> +	break;
> >> +	case POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN:
> >> +		val->intval = info->voltage_min_design_uv;
> >> +	break;
> >> +	case POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN:
> >> +		val->intval = info->voltage_max_design_uv;
> >> +	break;
> >> +	default:
> >> +		return -EINVAL;
> >> +	};
> >> +  
> > Can't get here, so drop.  
> Execution actually does reach this point.
> Would you rather have me return in individual switch cases instead?
good point. I'm clearly blind ;)

Yes, return in each case would probably be cleaner for consistency
with the default case.  Either that or set ret and break in that
path as well.

> >   
> >> +	return ret;
> >> +}
> >> +
> >> +/* Set the most appropriate IIO channel voltage reference scale
> >> + * based on the battery's max voltage.
> >> + */
> >> +static int ingenic_battery_set_scale(struct ingenic_battery *bat)
> >> +{
> >> +	const int *scale_raw;
> >> +	int scale_len, scale_type, best_idx = -1, best_mV, max_raw, i, ret;
> >> +	u64 max_mV;
> >> +
> >> +	ret = iio_read_max_channel_raw(bat->channel, &max_raw);
> >> +	if (ret) {
> >> +		dev_err(bat->dev, "Unable to read max raw channel value\n");
> >> +		return ret;
> >> +	}
> >> +
> >> +	ret = iio_read_avail_channel_attribute(bat->channel, &scale_raw,
> >> +					       &scale_type, &scale_len,
> >> +					       IIO_CHAN_INFO_SCALE);
> >> +	if (ret < 0) {
> >> +		dev_err(bat->dev, "Unable to read channel avail scale\n");
> >> +		return ret;
> >> +	}  
> > This works under the constraints of knowing what ADC we are talking to, 
> > but
> > this isn't generic.  The relationship between scale and the range isn't 
> > always
> > direct.  i.e. there are devices where you sample the same range at a 
> > lower
> > scale to be able to read it faster (can be thought of as oversampling, 
> > or
> > just not running the last stages of a pipelined ADC).
> > Anyhow, doesn't matter here as I assume this is fine on this part.
> >   
> >> +	if (ret != IIO_AVAIL_LIST || scale_type != IIO_VAL_FRACTIONAL_LOG2)
> >> +		return -EINVAL;
> >> +
> >> +	max_mV = bat->info.voltage_max_design_uv / 1000;
> >> +
> >> +	for (i = 1; i < scale_len; i += 2) {
> >> +		u64 scale_mV = (max_raw * scale_raw[i - 1]) >> scale_raw[i];  
> > Not keen on the offset in the index being backwards.
> > 
> > This would be clearer I think as
> > 
> > for (i = 0; i < scale_len; i+= 2) {
> > 	u64 scale_mv = (max_raw * scale_raw[i]) >> scale_raw[i + 1];
> > 	...
> > 	best_idx = i;
> > }
> >   
> >> +
> >> +		if (scale_mV < max_mV)
> >> +			continue;
> >> +
> >> +		if (best_idx >= 0 && scale_mV > best_mV)
> >> +			continue;
> >> +
> >> +		best_mV = scale_mV;
> >> +		best_idx = i - 1;
> >> +	}
> >> +
> >> +	if (best_idx < 0) {
> >> +		dev_err(bat->dev, "Unable to find matching voltage scale\n");
> >> +		return -EINVAL;
> >> +	}
> >> +
> >> +	return iio_write_channel_attribute(bat->channel,
> >> +					   scale_raw[best_idx],
> >> +					   scale_raw[best_idx+1],  
> > 
> > Spacing around that +.
> >   
> >> +					   IIO_CHAN_INFO_SCALE);
> >> +}
> >> +
> >> +static enum power_supply_property ingenic_battery_properties[] = {
> >> +	POWER_SUPPLY_PROP_HEALTH,
> >> +	POWER_SUPPLY_PROP_VOLTAGE_NOW,
> >> +	POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN,
> >> +	POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN,
> >> +};
> >> +
> >> +static int ingenic_battery_probe(struct platform_device *pdev)
> >> +{
> >> +	struct device *dev = &pdev->dev;
> >> +	struct ingenic_battery *bat;
> >> +	struct power_supply_config psy_cfg = {};
> >> +	struct power_supply_desc *desc;
> >> +	int ret;
> >> +
> >> +	bat = devm_kzalloc(dev, sizeof(*bat), GFP_KERNEL);
> >> +	if (!bat)
> >> +		return -ENOMEM;
> >> +
> >> +	bat->dev = dev;
> >> +	bat->channel = devm_iio_channel_get(dev, "battery");
> >> +	if (IS_ERR(bat->channel))
> >> +		return PTR_ERR(bat->channel);
> >> +
> >> +	desc = &bat->desc;
> >> +	desc->name = "jz-battery";
> >> +	desc->type = POWER_SUPPLY_TYPE_BATTERY;
> >> +	desc->properties = ingenic_battery_properties;
> >> +	desc->num_properties = ARRAY_SIZE(ingenic_battery_properties);
> >> +	desc->get_property = ingenic_battery_get_property;
> >> +	psy_cfg.drv_data = bat;
> >> +	psy_cfg.of_node = dev->of_node;
> >> +
> >> +	bat->battery = devm_power_supply_register(dev, desc, &psy_cfg);
> >> +	if (IS_ERR(bat->battery)) {
> >> +		dev_err(dev, "Unable to register battery\n");
> >> +		return PTR_ERR(bat->battery);
> >> +	}
> >> +
> >> +	ret = power_supply_get_battery_info(bat->battery, &bat->info);
> >> +	if (ret) {
> >> +		dev_err(dev, "Unable to get battery info: %d\n", ret);
> >> +		return ret;
> >> +	}
> >> +	if (bat->info.voltage_min_design_uv < 0) {
> >> +		dev_err(dev, "Unable to get voltage min design\n");
> >> +		return bat->info.voltage_min_design_uv;
> >> +	}
> >> +	if (bat->info.voltage_max_design_uv < 0) {
> >> +		dev_err(dev, "Unable to get voltage max design\n");
> >> +		return bat->info.voltage_max_design_uv;
> >> +	}
> >> +
> >> +	return ingenic_battery_set_scale(bat);
> >> +}
> >> +
> >> +#ifdef CONFIG_OF
> >> +static const struct of_device_id ingenic_battery_of_match[] = {
> >> +	{ .compatible = "ingenic,jz4740-battery", },
> >> +	{ },
> >> +};
> >> +MODULE_DEVICE_TABLE(of, ingenic_battery_of_match);
> >> +#endif
> >> +
> >> +static struct platform_driver ingenic_battery_driver = {
> >> +	.driver = {
> >> +		.name = "ingenic-battery",
> >> +		.of_match_table = of_match_ptr(ingenic_battery_of_match),
> >> +	},
> >> +	.probe = ingenic_battery_probe,
> >> +};
> >> +module_platform_driver(ingenic_battery_driver);  
>