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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <20240225114013.7a5e11be@jic23-huawei>
Date: Sun, 25 Feb 2024 11:40:13 +0000
From: Jonathan Cameron <jic23@...nel.org>
To: Subhajit Ghosh <subhajit.ghosh@...aklogic.com>
Cc: 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>, Andy
 Shevchenko <andriy.shevchenko@...ux.intel.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


> >   
> >> +
> >> +	/*
> >> +	 * 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