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

[Jonathan Cameron <jic23@kernel.org>]

On Fri, 21 Jul 2023 21:47:37 +0200
Andrea Collamati <andrea.collamati@gmail.com> wrote:

> 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.

Ah. I'd missed the |= because you then didn't mask the dac_value.

> 
> Something like this could be the solution?
> 
> #defineMCP4728_DAC_H_FIELD GENMASK(3, 0)
> 
> #defineMCP4728_DAC_L_FIELD GENMASK(7, 0)

Call them _MASK or _MSK rather than field.
I think that is much more common naming.

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

That's better.

> 
> 
> >> +		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?

Ok. I'd missed that we had an existing driver doing this and indeed
we have documented it.  Fair enough - I must have been persuaded in the past
and then forgotten about 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
> 

Ok.  Good to support that rather than a full power down.

> >  
> >> +				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.

Ok - this is fine if it's reading back from EEPROM.
Please add a comment on that though because as demonstrated above
I'm forgetful and may wonder why it was done like this in the future.

Jonathan


> 
> 
> Thank you again.
> 
>         Andrea
> 
>