Re: [PATCH v3 2/2] iio: adc: Add driver for ADS7128 / ADS7138

[Jonathan Cameron <jic23@kernel.org>]

On Wed, 12 Feb 2025 12:23:15 +0000
"Sperling, Tobias" <Tobias.Sperling@Softing.com> wrote:

> Hi Jonathan,
> 
> I'll change the things you've mentioned. Just some comments inline.
> 
> Regards,
> Tobias
> 
> > Von: Jonathan Cameron <jic23@kernel.org>
> > Gesendet: Samstag, 8. Februar 2025 16:05
> >   
> > > From: Tobias Sperling <tobias.sperling@softing.com>
> > >
> > > 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@softing.com>  
> > 
> > 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.
> 
> >   
> > > +};