Received-SPF: pass (google.com: domain of linux-kernel+bounces-82213-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Mon, 26 Feb 2024 19:10:11 +0000
From: Jonathan Cameron <jic23@kernel.org>
To: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Subhajit Ghosh <subhajit.ghosh@tweaklogic.com>, Lars-Peter Clausen
 <lars@metafoo.de>, Rob Herring <robh+dt@kernel.org>, Krzysztof Kozlowski
 <krzysztof.kozlowski+dt@linaro.org>, Conor Dooley <conor+dt@kernel.org>,
 Matti Vaittinen <mazziesaccount@gmail.com>, Marek Vasut <marex@denx.de>,
 Anshul Dalal <anshulusr@gmail.com>, Javier Carrasco
 <javier.carrasco.cruz@gmail.com>, Matt Ranostay <matt@ranostay.sg>, Stefan
 Windfeldt-Prytz <stefan.windfeldt-prytz@axis.com>,
 linux-iio@vger.kernel.org, devicetree@vger.kernel.org,
 linux-kernel@vger.kernel.org
Subject: Re: [PATCH v7 5/5] iio: light: Add support for APDS9306 Light
 Sensor
Message-ID: <20240226190953.1467da54@jic23-huawei>
In-Reply-To: <ZdycR6nr3rtrnuth@smile.fi.intel.com>
References: <20240218054826.2881-1-subhajit.ghosh@tweaklogic.com>
	<20240218054826.2881-6-subhajit.ghosh@tweaklogic.com>
	<ZdycR6nr3rtrnuth@smile.fi.intel.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit

On Mon, 26 Feb 2024 16:12:23 +0200
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> On Sun, Feb 18, 2024 at 04:18:26PM +1030, Subhajit Ghosh wrote:
> > Driver support for Avago (Broadcom) APDS9306 Ambient Light Sensor.
> > It has two channels - ALS and CLEAR. The ALS (Ambient Light Sensor)
> > channel approximates the response of the human-eye providing direct
> > read out where the output count is proportional to ambient light levels.
> > It is internally temperature compensated and rejects 50Hz and 60Hz flicker
> > caused by artificial light sources. Hardware interrupt configuration is
> > optional. It is a low power device with 20 bit resolution and has
> > configurable adaptive interrupt mode and interrupt persistence mode.
> > The device also features inbuilt hardware gain, multiple integration time
> > selection options and sampling frequency selection options.
> > 
> > This driver also uses the IIO GTS (Gain Time Scale) Helpers Namespace for
> > Scales, Gains and Integration time implementation.  
> 
> ...
> 
> > +/*
> > + * Available scales with gain 1x - 18x, timings 3.125, 25, 50, 100, 200, 400 mS  
> 
> "mS" --> "ms."
> 
> > + * Time impacts to gain: 1x, 8x, 16x, 32x, 64x, 128x
> > + */  
> 
> ...
> 
> > +	/*
> > +	 * If this function runs parallel with the interrupt handler, either
> > +	 * this reads and clears the status registers or the interrupt handler
> > +	 * does. The interrupt handler sets a flag for read data available
> > +	 * in our private structure which we read here.
> > +	 */
> > +	ret = regmap_read_poll_timeout(data->regmap, APDS9306_MAIN_STATUS_REG,
> > +				       status, data->read_data_available ||
> > +				       (status & (APDS9306_ALS_DATA_STAT_MASK |
> > +						  APDS9306_ALS_INT_STAT_MASK)),
> > +				       APDS9306_ALS_READ_DATA_DELAY_US, delay * 2);  
> 
> > +  
> 
> Redundant blank line
> 
> > +	if (ret)
> > +		return ret;  
> 
> ...
> 
> > +static int apds9306_init_iio_gts(struct apds9306_data *data)
> > +{
> > +	int i, ret, part_id;
> > +
> > +	ret = regmap_read(data->regmap, APDS9306_PART_ID_REG, &part_id);
> > +	if (ret)
> > +		return ret;
> > +
> > +	for (i = 0; i < ARRAY_SIZE(apds9306_gts_mul); i++)
> > +		if (part_id == apds9306_gts_mul[i].part_id)
> > +			break;
> > +
> > +	if (i == ARRAY_SIZE(apds9306_gts_mul))
> > +		return -ENXIO;  
> 
> Strange choice of the error code, why not (one of) -ENOENT / -ENODATA ?
> 
> > +	return devm_iio_init_iio_gts(data->dev,
> > +				     apds9306_gts_mul[i].max_scale_int,
> > +				     apds9306_gts_mul[i].max_scale_nano,
> > +				     apds9306_gains, ARRAY_SIZE(apds9306_gains),
> > +				     apds9306_itimes, ARRAY_SIZE(apds9306_itimes),
> > +				     &data->gts);  
> 
> > +
> > +	return -ENXIO;  
> 
> Dead code.
> 
> > +}  
> 
> ...
> 
> Jonathan, are you going to apply this and addressing comments at the same time?
> Or should it be another version?
> 

The multibit field pretending to be a boolean was too complex for to want to modify
whilst applying. So yes, v8 with that tidied up and your comments sorted out

Jonathan