Re: [PATCH v2 06/10] iio: adc: mcp3911: add support for oversampling ratio

[Jonathan Cameron]

On Sat, 25 Jun 2022 12:38:49 +0200
Marcus Folkesson <marcus.folkesson@xxxxxxxxx> wrote:

> The chip supports oversampling ratio, so expose it to userspace.
> 
> Signed-off-by: Marcus Folkesson <marcus.folkesson@xxxxxxxxx>
Hi Marcus,

A few minor comments inline.

Thanks,

Jonathan


> ---
> 
> Notes:
>     v2:
>         - Make use of osr table
>         - Change formatting and typos
> 
>  drivers/iio/adc/mcp3911.c | 47 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 47 insertions(+)
> 
> diff --git a/drivers/iio/adc/mcp3911.c b/drivers/iio/adc/mcp3911.c
> index f4ee0c27c2ab..1469c12ebbb2 100644
> --- a/drivers/iio/adc/mcp3911.c
> +++ b/drivers/iio/adc/mcp3911.c
> @@ -5,6 +5,8 @@
>   * Copyright (C) 2018 Marcus Folkesson <marcus.folkesson@xxxxxxxxx>
>   * Copyright (C) 2018 Kent Gustavsson <kent@xxxxxxxxxx>
>   */
> +#include <linux/bitfield.h>
> +#include <linux/bits.h>
>  #include <linux/clk.h>
>  #include <linux/delay.h>
>  #include <linux/err.h>
> @@ -35,6 +37,7 @@
>  #define MCP3911_REG_CONFIG		0x0c
>  #define MCP3911_CONFIG_CLKEXT		BIT(1)
>  #define MCP3911_CONFIG_VREFEXT		BIT(2)
> +#define MCP3911_CONFIG_OSR		GENMASK(13, 11)
>  
>  #define MCP3911_REG_OFFCAL_CH0		0x0e
>  #define MCP3911_REG_GAINCAL_CH0		0x11
> @@ -53,6 +56,8 @@
>  
>  #define MCP3911_NUM_CHANNELS		2
>  
> +static const int mcp3911_osr_table[] = {32, 64, 128, 256, 512, 1024, 2048, 4096};
> +
>  struct mcp3911 {
>  	struct spi_device *spi;
>  	struct mutex lock;
> @@ -108,6 +113,22 @@ static int mcp3911_update(struct mcp3911 *adc, u8 reg, u32 mask,
>  	return mcp3911_write(adc, reg, val, len);
>  }
>  
> +static int mcp3911_read_avail(struct iio_dev *indio_dev,
> +			     struct iio_chan_spec const *chan,
> +			     const int **vals, int *type, int *length,
> +			     long info)
> +{
> +	switch (info) {
> +	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> +		*type = IIO_VAL_INT;
> +		*vals = mcp3911_osr_table;
> +		*length = ARRAY_SIZE(mcp3911_osr_table);
> +		return IIO_AVAIL_LIST;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
>  static int mcp3911_read_raw(struct iio_dev *indio_dev,
>  			    struct iio_chan_spec const *channel, int *val,
>  			    int *val2, long mask)
> @@ -134,6 +155,16 @@ static int mcp3911_read_raw(struct iio_dev *indio_dev,
>  
>  		ret = IIO_VAL_INT;
>  		break;
> +	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> +		ret = mcp3911_read(adc,
> +				MCP3911_REG_CONFIG, val, 2);
> +		if (ret)
> +			goto out;
> +
> +		*val = FIELD_GET(MCP3911_CONFIG_OSR, *val);
> +		*val = 32 << *val;
> +		ret = IIO_VAL_INT;
> +		break;
>  
>  	case IIO_CHAN_INFO_SCALE:
>  		if (adc->vref) {
> @@ -186,6 +217,17 @@ static int mcp3911_write_raw(struct iio_dev *indio_dev,
>  				MCP3911_STATUSCOM_EN_OFFCAL,
>  				MCP3911_STATUSCOM_EN_OFFCAL, 2);
>  		break;
> +
> +	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> +		for (int i = 0; i < sizeof(mcp3911_osr_table); i++) {
> +			if (val == mcp3911_osr_table[i]) {

Default type of IIO writes is IIO_VAL_INT_PLUS_MICRO.  You can either provide
write_raw_get_fmt() or be lazy and check val2 == 0 here.
 
> +				val = FIELD_PREP(MCP3911_CONFIG_OSR, i);
> +				ret = mcp3911_update(adc, MCP3911_REG_CONFIG, MCP3911_CONFIG_OSR,
> +						val, 2);
> +				break;
> +			}
> +		}
> +		break;
>  	}
>  
>  out:
> @@ -198,9 +240,13 @@ static int mcp3911_write_raw(struct iio_dev *indio_dev,
>  		.indexed = 1,					\
>  		.channel = idx,					\
>  		.scan_index = idx,				\
> +		.scan_index = idx,				\

repeated... I guess a merge conflict resolution issue.

> +		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), \
>  		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |	\
>  			BIT(IIO_CHAN_INFO_OFFSET) |		\
>  			BIT(IIO_CHAN_INFO_SCALE),		\
> +		.info_mask_shared_by_type_available =		\
> +			BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),	\
>  		.scan_type = {					\
>  			.sign = 's',				\
>  			.realbits = 24,				\
> @@ -252,6 +298,7 @@ static irqreturn_t mcp3911_trigger_handler(int irq, void *p)
>  static const struct iio_info mcp3911_info = {
>  	.read_raw = mcp3911_read_raw,
>  	.write_raw = mcp3911_write_raw,
> +	.read_avail = mcp3911_read_avail,
>  };
>  
>  static irqreturn_t mcp3911_interrupt(int irq, void *dev_id)