Messages in this thread | | | Date | Sun, 25 Nov 2018 16:14:34 +0530 | From | Himanshu Jha <> | Subject | Re: [PATCH 2/3] iio: chemical: add support for Sensirion SPS30 sensor |
| |
On Sat, Nov 24, 2018 at 11:14:14PM +0100, Tomasz Duszynski wrote: > Add support for Sensirion SPS30 particulate matter sensor. > > Signed-off-by: Tomasz Duszynski <tduszyns@gmail.com> > --- > drivers/iio/chemical/Kconfig | 11 ++ > drivers/iio/chemical/Makefile | 1 + > drivers/iio/chemical/sps30.c | 359 ++++++++++++++++++++++++++++++++++ > 3 files changed, 371 insertions(+) > create mode 100644 drivers/iio/chemical/sps30.c
[]
> +#define pr_fmt(fmt) "sps30: " fmt
I don't see its usage ?
> +#include <linux/crc8.h> > +#include <linux/delay.h> > +#include <linux/i2c.h> > +#include <linux/iio/buffer.h> > +#include <linux/iio/iio.h> > +#include <linux/iio/sysfs.h> > +#include <linux/iio/trigger_consumer.h> > +#include <linux/iio/triggered_buffer.h> > +#include <linux/module.h> > + > +#define SPS30_CRC8_POLYNOMIAL 0x31 > + > +/* SPS30 commands */ > +#define SPS30_START_MEAS 0x0010 > +#define SPS30_STOP_MEAS 0x0104 > +#define SPS30_RESET 0xd304 > +#define SPS30_READ_DATA_READY_FLAG 0x0202 > +#define SPS30_READ_DATA 0x0300 > +#define SPS30_READ_SERIAL 0xD033
Could you please put a tab and align these macros.
#define SPS30_START_MEAS 0x0010 #define SPS30_STOP_MEAS 0x0104
> +static int sps30_write_then_read(struct sps30_state *state, u8 *buf, > + int buf_size, u8 *data, int data_size) > +{ > + /* every two received data bytes are checksummed */ > + u8 tmp[data_size + data_size / 2];
No VLAs!
https://lore.kernel.org/lkml/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qPXydAacU1RqZWA@mail.gmail.com/
> + int ret, i; > + > + /* > + * Sensor does not support repeated start so instead of > + * sending two i2c messages in a row we just send one by one. > + */ > + ret = i2c_master_send(state->client, buf, buf_size); > + if (ret != buf_size) > + return ret < 0 ? ret : -EIO; > + > + if (!data) > + return 0; > + > + ret = i2c_master_recv(state->client, tmp, sizeof(tmp)); > + if (ret != sizeof(tmp)) > + return ret < 0 ? ret : -EIO; > + > + for (i = 0; i < sizeof(tmp); i += 3) { > + u8 crc = crc8(sps30_crc8_table, &tmp[i], 2, CRC8_INIT_VALUE); > + > + if (crc != tmp[i + 2]) { > + dev_err(&state->client->dev, > + "data integrity check failed\n"); > + return -EIO; > + } > + > + *data++ = tmp[i]; > + *data++ = tmp[i + 1]; > + } > + > + return 0; > +} > + > +static int sps30_do_cmd(struct sps30_state *state, u16 cmd, u8 *data, int size) > +{ > + /* depending on the command up to 3 bytes may be needed for argument */ > + u8 buf[sizeof(cmd) + 3] = { cmd >> 8, cmd };
This is fine, since sizeof returns constant integer expression.
> + switch (cmd) { > + case SPS30_START_MEAS: > + buf[2] = 0x03; > + buf[3] = 0x00; > + buf[4] = 0xac; /* precomputed crc */ > + return sps30_write_then_read(state, buf, 5, NULL, 0); > + case SPS30_STOP_MEAS: > + case SPS30_RESET: > + return sps30_write_then_read(state, buf, 2, NULL, 0); > + case SPS30_READ_DATA_READY_FLAG: > + case SPS30_READ_DATA: > + case SPS30_READ_SERIAL: > + return sps30_write_then_read(state, buf, 2, data, size); > + default: > + return -EINVAL; > + }; > +}
> +static int sps30_read_raw(struct iio_dev *indio_dev, > + struct iio_chan_spec const *chan, > + int *val, int *val2, long mask) > +{ > + struct sps30_state *state = iio_priv(indio_dev); > + int ret; > + > + switch (mask) { > + case IIO_CHAN_INFO_PROCESSED: > + switch (chan->type) { > + case IIO_MASSCONCENTRATION: > + mutex_lock(&state->lock); > + switch (chan->channel2) { > + case IIO_MOD_PM2p5:
I think lock should be placed here
> + ret = sps30_do_meas(state, val, val2);
... and unlock here.
> + break; > + case IIO_MOD_PM10: > + ret = sps30_do_meas(state, val2, val); > + break; > + default: > + break; > + } > + mutex_unlock(&state->lock); > + if (ret) > + return ret; > + > + return IIO_VAL_INT; > + default: > + return -EINVAL; > + } > + break; > + default: > + return -EINVAL; > + } > +}
[]
> +static int sps30_probe(struct i2c_client *client) > +{ > + struct iio_dev *indio_dev; > + struct sps30_state *state; > + u8 buf[32] = { };
This is not valid in ISO-C but only in C++.
Instead,
u8 buf[32] = {0};
> + int ret; > + > + if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) > + return -EOPNOTSUPP; > + > + indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*state)); > + if (!indio_dev) > + return -ENOMEM; > + > + state = iio_priv(indio_dev); > + i2c_set_clientdata(client, indio_dev); > + state->client = client; > + indio_dev->dev.parent = &client->dev; > + indio_dev->info = &sps30_info; > + indio_dev->name = client->name; > + indio_dev->channels = sps30_channels; > + indio_dev->num_channels = ARRAY_SIZE(sps30_channels); > + indio_dev->modes = INDIO_DIRECT_MODE; > + indio_dev->available_scan_masks = sps30_scan_masks; > + > + mutex_init(&state->lock); > + crc8_populate_msb(sps30_crc8_table, SPS30_CRC8_POLYNOMIAL); > + > + /* > + * Power-on-reset causes sensor to produce some glitch on i2c bus > + * and some controllers end up in error state. Recover simply > + * by placing something on the bus. > + */ > + ret = sps30_do_cmd(state, SPS30_RESET, NULL, 0); > + if (ret) { > + dev_err(&client->dev, "failed to reset device\n"); > + return ret; > + } > + usleep_range(2500000, 3500000);
Isn't that range too high ? msleep_interruptible ?
> + sps30_do_cmd(state, SPS30_STOP_MEAS, NULL, 0); > + > + ret = sps30_do_cmd(state, SPS30_READ_SERIAL, buf, sizeof(buf)); > + if (ret) { > + dev_err(&client->dev, "failed to read serial number\n"); > + return ret; > + } > + dev_info(&client->dev, "serial number: %s\n", buf); > + > + ret = sps30_do_cmd(state, SPS30_START_MEAS, NULL, 0); > + if (ret) { > + dev_err(&client->dev, "failed to start measurement\n"); > + return ret; > + } > + > + ret = devm_iio_triggered_buffer_setup(&client->dev, indio_dev, NULL, > + sps30_trigger_handler, NULL); > + if (ret) > + return ret; > + > + return devm_iio_device_register(&client->dev, indio_dev); > +} > + > +static int sps30_remove(struct i2c_client *client)
Perfect candidate for `devm_add_action_or_reset()` ?
> +{ > + struct iio_dev *indio_dev = i2c_get_clientdata(client); > + struct sps30_state *state = iio_priv(indio_dev); > + > + sps30_do_cmd(state, SPS30_STOP_MEAS, NULL, 0); > + > + return 0; > +}
-- Himanshu Jha Undergraduate Student Department of Electronics & Communication Guru Tegh Bahadur Institute of Technology
| |