Re: [PATCH v2 2/2] iio: dac: driver for MCP4821

From: Jonathan Cameron
Date: Wed Dec 20 2023 - 09:23:52 EST

On Sun, 17 Dec 2023 23:38:34 +0530
Anshul Dalal <anshulusr@xxxxxxxxx> wrote:

> Adds driver for the MCP48xx series of DACs.
>
> Device uses a simplex SPI channel. To set the value of an output channel,
> a 16-bit data of following format must be written:
>
> Bit field | Description
> 15 [MSB] | Channel selection bit
> 0 -> Channel A
> 1 -> Channel B
> 13 | Output Gain Selection bit
> 0 -> 2x Gain (Vref = 4.096V)
> 1 -> 1x Gain (Vref = 2.048V)
> 12 | Output Shutdown Control bit
> 0 -> Shutdown the selected channel
> 1 -> Active mode operation
> 11-0 [LSB]| DAC Input Data bits
> Value's big endian representation is taken as input for the
> selected DAC channel. For devices with a resolution of less
> than 12-bits, only the x most significant bits are considered
> where x is the resolution of the device.
> Reference: Page#22 [MCP48x2 Datasheet]
>
> Supported devices:
> +---------+--------------+-------------+
> | Device | Resolution | Channels |
> |---------|--------------|-------------|
> | MCP4801 | 8-bit | 1 |
> | MCP4802 | 8-bit | 2 |
> | MCP4811 | 10-bit | 1 |
> | MCP4812 | 10-bit | 2 |
> | MCP4821 | 12-bit | 1 |
> | MCP4822 | 12-bit | 2 |
> +---------+--------------+-------------+
>
> Devices tested:
> MCP4821 [12-bit single channel]
> MCP4802 [8-bit dual channel]
>
> Tested on Raspberry Pi Zero 2W
>
> Datasheet: https://ww1.microchip.com/downloads/en/DeviceDoc/22244B.pdf #MCP48x1
> Datasheet: https://ww1.microchip.com/downloads/en/DeviceDoc/20002249B.pdf #MCP48x2
> Signed-off-by: Anshul Dalal <anshulusr@xxxxxxxxx>
>
>

Hi Anshul,

Various minor comments inline.

Thanks,

Jonathan

> diff --git a/drivers/iio/dac/mcp4821.c b/drivers/iio/dac/mcp4821.c
> new file mode 100644
> index 000000000000..7384df71f702
> --- /dev/null
> +++ b/drivers/iio/dac/mcp4821.c
> @@ -0,0 +1,228 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (C) 2023 Anshul Dalal <anshulusr@xxxxxxxxx>
> + *
> + * Driver for Microchip MCP4801, MCP4802, MCP4811, MCP4812, MCP4821 and MCP4822
> + *
> + * Based on the work of:
> + * Michael Welling (MCP4922 Driver)
> + *
> + * Datasheet:
> + * MCP48x1: https://ww1.microchip.com/downloads/en/DeviceDoc/22244B.pdf
> + * MCP48x2: https://ww1.microchip.com/downloads/en/DeviceDoc/20002249B.pdf
> + *
> + * TODO:
> + * - Configurable gain
> + * - Regulator control
> + */
> +
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/of.h>
> +#include <linux/spi/spi.h>
> +
> +#include <linux/iio/iio.h>
> +#include <linux/iio/types.h>
> +
> +#include <asm/unaligned.h>
> +
> +#define MCP4821_ACTIVE_MODE BIT(12)
> +#define MCP4802_SECOND_CHAN BIT(15)
> +
> +/* DAC uses an internal Voltage reference of 4.096V at a gain of 2x */
> +#define MCP4821_2X_GAIN_VREF_MV 4096
> +
> +enum mcp4821_supported_drvice_ids {
> + ID_MCP4801,
> + ID_MCP4802,
> + ID_MCP4811,
> + ID_MCP4812,
> + ID_MCP4821,
> + ID_MCP4822,
> +};
> +
> +struct mcp4821_state {
> + struct spi_device *spi;
> + // Protects dac_value
/* Protects dac_value */

However I'm not clear why a u16 array needs protecting?
Whilst in theory the compiler might be annoying and do torn writes
(write it as 2 1 byte values) that's pretty unlikely and
we don't normally bother to protect against that.

> + struct mutex lock;
> + u16 dac_value[2];
> +};
> +
> +struct mcp4821_chip_info {
> + const char *name;
> + int num_channels;
> + const struct iio_chan_spec channels[2];
> +};
> +
> +#define MCP4821_CHAN(channel_id, resolution) \
> + { \
> + .type = IIO_VOLTAGE, .output = 1, .indexed = 1, \
> + .channel = (channel_id), \
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
> + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \
> + .scan_type = { \
> + .sign = 'u', \

Not used so don't bother setting it until you add buffered support.

> + .realbits = (resolution), \
> + .storagebits = 16, \

Not used, so don't bother setting it.

> + .shift = 12 - (resolution), \
> + }, \
> + }

> +static int mcp4821_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan, int *val,
> + int *val2, long mask)
> +{
> + struct mcp4821_state *state;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_RAW:
> + state = iio_priv(indio_dev);
> + scoped_guard(mutex, &state->lock)
> + *val = state->dac_value[chan->channel];
> + return IIO_VAL_INT;
> + case IIO_CHAN_INFO_SCALE:
> + *val = MCP4821_2X_GAIN_VREF_MV;
> + *val2 = chan->scan_type.realbits;
> + return IIO_VAL_FRACTIONAL_LOG2;
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static int mcp4821_write_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan, int val,
> + int val2, long mask)
> +{
> + struct mcp4821_state *state;
> + u16 write_val;
> + __be16 write_buffer;
> + int ret;
> + bool is_value_valid = val >= 0 && val < BIT(chan->scan_type.realbits) &&
> + val2 == 0;
> +
> + if (!is_value_valid)

Check the opposite and drop the local variable that adds nothing.

if ((val < 0) || (val >= BIT(chan_type).realbits)) || (val2 != 0))
or split it up into checks on val2 in one check then on val in the other.

> + return -EINVAL;
> + if (mask != IIO_CHAN_INFO_RAW)
> + return -EINVAL;
> +
> + state = iio_priv(indio_dev);
I'd do this at point of local variable declaration above.

struct mcp4821_state *state = iio_priv(indio_dev);

> + write_val = MCP4821_ACTIVE_MODE | val << chan->scan_type.shift;
> + if (chan->channel)
> + write_val |= MCP4802_SECOND_CHAN;
> + put_unaligned_be16(write_val, &write_buffer);
It's aligned given it's a __be16

> + ret = spi_write(state->spi, &write_buffer, sizeof(write_buffer));
> + if (ret) {
> + dev_err(&state->spi->dev, "Failed to write to device: %d", ret);
> + return ret;
> + }
> +
> + scoped_guard(mutex, &state->lock)
> + state->dac_value[chan->channel] = val;

> + return 0;
> +}
> +

Jonathan