Message-ID: <20250426120841.249aabc9@jic23-huawei>
Date: Sat, 26 Apr 2025 12:08:41 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: surajsonawane0215@...il.com
Cc: lars@...afoo.de, linux-kernel@...r.kernel.org, linux-iio@...r.kernel.org
Subject: Re: [PATCH 2/2] iio: chemical: Add driver for Sharp GP2Y1010AU0F
 dust sensor

On Sat, 26 Apr 2025 03:42:14 +0530
surajsonawane0215@...il.com wrote:

> From: Suraj Sonawane <surajsonawane0215@...il.com>
> 
> Implement support for the Sharp GP2Y1010AU0F optical dust sensor which
> measures particulate matter concentration using infrared scattering. The
> sensor requires precise 280us LED pulses with ADC sampling at 200us after
> LED activation.
> 
> The driver provides:
> - Raw voltage readings through IIO interface
> - Hardware-agnostic operation via GPIO and IIO ADC interfaces
> - Power management through regulator framework
> - Device Tree binding support
> 
> Device operation:
> 1. Activate IR LED for 280us
> 2. Wait 40us after LED activation
> 3. Sample analog output at 200us (peak sensitivity)
> 4. Convert voltage to dust density via calibration parameters
> 
> Tested on BeagleBone Black with:
> - P8_12 (GPIO_44) for LED control
> - P9_39 (AIN0) for analog output
> 
> Datasheet:
> https://global.sharp/products/device/lineup/data/pdf/datasheet/gp2y1010au_appl_e.pdf

As in patch 1, make this part of the tags block.

> 
> Signed-off-by: Suraj Sonawane <surajsonawane0215@...il.com>

Various comments inline.  Interesting new type of sensor, so I think
we need a new channel type.  Trying to make that general is always the
challenge, but currently density looks appropriate to me.

Jonathan

> ---
>  MAINTAINERS                     |   7 ++
>  drivers/iio/chemical/Kconfig    |  12 +++
>  drivers/iio/chemical/Makefile   |   1 +
>  drivers/iio/chemical/gp2y1010.c | 183 ++++++++++++++++++++++++++++++++
>  4 files changed, 203 insertions(+)
>  create mode 100644 drivers/iio/chemical/gp2y1010.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index f31aeb6b4..54e0f67e0 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -21974,6 +21974,13 @@ F:	drivers/iio/chemical/sps30.c
>  F:	drivers/iio/chemical/sps30_i2c.c
>  F:	drivers/iio/chemical/sps30_serial.c
>  
> +SHARP GP2Y1010AU0F DUST SENSOR DRIVER
> +M:	Suraj Sonawane <surajsonawane0215@...il.com>
> +L:	linux-iio@...r.kernel.org
> +S:	Maintained
> +F:	Documentation/devicetree/bindings/iio/chemical/sharp,gp2y1010au0f.yaml
> +F:	drivers/iio/chemical/gp2y1010.c
> +
>  SERIAL DEVICE BUS
>  M:	Rob Herring <robh@...nel.org>
>  L:	linux-serial@...r.kernel.org
> diff --git a/drivers/iio/chemical/Kconfig b/drivers/iio/chemical/Kconfig
> index 330fe0af9..119c6e6d8 100644
> --- a/drivers/iio/chemical/Kconfig
> +++ b/drivers/iio/chemical/Kconfig
> @@ -100,6 +100,18 @@ config ENS160_SPI
>  	tristate
>  	select REGMAP_SPI
>  
> +config GP2Y1010AU0F
> +	tristate "Sharp GP2Y1010AU0F optical dust sensor"
> +	depends on IIO
> +	select IIO_BUFFER
> +	select IIO_TRIGGERED_BUFFER

Not seeing any buffered support in here so these seem unnecessary.
For a device like this I'd be very surprised if there was any reason to add it!

> +	help
> +	  Say Y here to build support for Sharp GP2Y1010AU0F optical dust sensor
> +	  that measures particulate matter concentration in air.
> +
> +	  To compile this driver as a module, choose M here: the module will be
> +	  called gp2y1010au0f.
> +
>  config IAQCORE
>  	tristate "AMS iAQ-Core VOC sensors"
>  	depends on I2C
> diff --git a/drivers/iio/chemical/Makefile b/drivers/iio/chemical/Makefile
> index 4866db06b..296bbc0d0 100644
> --- a/drivers/iio/chemical/Makefile
> +++ b/drivers/iio/chemical/Makefile
> @@ -15,6 +15,7 @@ obj-$(CONFIG_ENS160) += ens160_core.o
>  obj-$(CONFIG_ENS160_I2C) += ens160_i2c.o
>  obj-$(CONFIG_ENS160_SPI) += ens160_spi.o
>  obj-$(CONFIG_IAQCORE)		+= ams-iaq-core.o
> +obj-$(CONFIG_GP2Y1010AU0F) += gp2y1010.o

