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