On Wed, 12 Feb 2025 12:23:15 +0000
"Sperling, Tobias" <Tobias.Sperling@xxxxxxxxxxx> wrote:
> Hi Jonathan,
>
> I'll change the things you've mentioned. Just some comments inline.
>
> Regards,
> Tobias
>
> > Von: Jonathan Cameron <jic23@xxxxxxxxxx>
> > Gesendet: Samstag, 8. Februar 2025 16:05
> >
> > > From: Tobias Sperling <tobias.sperling@xxxxxxxxxxx>
> > >
> > > Add driver for ADS7128 and ADS7138 12-bit, 8-channel analog-to-digital
> > > converters. These ADCs have a wide operating range and a wide feature
> > > set. Communication is based on the I2C interface.
> > > ADS7128 differs in the addition of further hardware features, like a
> > > root-mean-square (RMS) and a zero-crossing-detect (ZCD) module.
> > >
> > > Signed-off-by: Tobias Sperling <tobias.sperling@xxxxxxxxxxx>
> >
> > Hi Tobias,
> >
> > Minor comments below and one question about power management
> >
> > Thanks,
> >
> > Jonathan
> >
> >
> > > +static int ads71x8_i2c_write(const struct i2c_client *client, u8 reg, u8 value)
> > > +{
> > > + return ads71x8_i2c_write_block(client, reg, &value, sizeof(value));
> >
> > Maybe this should use the single register write (figure 35) rather than bulk one?
> > It makes no real difference though other than different opcode.
>
> Yeah can be done, but as there's no difference I didn't want to introduce yet
> another function just for single writes. However, as you mentioned below,
> ads71x8_i2c_setclear_bit() can be used for that, too. So I'll change that.
>
> > > +}
> > > +
> > > +static int ads71x8_i2c_setclear_bit(const struct i2c_client *client, u8 reg,
> > > + u8 bits, u8 opcode)
> >
> > > +{
> > > + u8 buf[3] = { opcode, reg, bits };
> > > + int ret;
> > > +
> > > + ret = i2c_master_send(client, buf, ARRAY_SIZE(buf));
> > > + if (ret < 0)
> > > + return ret;
> > > + if (ret != ARRAY_SIZE(buf))
> > > + return -EIO;
> > > +
> > > + return 0;
> > > +}
> >
> > Whilst this is currently just used for setclear_bit, it is slightly more general
> > so maybe the name should reflect that it could be used for single register
> > writes for instance. Naming is hard though and I can't immediately think
> > what name covers this combination.
>
> Having something like _write_single_reg_with_opcode() in mind.
>
> > > +static int ads7138_init_hw(struct ads71x8_data *data)
> > > +{
> > > + int ret;
> > > +
> > > + data->vref_regu = devm_regulator_get_optional(&data->client->dev,
> > "avdd");
> >
> > avdd isn't optional. We need the power! As such I'd not paper over the lack
> > of it being available. To avoid weird effects on reading the scale later,
> > you may want to do a read here so that we can error out if a stub regulator
> > has been provided.
>
> Ok, just wanted to add flexibility for the enduser to not having to define it
> in the DTS, but right, AVDD needs to be connected physically. Will change
> accordingly.
In most cases they will have that flexibility anyway. It is normally
fine to not provide regulators.
The regulator core just provides a stub regulator instead that
represents an always on supply of unknown characteristics.
>
> > > +
> > > +static const struct dev_pm_ops ads71x8_pm_ops = {
> > > + RUNTIME_PM_OPS(ads71x8_runtime_suspend,
> > ads71x8_runtime_resume, NULL)
> >
> > Given it's likely that the runtime pm ops are better than nothing in
> > suspend and resume cases as well could we make this
> > DEFINE_RUNTIME_PM_OPS() which uses the runtime ops for those
> > cases as well?
> >
>
> Yes, looks like DEFINE_RUNTIME_DEV_PM_OPS() can be used to use
> these functions for the other cases, too.
>
> >
> > > +};
