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