[PATCH 0/8] add support for AXP813 ADC and battery power supply

[PATCH 0/8] add support for AXP813 ADC and battery power supply

[Quentin Schulz]

The AXP813 PMIC is relatively close to the already supported AXP20X and
AXP22X. It provides three different power outputs: battery, AC and USB, and
measures a few different things: temperature, power supply status, current
current and voltage supplied, maximum current limit, battery capacity, min
and max voltage limits.

One of its two GPIOs can be used as an ADC.

There are a few differences with AXP20X/AXP22X PMICs though:
  - a different constant charge current formula,
  - battery temperature, GPIO0 and battery voltages are the only voltages
  measurable,
  - all data are stored on 12 bits (AXP20X/AXP22X had one type of data that
  was stored on 13 bits),
  - different scales and offsets,
  - a different ADC rate formula and register,

This patch series adds support for the PMIC's ADC and battery power supply
in the existing drivers.

Make the axp20x MFD automatically probe the ADC driver, add the battery
power supply node in axp81x node and enable it for the TBS A711 since it
has a soldered battery.

Q: The BananaPi M3 has two solder balls for battery, should the battery
power supply node be enabled for this board as well?

Thanks,
Quentin

Quentin Schulz (8):
  iio: adc: axp20x_adc: put ADC rate setting in a per-variant function
  iio: adc: axp20x_adc: add support for AXP813 ADC
  mfd: axp20x: probe axp20x_adc driver for AXP813
  dt-bindings: power: supply: axp20x: add AXP813 battery DT binding
  power: supply: axp20x_battery: add support for AXP813
  mfd: axp20x: add battery power supply cell for AXP813
  ARM: dtsi: axp81x: add battery power supply subnode
  ARM: dtsi: sun8i: a711: enable battery power supply subnode

 Documentation/devicetree/bindings/power/supply/axp20x_battery.txt |   8 ++--
 arch/arm/boot/dts/axp81x.dtsi                                     |   5 +++-
 arch/arm/boot/dts/sun8i-a83t-tbs-a711.dts                         |   4 ++-
 drivers/iio/adc/axp20x_adc.c                                      | 139 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++----
 drivers/mfd/axp20x.c                                              |   7 +++-
 drivers/power/supply/axp20x_battery.c                             |  44 ++++++++++++++++++++++-
 include/linux/mfd/axp20x.h                                        |   2 +-
 7 files changed, 196 insertions(+), 13 deletions(-)

base-commit: 7cc61a0a562c7005d2a34f97e94cf26689a2f57c
-- 
git-series 0.9.1

[PATCH 1/8] iio: adc: axp20x_adc: put ADC rate setting in a per-variant function

[Quentin Schulz]

To prepare for a new comer that set a different register with different
values, move rate setting in a function that is specific to each AXP
variant.

Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
---
 drivers/iio/adc/axp20x_adc.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/drivers/iio/adc/axp20x_adc.c b/drivers/iio/adc/axp20x_adc.c
index a30a972..7274f4f 100644
--- a/drivers/iio/adc/axp20x_adc.c
+++ b/drivers/iio/adc/axp20x_adc.c
@@ -470,14 +470,18 @@ static const struct iio_info axp22x_adc_iio_info = {
 	.read_raw = axp22x_read_raw,
 };
 
-static int axp20x_adc_rate(int rate)
+static int axp20x_adc_rate(struct axp20x_adc_iio *info, int rate)
 {
-	return AXP20X_ADC_RATE_HZ(rate);
+	return regmap_update_bits(info->regmap, AXP20X_ADC_RATE,
+				  AXP20X_ADC_RATE_MASK,
+				  AXP20X_ADC_RATE_HZ(rate));
 }
 
-static int axp22x_adc_rate(int rate)
+static int axp22x_adc_rate(struct axp20x_adc_iio *info, int rate)
 {
-	return AXP22X_ADC_RATE_HZ(rate);
+	return regmap_update_bits(info->regmap, AXP20X_ADC_RATE,
+				  AXP20X_ADC_RATE_MASK,
+				  AXP22X_ADC_RATE_HZ(rate));
 }
 
 struct axp_data {
@@ -485,7 +489,7 @@ struct axp_data {
 	int				num_channels;
 	struct iio_chan_spec const	*channels;
 	unsigned long			adc_en1_mask;
-	int				(*adc_rate)(int rate);
+	int				(*adc_rate)(struct axp20x_adc_iio *info, int rate);
 	bool				adc_en2;
 	struct iio_map			*maps;
 };
@@ -554,8 +558,7 @@ static int axp20x_probe(struct platform_device *pdev)
 				   AXP20X_ADC_EN2_MASK, AXP20X_ADC_EN2_MASK);
 
 	/* Configure ADCs rate */
