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

[Jonathan Cameron <jic23@kernel.org>]

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