Message-ID: <859d8472a8f9e8d28b890ad565f9d3ce11e162d5.camel@gmail.com>
Date: Mon, 22 Sep 2025 21:10:47 +0100
From: Nuno Sá <noname.nuno@...il.com>
To: Ariana Lazar <ariana.lazar@...rochip.com>, Jonathan Cameron
	 <jic23@...nel.org>, David Lechner <dlechner@...libre.com>, Nuno
 Sá
	 <nuno.sa@...log.com>, Andy Shevchenko <andy@...nel.org>, Rob Herring
	 <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
	 <conor+dt@...nel.org>
Cc: linux-iio@...r.kernel.org, devicetree@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] iio: dac: adding support for Microchip MCP47FEB02

Hi Ariana,

Thanks for your patches. Some initial comments from me...

On Mon, 2025-09-22 at 14:30 +0300, Ariana Lazar wrote:
> This is the iio driver for Microchip MCP47F(E/V)B(0/1/2)1, MCP47F(E/V)B(0/1/2)2,
> MCP47F(E/V)B(0/1/2)4 and MCP47F(E/V)B(0/1/2)8 series of buffered voltage output
> Digital-to-Analog Converters with nonvolatile or volatile memory and an I2C
> Interface.
> 
> The families support up to 8 output channels.
> 
> The devices can be 8-bit, 10-bit and 12-bit.
> 
> Signed-off-by: Ariana Lazar <ariana.lazar@...rochip.com>
> ---
>  MAINTAINERS                  |    1 +
>  drivers/iio/dac/Kconfig      |   16 +
>  drivers/iio/dac/Makefile     |    1 +
>  drivers/iio/dac/mcp47feb02.c | 1347 ++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 1365 insertions(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index
> 6f51890cfc3081bc49c08fddc8af526c1ecc8d72..0f97f90ac2f492895d27da86d831df83cb402516
> 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -14943,6 +14943,7 @@ M:	Ariana Lazar <ariana.lazar@...rochip.com>
>  L:	linux-iio@...r.kernel.org
>  S:	Supported
>  F:	Documentation/devicetree/bindings/iio/dac/microchip,mcp47feb02.yaml
> +F:	drivers/iio/dac/mcp47feb02.c
>  
>  MCP4821 DAC DRIVER
>  M:	Anshul Dalal <anshulusr@...il.com>
> diff --git a/drivers/iio/dac/Kconfig b/drivers/iio/dac/Kconfig
> index
> e0996dc014a3d538ab6b4e0d50ff54ede50f1527..179ce565036e3494e4ce43bb926de31f38b547c4
> 100644
> --- a/drivers/iio/dac/Kconfig
> +++ b/drivers/iio/dac/Kconfig
> @@ -509,6 +509,22 @@ config MCP4728
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called mcp4728.
>  
> +config MCP47FEB02
> +	tristate "MCP47F(E/V)B|(0/1/2)(1/2/4/8)DAC driver"
> +	depends on I2C
> +	help
> +          Say yes here if you want to build a driver for the Microchip
> +          MCP47FEB01, MCP47FEB11, MCP47FEB21, MCP47FEB02, MCP47FEB12,
> +          MCP47FEB22, MCP47FVB01, MCP47FVB11, MCP47FVB21, MCP47FVB02,
> +          MCP47FVB12, MCP47FVB02, MCP47FVB12, MCP47FVB22, MCP47FVB04,
> +          MCP47FVB14, MCP47FVB24, MCP47FVB04, MCP47FVB08, MCP47FVB18,
> +          MCP47FVB28, MCP47FEB04, MCP47FEB14 and MCP47FEB24 having up to 8
> +          channels, 8-bit, 10-bit or 12-bit digital-to-analog converter (DAC)
> +          with I2C interface.
> +
> +          To compile this driver as a module, choose M here: the module
> +          will be called mcp47feb02.
> +
>  config MCP4821
>  	tristate "MCP4801/02/11/12/21/22 DAC driver"
>  	depends on SPI
> diff --git a/drivers/iio/dac/Makefile b/drivers/iio/dac/Makefile
> index
> 3684cd52b7fa9bc0ad9f855323dcbb2e4965c404..d633a6440fc4b9aba7d8b1c209b6dcd05cd982dd
> 100644
> --- a/drivers/iio/dac/Makefile
> +++ b/drivers/iio/dac/Makefile
> @@ -50,6 +50,7 @@ obj-$(CONFIG_MAX5522) += max5522.o
>  obj-$(CONFIG_MAX5821) += max5821.o
>  obj-$(CONFIG_MCP4725) += mcp4725.o
>  obj-$(CONFIG_MCP4728) += mcp4728.o
> +obj-$(CONFIG_MCP47FEB02) += mcp47feb02.o
>  obj-$(CONFIG_MCP4821) += mcp4821.o
>  obj-$(CONFIG_MCP4922) += mcp4922.o
>  obj-$(CONFIG_STM32_DAC_CORE) += stm32-dac-core.o
> diff --git a/drivers/iio/dac/mcp47feb02.c b/drivers/iio/dac/mcp47feb02.c
> new file mode 100644
> index
> 0000000000000000000000000000000000000000..c9c2ded78d9c6013e5618e6342ebc8f50e79a31e
> --- /dev/null
> +++ b/drivers/iio/dac/mcp47feb02.c
> @@ -0,0 +1,1347 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * IIO driver for MCP47FEB02 Multi-Channel DAC with I2C interface
> + *
> + * Copyright (C) 2025 Microchip Technology Inc. and its subsidiaries
> + *
> + * Author: Ariana Lazar <ariana.lazar@...rochip.com>
> + *
> + * Datasheet for MCP47FEBXX can be found here:
> + *
> https://ww1.microchip.com/downloads/aemDocuments/documents/OTH/ProductDocuments/DataSheets/20005375A.pdf
> + *
> + * Datasheet for MCP47FVBXX can be found here:
> + *
> https://ww1.microchip.com/downloads/aemDocuments/documents/OTH/ProductDocuments/DataSheets/20005405A.pdf
> + *
> + * Datasheet for MCP47FXBX4/8 can be found here:
> + *
> https://ww1.microchip.com/downloads/aemDocuments/documents/MSLD/ProductDocuments/DataSheets/MCP47FXBX48-Data-Sheet-DS200006368A.pdf
> + */
> +#include <linux/bits.h>
> +#include <linux/bitfield.h>
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/i2c.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/module.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/mutex.h>
> +#include <linux/property.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
> +
> 

...

> +
> +static int mcp47feb02_write_to_register(struct regmap *regmap, unsigned int reg,
> +					int channel, int tmp_val)
> +{
> +	int ret, val;
> +
> +	ret = regmap_read(regmap, CMD_FORMAT(reg, READ_CMD), &val);
> +	if (ret)
> +		return ret;
> +
> +	if (reg == MCP47FEB02_GAIN_STATUS_REG_ADDR) {
> +		switch (channel) {
> +		case 0:
> +			FIELD_MODIFY(G_0, &val, tmp_val);
> +			break;
> +		case 1:
> +			FIELD_MODIFY(G_1, &val, tmp_val);
> +			break;
> +		case 2:
> +			FIELD_MODIFY(G_2, &val, tmp_val);
> +			break;
> +		case 3:
> +			FIELD_MODIFY(G_3, &val, tmp_val);
> +			break;
> +		case 4:
> +			FIELD_MODIFY(G_4, &val, tmp_val);
> +			break;
> +		case 5:
> +			FIELD_MODIFY(G_5, &val, tmp_val);
> +			break;
> +		case 6:
> +			FIELD_MODIFY(G_6, &val, tmp_val);
> +			break;
> +		case 7:
> +			FIELD_MODIFY(G_7, &val, tmp_val);
> +			break;
> +		default:
> +			FIELD_MODIFY(G_0, &val, tmp_val);
> +			break;
> +		}
> +	} else {
> +		switch (channel) {
> +		case 0:
> +			FIELD_MODIFY(CH_0, &val, tmp_val);
> +			break;
> +		case 1:
> +			FIELD_MODIFY(CH_1, &val, tmp_val);
> +			break;
> +		case 2:
> +			FIELD_MODIFY(CH_2, &val, tmp_val);
> +			break;
> +		case 3:
> +			FIELD_MODIFY(CH_3, &val, tmp_val);
> +			break;
> +		case 4:
> +			FIELD_MODIFY(CH_4, &val, tmp_val);
> +			break;
> +		case 5:
> +			FIELD_MODIFY(CH_5, &val, tmp_val);
> +			break;
> +		case 6:
> +			FIELD_MODIFY(CH_6, &val, tmp_val);
> +			break;
> +		case 7:
> +			FIELD_MODIFY(CH_7, &val, tmp_val);
> +			break;
> +		default:
> +			FIELD_MODIFY(CH_0, &val, tmp_val);
> +			break;
> +		}
> +	}
> +
> +	ret = regmap_write(regmap, CMD_FORMAT(reg, WRITE_CMD), val);

Looking at the above, this looks like a creative way of doing regmap_update_bits()?
Am I missing something?

> +
> +	return ret;

return regmap_write()

> +}
> +
> +static int mcp47feb02_write_to_eeprom(struct regmap *regmap, unsigned int reg,
> +				      unsigned int val)
> +{
> +	int eewa_val, ret;
> +
> +	/*
> +	 * wait till the currently occurring EEPROM Write Cycle is completed.
> +	 * Only serial commands to the volatile memory are allowed.
> +	 */
> +	ret = regmap_read_poll_timeout(regmap,
> +				       CMD_FORMAT(MCP47FEB02_GAIN_STATUS_REG_ADDR,
> READ_CMD),
> +				       eewa_val, !(eewa_val &
> MCP47FEB02_GAIN_STATUS_EEWA_MASK),
> +				       MCP47FEB02_DELAY_1_MS,
> MCP47FEB02_DELAY_1_MS * 5);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_write(regmap, CMD_FORMAT(reg, WRITE_CMD), val);
> +
> +	return ret;
> +}
> +
> +static ssize_t mcp47feb02_store_eeprom(struct device *dev, struct device_attribute
> *attr,
> +				       const char *buf, size_t len)
> +{
> +	struct mcp47feb02_data *data = iio_priv(dev_to_iio_dev(dev));
> +	int ret, i, val, val1;
> +	bool state;
> +
> +	ret = kstrtobool(buf, &state);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (!state)
> +		return 0;
> +
> +	if (!data->have_eeprom) {
> +		dev_err(dev, "Device has no eeprom memory.\n");
> +		return -EINVAL;
> +	}
> +
> +	/*
> +	 * Verify DAC Wiper and DAC Configuratioin are unlocked. If both are
> disabled,
> +	 * writing to EEPROM is available.
> +	 */
> +	ret = regmap_read(data->regmap,
> CMD_FORMAT(MCP47FEB02_WIPERLOCK_STATUS_REG_ADDR, READ_CMD),
> +			  &val);
> +	if (ret)
> +		return ret;
> +
> +	if (val)  {
> +		dev_err(dev, "DAC Wiper and DAC Configuration not are
> unlocked.\n");
> +		return -EINVAL;
> +	}
> +
> +	for_each_set_bit(i, &data->active_channels_mask, data->phys_channels) {
> +		ret = mcp47feb02_write_to_eeprom(data->regmap,
> MCP47FEB02_NV_DAC0_REG_ADDR + i,
> +						 data->chdata[i].dac_data);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	ret = regmap_read(data->regmap, CMD_FORMAT(MCP47FEB02_VREF_REG_ADDR,
> READ_CMD), &val);
> +	if (ret)
> +		return ret;
> +
> +	ret = mcp47feb02_write_to_eeprom(data->regmap,
> MCP47FEB02_NV_VREF_REG_ADDR, val);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_read(data->regmap, CMD_FORMAT(MCP47FEB02_POWER_DOWN_REG_ADDR,
> READ_CMD), &val);
> +	if (ret)
> +		return ret;
> +
> +	ret = mcp47feb02_write_to_eeprom(data->regmap,
> MCP47FEB02_NV_POWER_DOWN_REG_ADDR, val);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_read(data->regmap,
> +			  CMD_FORMAT(MCP47FEB02_NV_GAIN_I2C_SLAVE_REG_ADDR,
> READ_CMD), &val);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_read(data->regmap,
> +			  CMD_FORMAT(MCP47FEB02_GAIN_STATUS_REG_ADDR, READ_CMD),
> &val1);
> +	if (ret)
> +		return ret;
> +
> +	ret = mcp47feb02_write_to_eeprom(data->regmap,
> MCP47FEB02_NV_GAIN_I2C_SLAVE_REG_ADDR,
> +					 (val1 & MCP47FEB02_VOLATILE_GAIN_MASK) |
> +					  (val &
> MCP47FEB02_NV_I2C_SLAVE_ADDR_MASK));
> +	if (ret)
> +		return ret;
> +
> +	return len;
> +}
> +
> +static IIO_DEVICE_ATTR(store_eeprom, 0200, NULL, mcp47feb02_store_eeprom, 0);
> +static struct attribute *mcp47feb02_attributes[] = {
> +	&iio_dev_attr_store_eeprom.dev_attr.attr,
> +	NULL,
> +};
> +

Not going to argue about the ABI for now but I don't think this is a standard one? So
if acceptable you need an ABI doc.

> +static int mcp47feb02_suspend(struct device *dev)
> +{
> +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> +	struct mcp47feb02_data *data = iio_priv(indio_dev);
> +	int ret, ch;
> +
> +	for_each_set_bit(ch, &data->active_channels_mask, data->phys_channels) {
> +		data->chdata[ch].powerdown = true;
> +		ret = mcp47feb02_write_to_register(data->regmap,
> MCP47FEB02_POWER_DOWN_REG_ADDR,
> +						   ch, data-
> >chdata[ch].powerdown_mode + 1);
> +		if (ret)
> +			return ret;

So looking at this, can we actually powerdown channels individually?

> +
> +		ret = regmap_write(data->regmap, CMD_FORMAT(ch, WRITE_CMD),
> +				   data->chdata[ch].dac_data);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int mcp47feb02_resume(struct device *dev)
> +{
> +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> +	struct mcp47feb02_data *data = iio_priv(indio_dev);
> +	int ch, ret;
> +
> +	for_each_set_bit(ch, &data->active_channels_mask, data->phys_channels) {
> +		data->chdata[ch].powerdown = false;
> +
> +		ret = regmap_write(data->regmap, CMD_FORMAT(ch, WRITE_CMD),
> +				   data->chdata[ch].dac_data);
> +		if (ret)
> +			return ret;
> +
> +		ret = mcp47feb02_write_to_register(data->regmap,
> MCP47FEB02_VREF_REG_ADDR,
> +						   ch, data->chdata[ch].ref_mode);
> +		if (ret)
> +			return ret;
> +
> +		ret = mcp47feb02_write_to_register(data->regmap,
> MCP47FEB02_GAIN_STATUS_REG_ADDR,
> +						   ch, data-
> >chdata[ch].use_2x_gain);
> +		if (ret)
> +			return ret;
> +
> +		ret = mcp47feb02_write_to_register(data->regmap,
> MCP47FEB02_POWER_DOWN_REG_ADDR,
> +						   ch,
> MCP47FEB02_NORMAL_OPERATION);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static DEFINE_SIMPLE_DEV_PM_OPS(mcp47feb02_pm_ops, mcp47feb02_suspend,
> mcp47feb02_resume);
> +
> +static const struct attribute_group mcp47feb02_attribute_group = {
> +	.attrs = mcp47feb02_attributes,
> +};
> +
> +static const char * const mcp47feb02_powerdown_modes[] = {
> +	"1kohm_to_gnd",
> +	"100kohm_to_gnd",
> +	"open_circuit",
> +};
> +
> +static int mcp47feb02_get_powerdown_mode(struct iio_dev *indio_dev,
> +					 const struct iio_chan_spec *chan)
> +{
> +	struct mcp47feb02_data *data = iio_priv(indio_dev);
> +	int bit_set;
> +
> +	bit_set = test_bit(chan->address, &data->active_channels_mask);
> +	if (bit_set)

not seeing the need for bit_set.

> +		return data->chdata[chan->address].powerdown_mode;
> +
> +	return -EINVAL;
> +}
> +
> +static int mcp47feb02_set_powerdown_mode(struct iio_dev *indio_dev,
> +					 const struct iio_chan_spec *chan,
> unsigned int mode)
> +{
> +	struct mcp47feb02_data *data = iio_priv(indio_dev);
> +	struct device *dev = &data->client->dev;
> +	int bit_set, ret;
> +
> +	bit_set = test_bit(chan->address, &data->active_channels_mask);
> +	if (bit_set) {

same

> +		ret = mcp47feb02_write_to_register(data->regmap,
> MCP47FEB02_POWER_DOWN_REG_ADDR,
> +						   chan->address, mode + 1);
> +		if (ret)
> +			return ret;
> +
> +		data->chdata[chan->address].powerdown_mode = mode;
> +
> +		return 0;
> +	}
> +
> +	dev_err(dev, "Channel %ld not enabled\n", chan->address);
> +	return -EINVAL;
> +}
> +
> +static ssize_t mcp47feb02_read_powerdown(struct iio_dev *indio_dev, uintptr_t
> private,
> +					 const struct iio_chan_spec *chan, char
> *buf)
> +{
> +	struct mcp47feb02_data *data = iio_priv(indio_dev);
> +	int bit_set;
> +
> +	bit_set = test_bit(chan->address, &data->active_channels_mask);
> +	if (bit_set)
> +		return sysfs_emit(buf, "%d\n", data->chdata[chan-
> >address].powerdown);
> +
> +	dev_err(&data->client->dev, "Channel %ld not enabled\n", chan->address);
> +	return -EINVAL;
> +}
> +
> +static ssize_t mcp47feb02_write_powerdown(struct iio_dev *indio_dev, uintptr_t
> private,
> +					  const struct iio_chan_spec *chan, const
> char *buf,
> +					  size_t len)
> +{
> +	struct mcp47feb02_data *data = iio_priv(indio_dev);
> +	struct device *dev = &data->client->dev;
> +	int bit_set, ret;
> +	bool state;
> +
> +	ret = kstrtobool(buf, &state);
> +	if (ret)
> +		return ret;
> +
> +	bit_set = test_bit(chan->address, &data->active_channels_mask);
> +	if (bit_set)
> +		data->chdata[chan->address].powerdown = state;
> +	else
> +		dev_err(dev, "Channel %ld not enabled\n", chan->address);

I might be missing something but I'm puzzled. So powering down is just setting a
variable? I would expect some register read.

> +
> +	return len;
> +}
> +
> +static const struct iio_enum mcp47febxx_powerdown_mode_enum = {
> +	.items = mcp47feb02_powerdown_modes,
> +	.num_items = ARRAY_SIZE(mcp47feb02_powerdown_modes),
> +	.get = mcp47feb02_get_powerdown_mode,
> +	.set = mcp47feb02_set_powerdown_mode,
> +};
> +
> +static const struct iio_chan_spec_ext_info mcp47feb02_ext_info[] = {
> +	{
> +		.name = "powerdown",
> +		.read = mcp47feb02_read_powerdown,
> +		.write = mcp47feb02_write_powerdown,
> +		.shared = IIO_SEPARATE,
> +	},
> +	IIO_ENUM("powerdown_mode", IIO_SEPARATE, &mcp47febxx_powerdown_mode_enum),
> +	IIO_ENUM_AVAILABLE("powerdown_mode", IIO_SHARED_BY_TYPE,
> &mcp47febxx_powerdown_mode_enum),
> +	{ },
> +};
> +
> +static const struct iio_chan_spec mcp47febxx_ch_template = {
> +	.type = IIO_VOLTAGE,
> +	.output = 1,
> +	.indexed = 1,
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW)	|
> +			      BIT(IIO_CHAN_INFO_SCALE),
> +	.info_mask_separate_available = BIT(IIO_CHAN_INFO_SCALE),
> +	.ext_info = mcp47feb02_ext_info,
> +};
> +
> +static void mcp47feb02_init_scale(const struct mcp47feb02_features *info,
> +				  enum mcp47feb02_scale scale, int vref_mv,
> +				  int scale_avail[])
> +{
> +	int value_micro, value_int;
> +	s64 tmp;
> +
> +	tmp = (s64)vref_mv * 1000000LL >> info->resolution;
> +	value_int = div_s64_rem(tmp, 1000000LL, &value_micro);
> +	scale_avail[scale * 2] = value_int;
> +	scale_avail[scale * 2 + 1] = value_micro;
> +}
> +
> +static int mcp47feb02_init_scales_avail(const struct mcp47feb02_features *info,
> +					struct mcp47feb02_data *data, int vdd_mv,
> int vref_mv,
> +					int vref1_mv)
> +{
> +	struct device *dev = &data->client->dev;
> +	int tmp_vref;
> +
> +	if (data->use_vref && vref_mv <= 0) {
> +		dev_err(dev, "use_vref set but vref_mv invalid\n");
> +		return -EINVAL;

dev_err_probe()?

> +	}
> +
> +	mcp47feb02_init_scale(info, MCP47FEB02_SCALE_VDD, vdd_mv, data->scale);
> +
> +	if (data->use_vref)
> +		tmp_vref = vref_mv;
> +	else
> +		tmp_vref = MCP47FEB02_INTERNAL_BAND_GAP_MV;
> +
> +	mcp47feb02_init_scale(info, MCP47FEB02_SCALE_GAIN_X1, tmp_vref, data-
> >scale);
> +	mcp47feb02_init_scale(info, MCP47FEB02_SCALE_GAIN_X2, tmp_vref * 2, data-
> >scale);
> +
> +	if (data->phys_channels >= 4) {
> +		mcp47feb02_init_scale(info, MCP47FEB02_SCALE_VDD, vdd_mv, data-
> >scale_1);
> +
> +		if (data->use_vref1 && vref1_mv <= 0) {
> +			dev_err(dev, "use_vref1 set but vref1_mv invalid\n");
> +			return -EINVAL;

same

...

> +
> +static int mcp47feb02_set_scale(struct mcp47feb02_data *data, int channel, int
> scale)
> +{
> +	int tmp_val, ret;
> +
> +	mcp47feb02_ch_scale(data->phys_channels, data, channel, scale);
> +
> +	if (scale == MCP47FEB02_SCALE_GAIN_X2)
> +		tmp_val = MCP47FEB02_GAIN_X2;
> +	else
> +		tmp_val = MCP47FEB02_GAIN_X1;
> +
> +	ret = mcp47feb02_write_to_register(data->regmap,
> MCP47FEB02_GAIN_STATUS_REG_ADDR,
> +					   channel, tmp_val);
> +	if (ret)
> +		return ret;
> +
> +	data->chdata[channel].use_2x_gain = tmp_val;
> 

Also looks like it could use a lock.

> +	return 0;
> +}
> +
> +static int mcp47feb02_read_raw(struct iio_dev *indio_dev, struct iio_chan_spec
> const *chan,
> +			       int *val, int *val2, long mask)
> +{
> +	struct mcp47feb02_data *data = iio_priv(indio_dev);
> +	int ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		ret = regmap_read(data->regmap, CMD_FORMAT(chan->address,
> READ_CMD), val);
> +		if (ret)
> +			return ret;
> +
> +		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_SCALE:
> +		mcp47feb02_get_scale(chan->address, data, val, val2);
> +		return IIO_VAL_INT_PLUS_MICRO;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int mcp47feb02_write_raw(struct iio_dev *indio_dev, struct iio_chan_spec
> const *chan,
> +				int val, int val2, long mask)
> +{
> +	struct mcp47feb02_data *data = iio_priv(indio_dev);
> +	int ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		ret = regmap_write(data->regmap, CMD_FORMAT(chan->address,
> WRITE_CMD), val);
> +		data->chdata[chan->address].dac_data = val;

You should only save the value in case regmap_write() succeeds. And given that you're
doing that you should use a mutx.

> +		return ret;
> +	case IIO_CHAN_INFO_SCALE:
> +		if (data->phys_channels >= 4 && (chan->address % 2))
> +			ret = mcp47feb02_check_scale(data, val, val2, data-
> >scale_1);
> +		else
> +			ret = mcp47feb02_check_scale(data, val, val2, data-
> >scale);
> +
> +		if (ret < 0)
> +			return ret;
> +
> +		return mcp47feb02_set_scale(data, chan->address, ret);
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static const struct iio_info mcp47feb02_info = {
> +	.read_raw = mcp47feb02_read_raw,
> +	.write_raw = mcp47feb02_write_raw,
> +	.read_avail = &mcp47feb02_read_avail,
> +	.attrs = &mcp47feb02_attribute_group,
> +};
> +
> +static const struct iio_info mcp47fvb02_info = {
> +	.read_raw = mcp47feb02_read_raw,
> +	.write_raw = mcp47feb02_write_raw,
> +	.read_avail = &mcp47feb02_read_avail,
> +	.attrs = &mcp47feb02_attribute_group,
> +};
> +
> +static int mcp47feb02_parse_fw(struct iio_dev *indio_dev, const struct
> mcp47feb02_features *info)
> +{
> +	struct iio_chan_spec chanspec = mcp47febxx_ch_template;
> +	struct mcp47feb02_data *data = iio_priv(indio_dev);
> +	struct device *dev = &data->client->dev;
> +	struct iio_chan_spec *channels;
> +	u32 reg, num_channels;
> +	int chan_idx = 0;
> +
> +	num_channels = device_get_child_node_count(dev);
> +	if (num_channels > info->phys_channels)
> +		return dev_err_probe(dev, -EINVAL, "More channels than the chip
> supports\n");
> +

Are 0 channels acceptable? devm_kcalloc() wont't give you a NULL ptr and you might
get subtle issues.

> +	channels = devm_kcalloc(dev, num_channels, sizeof(*channels), GFP_KERNEL);
> +	if (!channels)
> +		return -ENOMEM;
> +
> +	device_for_each_child_node_scoped(dev, child) {
> +		fwnode_property_read_u32(child, "reg", &reg);
> +

reg is a mandatory property so you need to check for errors. If the property is not
given, fwnode_property_read_u32() won't touch in reg so you have a random number in
the next condition.

> +		if (reg >= info->phys_channels)
> +			return dev_err_probe(dev, -EINVAL,
> +					     "The index of the channels does not
> match the chip\n");
> +
> +		set_bit(reg, &data->active_channels_mask);
> +
> +		if (fwnode_property_present(child, "label"))
> +			fwnode_property_read_string(child, "label",
> +						    (const char **)&data-
> >labels[reg]);
> +
> +		chanspec.address = reg;
> +		chanspec.channel = reg;
> +		channels[chan_idx] = chanspec;
> +		chan_idx++;
> +	}
> +
> +	indio_dev->num_channels = num_channels;
> +	indio_dev->channels = channels;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	data->phys_channels = info->phys_channels;
> +
> +	/*
> +	 * check if vref-supply, vref1-supply, microchip,vref-buffered and
> +	 * microchip,vref1-buffered are defined in the devicetree
> +	 */
> +	data->use_vref = device_property_present(dev, "vref-supply");
> +	data->use_vref1 = device_property_present(dev, "vref1-supply");
> +	data->vref_buffered = device_property_read_bool(dev, "microchip,vref-
> buffered");
> +	data->vref1_buffered = device_property_read_bool(dev, "microchip,vref1-
> buffered");
> +
> +	return 0;
> +}
> +
> +static int mcp47feb02_probe(struct i2c_client *client)
> +{
> +	int err, ret, vdd_mv, vref_mv, vref1_mv, i, tmp_vref, vref_ch, gain_ch;
> +	const struct i2c_device_id *id = i2c_client_get_device_id(client);
> +	const struct mcp47feb02_features *info;
> +	enum vref_mode ref_mode, ref_mode1;
> +	struct device *dev = &client->dev;
> +	struct mcp47feb02_data *data;
> +	struct iio_dev *indio_dev;
> +
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	data = iio_priv(indio_dev);
> +	i2c_set_clientdata(client, indio_dev);

You can drop the above. See my devm_ comment below.
> +	data->client = client;
> +	info = i2c_get_match_data(client);

for consistency, add some error handling.

> +
> +	if (info->have_eeprom) {
> +		data->regmap = devm_regmap_init_i2c(client,
> &mcp47feb02_regmap_config);
> +		data->have_eeprom = true;
> +	} else {
> +		data->regmap = devm_regmap_init_i2c(client,
> &mcp47fvb02_regmap_config);
> +		data->have_eeprom = false;
> +	}
> +
> +	if (IS_ERR(data->regmap))
> +		dev_err_probe(dev, PTR_ERR(data->regmap), "Error initializing i2c
> regmap\n");
> +
> +	err = mcp47feb02_parse_fw(indio_dev, info);
> +	if (err)
> +		return dev_err_probe(dev, err, "Error parsing devicetree data\n");
> +
> +	if (!info->have_ext_vref2 && data->use_vref1)
> +		return dev_err_probe(dev, -EINVAL,
> +				     "Second External reference is unavailable on
> %s\n",
> +				     info->name);
> +
> +	ret = regmap_read(data->regmap, CMD_FORMAT(MCP47FEB02_VREF_REG_ADDR,
> READ_CMD), &vref_ch);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_read(data->regmap,
> CMD_FORMAT(MCP47FEB02_GAIN_STATUS_REG_ADDR, READ_CMD),
> +			  &gain_ch);
> +	if (ret)
> +		return ret;
> +
> +	gain_ch = gain_ch >> 8;
> +
> +	/*
> +	 * Values stored in the nonvolatile memory will be transferred to the
> volatile registers
> +	 * at startup. For safety reasons, the user receives a warning if startup
> values
> +	 * do not match the ones from current devicetree configuration.
> +	 * Nonvolatile memory can be written at any time
> +	 */
> +	for_each_set_bit(i, &data->active_channels_mask, data->phys_channels) {
> +		/* VDD can be set as Vref only with Gain x1 */
> +		if ((vref_ch & 0x03) == MCP47FEB02_VREF_VDD &&
> +		    (gain_ch & 0x01) == MCP47FEB02_GAIN_X2) {

some defines with be nice for those magic bits.

> +			dev_info(dev, "vdd can be used only with gain x1\n");
dev_dbg()
> +			ret = mcp47feb02_write_to_register(data->regmap,
> +							  
> MCP47FEB02_GAIN_STATUS_REG_ADDR,
> +							   i, MCP47FEB02_GAIN_X1);
> +			if (ret)
> +				return ret;
> +
> +			data->chdata[i].use_2x_gain = MCP47FEB02_GAIN_X1;
> +		}
> +
> +		if (data->phys_channels >= 4 && (i % 2)) {
> +			if ((vref_ch & 0x03) == MCP47FEB02_EXTERNAL_VREF_BUFFERED
> &&
> +			    data->use_vref1 && !data->vref1_buffered)
> +				dev_info(dev, "vref1 is unbuffered\n");
> +			else if ((vref_ch & 0x03) ==
> MCP47FEB02_EXTERNAL_VREF_UNBUFFERED &&
> +				 data->use_vref1 && data->vref1_buffered)
> +				dev_info(dev, "vref1 is buffered\n");
> +		} else {
> +			if ((vref_ch & 0x03) == MCP47FEB02_EXTERNAL_VREF_BUFFERED
> &&
> +			    data->use_vref && !data->vref_buffered)
> +				dev_info(dev, "vref is unbuffered\n");
> +			else if ((vref_ch & 0x03) ==
> MCP47FEB02_EXTERNAL_VREF_UNBUFFERED &&
> +				 data->use_vref && data->vref_buffered)
> +				dev_info(dev, "vref is buffered\n");
> +		}
> +
> +		vref_ch = vref_ch >> 2;
> +		gain_ch = gain_ch >> 1;
> +	}
> +
> +	if (data->use_vref)
> +		ref_mode = data->vref_buffered ?
> +			MCP47FEB02_EXTERNAL_VREF_BUFFERED :
> MCP47FEB02_EXTERNAL_VREF_UNBUFFERED;
> +	else
> +		ref_mode = MCP47FEB02_INTERNAL_BAND_GAP;
> +
> +	if (data->use_vref1)
> +		ref_mode1 = data->vref1_buffered ?
> +			MCP47FEB02_EXTERNAL_VREF_BUFFERED :
> MCP47FEB02_EXTERNAL_VREF_UNBUFFERED;
> +
> +	else
> +		ref_mode1 =  MCP47FEB02_INTERNAL_BAND_GAP;
> +
> +	for_each_set_bit(i, &data->active_channels_mask, data->phys_channels) {
> +		if (data->phys_channels >= 4 && (i % 2))
> +			tmp_vref = ref_mode1;
> +		else
> +			tmp_vref = ref_mode;
> +
> +		ret = mcp47feb02_write_to_register(data->regmap,
> MCP47FEB02_VREF_REG_ADDR,
> +						   i, tmp_vref);
> +		if (ret)
> +			return ret;
> +
> +		data->chdata[i].ref_mode = tmp_vref;
> +	}

Maybe have the above in a setup function?

> +
> +	indio_dev->name = id->name;
> +	if (info->have_eeprom)
> +		indio_dev->info = &mcp47feb02_info;
> +	else
> +		indio_dev->info = &mcp47fvb02_info;
> +
> +	ret = devm_mutex_init(dev, &data->lock);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = devm_regulator_get_enable_read_voltage(dev, "vdd");
> +	if (ret < 0)
> +		return ret;
> +
> +	vdd_mv = ret / 1000;
> +
> +	if (data->use_vref) {
> +		ret = devm_regulator_get_enable_read_voltage(dev, "vref");
> +		if (ret < 0)
> +			return ret;
> +
> +		vref_mv = ret / 1000;
> +	}
> +
> +	if (data->use_vref1) {
> +		ret = devm_regulator_get_enable_read_voltage(dev, "vref1");
> +		if (ret < 0)
> +			return ret;
> +
> +		vref1_mv = ret / 1000;
> +	}
> +
> +	for_each_set_bit(i, &data->active_channels_mask, data->phys_channels) {
> +		ret = mcp47feb02_init_scales_avail(info, data, vdd_mv, vref_mv,
> vref1_mv);
> +		if (ret)
> +			dev_err_probe(dev, ret, "failed to init scales for ch i
> %d\n", i);
> +	}

I guess the above loop could also be in a setup function.

> +
> +	err = iio_device_register(indio_dev);
> +

devm_iio_device_register(). With that you can drop mcp47feb02_remove()


- Nuno Sá

Powered by blists - more mailing lists