-	regmap_update_bits(info->regmap, AXP20X_ADC_RATE, AXP20X_ADC_RATE_MASK,
-			   info->data->adc_rate(100));
+	info->data->adc_rate(info, 100);
 
 	ret = iio_map_array_register(indio_dev, info->data->maps);
 	if (ret < 0) {
-- 
git-series 0.9.1

[PATCH 2/8] iio: adc: axp20x_adc: add support for AXP813 ADC

[Quentin Schulz]

The X-Powers AXP813 PMIC is really close to what is already done for
AXP20X/AXP22X.

There are two pairs of bits to set the rate (one for Voltage and Current
measurements and one for TS/GPIO0 voltage measurements) instead of one.

The register to set the ADC rates is different from the one for
AXP20X/AXP22X.

GPIO0 can be used as an ADC (measuring Volts) unlike for AXP22X.

The scales to apply to the different inputs are unlike the ones from
AXP20X and AXP22X.

Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
---
 drivers/iio/adc/axp20x_adc.c | 122 ++++++++++++++++++++++++++++++++++++-
 include/linux/mfd/axp20x.h   |   2 +-
 2 files changed, 124 insertions(+)

diff --git a/drivers/iio/adc/axp20x_adc.c b/drivers/iio/adc/axp20x_adc.c
index 7274f4f..03d489b 100644
--- a/drivers/iio/adc/axp20x_adc.c
+++ b/drivers/iio/adc/axp20x_adc.c
@@ -35,8 +35,13 @@
 #define AXP20X_GPIO10_IN_RANGE_GPIO1_VAL(x)	(((x) & BIT(0)) << 1)
 
 #define AXP20X_ADC_RATE_MASK			GENMASK(7, 6)
+#define AXP813_V_I_ADC_RATE_MASK		GENMASK(5, 4)
+#define AXP813_ADC_RATE_MASK			(AXP20X_ADC_RATE_MASK | AXP813_V_I_ADC_RATE_MASK)
 #define AXP20X_ADC_RATE_HZ(x)			((ilog2((x) / 25) << 6) & AXP20X_ADC_RATE_MASK)
 #define AXP22X_ADC_RATE_HZ(x)			((ilog2((x) / 100) << 6) & AXP20X_ADC_RATE_MASK)
+#define AXP813_TS_GPIO0_ADC_RATE_HZ(x)		AXP20X_ADC_RATE_HZ(x)
+#define AXP813_V_I_ADC_RATE_HZ(x)		((ilog2((x) / 100) << 4) & AXP813_V_I_ADC_RATE_MASK)
+#define AXP813_ADC_RATE_HZ(x)			(AXP20X_ADC_RATE_HZ(x) | AXP813_V_I_ADC_RATE_HZ(x))
 
 #define AXP20X_ADC_CHANNEL(_channel, _name, _type, _reg)	\
 	{							\
@@ -95,6 +100,12 @@ enum axp22x_adc_channel_i {
 	AXP22X_BATT_DISCHRG_I,
 };
 
+enum axp813_adc_channel_v {
+	AXP813_TS_IN = 0,
+	AXP813_GPIO0_V,
+	AXP813_BATT_V,
+};
+
 static struct iio_map axp20x_maps[] = {
 	{
 		.consumer_dev_name = "axp20x-usb-power-supply",
@@ -197,6 +208,25 @@ static const struct iio_chan_spec axp22x_adc_channels[] = {
 			   AXP20X_BATT_DISCHRG_I_H),
 };
 
+static const struct iio_chan_spec axp813_adc_channels[] = {
+	{
+		.type = IIO_TEMP,
+		.address = AXP22X_PMIC_TEMP_H,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+				      BIT(IIO_CHAN_INFO_SCALE) |
+				      BIT(IIO_CHAN_INFO_OFFSET),
+		.datasheet_name = "pmic_temp",
+	},
+	AXP20X_ADC_CHANNEL(AXP813_GPIO0_V, "gpio0_v", IIO_VOLTAGE,
+			   AXP288_GP_ADC_H),
+	AXP20X_ADC_CHANNEL(AXP813_BATT_V, "batt_v", IIO_VOLTAGE,
+			   AXP20X_BATT_V_H),
+	AXP20X_ADC_CHANNEL(AXP22X_BATT_CHRG_I, "batt_chrg_i", IIO_CURRENT,
+			   AXP20X_BATT_CHRG_I_H),
+	AXP20X_ADC_CHANNEL(AXP22X_BATT_DISCHRG_I, "batt_dischrg_i", IIO_CURRENT,
+			   AXP20X_BATT_DISCHRG_I_H),
+};
+
 static int axp20x_adc_raw(struct iio_dev *indio_dev,
 			  struct iio_chan_spec const *chan, int *val)
 {
@@ -243,6 +273,18 @@ static int axp22x_adc_raw(struct iio_dev *indio_dev,
 	return IIO_VAL_INT;
 }
 
+static int axp813_adc_raw(struct iio_dev *indio_dev,
+			  struct iio_chan_spec const *chan, int *val)
+{
+	struct axp20x_adc_iio *info = iio_priv(indio_dev);
+
+	*val = axp20x_read_variable_width(info->regmap, chan->address, 12);
+	if (*val < 0)
+		return *val;
+
+	return IIO_VAL_INT;
+}
+
 static int axp20x_adc_scale_voltage(int channel, int *val, int *val2)
 {
 	switch (channel) {
@@ -273,6 +315,24 @@ static int axp20x_adc_scale_voltage(int channel, int *val, int *val2)
 	}
 }
 
+static int axp813_adc_scale_voltage(int channel, int *val, int *val2)
+{
+	switch (channel) {
+	case AXP813_GPIO0_V:
+		*val = 0;
+		*val2 = 800000;
+		return IIO_VAL_INT_PLUS_MICRO;
+
+	case AXP813_BATT_V:
+		*val = 1;
+		*val2 = 100000;
+		return IIO_VAL_INT_PLUS_MICRO;
+
+	default:
+		return -EINVAL;
+	}
+}
+
 static int axp20x_adc_scale_current(int channel, int *val, int *val2)
 {
 	switch (channel) {
@@ -342,6 +402,26 @@ static int axp22x_adc_scale(struct iio_chan_spec const *chan, int *val,
 	}
 }
 
+static int axp813_adc_scale(struct iio_chan_spec const *chan, int *val,
+			    int *val2)
+{
+	switch (chan->type) {
+	case IIO_VOLTAGE:
+		return axp813_adc_scale_voltage(chan->channel, val, val2);
+
+	case IIO_CURRENT:
+		*val = 1;
+		return IIO_VAL_INT;
+
+	case IIO_TEMP:
+		*val = 100;
+		return IIO_VAL_INT;
+
+	default:
+		return -EINVAL;
+	}
+}
+
 static int axp20x_adc_offset_voltage(struct iio_dev *indio_dev, int channel,
 				     int *val)
 {
@@ -425,6 +505,26 @@ static int axp22x_read_raw(struct iio_dev *indio_dev,
 	}
 }
 
+static int axp813_read_raw(struct iio_dev *indio_dev,
+			   struct iio_chan_spec const *chan, int *val,
+			   int *val2, long mask)
+{
+	switch (mask) {
+	case IIO_CHAN_INFO_OFFSET:
+		*val = -2667;
+		return IIO_VAL_INT;
+
+	case IIO_CHAN_INFO_SCALE:
+		return axp813_adc_scale(chan, val, val2);
+
+	case IIO_CHAN_INFO_RAW:
+		return axp813_adc_raw(indio_dev, chan, val);
+
+	default:
+		return -EINVAL;
+	}
+}
+
 static int axp20x_write_raw(struct iio_dev *indio_dev,
 			    struct iio_chan_spec const *chan, int val, int val2,
 			    long mask)
@@ -470,6 +570,10 @@ static const struct iio_info axp22x_adc_iio_info = {
 	.read_raw = axp22x_read_raw,
 };
 
+static const struct iio_info axp813_adc_iio_info = {
+	.read_raw = axp813_read_raw,
+};
+
 static int axp20x_adc_rate(struct axp20x_adc_iio *info, int rate)
 {
 	return regmap_update_bits(info->regmap, AXP20X_ADC_RATE,
@@ -484,6 +588,13 @@ static int axp22x_adc_rate(struct axp20x_adc_iio *info, int rate)
 				  AXP22X_ADC_RATE_HZ(rate));
 }
 
+static int axp813_adc_rate(struct axp20x_adc_iio *info, int rate)
+{
+	return regmap_update_bits(info->regmap, AXP813_ADC_RATE,
+				 AXP813_ADC_RATE_MASK,
+				 AXP813_ADC_RATE_HZ(rate));
+}
+
 struct axp_data {
 	const struct iio_info		*iio_info;
 	int				num_channels;
@@ -514,9 +625,20 @@ static const struct axp_data axp22x_data = {
 	.maps = axp22x_maps,
 };
 
+static const struct axp_data axp813_data = {
+	.iio_info = &axp813_adc_iio_info,
+	.num_channels = ARRAY_SIZE(axp813_adc_channels),
+	.channels = axp813_adc_channels,
+	.adc_en1_mask = AXP22X_ADC_EN1_MASK,
+	.adc_rate = axp813_adc_rate,
+	.adc_en2 = false,
+	.maps = axp22x_maps,
+};
+
 static const struct platform_device_id axp20x_adc_id_match[] = {
 	{ .name = "axp20x-adc", .driver_data = (kernel_ulong_t)&axp20x_data, },
 	{ .name = "axp22x-adc", .driver_data = (kernel_ulong_t)&axp22x_data, },
+	{ .name = "axp813-adc", .driver_data = (kernel_ulong_t)&axp813_data, },
 	{ /* sentinel */ },
 };
 MODULE_DEVICE_TABLE(platform, axp20x_adc_id_match);
diff --git a/include/linux/mfd/axp20x.h b/include/linux/mfd/axp20x.h
index 78dc853..ff95414 100644
--- a/include/linux/mfd/axp20x.h
+++ b/include/linux/mfd/axp20x.h
@@ -266,6 +266,8 @@ enum axp20x_variants {
 #define AXP288_RT_BATT_V_H		0xa0
 #define AXP288_RT_BATT_V_L		0xa1
 
+#define AXP813_ADC_RATE			0x85
+
 /* Fuel Gauge */
 #define AXP288_FG_RDC1_REG          0xba
 #define AXP288_FG_RDC0_REG          0xbb
-- 
git-series 0.9.1

[PATCH 3/8] mfd: axp20x: probe axp20x_adc driver for AXP813

[Quentin Schulz]

This makes the axp20x_adc driver probe with platform device id
"axp813-adc".

Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
---
 drivers/mfd/axp20x.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c
index 2468b43..42e54d1 100644
--- a/drivers/mfd/axp20x.c
+++ b/drivers/mfd/axp20x.c
@@ -878,7 +878,9 @@ static struct mfd_cell axp813_cells[] = {
 		.resources		= axp803_pek_resources,
 	}, {
 		.name			= "axp20x-regulator",
-	}
+	}, {
+		.name			= "axp813-adc",
+	},
 };
 
 static struct axp20x_dev *axp20x_pm_power_off;
-- 
git-series 0.9.1

[PATCH 4/8] dt-bindings: power: supply: axp20x: add AXP813 battery DT binding

[Quentin Schulz]

The AXP813 can have a battery as power supply, so let's add it to the
list of compatibles.

Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
---
 Documentation/devicetree/bindings/power/supply/axp20x_battery.txt | 8 +++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/power/supply/axp20x_battery.txt b/Documentation/devicetree/bindings/power/supply/axp20x_battery.txt
index c248866..4614c8e 100644
--- a/Documentation/devicetree/bindings/power/supply/axp20x_battery.txt
+++ b/Documentation/devicetree/bindings/power/supply/axp20x_battery.txt
@@ -4,12 +4,12 @@ Required Properties:
  - compatible, one of:
 			"x-powers,axp209-battery-power-supply"
 			"x-powers,axp221-battery-power-supply"
+			"x-powers,axp813-battery-power-supply"
 
-This node is a subnode of the axp20x/axp22x PMIC.
+This node is a subnode of the axp20x/axp22x/axp81x PMIC.
 
-The AXP20X and AXP22X can read the battery voltage, charge and discharge
-currents of the battery by reading ADC channels from the AXP20X/AXP22X
-ADC.
+The AXP20X, AXP22X and AXP81X can read the battery voltage, charge and
+discharge currents of the battery by reading ADC channels from the ADC.
 
 Example:
 
-- 
git-series 0.9.1

[PATCH 5/8] power: supply: axp20x_battery: add support for AXP813

[Quentin Schulz]

The X-Powers AXP813 PMIC has got some slight differences from
AXP20X/AXP22X PMICs:
 - the maximum voltage supplied by the PMIC is 4.35 instead of 4.36/4.24
 for AXP20X/AXP22X,
 - the constant charge current formula is different,

It also has a bit to tell whether the battery percentage returned by the
PMIC is valid.

Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
---
 drivers/power/supply/axp20x_battery.c | 44 +++++++++++++++++++++++++++-
 1 file changed, 43 insertions(+), 1 deletion(-)

diff --git a/drivers/power/supply/axp20x_battery.c b/drivers/power/supply/axp20x_battery.c
index 7494f0f..cb30302 100644
--- a/drivers/power/supply/axp20x_battery.c
+++ b/drivers/power/supply/axp20x_battery.c
@@ -46,6 +46,8 @@
 #define AXP20X_CHRG_CTRL1_TGT_4_2V	(2 << 5)
 #define AXP20X_CHRG_CTRL1_TGT_4_36V	(3 << 5)
 
+#define AXP813_CHRG_CTRL1_TGT_4_35V	(3 << 5)
+
 #define AXP22X_CHRG_CTRL1_TGT_4_22V	(1 << 5)
 #define AXP22X_CHRG_CTRL1_TGT_4_24V	(3 << 5)
 
@@ -123,10 +125,41 @@ static int axp22x_battery_get_max_voltage(struct axp20x_batt_ps *axp20x_batt,
 	return 0;
 }
 
+static int axp813_battery_get_max_voltage(struct axp20x_batt_ps *axp20x_batt,
+					  int *val)
+{
+	int ret, reg;
+
+	ret = regmap_read(axp20x_batt->regmap, AXP20X_CHRG_CTRL1, &reg);
+	if (ret)
+		return ret;
+
+	switch (reg & AXP20X_CHRG_CTRL1_TGT_VOLT) {
+	case AXP20X_CHRG_CTRL1_TGT_4_1V:
+		*val = 4100000;
+		break;
+	case AXP20X_CHRG_CTRL1_TGT_4_15V:
+		*val = 4150000;
+		break;
+	case AXP20X_CHRG_CTRL1_TGT_4_2V:
+		*val = 4200000;
+		break;
+	case AXP813_CHRG_CTRL1_TGT_4_35V:
+		*val = 4350000;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 static void raw_to_constant_charge_current(struct axp20x_batt_ps *axp, int *val)
 {
 	if (axp->axp_id == AXP209_ID)
 		*val = *val * 100000 + 300000;
+	else if (axp->axp_id == AXP813_ID)
+		*val = *val * 200000 + 200000;
 	else
 		*val = *val * 150000 + 300000;
 }
@@ -135,6 +168,8 @@ static void constant_charge_current_to_raw(struct axp20x_batt_ps *axp, int *val)
 {
 	if (axp->axp_id == AXP209_ID)
 		*val = (*val - 300000) / 100000;
+	else if (axp->axp_id == AXP813_ID)
+		*val = (*val - 200000) / 200000;
 	else
 		*val = (*val - 300000) / 150000;
 }
@@ -269,7 +304,8 @@ static int axp20x_battery_get_prop(struct power_supply *psy,
 		if (ret)
 			return ret;
 
-		if (axp20x_batt->axp_id == AXP221_ID &&
+		if ((axp20x_batt->axp_id == AXP221_ID ||
+		     axp20x_batt->axp_id == AXP813_ID) &&
 		    !(reg & AXP22X_FG_VALID))
 			return -EINVAL;
 
@@ -284,6 +320,9 @@ static int axp20x_battery_get_prop(struct power_supply *psy,
 		if (axp20x_batt->axp_id == AXP209_ID)
 			return axp20x_battery_get_max_voltage(axp20x_batt,
 							      &val->intval);
+		else if (axp20x_batt->axp_id == AXP813_ID)
+			return axp813_battery_get_max_voltage(axp20x_batt,
+							      &val->intval);
 		return axp22x_battery_get_max_voltage(axp20x_batt,
 						      &val->intval);
 
@@ -467,6 +506,9 @@ static const struct of_device_id axp20x_battery_ps_id[] = {
 	}, {
 		.compatible = "x-powers,axp221-battery-power-supply",
 		.data = (void *)AXP221_ID,
+	}, {
+		.compatible = "x-powers,axp813-battery-power-supply",
+		.data = (void *)AXP813_ID,
 	}, { /* sentinel */ },
 };
 MODULE_DEVICE_TABLE(of, axp20x_battery_ps_id);
-- 
git-series 0.9.1

[PATCH 6/8] mfd: axp20x: add battery power supply cell for AXP813

[Quentin Schulz]

As axp20x-battery-power-supply now supports AXP813, add a cell for it.

Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
---
 drivers/mfd/axp20x.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c
index 42e54d1..7566358 100644
--- a/drivers/mfd/axp20x.c
+++ b/drivers/mfd/axp20x.c
@@ -880,6 +880,9 @@ static struct mfd_cell axp813_cells[] = {
 		.name			= "axp20x-regulator",
 	}, {
 		.name			= "axp813-adc",
+	}, {
+		.name			= "axp20x-battery-power-supply",
+		.of_compatible		= "x-powers,axp813-battery-power-supply",
 	},
 };
 
-- 
git-series 0.9.1

[PATCH 7/8] ARM: dtsi: axp81x: add battery power supply subnode

[Quentin Schulz]

The X-Powers AXP81X PMIC exposes battery supply various data such as
the battery status (charging, discharging, full, dead), current max
limit, current current, battery capacity (in percentage), voltage max
and min limits, current voltage, and battery capacity (in Ah).

This adds the battery power supply subnode for AXP81X PMIC.

Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
---
 arch/arm/boot/dts/axp81x.dtsi | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/arm/boot/dts/axp81x.dtsi b/arch/arm/boot/dts/axp81x.dtsi
index 73b761f..f7401c3 100644
--- a/arch/arm/boot/dts/axp81x.dtsi
+++ b/arch/arm/boot/dts/axp81x.dtsi
@@ -48,6 +48,11 @@
 	interrupt-controller;
 	#interrupt-cells = <1>;
 
+	battery_power_supply: battery-power-supply {
+		compatible = "x-powers,axp813-battery-power-supply";
+		status = "disabled";
+	};
+
 	regulators {
 		/* Default work frequency for buck regulators */
 		x-powers,dcdc-freq = <3000>;
-- 
git-series 0.9.1

[PATCH 8/8] ARM: dtsi: sun8i: a711: enable battery power supply subnode

[Quentin Schulz]

The TBS A711 has an AXP813 PMIC and a soldered battery, thus, we enable
the battery power supply subnode in its Device Tree.

Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
---
 arch/arm/boot/dts/sun8i-a83t-tbs-a711.dts | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm/boot/dts/sun8i-a83t-tbs-a711.dts b/arch/arm/boot/dts/sun8i-a83t-tbs-a711.dts
index 9871553..806cb17 100644
--- a/arch/arm/boot/dts/sun8i-a83t-tbs-a711.dts
+++ b/arch/arm/boot/dts/sun8i-a83t-tbs-a711.dts
@@ -181,6 +181,10 @@
 
 #include "axp81x.dtsi"
 
+&battery_power_supply {
+	status = "okay";
+};
+
 &reg_aldo1 {
 	regulator-min-microvolt = <1800000>;
 	regulator-max-microvolt = <1800000>;
-- 
git-series 0.9.1

Re: [linux-sunxi] [PATCH 1/8] iio: adc: axp20x_adc: put ADC rate setting in a per-variant function

[Chen-Yu Tsai]

On Mon, Dec 4, 2017 at 10:12 PM, Quentin Schulz
<quentin.schulz@free-electrons.com> wrote:
> To prepare for a new comer that set a different register with different
> values, move rate setting in a function that is specific to each AXP
> variant.
>
> Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
> ---
>  drivers/iio/adc/axp20x_adc.c | 17 ++++++++++-------
>  1 file changed, 10 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/iio/adc/axp20x_adc.c b/drivers/iio/adc/axp20x_adc.c
> index a30a972..7274f4f 100644
> --- a/drivers/iio/adc/axp20x_adc.c
> +++ b/drivers/iio/adc/axp20x_adc.c
> @@ -470,14 +470,18 @@ static const struct iio_info axp22x_adc_iio_info = {
>         .read_raw = axp22x_read_raw,
>  };
>
> -static int axp20x_adc_rate(int rate)
> +static int axp20x_adc_rate(struct axp20x_adc_iio *info, int rate)
>  {
> -       return AXP20X_ADC_RATE_HZ(rate);
> +       return regmap_update_bits(info->regmap, AXP20X_ADC_RATE,
> +                                 AXP20X_ADC_RATE_MASK,
> +                                 AXP20X_ADC_RATE_HZ(rate));
>  }
>
> -static int axp22x_adc_rate(int rate)
> +static int axp22x_adc_rate(struct axp20x_adc_iio *info, int rate)
>  {
> -       return AXP22X_ADC_RATE_HZ(rate);
> +       return regmap_update_bits(info->regmap, AXP20X_ADC_RATE,
> +                                 AXP20X_ADC_RATE_MASK,
> +                                 AXP22X_ADC_RATE_HZ(rate));
>  }
>
>  struct axp_data {
> @@ -485,7 +489,7 @@ struct axp_data {
>         int                             num_channels;
>         struct iio_chan_spec const      *channels;
>         unsigned long                   adc_en1_mask;
> -       int                             (*adc_rate)(int rate);
> +       int                             (*adc_rate)(struct axp20x_adc_iio *info, int rate);

Could you also change the name of the callback, to say, adc_set_rate?
This would make it much clearer what the callback does. Previously
it was just a conversion helper.

ChenYu

>         bool                            adc_en2;
>         struct iio_map                  *maps;
>  };
> @@ -554,8 +558,7 @@ static int axp20x_probe(struct platform_device *pdev)
>                                    AXP20X_ADC_EN2_MASK, AXP20X_ADC_EN2_MASK);
>
>         /* Configure ADCs rate */
> -       regmap_update_bits(info->regmap, AXP20X_ADC_RATE, AXP20X_ADC_RATE_MASK,
> -                          info->data->adc_rate(100));
> +       info->data->adc_rate(info, 100);
>
>         ret = iio_map_array_register(indio_dev, info->data->maps);
>         if (ret < 0) {
> --
> git-series 0.9.1
>
> --
> You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.

Re: [PATCH 3/8] mfd: axp20x: probe axp20x_adc driver for AXP813

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 858 bytes --]

On Mon, Dec 04, 2017 at 03:12:49PM +0100, Quentin Schulz wrote:
> This makes the axp20x_adc driver probe with platform device id
> "axp813-adc".
> 
> Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
> ---
>  drivers/mfd/axp20x.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c
> index 2468b43..42e54d1 100644
> --- a/drivers/mfd/axp20x.c
> +++ b/drivers/mfd/axp20x.c
> @@ -878,7 +878,9 @@ static struct mfd_cell axp813_cells[] = {
>  		.resources		= axp803_pek_resources,
>  	}, {
>  		.name			= "axp20x-regulator",
> -	}
> +	}, {
> +		.name			= "axp813-adc",
> +	},

Any particular reason you're not adding it to the DT?

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 6/8] mfd: axp20x: add battery power supply cell for AXP813

[Lee Jones]

On Mon, 04 Dec 2017, Quentin Schulz wrote:

> As axp20x-battery-power-supply now supports AXP813, add a cell for it.
> 
> Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
> ---
>  drivers/mfd/axp20x.c | 3 +++
>  1 file changed, 3 insertions(+)

For my own reference:
  Acked-for-MFD-by: Lee Jones <lee.jones@linaro.org>

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 4/8] dt-bindings: power: supply: axp20x: add AXP813 battery DT binding

[Rob Herring]

On Mon, Dec 04, 2017 at 03:12:50PM +0100, Quentin Schulz wrote:
> The AXP813 can have a battery as power supply, so let's add it to the
> list of compatibles.
> 
> Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
> ---
>  Documentation/devicetree/bindings/power/supply/axp20x_battery.txt | 8 +++----
>  1 file changed, 4 insertions(+), 4 deletions(-)

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH 3/8] mfd: axp20x: probe axp20x_adc driver for AXP813

[Quentin Schulz]

[-- Attachment #1.1: Type: text/plain, Size: 1190 bytes --]

Hi Maxime,

On 05/12/2017 09:08, Maxime Ripard wrote:
> On Mon, Dec 04, 2017 at 03:12:49PM +0100, Quentin Schulz wrote:
>> This makes the axp20x_adc driver probe with platform device id
>> "axp813-adc".
>>
>> Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
>> ---
>>  drivers/mfd/axp20x.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c
>> index 2468b43..42e54d1 100644
>> --- a/drivers/mfd/axp20x.c
>> +++ b/drivers/mfd/axp20x.c
>> @@ -878,7 +878,9 @@ static struct mfd_cell axp813_cells[] = {
>>  		.resources		= axp803_pek_resources,
>>  	}, {
>>  		.name			= "axp20x-regulator",
>> -	}
>> +	}, {
>> +		.name			= "axp813-adc",
>> +	},
> 
> Any particular reason you're not adding it to the DT?
> 

No, no particular reason. It's just the way it is currently for AXP209
and AXP22x so did the same for AXP813.

I'll add DT "support" in next version for all AXPs supported by this
driver. Or is it worthy of a small separate patch series?

Thanks,
Quentin
-- 
Quentin Schulz, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH 3/8] mfd: axp20x: probe axp20x_adc driver for AXP813

[Chen-Yu Tsai]

On Thu, Dec 7, 2017 at 4:51 PM, Quentin Schulz
<quentin.schulz@free-electrons.com> wrote:
> Hi Maxime,
>
> On 05/12/2017 09:08, Maxime Ripard wrote:
>> On Mon, Dec 04, 2017 at 03:12:49PM +0100, Quentin Schulz wrote:
>>> This makes the axp20x_adc driver probe with platform device id
>>> "axp813-adc".
>>>
>>> Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
>>> ---
>>>  drivers/mfd/axp20x.c | 4 +++-
>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c
>>> index 2468b43..42e54d1 100644
>>> --- a/drivers/mfd/axp20x.c
>>> +++ b/drivers/mfd/axp20x.c
>>> @@ -878,7 +878,9 @@ static struct mfd_cell axp813_cells[] = {
>>>              .resources              = axp803_pek_resources,
>>>      }, {
>>>              .name                   = "axp20x-regulator",
>>> -    }
>>> +    }, {
>>> +            .name                   = "axp813-adc",
>>> +    },
>>
>> Any particular reason you're not adding it to the DT?
>>
>
> No, no particular reason. It's just the way it is currently for AXP209
> and AXP22x so did the same for AXP813.
>
> I'll add DT "support" in next version for all AXPs supported by this
> driver. Or is it worthy of a small separate patch series?

IIRC there's no DT support because there's no need to reference
it in the device tree.

ChenYu

Re: [PATCH 3/8] mfd: axp20x: probe axp20x_adc driver for AXP813

[Quentin Schulz]

Hi Chen-Yu,

On 07/12/2017 09:54, Chen-Yu Tsai wrote:
> On Thu, Dec 7, 2017 at 4:51 PM, Quentin Schulz
> <quentin.schulz@free-electrons.com> wrote:
>> Hi Maxime,
>>
>> On 05/12/2017 09:08, Maxime Ripard wrote:
>>> On Mon, Dec 04, 2017 at 03:12:49PM +0100, Quentin Schulz wrote:
>>>> This makes the axp20x_adc driver probe with platform device id
>>>> "axp813-adc".
>>>>
>>>> Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
>>>> ---
>>>>  drivers/mfd/axp20x.c | 4 +++-
>>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c
>>>> index 2468b43..42e54d1 100644
>>>> --- a/drivers/mfd/axp20x.c
>>>> +++ b/drivers/mfd/axp20x.c
>>>> @@ -878,7 +878,9 @@ static struct mfd_cell axp813_cells[] = {
>>>>              .resources              = axp803_pek_resources,
>>>>      }, {
>>>>              .name                   = "axp20x-regulator",
>>>> -    }
>>>> +    }, {
>>>> +            .name                   = "axp813-adc",
>>>> +    },
>>>
>>> Any particular reason you're not adding it to the DT?
>>>
>>
>> No, no particular reason. It's just the way it is currently for AXP209
>> and AXP22x so did the same for AXP813.
>>
>> I'll add DT "support" in next version for all AXPs supported by this
>> driver. Or is it worthy of a small separate patch series?
> 
> IIRC there's no DT support because there's no need to reference
> it in the device tree.
> 

No current need but that does not mean there won't be a need later for
drivers to map IIO channels from the ADC driver (i.e. some components
wired to GPIOs of the PMIC for example).

Quentin
-- 
Quentin Schulz, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH 3/8] mfd: axp20x: probe axp20x_adc driver for AXP813

[Chen-Yu Tsai]

On Thu, Dec 7, 2017 at 5:03 PM, Quentin Schulz
<quentin.schulz@free-electrons.com> wrote:
> Hi Chen-Yu,
>
> On 07/12/2017 09:54, Chen-Yu Tsai wrote:
>> On Thu, Dec 7, 2017 at 4:51 PM, Quentin Schulz
>> <quentin.schulz@free-electrons.com> wrote:
>>> Hi Maxime,
>>>
>>> On 05/12/2017 09:08, Maxime Ripard wrote:
>>>> On Mon, Dec 04, 2017 at 03:12:49PM +0100, Quentin Schulz wrote:
>>>>> This makes the axp20x_adc driver probe with platform device id
>>>>> "axp813-adc".
>>>>>
>>>>> Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
>>>>> ---
>>>>>  drivers/mfd/axp20x.c | 4 +++-
>>>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c
>>>>> index 2468b43..42e54d1 100644
>>>>> --- a/drivers/mfd/axp20x.c
>>>>> +++ b/drivers/mfd/axp20x.c
>>>>> @@ -878,7 +878,9 @@ static struct mfd_cell axp813_cells[] = {
>>>>>              .resources              = axp803_pek_resources,
>>>>>      }, {
>>>>>              .name                   = "axp20x-regulator",
>>>>> -    }
>>>>> +    }, {
>>>>> +            .name                   = "axp813-adc",
>>>>> +    },
>>>>
>>>> Any particular reason you're not adding it to the DT?
>>>>
>>>
>>> No, no particular reason. It's just the way it is currently for AXP209
>>> and AXP22x so did the same for AXP813.
>>>
>>> I'll add DT "support" in next version for all AXPs supported by this
>>> driver. Or is it worthy of a small separate patch series?
>>
>> IIRC there's no DT support because there's no need to reference
>> it in the device tree.
>>
>
> No current need but that does not mean there won't be a need later for
> drivers to map IIO channels from the ADC driver (i.e. some components
> wired to GPIOs of the PMIC for example).

Hmm... Why would you map the IIO channels from the ADC? I thought those
were all accessible from userspace?

However, proper muxing of the GPIO pin to the ADC function makes sense.

ChenYu

Re: [PATCH 2/8] iio: adc: axp20x_adc: add support for AXP813 ADC

[Jonathan Cameron]

On Mon,  4 Dec 2017 15:12:48 +0100
Quentin Schulz <quentin.schulz@free-electrons.com> wrote:

> The X-Powers AXP813 PMIC is really close to what is already done for
> AXP20X/AXP22X.
> 
> There are two pairs of bits to set the rate (one for Voltage and Current
> measurements and one for TS/GPIO0 voltage measurements) instead of one.

This would normally imply we need to split the device into two logical
IIO devices.  However, that only becomes relevant if we are using
buffered output which this driver doesn't support.

It'll be nasty to deal with this if we add that support down the line
though.  Up to you though as it's more likely to be your problem than
anyone else's :)

For now you could elect to support the different sampling frequencies
if you wanted to but just providing controls for each channel.

Given the driver doesn't currently expose these at all (I think)
this is all rather immaterial ;)
> 
> The register to set the ADC rates is different from the one for
> AXP20X/AXP22X.
> 
> GPIO0 can be used as an ADC (measuring Volts) unlike for AXP22X.
> 
> The scales to apply to the different inputs are unlike the ones from
> AXP20X and AXP22X.
> 
> Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>

