Re: [PATCH v2 2/2] iio: chemical: Add driver for SEN0322

Tóth János
2025-05-06T07:52:09Z

Hi!

Thank you for the review!

> Checkpatch is lagging behind the times, but it is fine to use this
> as a formal tag in the tag block..
> >
> Datasheet: https://wiki.dfrobot.com/Gravity_I2C_Oxygen_Sensor_SKU_SEN0322
>
> > Signed-off-by: Tóth János <gomba007@gmail.com>

Sure.

> > + if (val) {
> > + *num = val;
> > + *den = 100000;
> > + } else {
> > + *num = 209;
> > + *den = 120000;
>
> This is odd enough, that perhaps we could add a comment on why, or at least
> a cross reference to where these numbers come from?
> What is the special meaning of 0?

Okay, I'll add some explanation.

> > + u8 data[4] = { 0 };
>
> If you are only read 3 bytes, why is this 4 long?

It is the closest power of 2, to pacify my OCD, but you are right.

> > + ret = regmap_bulk_read(sen0322->regmap, SEN0322_REG_DATA, data, 3);
>
> Having shortened above, use sizeof(data) for that 3 to avoid
> any potential future mismatch in sizes.

Agreed.

> > + dev_dbg(regmap_get_device(sen0322->regmap), "data: %d\n", ret);
>
> Given you more or less directly provide this to userspace now I'd drop
> the dev_dbg() as not adding any value for debugging.
>

I just like to see if the function actually ran and not reading some buffered
value stuck somewhere, but okay.

> > + switch (mask) {
> > + case IIO_CHAN_INFO_RAW:
> > + switch (chan->type) {
> > + case IIO_CONCENTRATION:
> As the sensor only does concentration, you could either drop this
> check on basis we can't get here without it or if you want
> a strong sanity check do it outside the switch statement as
> if (chan->type != IIO_CONCENTRATION)
> return -EINVAL;

I did not want to deviate from the pattern, but yes, it will make the code
more readable.

Best regards,
János