On Mon, 8 Apr 2024 11:31:42 -0300
Marcelo Schmitt <marcelo.schmitt@xxxxxxxxxx> wrote:
> Add support for AD4000 family of low noise, low power, high speed,
> successive aproximation register (SAR) ADCs.
>
> Signed-off-by: Marcelo Schmitt <marcelo.schmitt@xxxxxxxxxx>
A few trivial comments inline to add to David's much more thorough review.
> +#include <asm/unaligned.h>
> +#include <linux/bits.h>
> +#include <linux/bitfield.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/kernel.h>
> +#include <linux/math.h>
> +#include <linux/module.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/spi/spi.h>
> +#include <linux/sysfs.h>
> +#include <linux/units.h>
> +#include <linux/util_macros.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
You don't have custom attributes, so doubt you need that include.
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/triggered_buffer.h>
> +#include <linux/iio/trigger_consumer.h>
> +
> +static int ad4000_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan, int *val,
> + int *val2, long info)
> +{
> + struct ad4000_state *st = iio_priv(indio_dev);
> +
> + switch (info) {
> + case IIO_CHAN_INFO_RAW:
> + iio_device_claim_direct_scoped(return -EBUSY, indio_dev)
> + return ad4000_single_conversion(indio_dev, chan, val);
> + unreachable();
> + case IIO_CHAN_INFO_SCALE:
> + *val = st->scale_tbl[st->pin_gain][st->span_comp][0];
> + *val2 = st->scale_tbl[st->pin_gain][st->span_comp][1];
> + return IIO_VAL_INT_PLUS_NANO;
> + case IIO_CHAN_INFO_OFFSET:
> + *val = 0;
> + if (st->span_comp)
> + *val = mult_frac(st->vref / 1000, 1, 10);
> +
> + return IIO_VAL_INT;
> + default:
> + break;
> + }
> +
> + return -EINVAL;
As below - I'd push into the default: and drop down here.
> +}
> +
> +static int ad4000_read_avail(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + const int **vals, int *type, int *length,
> + long info)
> +{
> + struct ad4000_state *st = iio_priv(indio_dev);
> +
> + switch (info) {
> + case IIO_CHAN_INFO_SCALE:
> + *vals = (int *)st->scale_tbl[st->pin_gain];
> + *length = 2 * 2;
> + *type = IIO_VAL_INT_PLUS_NANO;
> + return IIO_AVAIL_LIST;
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static int ad4000_write_raw_get_fmt(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan, long mask)
> +{
> + switch (mask) {
> + case IIO_CHAN_INFO_SCALE:
> + return IIO_VAL_INT_PLUS_NANO;
> + default:
> + return IIO_VAL_INT_PLUS_MICRO;
> + }
> + return -EINVAL;
dead code.
> +}
> +
> +static int ad4000_write_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan, int val, int val2,
> + long mask)
> +{
> + struct ad4000_state *st = iio_priv(indio_dev);
> + unsigned int reg_val;
> + bool span_comp_en;
> + int ret;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_SCALE:
> + iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
> + ret = ad4000_read_reg(st, ®_val);
> + if (ret < 0)
> + return ret;
> +
> + span_comp_en = (val2 == st->scale_tbl[st->pin_gain][1][1]);
> + reg_val &= ~AD4000_SPAN_COMP;
> + reg_val |= FIELD_PREP(AD4000_SPAN_COMP, span_comp_en);
> +
> + ret = ad4000_write_reg(st, reg_val);
> + if (ret < 0)
> + return ret;
> +
> + st->span_comp = span_comp_en;
> + return 0;
> + }
> + unreachable();
> + default:
> + break;
> + }
> +
> + return -EINVAL;
Trivial but could push up into the default and make it clear there are no other ways out
of the switch statement.
> +}
