Received-SPF: pass (google.com: domain of linux-kernel+bounces-208121-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Mon, 10 Jun 2024 13:59:09 +0200
From: Francesco Dolcini <francesco@dolcini.it>
To: Jonathan Cameron <jic23@kernel.org>
Cc: Francesco Dolcini <francesco@dolcini.it>,
	Lars-Peter Clausen <lars@metafoo.de>,
	=?iso-8859-1?Q?Jo=E3o_Paulo_Gon=E7alves?= <jpaulo.silvagoncalves@gmail.com>,
	Liam Girdwood <lgirdwood@gmail.com>,
	Mark Brown <broonie@kernel.org>,
	=?iso-8859-1?Q?Jo=E3o_Paulo_Gon=E7alves?= <joao.goncalves@toradex.com>,
	linux-kernel@vger.kernel.org, linux-iio@vger.kernel.org,
	Francesco Dolcini <francesco.dolcini@toradex.com>
Subject: Re: [PATCH v2 2/2] iio: adc: ti-ads1119: Add driver
Message-ID: <20240610115909.GA27057@francesco-nb>
References: <20240606163529.87528-1-francesco@dolcini.it>
 <20240606163529.87528-3-francesco@dolcini.it>
 <20240609115234.20e08840@jic23-huawei>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20240609115234.20e08840@jic23-huawei>

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@dolcini.it> wrote:
> 
> > From: Jo?o Paulo Gon?alves <joao.goncalves@toradex.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@toradex.com>
> > Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.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