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

[Jonathan Cameron <Jonathan.Cameron@huawei.com>]

On Mon, 31 Mar 2025 23:36:17 +0900
gyeyoung <gye976@gmail.com> wrote:

> Hello Jonathan, thank you for the review.
> Sorry for the late response.
> 
> > > +
> > > +             /* at least 1000ppm */
> > > +             if (ppm < 1000 || ppm > 5000) {
> > > +                     dev_dbg(&indio_dev->dev, "span point ppm should be 1000~5000");
> > > +                     return -EINVAL;
> > > +             }
> > > +
> > > +             cmd_buf[3] = ppm / 256;
> > > +             cmd_buf[4] = ppm % 256;  
> >
> > That's an elaborate way of doing
> >                 unaligned_put_be16()
> > so use that instead as it's also clearly documenting what is going on.  
> 
> Since I couldn't find a function like 'unaligned_put_be16',
> but I found a function like 'be16_to_cpu', so I will use that.

You can't. An array of u8 is not guaranteed to be aligned so in
some cases be16_to_cpu() will fail.

I was half asleep :(
put_unaligned_be16() is what you are looking for.
https://elixir.bootlin.com/linux/v6.13.7/source/include/linux/unaligned.h#L61


> > > +static void mhz19b_write_wakeup(struct serdev_device *serdev)
> > > +{
> > > +     struct iio_dev *indio_dev;
> > > +
> > > +     indio_dev = dev_get_drvdata(&serdev->dev);
> > > +
> > > +     dev_dbg(&indio_dev->dev, "mhz19b_write_wakeup");  
> >
> > This doesn't do anything which makes me suspicious. Would
> > using serdev_device_write_wakeup() as the callback make
> > sense?  I'm not that familiar with serial drivers but I can
> > see that a number of other drivers do that.
> >  
> 
> 'serdev_device_write_wakeup' member function is mandatory.
> If this function is not set and remains NULL, the
> 'serdev_device_write' function will just return -EINVAL.
> 
> The following is a part of serdev_device_write().
> ------------
> ssize_t serdev_device_write(struct serdev_device *serdev, const u8 *buf,
>    size_t count, long timeout)
> {
> struct serdev_controller *ctrl = serdev->ctrl;
> size_t written = 0;
> ssize_t ret;
> 
> if (!ctrl || !ctrl->ops->write_buf || !serdev->ops->write_wakeup)
> return -EINVAL;
> .
> .
> .
> ------------

Ok. So why not serdev_device_write_wakeup()?

> 
> > > +}
> > > +
> > > +static const struct serdev_device_ops mhz19b_ops = {
> > > +     .receive_buf = mhz19b_receive_buf,
> > > +     .write_wakeup = mhz19b_write_wakeup,
> > > +};  
>
> > > +static int mhz19b_probe(struct serdev_device *serdev)
> > > +{
> > > +     int ret;
> > > +
> > > +     struct device *dev;
> > > +
> > > +     dev = &serdev->dev;
> > > +     serdev_device_set_client_ops(serdev, &mhz19b_ops);
> > > +
> > > +     ret = devm_serdev_device_open(dev, serdev);
> > > +     if (ret)
> > > +             return ret;
> > > +
> > > +     serdev_device_set_baudrate(serdev, 9600);
> > > +     serdev_device_set_flow_control(serdev, false);
> > > +     ret = serdev_device_set_parity(serdev, SERDEV_PARITY_NONE);
> > > +     if (ret < 0)
> > > +             return ret;  
> >
> > Why check return value from this call but not the previous two?
> > I'm not immediately able to see a reason this is more likely to fail.  
> 
> 'serdev_device_set_flow_control' is a void function.
> and as far as I know, 'serdev_device_set_baudrate' does not return an error.
> but I'll check again.

Ah I missed that. No problem not checking it.

Jonathan

> 
> I'll revise it considering your overall coding style guide.
> 
> Thanks,
> Gyeyoung Baek
> 
>