Re: [PATCH v2 2/3] iio: chemical: Add Sensteair Sunrise 006-0-007 driver

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

On Fri, Aug 20, 2021 at 4:38 PM Jacopo Mondi <jacopo@jmondi.org> wrote:
>
> Add support for the Senseair Sunrise 006-0-0007 driver through the
> IIO subsystem.

...

> +config SENSEAIR_SUNRISE_CO2
> +       tristate "Senseair Sunrise 006-0-0007 CO2 sensor"

> +       depends on I2C

Actually

select REGMAP_I2C

...

> + * List of features not yet supported by the driver:
> + * - support for controllable EN pin

To avoid tautology

* - controllable EN pin


> + * - support for single-shot operations using the nDRY pin.

Ditto.

* - single-shot operations using the nDRY pin.

> + * - ABC/target calibration

...

> +#include <linux/i2c.h>

> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>

Can you move this as a separate group...

> +#include <linux/kernel.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/regmap.h>
> +#include <linux/sysfs.h>

...here ?

...

> +#define SUNRISE_CALIBRATION_TIMEOUT_US         30000000

30 * USEC_PER_SEC ?

...

> +static void sunrise_wakeup(struct sunrise_dev *sunrise)
> +{
> +       struct i2c_client *client = sunrise->client;
> +
> +       /*
> +        * Wake up sensor by sending sensor address: START, sensor address,
> +        * STOP. Sensor will not ACK this byte.
> +        *
> +        * The chip returns in low power state after 15msec without
> +        * communications or after a complete read/write sequence.
> +        */

I'm wondering if there is a better way to perform this.

> +       i2c_smbus_xfer(client->adapter, client->addr, I2C_M_IGNORE_NAK,
> +                      I2C_SMBUS_WRITE, 0, I2C_SMBUS_QUICK, NULL);
> +}

...

> +               dev_err(sunrise->dev, "Read word failed: reg 0x%2x (%d)\n",
> +                       reg, ret);

One line?

...

> +               dev_err(sunrise->dev, "Write byte failed: reg 0x%2x (%d)\n",
> +                       reg, ret);

One line?

...

> +               dev_err(sunrise->dev, "Write word failed: reg 0x%2x (%d)\n",
> +                       reg, ret);

One line?

...

> +       /* Write calibration command and poll the calibration status bit. */

Write a calibration

...

> +static ssize_t sunrise_calibration_write(struct iio_dev *iiodev,
> +                                        uintptr_t private,
> +                                        const struct iio_chan_spec *chan,
> +                                        const char *buf, size_t len)
> +{
> +       struct sunrise_dev *sunrise = iio_priv(iiodev);
> +       bool calibrate;
> +       int ret;
> +
> +       ret = kstrtobool(buf, &calibrate);
> +       if (ret)

> +               return -EINVAL;

Shadowed return code.

> +       if (!calibrate)
> +               return 0;
> +
> +       ret = sunrise_calibrate(sunrise);
> +
> +       return ret ?: len;

In this case

  if (ret)
    return ret;

return len;

will look more natural.

> +}

...

> +static ssize_t sunrise_error_status_read(struct iio_dev *iiodev,
> +                                        uintptr_t private,
> +                                        const struct iio_chan_spec *chan,
> +                                        char *buf)
> +{
> +       struct sunrise_dev *sunrise = iio_priv(iiodev);
> +       ssize_t len = 0;
> +       u16 value;
> +       int ret;
> +       u8 i;
> +
> +       ret = sunrise_read_word(sunrise, SUNRISE_ERROR_STATUS_REG, &value);
> +       if (ret)
> +               return -EINVAL;

> +       for (i = 0; i < ARRAY_SIZE(error_codes); ++i) {
> +               if (!(value & BIT(error_codes[i])))
> +                       continue;

for_each_set_bit()

> +               len += sysfs_emit_at(buf, len, "%s ",
> +                                    sunrise_error_statuses[i]);

One line?

> +       }
> +
> +       if (len)
> +               buf[len - 1] = '\n';
> +
> +       return len;
> +}

...

> +static const struct iio_chan_spec_ext_info sunrise_concentration_ext_info[] = {
> +       /* Calibration modes and calibration trigger. */
> +       {
> +               .name = "calibration",
> +               .write = sunrise_calibration_write,
> +               .shared = IIO_SEPARATE,
> +       },

> +       IIO_ENUM("calibration_mode", IIO_SEPARATE,
> +                &sunrise_calibration_modes_enum),

One line?

> +       IIO_ENUM_AVAILABLE("calibration_mode",
> +                          &sunrise_calibration_modes_enum),

One line?

> +       /* Error statuses. */
> +       {
> +               .name = "error_status",
> +               .read = sunrise_error_status_read,
> +               .shared = IIO_SEPARATE,
> +       },
> +       IIO_ENUM_AVAILABLE("error_status", &sunrise_error_statuses_enum),

> +       {},

No comma for terminator entries.

> +};

...

> +static int sunrise_read_raw(struct iio_dev *iio_dev,
> +                           const struct iio_chan_spec *chan,
> +                           int *val, int *val2, long mask)
> +{
> +       struct sunrise_dev *sunrise = iio_priv(iio_dev);
> +       u16 value;
> +       int ret;
> +
> +       switch (mask) {
> +       case IIO_CHAN_INFO_RAW:

> +

Redundant blank line.

> +               mutex_lock(&sunrise->lock);
> +
> +               switch (chan->type) {
> +               case IIO_CONCENTRATION: {
> +                       ret = sunrise_read_word(sunrise,
> +                                               SUNRISE_CO2_FILTERED_COMP_REG,
> +                                               &value);
> +                       *val = value;
> +                       mutex_unlock(&sunrise->lock);
> +
> +                       return ret ?: IIO_VAL_INT;
> +               }

You don't need {} anymore.

> +               case IIO_TEMP: {
> +                       ret = sunrise_read_word(sunrise,
> +                                               SUNRISE_CHIP_TEMPERATURE_REG,
> +                                               &value);
> +                       *val = value;
> +                       mutex_unlock(&sunrise->lock);
> +
> +                       return ret ?: IIO_VAL_INT;
> +               }

Ditto.

> +               default:
> +                       mutex_unlock(&sunrise->lock);
> +                       return -EINVAL;
> +               }
> +
> +       case IIO_CHAN_INFO_SCALE:
> +               /* Chip temperature scale = 1/100 */
> +               *val = 1;
> +               *val2 = 100;
> +               return IIO_VAL_FRACTIONAL;
> +
> +       default:
> +               return -EINVAL;
> +       }

> +
> +       return 0;

Dead code?

> +}


-- 
With Best Regards,
Andy Shevchenko