Re: [PATCH 1/3] iio: sps30: separate core and interface specific code

[Tomasz Duszynski <tomasz.duszynski@octakon.com>]

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@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.

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@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.
>

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);
> > +};
> > +
> [...]