linux-kernel - RE: [PATCH v3 3/3] iio: dac: ad3530r: Add driver for AD3530R and AD3531R

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID:
 <PH0PR03MB714159E822B96E7A788A5CD4F9AA2@PH0PR03MB7141.namprd03.prod.outlook.com>
Date: Mon, 7 Apr 2025 08:01:38 +0000
From: "Paller, Kim Seer" <KimSeer.Paller@...log.com>
To: David Lechner <dlechner@...libre.com>,
        Jonathan Cameron
	<jic23@...nel.org>,
        Lars-Peter Clausen <lars@...afoo.de>,
        "Hennerich,
 Michael" <Michael.Hennerich@...log.com>,
        Rob Herring <robh@...nel.org>,
        Krzysztof Kozlowski <krzk+dt@...nel.org>,
        Conor Dooley <conor+dt@...nel.org>
CC: "linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>
Subject: RE: [PATCH v3 3/3] iio: dac: ad3530r: Add driver for AD3530R and
 AD3531R

> > +	for (i = 0; i < chip_info->num_channels; i++)
> > +		st->chan[i].powerdown_mode =
> AD3530R_32KOHM_POWERDOWN_MODE;
> > +
> > +	st->ldac_gpio = devm_gpiod_get_optional(dev, "ldac",
> GPIOD_OUT_HIGH);
> > +	if (IS_ERR(st->ldac_gpio))
> > +		return dev_err_probe(dev, PTR_ERR(st->ldac_gpio),
> > +				     "Failed to get ldac GPIO\n");
> > +
> > +	if (device_property_present(dev, "adi,double-output-range")) {
> 
> This is not documented in the DT bindings.
> 
> This could instead be implemented by making the out_voltage_scale attribute
> writeable.
> 
> If we really do need it in the devicetree because we want to protect against
> someone accidentally setting it too high, then the property should be the actual
> multipler value with an enum specifier of 1, 2 and a default of 1 rather than a
> flag (e.g. adi,output-multipler).

Thank you for the feedback. This should be adi,range-double, which is already
documented in the DT bindings and is also used as a boolean type in other drivers.
I just forgot to update it here. This is a one-bit configuration that doubles the
output range (multiplier of 2).  Should I proceed with the suggested approach?

> 
> > +		st->range_multiplier = true;
> > +
> > +		return regmap_update_bits(st->regmap,
> AD3530R_OUTPUT_CONTROL_0,
> > +					  AD3530R_OUTPUT_CONTROL_MASK,
> > +
> FIELD_PREP(AD3530R_OUTPUT_CONTROL_MASK, 1));
> 
> nit: can be simplified with regmap_set_bits().
> 
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct regmap_config ad3530r_regmap_config = {
> > +	.reg_bits = 16,
> > +	.val_bits = 8,
> 
> Should have at least a .max_register.
> 
> .reg_read and .reg_write tables can make debuging easier too.
> 
> > +};
> > +
> > +static const struct iio_info ad3530r_info = {
> > +	.read_raw = ad3530r_read_raw,
> > +	.write_raw = ad3530r_write_raw,
> > +	.debugfs_reg_access = &ad3530r_reg_access,
> 
> nit: style is not consistent - can omit &.
> 
> > +};
> > +
> > +static int ad3530r_probe(struct spi_device *spi) {
> > +	static const char * const regulators[] = { "vdd", "iovdd" };
> > +	const struct ad3530r_chip_info *chip_info;
> > +	struct device *dev = &spi->dev;
> > +	struct iio_dev *indio_dev;
> > +	struct ad3530r_state *st;
> > +	int ret;
> > +
> > +	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
> > +	if (!indio_dev)
> > +		return -ENOMEM;
> > +
> > +	st = iio_priv(indio_dev);
> > +
> > +	st->regmap = devm_regmap_init_spi(spi, &ad3530r_regmap_config);
> > +	if (IS_ERR(st->regmap))
> > +		return dev_err_probe(dev, PTR_ERR(st->regmap),
> > +				     "Failed to init regmap");
> > +
> > +	ret = devm_mutex_init(dev, &st->lock);
> > +	if (ret)
> > +		return ret;
> > +
> > +	chip_info = spi_get_device_match_data(spi);
> > +	if (!chip_info)
> > +		return -ENODEV;
> > +
> > +	st->chip_info = chip_info;
> 
> nit: local variable isn't that useful since st->chip_info is short enough.
> 
> > +
> > +	ret = ad3530r_setup(st);
> > +	if (ret)
> > +		return ret;
> 
> Setting up the chip before turning on power would not work well if the
> regulators are actually controlled. :-)
> 
> > +
> > +	ret = devm_regulator_bulk_get_enable(dev, ARRAY_SIZE(regulators),
> > +					     regulators);
> > +	if (ret)
> > +		return dev_err_probe(dev, ret, "Failed to enable
> regulators\n");
> > +
> > +	ret = devm_regulator_get_enable_read_voltage(dev, "ref");
> > +	if (ret < 0 && ret != -ENODEV)
> > +		return ret;
> > +
> > +	if (chip_info->internal_ref_support && ret == -ENODEV) {
> > +		/* Internal reference. */
> > +		ret = regmap_update_bits(st->regmap,
> AD3530R_REFERENCE_CONTROL_0,
> > +
> AD3530R_REFERENCE_CONTROL_MASK,
> > +
> FIELD_PREP(AD3530R_REFERENCE_CONTROL_MASK, 1));
> > +		if (ret)
> > +			return ret;
> > +
> > +		st->vref_mv = st->range_multiplier ?
> > +			      2 * AD3530R_INTERNAL_VREF_MV :
> > +			      AD3530R_INTERNAL_VREF_MV;
> > +	} else {
> > +		st->vref_mv = st->range_multiplier ? 2 * ret / 1000 : ret / 1000;
> 
> ret can be -ENODEV here if !chip_info->internal_ref_support
> 
> > +	}> +
> > +	indio_dev->name = chip_info->name;
> > +	indio_dev->info = &ad3530r_info;
> > +	indio_dev->modes = INDIO_DIRECT_MODE;
> > +	indio_dev->channels = chip_info->channels;
> > +	indio_dev->num_channels = chip_info->num_channels;
> > +
> > +	return devm_iio_device_register(&spi->dev, indio_dev); }
> > +