| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 4 Oct 2022 10:40:05 +0200
From: Marcus Folkesson <marcus.folkesson@...il.com>
To: Ciprian Regus <ciprian.regus@...log.com>
Cc: jic23@...nel.org, krzysztof.kozlowski+dt@...aro.org,
robh+dt@...nel.org, andriy.shevchenko@...ux.intel.com,
linux-kernel@...r.kernel.org, linux-iio@...r.kernel.org,
devicetree@...r.kernel.org
Subject: Re: [PATCH 2/2] drivers: iio: dac: Add AD5754 DAC driver
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@...log.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@...log.com>");
> +MODULE_DESCRIPTION("Analog Devices AD5754 DAC");
> +MODULE_LICENSE("GPL");
> --
> 2.30.2
>
Best regards
Marcus Folkesson
Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)
Powered by blists - more mailing lists