Messages in this thread | | | Subject | Re: [PATCH 1/3] iio: sps30: separate core and interface specific code | From | Lars-Peter Clausen <> | Date | Sun, 25 Apr 2021 17:16:14 +0200 |
| |
On 4/25/21 3:55 PM, Tomasz Duszynski wrote: > Move code responsible for handling i2c communication to a separate file. > Rationale for this change is preparation for adding support for serial > communication. > > Signed-off-by: Tomasz Duszynski <tomasz.duszynski@octakon.com>
Hi,
The whole series looks really great. Couple of small comments inline.
> [...] > diff --git a/drivers/iio/chemical/Kconfig b/drivers/iio/chemical/Kconfig > index 10bb431bc3ce..82af5f62fbc6 100644 > --- a/drivers/iio/chemical/Kconfig > +++ b/drivers/iio/chemical/Kconfig > @@ -133,8 +133,6 @@ config SENSIRION_SGP30 > > config SPS30 > tristate "SPS30 particulate matter sensor" > - depends on I2C > - select CRC8 > select IIO_BUFFER > select IIO_TRIGGERED_BUFFER > help > @@ -144,6 +142,17 @@ config SPS30 > To compile this driver as a module, choose M here: the module will > be called sps30. > > +config SPS30_I2C > + tristate "SPS30 particulate matter sensor I2C driver" > + depends on SPS30 && I2C > + select CRC8 Since the base module is not very useful without any of the drivers enabled, what you can do here is, make the base module non-user-selectable, e.g. remove the description text after the tristate and then do a `select SPS30` both here from he I2C module and the serial module. > + help > + Say Y here to build support for the Sensirion SPS30 I2C interface > + driver. > + > + To compile this driver as a module, choose M here: the module will > + be called sps30_i2c. > + > config VZ89X > tristate "SGX Sensortech MiCS VZ89X VOC sensor" > depends on I2C > diff --git a/drivers/iio/chemical/Makefile b/drivers/iio/chemical/Makefile > index fef63dd5bf92..41c264a229c0 100644 > --- a/drivers/iio/chemical/Makefile > +++ b/drivers/iio/chemical/Makefile > @@ -17,4 +17,5 @@ obj-$(CONFIG_SCD30_I2C) += scd30_i2c.o > obj-$(CONFIG_SCD30_SERIAL) += scd30_serial.o > obj-$(CONFIG_SENSIRION_SGP30) += sgp30.o > obj-$(CONFIG_SPS30) += sps30.o > +obj-$(CONFIG_SPS30_I2C) += sps30_i2c.o > obj-$(CONFIG_VZ89X) += vz89x.o > diff --git a/drivers/iio/chemical/sps30.c b/drivers/iio/chemical/sps30.c > index 7486591588c3..ec9db99e324c 100644 > --- a/drivers/iio/chemical/sps30.c > +++ b/drivers/iio/chemical/sps30.c > [...] > +EXPORT_SYMBOL(sps30_probe); EXPORT_SYMBOL_GPL() > > MODULE_AUTHOR("Tomasz Duszynski <tduszyns@gmail.com>"); > MODULE_DESCRIPTION("Sensirion SPS30 particulate matter sensor driver"); > diff --git a/drivers/iio/chemical/sps30.h b/drivers/iio/chemical/sps30.h > new file mode 100644 > index 000000000000..d2b7140fdeb4 > --- /dev/null > +++ b/drivers/iio/chemical/sps30.h > @@ -0,0 +1,73 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +#ifndef _SPS30_H > +#define _SPS30_H > + > +struct sps30_state; > +struct sps30_ops { > + int (*start_meas)(struct sps30_state *state); > + int (*stop_meas)(struct sps30_state *state); > + int (*read_meas)(struct sps30_state *state, int *meas, int num); > + int (*reset)(struct sps30_state *state); > + int (*clean_fan)(struct sps30_state *state); > + int (*read_cleaning_period)(struct sps30_state *state, int *period); > + int (*write_cleaning_period)(struct sps30_state *state, int period);
The interface for {read,write}_cleaning_period() should use __be32, since that is what is being passed around.
I was a bit confused when reviewing the uart driver why we can just memcpy an int into the output buffer without endianess problems.
> + int (*show_info)(struct sps30_state *state); > +}; > + [...]
| |