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: Mon, 10 Jun 2024 13:59:09 +0200
From: Francesco Dolcini <francesco@...cini.it>
To: Jonathan Cameron <jic23@...nel.org>
Cc: Francesco Dolcini <francesco@...cini.it>,
	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

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.

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.

> > +
> > +	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 ?

Francesco