lkml.org 
[lkml]   [2021]   [Apr]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH 1/3] iio: sps30: separate core and interface specific code
From
Date
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);
> +};
> +
[...]

\
 
 \ /
  Last update: 2021-04-25 17:16    [W:0.099 / U:0.888 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site