Re: [PATCH v3 2/2] iio: add MCP4728 I2C DAC driver

[Andrea Collamati]

Hi Jonathan,

thank you for your first review.

See below.

On 7/20/23 21:13, Jonathan Cameron wrote:

>> +
>> +	outbuf[1] = ch->ref_mode << MCP4728_CMD_VREF_POS;
>> +
>> +	if (data->powerdown) {
>> +		u8 mcp4728_pd_mode = ch->pd_mode + 1;
>> +
>> +		outbuf[1] |= mcp4728_pd_mode << MCP4728_CMD_PDMODE_POS;
>> +	}
>> +
>> +	outbuf[1] |= ch->g_mode << MCP4728_CMD_GAIN_POS;
> FIELD_PREP()
>
>> +	outbuf[1] |= ch->dac_value >> 8;
>> +	outbuf[2] = ch->dac_value & 0xff;
> put_unaligned_be16()
>
outbuf[1] contains other data (gain mode, powerdown  mode, etc) in addition to dac value. Using put_unaligned_be16 will probably reset them.

Something like this could be the solution?

#defineMCP4728_DAC_H_FIELD GENMASK(3, 0)

#defineMCP4728_DAC_L_FIELD GENMASK(7, 0)

...

outbuf[1] |= FIELD_PREP(MCP4728_DAC_H_FIELD, ch->dac_value>> 8);

outbuf[2] = FIELD_PREP(MCP4728_DAC_L_FIELD, ch->dac_value);


>> +		return 0;
>> +}
>> +
>> +// powerdown mode
>> +static const char *const mcp4728_powerdown_modes[] = { "1kohm_to_gnd",
>> +						       "100kohm_to_gnd",
>> +						       "500kohm_to_gnd" };
>> +
>> +static int mcp4728_get_powerdown_mode(struct iio_dev *indio_dev,
>> +				      const struct iio_chan_spec *chan)
>> +{
>> +	struct mcp4728_data *data = iio_priv(indio_dev);
>> +
>> +	return data->channel_data[chan->channel].pd_mode;
>> +}
>> +
>> +static int mcp4728_set_powerdown_mode(struct iio_dev *indio_dev,
>> +				      const struct iio_chan_spec *chan,
>> +				      unsigned int mode)
>> +{
>> +	struct mcp4728_data *data = iio_priv(indio_dev);
>> +
>> +	data->channel_data[chan->channel].pd_mode = mode;
>> +
>> +	return 0;
>> +}
>> +
>> +static ssize_t mcp4728_read_powerdown(struct iio_dev *indio_dev,
>> +				      uintptr_t private,
>> +				      const struct iio_chan_spec *chan,
>> +				      char *buf)
>> +{
>> +	struct mcp4728_data *data = iio_priv(indio_dev);
>> +
>> +	return sysfs_emit(buf, "%d\n", data->powerdown);
>> +}
>> +
>> +static ssize_t mcp4728_write_powerdown(struct iio_dev *indio_dev,
>> +				       uintptr_t private,
>> +				       const struct iio_chan_spec *chan,
>> +				       const char *buf, size_t len)
>> +{
>> +	struct mcp4728_data *data = iio_priv(indio_dev);
>> +	bool state;
>> +	int ret;
>> +
>> +	ret = kstrtobool(buf, &state);
>> +	if (ret)
>> +		return ret;
>> +
>> +	if (state)
>> +		ret = mcp4728_suspend(&data->client->dev);
>> +	else
>> +		ret = mcp4728_resume(&data->client->dev);
>> +
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	return len;
> Don't support custom powering down. If this is needed it should be
> using the existing power management flows.

Could you explain better? I have extended customized powering down because I took as reference  the driver mcp4725.c and  I extended to multichannel.

How should I change it?

> Might have been more interesting if it were per channel, but it's not.
> (and I have no idea off the top of my head on how we would deal with it
> if it were per channel).

MCP4728 can handle power down per channel...

There are two bits per each channel the could be

00 => No Power Down

01 => 1kohm_to_gnd

10 => 100kohm_to_gnd

11 => 500kohm_to_gnd

>
>> +				mcp4728_resume);
>> +
>> +static int mcp4728_init_channels_data(struct mcp4728_data *data)
>> +{
>> +	u8 inbuf[MCP4728_READ_RESPONSE_LEN];
>> +	int ret;
>> +	unsigned int i;
>> +
>> +	ret = i2c_master_recv(data->client, inbuf, MCP4728_READ_RESPONSE_LEN);
>> +	if (ret < 0) {
>> +		dev_err(&data->client->dev,
>> +			"failed to read mcp4728 conf. Err=%d\n", ret);
>> +		return ret;
>> +	} else if (ret != MCP4728_READ_RESPONSE_LEN) {
>> +		dev_err(&data->client->dev,
>> +			"failed to read mcp4728 conf. Wrong Response Len ret=%d\n",
>> +			ret);
>> +		return -EIO;
>> +	}
>> +
>> +	for (i = 0; i < MCP4728_N_CHANNELS; i++) {
> Unusual to read back existing values from the device rather than resetting it into a clean
> state. What is your reasoning?

MCP4728 has an EEPROM memory.

Under the reset conditions, the device uploads the
EEPROM data into both of the DAC input and output
registers simultaneously.

My reasoning was that the driver syncs with device at probe time and let the user change each channel parameters via iio_chan_spec_ext_info.


Thank you again.

        Andrea