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

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

On Fri, Sep 3, 2021 at 5:50 PM Jacopo Mondi <jacopo@jmondi.org> wrote:
>
> Add support for the Senseair Sunrise 006-0-0007 driver through the
> IIO subsystem.

...

> +static int sunrise_read_byte(struct sunrise_dev *sunrise, u8 reg)
> +{
> +       unsigned int val;
> +       int ret;
> +
> +       /*
> +        * Lock the 'wakeup' session.
> +        *
> +        * If another read/write call sneaks in between the wakeup message
> +        * and the i2c transaction, the chip goes back in sleep state.
> +        */

> +       mutex_lock(&sunrise->wakeup_lock);
> +       sunrise_wakeup(sunrise);
> +       ret = regmap_read(sunrise->regmap, reg, &val);
> +       mutex_unlock(&sunrise->wakeup_lock);

Seems to me that you may redefine ->read() for regmap (but double
check this, esp. in regard to bulk transfers) with wakeup implied and
in that case you probably can use regmap's lock only.

> +       if (ret) {

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

With the same LOCs I slightly prefer temporary variable

struct device *dev = ...;
  ...
dev_err(dev, ...); // on a single line

Ditto everywhere.

> +               return ret;
> +       }
> +
> +       return val;
> +}

...

> +static int sunrise_read_word(struct sunrise_dev *sunrise, u8 reg, u16 *val)
> +{
> +       __be16 be_val;
> +       int ret;
> +
> +       mutex_lock(&sunrise->wakeup_lock);
> +       sunrise_wakeup(sunrise);

> +       ret = regmap_bulk_read(sunrise->regmap, reg, &be_val, 2);

sizeof(be_val)

> +       mutex_unlock(&sunrise->wakeup_lock);
> +       if (ret) {
> +               dev_err(&sunrise->client->dev,
> +                       "Read word failed: reg 0x%2x (%d)\n", reg, ret);
> +               return ret;
> +       }
> +
> +       *val = be16_to_cpu(be_val);
> +
> +       return 0;
> +}

...

> +static int sunrise_write_word(struct sunrise_dev *sunrise, u8 reg, u16 data)
> +{
> +       __be16 be_data = cpu_to_be16(data);
> +       int ret;
> +
> +       mutex_lock(&sunrise->wakeup_lock);
> +       sunrise_wakeup(sunrise);
> +       ret = regmap_bulk_write(sunrise->regmap, reg, &be_data, 2);
> +       mutex_unlock(&sunrise->wakeup_lock);
> +       if (ret) {
> +               dev_err(&sunrise->client->dev,
> +                       "Write word failed: reg 0x%2x (%d)\n", reg, ret);

> +               return ret;
> +       }
> +
> +       return 0;

return ret;

> +}

...

> +       /* Write a calibration command and poll the calibration status bit. */
> +       ret = sunrise_write_word(sunrise, SUNRISE_CALIBRATION_COMMAND_REG,
> +                                data->cmd);

Dunno how long it is, but to me seems one line is okay.

> +       if (ret)
> +               return ret;

...

> +static ssize_t sunrise_cal_read(const char *buf, size_t len)
> +{
> +       bool enable;
> +       int ret;
> +
> +       ret = kstrtobool(buf, &enable);
> +       if (ret)
> +               return ret;
> +
> +       if (!enable)
> +               return len;
> +
> +       return 0;

Why is this a separate function to begin with?

Not sure I have got the logic behind. If enable is true you return 0?!

> +}

...

> +static ssize_t sunrise_cal_factory_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);
> +       int ret;
> +
> +       mutex_lock(&sunrise->lock);
> +       ret = sunrise_cal_read(buf, len);

> +       if (ret) {
> +               mutex_unlock(&sunrise->lock);
> +               return ret;
> +       }

Possibly

  if (ret)
    goto out_unlock;

> +       ret = sunrise_calibrate(sunrise,
> +                               &calib_data[SUNRISE_CALIBRATION_FACTORY]);

One line?

out_unlock:

> +       mutex_unlock(&sunrise->lock);
> +       if (ret)
> +               return ret;
> +
> +       return len;
> +}

...

> +       errors = value;
> +       for_each_set_bit(i, &errors, ARRAY_SIZE(sunrise_error_statuses))
> +               len += sysfs_emit_at(buf, len, "%s ", sunrise_error_statuses[i]);

> +

Can drop this (up to you).

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

...

> +                       ret = sunrise_read_word(sunrise, SUNRISE_CO2_FILTERED_COMP_REG,
> +                                               &value);

One line?

...

> +                       ret = sunrise_read_word(sunrise, SUNRISE_CHIP_TEMPERATURE_REG,
> +                                               &value);

Ditto.

...

> +               /* x10 to comply with IIO scale (1/1000 degress celsius). */

1/1000 -->  millidegrees
(Also note spelling)

--
With Best Regards,
Andy Shevchenko