Message-ID: <20251109155312.499d5eb5@jic23-huawei>
Date: Sun, 9 Nov 2025 15:53:12 +0000
From: Jonathan Cameron <jic23@...nel.org>
To: Ariana Lazar <ariana.lazar@...rochip.com>
Cc: 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>, <linux-iio@...r.kernel.org>,
 <devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 2/2] iio: dac: adding support for Microchip
 MCP47FEB02

On Mon, 3 Nov 2025 17:50:30 +0200
Ariana Lazar <ariana.lazar@...rochip.com> 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>

A few minor things. Some of which probably overlap with Andy's comments.

Jonathan

> diff --git a/drivers/iio/dac/mcp47feb02.c b/drivers/iio/dac/mcp47feb02.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..69f5ebbc89aed8ce229cd0c6a37ca58f8a822d46
> --- /dev/null
> +++ b/drivers/iio/dac/mcp47feb02.c



> +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;
> +	u8 pd_mode;
> +
> +	guard(mutex)(&data->lock);
> +
> +	for_each_set_bit(ch, &data->active_channels_mask, data->phys_channels) {
> +		data->chdata[ch].powerdown = true;
> +		pd_mode = data->chdata[ch].powerdown_mode + 1;
> +		regmap_update_bits(data->regmap, MCP47FEB02_POWER_DOWN_REG_ADDR,
> +				   DAC_CTRL_MASK(ch), DAC_CTRL_VAL(ch, pd_mode));

ret = 

> +		if (ret)
> +			return ret;
> +
> +		ret = regmap_write(data->regmap, ch << 3, data->chdata[ch].dac_data);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}


> +static void mcp47feb02_init_scale(struct mcp47feb02_data *data, enum mcp47feb02_scale scale,
> +				  int vref_mv, int scale_avail[])
> +{
> +	int value_micro, value_int;
> +	s64 tmp;
> +
> +	tmp = (s64)vref_mv * 1000000LL >> data->info->resolution;

MICRO or similar appropriate and avoids need to count zeros.

> +	value_int = div_s64_rem(tmp, 1000000LL, &value_micro);
> +	scale_avail[scale * 2] = value_int;
> +	scale_avail[scale * 2 + 1] = value_micro;
> +}

> +
> +static void mcp47feb02_get_scale_avail(struct mcp47feb02_data *data, int *val, int *val2,
> +				       enum mcp47feb02_scale scale, int ch)

I'm not really following why this is get_scale_avail.  Just seems to be getting
the scale from the index.  The function name should probably be less
about 'how' than 'what' it is doing.

> +{
> +	if (data->phys_channels >= 4 && (ch % 2)) {
> +		*val = data->scale_1[scale * 2];
> +		*val2 = data->scale_1[scale * 2 + 1];
> +	} else {
> +		*val = data->scale[scale * 2];
> +		*val2 = data->scale[scale * 2 + 1];
> +	}
> +}


> +
> +static int mcp47feb02_read_label(struct iio_dev *indio_dev,
> +				 struct iio_chan_spec const *ch, char *label)
> +{
> +	struct mcp47feb02_data *data = iio_priv(indio_dev);
> +
> +	return sysfs_emit(label, "%s\n", data->labels[ch->address]);
> +
> +	return 0;

I'm a bit surprised the compiler isn't moaning about unreachable code.

> +}