Alphabetical order by config name.

>  obj-$(CONFIG_PMS7003) += pms7003.o
>  obj-$(CONFIG_SCD30_CORE) += scd30_core.o
>  obj-$(CONFIG_SCD30_I2C) += scd30_i2c.o
> diff --git a/drivers/iio/chemical/gp2y1010.c b/drivers/iio/chemical/gp2y1010.c
> new file mode 100644
> index 000000000..19c09e0e3
> --- /dev/null
> +++ b/drivers/iio/chemical/gp2y1010.c
> @@ -0,0 +1,183 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2025 Suraj Sonawane <surajsonawane0215@...il.com>
> + *
> + * Sharp GP2Y1010AU0F Dust Sensor Driver
> + *
> + * Datasheet:
> + * https://global.sharp/products/device/lineup/data/pdf/datasheet/gp2y1010au_appl_e.pdf
> + *
This blank line doesn't add anything so drop it.

> + */
> +
> +#include <linux/module.h>

Alphabetical order preferred in IIO.

> +#include <linux/init.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>

This only applies if there is custom userspace ABI.

> +#include <linux/iio/consumer.h>
> +#include <linux/of_device.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/of.h>

Why?  I'm not (correctly) seeing an of specific alls in here.

> +#include <linux/platform_device.h>
> +#include <linux/err.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/delay.h>
> +
> +#define GP2Y1010_LED_PULSE_US    280  /* LED on-time (280us) */
> +#define GP2Y1010_SAMPLE_DELAY_US  40  /* Wait 40us after LED on */
> +#define GP2Y1010_MEASUREMENT_US  200  /* Measure 200us after LED on */
> +
> +struct gp2y1010_data {
> +	struct gpio_desc *led_gpio;
> +	struct iio_dev *indio_dev;

As below. We shouldn't need to get to the indio_dev from here.
Everything should go the other way (iio_priv(indio_dev) to get to this).

> +	struct iio_channel *adc_chan;
> +	struct regulator *vdd;
> +	int v_clean;  /* Calibration: voltage in clean air (mV) */
> +};
> +
> +static int gp2y1010_power_on(struct gp2y1010_data *data)
> +{
> +	int ret;
> +
> +	ret = regulator_enable(data->vdd);
> +	if (ret) {
> +		dev_err(&data->indio_dev->dev, "Failed to enable vdd regulator\n");

Generally we'd use the pdev->dev for error messages not the IIO one because the
resulting name print is normally easier to use.

> +		return ret;
> +	}
See below.  I don't currently see the usefulness for this manual
enabling /disabling code.
> +
> +	udelay(100); /* Short delay for regulator stability */
> +	return 0;
> +}
> +
> +static void gp2y1010_power_off(struct gp2y1010_data *data)
> +{
> +	regulator_disable(data->vdd);
> +}
> +
> +static int gp2y1010_read_raw(struct iio_dev *indio_dev,
> +							 struct iio_chan_spec const *chan,
> +							 int *val, int *val2, long mask)
> +{
> +	struct gp2y1010_data *data = iio_priv(indio_dev);
> +	int ret, voltage_mv;
> +
> +	if (mask != IIO_CHAN_INFO_RAW)
> +		return -EINVAL;
> +
> +    /* Turn on LED */
Kind of obvious so not sure the comment helps.
> +	gpiod_set_value(data->led_gpio, 1);
> +
> +    /* Wait 40us (datasheet: delay after LED on) */
> +	udelay(GP2Y1010_SAMPLE_DELAY_US);
> +
> +    /* Read ADC at 200us (peak sensitivity) */
> +	udelay(GP2Y1010_MEASUREMENT_US - GP2Y1010_SAMPLE_DELAY_US);

Why not a single delay of the total time?

> +	ret = iio_read_channel_processed(data->adc_chan, &voltage_mv);
	gpiod_set_value(data->led_gpio, 0);
	if (ret < 0 ) {
...

as then you can avoid turning the led off in both good and bad paths
via separate calls.

> +	if (ret < 0) {
> +		gpiod_set_value(data->led_gpio, 0);
> +		return ret;
> +	}
> +
> +    /* Turn off LED (total pulse width = 280us) */

This comment is useful because of the provided time information
so keep this one.

> +	gpiod_set_value(data->led_gpio, 0);
> +
> +    /* Store raw voltage (for debugging) */

Why is it for debugging?  This is ultimately what we pass to userspace.

> +	*val = voltage_mv;
> +
> +	return IIO_VAL_INT;
> +}
> +
> +static const struct iio_info gp2y1010_info = {
> +	.read_raw = gp2y1010_read_raw,
> +};
> +
> +static const struct iio_chan_spec gp2y1010_channels[] = {
> +	{
> +		.type = IIO_VOLTAGE,

Whilst you are measuring a voltage the real thing being measured is
about dust.  There is a graph in the datasheet of voltage vs dust density
in mg/m^3  So I think this needs a new channel type.  Probably
IIO_DENSITY with base units of g / m^3 

in this case without per sensor calibration I doubt we can get
to a processed value but we can still have a raw channel of
IIO_DENSITY type.

Remember to add Docs to Documentation/ABI/testing/sysfs-bus-iio for
the new density type.



> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> +		.extend_name = "dust",
extend_name is a terrible old design decision that we more or less never
use in new drivers because it makes for a hard ABI for userspace code to parse.

Instead provide the get_label() callback if this is useful.

> +	},
> +};
> +
> +static int gp2y1010_probe(struct platform_device *pdev)
> +{
> +	struct gp2y1010_data *data;
> +	struct iio_dev *indio_dev;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*data));
Quite a lot of references to &pdev->dev so I would have a local
variable
	struct device *dev = &pdev->dev;

