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

From: Jonathan Cameron <jic23@kernel.org>
To: Subhajit Ghosh <subhajit.ghosh@tweaklogic.com>
Cc: 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>,
	Andy Shevchenko <andriy.shevchenko@linux.intel.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
Date: Sun, 25 Feb 2024 11:40:13 +0000	[thread overview]
Message-ID: <20240225114013.7a5e11be@jic23-huawei> (raw)
In-Reply-To: <15a46491-d126-4998-88f0-1720316f0a6c@tweaklogic.com>

> >   
> >> +
> >> +	/*
> >> +	 * If a one-shot read through sysfs is underway at the same time
> >> +	 * as this interrupt handler is executing and a read data available
> >> +	 * flag was set, this flag is set to inform read_poll_timeout()
> >> +	 * to exit.
> >> +	 */
> >> +	if ((status & APDS9306_ALS_DATA_STAT_MASK))
> >> +		data->read_data_available = 1;
> >> +
> >> +	return IRQ_HANDLED;
> >> +}  
> > 
> > ...
> >   
> >> +static int apds9306_read_event_config(struct iio_dev *indio_dev,
> >> +				      const struct iio_chan_spec *chan,
> >> +				      enum iio_event_type type,
> >> +				      enum iio_event_direction dir)
> >> +{
> >> +	struct apds9306_data *data = iio_priv(indio_dev);
> >> +	struct apds9306_regfields *rf = &data->rf;
> >> +	int int_en, event_ch_is_light, ret;
> >> +
> >> +	switch (type) {
> >> +	case IIO_EV_TYPE_THRESH:
> >> +		guard(mutex)(&data->mutex);
> >> +
> >> +		ret = regmap_field_read(rf->int_src, &event_ch_is_light);  
> > 
> > Call the local value int_src - it's not obvious to a reviewer what
> > relationship between that and int_src is. I had to go read the datasheet
> > to find out.  
> This unique name was suggested in a previous review:
> https://lore.kernel.org/all/20240121152332.6b15666a@jic23-huawei/
> I will change it next version. int_src is logical.

Ah, I failed to register that it was a multi bit field (which it isn't
really but we should track what the datasheet says).  If it had
been a single bit then it would have made sense to name it as a boolean
flag.  Not so when in theory it could take 4 values (even if only 2 are
defined).

> >   
> >> +		if (ret)
> >> +			return ret;
> >> +
> >> +		ret = regmap_field_read(rf->int_en, &int_en);
> >> +		if (ret)
> >> +			return ret;
> >> +
> >> +		if (chan->type == IIO_LIGHT)
> >> +			return int_en & event_ch_is_light;
> >> +
> >> +		if (chan->type == IIO_INTENSITY)
> >> +			return int_en & !event_ch_is_light;  
> > This is the specific line the compiler doesn't like
> > drivers/iio/light/apds9306.c:1036:39: warning: dubious: x & !y  
> I am using gcc 12.2.0 for cross compiling. I definitely do not want to send
> patches with warnings in them. Can you please let me know the gcc version
> or flags using which you got the above warning? Should I always use the
> latest released version?
Version shouldn't matter that much but this was x86 build with gcc 13.2.1
W=1 is maybe what is showing this up as it enables a bunch more warnings.

IIO is almost clean with W=1 though a few minor things have gotten in recently
that I need to tidy up.

> > 
> > I would match int_src against specific values rather than using tricks
> > based on what those values happen to be.
> > 
> > 			return int_en && (int_src == APDS9306_INT_SRC_CLEAR);  
> I will implement this.
> 
> 
> Thank you for taking time to review the code in detail and also appreciate
> your suggestions.
> 
> Regards,
> Subhajit Ghosh