On Fri, Aug 20, 2021 at 4:38 PM Jacopo Mondi <jacopo@xxxxxxxxxx> 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