> +
> +static int mcp47feb02_init_ctrl_regs(struct mcp47feb02_data *data)
> +{
> +	int ret, i, vref_ch, gain_ch, pd_ch, pd_tmp;
> +	struct device *dev = &data->client->dev;
> +
> +	ret = regmap_read(data->regmap, MCP47FEB02_VREF_REG_ADDR, &vref_ch);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_read(data->regmap, MCP47FEB02_GAIN_BIT_STATUS_REG_ADDR, &gain_ch);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_read(data->regmap, MCP47FEB02_POWER_DOWN_REG_ADDR, &pd_ch);
> +	if (ret)
> +		return ret;
> +
> +	gain_ch = gain_ch >> 8;

Is this extracting a field from a register? Probably better as a mask definition and
FIELD_GET()

> +	for_each_set_bit(i, &data->active_channels_mask, data->phys_channels) {
> +		data->chdata[i].ref_mode = (vref_ch >> (2 * i)) & SET_DAC_CTRL_MASK;
> +		data->chdata[i].use_2x_gain = (gain_ch >> i)  & SET_GAIN_BIT;
> +
> +		/*
> +		 * Inform the user that the current voltage reference read from volatile
> +		 * register of the chip is different from the one from device tree.
> +		 * You can't have an external voltage reference connected to the pin and
> +		 * select the internal BandGap, because the VREF pin is either an input or
> +		 * an output. When the DAC’s voltage reference is configured as the VREF pin,
> +		 * the pin is an input. When the DAC’s voltage reference is configured as the
> +		 * internal band gap, the pin is an output.
> +		 */
> +		if (data->chdata[i].ref_mode == MCP47FEB02_INTERNAL_BAND_GAP) {
> +			if (data->phys_channels >= 4 && (i % 2)) {
> +				if (data->use_vref1)
> +					dev_info(dev, "cannot use Vref1 and internal BandGap");
> +			} else {
> +				if (data->use_vref)
> +					dev_info(dev, "cannot use Vref and internal BandGap");
> +			}
> +		}
> +
> +		pd_tmp = (pd_ch >> (2 * i)) & SET_DAC_CTRL_MASK;
> +		data->chdata[i].powerdown_mode = pd_tmp ? (pd_tmp - 1) : pd_tmp;
> +		data->chdata[i].powerdown = !!(data->chdata[i].powerdown_mode);
> +	}
> +
> +	return 0;
> +}

> +
> +static int mcp47feb02_probe(struct i2c_client *client)
> +{
> +	const struct i2c_device_id *id = i2c_client_get_device_id(client);
> +	const struct mcp47feb02_features *info;
> +	struct device *dev = &client->dev;
> +	struct mcp47feb02_data *data;
> +	struct iio_dev *indio_dev;
> +	int vref1_mv = 0;
> +	int vref_mv = 0;
> +	int vdd_mv = 0;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	data = iio_priv(indio_dev);
> +	data->client = client;
> +	info = i2c_get_match_data(client);
> +	if (!info)
> +		return -EINVAL;
> +
> +	data->info = info;
> +
> +	if (info->have_eeprom) {
> +		data->regmap = devm_regmap_init_i2c(client, &mcp47feb02_regmap_config);
> +		indio_dev->info = &mcp47feb02_info;
> +	} else {
> +		data->regmap = devm_regmap_init_i2c(client, &mcp47fvb02_regmap_config);
> +		indio_dev->info = &mcp47fvb02_info;
> +	}
> +
> +	if (IS_ERR(data->regmap))
> +		return dev_err_probe(dev, PTR_ERR(data->regmap), "Error initializing i2c regmap\n");
> +
> +	indio_dev->name = id->name;

This is fragile because it ultimately looks up in one type of
firmware matching structure when we might have probed from another and
hence relies on names matching precisely across those structures.
Best to avoid that. Just embed the name string in your info structure instead.

> +
> +	ret = mcp47feb02_parse_fw(indio_dev, info);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Error parsing devicetree data\n");
Error parsing firmware data.  As it should be, your code is firmware type independent
so error messages should not suggest it is device tree only.


> +
> +	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;
> +
> +	ret = devm_regulator_get_enable_read_voltage(dev, "vref");
> +	if (ret > 0) {
> +		vref_mv = ret / 1000;
> +		data->use_vref = true;
> +	} else {
> +		dev_info(dev, "Vref is unavailable, internal band gap can be used instead\n");

Feels too noisy.  dev_dbg() appropriate I think.

> +	}
> +
> +	if (info->have_ext_vref1) {
> +		ret = devm_regulator_get_enable_read_voltage(dev, "vref1");
> +		if (ret > 0) {
> +			vref1_mv = ret / 1000;
> +			data->use_vref1 = true;
> +		} else {
> +			dev_info(dev,
> +				 "Vref1 is unavailable, internal band gap can be used instead\n");

Likewise, dev_dbg().

> +		}
> +	}
> +
> +	ret = mcp47feb02_init_ctrl_regs(data);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Error initialising vref register\n");
> +
> +	ret = mcp47feb02_init_ch_scales(data, vdd_mv, vref_mv, vref1_mv);
> +	if (ret)
> +		return ret;
> +
> +	return devm_iio_device_register(dev, indio_dev);
> +}

Powered by blists - more mailing lists