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
