On Fri, 21 Jul 2023 21:47:37 +0200
Andrea Collamati <andrea.collamati@xxxxxxxxx> 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
>
>
