Re: [PATCH v4 2/2] drivers:iio:dac: Add AD3552R driver support

From: Jonathan Cameron
Date: Sun Oct 24 2021 - 12:45:11 EST

Next message: amirmizi6: "[PATCH v17 6/6] tpm: Add YAML schema for TPM TIS I2C options"
Previous message: Christophe JAILLET: "[PATCH 2/2] RDMA/rxe: Use 'bitmap_zalloc()' when applicable"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Fri, 22 Oct 2021 12:04:27 +0000
Mihail Chindris <mihail.chindris@xxxxxxxxxx> wrote:

> The AD3552R-16 is a low drift ultrafast, 16-bit accuracy,
> current output digital-to-analog converter (DAC) designed
> to generate multiple output voltage span ranges.
> The AD3552R-16 operates with a fixed 2.5V reference.
>
> Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ad3552r.pdf

We got an oddly delayed 0-day response:

All errors (new ones prefixed by >>):

drivers/iio/dac/ad3552r.c: In function 'ad3552r_configure_device':
>> drivers/iio/dac/ad3552r.c:978:14: error: variable 'is_custom' set but not used [-Werror=unused-but-set-variable]
978 | bool is_custom;
| ^~~~~~~~~
cc1: all warnings being treated as errors

Needs fixing.

Otherwise one trivial request for an improved comment.

On process front, you seem to have forgotten to update the cover letter with the
changes since v2.

This is looking very nice now. Unfortunately timing is against us so this won't
go into mainline until next cycle now as IIO is effectively closed for the coming
merge window.

Thanks,

Jonathan

>
> Signed-off-by: Mihail Chindris <mihail.chindris@xxxxxxxxxx>
> ---
> drivers/iio/dac/Kconfig | 10 +
> drivers/iio/dac/Makefile | 1 +
> drivers/iio/dac/ad3552r.c | 1199 +++++++++++++++++++++++++++++++++++++
> 3 files changed, 1210 insertions(+)
> create mode 100644 drivers/iio/dac/ad3552r.c

...

> +
> +struct ad3552r_desc {
> + /* Used to look the spi bus for atomic operations where needed */

This comment should be slightly expanded upon. It's also protecting accesses
to buf_data below.

> + struct mutex lock;
> + struct gpio_desc *gpio_reset;
> + struct gpio_desc *gpio_ldac;
> + struct spi_device *spi;
> + struct ad3552r_ch_data ch_data[AD3552R_NUM_CH];
> + struct iio_chan_spec channels[AD3552R_NUM_CH + 1];
> + unsigned long enabled_ch;
> + unsigned int num_ch;
> + enum ad3542r_id chip_id;
> + /*
> + * The maximum spi transfer size consist 1 bytes (reg address)
> + * + 2 registers of 3 bytes + 1 reg of 1 byte (SW LDAC)
> + */
> + u8 buf_data[8] ____cacheline_aligned;
> +};
> +

...

