On Sun, Apr 25, 2021 at 05:16:14PM +0200, Lars-Peter Clausen wrote:
> 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@xxxxxxxxxxx>
>
> 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.
Right. Current form was helpful in debugging and eventually I forgot to
change it to something reasonable. Thanks for catching this.
> > + 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()
Ack.
> > MODULE_AUTHOR("Tomasz Duszynski <tduszyns@xxxxxxxxx>");
> > 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.
>
Agree that indeed might be confusing at first read since as you said
all endianess conversions are handled in core and other components just
take everything verbatim - which isn't that obvious.
> > + int (*show_info)(struct sps30_state *state);
> > +};
> > +
> [...]
