Re: [PATCH 2/2] drivers: iio: dac: Add AD5754 DAC driver

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

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