mbox series

[00/10] Add support for AXP192 PMIC

Message ID 20220603135714.12007-1-aidanmacdonald.0x0@gmail.com
Headers show
Series Add support for AXP192 PMIC | expand

Message

Aidan MacDonald June 3, 2022, 1:57 p.m. UTC
Hi all,

This patch series adds support for the X-Powers AXP192 PMIC to the
AXP20x driver framework.

The first patch is a small change to regmap-irq to support the AXP192's
unusual IRQ register layout. It isn't possible to include all of the
IRQ registers in one regmap-irq chip without this.

The rest of the changes are pretty straightforward, I think the only
notable parts are the axp20x_adc driver where there seems to be some
opportunities for code reuse (the axp192 is nearly a duplicate of the
axp20x) and the addition of a new pinctrl driver for the axp192, since
the axp20x pinctrl driver was not very easy to adapt.

Aidan MacDonald (10):
  regmap-irq: Add get_irq_reg to support unusual register layouts
  dt-bindings: mfd: add bindings for AXP192 MFD device
  dt-bindings: iio: adc: axp209: Add AXP192 compatible
  dt-bindings: power: supply: axp20x: Add AXP192 compatible
  dt-bindings: gpio: Add AXP192 GPIO bindings
  mfd: axp20x: Add support for AXP192
  regulator: axp20x: Add support for AXP192
  iio: adc: axp20x_adc: Add support for AXP192
  power: supply: axp20x_usb_power: Add support for AXP192
  pinctrl: Add AXP192 pin control driver

 .../bindings/gpio/x-powers,axp192-gpio.yaml   |  59 ++
 .../bindings/iio/adc/x-powers,axp209-adc.yaml |  18 +
 .../bindings/mfd/x-powers,axp152.yaml         |   1 +
 .../x-powers,axp20x-usb-power-supply.yaml     |   1 +
 drivers/base/regmap/regmap-irq.c              |  19 +-
 drivers/iio/adc/axp20x_adc.c                  | 289 ++++++++-
 drivers/mfd/axp20x-i2c.c                      |   2 +
 drivers/mfd/axp20x.c                          | 150 +++++
 drivers/pinctrl/Kconfig                       |  14 +
 drivers/pinctrl/Makefile                      |   1 +
 drivers/pinctrl/pinctrl-axp192.c              | 589 ++++++++++++++++++
 drivers/power/supply/axp20x_usb_power.c       |  75 ++-
 drivers/regulator/axp20x-regulator.c          | 101 ++-
 include/linux/mfd/axp20x.h                    |  84 +++
 include/linux/regmap.h                        |   5 +
 15 files changed, 1375 insertions(+), 33 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/gpio/x-powers,axp192-gpio.yaml
 create mode 100644 drivers/pinctrl/pinctrl-axp192.c

Comments

Jonathan Cameron June 3, 2022, 4:47 p.m. UTC | #1
On Fri,  3 Jun 2022 14:57:12 +0100
Aidan MacDonald <aidanmacdonald.0x0@gmail.com> wrote:

> The AXP192 is identical to the AXP20x, except for the addition of
> two more GPIO ADC channels.
> 
> Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
Hi Aidan,

A few minor questions and comments inline.

Thanks,

Jonathan

> ---
>  drivers/iio/adc/axp20x_adc.c | 289 +++++++++++++++++++++++++++++++++--
>  1 file changed, 280 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/iio/adc/axp20x_adc.c b/drivers/iio/adc/axp20x_adc.c
> index 53bf7d4899d2..7d2bf9529420 100644
> --- a/drivers/iio/adc/axp20x_adc.c
> +++ b/drivers/iio/adc/axp20x_adc.c
> @@ -21,6 +21,9 @@
>  #include <linux/iio/machine.h>
>  #include <linux/mfd/axp20x.h>
>  
> +#define AXP192_ADC_EN1_MASK			GENMASK(7, 0)
> +#define AXP192_ADC_EN2_MASK			(BIT(7) | GENMASK(3, 0))

Obviously doesn't really mater, but for consistency, maybe match the
ordering in AXP20X_ADC_EN2_MASK below?

> +
>  #define AXP20X_ADC_EN1_MASK			GENMASK(7, 0)
>  
>  #define AXP20X_ADC_EN2_MASK			(GENMASK(3, 2) | BIT(7))
> @@ -31,6 +34,15 @@
>  #define AXP20X_GPIO10_IN_RANGE_GPIO0_VAL(x)	((x) & BIT(0))
>  #define AXP20X_GPIO10_IN_RANGE_GPIO1_VAL(x)	(((x) & BIT(0)) << 1)
>  
> +#define AXP192_GPIO30_IN_RANGE_GPIO0		BIT(0)
> +#define AXP192_GPIO30_IN_RANGE_GPIO1		BIT(1)
> +#define AXP192_GPIO30_IN_RANGE_GPIO2		BIT(2)
> +#define AXP192_GPIO30_IN_RANGE_GPIO3		BIT(3)
> +#define AXP192_GPIO30_IN_RANGE_GPIO0_VAL(x)	((x) & BIT(0))
> +#define AXP192_GPIO30_IN_RANGE_GPIO1_VAL(x)	(((x) & BIT(0)) << 1)
> +#define AXP192_GPIO30_IN_RANGE_GPIO2_VAL(x)	(((x) & BIT(0)) << 2)
> +#define AXP192_GPIO30_IN_RANGE_GPIO3_VAL(x)	(((x) & BIT(0)) << 3)

FIELD_PREP() using the masks is cleaner than hand rolling it.
If you fancy updating the driver in general whilst here that would be great,
but I don't mind if not.