> +
> +static int ad3552r_configure_custom_gain(struct ad3552r_desc *dac,
> + struct fwnode_handle *child,
> + u32 ch)
> +{
> + struct device *dev = &dac->spi->dev;
> + struct fwnode_handle *gain_child;
> + u32 val;
> + int err;
> + u8 addr;
> + u16 mask = 0, reg = 0, offset;
> +
> + gain_child = fwnode_get_named_child_node(child,
> + "custom-output-range-config");
> + if (IS_ERR(gain_child)) {
> + dev_err(dev,
> + "mandatory custom-output-range-config property missing\n");
> + return PTR_ERR(gain_child);
> + }
> +
> + dac->ch_data[ch].range_override = 1;
> + reg |= ad3552r_field_prep(1, AD3552R_MASK_CH_RANGE_OVERRIDE);
> + mask |= AD3552R_MASK_CH_RANGE_OVERRIDE;
> +
> + err = fwnode_property_read_u32(gain_child, "adi,gain-scaling-p", &val);
> + if (err) {
> + dev_err(dev, "mandatory adi,gain-scaling-p property missing\n");
> + goto put_child;
> + }
> + reg |= ad3552r_field_prep(val, AD3552R_MASK_CH_GAIN_SCALING_P);
> + mask |= AD3552R_MASK_CH_GAIN_SCALING_P;
> + dac->ch_data[ch].p = val;
> +
> + err = fwnode_property_read_u32(gain_child, "adi,gain-scaling-n", &val);
> + if (err) {
> + dev_err(dev, "mandatory adi,gain-scaling-n property missing\n");
> + goto put_child;
> + }
> + reg |= ad3552r_field_prep(val, AD3552R_MASK_CH_GAIN_SCALING_N);
> + mask |= AD3552R_MASK_CH_GAIN_SCALING_N;
> + dac->ch_data[ch].n = val;
> +
> + err = fwnode_property_read_u32(gain_child, "adi,rfb-ohms", &val);
> + if (err) {
> + dev_err(dev, "mandatory adi,rfb-ohms property missing\n");
> + goto put_child;
> + }
> + dac->ch_data[ch].rfb = val;
> +
> + err = fwnode_property_read_u32(gain_child, "adi,gain-offset", &val);
> + if (err) {
> + dev_err(dev, "mandatory adi,gain-offset property missing\n");
> + goto put_child;
> + }
> + dac->ch_data[ch].gain_offset = val;
> +
> + offset = abs((s32)val);
> + reg |= ad3552r_field_prep((offset >> 8), AD3552R_MASK_CH_OFFSET_BIT_8);
> + mask |= AD3552R_MASK_CH_OFFSET_BIT_8;
> +
> + reg |= ad3552r_field_prep((s32)val < 0, AD3552R_MASK_CH_OFFSET_POLARITY);
> + mask |= AD3552R_MASK_CH_OFFSET_POLARITY;
> + addr = AD3552R_REG_ADDR_CH_GAIN(ch);
> + err = ad3552r_write_reg(dac, addr,
> + offset & AD3552R_MASK_CH_OFFSET_BITS_0_7);
> + if (err) {
> + dev_err(dev, "Error writing register\n");
> + goto put_child;
> + }
> +
> + err = ad3552r_write_reg(dac, addr, reg);
> + if (err) {
> + dev_err(dev, "Error writing register\n");
> + goto put_child;
> + }
> +
> +put_child:
> + fwnode_handle_put(gain_child);
> +
> + return err;
> +}
> +
> +static int ad3552r_configure_device(struct ad3552r_desc *dac)
> +{
> + struct device *dev = &dac->spi->dev;
> + struct fwnode_handle *child;
> + struct regulator *vref;
> + int err, cnt = 0, voltage, delta = 100000;
> + u32 vals[2], val, ch;
> + bool is_custom;

Set but not used as per the 0-day warning that came in for v3 (after
you had already sent v4)

> +
> + dac->gpio_ldac = devm_gpiod_get_optional(dev, "ldac", GPIOD_OUT_HIGH);
> + if (IS_ERR(dac->gpio_ldac))
> + return dev_err_probe(dev, PTR_ERR(dac->gpio_ldac),
> + "Error getting gpio ldac");
> +
> + vref = devm_regulator_get_optional(dev, "vref");
> + if (IS_ERR(vref)) {
> + if (PTR_ERR(vref) != -ENODEV)
> + return dev_err_probe(dev, PTR_ERR(vref),
> + "Error getting vref");
> + vref = NULL;
> + }
> + if (vref) {
> + voltage = regulator_get_voltage(vref);
> + if (voltage > 2500000 + delta || voltage < 2500000 - delta) {
> + dev_warn(dev, "vref-supply must be 2.5V");
> + return -EINVAL;
> + }
> + val = AD3552R_EXTERNAL_VREF_PIN_INPUT;
> + } else {
> + if (device_property_read_bool(dev, "adi,vref-out-en"))
> + val = AD3552R_INTERNAL_VREF_PIN_2P5V;
> + else
> + val = AD3552R_INTERNAL_VREF_PIN_FLOATING;
> + }
> + err = ad3552r_update_reg_field(dac,
> + addr_mask_map[AD3552R_VREF_SELECT][0],
> + addr_mask_map[AD3552R_VREF_SELECT][1],
> + val);
> + if (err)
> + return err;
> +
> + err = device_property_read_u32(dev, "adi,sdo-drive-strength", &val);
> + if (!err) {
> + if (val > 3) {
> + dev_err(dev, "adi,sdo-drive-strength must be less than 4\n");
> + return -EINVAL;
> + }
> +
> + err = ad3552r_update_reg_field(dac,
> + addr_mask_map[AD3552R_SDO_DRIVE_STRENGTH][0],
> + addr_mask_map[AD3552R_SDO_DRIVE_STRENGTH][1],
> + val);
> + if (err)
> + return err;
> + }
> +
> + dac->num_ch = device_get_child_node_count(dev);
> + if (!dac->num_ch) {
> + dev_err(dev, "No channels defined\n");
> + return -ENODEV;
> + }
> +
> + device_for_each_child_node(dev, child) {
> + err = fwnode_property_read_u32(child, "reg", &ch);
> + if (err) {
> + dev_err(dev, "mandatory reg property missing\n");
> + goto put_child;
> + }
> + if (ch >= AD3552R_NUM_CH) {
> + dev_err(dev, "reg must be less than %d\n",
> + AD3552R_NUM_CH);
> + err = -EINVAL;
> + goto put_child;
> + }
> +
> + if (fwnode_property_present(child, "adi,output-range-microvolt")) {
> + is_custom = false;
> + err = fwnode_property_read_u32_array(child,
> + "adi,output-range-microvolt",
> + vals,
> + 2);
> + if (err) {
> + dev_err(dev,
> + "mandatory adi,output-range-microvolt property missing\n");
> + goto put_child;
> + }
> +
> + err = ad3552r_find_range(dac->chip_id, vals);
> + if (err < 0) {
> + dev_err(dev,
> + "Invalid adi,output-range-microvolt value\n");
> + goto put_child;
> + }
> + val = err;
> + err = ad3552r_set_ch_value(dac,
> + AD3552R_CH_OUTPUT_RANGE_SEL,
> + ch, val);
> + if (err)
> + goto put_child;
> +
> + dac->ch_data[ch].range = val;
> + } else {
> + is_custom = true;
> + err = ad3552r_configure_custom_gain(dac, child, ch);
> + if (err)
> + goto put_child;
> + }
> +
> + ad3552r_calc_gain_and_offset(dac, ch);
> + dac->enabled_ch |= BIT(ch);
> +
> + err = ad3552r_set_ch_value(dac, AD3552R_CH_SELECT, ch, 1);
> + if (err < 0)
> + goto put_child;
> +
> + dac->channels[cnt] = AD3552R_CH_DAC(ch);
> + ++cnt;
> +
> + }
> +
> + /* Disable unused channels */
> + for_each_clear_bit(ch, &dac->enabled_ch, AD3552R_NUM_CH) {
> + err = ad3552r_set_ch_value(dac, AD3552R_CH_AMPLIFIER_POWERDOWN,
> + ch, 0);
> + if (err)
> + return err;
> + }
> +
> + dac->num_ch = cnt;
> +
> + return 0;
> +put_child:
> + fwnode_handle_put(child);
> +
> + return err;
> +}
> +

...

Next message: amirmizi6: "[PATCH v17 6/6] tpm: Add YAML schema for TPM TIS I2C options"
Previous message: Christophe JAILLET: "[PATCH 2/2] RDMA/rxe: Use 'bitmap_zalloc()' when applicable"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]