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)