> +
>  #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)
> @@ -70,6 +82,25 @@ struct axp20x_adc_iio {
>  	const struct axp_data	*data;
>  };
>  
> +enum axp192_adc_channel_v {
> +	AXP192_ACIN_V = 0,
> +	AXP192_VBUS_V,
> +	AXP192_TS_IN,
> +	AXP192_GPIO0_V,
> +	AXP192_GPIO1_V,
> +	AXP192_GPIO2_V,
> +	AXP192_GPIO3_V,
> +	AXP192_IPSOUT_V,
> +	AXP192_BATT_V,
> +};
> +
> +enum axp192_adc_channel_i {
> +	AXP192_ACIN_I = 0,
> +	AXP192_VBUS_I,
> +	AXP192_BATT_CHRG_I,
> +	AXP192_BATT_DISCHRG_I,
> +};
> +
>  enum axp20x_adc_channel_v {
>  	AXP20X_ACIN_V = 0,
>  	AXP20X_VBUS_V,
> @@ -157,6 +188,43 @@ static struct iio_map axp22x_maps[] = {
>   * The only exception is for the battery. batt_v will be in_voltage6_raw and
>   * charge current in_current6_raw and discharge current will be in_current7_raw.
>   */
> +static const struct iio_chan_spec axp192_adc_channels[] = {
> +	AXP20X_ADC_CHANNEL(AXP192_ACIN_V, "acin_v", IIO_VOLTAGE,
> +			   AXP20X_ACIN_V_ADC_H),
> +	AXP20X_ADC_CHANNEL(AXP192_ACIN_I, "acin_i", IIO_CURRENT,
> +			   AXP20X_ACIN_I_ADC_H),
> +	AXP20X_ADC_CHANNEL(AXP192_VBUS_V, "vbus_v", IIO_VOLTAGE,
> +			   AXP20X_VBUS_V_ADC_H),
> +	AXP20X_ADC_CHANNEL(AXP192_VBUS_I, "vbus_i", IIO_CURRENT,
> +			   AXP20X_VBUS_I_ADC_H),
> +	{
> +		.type = IIO_TEMP,
> +		.address = AXP20X_TEMP_ADC_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_OFFSET(AXP192_GPIO0_V, "gpio0_v", IIO_VOLTAGE,
> +				  AXP20X_GPIO0_V_ADC_H),
> +	AXP20X_ADC_CHANNEL_OFFSET(AXP192_GPIO1_V, "gpio1_v", IIO_VOLTAGE,
> +				  AXP20X_GPIO1_V_ADC_H),
> +	AXP20X_ADC_CHANNEL_OFFSET(AXP192_GPIO2_V, "gpio2_v", IIO_VOLTAGE,
> +				  AXP192_GPIO2_V_ADC_H),
> +	AXP20X_ADC_CHANNEL_OFFSET(AXP192_GPIO3_V, "gpio3_v", IIO_VOLTAGE,
> +				  AXP192_GPIO3_V_ADC_H),
> +	AXP20X_ADC_CHANNEL(AXP192_IPSOUT_V, "ipsout_v", IIO_VOLTAGE,
> +			   AXP20X_IPSOUT_V_HIGH_H),
> +	AXP20X_ADC_CHANNEL(AXP192_BATT_V, "batt_v", IIO_VOLTAGE,
> +			   AXP20X_BATT_V_H),
> +	AXP20X_ADC_CHANNEL(AXP192_BATT_CHRG_I, "batt_chrg_i", IIO_CURRENT,
> +			   AXP20X_BATT_CHRG_I_H),
> +	AXP20X_ADC_CHANNEL(AXP192_BATT_DISCHRG_I, "batt_dischrg_i", IIO_CURRENT,
> +			   AXP20X_BATT_DISCHRG_I_H),
> +	AXP20X_ADC_CHANNEL(AXP192_TS_IN, "ts_v", IIO_VOLTAGE,
> +			   AXP20X_TS_IN_H),
> +};
> +
>  static const struct iio_chan_spec axp20x_adc_channels[] = {
>  	AXP20X_ADC_CHANNEL(AXP20X_ACIN_V, "acin_v", IIO_VOLTAGE,
>  			   AXP20X_ACIN_V_ADC_H),
> @@ -277,6 +345,44 @@ static int axp813_adc_raw(struct iio_dev *indio_dev,
>  	return IIO_VAL_INT;
>  }
>  
> +static int axp192_adc_scale_voltage(int channel, int *val, int *val2)
> +{
> +	switch (channel) {
> +	case AXP192_ACIN_V:
> +	case AXP192_VBUS_V:
> +		*val = 1;
> +		*val2 = 700000;
> +		return IIO_VAL_INT_PLUS_MICRO;
> +
> +	case AXP192_GPIO0_V:
> +	case AXP192_GPIO1_V:
> +	case AXP192_GPIO2_V:
> +	case AXP192_GPIO3_V:
> +		*val = 0;
> +		*val2 = 500000;
> +		return IIO_VAL_INT_PLUS_MICRO;
> +
> +	case AXP192_BATT_V:
> +		*val = 1;
> +		*val2 = 100000;
> +		return IIO_VAL_INT_PLUS_MICRO;
> +
> +	case AXP192_IPSOUT_V:
> +		*val = 1;
> +		*val2 = 400000;
> +		return IIO_VAL_INT_PLUS_MICRO;
> +
> +	case AXP192_TS_IN:
> +		/* 0.8 mV per LSB */
> +		*val = 0;
> +		*val2 = 800000;
> +		return IIO_VAL_INT_PLUS_MICRO;
> +
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
>  static int axp20x_adc_scale_voltage(int channel, int *val, int *val2)
>  {
>  	switch (channel) {
> @@ -380,6 +486,29 @@ static int axp20x_adc_scale_current(int channel, int *val, int *val2)
>  	}
>  }
>  
> +static int axp192_adc_scale(struct iio_chan_spec const *chan, int *val,
> +			    int *val2)
> +{
> +	switch (chan->type) {
> +	case IIO_VOLTAGE:
> +		return axp192_adc_scale_voltage(chan->channel, val, val2);
> +
> +	case IIO_CURRENT:
> +		/*
> +		 * AXP192 current channels are identical to the AXP20x,
> +		 * therefore we can re-use the scaling function.
> +		 */
> +		return axp20x_adc_scale_current(chan->channel, val, val2);
> +
> +	case IIO_TEMP:
> +		*val = 100;
> +		return IIO_VAL_INT;
> +
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
>  static int axp20x_adc_scale(struct iio_chan_spec const *chan, int *val,
>  			    int *val2)
>  {
> @@ -439,6 +568,42 @@ static int axp813_adc_scale(struct iio_chan_spec const *chan, int *val,
>  	}
>  }
>  
> +static int axp192_adc_offset_voltage(struct iio_dev *indio_dev, int channel,
> +				     int *val)
> +{
> +	struct axp20x_adc_iio *info = iio_priv(indio_dev);
> +	int ret;
> +
> +	ret = regmap_read(info->regmap, AXP192_GPIO30_IN_RANGE, val);
> +	if (ret < 0)
> +		return ret;
> +
> +	switch (channel) {
> +	case AXP192_GPIO0_V:
> +		*val &= AXP192_GPIO30_IN_RANGE_GPIO0;

Obviously existing case does it this way, but I'd prefer a local variable
and FIELD_GET() for these.  Reusing *val for this is confusing.

A precursor patch to tidy up earlier code would be great if you have
time.

> +		break;
> +
> +	case AXP192_GPIO1_V:
> +		*val &= AXP192_GPIO30_IN_RANGE_GPIO1;
> +		break;
> +
> +	case AXP192_GPIO2_V:
> +		*val &= AXP192_GPIO30_IN_RANGE_GPIO2;
> +		break;
> +
> +	case AXP192_GPIO3_V:
> +		*val &= AXP192_GPIO30_IN_RANGE_GPIO3;
> +		break;
> +
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	*val = *val ? 700000 : 0;
> +
> +	return IIO_VAL_INT;
> +}
> +
>  static int axp20x_adc_offset_voltage(struct iio_dev *indio_dev, int channel,
>  				     int *val)
>  {
> @@ -467,6 +632,22 @@ static int axp20x_adc_offset_voltage(struct iio_dev *indio_dev, int channel,
>  	return IIO_VAL_INT;
>  }
>  
> +static int axp192_adc_offset(struct iio_dev *indio_dev,
> +			     struct iio_chan_spec const *chan, int *val)
> +{
> +	switch (chan->type) {
> +	case IIO_VOLTAGE:
> +		return axp192_adc_offset_voltage(indio_dev, chan->channel, val);
> +
> +	case IIO_TEMP:
> +		*val = -1447;
> +		return IIO_VAL_INT;
> +
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
>  static int axp20x_adc_offset(struct iio_dev *indio_dev,
>  			     struct iio_chan_spec const *chan, int *val)
>  {
> @@ -483,6 +664,25 @@ static int axp20x_adc_offset(struct iio_dev *indio_dev,
>  	}
>  }
>  
> +static int axp192_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:
> +		return axp192_adc_offset(indio_dev, chan, val);
> +
> +	case IIO_CHAN_INFO_SCALE:
> +		return axp192_adc_scale(chan, val, val2);
> +
> +	case IIO_CHAN_INFO_RAW:
> +		return axp20x_adc_raw(indio_dev, chan, val);
> +
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
>  static int axp20x_read_raw(struct iio_dev *indio_dev,
>  			   struct iio_chan_spec const *chan, int *val,
>  			   int *val2, long mask)
> @@ -543,6 +743,54 @@ static int axp813_read_raw(struct iio_dev *indio_dev,
>  	}
>  }
>  
> +static int axp192_write_raw(struct iio_dev *indio_dev,
> +			    struct iio_chan_spec const *chan, int val, int val2,
> +			    long mask)
> +{
> +	struct axp20x_adc_iio *info = iio_priv(indio_dev);
> +	unsigned int reg, regval;
> +
> +	/*
> +	 * The AXP192 PMIC allows the user to choose between 0V and 0.7V offsets
> +	 * for (independently) GPIO0-3 when in ADC mode.
> +	 */
> +	if (mask != IIO_CHAN_INFO_OFFSET)
> +		return -EINVAL;
> +
> +	if (val != 0 && val != 700000)
> +		return -EINVAL;
> +
> +	val = val ? 1 : 0;

Use a local variable for this. It's no longer val in any way so the
reuse is confusing.

> +
> +	switch (chan->channel) {
> +	case AXP192_GPIO0_V:
> +		reg = AXP192_GPIO30_IN_RANGE_GPIO0;
> +		regval = AXP192_GPIO30_IN_RANGE_GPIO0_VAL(val);

FIELD_PREP()

> +		break;
> +
> +	case AXP192_GPIO1_V:
> +		reg = AXP192_GPIO30_IN_RANGE_GPIO1;
> +		regval = AXP192_GPIO30_IN_RANGE_GPIO1_VAL(val);
> +		break;
> +
> +	case AXP192_GPIO2_V:
> +		reg = AXP192_GPIO30_IN_RANGE_GPIO2;
> +		regval = AXP192_GPIO30_IN_RANGE_GPIO2_VAL(val);
> +		break;
> +
> +	case AXP192_GPIO3_V:
> +		reg = AXP192_GPIO30_IN_RANGE_GPIO3;
> +		regval = AXP192_GPIO30_IN_RANGE_GPIO3_VAL(val);
> +		break;
> +
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return regmap_update_bits(info->regmap, AXP192_GPIO30_IN_RANGE, reg,
> +				  regval);
> +}
> +
>  static int axp20x_write_raw(struct iio_dev *indio_dev,
>  			    struct iio_chan_spec const *chan, int val, int val2,
>  			    long mask)
> @@ -581,6 +829,18 @@ static int axp20x_write_raw(struct iio_dev *indio_dev,
>  				  regval);
>  }
>  
> +static int axp192_read_label(struct iio_dev *iio_dev,
> +			     const struct iio_chan_spec *chan, char *label)
> +{
> +	return snprintf(label, PAGE_SIZE, "%s\n", chan->datasheet_name);
> +}

Unless I missed a previous patch adding labels to the other devices supported,
this is the first driver to use these.  Why do they make sense here but not
to add to existing supported devices?

I don't particularly mind this addition, just looking for an explanation.

> +
> +static const struct iio_info axp192_adc_iio_info = {
> +	.read_raw = axp192_read_raw,
> +	.write_raw = axp192_write_raw,
> +	.read_label = axp192_read_label,
> +};
> +
>  static const struct iio_info axp20x_adc_iio_info = {
>  	.read_raw = axp20x_read_raw,
>  	.write_raw = axp20x_write_raw,
> @@ -620,19 +880,29 @@ struct axp_data {
>  	int				num_channels;
>  	struct iio_chan_spec const	*channels;
>  	unsigned long			adc_en1_mask;
> +	unsigned long			adc_en2_mask;
>  	int				(*adc_rate)(struct axp20x_adc_iio *info,
>  						    int rate);
> -	bool				adc_en2;
>  	struct iio_map			*maps;
>  };
>  
> +static const struct axp_data axp192_data = {
> +	.iio_info = &axp192_adc_iio_info,
> +	.num_channels = ARRAY_SIZE(axp192_adc_channels),
> +	.channels = axp192_adc_channels,
> +	.adc_en1_mask = AXP192_ADC_EN1_MASK,
> +	.adc_en2_mask = AXP192_ADC_EN2_MASK,
> +	.adc_rate = axp20x_adc_rate,
> +	.maps = axp20x_maps,
> +};
> +
>  static const struct axp_data axp20x_data = {
>  	.iio_info = &axp20x_adc_iio_info,
>  	.num_channels = ARRAY_SIZE(axp20x_adc_channels),
>  	.channels = axp20x_adc_channels,
>  	.adc_en1_mask = AXP20X_ADC_EN1_MASK,
> +	.adc_en2_mask = AXP20X_ADC_EN2_MASK,
>  	.adc_rate = axp20x_adc_rate,
> -	.adc_en2 = true,
>  	.maps = axp20x_maps,
>  };
>  
> @@ -642,7 +912,6 @@ static const struct axp_data axp22x_data = {
>  	.channels = axp22x_adc_channels,
>  	.adc_en1_mask = AXP22X_ADC_EN1_MASK,
>  	.adc_rate = axp22x_adc_rate,
> -	.adc_en2 = false,
>  	.maps = axp22x_maps,
>  };
>  
> @@ -652,11 +921,11 @@ static const struct axp_data axp813_data = {
>  	.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 of_device_id axp20x_adc_of_match[] = {
> +	{ .compatible = "x-powers,axp192-adc", .data = (void *)&axp192_data, },
>  	{ .compatible = "x-powers,axp209-adc", .data = (void *)&axp20x_data, },
>  	{ .compatible = "x-powers,axp221-adc", .data = (void *)&axp22x_data, },
>  	{ .compatible = "x-powers,axp813-adc", .data = (void *)&axp813_data, },
> @@ -665,6 +934,7 @@ static const struct of_device_id axp20x_adc_of_match[] = {
>  MODULE_DEVICE_TABLE(of, axp20x_adc_of_match);
>  
>  static const struct platform_device_id axp20x_adc_id_match[] = {
> +	{ .name = "axp192-adc", .driver_data = (kernel_ulong_t)&axp192_data, },
>  	{ .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, },
> @@ -710,10 +980,11 @@ static int axp20x_probe(struct platform_device *pdev)
>  	/* Enable the ADCs on IP */
>  	regmap_write(info->regmap, AXP20X_ADC_EN1, info->data->adc_en1_mask);
>  
> -	if (info->data->adc_en2)
> -		/* Enable GPIO0/1 and internal temperature ADCs */
> +	if (info->data->adc_en2_mask)
> +		/* Enable GPIO and internal temperature ADCs */
>  		regmap_update_bits(info->regmap, AXP20X_ADC_EN2,
> -				   AXP20X_ADC_EN2_MASK, AXP20X_ADC_EN2_MASK);
> +				   info->data->adc_en2_mask,
> +				   info->data->adc_en2_mask);
>  
>  	/* Configure ADCs rate */
>  	info->data->adc_rate(info, 100);
> @@ -738,7 +1009,7 @@ static int axp20x_probe(struct platform_device *pdev)
>  fail_map:
>  	regmap_write(info->regmap, AXP20X_ADC_EN1, 0);
>  
> -	if (info->data->adc_en2)
> +	if (info->data->adc_en2_mask)
>  		regmap_write(info->regmap, AXP20X_ADC_EN2, 0);
>  
>  	return ret;
> @@ -754,7 +1025,7 @@ static int axp20x_remove(struct platform_device *pdev)
>  
>  	regmap_write(info->regmap, AXP20X_ADC_EN1, 0);
>  
> -	if (info->data->adc_en2)
> +	if (info->data->adc_en2_mask)
>  		regmap_write(info->regmap, AXP20X_ADC_EN2, 0);
>  
>  	return 0;
Aidan MacDonald June 4, 2022, 11:47 a.m. UTC | #2
Jonathan Cameron <jic23@kernel.org> writes:

> On Fri,  3 Jun 2022 14:57:12 +0100
> Aidan MacDonald <aidanmacdonald.0x0@gmail.com> wrote:
>
>> The AXP192 is identical to the AXP20x, except for the addition of
>> two more GPIO ADC channels.
>> 
>> Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
> Hi Aidan,
>
> A few minor questions and comments inline.
>
> Thanks,
>
> Jonathan
>
>> ---
>>  drivers/iio/adc/axp20x_adc.c | 289 +++++++++++++++++++++++++++++++++--
>>  1 file changed, 280 insertions(+), 9 deletions(-)
>> 
>> diff --git a/drivers/iio/adc/axp20x_adc.c b/drivers/iio/adc/axp20x_adc.c
>> index 53bf7d4899d2..7d2bf9529420 100644
>> --- a/drivers/iio/adc/axp20x_adc.c
>> +++ b/drivers/iio/adc/axp20x_adc.c
>> @@ -21,6 +21,9 @@
>>  #include <linux/iio/machine.h>
>>  #include <linux/mfd/axp20x.h>
>>  
>> +#define AXP192_ADC_EN1_MASK			GENMASK(7, 0)
>> +#define AXP192_ADC_EN2_MASK			(BIT(7) | GENMASK(3, 0))
>
> Obviously doesn't really mater, but for consistency, maybe match the
> ordering in AXP20X_ADC_EN2_MASK below?
>

Ack, will do

>> +
>>  #define AXP20X_ADC_EN1_MASK			GENMASK(7, 0)
>>  
>>  #define AXP20X_ADC_EN2_MASK			(GENMASK(3, 2) | BIT(7))
>> @@ -31,6 +34,15 @@
>>  #define AXP20X_GPIO10_IN_RANGE_GPIO0_VAL(x)	((x) & BIT(0))
>>  #define AXP20X_GPIO10_IN_RANGE_GPIO1_VAL(x)	(((x) & BIT(0)) << 1)
>>  
>> +#define AXP192_GPIO30_IN_RANGE_GPIO0		BIT(0)
>> +#define AXP192_GPIO30_IN_RANGE_GPIO1		BIT(1)
>> +#define AXP192_GPIO30_IN_RANGE_GPIO2		BIT(2)
>> +#define AXP192_GPIO30_IN_RANGE_GPIO3		BIT(3)
>> +#define AXP192_GPIO30_IN_RANGE_GPIO0_VAL(x)	((x) & BIT(0))
>> +#define AXP192_GPIO30_IN_RANGE_GPIO1_VAL(x)	(((x) & BIT(0)) << 1)
>> +#define AXP192_GPIO30_IN_RANGE_GPIO2_VAL(x)	(((x) & BIT(0)) << 2)
>> +#define AXP192_GPIO30_IN_RANGE_GPIO3_VAL(x)	(((x) & BIT(0)) << 3)
>
> FIELD_PREP() using the masks is cleaner than hand rolling it.
> If you fancy updating the driver in general whilst here that would be great,
> but I don't mind if not.
>

Yeah, I don't mind, I'll include a cleanup patch in my v2.

>> +
>>  #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)
>> @@ -70,6 +82,25 @@ struct axp20x_adc_iio {
>>  	const struct axp_data	*data;
>>  };
>>  
>> +enum axp192_adc_channel_v {
>> +	AXP192_ACIN_V = 0,
>> +	AXP192_VBUS_V,
>> +	AXP192_TS_IN,
>> +	AXP192_GPIO0_V,
>> +	AXP192_GPIO1_V,
>> +	AXP192_GPIO2_V,
>> +	AXP192_GPIO3_V,
>> +	AXP192_IPSOUT_V,
>> +	AXP192_BATT_V,
>> +};
>> +
>> +enum axp192_adc_channel_i {
>> +	AXP192_ACIN_I = 0,
>> +	AXP192_VBUS_I,
>> +	AXP192_BATT_CHRG_I,
>> +	AXP192_BATT_DISCHRG_I,
>> +};
>> +
>>  enum axp20x_adc_channel_v {
>>  	AXP20X_ACIN_V = 0,
>>  	AXP20X_VBUS_V,
>> @@ -157,6 +188,43 @@ static struct iio_map axp22x_maps[] = {
>>   * The only exception is for the battery. batt_v will be in_voltage6_raw and
>>   * charge current in_current6_raw and discharge current will be in_current7_raw.
>>   */
>> +static const struct iio_chan_spec axp192_adc_channels[] = {
>> +	AXP20X_ADC_CHANNEL(AXP192_ACIN_V, "acin_v", IIO_VOLTAGE,
>> +			   AXP20X_ACIN_V_ADC_H),
>> +	AXP20X_ADC_CHANNEL(AXP192_ACIN_I, "acin_i", IIO_CURRENT,
>> +			   AXP20X_ACIN_I_ADC_H),
>> +	AXP20X_ADC_CHANNEL(AXP192_VBUS_V, "vbus_v", IIO_VOLTAGE,
>> +			   AXP20X_VBUS_V_ADC_H),
>> +	AXP20X_ADC_CHANNEL(AXP192_VBUS_I, "vbus_i", IIO_CURRENT,
>> +			   AXP20X_VBUS_I_ADC_H),
>> +	{
>> +		.type = IIO_TEMP,
>> +		.address = AXP20X_TEMP_ADC_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_OFFSET(AXP192_GPIO0_V, "gpio0_v", IIO_VOLTAGE,
>> +				  AXP20X_GPIO0_V_ADC_H),
>> +	AXP20X_ADC_CHANNEL_OFFSET(AXP192_GPIO1_V, "gpio1_v", IIO_VOLTAGE,
>> +				  AXP20X_GPIO1_V_ADC_H),
>> +	AXP20X_ADC_CHANNEL_OFFSET(AXP192_GPIO2_V, "gpio2_v", IIO_VOLTAGE,
>> +				  AXP192_GPIO2_V_ADC_H),
>> +	AXP20X_ADC_CHANNEL_OFFSET(AXP192_GPIO3_V, "gpio3_v", IIO_VOLTAGE,
>> +				  AXP192_GPIO3_V_ADC_H),
>> +	AXP20X_ADC_CHANNEL(AXP192_IPSOUT_V, "ipsout_v", IIO_VOLTAGE,
>> +			   AXP20X_IPSOUT_V_HIGH_H),
>> +	AXP20X_ADC_CHANNEL(AXP192_BATT_V, "batt_v", IIO_VOLTAGE,
>> +			   AXP20X_BATT_V_H),
>> +	AXP20X_ADC_CHANNEL(AXP192_BATT_CHRG_I, "batt_chrg_i", IIO_CURRENT,
>> +			   AXP20X_BATT_CHRG_I_H),
>> +	AXP20X_ADC_CHANNEL(AXP192_BATT_DISCHRG_I, "batt_dischrg_i", IIO_CURRENT,
>> +			   AXP20X_BATT_DISCHRG_I_H),
>> +	AXP20X_ADC_CHANNEL(AXP192_TS_IN, "ts_v", IIO_VOLTAGE,
>> +			   AXP20X_TS_IN_H),
>> +};
>> +
>>  static const struct iio_chan_spec axp20x_adc_channels[] = {
>>  	AXP20X_ADC_CHANNEL(AXP20X_ACIN_V, "acin_v", IIO_VOLTAGE,
>>  			   AXP20X_ACIN_V_ADC_H),
>> @@ -277,6 +345,44 @@ static int axp813_adc_raw(struct iio_dev *indio_dev,
>>  	return IIO_VAL_INT;
>>  }
>>  
>> +static int axp192_adc_scale_voltage(int channel, int *val, int *val2)
>> +{
>> +	switch (channel) {
>> +	case AXP192_ACIN_V:
>> +	case AXP192_VBUS_V:
>> +		*val = 1;
>> +		*val2 = 700000;
>> +		return IIO_VAL_INT_PLUS_MICRO;
>> +
>> +	case AXP192_GPIO0_V:
>> +	case AXP192_GPIO1_V:
>> +	case AXP192_GPIO2_V:
>> +	case AXP192_GPIO3_V:
>> +		*val = 0;
>> +		*val2 = 500000;
>> +		return IIO_VAL_INT_PLUS_MICRO;
>> +
>> +	case AXP192_BATT_V:
>> +		*val = 1;
>> +		*val2 = 100000;
>> +		return IIO_VAL_INT_PLUS_MICRO;
>> +
>> +	case AXP192_IPSOUT_V:
>> +		*val = 1;
>> +		*val2 = 400000;
>> +		return IIO_VAL_INT_PLUS_MICRO;
>> +
>> +	case AXP192_TS_IN:
>> +		/* 0.8 mV per LSB */
>> +		*val = 0;
>> +		*val2 = 800000;
>> +		return IIO_VAL_INT_PLUS_MICRO;
>> +
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +}
>> +
>>  static int axp20x_adc_scale_voltage(int channel, int *val, int *val2)
>>  {
>>  	switch (channel) {
>> @@ -380,6 +486,29 @@ static int axp20x_adc_scale_current(int channel, int *val, int *val2)
>>  	}
>>  }
>>  
>> +static int axp192_adc_scale(struct iio_chan_spec const *chan, int *val,
>> +			    int *val2)
>> +{
>> +	switch (chan->type) {
>> +	case IIO_VOLTAGE:
>> +		return axp192_adc_scale_voltage(chan->channel, val, val2);
>> +
>> +	case IIO_CURRENT:
>> +		/*
>> +		 * AXP192 current channels are identical to the AXP20x,
>> +		 * therefore we can re-use the scaling function.
>> +		 */
>> +		return axp20x_adc_scale_current(chan->channel, val, val2);
>> +
>> +	case IIO_TEMP:
>> +		*val = 100;
>> +		return IIO_VAL_INT;
>> +
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +}
>> +
>>  static int axp20x_adc_scale(struct iio_chan_spec const *chan, int *val,
>>  			    int *val2)
>>  {
>> @@ -439,6 +568,42 @@ static int axp813_adc_scale(struct iio_chan_spec const *chan, int *val,
>>  	}
>>  }
>>  
>> +static int axp192_adc_offset_voltage(struct iio_dev *indio_dev, int channel,
>> +				     int *val)
>> +{
>> +	struct axp20x_adc_iio *info = iio_priv(indio_dev);
>> +	int ret;
>> +
>> +	ret = regmap_read(info->regmap, AXP192_GPIO30_IN_RANGE, val);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	switch (channel) {
>> +	case AXP192_GPIO0_V:
>> +		*val &= AXP192_GPIO30_IN_RANGE_GPIO0;
>
> Obviously existing case does it this way, but I'd prefer a local variable
> and FIELD_GET() for these.  Reusing *val for this is confusing.
>
> A precursor patch to tidy up earlier code would be great if you have
> time.
>

Ack

>> +		break;
>> +
>> +	case AXP192_GPIO1_V:
>> +		*val &= AXP192_GPIO30_IN_RANGE_GPIO1;
>> +		break;
>> +
>> +	case AXP192_GPIO2_V:
>> +		*val &= AXP192_GPIO30_IN_RANGE_GPIO2;
>> +		break;
>> +
>> +	case AXP192_GPIO3_V:
>> +		*val &= AXP192_GPIO30_IN_RANGE_GPIO3;
>> +		break;
>> +
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +
>> +	*val = *val ? 700000 : 0;
>> +
>> +	return IIO_VAL_INT;
>> +}
>> +
>>  static int axp20x_adc_offset_voltage(struct iio_dev *indio_dev, int channel,
>>  				     int *val)
>>  {
>> @@ -467,6 +632,22 @@ static int axp20x_adc_offset_voltage(struct iio_dev *indio_dev, int channel,
>>  	return IIO_VAL_INT;
>>  }
>>  
>> +static int axp192_adc_offset(struct iio_dev *indio_dev,
>> +			     struct iio_chan_spec const *chan, int *val)
>> +{
>> +	switch (chan->type) {
>> +	case IIO_VOLTAGE:
>> +		return axp192_adc_offset_voltage(indio_dev, chan->channel, val);
>> +
>> +	case IIO_TEMP:
>> +		*val = -1447;
>> +		return IIO_VAL_INT;
>> +
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +}
>> +
>>  static int axp20x_adc_offset(struct iio_dev *indio_dev,
>>  			     struct iio_chan_spec const *chan, int *val)
>>  {
>> @@ -483,6 +664,25 @@ static int axp20x_adc_offset(struct iio_dev *indio_dev,
>>  	}
>>  }
>>  
>> +static int axp192_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:
>> +		return axp192_adc_offset(indio_dev, chan, val);
>> +
>> +	case IIO_CHAN_INFO_SCALE:
>> +		return axp192_adc_scale(chan, val, val2);
>> +
>> +	case IIO_CHAN_INFO_RAW:
>> +		return axp20x_adc_raw(indio_dev, chan, val);
>> +
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +}
>> +
>>  static int axp20x_read_raw(struct iio_dev *indio_dev,
>>  			   struct iio_chan_spec const *chan, int *val,
>>  			   int *val2, long mask)
>> @@ -543,6 +743,54 @@ static int axp813_read_raw(struct iio_dev *indio_dev,
>>  	}
>>  }
>>  
>> +static int axp192_write_raw(struct iio_dev *indio_dev,
>> +			    struct iio_chan_spec const *chan, int val, int val2,
>> +			    long mask)
>> +{
>> +	struct axp20x_adc_iio *info = iio_priv(indio_dev);
>> +	unsigned int reg, regval;
>> +
>> +	/*
>> +	 * The AXP192 PMIC allows the user to choose between 0V and 0.7V offsets
>> +	 * for (independently) GPIO0-3 when in ADC mode.
>> +	 */
>> +	if (mask != IIO_CHAN_INFO_OFFSET)
>> +		return -EINVAL;
>> +
>> +	if (val != 0 && val != 700000)
>> +		return -EINVAL;
>> +
>> +	val = val ? 1 : 0;
>
> Use a local variable for this. It's no longer val in any way so the
> reuse is confusing.
>

Ack, I'll also clean up the existing code that I copied this from.

>> +
>> +	switch (chan->channel) {
>> +	case AXP192_GPIO0_V:
>> +		reg = AXP192_GPIO30_IN_RANGE_GPIO0;
>> +		regval = AXP192_GPIO30_IN_RANGE_GPIO0_VAL(val);
>
> FIELD_PREP()
>

Ack

>> +		break;
>> +
>> +	case AXP192_GPIO1_V:
>> +		reg = AXP192_GPIO30_IN_RANGE_GPIO1;
>> +		regval = AXP192_GPIO30_IN_RANGE_GPIO1_VAL(val);
>> +		break;
>> +
>> +	case AXP192_GPIO2_V:
>> +		reg = AXP192_GPIO30_IN_RANGE_GPIO2;
>> +		regval = AXP192_GPIO30_IN_RANGE_GPIO2_VAL(val);
>> +		break;
>> +
>> +	case AXP192_GPIO3_V:
>> +		reg = AXP192_GPIO30_IN_RANGE_GPIO3;
>> +		regval = AXP192_GPIO30_IN_RANGE_GPIO3_VAL(val);
>> +		break;
>> +
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +
>> +	return regmap_update_bits(info->regmap, AXP192_GPIO30_IN_RANGE, reg,
>> +				  regval);
>> +}
>> +
>>  static int axp20x_write_raw(struct iio_dev *indio_dev,
>>  			    struct iio_chan_spec const *chan, int val, int val2,
>>  			    long mask)
>> @@ -581,6 +829,18 @@ static int axp20x_write_raw(struct iio_dev *indio_dev,
>>  				  regval);
>>  }
>>  
>> +static int axp192_read_label(struct iio_dev *iio_dev,
>> +			     const struct iio_chan_spec *chan, char *label)
>> +{
>> +	return snprintf(label, PAGE_SIZE, "%s\n", chan->datasheet_name);
>> +}
>
> Unless I missed a previous patch adding labels to the other devices supported,
> this is the first driver to use these.  Why do they make sense here but not
> to add to existing supported devices?
>
> I don't particularly mind this addition, just looking for an explanation.
>

That'd be because 1d4ef9b39ebecca8 ("iio: core: Add optional symbolic
label to a device channel") added read_label in 2020, while the AXP
driver was introduced in 2017. I could add read_label for the other
chips while I'm here, for consistency.

One question I have is why read_label exists when the kernel already has
unique names for IIO channels. Why not just expose the datasheet_name to
userspace from the IIO core instead of making drivers do it?

>> +
>> +static const struct iio_info axp192_adc_iio_info = {
>> +	.read_raw = axp192_read_raw,
>> +	.write_raw = axp192_write_raw,
>> +	.read_label = axp192_read_label,
>> +};
>> +
>>  static const struct iio_info axp20x_adc_iio_info = {
>>  	.read_raw = axp20x_read_raw,
>>  	.write_raw = axp20x_write_raw,
>> @@ -620,19 +880,29 @@ struct axp_data {
>>  	int				num_channels;
>>  	struct iio_chan_spec const	*channels;
>>  	unsigned long			adc_en1_mask;
>> +	unsigned long			adc_en2_mask;
>>  	int				(*adc_rate)(struct axp20x_adc_iio *info,
>>  						    int rate);
>> -	bool				adc_en2;
>>  	struct iio_map			*maps;
>>  };
>>  
>> +static const struct axp_data axp192_data = {
>> +	.iio_info = &axp192_adc_iio_info,
>> +	.num_channels = ARRAY_SIZE(axp192_adc_channels),
>> +	.channels = axp192_adc_channels,
>> +	.adc_en1_mask = AXP192_ADC_EN1_MASK,
>> +	.adc_en2_mask = AXP192_ADC_EN2_MASK,
>> +	.adc_rate = axp20x_adc_rate,
>> +	.maps = axp20x_maps,
>> +};
>> +
>>  static const struct axp_data axp20x_data = {
>>  	.iio_info = &axp20x_adc_iio_info,
>>  	.num_channels = ARRAY_SIZE(axp20x_adc_channels),
>>  	.channels = axp20x_adc_channels,
>>  	.adc_en1_mask = AXP20X_ADC_EN1_MASK,
>> +	.adc_en2_mask = AXP20X_ADC_EN2_MASK,
>>  	.adc_rate = axp20x_adc_rate,
>> -	.adc_en2 = true,
>>  	.maps = axp20x_maps,
>>  };
>>  
>> @@ -642,7 +912,6 @@ static const struct axp_data axp22x_data = {
>>  	.channels = axp22x_adc_channels,
>>  	.adc_en1_mask = AXP22X_ADC_EN1_MASK,
>>  	.adc_rate = axp22x_adc_rate,
>> -	.adc_en2 = false,
>>  	.maps = axp22x_maps,
>>  };
>>  
>> @@ -652,11 +921,11 @@ static const struct axp_data axp813_data = {
>>  	.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 of_device_id axp20x_adc_of_match[] = {
>> +	{ .compatible = "x-powers,axp192-adc", .data = (void *)&axp192_data, },
>>  	{ .compatible = "x-powers,axp209-adc", .data = (void *)&axp20x_data, },
>>  	{ .compatible = "x-powers,axp221-adc", .data = (void *)&axp22x_data, },
>>  	{ .compatible = "x-powers,axp813-adc", .data = (void *)&axp813_data, },
>> @@ -665,6 +934,7 @@ static const struct of_device_id axp20x_adc_of_match[] = {
>>  MODULE_DEVICE_TABLE(of, axp20x_adc_of_match);
>>  
>>  static const struct platform_device_id axp20x_adc_id_match[] = {
>> +	{ .name = "axp192-adc", .driver_data = (kernel_ulong_t)&axp192_data, },
>>  	{ .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, },
>> @@ -710,10 +980,11 @@ static int axp20x_probe(struct platform_device *pdev)
>>  	/* Enable the ADCs on IP */
>>  	regmap_write(info->regmap, AXP20X_ADC_EN1, info->data->adc_en1_mask);
>>  
>> -	if (info->data->adc_en2)
>> -		/* Enable GPIO0/1 and internal temperature ADCs */
>> +	if (info->data->adc_en2_mask)
>> +		/* Enable GPIO and internal temperature ADCs */
>>  		regmap_update_bits(info->regmap, AXP20X_ADC_EN2,
>> -				   AXP20X_ADC_EN2_MASK, AXP20X_ADC_EN2_MASK);
>> +				   info->data->adc_en2_mask,
>> +				   info->data->adc_en2_mask);
>>  
>>  	/* Configure ADCs rate */
>>  	info->data->adc_rate(info, 100);
>> @@ -738,7 +1009,7 @@ static int axp20x_probe(struct platform_device *pdev)
>>  fail_map:
>>  	regmap_write(info->regmap, AXP20X_ADC_EN1, 0);
>>  
>> -	if (info->data->adc_en2)
>> +	if (info->data->adc_en2_mask)
>>  		regmap_write(info->regmap, AXP20X_ADC_EN2, 0);
>>  
>>  	return ret;
>> @@ -754,7 +1025,7 @@ static int axp20x_remove(struct platform_device *pdev)
>>  
>>  	regmap_write(info->regmap, AXP20X_ADC_EN1, 0);
>>  
>> -	if (info->data->adc_en2)
>> +	if (info->data->adc_en2_mask)
>>  		regmap_write(info->regmap, AXP20X_ADC_EN2, 0);
>>  
>>  	return 0;
Jonathan Cameron June 4, 2022, 2:27 p.m. UTC | #3
On Sat, 04 Jun 2022 12:47:38 +0100
Aidan MacDonald <aidanmacdonald.0x0@gmail.com> wrote:

> Jonathan Cameron <jic23@kernel.org> writes:
> 
> > On Fri,  3 Jun 2022 14:57:12 +0100
> > Aidan MacDonald <aidanmacdonald.0x0@gmail.com> wrote:
> >  
> >> The AXP192 is identical to the AXP20x, except for the addition of
> >> two more GPIO ADC channels.
> >> 
> >> Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>  
> > Hi Aidan,
> >
> > A few minor questions and comments inline.
> >
> > Thanks,
> >
> > Jonathan
> >
> > Unless I missed a previous patch adding labels to the other devices supported,
> > this is the first driver to use these.  Why do they make sense here but not
> > to add to existing supported devices?
> >
> > I don't particularly mind this addition, just looking for an explanation.
> >  
> 
> That'd be because 1d4ef9b39ebecca8 ("iio: core: Add optional symbolic
> label to a device channel") added read_label in 2020, while the AXP
> driver was introduced in 2017. I could add read_label for the other
> chips while I'm here, for consistency.

Thanks, I don't really mind either way on adding support for additional parts.

> 
> One question I have is why read_label exists when the kernel already has
> unique names for IIO channels. Why not just expose the datasheet_name to
> userspace from the IIO core instead of making drivers do it?

In general, datasheet_name refers to the name of the pin on a datasheet for this
device, whereas label can refer to how it is used.
There are dt bindings to allow a per channel label letting a driver (where it
makes sense) provide them for each individual ADC channel.
(e.g. the ad7768-1 driver does this).

On other devices they come from entirely different sources such as the hardcoded
choices in hid-sensor-custom-intel-hinge.

I vaguely recall that we've talked in the past about exposing datasheet name directly
but for many devices it's not that useful (the user doesn't care if a channel is
aux channel 1 or 7, but rather what it is wired up to).

At the moment this driver just exposes all channels rather than having
per channel bindings, so we don't have the option to use labeling in the device
tree to assign the names.   If it's particularly useful to you to have labels
that are datasheet names that's fine.

Jonathan
Rob Herring (Arm) June 5, 2022, 10:49 p.m. UTC | #4
On Fri, 03 Jun 2022 14:57:06 +0100, Aidan MacDonald wrote:
> The AXP192 is another X-Powers PMIC similar to the existing ones.
> 
> Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
> ---
>  Documentation/devicetree/bindings/mfd/x-powers,axp152.yaml | 1 +
>  1 file changed, 1 insertion(+)
> 

Acked-by: Rob Herring <robh@kernel.org>
Aidan MacDonald June 7, 2022, 10:49 a.m. UTC | #5
Jonathan Cameron <jic23@kernel.org> writes:

> On Sat, 04 Jun 2022 12:47:38 +0100
> Aidan MacDonald <aidanmacdonald.0x0@gmail.com> wrote:
>
>> Jonathan Cameron <jic23@kernel.org> writes:
>> 
>> > On Fri,  3 Jun 2022 14:57:12 +0100
>> > Aidan MacDonald <aidanmacdonald.0x0@gmail.com> wrote:
>> >  
>> >> The AXP192 is identical to the AXP20x, except for the addition of
>> >> two more GPIO ADC channels.
>> >> 
>> >> Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>  
>> > Hi Aidan,
>> >
>> > A few minor questions and comments inline.
>> >
>> > Thanks,
>> >
>> > Jonathan
>> >
>> > Unless I missed a previous patch adding labels to the other devices supported,
>> > this is the first driver to use these.  Why do they make sense here but not
>> > to add to existing supported devices?
>> >
>> > I don't particularly mind this addition, just looking for an explanation.
>> >  
>> 
>> That'd be because 1d4ef9b39ebecca8 ("iio: core: Add optional symbolic
>> label to a device channel") added read_label in 2020, while the AXP
>> driver was introduced in 2017. I could add read_label for the other
>> chips while I'm here, for consistency.
>
> Thanks, I don't really mind either way on adding support for additional parts.
>
>> 
>> One question I have is why read_label exists when the kernel already has
>> unique names for IIO channels. Why not just expose the datasheet_name to
>> userspace from the IIO core instead of making drivers do it?
>
> In general, datasheet_name refers to the name of the pin on a datasheet for this
> device, whereas label can refer to how it is used.
> There are dt bindings to allow a per channel label letting a driver (where it
> makes sense) provide them for each individual ADC channel.
> (e.g. the ad7768-1 driver does this).
>
> On other devices they come from entirely different sources such as the hardcoded
> choices in hid-sensor-custom-intel-hinge.
>
> I vaguely recall that we've talked in the past about exposing datasheet name directly
> but for many devices it's not that useful (the user doesn't care if a channel is
> aux channel 1 or 7, but rather what it is wired up to).
>
> At the moment this driver just exposes all channels rather than having
> per channel bindings, so we don't have the option to use labeling in the device
> tree to assign the names.   If it's particularly useful to you to have labels
> that are datasheet names that's fine.
>
> Jonathan

Thanks for the explanation, makes sense that "gpio0_v" is probably not
all that useful. I was thinking mainly of channels like battery charge
current where the channel usage is fixed by hardware. The labels aren't
terribly useful to me so I'll just leave them out, I'd rather not bother
with adding dt labels.

Regards,
Aidan