Re: [PATCH v4 3/4] iio: chemical: add support for winsen MHZ19B CO2 sensor

[Andy Shevchenko <andy.shevchenko@gmail.com>]

On Mon, Apr 21, 2025 at 03:41:58PM +0900, Gyeyoung Baek wrote:
> On Mon, Apr 21, 2025 at 4:22 AM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> > On Sun, Apr 20, 2025 at 9:10 PM Gyeyoung Baek <gye976@gmail.com> wrote:

...

> > > The datasheet is available at
> > > Link: https://www.winsen-sensor.com/d/files/infrared-gas-sensor/mh-z19b-co2-ver1_0.pdf
> >
> > Instead, just make it Datasheet: tag.

> > >

> > Should not be this blank line here.
> > > Signed-off-by: Gyeyoung Baek <gye976@gmail.com>
> 
> Sorry, I now understand what you mean.
> ('Datasheet:' is a tag, not an explanation. So there should be no
> space between the tags.)
> However, when I use the 'Datasheet:' tag, I get the following warning
> by checkpatch.pl.
> Would it be OK? then I'll use 'Datasheet:' tag.
> 
>     "WARNING: Unknown link reference 'Datasheet:', use 'Link:' or
> 'Closes:' instead
>      #8:
>      Datasheet:
> https://www.winsen-sensor.com/d/files/infrared-gas-sensor/mh-z19b-co2-ver1_0.pdf"

checkpatch is recommendation and sometimes has not fully suitable warnings. You
may ignore this one.

...

> > > +#include <linux/completion.h>
> > > +#include <linux/device.h>
> > > +#include <linux/iio/iio.h>
> > > +#include <linux/iio/sysfs.h>
> > > +#include <linux/mod_devicetable.h>
> > > +#include <linux/module.h>
> > > +#include <linux/regulator/consumer.h>
> > > +#include <linux/serdev.h>
> > > +#include <linux/unaligned.h>
> >
> > Semi-baked list, see below what's missing (actually a lot). I believe
> > I already pointed that out.
> 
> From the last review, I understood the two main points as:
> (1) Avoid using large headers and explicitly include only the necessary ones.
> (2) Reduce including headers as much as possible to improve build times.
> 
> However, I found that removing headers like 'types.h' didn't cause any
> build failure,

Because we have other headers that may use it, but you shouldn't do that.

> So I thought it would be better to remove headers that are unnecessary
> to improve build time.
> But in the next patch, I'll explicitly include the headers you've pointed out.

You should follow IWYU principle, i.e. Include What You Use, e.g., types.h.

...

> > > +       if (ppm) {
> > > +               if (!in_range(ppm, 1000, 4001)) {
> >
> > Missing minmax.h.
> >
> > The second parameter is length of the range.
> >
> > > +                       dev_dbg(&indio_dev->dev,
> > > +                               "span point ppm should be 1000~5000");
> >
> > The above range check doesn't agree with this message.
> 
> in_range() uses the range 'start <= val && val < (start + len)'.
> So, to allow 5000, the 'len' should be 4001, not 4000.
> I have tested this.
> But would it be correct that I understand your point?

I see now, I recommend then to define those two:

#define ..._PPM_MIN	1000
#define ..._PPM_MAX	5000

And use here as

               if (!in_range(ppm, ..._MIN, ..._MAX - ..._MIN + 1)) {
                       dev_dbg(&indio_dev->dev,
                               "span point PPM should be in a range [%d-%d]\n",
			       _MIN, _MAX);

Note, you missed \n as well.

> > > +                       return -EINVAL;
> > > +               }

-- 
With Best Regards,
Andy Shevchenko