linux-kernel - Re: [PATCH v7 5/5] iio: light: Add support for APDS9306 Light Sensor

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 for Android: free password hash cracker in your pocket

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

Message-ID: <ZdycR6nr3rtrnuth@smile.fi.intel.com>
Date: Mon, 26 Feb 2024 16:12:23 +0200
From: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To: Subhajit Ghosh <subhajit.ghosh@...aklogic.com>
Cc: Jonathan Cameron <jic23@...nel.org>,
	Lars-Peter Clausen <lars@...afoo.de>,
	Rob Herring <robh+dt@...nel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Matti Vaittinen <mazziesaccount@...il.com>,
	Marek Vasut <marex@...x.de>, Anshul Dalal <anshulusr@...il.com>,
	Javier Carrasco <javier.carrasco.cruz@...il.com>,
	Matt Ranostay <matt@...ostay.sg>,
	Stefan Windfeldt-Prytz <stefan.windfeldt-prytz@...s.com>,
	linux-iio@...r.kernel.org, devicetree@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v7 5/5] iio: light: Add support for APDS9306 Light Sensor

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?

-- 
With Best Regards,
Andy Shevchenko