Re: [PATCH 5/8] iio: adc: axp20x_adc: add support for AXP717 ADC

[Jonathan Cameron <jic23@kernel.org>]

On Mon, 17 Jun 2024 17:05:32 -0500
Chris Morgan <macroalpha82@gmail.com> wrote:

> From: Chris Morgan <macromorgan@hotmail.com>
> 
> Add support for the AXP717 ADC. The AXP717 differs from other ADCs
> in this series by utilizing a 14 bit ADC for all channels (a full 16
> bits with the first 2 digits reserved). It also differs by lacking a
> battery discharge current channel.
> 
> Note that while the current charge channel itself is included in this
> driver for the AXP717 and listed in the datasheet, no scale or offset
> was given for this channel. For now no scale or offset is provided in
> this driver.
> 
> Signed-off-by: Chris Morgan <macromorgan@hotmail.com>
Hi Chris

A few minor comments inline,

Thanks,

Jonathan

> ---
>  drivers/iio/adc/axp20x_adc.c | 167 +++++++++++++++++++++++++++++++++--
>  1 file changed, 160 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/iio/adc/axp20x_adc.c b/drivers/iio/adc/axp20x_adc.c
> index d6c51b0f48e3..f35ba2c11e1b 100644
> --- a/drivers/iio/adc/axp20x_adc.c
> +++ b/drivers/iio/adc/axp20x_adc.c
> @@ -5,6 +5,7 @@
>   *	Quentin Schulz <quentin.schulz@free-electrons.com>
>   */
>  
> +#include <asm/unaligned.h>
>  #include <linux/bitfield.h>
>  #include <linux/completion.h>
>  #include <linux/interrupt.h>
> @@ -27,6 +28,8 @@
>  
>  #define AXP22X_ADC_EN1_MASK			(GENMASK(7, 5) | BIT(0))
>  
> +#define AXP717_ADC_EN1_MASK			GENMASK(5, 0)
> +
>  #define AXP20X_GPIO10_IN_RANGE_GPIO0		BIT(0)
>  #define AXP20X_GPIO10_IN_RANGE_GPIO1		BIT(1)
>  
> @@ -35,6 +38,11 @@
>  
>  #define AXP22X_ADC_RATE_HZ(x)			((ilog2((x) / 100) << 6) & AXP20X_ADC_RATE_MASK)
>  
> +#define AXP717_ADC_DATA_TS			0x00
> +#define AXP717_ADC_DATA_TEMP			0x01
> +
> +#define AXP717_ADC_DATA_MASK			0x3fff

GENMASK()

> +static int axp717_adc_raw(struct iio_dev *indio_dev,
> +			  struct iio_chan_spec const *chan, int *val)
> +{
> +	struct axp20x_adc_iio *info = iio_priv(indio_dev);
> +	u8 bulk_reg[2];
> +	int ret;
> +
> +	/*
> +	 * A generic "ADC data" channel is used for TS, tdie, vmid,
> +	 * and vbackup. This channel must both first be enabled and
> +	 * also selected before it can be read.
> +	 */
> +	switch (chan->channel) {
> +	case AXP717_TS_IN:
> +		regmap_write(info->regmap, AXP717_ADC_DATA_SEL,
> +			     AXP717_ADC_DATA_TS);
> +		break;
> +	case AXP717_DIE_TEMP_V:
> +		regmap_write(info->regmap, AXP717_ADC_DATA_SEL,
> +			     AXP717_ADC_DATA_TEMP);
> +		break;
> +
> +	default:
> +		break;
> +	}
> +
> +	/*
> +	 * All channels are 14 bits, with the first 2 bits on the high
> +	 * register reserved and the remaining bits as the ADC value.
> +	 */
> +	ret = regmap_bulk_read(info->regmap, chan->address, bulk_reg, 2);
> +	if (ret < 0)
> +		return ret;
> +
> +	*val = get_unaligned_be16(bulk_reg) & AXP717_ADC_DATA_MASK;

FIELD_GET() preferred as then I don't have to check if DATA_MASK includes
the 0th bit.

> +	return IIO_VAL_INT;
> +}
> +