Re: [PATCH 2/2] drivers: iio: dac: Add AD5754 DAC driver
From: Andy Shevchenko
Date: Tue Oct 04 2022 - 04:51:07 EST
On Tue, Oct 04, 2022 at 10:18:25AM +0300, Ciprian Regus wrote:
> The AD5724/AD5734/AD5754 are quad, 12-/14-/16-bit, serial
> input, voltage output DACs. The devices operate from single-
> supply voltages from +4.5 V up to +16.5 V or dual-supply
> voltages from ±4.5 V up to ±16.5 V. The input coding is
> user-selectable twos complement or offset binary for a bipolar
> output (depending on the state of Pin BIN/2sComp), and straight
> binary for a unipolar output.
...
> +#define AD5754_INT_VREF 2500
Units? (Like _mV or _uV or what? Note, small u is OK to have in such cases)
...
> +#define AD5754_CLEAR_FUNC BIT(2)
> +#define AD5754_LOAD_FUNC (BIT(2) | BIT(0))
> +#define AD5754_NOOP_FUNC GENMASK(4, 3)
Seems like abuse of BIT and GENMASK, use plain numbers as it's probably is.
Otherwise _each_ bit should have it's own descriptive meaning.
...
> +#define AD5754_DAC_REG 0
> +#define AD5754_RANGE_REG BIT(0)
> +#define AD5754_PWR_REG BIT(1)
...
> +#define AD5754_CTRL_REG GENMASK(1, 0)
Why _REG uses GENMASK()?
...
> +struct ad5754_span_tbl {
> + int min;
> + int max;
> +};
I'm wondering if linear_range.h can anyhow help with this code.
...
> +struct ad5754_state {
> + struct regmap *regmap;
> + struct spi_device *spi;
> + struct device *dev;
You always can derive dev from regmap, is this one different?
> +
> + const struct ad5754_chip_info *chip_info;
> +
> + u32 range_idx[AD5754_MAX_CHANNELS];
> + int offset[AD5754_MAX_CHANNELS];
> + u32 dac_max_code;
> + u32 data_mask;
> + u32 sub_lsb;
> + u32 vref;
> +
> + /*
> + * DMA (thus cache coherency maintenance) may require the
> + * transfer buffers to live in their own cache lines.
> + */
> + u8 buff[AD5754_FRAME_SIZE] __aligned(IIO_DMA_MINALIGN);
> +};
...
> +static const struct regmap_config ad5754_regmap_config = {
> + .reg_bits = 8,
> + .val_bits = 16,
> + .reg_write = ad5754_reg_write,
> + .reg_read = ad5754_reg_read,
No max register address?
> +};
...
> + struct fwnode_handle *channel_node = NULL;
Redundant assignment.
...
> + fwnode_for_each_available_child_node(dev_fwnode(st->dev), channel_node) {
Why not device_for_each_child_node() ?
(Yes, it uses available ones)
> + }
...
> + range = &ad5754_range[st->range_idx[chan->channel]];
> + gain = (range->max - range->min) / 2500;
> + *val = st->vref * gain / 1000;
> + *val2 = st->chip_info->resolution;
Yeah, looks familiar to the linear_range APIs.
...
> +static int ad5754_probe(struct spi_device *spi)
> +{
> + struct regulator *vref_reg;
> + struct iio_dev *indio_dev;
> + struct ad5754_state *st;
> + struct device *dev;
> + int ret;
> + dev = &spi->dev;
Can be done in the definition block (inline).
...
> + st->chip_info = device_get_match_data(dev);
> + if (!st->chip_info)
> + st->chip_info =
> + (const struct ad5754_chip_info *)spi_get_device_id(spi)->driver_data;
This can look better with a temporary variable. But doesn't matter since we
would like to have these lines to be packed in a new SPI API helper in the
future.
...
> + st->vref = ret / 1000;
Do we have uV_PER_mV or so?
...
> +static const struct spi_device_id ad5754_id[] = {
> + {},
No comma for the terminator line.
> +};
...
> +static const struct of_device_id ad5754_dt_id[] = {
> + {},
Ditto.
> +};
...
> +module_driver(ad5754_driver,
> + ad5754_register_driver,
> + ad5754_unregister_driver);
Why not module_spi_driver()?
--
With Best Regards,
Andy Shevchenko