Looks good to me.

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

I'm assuming these will probably go via MFD..

Jonathan
> ---
>  drivers/iio/adc/axp20x_adc.c | 122 ++++++++++++++++++++++++++++++++++++-
>  include/linux/mfd/axp20x.h   |   2 +-
>  2 files changed, 124 insertions(+)
> 
> diff --git a/drivers/iio/adc/axp20x_adc.c b/drivers/iio/adc/axp20x_adc.c
> index 7274f4f..03d489b 100644
> --- a/drivers/iio/adc/axp20x_adc.c
> +++ b/drivers/iio/adc/axp20x_adc.c
> @@ -35,8 +35,13 @@
>  #define AXP20X_GPIO10_IN_RANGE_GPIO1_VAL(x)	(((x) & BIT(0)) << 1)
>  
>  #define AXP20X_ADC_RATE_MASK			GENMASK(7, 6)
> +#define AXP813_V_I_ADC_RATE_MASK		GENMASK(5, 4)
> +#define AXP813_ADC_RATE_MASK			(AXP20X_ADC_RATE_MASK | AXP813_V_I_ADC_RATE_MASK)
>  #define AXP20X_ADC_RATE_HZ(x)			((ilog2((x) / 25) << 6) & AXP20X_ADC_RATE_MASK)
>  #define AXP22X_ADC_RATE_HZ(x)			((ilog2((x) / 100) << 6) & AXP20X_ADC_RATE_MASK)
> +#define AXP813_TS_GPIO0_ADC_RATE_HZ(x)		AXP20X_ADC_RATE_HZ(x)
> +#define AXP813_V_I_ADC_RATE_HZ(x)		((ilog2((x) / 100) << 4) & AXP813_V_I_ADC_RATE_MASK)
> +#define AXP813_ADC_RATE_HZ(x)			(AXP20X_ADC_RATE_HZ(x) | AXP813_V_I_ADC_RATE_HZ(x))
>  
>  #define AXP20X_ADC_CHANNEL(_channel, _name, _type, _reg)	\
>  	{							\
> @@ -95,6 +100,12 @@ enum axp22x_adc_channel_i {
>  	AXP22X_BATT_DISCHRG_I,
>  };
>  
> +enum axp813_adc_channel_v {
> +	AXP813_TS_IN = 0,
> +	AXP813_GPIO0_V,
> +	AXP813_BATT_V,
> +};
> +
>  static struct iio_map axp20x_maps[] = {
>  	{
>  		.consumer_dev_name = "axp20x-usb-power-supply",
> @@ -197,6 +208,25 @@ static const struct iio_chan_spec axp22x_adc_channels[] = {
>  			   AXP20X_BATT_DISCHRG_I_H),
>  };
>  
> +static const struct iio_chan_spec axp813_adc_channels[] = {
> +	{
> +		.type = IIO_TEMP,
> +		.address = AXP22X_PMIC_TEMP_H,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> +				      BIT(IIO_CHAN_INFO_SCALE) |
> +				      BIT(IIO_CHAN_INFO_OFFSET),
> +		.datasheet_name = "pmic_temp",
> +	},
> +	AXP20X_ADC_CHANNEL(AXP813_GPIO0_V, "gpio0_v", IIO_VOLTAGE,
> +			   AXP288_GP_ADC_H),
> +	AXP20X_ADC_CHANNEL(AXP813_BATT_V, "batt_v", IIO_VOLTAGE,
> +			   AXP20X_BATT_V_H),
> +	AXP20X_ADC_CHANNEL(AXP22X_BATT_CHRG_I, "batt_chrg_i", IIO_CURRENT,
> +			   AXP20X_BATT_CHRG_I_H),
> +	AXP20X_ADC_CHANNEL(AXP22X_BATT_DISCHRG_I, "batt_dischrg_i", IIO_CURRENT,
> +			   AXP20X_BATT_DISCHRG_I_H),
> +};
> +
>  static int axp20x_adc_raw(struct iio_dev *indio_dev,
>  			  struct iio_chan_spec const *chan, int *val)
>  {
> @@ -243,6 +273,18 @@ static int axp22x_adc_raw(struct iio_dev *indio_dev,
>  	return IIO_VAL_INT;
>  }
>  
> +static int axp813_adc_raw(struct iio_dev *indio_dev,
> +			  struct iio_chan_spec const *chan, int *val)
> +{
> +	struct axp20x_adc_iio *info = iio_priv(indio_dev);
> +
> +	*val = axp20x_read_variable_width(info->regmap, chan->address, 12);
> +	if (*val < 0)
> +		return *val;
> +
> +	return IIO_VAL_INT;
> +}
> +
>  static int axp20x_adc_scale_voltage(int channel, int *val, int *val2)
>  {
>  	switch (channel) {
> @@ -273,6 +315,24 @@ static int axp20x_adc_scale_voltage(int channel, int *val, int *val2)
>  	}
>  }
>  
> +static int axp813_adc_scale_voltage(int channel, int *val, int *val2)
> +{
> +	switch (channel) {
> +	case AXP813_GPIO0_V:
> +		*val = 0;
> +		*val2 = 800000;
> +		return IIO_VAL_INT_PLUS_MICRO;
> +
> +	case AXP813_BATT_V:
> +		*val = 1;
> +		*val2 = 100000;
> +		return IIO_VAL_INT_PLUS_MICRO;
> +
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
>  static int axp20x_adc_scale_current(int channel, int *val, int *val2)
>  {
>  	switch (channel) {
> @@ -342,6 +402,26 @@ static int axp22x_adc_scale(struct iio_chan_spec const *chan, int *val,
>  	}
>  }
>  
> +static int axp813_adc_scale(struct iio_chan_spec const *chan, int *val,
> +			    int *val2)
> +{
> +	switch (chan->type) {
> +	case IIO_VOLTAGE:
> +		return axp813_adc_scale_voltage(chan->channel, val, val2);
> +
> +	case IIO_CURRENT:
> +		*val = 1;
> +		return IIO_VAL_INT;
> +
> +	case IIO_TEMP:
> +		*val = 100;
> +		return IIO_VAL_INT;
> +
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
>  static int axp20x_adc_offset_voltage(struct iio_dev *indio_dev, int channel,
>  				     int *val)
>  {
> @@ -425,6 +505,26 @@ static int axp22x_read_raw(struct iio_dev *indio_dev,
>  	}
>  }
>  
> +static int axp813_read_raw(struct iio_dev *indio_dev,
> +			   struct iio_chan_spec const *chan, int *val,
> +			   int *val2, long mask)
> +{
> +	switch (mask) {
> +	case IIO_CHAN_INFO_OFFSET:
> +		*val = -2667;
> +		return IIO_VAL_INT;
> +
> +	case IIO_CHAN_INFO_SCALE:
> +		return axp813_adc_scale(chan, val, val2);
> +
> +	case IIO_CHAN_INFO_RAW:
> +		return axp813_adc_raw(indio_dev, chan, val);
> +
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
>  static int axp20x_write_raw(struct iio_dev *indio_dev,
>  			    struct iio_chan_spec const *chan, int val, int val2,
>  			    long mask)
> @@ -470,6 +570,10 @@ static const struct iio_info axp22x_adc_iio_info = {
>  	.read_raw = axp22x_read_raw,
>  };
>  
> +static const struct iio_info axp813_adc_iio_info = {
> +	.read_raw = axp813_read_raw,
> +};
> +
>  static int axp20x_adc_rate(struct axp20x_adc_iio *info, int rate)
>  {
>  	return regmap_update_bits(info->regmap, AXP20X_ADC_RATE,
> @@ -484,6 +588,13 @@ static int axp22x_adc_rate(struct axp20x_adc_iio *info, int rate)
>  				  AXP22X_ADC_RATE_HZ(rate));
>  }
>  
> +static int axp813_adc_rate(struct axp20x_adc_iio *info, int rate)
> +{
> +	return regmap_update_bits(info->regmap, AXP813_ADC_RATE,
> +				 AXP813_ADC_RATE_MASK,
> +				 AXP813_ADC_RATE_HZ(rate));
> +}
> +
>  struct axp_data {
>  	const struct iio_info		*iio_info;
>  	int				num_channels;
> @@ -514,9 +625,20 @@ static const struct axp_data axp22x_data = {
>  	.maps = axp22x_maps,
>  };
>  
> +static const struct axp_data axp813_data = {
> +	.iio_info = &axp813_adc_iio_info,
> +	.num_channels = ARRAY_SIZE(axp813_adc_channels),
> +	.channels = axp813_adc_channels,
> +	.adc_en1_mask = AXP22X_ADC_EN1_MASK,
> +	.adc_rate = axp813_adc_rate,
> +	.adc_en2 = false,
> +	.maps = axp22x_maps,
> +};
> +
>  static const struct platform_device_id axp20x_adc_id_match[] = {
>  	{ .name = "axp20x-adc", .driver_data = (kernel_ulong_t)&axp20x_data, },
>  	{ .name = "axp22x-adc", .driver_data = (kernel_ulong_t)&axp22x_data, },
> +	{ .name = "axp813-adc", .driver_data = (kernel_ulong_t)&axp813_data, },
>  	{ /* sentinel */ },
>  };
>  MODULE_DEVICE_TABLE(platform, axp20x_adc_id_match);
> diff --git a/include/linux/mfd/axp20x.h b/include/linux/mfd/axp20x.h
> index 78dc853..ff95414 100644
> --- a/include/linux/mfd/axp20x.h
> +++ b/include/linux/mfd/axp20x.h
> @@ -266,6 +266,8 @@ enum axp20x_variants {
>  #define AXP288_RT_BATT_V_H		0xa0
>  #define AXP288_RT_BATT_V_L		0xa1
>  
> +#define AXP813_ADC_RATE			0x85
> +
>  /* Fuel Gauge */
>  #define AXP288_FG_RDC1_REG          0xba
>  #define AXP288_FG_RDC0_REG          0xbb

Re: [linux-sunxi] [PATCH 1/8] iio: adc: axp20x_adc: put ADC rate setting in a per-variant function

[Jonathan Cameron]

On Tue, 5 Dec 2017 11:35:49 +0800
Chen-Yu Tsai <wens@csie.org> wrote:

> On Mon, Dec 4, 2017 at 10:12 PM, Quentin Schulz
> <quentin.schulz@free-electrons.com> wrote:
> > To prepare for a new comer that set a different register with different
> > values, move rate setting in a function that is specific to each AXP
> > variant.
> >
> > Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
> > ---
> >  drivers/iio/adc/axp20x_adc.c | 17 ++++++++++-------
> >  1 file changed, 10 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/iio/adc/axp20x_adc.c b/drivers/iio/adc/axp20x_adc.c
> > index a30a972..7274f4f 100644
> > --- a/drivers/iio/adc/axp20x_adc.c
> > +++ b/drivers/iio/adc/axp20x_adc.c
> > @@ -470,14 +470,18 @@ static const struct iio_info axp22x_adc_iio_info = {
> >         .read_raw = axp22x_read_raw,
> >  };
> >
> > -static int axp20x_adc_rate(int rate)
> > +static int axp20x_adc_rate(struct axp20x_adc_iio *info, int rate)
> >  {
> > -       return AXP20X_ADC_RATE_HZ(rate);
> > +       return regmap_update_bits(info->regmap, AXP20X_ADC_RATE,
> > +                                 AXP20X_ADC_RATE_MASK,
> > +                                 AXP20X_ADC_RATE_HZ(rate));
> >  }
> >
> > -static int axp22x_adc_rate(int rate)
> > +static int axp22x_adc_rate(struct axp20x_adc_iio *info, int rate)
> >  {
> > -       return AXP22X_ADC_RATE_HZ(rate);
> > +       return regmap_update_bits(info->regmap, AXP20X_ADC_RATE,
> > +                                 AXP20X_ADC_RATE_MASK,
> > +                                 AXP22X_ADC_RATE_HZ(rate));
> >  }
> >
> >  struct axp_data {
> > @@ -485,7 +489,7 @@ struct axp_data {
> >         int                             num_channels;
> >         struct iio_chan_spec const      *channels;
> >         unsigned long                   adc_en1_mask;
> > -       int                             (*adc_rate)(int rate);
> > +       int                             (*adc_rate)(struct axp20x_adc_iio *info, int rate);  
> 
> Could you also change the name of the callback, to say, adc_set_rate?
> This would make it much clearer what the callback does. Previously
> it was just a conversion helper.
> 
Agreed.

With that change you can add my
Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Thanks,

Jonathan

> ChenYu
> 
> >         bool                            adc_en2;
> >         struct iio_map                  *maps;
> >  };
> > @@ -554,8 +558,7 @@ static int axp20x_probe(struct platform_device *pdev)
> >                                    AXP20X_ADC_EN2_MASK, AXP20X_ADC_EN2_MASK);
> >
> >         /* Configure ADCs rate */
> > -       regmap_update_bits(info->regmap, AXP20X_ADC_RATE, AXP20X_ADC_RATE_MASK,
> > -                          info->data->adc_rate(100));
> > +       info->data->adc_rate(info, 100);
> >
> >         ret = iio_map_array_register(indio_dev, info->data->maps);
> >         if (ret < 0) {
> > --
> > git-series 0.9.1
> >
> > --
> > You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
> > To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe@googlegroups.com.
> > For more options, visit https://groups.google.com/d/optout.  
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/8] mfd: axp20x: probe axp20x_adc driver for AXP813

[Jonathan Cameron]

On Thu, 7 Dec 2017 17:14:30 +0800
Chen-Yu Tsai <wens@csie.org> wrote:

> On Thu, Dec 7, 2017 at 5:03 PM, Quentin Schulz
> <quentin.schulz@free-electrons.com> wrote:
> > Hi Chen-Yu,
> >
> > On 07/12/2017 09:54, Chen-Yu Tsai wrote:  
> >> On Thu, Dec 7, 2017 at 4:51 PM, Quentin Schulz
> >> <quentin.schulz@free-electrons.com> wrote:  
> >>> Hi Maxime,
> >>>
> >>> On 05/12/2017 09:08, Maxime Ripard wrote:  
> >>>> On Mon, Dec 04, 2017 at 03:12:49PM +0100, Quentin Schulz wrote:  
> >>>>> This makes the axp20x_adc driver probe with platform device id
> >>>>> "axp813-adc".
> >>>>>
> >>>>> Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
> >>>>> ---
> >>>>>  drivers/mfd/axp20x.c | 4 +++-
> >>>>>  1 file changed, 3 insertions(+), 1 deletion(-)
> >>>>>
> >>>>> diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c
> >>>>> index 2468b43..42e54d1 100644
> >>>>> --- a/drivers/mfd/axp20x.c
> >>>>> +++ b/drivers/mfd/axp20x.c
> >>>>> @@ -878,7 +878,9 @@ static struct mfd_cell axp813_cells[] = {
> >>>>>              .resources              = axp803_pek_resources,
> >>>>>      }, {
> >>>>>              .name                   = "axp20x-regulator",
> >>>>> -    }
> >>>>> +    }, {
> >>>>> +            .name                   = "axp813-adc",
> >>>>> +    },  
> >>>>
> >>>> Any particular reason you're not adding it to the DT?
> >>>>  
> >>>
> >>> No, no particular reason. It's just the way it is currently for AXP209
> >>> and AXP22x so did the same for AXP813.
> >>>
> >>> I'll add DT "support" in next version for all AXPs supported by this
> >>> driver. Or is it worthy of a small separate patch series?  
> >>
> >> IIRC there's no DT support because there's no need to reference
> >> it in the device tree.
> >>  
> >
> > No current need but that does not mean there won't be a need later for
> > drivers to map IIO channels from the ADC driver (i.e. some components
> > wired to GPIOs of the PMIC for example).  
> 
> Hmm... Why would you map the IIO channels from the ADC? I thought those
> were all accessible from userspace?

There is a reasonably fully featured consumer interface for IIO channels
as well. Here it's being used internal to the hardware, but yes if
you want to do the mappings to other devices, it will need to 'exist'
in the device tree.

I'm guessing that you have something in mind that needs this.  If not I'd
leave it until there is a real user.

> 
> However, proper muxing of the GPIO pin to the ADC function makes sense.
> 
Agreed.

Jonathan

> ChenYu
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/8] dt-bindings: power: supply: axp20x: add AXP813 battery DT binding

[Jonathan Cameron]

On Mon,  4 Dec 2017 15:12:50 +0100
Quentin Schulz <quentin.schulz@free-electrons.com> wrote:

> The AXP813 can have a battery as power supply, so let's add it to the
> list of compatibles.
> 
> Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
> ---
>  Documentation/devicetree/bindings/power/supply/axp20x_battery.txt | 8 +++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/power/supply/axp20x_battery.txt b/Documentation/devicetree/bindings/power/supply/axp20x_battery.txt
> index c248866..4614c8e 100644
> --- a/Documentation/devicetree/bindings/power/supply/axp20x_battery.txt
> +++ b/Documentation/devicetree/bindings/power/supply/axp20x_battery.txt
> @@ -4,12 +4,12 @@ Required Properties:
>   - compatible, one of:
>  			"x-powers,axp209-battery-power-supply"
>  			"x-powers,axp221-battery-power-supply"
> +			"x-powers,axp813-battery-power-supply"
>  
> -This node is a subnode of the axp20x/axp22x PMIC.
> +This node is a subnode of the axp20x/axp22x/axp81x PMIC.
>  
> -The AXP20X and AXP22X can read the battery voltage, charge and discharge
> -currents of the battery by reading ADC channels from the AXP20X/AXP22X
> -ADC.
> +The AXP20X, AXP22X and AXP81X can read the battery voltage, charge and
> +discharge currents of the battery by reading ADC channels from the ADC.
Might just be me, but this looks like a recipe for unneeded churn in future.

The supported devices can read...

Maybe also

This node is a subnode of the PMIC with the same part number. ?

I don't really care though!

>  
>  Example:
>  

Re: [PATCH 5/8] power: supply: axp20x_battery: add support for AXP813

[Jonathan Cameron]

On Mon,  4 Dec 2017 15:12:51 +0100
Quentin Schulz <quentin.schulz@free-electrons.com> wrote:

> The X-Powers AXP813 PMIC has got some slight differences from
> AXP20X/AXP22X PMICs:
>  - the maximum voltage supplied by the PMIC is 4.35 instead of 4.36/4.24
>  for AXP20X/AXP22X,
>  - the constant charge current formula is different,
> 
> It also has a bit to tell whether the battery percentage returned by the
> PMIC is valid.
> 
> Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>

I'd use switch statements when matching the IDs as that'll be more elegant
as you perhaps add further devices going forward...

Other than that, looks good to me.

Jonathan

> ---
>  drivers/power/supply/axp20x_battery.c | 44 +++++++++++++++++++++++++++-
>  1 file changed, 43 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/power/supply/axp20x_battery.c b/drivers/power/supply/axp20x_battery.c
> index 7494f0f..cb30302 100644
> --- a/drivers/power/supply/axp20x_battery.c
> +++ b/drivers/power/supply/axp20x_battery.c
> @@ -46,6 +46,8 @@
>  #define AXP20X_CHRG_CTRL1_TGT_4_2V	(2 << 5)
>  #define AXP20X_CHRG_CTRL1_TGT_4_36V	(3 << 5)
>  
> +#define AXP813_CHRG_CTRL1_TGT_4_35V	(3 << 5)
> +
>  #define AXP22X_CHRG_CTRL1_TGT_4_22V	(1 << 5)
>  #define AXP22X_CHRG_CTRL1_TGT_4_24V	(3 << 5)
>  
> @@ -123,10 +125,41 @@ static int axp22x_battery_get_max_voltage(struct axp20x_batt_ps *axp20x_batt,
>  	return 0;
>  }
>  
> +static int axp813_battery_get_max_voltage(struct axp20x_batt_ps *axp20x_batt,
> +					  int *val)
> +{
> +	int ret, reg;
> +
> +	ret = regmap_read(axp20x_batt->regmap, AXP20X_CHRG_CTRL1, &reg);
> +	if (ret)
> +		return ret;
> +
> +	switch (reg & AXP20X_CHRG_CTRL1_TGT_VOLT) {

You could do a lookup based from a table instead which might
be ever so slightly more elegant..

> +	case AXP20X_CHRG_CTRL1_TGT_4_1V:
> +		*val = 4100000;
> +		break;
> +	case AXP20X_CHRG_CTRL1_TGT_4_15V:
> +		*val = 4150000;
> +		break;
> +	case AXP20X_CHRG_CTRL1_TGT_4_2V:
> +		*val = 4200000;
> +		break;
> +	case AXP813_CHRG_CTRL1_TGT_4_35V:
> +		*val = 4350000;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
>  static void raw_to_constant_charge_current(struct axp20x_batt_ps *axp, int *val)
>  {
>  	if (axp->axp_id == AXP209_ID)
>  		*val = *val * 100000 + 300000;
> +	else if (axp->axp_id == AXP813_ID)
> +		*val = *val * 200000 + 200000;
>  	else
>  		*val = *val * 150000 + 300000;

Switch?

>  }
> @@ -135,6 +168,8 @@ static void constant_charge_current_to_raw(struct axp20x_batt_ps *axp, int *val)
>  {
>  	if (axp->axp_id == AXP209_ID)
>  		*val = (*val - 300000) / 100000;
> +	else if (axp->axp_id == AXP813_ID)
> +		*val = (*val - 200000) / 200000;
>  	else
>  		*val = (*val - 300000) / 150000;
>  }
> @@ -269,7 +304,8 @@ static int axp20x_battery_get_prop(struct power_supply *psy,
>  		if (ret)
>  			return ret;
>  
> -		if (axp20x_batt->axp_id == AXP221_ID &&
> +		if ((axp20x_batt->axp_id == AXP221_ID ||
> +		     axp20x_batt->axp_id == AXP813_ID) &&
>  		    !(reg & AXP22X_FG_VALID))
>  			return -EINVAL;
>  
> @@ -284,6 +320,9 @@ static int axp20x_battery_get_prop(struct power_supply *psy,
>  		if (axp20x_batt->axp_id == AXP209_ID)
>  			return axp20x_battery_get_max_voltage(axp20x_batt,
>  							      &val->intval);
> +		else if (axp20x_batt->axp_id == AXP813_ID)
> +			return axp813_battery_get_max_voltage(axp20x_batt,
> +							      &val->intval);
>  		return axp22x_battery_get_max_voltage(axp20x_batt,
>  						      &val->intval);

Worth converting to a switch statement to make it more elegant for future
devices?

>  
> @@ -467,6 +506,9 @@ static const struct of_device_id axp20x_battery_ps_id[] = {
>  	}, {
>  		.compatible = "x-powers,axp221-battery-power-supply",
>  		.data = (void *)AXP221_ID,
> +	}, {
> +		.compatible = "x-powers,axp813-battery-power-supply",
> +		.data = (void *)AXP813_ID,
>  	}, { /* sentinel */ },
>  };
>  MODULE_DEVICE_TABLE(of, axp20x_battery_ps_id);

Re: [PATCH 2/8] iio: adc: axp20x_adc: add support for AXP813 ADC

[Quentin Schulz]

Hi Jonathan,

On 10/12/2017 17:36, Jonathan Cameron wrote:
> On Mon,  4 Dec 2017 15:12:48 +0100
> Quentin Schulz <quentin.schulz@free-electrons.com> wrote:
> 
>> The X-Powers AXP813 PMIC is really close to what is already done for
>> AXP20X/AXP22X.
>>
>> There are two pairs of bits to set the rate (one for Voltage and Current
>> measurements and one for TS/GPIO0 voltage measurements) instead of one.
> 
> This would normally imply we need to split the device into two logical
> IIO devices.  However, that only becomes relevant if we are using
> buffered output which this driver doesn't support.
> > It'll be nasty to deal with this if we add that support down the line
> though.  Up to you though as it's more likely to be your problem than
> anyone else's :)
> 

I have no plans for supporting buffered output for the AXPs at the
moment. But that's an interesting (and important) limitation to raise.
Wouldn't be more of a hack to have two IIO devices representing the
actual same IP?

> For now you could elect to support the different sampling frequencies
> if you wanted to but just providing controls for each channel.
> 

I guess that you're offering to use IIO_CHAN_INFO_SAMP_FREQ in
info_mask_separate for each channel?

> Given the driver doesn't currently expose these at all (I think)
> this is all rather immaterial ;)

I'm not giving the user the option to chose the sampling frequency for
now. I have no plans to do it either, but I think it would be rather
simple to later add support for setting frequency sampling since we only
need to add a sysfs entry (with IIO_CHAN_INFO_SAMP_FREQ) that does not
exist yet. Don't you think? Am I missing something?

Thanks,
Quentin
-- 
Quentin Schulz, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH 5/8] power: supply: axp20x_battery: add support for AXP813

[Quentin Schulz]

Hi Jonathan,

On 10/12/2017 17:49, Jonathan Cameron wrote:
> On Mon,  4 Dec 2017 15:12:51 +0100
> Quentin Schulz <quentin.schulz@free-electrons.com> wrote:
> 
>> The X-Powers AXP813 PMIC has got some slight differences from
>> AXP20X/AXP22X PMICs:
>>  - the maximum voltage supplied by the PMIC is 4.35 instead of 4.36/4.24
>>  for AXP20X/AXP22X,
>>  - the constant charge current formula is different,
>>
>> It also has a bit to tell whether the battery percentage returned by the
>> PMIC is valid.
>>
>> Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
> 
> I'd use switch statements when matching the IDs as that'll be more elegant
> as you perhaps add further devices going forward...
> 
> Other than that, looks good to me.
> 

Well, I was wondering if it shouldn't be better to define a structure
for each device containing their quirks, functions, etc... like it is
done for the ADC or the ACIN power supply driver part.

struct axp20x_data {
	bool	has_valid_fg_reg;
	void 	constant_charge_current_to_raw(struct axp20x_batt_ps *axp, int *val);
	void 	raw_to_constant_charge_current(struct axp20x_batt_ps *axp, int *val);
	int 	get_max_voltage(struct axp20x_batt_ps *axp, int *val);
	[...]
};

static const struct of_device_id axp20x_battery_ps_id[] = {
	{ .compatible = "x-powers,axp209-battery-power-supply", .data = (void
*)&axp209_data, }, {}
};

void probe()
{
	[...]
	axp20x_batt->info = of_device_get_match_data(&pdev->dev);
	[...]
}

Sebastian, any objection on doing this?

Thanks,
Quentin

> Jonathan
> 
>> ---
>>  drivers/power/supply/axp20x_battery.c | 44 +++++++++++++++++++++++++++-
>>  1 file changed, 43 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/power/supply/axp20x_battery.c b/drivers/power/supply/axp20x_battery.c
>> index 7494f0f..cb30302 100644
>> --- a/drivers/power/supply/axp20x_battery.c
>> +++ b/drivers/power/supply/axp20x_battery.c
>> @@ -46,6 +46,8 @@
>>  #define AXP20X_CHRG_CTRL1_TGT_4_2V	(2 << 5)
>>  #define AXP20X_CHRG_CTRL1_TGT_4_36V	(3 << 5)
>>  
>> +#define AXP813_CHRG_CTRL1_TGT_4_35V	(3 << 5)
>> +
>>  #define AXP22X_CHRG_CTRL1_TGT_4_22V	(1 << 5)
>>  #define AXP22X_CHRG_CTRL1_TGT_4_24V	(3 << 5)
>>  
>> @@ -123,10 +125,41 @@ static int axp22x_battery_get_max_voltage(struct axp20x_batt_ps *axp20x_batt,
>>  	return 0;
>>  }
>>  
>> +static int axp813_battery_get_max_voltage(struct axp20x_batt_ps *axp20x_batt,
>> +					  int *val)
>> +{
>> +	int ret, reg;
>> +
>> +	ret = regmap_read(axp20x_batt->regmap, AXP20X_CHRG_CTRL1, &reg);
>> +	if (ret)
>> +		return ret;
>> +
>> +	switch (reg & AXP20X_CHRG_CTRL1_TGT_VOLT) {
> 
> You could do a lookup based from a table instead which might
> be ever so slightly more elegant..
> 
>> +	case AXP20X_CHRG_CTRL1_TGT_4_1V:
>> +		*val = 4100000;
>> +		break;
>> +	case AXP20X_CHRG_CTRL1_TGT_4_15V:
>> +		*val = 4150000;
>> +		break;
>> +	case AXP20X_CHRG_CTRL1_TGT_4_2V:
>> +		*val = 4200000;
>> +		break;
>> +	case AXP813_CHRG_CTRL1_TGT_4_35V:
>> +		*val = 4350000;
>> +		break;
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>>  static void raw_to_constant_charge_current(struct axp20x_batt_ps *axp, int *val)
>>  {
>>  	if (axp->axp_id == AXP209_ID)
>>  		*val = *val * 100000 + 300000;
>> +	else if (axp->axp_id == AXP813_ID)
>> +		*val = *val * 200000 + 200000;
>>  	else
>>  		*val = *val * 150000 + 300000;
> 
> Switch?
> 
>>  }
>> @@ -135,6 +168,8 @@ static void constant_charge_current_to_raw(struct axp20x_batt_ps *axp, int *val)
>>  {
>>  	if (axp->axp_id == AXP209_ID)
>>  		*val = (*val - 300000) / 100000;
>> +	else if (axp->axp_id == AXP813_ID)
>> +		*val = (*val - 200000) / 200000;
>>  	else
>>  		*val = (*val - 300000) / 150000;
>>  }
>> @@ -269,7 +304,8 @@ static int axp20x_battery_get_prop(struct power_supply *psy,
>>  		if (ret)
>>  			return ret;
>>  
>> -		if (axp20x_batt->axp_id == AXP221_ID &&
>> +		if ((axp20x_batt->axp_id == AXP221_ID ||
>> +		     axp20x_batt->axp_id == AXP813_ID) &&
>>  		    !(reg & AXP22X_FG_VALID))
>>  			return -EINVAL;
>>  
>> @@ -284,6 +320,9 @@ static int axp20x_battery_get_prop(struct power_supply *psy,
>>  		if (axp20x_batt->axp_id == AXP209_ID)
>>  			return axp20x_battery_get_max_voltage(axp20x_batt,
>>  							      &val->intval);
>> +		else if (axp20x_batt->axp_id == AXP813_ID)
>> +			return axp813_battery_get_max_voltage(axp20x_batt,
>> +							      &val->intval);
>>  		return axp22x_battery_get_max_voltage(axp20x_batt,
>>  						      &val->intval);
> 
> Worth converting to a switch statement to make it more elegant for future
> devices?
> 
>>  
>> @@ -467,6 +506,9 @@ static const struct of_device_id axp20x_battery_ps_id[] = {
>>  	}, {
>>  		.compatible = "x-powers,axp221-battery-power-supply",
>>  		.data = (void *)AXP221_ID,
>> +	}, {
>> +		.compatible = "x-powers,axp813-battery-power-supply",
>> +		.data = (void *)AXP813_ID,
>>  	}, { /* sentinel */ },
>>  };
>>  MODULE_DEVICE_TABLE(of, axp20x_battery_ps_id);
> 

-- 
Quentin Schulz, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH 2/8] iio: adc: axp20x_adc: add support for AXP813 ADC

[Jonathan Cameron]

On Mon, 11 Dec 2017 09:18:55 +0100
Quentin Schulz <quentin.schulz@free-electrons.com> wrote:

> Hi Jonathan,
> 
> On 10/12/2017 17:36, Jonathan Cameron wrote:
> > On Mon,  4 Dec 2017 15:12:48 +0100
> > Quentin Schulz <quentin.schulz@free-electrons.com> wrote:
> >   
> >> The X-Powers AXP813 PMIC is really close to what is already done for
> >> AXP20X/AXP22X.
> >>
> >> There are two pairs of bits to set the rate (one for Voltage and Current
> >> measurements and one for TS/GPIO0 voltage measurements) instead of one.  
> > 
> > This would normally imply we need to split the device into two logical
> > IIO devices.  However, that only becomes relevant if we are using
> > buffered output which this driver doesn't support.  
> > > It'll be nasty to deal with this if we add that support down the line  
> > though.  Up to you though as it's more likely to be your problem than
> > anyone else's :)
> >   
> 
> I have no plans for supporting buffered output for the AXPs at the
> moment. But that's an interesting (and important) limitation to raise.
> Wouldn't be more of a hack to have two IIO devices representing the
> actual same IP?

We have thought about allowing multiple buffers from a single IIO device
but that makes for some horrible changes to the ABI - so as things stand
the only option is two devices for one IP.  Ultimately they aren't really
two devices - in the same way we have triggers separating registered on
the IIO bus (often many of them).  Just two different elements of the same IP.

> 
> > For now you could elect to support the different sampling frequencies
> > if you wanted to but just providing controls for each channel.
> >   
> 
> I guess that you're offering to use IIO_CHAN_INFO_SAMP_FREQ in
> info_mask_separate for each channel?
Yes
> 
> > Given the driver doesn't currently expose these at all (I think)
> > this is all rather immaterial ;)  
> 
> I'm not giving the user the option to chose the sampling frequency for
> now. I have no plans to do it either, but I think it would be rather
> simple to later add support for setting frequency sampling since we only
> need to add a sysfs entry (with IIO_CHAN_INFO_SAMP_FREQ) that does not
> exist yet. Don't you think? Am I missing something?
No should be straight forward as long as we keep clear of the buffered
interfaces with their limitations.

> 
> Thanks,
> Quentin

Re: [PATCH 5/8] power: supply: axp20x_battery: add support for AXP813

[Jonathan Cameron]

On Mon, 11 Dec 2017 09:35:43 +0100
Quentin Schulz <quentin.schulz@free-electrons.com> wrote:

> Hi Jonathan,
> 
> On 10/12/2017 17:49, Jonathan Cameron wrote:
> > On Mon,  4 Dec 2017 15:12:51 +0100
> > Quentin Schulz <quentin.schulz@free-electrons.com> wrote:
> >   
> >> The X-Powers AXP813 PMIC has got some slight differences from
> >> AXP20X/AXP22X PMICs:
> >>  - the maximum voltage supplied by the PMIC is 4.35 instead of 4.36/4.24
> >>  for AXP20X/AXP22X,
> >>  - the constant charge current formula is different,
> >>
> >> It also has a bit to tell whether the battery percentage returned by the
> >> PMIC is valid.
> >>
> >> Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>  
> > 
> > I'd use switch statements when matching the IDs as that'll be more elegant
> > as you perhaps add further devices going forward...
> > 
> > Other than that, looks good to me.
> >   
> 
> Well, I was wondering if it shouldn't be better to define a structure
> for each device containing their quirks, functions, etc... like it is
> done for the ADC or the ACIN power supply driver part.
> 

Even better.

> struct axp20x_data {
> 	bool	has_valid_fg_reg;
> 	void 	constant_charge_current_to_raw(struct axp20x_batt_ps *axp, int *val);
> 	void 	raw_to_constant_charge_current(struct axp20x_batt_ps *axp, int *val);
> 	int 	get_max_voltage(struct axp20x_batt_ps *axp, int *val);
> 	[...]
> };
> 
> static const struct of_device_id axp20x_battery_ps_id[] = {
> 	{ .compatible = "x-powers,axp209-battery-power-supply", .data = (void
> *)&axp209_data, }, {}
> };
> 
> void probe()
> {
> 	[...]
> 	axp20x_batt->info = of_device_get_match_data(&pdev->dev);
> 	[...]
> }
> 
> Sebastian, any objection on doing this?
> 
> Thanks,
> Quentin
> 
> > Jonathan
> >   
> >> ---
> >>  drivers/power/supply/axp20x_battery.c | 44 +++++++++++++++++++++++++++-
> >>  1 file changed, 43 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/power/supply/axp20x_battery.c b/drivers/power/supply/axp20x_battery.c
> >> index 7494f0f..cb30302 100644
> >> --- a/drivers/power/supply/axp20x_battery.c
> >> +++ b/drivers/power/supply/axp20x_battery.c
> >> @@ -46,6 +46,8 @@
> >>  #define AXP20X_CHRG_CTRL1_TGT_4_2V	(2 << 5)
> >>  #define AXP20X_CHRG_CTRL1_TGT_4_36V	(3 << 5)
> >>  
> >> +#define AXP813_CHRG_CTRL1_TGT_4_35V	(3 << 5)
> >> +
> >>  #define AXP22X_CHRG_CTRL1_TGT_4_22V	(1 << 5)
> >>  #define AXP22X_CHRG_CTRL1_TGT_4_24V	(3 << 5)
> >>  
> >> @@ -123,10 +125,41 @@ static int axp22x_battery_get_max_voltage(struct axp20x_batt_ps *axp20x_batt,
> >>  	return 0;
> >>  }
> >>  
> >> +static int axp813_battery_get_max_voltage(struct axp20x_batt_ps *axp20x_batt,
> >> +					  int *val)
> >> +{
> >> +	int ret, reg;
> >> +
> >> +	ret = regmap_read(axp20x_batt->regmap, AXP20X_CHRG_CTRL1, &reg);
> >> +	if (ret)
> >> +		return ret;
> >> +
> >> +	switch (reg & AXP20X_CHRG_CTRL1_TGT_VOLT) {  
> > 
> > You could do a lookup based from a table instead which might
> > be ever so slightly more elegant..
> >   
> >> +	case AXP20X_CHRG_CTRL1_TGT_4_1V:
> >> +		*val = 4100000;
> >> +		break;
> >> +	case AXP20X_CHRG_CTRL1_TGT_4_15V:
> >> +		*val = 4150000;
> >> +		break;
> >> +	case AXP20X_CHRG_CTRL1_TGT_4_2V:
> >> +		*val = 4200000;
> >> +		break;
> >> +	case AXP813_CHRG_CTRL1_TGT_4_35V:
> >> +		*val = 4350000;
> >> +		break;
> >> +	default:
> >> +		return -EINVAL;
> >> +	}
> >> +
> >> +	return 0;
> >> +}
> >> +
> >>  static void raw_to_constant_charge_current(struct axp20x_batt_ps *axp, int *val)
> >>  {
> >>  	if (axp->axp_id == AXP209_ID)
> >>  		*val = *val * 100000 + 300000;
> >> +	else if (axp->axp_id == AXP813_ID)
> >> +		*val = *val * 200000 + 200000;
> >>  	else
> >>  		*val = *val * 150000 + 300000;  
> > 
> > Switch?
> >   
> >>  }
> >> @@ -135,6 +168,8 @@ static void constant_charge_current_to_raw(struct axp20x_batt_ps *axp, int *val)
> >>  {
> >>  	if (axp->axp_id == AXP209_ID)
> >>  		*val = (*val - 300000) / 100000;
> >> +	else if (axp->axp_id == AXP813_ID)
> >> +		*val = (*val - 200000) / 200000;
> >>  	else
> >>  		*val = (*val - 300000) / 150000;
> >>  }
> >> @@ -269,7 +304,8 @@ static int axp20x_battery_get_prop(struct power_supply *psy,
> >>  		if (ret)
> >>  			return ret;
> >>  
> >> -		if (axp20x_batt->axp_id == AXP221_ID &&
> >> +		if ((axp20x_batt->axp_id == AXP221_ID ||
> >> +		     axp20x_batt->axp_id == AXP813_ID) &&
> >>  		    !(reg & AXP22X_FG_VALID))
> >>  			return -EINVAL;
> >>  
> >> @@ -284,6 +320,9 @@ static int axp20x_battery_get_prop(struct power_supply *psy,
> >>  		if (axp20x_batt->axp_id == AXP209_ID)
> >>  			return axp20x_battery_get_max_voltage(axp20x_batt,
> >>  							      &val->intval);
> >> +		else if (axp20x_batt->axp_id == AXP813_ID)
> >> +			return axp813_battery_get_max_voltage(axp20x_batt,
> >> +							      &val->intval);
> >>  		return axp22x_battery_get_max_voltage(axp20x_batt,
> >>  						      &val->intval);  
> > 
> > Worth converting to a switch statement to make it more elegant for future
> > devices?
> >   
> >>  
> >> @@ -467,6 +506,9 @@ static const struct of_device_id axp20x_battery_ps_id[] = {
> >>  	}, {
> >>  		.compatible = "x-powers,axp221-battery-power-supply",
> >>  		.data = (void *)AXP221_ID,
> >> +	}, {
> >> +		.compatible = "x-powers,axp813-battery-power-supply",
> >> +		.data = (void *)AXP813_ID,
> >>  	}, { /* sentinel */ },
> >>  };
> >>  MODULE_DEVICE_TABLE(of, axp20x_battery_ps_id);  
> >   
>