On Fri, Sep 3, 2021 at 5:50 PM Jacopo Mondi <jacopo@xxxxxxxxxx> 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
