linux-kernel - Re: [PATCH v2 2/2] iio: adc: ti-ads1119: Add driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Tue, 11 Jun 2024 18:26:52 +0100
From: Jonathan Cameron <Jonathan.Cameron@...wei.com>
To: Francesco Dolcini <francesco@...cini.it>
CC: Jonathan Cameron <jic23@...nel.org>, Lars-Peter Clausen <lars@...afoo.de>,
	João Paulo Gonçalves
	<jpaulo.silvagoncalves@...il.com>, Liam Girdwood <lgirdwood@...il.com>, Mark
 Brown <broonie@...nel.org>, João Paulo
 Gonçalves <joao.goncalves@...adex.com>,
	<linux-kernel@...r.kernel.org>, <linux-iio@...r.kernel.org>, Francesco
 Dolcini <francesco.dolcini@...adex.com>
Subject: Re: [PATCH v2 2/2] iio: adc: ti-ads1119: Add driver

On Mon, 10 Jun 2024 13:59:09 +0200
Francesco Dolcini <francesco@...cini.it> wrote:

> Hello Jonathan,
> thanks for the review.
> 
> On Sun, Jun 09, 2024 at 11:52:34AM +0100, Jonathan Cameron wrote:
> > On Thu,  6 Jun 2024 18:35:29 +0200
> > Francesco Dolcini <francesco@...cini.it> wrote:
> >   
> > > From: João Paulo Gonçalves <joao.goncalves@...adex.com>
> > > 
> > > The ADS1119 is a precision, 16-bit, analog-to-digital converter (ADC)
> > > that features two differential or four single-ended inputs through a
> > > flexible input multiplexer (MUX), rail-to-rail input
> > > buffers, a programmable gain stage, a voltage reference, and an
> > > oscillator.
> > > 
> > > Apart from normal single conversion, the driver also supports
> > > continuous conversion mode using a triggered buffer. However, in this
> > > mode only one channel can be scanned at a time, and it only uses the data
> > > ready interrupt as a trigger. This is because the device channels are
> > > multiplexed, and using its own data ready interrupt as a trigger guarantees
> > > the signal sampling frequency.
> > > 
> > > Datasheet: https://www.ti.com/lit/gpn/ads1119
> > > Signed-off-by: João Paulo Gonçalves <joao.goncalves@...adex.com>
> > > Signed-off-by: Francesco Dolcini <francesco.dolcini@...adex.com>  
> > 
> > A few more comments inline. Some of these I missed in the first
> > versions - sorry about that.  Takes a few passes to pick up
> > on everything unfortunately.
> >   
> > > diff --git a/drivers/iio/adc/ti-ads1119.c b/drivers/iio/adc/ti-ads1119.c
> > > new file mode 100644
> > > index 000000000000..ea0573f07279
> > > --- /dev/null
> > > +++ b/drivers/iio/adc/ti-ads1119.c
> > > @@ -0,0 +1,850 @@  
> 
> ...
> 
> > > +
> > > +static int ads1119_cmd(struct ads1119_state *st, unsigned int cmd)
> > > +{
> > > +	dev_dbg(&st->client->dev, "cmd: %#x\n", cmd);
> > > +
> > > +	return i2c_smbus_write_byte(st->client, cmd);  
> > I'm not a fan of tiny wrappers to add debug info.
> > The i2c core has trace points that let you get to the relevant data. Better
> > to use those for debug and flatten this code so we
> > see the actual bus accesses inline.  
> 
> Fine on removing the debug prints, we'll do.
> 
> However ads1119_cmd_rdata() and ads1119_update_config_reg() wrappers are
> making the error handling in the caller easier.

for rdata() only 2 calls so not bad to add the error handling in those
locations.

The update one is more justifiable given the read modify write and that
it is reasonably common.  So perhaps just keep that one, but it is
odd that your cmd_wreg() hard codes the address and cmd_rreg() doesn't.
If keeping the update function I would pass in the register address.

Maybe better would be to just cache the contents of the cmd register.
Then you can avoid a bunch or RMW and make them simply a write.

Wrapping that up makes sense as both a register write and on success
update of a cached value.

Pity this isn't quite all register based as if it was I'd say just
use regmap which gives you all the accessors for free.


> 
> Either we remove only ads1119_cmd/ads1119_cmd_wreg or we keep them all
> for orthogonality. Both works for me, just let me know if you agree and
> which option do you prefer. Personally I would keep them all without
> debug prints.

I'd drop them, but have a update_config_reg() that uses a cached
value to avoid the reads and updates that cached value on the write
succeeding.  That one is non trivial.

> 
> > > +
> > > +	return read_poll_timeout(ads1119_data_ready,
> > > +				 data_ready,
> > > +				 (data_ready == true),
> > > +				 wait_time,
> > > +				 ADS1119_MAX_DRDY_TIMEOUT,
> > > +				 false,
> > > +				 st);  
> > Over wrapped. Aim for closer to 80 chars.  
> 
> I'll do.
> Do you have any rules on the IIO subsystem driver? IOW is it fine to
> have line slightly longer than 80chars if this improves readability ?

Absolutely.  Good to keep to 80 chars except:
1) String involved - don't break it up as can't grep - those can go way
   over 80 chars
2) Non trivial readability improvement.  Then stay under 100 chars.
   If you need to go over that, normally can use a local variable
   or similar to avoid it.


> 
> Francesco
> 
>