| Message ID | 20250409024311.19466-5-gye976@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | add support for winsen MHZ19B CO2 sensor | expand |
On Wed, 9 Apr 2025 11:43:10 +0900 Gyeyoung Baek <gye976@gmail.com> wrote: > Add support for winsen MHZ19B CO2 sensor. > > Datasheet: > https://www.winsen-sensor.com/d/files/infrared-gas-sensor/mh-z19b-co2-ver1_0.pdf > > Signed-off-by: Gyeyoung Baek <gye976@gmail.com> Hi Gyeyoung, A few additional comments inline. Jonathan > diff --git a/drivers/iio/chemical/mhz19b.c b/drivers/iio/chemical/mhz19b.c > new file mode 100644 > index 000000000000..d9a16e022b36 > --- /dev/null > +++ b/drivers/iio/chemical/mhz19b.c > @@ -0,0 +1,347 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * mh-z19b co2 sensor driver > + * > + * Copyright (c) 2025 Gyeyoung Baek <gye976@gmail.com> > + * > + * Datasheet: > + * https://www.winsen-sensor.com/d/files/infrared-gas-sensor/mh-z19b-co2-ver1_0.pdf > + */ > + > +#include <linux/cleanup.h> Do you use this? > +#include <linux/completion.h> > +#include <linux/device.h> > +#include <linux/iio/iio.h> > +#include <linux/iio/sysfs.h> > +#include <linux/init.h> > +#include <linux/kernel.h> > +#include <linux/mod_devicetable.h> > +#include <linux/module.h> > +#include <linux/regulator/consumer.h> > +#include <linux/serdev.h> > +#include <linux/unaligned.h> > + > +struct mhz19b_state { > + struct serdev_device *serdev; > + struct regulator *vin; As inline, you shouldn't need to keep this around if you use the devm based management for the regulator. > + > + /* > + * serdev receive buffer. > + * When data is received from the MH-Z19B, > + * the 'mhz19b_receive_buf' callback function is called and fills this buffer. > + */ > + char buf[9]; Use the CMD_SIZE define below for this 9 (having moved it up). > + int buf_idx; > + > + /* must wait the 'buf' is filled with 9 bytes.*/ > + struct completion buf_ready; > +}; > + > +/* > + * commands have following format: > + * > + * +------+------+-----+------+------+------+------+------+-------+ > + * | 0xFF | 0x01 | cmd | arg0 | arg1 | 0x00 | 0x00 | 0x00 | cksum | > + * +------+------+-----+------+------+------+------+------+-------+ > + */ > +#define MHZ19B_CMD_SIZE 9 > + > +#define MHZ19B_ABC_LOGIC_CMD 0x79 > +#define MHZ19B_READ_CO2_CMD 0x86 > +#define MHZ19B_SPAN_POINT_CMD 0x88 > +#define MHZ19B_ZERO_POINT_CMD 0x87 > + > +#define MHZ19B_ABC_LOGIC_OFF_CKSUM 0x86 > +#define MHZ19B_ABC_LOGIC_ON_CKSUM 0xE6 > +#define MHZ19B_READ_CO2_CKSUM 0x79 > +#define MHZ19B_ZERO_POINT_CKSUM 0x78 Can we not just calculate these from the buffer contents? > + > +/* ABC logic in MHZ19B means auto calibration. */ > + > +#define MHZ19B_SERDEV_TIMEOUT msecs_to_jiffies(100) > + > +static uint8_t mhz19b_get_checksum(uint8_t *packet) > +{ > + uint8_t i, checksum = 0; > + > + for (i = 1; i < 8; i++) > + checksum += packet[i]; > + > + checksum = 0xff - checksum; > + checksum += 1; > + > + return checksum; > +} > + > +static int mhz19b_serdev_cmd(struct iio_dev *indio_dev, > + int cmd, void *arg) > +{ > + struct mhz19b_state *st = iio_priv(indio_dev); > + struct serdev_device *serdev = st->serdev; > + struct device *dev = &indio_dev->dev; > + int ret; > + > + /* > + * cmd_buf[3,4] : arg0,1 > + * cmd_buf[8] : checksum > + */ > + uint8_t cmd_buf[MHZ19B_CMD_SIZE] = { > + 0xFF, 0x01, cmd, > + }; > + > + switch (cmd) { > + case MHZ19B_ABC_LOGIC_CMD: { > + bool enable = *((bool *)arg); Given you could just pass and u16 for the argument and use 0 /1 for the bool. The u16 works directly for the ppm value where needed for span point > + > + if (enable) { > + cmd_buf[3] = 0xA0; > + cmd_buf[8] = MHZ19B_ABC_LOGIC_ON_CKSUM; All these checksums should be easy enough to calculate which would be simpler to follow than writing constants like this. You are already doing so for the span point one so do that for them all. > + } else { > + cmd_buf[3] = 0; > + cmd_buf[8] = MHZ19B_ABC_LOGIC_OFF_CKSUM; > + } > + break; > + } case MHZ19B_READ_CO2_CMD: { > + cmd_buf[8] = MHZ19B_READ_CO2_CKSUM; > + break; > + } case MHZ19B_SPAN_POINT_CMD: { > + uint16_t ppm = *((uint16_t *)arg); For kernel internal code use the older types u16 etc > + > + put_unaligned_be16(ppm, &cmd_buf[3]); > + cmd_buf[MHZ19B_CMD_SIZE - 1] = mhz19b_get_checksum(cmd_buf); Using index value of 8 for all these for consistency. > + break; > + } case MHZ19B_ZERO_POINT_CMD: { > + cmd_buf[8] = MHZ19B_ZERO_POINT_CKSUM; > + break; > + } default: > + break; > + } > + > + /* write buf to uart ctrl syncronously */ > + ret = serdev_device_write(serdev, cmd_buf, MHZ19B_CMD_SIZE, 0); > + if (ret != MHZ19B_CMD_SIZE) { > + dev_err(dev, "write err, %d bytes written", ret); > + return -EINVAL; > + } > + > + switch (cmd) { > + case MHZ19B_READ_CO2_CMD: > + ret = wait_for_completion_interruptible_timeout(&st->buf_ready, > + MHZ19B_SERDEV_TIMEOUT); > + if (ret < 0) > + return ret; > + if (!ret) > + return -ETIMEDOUT; > + > + ret = mhz19b_get_checksum(st->buf); > + if (st->buf[MHZ19B_CMD_SIZE - 1] != mhz19b_get_checksum(st->buf)) { > + dev_err(dev, "checksum err"); > + return -EINVAL; > + } > + > + ret = get_unaligned_be16(&st->buf[2]); return get_unaligned_be16() > + return ret; > + default: > + /* no response commands. */ > + return 0; > + } > +} > + > +static int mhz19b_read_raw(struct iio_dev *indio_dev, > + struct iio_chan_spec const *chan, > + int *val, int *val2, long mask) Generally, unless we have very long lines align the following sets of parameters with just after the ( static int mhz19b_read_raw(struct iio_dev *indio_dev, struct iio_chan_spec const *chan, int *val, int *val2, long mask) Same applies in various other places in the code. > +{ > + int ret = mhz19b_serdev_cmd(indio_dev, MHZ19B_READ_CO2_CMD, NULL); > + > + if (ret < 0) > + return ret; > + > + *val = ret; > + return IIO_VAL_INT; > +} > + > +static const struct serdev_device_ops mhz19b_ops = { > + .receive_buf = mhz19b_receive_buf, > + .write_wakeup = serdev_device_write_wakeup, > +}; > + > +static int mhz19b_probe(struct serdev_device *serdev) > +{ > + int ret; > + struct device *dev = &serdev->dev; > + struct iio_dev *indio_dev; > + struct mhz19b_state *st; > + > + serdev_device_set_client_ops(serdev, &mhz19b_ops); > + > + ret = devm_serdev_device_open(dev, serdev); > + if (ret) > + return ret; > + > + ret = serdev_device_set_baudrate(serdev, 9600); > + if (ret < 0) > + return ret; > + > + serdev_device_set_flow_control(serdev, false); > + > + ret = serdev_device_set_parity(serdev, SERDEV_PARITY_NONE); > + if (ret < 0) > + return ret; > + > + indio_dev = devm_iio_device_alloc(dev, sizeof(struct mhz19b_state)); sizeof(*st) is both more compact and obviously correct with out a reader having to go check if the st in.. > + if (!indio_dev) > + return ret; > + dev_set_drvdata(dev, indio_dev); > + > + st = iio_priv(indio_dev); this line is the same size as we passed to devm_iio_device_alloc() > + st->serdev = serdev; > + > + init_completion(&st->buf_ready); > + > + st->vin = devm_regulator_get(dev, "vin"); > + if (IS_ERR(st->vin)) > + return PTR_ERR(st->vin); > + > + ret = regulator_enable(st->vin); Don't mix devm and non devm calls in probe. In this case you introduced a race where the power is turned off before we remove the userspace interfaces which is unlikely to go well... ret = devm_regulator_get_enabled(dev, "vin); if (ret) return ret; and no need for a remove. > + if (ret) > + return ret; > + > + indio_dev->name = "mh-z19b"; > + indio_dev->channels = mhz19b_channels; > + indio_dev->num_channels = ARRAY_SIZE(mhz19b_channels); > + indio_dev->info = &mhz19b_info; > + > + return devm_iio_device_register(dev, indio_dev); > +} > + > +static void mhz19b_remove(struct serdev_device *serdev) > +{ > + struct iio_dev *indio_dev = dev_get_drvdata(&serdev->dev); > + struct mhz19b_state *st = iio_priv(indio_dev); > + > + regulator_disable(st->vin); As above - remove will be unnecessary once the regulator is also device managed (devm_...) J
On Wed, Apr 9, 2025 at 5:45 AM Gyeyoung Baek <gye976@gmail.com> wrote: > > Add support for winsen MHZ19B CO2 sensor. > > Datasheet: > https://www.winsen-sensor.com/d/files/infrared-gas-sensor/mh-z19b-co2-ver1_0.pdf Make the above one line... > ...and remove this blank line, hence this becomes a tag in the commit message. > Signed-off-by: Gyeyoung Baek <gye976@gmail.com> ... > +#include <linux/kernel.h> No usual driver has a business to include the kernel.h. Just follow the IWYU principle and make sure what you include is what you use here. ... > +struct mhz19b_state { > + struct serdev_device *serdev; > + struct regulator *vin; > + > + /* > + * serdev receive buffer. > + * When data is received from the MH-Z19B, > + * the 'mhz19b_receive_buf' callback function is called and fills this buffer. > + */ > + char buf[9]; Should it be DMA-safe? > + int buf_idx; > + > + /* must wait the 'buf' is filled with 9 bytes.*/ ...wait until the... > + struct completion buf_ready; > +}; Have you run `pahole` to check if this is the best layout? ... > +/* > + * commands have following format: Commands > + * > + * +------+------+-----+------+------+------+------+------+-------+ > + * | 0xFF | 0x01 | cmd | arg0 | arg1 | 0x00 | 0x00 | 0x00 | cksum | > + * +------+------+-----+------+------+------+------+------+-------+ > + */ ... > +static uint8_t mhz19b_get_checksum(uint8_t *packet) What's wrong with u8 here and everywhere else? > +{ > + uint8_t i, checksum = 0; > + > + for (i = 1; i < 8; i++) Shouldn't you use sizeof here? It seems related to that 9 above. > + checksum += packet[i]; > + checksum = 0xff - checksum; > + checksum += 1; > + > + return checksum; Looks to me like these lines can be simplified to return -checksum; > +} ... > + case MHZ19B_ABC_LOGIC_CMD: { > + bool enable = *((bool *)arg); Oh, no. The boolean type is a tricky one and here you probably break all the possible implementation defined behaviours esp. on bigendian systems. > + if (enable) { > + cmd_buf[3] = 0xA0; > + cmd_buf[8] = MHZ19B_ABC_LOGIC_ON_CKSUM; > + } else { > + cmd_buf[3] = 0; > + cmd_buf[8] = MHZ19B_ABC_LOGIC_OFF_CKSUM; > + } > + break; > + } case MHZ19B_SPAN_POINT_CMD: { Please, split the leading } from the case (everywhere). This is an unusual style. > + uint16_t ppm = *((uint16_t *)arg); Do you guarantee the alignment? > + put_unaligned_be16(ppm, &cmd_buf[3]); > + cmd_buf[MHZ19B_CMD_SIZE - 1] = mhz19b_get_checksum(cmd_buf); > + break; > + } case MHZ19B_ZERO_POINT_CMD: { > + cmd_buf[8] = MHZ19B_ZERO_POINT_CKSUM; > + break; > + } default: > + break; > + } ... > + /* write buf to uart ctrl syncronously */ UART synchronously Perhaps a spellchecker to run? > + ret = serdev_device_write(serdev, cmd_buf, MHZ19B_CMD_SIZE, 0); > + if (ret != MHZ19B_CMD_SIZE) { > + dev_err(dev, "write err, %d bytes written", ret); > + return -EINVAL; Can ret be negative? In such a case, please propagate the actual error code. > + } ... > + ret = mhz19b_get_checksum(st->buf); How ret is being used? And if it may hold an error code, it should be return to the upper layers, otherwise the ret variable is a bad choice to keep this locally. > + if (st->buf[MHZ19B_CMD_SIZE - 1] != mhz19b_get_checksum(st->buf)) { > + dev_err(dev, "checksum err"); > + return -EINVAL; > + } > + > + ret = get_unaligned_be16(&st->buf[2]); > + return ret; ... > +static int mhz19b_read_raw(struct iio_dev *indio_dev, > + struct iio_chan_spec const *chan, > + int *val, int *val2, long mask) > +{ > + int ret = mhz19b_serdev_cmd(indio_dev, MHZ19B_READ_CO2_CMD, NULL); > + > + if (ret < 0) > + return ret; It's better to use the form of int ret; ret = ... if (ret) ... Also why do you have ' < 0' parts? Please, double check that you use this form of the errorcheck if and only if the callee may return a positive value. > + *val = ret; > + return IIO_VAL_INT; > +} ... > +/* > + * MHZ19B only supports writing configuration values. > + * > + * echo 0 > calibration_auto_enable : ABC logic off > + * echo 1 > calibration_auto_enable : ABC logic on > + * > + * echo 0 > calibration_forced_value : zero point calibration > + * (make sure the sensor had been worked under 400ppm for over 20 minutes.) has been working / had been working ? > + * echo [1000 1 5000] > calibration_forced_value : span point calibration > + * (make sure the sensor had been worked under a certain level co2 for over 20 minutes.) > + */ ... > +{ > + struct iio_dev *indio_dev = dev_to_iio_dev(dev); > + bool enable; > + int ret = kstrtobool(buf, &enable); > + > + if (ret) > + return ret; Same comment to everywhere where you assign in the definition block and check that in the code later on, this form is harder to maintain. > + > + ret = mhz19b_serdev_cmd(indio_dev, MHZ19B_ABC_LOGIC_CMD, &enable); > + if (ret < 0) > + return ret; > + > + return len; > +} ... > + uint16_t ppm; u16. Please, use kernel types everywhere. ... > + ret = kstrtou16(buf, 10, &ppm); No hex? > + if (ret) > + return ret; ... > + if (ppm < 1000 || ppm > 5000) { in_range() > + dev_dbg(&indio_dev->dev, > + "span point ppm should be 1000~5000"); > + return -EINVAL; > + } ... > +static const struct attribute_group mhz19b_attr_group = { > + .attrs = mhz19b_attrs, > +}; ATTRIBUTE_GROUP() ? ... > +static size_t mhz19b_receive_buf(struct serdev_device *serdev, > + const u8 *data, size_t len) > +{ > + struct iio_dev *indio_dev = dev_get_drvdata(&serdev->dev); > + struct mhz19b_state *st = iio_priv(indio_dev); > + > + for (int i = 0; i < len; i++) Why signed? > + st->buf[st->buf_idx++] = data[i]; > + > + if (st->buf_idx == MHZ19B_CMD_SIZE) { > + st->buf_idx = 0; > + complete(&st->buf_ready); > + } > + > + return len; > +} ... > + ret = serdev_device_set_baudrate(serdev, 9600); > + if (ret < 0) Why < 0? > + return ret; > + > + serdev_device_set_flow_control(serdev, false); > + > + ret = serdev_device_set_parity(serdev, SERDEV_PARITY_NONE); > + if (ret < 0) Why < 0? > + return ret;
Hello Jonathan, thank you for taking the time to review my patch. > > +/* > > + * mh-z19b co2 sensor driver > > + * > > + * Copyright (c) 2025 Gyeyoung Baek <gye976@gmail.com> > > + * > > + * Datasheet: > > + * https://www.winsen-sensor.com/d/files/infrared-gas-sensor/mh-z19b-co2-ver1_0.pdf > > + */ > > + > > +#include <linux/cleanup.h> > > Do you use this? Sorry, It's not used, I missed that. --- > > +#define MHZ19B_CMD_SIZE 9 > > + > > +#define MHZ19B_ABC_LOGIC_CMD 0x79 > > +#define MHZ19B_READ_CO2_CMD 0x86 > > +#define MHZ19B_SPAN_POINT_CMD 0x88 > > +#define MHZ19B_ZERO_POINT_CMD 0x87 > > + > > +#define MHZ19B_ABC_LOGIC_OFF_CKSUM 0x86 > > +#define MHZ19B_ABC_LOGIC_ON_CKSUM 0xE6 > > +#define MHZ19B_READ_CO2_CKSUM 0x79 > > +#define MHZ19B_ZERO_POINT_CKSUM 0x78 > Can we not just calculate these from the buffer contents? Yes, we can calculate it simply, (actually previous patches did it in that way.) I'll revert to that way for readability. --- > > + switch (cmd) { > > + case MHZ19B_ABC_LOGIC_CMD: { > > + bool enable = *((bool *)arg); > Given you could just pass and u16 for the argument and use 0 /1 for > the bool. The u16 works directly for the ppm value where needed for span > point I used bool type for readability, but on second thought The consistency of using u16 type seems more important than readability. I'll edit this. --- > > + st->vin = devm_regulator_get(dev, "vin"); > > + if (IS_ERR(st->vin)) > > + return PTR_ERR(st->vin); > > + > > + ret = regulator_enable(st->vin); > > Don't mix devm and non devm calls in probe. In this case you introduced > a race where the power is turned off before we remove the userspace > interfaces which is unlikely to go well... Sorry, I missed the existence of the devm_regulator_get_enable(). I'll edit this. Thanks, Gyeyoung
Hello Andy, thank you for taking the time to review my patch. > > +#include <linux/kernel.h> > > No usual driver has a business to include the kernel.h. Just follow > the IWYU principle and make sure what you include is what you use > here. I thought "linux/kernel.h" was a globally essential header. Thank you for the information. --- > > +struct mhz19b_state { > > + struct serdev_device *serdev; > > + struct regulator *vin; > > + > > + /* > > + * serdev receive buffer. > > + * When data is received from the MH-Z19B, > > + * the 'mhz19b_receive_buf' callback function is called and fills this buffer. > > + */ > > + char buf[9]; > > Should it be DMA-safe? > > > + int buf_idx; I'm not sure if I understood your point correctly, This code isn't DMA-safe. I'm currently understanding why DMA-safe is necessary. (but actually other drivers implementing 'serdev ops' use non-DMA-safe buffers.) I will verify this part and then send the next patch. --- > > + struct completion buf_ready; > > +}; > > Have you run `pahole` to check if this is the best layout? No, I haven't run 'pahole' yet, but I'll verify the structure layout using the tool from now on. Thank you for the information. --- > > + case MHZ19B_ABC_LOGIC_CMD: { > > + bool enable = *((bool *)arg); > > Oh, no. The boolean type is a tricky one and here you probably break > all the possible implementation defined behaviours esp. on bigendian > systems. > > + uint16_t ppm = *((uint16_t *)arg); > > Do you guarantee the alignment? So far, the arg has been the address of a u16 type stack variable, so there was no error. But I'll edit this by referring to the alignment documentation. --- > > + ret = serdev_device_write(serdev, cmd_buf, MHZ19B_CMD_SIZE, 0); > > + if (ret != MHZ19B_CMD_SIZE) { > > + dev_err(dev, "write err, %d bytes written", ret); > > > + return -EINVAL; > > Can ret be negative? In such a case, please propagate the actual error code. Yes, ret can be negative. and I'll propagate the actual error code from now on. > > + } > > ... > > > + ret = mhz19b_get_checksum(st->buf); > > How ret is being used? And if it may hold an error code, it should be > return to the upper layers, otherwise the ret variable is a bad choice > to keep this locally. Sorry, this "ret = mhz19b_get_checksum(st->buf);" line should be dropped, I missed that. --- > > +static int mhz19b_read_raw(struct iio_dev *indio_dev, > > + struct iio_chan_spec const *chan, > > + int *val, int *val2, long mask) > > +{ > > + int ret = mhz19b_serdev_cmd(indio_dev, MHZ19B_READ_CO2_CMD, NULL); > > + > > + if (ret < 0) > > + return ret; > > It's better to use the form of > > int ret; > > ret = ... > if (ret) > ... > > Also why do you have ' < 0' parts? Please, double check that you use > this form of the errorcheck if and only if the callee may return a > positive value. Yes, this function returns either a positive value(0 ~ 2000), depending on the cmd argument. So 'if (ret < 0)' is more appropriate than 'if (ret)' in this case. --- > > > +static const struct attribute_group mhz19b_attr_group = { > > + .attrs = mhz19b_attrs, > > +}; > > ATTRIBUTE_GROUP() ? I looked into the API and found only ATTRIBUTE_GROUPS(), But using ATTRIBUTE_GROUPS() requires the attribute_group value to be declared as an array ending with a NULL entry. Would this be OK? then i'll use ATTRIBUTE_GROUPS(). --- > > +static size_t mhz19b_receive_buf(struct serdev_device *serdev, > > + const u8 *data, size_t len) > > +{ > > + struct iio_dev *indio_dev = dev_get_drvdata(&serdev->dev); > > + struct mhz19b_state *st = iio_priv(indio_dev); > > + > > + for (int i = 0; i < len; i++) > > Why signed? Would it be better to declare 'i' as 'size_t' to match the type of 'len'? then I'll change 'int i' to 'size_t i'. --- > > + ret = serdev_device_set_baudrate(serdev, 9600); > > + if (ret < 0) > > Why < 0? First I assumed there was an error, but after checking the serdev_device_set_baudrate() function, I found that it doesn't return any error. so I'll remove the error check. --- > > + return ret; > > + > > + serdev_device_set_flow_control(serdev, false); > > + > > + ret = serdev_device_set_parity(serdev, SERDEV_PARITY_NONE); > > + if (ret < 0) > > Why < 0? I'll simply change it to 'if (ret)'. --- I'll revise the overall coding style by referring to other reviews. Thanks, Gyeyoung
On Mon, Apr 14, 2025 at 6:49 PM gyeyoung <gye976@gmail.com> wrote: ... > > > +#include <linux/kernel.h> > > > > No usual driver has a business to include the kernel.h. Just follow > > the IWYU principle and make sure what you include is what you use > > here. > > I thought "linux/kernel.h" was a globally essential header. Not at all, it's a big mess which no driver should use. And on top of that (to any header) you should not use 'proxy' headers. It's a bad style and practice with the real consequences as build time and headache for the others who want to clean up header dependencies in the future. ... > > > + /* > > > + * serdev receive buffer. > > > + * When data is received from the MH-Z19B, > > > + * the 'mhz19b_receive_buf' callback function is called and fills this buffer. > > > + */ > > > + char buf[9]; > > > > Should it be DMA-safe? > > I'm not sure if I understood your point correctly, > This code isn't DMA-safe. I'm currently understanding why DMA-safe is necessary. > (but actually other drivers implementing 'serdev ops' use non-DMA-safe buffers.) > I will verify this part and then send the next patch. Because some of the UART drivers may enable DMA by default if it's available and your code won't work on them, right? But double check if serdev makes it DMA-safe before use. ... > > > + case MHZ19B_ABC_LOGIC_CMD: { > > > + bool enable = *((bool *)arg); > > > > Oh, no. The boolean type is a tricky one and here you probably break > > all the possible implementation defined behaviours esp. on bigendian > > systems. > > > > + uint16_t ppm = *((uint16_t *)arg); > > > > Do you guarantee the alignment? > > So far, the arg has been the address of a u16 type stack variable, so > there was no error. > But I'll edit this by referring to the alignment documentation. Easier to use the proper accessor, i.e. one of get_unaligned*() calls here. ... > > Also why do you have ' < 0' parts? Please, double check that you use > > this form of the errorcheck if and only if the callee may return a > > positive value. > > Yes, this function returns either a positive value(0 ~ 2000), > depending on the cmd argument. > So 'if (ret < 0)' is more appropriate than 'if (ret)' in this case. I see, so make sure that only those that may return positive values will have the < 0 filter, and others (that return 0 or negative error code) use the regular form. ... > > > +static const struct attribute_group mhz19b_attr_group = { > > > + .attrs = mhz19b_attrs, > > > +}; > > > > ATTRIBUTE_GROUP() ? > > I looked into the API and found only ATTRIBUTE_GROUPS(), > But using ATTRIBUTE_GROUPS() requires the attribute_group value to be > declared as an array ending with a NULL entry. Would this be OK? > then i'll use ATTRIBUTE_GROUPS(). Indeed, only if you use that array, otherwise the original code is OK. ... > > > + for (int i = 0; i < len; i++) > > > > Why signed? > > Would it be better to declare 'i' as 'size_t' to match the type of 'len'? > then I'll change 'int i' to 'size_t i'. It will be consistent.
On Tue, Apr 15, 2025 at 2:21 AM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Mon, Apr 14, 2025 at 6:49 PM gyeyoung <gye976@gmail.com> wrote: > > ... > > > > > +#include <linux/kernel.h> > > > > > > No usual driver has a business to include the kernel.h. Just follow > > > the IWYU principle and make sure what you include is what you use > > > here. > > > > I thought "linux/kernel.h" was a globally essential header. > > Not at all, it's a big mess which no driver should use. > And on top of that (to any header) you should not use 'proxy' headers. > It's a bad style and practice with the real consequences as build time > and headache for the others who want to clean up header dependencies > in the future. Now I see. I had been referring to some legacy examples, but looking at it again, It makes more sense to explicitly include only what's needed. Thank you for pointing out the good practice. Thanks, Gyeyoung
Hello Andy, On Tue, Apr 15, 2025 at 2:21 AM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > > > > + /* > > > > + * serdev receive buffer. > > > > + * When data is received from the MH-Z19B, > > > > + * the 'mhz19b_receive_buf' callback function is called and fills this buffer. > > > > + */ > > > > + char buf[9]; > > > > > > Should it be DMA-safe? > > > > I'm not sure if I understood your point correctly, > > This code isn't DMA-safe. I'm currently understanding why DMA-safe is necessary. > > (but actually other drivers implementing 'serdev ops' use non-DMA-safe buffers.) > > I will verify this part and then send the next patch. > > Because some of the UART drivers may enable DMA by default if it's > available and your code won't work on them, right? But double check if > serdev makes it DMA-safe before use. It seems that the serdev buf doesn't need to be DMA-safe. I looked into the PL011 driver as an example, which uses DMA, and found that the data received via DMA is firstly stored in the buffer within the 'uart_amba_port' structure, and then copied into the 'tty_bufhead' within the tty_port. Later, in serdev's receive_buf(), it simply copies from the tty_bufhead to into serdev buf. So I think there's no need to consider DMA-safe in the serdev buf itself. would this make sense? If so, I think there is no need to change the code related to the buffer. Regards, Gyeyoung
On Thu, Apr 17, 2025 at 11:03:12PM +0900, gyeyoung wrote: > On Tue, Apr 15, 2025 at 2:21 AM Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: ... > > > > > + /* > > > > > + * serdev receive buffer. > > > > > + * When data is received from the MH-Z19B, > > > > > + * the 'mhz19b_receive_buf' callback function is called and fills this buffer. > > > > > + */ > > > > > + char buf[9]; > > > > > > > > Should it be DMA-safe? > > > > > > I'm not sure if I understood your point correctly, > > > This code isn't DMA-safe. I'm currently understanding why DMA-safe is necessary. > > > (but actually other drivers implementing 'serdev ops' use non-DMA-safe buffers.) > > > I will verify this part and then send the next patch. > > > > Because some of the UART drivers may enable DMA by default if it's > > available and your code won't work on them, right? But double check if > > serdev makes it DMA-safe before use. > > It seems that the serdev buf doesn't need to be DMA-safe. I looked > into the PL011 driver as an example, > which uses DMA, and found that the data received via DMA is firstly > stored in the buffer within the 'uart_amba_port' structure, and then > copied into the 'tty_bufhead' within the tty_port. Later, in serdev's > receive_buf(), it simply copies from the tty_bufhead to into serdev > buf. So I think there's no need to consider DMA-safe in the serdev buf > itself. But who will give those guarantees (note, the code is most likely may be run on different UART controllers (PL011 is just one of many), have you checked all supported drivers for DMA? > would this make sense? If so, I think there is no need to change the > code related to the buffer. The bare minimum is to make sure this buffer occupies the cacheline. Read about DMA safety for the cache coherency.
diff --git a/drivers/iio/chemical/Kconfig b/drivers/iio/chemical/Kconfig index 330fe0af946f..641bf9b35915 100644 --- a/drivers/iio/chemical/Kconfig +++ b/drivers/iio/chemical/Kconfig @@ -237,4 +237,14 @@ config VZ89X Sensortech MiCS VZ89X VOC (Volatile Organic Compounds) sensors +config WINSEN_MHZ19B + tristate "Winsen MHZ19B CO2 sensor" + depends on SERIAL_DEV_BUS + help + Say Y here to build Serdev interface support for the Winsen + MHZ19B CO2 sensor. + + To compile this driver as a module, choose M here: the module will + be called mhz19b. + endmenu diff --git a/drivers/iio/chemical/Makefile b/drivers/iio/chemical/Makefile index 4866db06bdc9..deed437dd396 100644 --- a/drivers/iio/chemical/Makefile +++ b/drivers/iio/chemical/Makefile @@ -27,3 +27,4 @@ obj-$(CONFIG_SPS30) += sps30.o obj-$(CONFIG_SPS30_I2C) += sps30_i2c.o obj-$(CONFIG_SPS30_SERIAL) += sps30_serial.o obj-$(CONFIG_VZ89X) += vz89x.o +obj-$(CONFIG_WINSEN_MHZ19B) += mhz19b.o diff --git a/drivers/iio/chemical/mhz19b.c b/drivers/iio/chemical/mhz19b.c new file mode 100644 index 000000000000..d9a16e022b36 --- /dev/null +++ b/drivers/iio/chemical/mhz19b.c @@ -0,0 +1,347 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * mh-z19b co2 sensor driver + * + * Copyright (c) 2025 Gyeyoung Baek <gye976@gmail.com> + * + * Datasheet: + * https://www.winsen-sensor.com/d/files/infrared-gas-sensor/mh-z19b-co2-ver1_0.pdf + */ + +#include <linux/cleanup.h> +#include <linux/completion.h> +#include <linux/device.h> +#include <linux/iio/iio.h> +#include <linux/iio/sysfs.h> +#include <linux/init.h> +#include <linux/kernel.h> +#include <linux/mod_devicetable.h> +#include <linux/module.h> +#include <linux/regulator/consumer.h> +#include <linux/serdev.h> +#include <linux/unaligned.h> + +struct mhz19b_state { + struct serdev_device *serdev; + struct regulator *vin; + + /* + * serdev receive buffer. + * When data is received from the MH-Z19B, + * the 'mhz19b_receive_buf' callback function is called and fills this buffer. + */ + char buf[9]; + int buf_idx; + + /* must wait the 'buf' is filled with 9 bytes.*/ + struct completion buf_ready; +}; + +/* + * commands have following format: + * + * +------+------+-----+------+------+------+------+------+-------+ + * | 0xFF | 0x01 | cmd | arg0 | arg1 | 0x00 | 0x00 | 0x00 | cksum | + * +------+------+-----+------+------+------+------+------+-------+ + */ +#define MHZ19B_CMD_SIZE 9 + +#define MHZ19B_ABC_LOGIC_CMD 0x79 +#define MHZ19B_READ_CO2_CMD 0x86 +#define MHZ19B_SPAN_POINT_CMD 0x88 +#define MHZ19B_ZERO_POINT_CMD 0x87 + +#define MHZ19B_ABC_LOGIC_OFF_CKSUM 0x86 +#define MHZ19B_ABC_LOGIC_ON_CKSUM 0xE6 +#define MHZ19B_READ_CO2_CKSUM 0x79 +#define MHZ19B_ZERO_POINT_CKSUM 0x78 + +/* ABC logic in MHZ19B means auto calibration. */ + +#define MHZ19B_SERDEV_TIMEOUT msecs_to_jiffies(100) + +static uint8_t mhz19b_get_checksum(uint8_t *packet) +{ + uint8_t i, checksum = 0; + + for (i = 1; i < 8; i++) + checksum += packet[i]; + + checksum = 0xff - checksum; + checksum += 1; + + return checksum; +} + +static int mhz19b_serdev_cmd(struct iio_dev *indio_dev, + int cmd, void *arg) +{ + struct mhz19b_state *st = iio_priv(indio_dev); + struct serdev_device *serdev = st->serdev; + struct device *dev = &indio_dev->dev; + int ret; + + /* + * cmd_buf[3,4] : arg0,1 + * cmd_buf[8] : checksum + */ + uint8_t cmd_buf[MHZ19B_CMD_SIZE] = { + 0xFF, 0x01, cmd, + }; + + switch (cmd) { + case MHZ19B_ABC_LOGIC_CMD: { + bool enable = *((bool *)arg); + + if (enable) { + cmd_buf[3] = 0xA0; + cmd_buf[8] = MHZ19B_ABC_LOGIC_ON_CKSUM; + } else { + cmd_buf[3] = 0; + cmd_buf[8] = MHZ19B_ABC_LOGIC_OFF_CKSUM; + } + break; + } case MHZ19B_READ_CO2_CMD: { + cmd_buf[8] = MHZ19B_READ_CO2_CKSUM; + break; + } case MHZ19B_SPAN_POINT_CMD: { + uint16_t ppm = *((uint16_t *)arg); + + put_unaligned_be16(ppm, &cmd_buf[3]); + cmd_buf[MHZ19B_CMD_SIZE - 1] = mhz19b_get_checksum(cmd_buf); + break; + } case MHZ19B_ZERO_POINT_CMD: { + cmd_buf[8] = MHZ19B_ZERO_POINT_CKSUM; + break; + } default: + break; + } + + /* write buf to uart ctrl syncronously */ + ret = serdev_device_write(serdev, cmd_buf, MHZ19B_CMD_SIZE, 0); + if (ret != MHZ19B_CMD_SIZE) { + dev_err(dev, "write err, %d bytes written", ret); + return -EINVAL; + } + + switch (cmd) { + case MHZ19B_READ_CO2_CMD: + ret = wait_for_completion_interruptible_timeout(&st->buf_ready, + MHZ19B_SERDEV_TIMEOUT); + if (ret < 0) + return ret; + if (!ret) + return -ETIMEDOUT; + + ret = mhz19b_get_checksum(st->buf); + if (st->buf[MHZ19B_CMD_SIZE - 1] != mhz19b_get_checksum(st->buf)) { + dev_err(dev, "checksum err"); + return -EINVAL; + } + + ret = get_unaligned_be16(&st->buf[2]); + return ret; + default: + /* no response commands. */ + return 0; + } +} + +static int mhz19b_read_raw(struct iio_dev *indio_dev, + struct iio_chan_spec const *chan, + int *val, int *val2, long mask) +{ + int ret = mhz19b_serdev_cmd(indio_dev, MHZ19B_READ_CO2_CMD, NULL); + + if (ret < 0) + return ret; + + *val = ret; + return IIO_VAL_INT; +} + +/* + * MHZ19B only supports writing configuration values. + * + * echo 0 > calibration_auto_enable : ABC logic off + * echo 1 > calibration_auto_enable : ABC logic on + * + * echo 0 > calibration_forced_value : zero point calibration + * (make sure the sensor had been worked under 400ppm for over 20 minutes.) + * echo [1000 1 5000] > calibration_forced_value : span point calibration + * (make sure the sensor had been worked under a certain level co2 for over 20 minutes.) + */ +static ssize_t calibration_auto_enable_store(struct device *dev, + struct device_attribute *attr, + const char *buf, size_t len) +{ + struct iio_dev *indio_dev = dev_to_iio_dev(dev); + bool enable; + int ret = kstrtobool(buf, &enable); + + if (ret) + return ret; + + ret = mhz19b_serdev_cmd(indio_dev, MHZ19B_ABC_LOGIC_CMD, &enable); + if (ret < 0) + return ret; + + return len; +} +static IIO_DEVICE_ATTR_WO(calibration_auto_enable, 0); + +static ssize_t calibration_forced_value_store(struct device *dev, + struct device_attribute *attr, + const char *buf, size_t len) +{ + struct iio_dev *indio_dev = dev_to_iio_dev(dev); + uint16_t ppm; + int cmd, ret; + + ret = kstrtou16(buf, 10, &ppm); + if (ret) + return ret; + + /* at least 1000ppm */ + if (ppm) { + if (ppm < 1000 || ppm > 5000) { + dev_dbg(&indio_dev->dev, + "span point ppm should be 1000~5000"); + return -EINVAL; + } + + cmd = MHZ19B_SPAN_POINT_CMD; + } else { + cmd = MHZ19B_ZERO_POINT_CMD; + } + + ret = mhz19b_serdev_cmd(indio_dev, cmd, &ppm); + if (ret < 0) + return ret; + + return len; +} +static IIO_DEVICE_ATTR_WO(calibration_forced_value, 0); + +static struct attribute *mhz19b_attrs[] = { + &iio_dev_attr_calibration_auto_enable.dev_attr.attr, + &iio_dev_attr_calibration_forced_value.dev_attr.attr, + NULL +}; + +static const struct attribute_group mhz19b_attr_group = { + .attrs = mhz19b_attrs, +}; + +static const struct iio_info mhz19b_info = { + .attrs = &mhz19b_attr_group, + .read_raw = mhz19b_read_raw, +}; + +static const struct iio_chan_spec mhz19b_channels[] = { + { + .type = IIO_CONCENTRATION, + .channel2 = IIO_MOD_CO2, + .modified = 1, + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), + }, +}; + +static size_t mhz19b_receive_buf(struct serdev_device *serdev, + const u8 *data, size_t len) +{ + struct iio_dev *indio_dev = dev_get_drvdata(&serdev->dev); + struct mhz19b_state *st = iio_priv(indio_dev); + + for (int i = 0; i < len; i++) + st->buf[st->buf_idx++] = data[i]; + + if (st->buf_idx == MHZ19B_CMD_SIZE) { + st->buf_idx = 0; + complete(&st->buf_ready); + } + + return len; +} + +static const struct serdev_device_ops mhz19b_ops = { + .receive_buf = mhz19b_receive_buf, + .write_wakeup = serdev_device_write_wakeup, +}; + +static int mhz19b_probe(struct serdev_device *serdev) +{ + int ret; + struct device *dev = &serdev->dev; + struct iio_dev *indio_dev; + struct mhz19b_state *st; + + serdev_device_set_client_ops(serdev, &mhz19b_ops); + + ret = devm_serdev_device_open(dev, serdev); + if (ret) + return ret; + + ret = serdev_device_set_baudrate(serdev, 9600); + if (ret < 0) + return ret; + + serdev_device_set_flow_control(serdev, false); + + ret = serdev_device_set_parity(serdev, SERDEV_PARITY_NONE); + if (ret < 0) + return ret; + + indio_dev = devm_iio_device_alloc(dev, sizeof(struct mhz19b_state)); + if (!indio_dev) + return ret; + dev_set_drvdata(dev, indio_dev); + + st = iio_priv(indio_dev); + st->serdev = serdev; + + init_completion(&st->buf_ready); + + st->vin = devm_regulator_get(dev, "vin"); + if (IS_ERR(st->vin)) + return PTR_ERR(st->vin); + + ret = regulator_enable(st->vin); + if (ret) + return ret; + + indio_dev->name = "mh-z19b"; + indio_dev->channels = mhz19b_channels; + indio_dev->num_channels = ARRAY_SIZE(mhz19b_channels); + indio_dev->info = &mhz19b_info; + + return devm_iio_device_register(dev, indio_dev); +} + +static void mhz19b_remove(struct serdev_device *serdev) +{ + struct iio_dev *indio_dev = dev_get_drvdata(&serdev->dev); + struct mhz19b_state *st = iio_priv(indio_dev); + + regulator_disable(st->vin); +} + +static const struct of_device_id mhz19b_of_match[] = { + { .compatible = "winsen,mhz19b", }, + { } +}; +MODULE_DEVICE_TABLE(of, mhz19b_of_match); + +static struct serdev_device_driver mhz19b_driver = { + .driver = { + .name = "mhz19b", + .of_match_table = mhz19b_of_match, + }, + .probe = mhz19b_probe, + .remove = mhz19b_remove, +}; +module_serdev_device_driver(mhz19b_driver); + +MODULE_AUTHOR("Gyeyoung Baek"); +MODULE_DESCRIPTION("MH-Z19B CO2 sensor driver using serdev interface"); +MODULE_LICENSE("GPL");
Add support for winsen MHZ19B CO2 sensor. Datasheet: https://www.winsen-sensor.com/d/files/infrared-gas-sensor/mh-z19b-co2-ver1_0.pdf Signed-off-by: Gyeyoung Baek <gye976@gmail.com> --- drivers/iio/chemical/Kconfig | 10 + drivers/iio/chemical/Makefile | 1 + drivers/iio/chemical/mhz19b.c | 347 ++++++++++++++++++++++++++++++++++ 3 files changed, 358 insertions(+) create mode 100644 drivers/iio/chemical/mhz19b.c -- 2.34.1