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

[Marcus Folkesson <marcus.folkesson@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 4773 bytes --]

Hi,

See comments inline

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.
> 
> Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/AD5724_5734_5754.pdf
> Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ad5722_5732_5752.pdf
> Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ad5724r_5734r_5754r.pdf
> Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/AD5722R_5732R_5752R.pdf
> Signed-off-by: Ciprian Regus <ciprian.regus@analog.com>
> ---


[...]

> +struct ad5754_span_tbl {
> +	int min;
> +	int max;
> +};
> +
> +const struct ad5754_span_tbl ad5754_range[] = {
> +	{0, 5000000},
> +	{0, 10000000},
> +	{0, 10800000},
> +	{-5000000, 5000000},
> +	{-10000000, 10000000},
> +	{-10800000, 10800000},
> +};

Should these be static?

> +
> +enum AD5754_TYPE {
> +	AD5722,
> +	AD5732,
> +	AD5752,
> +	AD5724,
> +	AD5734,
> +	AD5754,
> +	AD5722R,
> +	AD5732R,
> +	AD5752R,
> +	AD5724R,
> +	AD5734R,
> +	AD5754R,
> +};
> +
> +struct ad5754_chip_info {
> +	const char *name;
> +	u32 resolution;
> +	bool internal_vref;
> +	const u32 data_mask;
> +	const struct iio_chan_spec *channels;
> +	u32 num_channels;
> +};
> +
> +const struct iio_chan_spec ad5754_channels[][AD5754_MAX_CHANNELS] = {

static?

> +	[AD5754_2_CHANNELS] = {
> +		AD5754_CHANNEL(0),
> +		AD5754_CHANNEL(1),
> +	},
> +	[AD5754_4_CHANNELS] = {
> +		AD5754_CHANNEL(0),
> +		AD5754_CHANNEL(1),
> +		AD5754_CHANNEL(2),
> +		AD5754_CHANNEL(3),
> +	},
> +};
> +
> +const struct ad5754_chip_info ad5754_chip_info_data[] = {

static?

[...]

> +
> +
> +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;
> +
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	st = iio_priv(indio_dev);
> +	spi_set_drvdata(spi, indio_dev);
> +
> +	st->spi = spi;
> +	st->dev = dev;
> +	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;
> +
> +	st->regmap = devm_regmap_init(st->dev, NULL, st, &ad5754_regmap_config);
> +	if (IS_ERR(st->regmap))
> +		return dev_err_probe(st->dev, PTR_ERR(vref_reg),
> +				     "Regmap init error\n");

Are you sure you want to pass vref_reg here? :-)
st->regmap

> +
> +	st->dac_max_code = BIT(st->chip_info->resolution) - 1;
> +	st->sub_lsb = AD5754_MAX_RESOLUTION - st->chip_info->resolution;
> +
> +	vref_reg = devm_regulator_get_optional(st->dev, "vref");
> +	if (IS_ERR(vref_reg)) {
> +		if (!st->chip_info->internal_vref)
> +			return dev_err_probe(st->dev, PTR_ERR(vref_reg),
> +			       "Failed to get the vref regulator\n");
> +
> +		st->vref = AD5754_INT_VREF;
> +		ret = ad5754_int_vref_enable(st);
> +		if (ret)
> +			return ret;
> +	} else {
> +		ret = regulator_enable(vref_reg);
> +		if (ret)
> +			return dev_err_probe(st->dev, PTR_ERR(vref_reg),
> +				"Failed to enable the vref regulator\n");

ret contains the error code here, not vref_reg.

> +
> +		ret = devm_add_action_or_reset(dev, ad5754_disable_regulator, vref_reg);
> +		if (ret)
> +			return ret;
> +
> +		ret = regulator_get_voltage(vref_reg);
> +		if (ret < 0)
> +			return dev_err_probe(dev, ret, "Failed to get vref\n");
> +
> +		st->vref = ret / 1000;
> +	}
> +
> +	indio_dev->name = st->chip_info->name;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->info = &ad5754_info;
> +	indio_dev->channels = st->chip_info->channels;
> +	indio_dev->num_channels = st->chip_info->num_channels;
> +
> +	ret = ad5754_enable_channels(st);
> +	if (ret)
> +		return ret;
> +
> +	return devm_iio_device_register(st->dev, indio_dev);
> +}
> +

[...]

> +module_driver(ad5754_driver,
> +	      ad5754_register_driver,
> +	      ad5754_unregister_driver);

Use module_spi_driver() instead

> +
> +MODULE_AUTHOR("Ciprian Regus <ciprian.regus@analog.com>");
> +MODULE_DESCRIPTION("Analog Devices AD5754 DAC");
> +MODULE_LICENSE("GPL");
> -- 
> 2.30.2
> 

Best regards
Marcus Folkesson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]