that will shorten a lot of lines a little bit.

> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	data = iio_priv(indio_dev);
> +	data->indio_dev = indio_dev;

This is usually not a great sign.   Generally it means that you have passed
data to some callback where you should have passed the iio_dev and used
iio_priv() to get to data.  We should not be seeing the linkages in both
directions.


> +	data->v_clean = 900;  /* Default calibration (adjust per sensor) */

If you need to adjust this per sensor, how do we do it?

> +
> +	/* Get LED GPIO */

This code is self explanatory.  Comments that don't add a lot of value tend
to just pad out code and provide places for future disagreements between docs
and code as a driver evolves.  So generally we just don't have this sort
of comment.

> +	data->led_gpio = devm_gpiod_get(&pdev->dev, "led", GPIOD_OUT_LOW);
> +	if (IS_ERR(data->led_gpio))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(data->led_gpio),
> +							"Failed to get LED GPIO\n");
> +
> +    /* Get regulator */
Likewise.  Self explanatory.
> +	data->vdd = devm_regulator_get(&pdev->dev, "vdd");

No need to keep hold of vdd for now (you may need it later if you do
runtime pm or similar) so can use
	ret = devm_regulator_get_enabled(dev, "vdd);
	if (ret)
		return ret;

	udelay(100); /* Short delay for regulator stability */

and drop the need to power it down explicitly.

> +	if (IS_ERR(data->vdd))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(data->vdd),
> +							"Failed to get regulator\n");
> +
> +    /* Power on sensor */
> +	ret = gp2y1010_power_on(data);
> +	if (ret)
> +		return ret;
As below
	ret = devm_add_action_or_reset(&pdev->dev, new_power_off_callback, data);
	if (ret)
		return ret;

however becomes irrelevant if you make changes suggested above.
> +
> +    /* Get ADC channel */
> +	data->adc_chan = devm_iio_channel_get(&pdev->dev, "dust");
> +	if (IS_ERR(data->adc_chan)) {
> +		gp2y1010_power_off(data);
> +		return dev_err_probe(&pdev->dev, PTR_ERR(data->adc_chan),
> +							"Failed to get ADC channel\n");
> +	}
> +
> +	/* Setup IIO device */
> +	indio_dev->name = "gp2y1010";
> +	indio_dev->info = &gp2y1010_info;
> +	indio_dev->channels = gp2y1010_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(gp2y1010_channels);
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +
> +	ret = devm_iio_device_register(&pdev->dev, indio_dev);
> +	if (ret) {
> +		gp2y1010_power_off(data);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static void gp2y1010_remove(struct platform_device *pdev)
> +{
> +	struct gp2y1010_data *data = platform_get_drvdata(pdev);
> +
> +	gp2y1010_power_off(data);
Use a devm_add_action_or_reset() with a custom callback to power this down.

As things currently stand, you are mixing devm cleanup and manual
cleanup which means that we remove the userspace interfaces after
turning the power off. That's generally a path to weird bug reports!

> +}
> +
> +static const struct of_device_id gp2y1010_of_match[] = {
> +	{ .compatible = "sharp,gp2y1010au0f", },
> +	{}
We have standardized in IIO on a space between brackets on terminators like
this 
	{ }
No particular reason for that choice beyond wanting to pick one or the other
style!

> +};
> +MODULE_DEVICE_TABLE(of, gp2y1010_of_match);
> +
> +static struct platform_driver gp2y1010_driver = {
> +	.driver = {
> +		.name = "gp2y1010",
> +		.of_match_table = gp2y1010_of_match,
> +	},
> +	.probe = gp2y1010_probe,
> +	.remove = gp2y1010_remove,
> +};
> +
> +module_platform_driver(gp2y1010_driver);
> +
> +MODULE_AUTHOR("Suraj Sonawane <surajsonawane0215@...il.com>");
> +MODULE_DESCRIPTION("Sharp GP2Y1010AU0F Dust Sensor Driver");
> +MODULE_LICENSE("GPL");

Powered by blists - more mailing lists