diff mbox series

[v2,1/2] dt-bindings: iio: temperature: Add AMS AS6200

Message ID 20231202041651.719963-1-alkuor@gmail.com
State Changes Requested
Headers show
Series [v2,1/2] dt-bindings: iio: temperature: Add AMS AS6200 | expand

Checks

Context Check Description
robh/checkpatch success
robh/patch-applied success
robh/dtbs-check warning build log
robh/dt-meta-schema success

Commit Message

Abdel Alkuor Dec. 2, 2023, 4:16 a.m. UTC 
as6200 is high accuracy temperature sensor of -/+ 0.4C degree
with a range between -40C to 125C degrees

Signed-off-by: Abdel Alkuor <alkuor@gmail.com>
---
Changes in v2:
  - Add vdd-supply

 .../bindings/iio/temperature/ams,as6200.yaml  | 49 +++++++++++++++++++
 1 file changed, 49 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/temperature/ams,as6200.yaml

Comments

Conor Dooley Dec. 3, 2023, 11:24 a.m. UTC | #1 
On Fri, Dec 01, 2023 at 11:16:50PM -0500, Abdel Alkuor wrote:
> as6200 is high accuracy temperature sensor of -/+ 0.4C degree

Is +/- 0.4 degrees really "high accuracy"?

> with a range between -40C to 125C degrees
> 
> Signed-off-by: Abdel Alkuor <alkuor@gmail.com>
> ---
> Changes in v2:
>   - Add vdd-supply
> 
>  .../bindings/iio/temperature/ams,as6200.yaml  | 49 +++++++++++++++++++
>  1 file changed, 49 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/temperature/ams,as6200.yaml
> 
> diff --git a/Documentation/devicetree/bindings/iio/temperature/ams,as6200.yaml b/Documentation/devicetree/bindings/iio/temperature/ams,as6200.yaml
> new file mode 100644
> index 000000000000..a1817795cdca
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/temperature/ams,as6200.yaml
> @@ -0,0 +1,49 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/temperature/ams,as6200.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: AMS AS6200 Temperature Sensor
> +
> +maintainers:
> +  - Abdel Alkuor <alkuor@gmail.com>
> +
> +description: |

Please add the text from your commit message (although perhaps dropping
the "high accuracy" section) here.

Otherwise, this looks okay to me.

Thanks,
Conor.

> +  https://ams.com/documents/20143/36005/AS6200_DS000449_4-00.pdf
> +
> +properties:
> +  compatible:
> +    const: ams,as6200
> +
> +  reg:
> +    maxItems: 1
> +
> +  vdd-supply: true
> +
> +  interrupts:
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - reg
> +  - vdd-supply
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        temperature-sensor@48 {
> +            compatible = "ams,as6200";
> +            reg = <0x48>;
> +            vdd-supply = <&vdd>;
> +            interrupt-parent = <&gpio1>;
> +            interrupts = <17 IRQ_TYPE_EDGE_BOTH>;
> +        };
> +    };
> +...
> -- 
> 2.34.1
>
Jonathan Cameron Dec. 4, 2023, 1:50 p.m. UTC | #2 
On Fri,  1 Dec 2023 23:16:51 -0500
Abdel Alkuor <alkuor@gmail.com> wrote:

> as6200 is a high accuracy temperature sensor of 0.0625C with a range
> between -40 to 125 Celsius degrees.
> 
> The driver implements the alert trigger event in comparator mode where
> consecutive faults are converted into periods, high/low temperature
> thresholds require to be above/below the set limit for n seconds for
> the alert to be triggered/cleared. The alert is only cleared when the
> current temperature is below the low temperature threshold for n seconds.
> 
> The driver supports the following:
> - show available sampling frequencey
> - read/write sampling frequency
> - read raw temperature
> - read scaling factor
> - read/write temperature period that needs to be met for low/high
>   temperature thresholds to trigger an alert
> - show available temperature period thresholds
> - buffer trigger
> - temperature alert event trigger

Hi Abdel,

A few comments inline. Looking good in general.

> 
> Datasheet: https://ams.com/documents/20143/36005/AS6200_DS000449_4-00.pdf
> 

No blank line here.  Tags block (and Datasheet is a tag) never has blank lines
as that breaks some existing tooling.

> Signed-off-by: Abdel Alkuor <alkuor@gmail.com>
>  
>  What:		/sys/bus/iio/devices/iio:deviceX/in_filter_notch_center_frequency
> diff --git a/drivers/iio/temperature/Kconfig b/drivers/iio/temperature/Kconfig
> index ed384f33e0c7..a0ffbc77e623 100644
> --- a/drivers/iio/temperature/Kconfig
> +++ b/drivers/iio/temperature/Kconfig
> @@ -4,6 +4,17 @@
>  #
>  menu "Temperature sensors"
>  
> +config AS6200
> +       tristate "AS6200 temperature sensor"
> +       depends on I2C
> +       select REGMAP_I2C
> +       help
> +         If you say yes here you get support for AS6200
> +         temperature sensor chip connected via I2C.
> +
> +         This driver can also be built as a module.  If so, the module
> +         will be called as6200.
> +
>  config IQS620AT_TEMP
>  	tristate "Azoteq IQS620AT temperature sensor"
>  	depends on MFD_IQS62X || COMPILE_TEST
> @@ -157,5 +168,4 @@ config MAX31865
>  
>  	  This driver can also be build as a module. If so, the module
>  	  will be called max31865.
> -
Stray change.

>  endmenu

> diff --git a/drivers/iio/temperature/as6200.c b/drivers/iio/temperature/as6200.c
> new file mode 100644
> index 000000000000..7fcc785871d8
> --- /dev/null
> +++ b/drivers/iio/temperature/as6200.c
> @@ -0,0 +1,493 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Driver for AMS AS6200 Temperature sensor
> + *
> + * Author: Abdel Alkuor <alkuor@gmail.com>
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/device.h>
> +#include <linux/i2c.h>
> +#include <linux/interrupt.h>
> +#include <linux/kstrtox.h>
> +#include <linux/module.h>
> +#include <linux/regmap.h>
> +
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/events.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/iio/trigger.h>
> +#include <linux/iio/triggered_buffer.h>
> +#include <linux/iio/trigger_consumer.h>
> +
> +#define AS6200_TVAL_REG		0x0
> +#define AS6200_CONFIG_REG	0x1
> +#define AS6200_TLOW_REG		0x2
> +#define AS6200_THIGH_REG	0x3
> +
> +#define AS6200_CONFIG_AL	BIT(5)
> +#define AS6200_CONFIG_CR	GENMASK(7, 6)
> +#define AS6200_CONFIG_SM	BIT(8)
> +#define AS6200_CONFIG_IM	BIT(9)
> +#define AS6200_CONFIG_POL	BIT(10)
> +#define AS6200_CONFIG_CF	GENMASK(12, 11)
> +
> +#define AS6200_TEMP_MASK	GENMASK(15, 4)
> +
> +struct as6200 {
> +	struct regmap *regmap;
> +	struct mutex lock; /* Prevent concurrent temp fault processing */

Why does it matter? What might cause such processing?

> +};
> +
> +static const int as6200_samp_freq[4][2] = {
> +	{ 0, 250000 },
> +	{ 1, 0 },
> +	{ 4, 0 },
> +	{ 8, 0 }
> +};
> +
> +/* Consective faults converted to period */
> +static const int as6200_temp_thresh_periods[4][4][2] = {
> +	{ { 4, 0 }, { 8, 0 }, { 16, 0 }, { 24, 0 } },
> +	{ { 1, 0 }, { 2, 0 }, { 4, 0 }, { 6, 0 } },
> +	{ { 0, 250000 }, { 0, 500000 }, { 1, 0 }, { 2, 0} },
> +	{ { 0, 125000 }, { 0, 250000 }, { 0, 500000 }, { 0, 750000 } }

I'd suggest naming the first column at least (which is CR I think?)

So define an enum and 
enum {
	AS6200_CR_0_25HZ = 0,
	AS6200_CR_1HZ = 1,
	AS6200_CR_4HZ = 2,
	AS6200_CR_8HZ = 3,
};
And use that for the samp freq entries above, so that they clearly relate
to the rows of this arram
	[AS6200_CR_0_25HZ] = { { 4, 0 }, { 8, 0 }, { 16, 0 }, { 24, 0 } },
You could take it further and use an enum for CF as well.

	[AS6200_CR_0_25HZ] = {
		[AS6200_CF_1] = { 4, 0 },
		[AS6200_CF_2] = { 8, 0 },
		[AS6200_CF_4] = { 16, 0 },
		[AS6200_CF_6] = { 24, 0 },
	},
	[AS6200_CR_1_HZ] = {
		[AS6200_CF_1] = { 1, 0 },
		[AS6200_CF_2] = { 2, 0 },
	...	
	}
	etc which makes it clear where all the numbers come.

> +};


> +static int as6200_read_event_value(struct iio_dev *indio_dev,
> +				   const struct iio_chan_spec *chan,
> +				   enum iio_event_type type,
> +				   enum iio_event_direction dir,
> +				   enum iio_event_info info,
> +				   int *val, int *val2)
> +{
> +	struct as6200 *as = iio_priv(indio_dev);
> +	unsigned int reg;
> +	unsigned int tmp;
> +	int ret;
> +	u8 cf;
> +	u8 cr;
> +
> +	switch (dir) {
> +	case IIO_EV_DIR_FALLING:
> +		reg = AS6200_TLOW_REG;
> +		break;
> +	case IIO_EV_DIR_RISING:
> +		reg = AS6200_THIGH_REG;
> +		break;
> +	case IIO_EV_DIR_EITHER:
> +		reg = AS6200_CONFIG_REG;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	ret = regmap_read(as->regmap, reg, &tmp);
> +	if (ret)
> +		return ret;
> +
> +	if (info == IIO_EV_INFO_VALUE) {
> +		*val = sign_extend32(FIELD_GET(AS6200_TEMP_MASK, tmp), 11);
> +		ret = IIO_VAL_INT;
return here.

> +	} else {
> +		cf = FIELD_GET(AS6200_CONFIG_CF, tmp);
> +		cr = FIELD_GET(AS6200_CONFIG_CR, tmp);
> +		*val = as6200_temp_thresh_periods[cr][cf][0];
> +		*val2 = as6200_temp_thresh_periods[cr][cf][1];
> +		ret = IIO_VAL_INT_PLUS_MICRO;

and here.  If there is nothing more to be done, it simplifies the code
flow being read to just return as quick as possible.

> +	}
> +
> +	return ret;
> +}

> +static irqreturn_t as6200_event_handler(int irq, void *private)
> +{
> +	struct iio_dev *indio_dev = private;
> +	struct as6200 *as = iio_priv(indio_dev);
> +	unsigned int alert;
> +	enum iio_event_direction dir;
> +	int ret;
> +
> +	guard(mutex)(&as->lock);
What data are we protecting here?

> +
> +	ret = regmap_read(as->regmap, AS6200_CONFIG_REG, &alert);
> +	if (ret)
> +		return IRQ_NONE;
> +
> +	alert = FIELD_GET(AS6200_CONFIG_AL, alert);
> +
> +	dir = alert ? IIO_EV_DIR_FALLING : IIO_EV_DIR_RISING;
> +
> +	iio_push_event(indio_dev,
> +		       IIO_EVENT_CODE(IIO_TEMP, 0, 0,
> +				      dir,
> +				      IIO_EV_TYPE_THRESH,
> +				      0, 0, 0),
> +		       iio_get_time_ns(indio_dev));
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static irqreturn_t as6200_trigger_handler(int irq, void *private)
> +{
> +	struct iio_poll_func *pf = private;
> +	struct iio_dev *indio_dev = pf->indio_dev;
> +	struct as6200 *as = iio_priv(indio_dev);
> +	int ret;
> +	u8 data[16];

I'd make this much more explicit and make sure you zero it to avoid leaking
data.

	struct data {
		u8 channel;
		s64 timestamp __aligned(8);
	};

	memset(&data, 0, sizeof(data)); /* Ensures the holes are zero filled */

Also, avoid the casting mess and read into a local variable that is an unsigned int
and copy it to the struct data if no error.
> +
> +	ret = regmap_read(as->regmap, AS6200_TVAL_REG, (unsigned int *)data);
> +	if (!ret)
Whilst it's more lines, greatly prefer to see error paths out of line and good
paths inline (so no if (!ret))

	if (ret)
		goto done;

	iio_push...

done:
	...

May seem silly but when reviewing a lot of code, keeping things looking "normal"
is a great benefit!

> +		iio_push_to_buffers_with_timestamp(indio_dev, data,
> +						   iio_get_time_ns(indio_dev));
> +
> +	iio_trigger_notify_done(indio_dev->trig);
> +
> +	return IRQ_HANDLED;
> +}

...

> +
> +static int __maybe_unused as6200_suspend(struct device *dev)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct as6200 *as = iio_priv(i2c_get_clientdata(client));
> +
> +	if (client->irq)
> +		disable_irq(client->irq);
> +
> +	return regmap_update_bits(as->regmap, AS6200_CONFIG_REG,
> +				  AS6200_CONFIG_SM, AS6200_CONFIG_SM);
> +}
> +
> +static int __maybe_unused as6200_resume(struct device *dev)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct as6200 *as = iio_priv(i2c_get_clientdata(client));
> +
> +	if (client->irq)
> +		enable_irq(client->irq);

I would normally expect suspend and resume to be mirror images. If that doesn't
make sense for some reason and we do need to do the irq handling
before the register write in both cases then add a comment.

> +
> +	return regmap_update_bits(as->regmap, AS6200_CONFIG_REG,
> +				  AS6200_CONFIG_SM, 0);
> +}
> +
> +static const struct dev_pm_ops as6200_pm_ops = {

DEFINE_SIMPLE_DEV_PM_OPS()


> +	SET_SYSTEM_SLEEP_PM_OPS(as6200_suspend, as6200_resume)
> +};
> +
> +static const struct i2c_device_id as6200_id_table[] = {
> +	{ "as6200" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(i2c, as6200_id_table);
> +
> +static const struct of_device_id as6200_of_match[] = {
> +	{ .compatible = "ams,as6200" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, as6200_of_match);
> +
> +static struct i2c_driver as6200_driver = {
> +	.driver = {
> +		.name = "as6200",
> +		.pm = pm_sleep_ptr(&as6200_pm_ops),
> +		.of_match_table = as6200_of_match,
> +	},
> +	.probe = as6200_probe,
> +	.id_table = as6200_id_table,
> +};
> +module_i2c_driver(as6200_driver);
> +
> +MODULE_AUTHOR("Abdel Alkuor <alkuor@gmail.com");
> +MODULE_DESCRIPTION("AMS AS6200 Temperature Sensor");
> +MODULE_LICENSE("GPL");
Abdel Alkuor Dec. 5, 2023, 2:16 a.m. UTC | #3 
On Mon, Dec 04, 2023 at 01:50:14PM +0000, Jonathan Cameron wrote:
> On Fri,  1 Dec 2023 23:16:51 -0500
> Abdel Alkuor <alkuor@gmail.com> wrote:
> 
> > as6200 is a high accuracy temperature sensor of 0.0625C with a range
> > between -40 to 125 Celsius degrees.
> > 
> > The driver implements the alert trigger event in comparator mode where
> > consecutive faults are converted into periods, high/low temperature
> > thresholds require to be above/below the set limit for n seconds for
> > the alert to be triggered/cleared. The alert is only cleared when the
> > current temperature is below the low temperature threshold for n seconds.
> > 
> > The driver supports the following:
> > - show available sampling frequencey
> > - read/write sampling frequency
> > - read raw temperature
> > - read scaling factor
> > - read/write temperature period that needs to be met for low/high
> >   temperature thresholds to trigger an alert
> > - show available temperature period thresholds
> > - buffer trigger
> > - temperature alert event trigger
> 
> Hi Abdel,
> 
> A few comments inline. Looking good in general.
>
Hi Jonathon,

Thank you for your time. I have a couple _silly_ questions about the tag
and returning from if else. Other than that, your comments will be addressed
in v3.
> > 
> > Datasheet: https://ams.com/documents/20143/36005/AS6200_DS000449_4-00.pdf
> > 
> 
> No blank line here.  Tags block (and Datasheet is a tag) never has blank lines
> as that breaks some existing tooling.
> 
Understood. 

P.S. when running checkpatch.pl on this patch, I get the following warning:

WARNING: Unknown link reference 'Datasheet:', use 'Link:' or 'Closes:' instead

should I use Link instead?
> > Signed-off-by: Abdel Alkuor <alkuor@gmail.com>
> >  
> >  What:		/sys/bus/iio/devices/iio:deviceX/in_filter_notch_center_frequency
> > diff --git a/drivers/iio/temperature/Kconfig b/drivers/iio/temperature/Kconfig
> > index ed384f33e0c7..a0ffbc77e623 100644
> > --- a/drivers/iio/temperature/Kconfig
> > +++ b/drivers/iio/temperature/Kconfig
> > @@ -4,6 +4,17 @@
> >  #
> >  menu "Temperature sensors"
> >  
> > +config AS6200
> > +       tristate "AS6200 temperature sensor"
> > +       depends on I2C
> > +       select REGMAP_I2C
> > +       help
> > +         If you say yes here you get support for AS6200
> > +         temperature sensor chip connected via I2C.
> > +
> > +         This driver can also be built as a module.  If so, the module
> > +         will be called as6200.
> > +
> >  config IQS620AT_TEMP
> >  	tristate "Azoteq IQS620AT temperature sensor"
> >  	depends on MFD_IQS62X || COMPILE_TEST
> > @@ -157,5 +168,4 @@ config MAX31865
> >  
> >  	  This driver can also be build as a module. If so, the module
> >  	  will be called max31865.
> > -
> Stray change.
>
Ops, I fixed it locally and forgot to regenerate a patch after. Will be fixed
in v3
> >  endmenu
> 
> > diff --git a/drivers/iio/temperature/as6200.c b/drivers/iio/temperature/as6200.c
> > new file mode 100644
> > index 000000000000..7fcc785871d8
> > --- /dev/null
> > +++ b/drivers/iio/temperature/as6200.c
> > @@ -0,0 +1,493 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Driver for AMS AS6200 Temperature sensor
> > + *
> > + * Author: Abdel Alkuor <alkuor@gmail.com>
> > + */
> > +
> > +#include <linux/bitfield.h>
> > +#include <linux/device.h>
> > +#include <linux/i2c.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/kstrtox.h>
> > +#include <linux/module.h>
> > +#include <linux/regmap.h>
> > +
> > +#include <linux/iio/buffer.h>
> > +#include <linux/iio/events.h>
> > +#include <linux/iio/iio.h>
> > +#include <linux/iio/sysfs.h>
> > +#include <linux/iio/trigger.h>
> > +#include <linux/iio/triggered_buffer.h>
> > +#include <linux/iio/trigger_consumer.h>
> > +
> > +#define AS6200_TVAL_REG		0x0
> > +#define AS6200_CONFIG_REG	0x1
> > +#define AS6200_TLOW_REG		0x2
> > +#define AS6200_THIGH_REG	0x3
> > +
> > +#define AS6200_CONFIG_AL	BIT(5)
> > +#define AS6200_CONFIG_CR	GENMASK(7, 6)
> > +#define AS6200_CONFIG_SM	BIT(8)
> > +#define AS6200_CONFIG_IM	BIT(9)
> > +#define AS6200_CONFIG_POL	BIT(10)
> > +#define AS6200_CONFIG_CF	GENMASK(12, 11)
> > +
> > +#define AS6200_TEMP_MASK	GENMASK(15, 4)
> > +
> > +struct as6200 {
> > +	struct regmap *regmap;
> > +	struct mutex lock; /* Prevent concurrent temp fault processing */
> 
> Why does it matter? What might cause such processing?
> 
Good point. I had a misunderstanding how the interrupts work and I did
some reading and realized that the interrupt is disabled until the
buttom half of the interrupt is completed as oneshot type is used.
I'll remove it in v3.
> > +};
> > +
> > +static const int as6200_samp_freq[4][2] = {
> > +	{ 0, 250000 },
> > +	{ 1, 0 },
> > +	{ 4, 0 },
> > +	{ 8, 0 }
> > +};
> > +
> > +/* Consective faults converted to period */
> > +static const int as6200_temp_thresh_periods[4][4][2] = {
> > +	{ { 4, 0 }, { 8, 0 }, { 16, 0 }, { 24, 0 } },
> > +	{ { 1, 0 }, { 2, 0 }, { 4, 0 }, { 6, 0 } },
> > +	{ { 0, 250000 }, { 0, 500000 }, { 1, 0 }, { 2, 0} },
> > +	{ { 0, 125000 }, { 0, 250000 }, { 0, 500000 }, { 0, 750000 } }
> 
> I'd suggest naming the first column at least (which is CR I think?)
> 
> So define an enum and 
> enum {
> 	AS6200_CR_0_25HZ = 0,
> 	AS6200_CR_1HZ = 1,
> 	AS6200_CR_4HZ = 2,
> 	AS6200_CR_8HZ = 3,
> };
> And use that for the samp freq entries above, so that they clearly relate
> to the rows of this arram
> 	[AS6200_CR_0_25HZ] = { { 4, 0 }, { 8, 0 }, { 16, 0 }, { 24, 0 } },
> You could take it further and use an enum for CF as well.
> 
> 	[AS6200_CR_0_25HZ] = {
> 		[AS6200_CF_1] = { 4, 0 },
> 		[AS6200_CF_2] = { 8, 0 },
> 		[AS6200_CF_4] = { 16, 0 },
> 		[AS6200_CF_6] = { 24, 0 },
> 	},
> 	[AS6200_CR_1_HZ] = {
> 		[AS6200_CF_1] = { 1, 0 },
> 		[AS6200_CF_2] = { 2, 0 },
> 	...	
> 	}
> 	etc which makes it clear where all the numbers come.
> 
> > +};
>
Good suggestion. I'll fix it in v3.
> 
> > +static int as6200_read_event_value(struct iio_dev *indio_dev,
> > +				   const struct iio_chan_spec *chan,
> > +				   enum iio_event_type type,
> > +				   enum iio_event_direction dir,
> > +				   enum iio_event_info info,
> > +				   int *val, int *val2)
> > +{
> > +	struct as6200 *as = iio_priv(indio_dev);
> > +	unsigned int reg;
> > +	unsigned int tmp;
> > +	int ret;
> > +	u8 cf;
> > +	u8 cr;
> > +
> > +	switch (dir) {
> > +	case IIO_EV_DIR_FALLING:
> > +		reg = AS6200_TLOW_REG;
> > +		break;
> > +	case IIO_EV_DIR_RISING:
> > +		reg = AS6200_THIGH_REG;
> > +		break;
> > +	case IIO_EV_DIR_EITHER:
> > +		reg = AS6200_CONFIG_REG;
> > +		break;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +
> > +	ret = regmap_read(as->regmap, reg, &tmp);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (info == IIO_EV_INFO_VALUE) {
> > +		*val = sign_extend32(FIELD_GET(AS6200_TEMP_MASK, tmp), 11);
> > +		ret = IIO_VAL_INT;
> return here.
> 
> > +	} else {
> > +		cf = FIELD_GET(AS6200_CONFIG_CF, tmp);
> > +		cr = FIELD_GET(AS6200_CONFIG_CR, tmp);
> > +		*val = as6200_temp_thresh_periods[cr][cf][0];
> > +		*val2 = as6200_temp_thresh_periods[cr][cf][1];
> > +		ret = IIO_VAL_INT_PLUS_MICRO;
> 
> and here.  If there is nothing more to be done, it simplifies the code
> flow being read to just return as quick as possible.
>
I did it as you mentioned, and when running check_patch.pl, it gives back a
warning that else is not needed here because of the return in the if
statement. So I opted into using ret instead, should I remove the else or ignore
the warning?
> > +	}
> > +
> > +	return ret;
> > +}
> 
> > +static irqreturn_t as6200_event_handler(int irq, void *private)
> > +{
> > +	struct iio_dev *indio_dev = private;
> > +	struct as6200 *as = iio_priv(indio_dev);
> > +	unsigned int alert;
> > +	enum iio_event_direction dir;
> > +	int ret;
> > +
> > +	guard(mutex)(&as->lock);
> What data are we protecting here?
> 
No data actually. As I mentioned prior, will be dropped in v3.
> > +
> > +	ret = regmap_read(as->regmap, AS6200_CONFIG_REG, &alert);
> > +	if (ret)
> > +		return IRQ_NONE;
> > +
> > +	alert = FIELD_GET(AS6200_CONFIG_AL, alert);
> > +
> > +	dir = alert ? IIO_EV_DIR_FALLING : IIO_EV_DIR_RISING;
> > +
> > +	iio_push_event(indio_dev,
> > +		       IIO_EVENT_CODE(IIO_TEMP, 0, 0,
> > +				      dir,
> > +				      IIO_EV_TYPE_THRESH,
> > +				      0, 0, 0),
> > +		       iio_get_time_ns(indio_dev));
> > +
> > +	return IRQ_HANDLED;
> > +}
> > +
> > +static irqreturn_t as6200_trigger_handler(int irq, void *private)
> > +{
> > +	struct iio_poll_func *pf = private;
> > +	struct iio_dev *indio_dev = pf->indio_dev;
> > +	struct as6200 *as = iio_priv(indio_dev);
> > +	int ret;
> > +	u8 data[16];
> 
> I'd make this much more explicit and make sure you zero it to avoid leaking
> data.
> 
> 	struct data {
> 		u8 channel;
> 		s64 timestamp __aligned(8);
> 	};
> 
> 	memset(&data, 0, sizeof(data)); /* Ensures the holes are zero filled */
>
My system 1 thinking got me here as I tested it and all holes
were set to 0. Will be fixed in v3.
> Also, avoid the casting mess and read into a local variable that is an unsigned int
> and copy it to the struct data if no error.
> > +
> > +	ret = regmap_read(as->regmap, AS6200_TVAL_REG, (unsigned int *)data);
> > +	if (!ret)
> Whilst it's more lines, greatly prefer to see error paths out of line and good
> paths inline (so no if (!ret))
> 
> 	if (ret)
> 		goto done;
> 
> 	iio_push...
> 
> done:
> 	...
> 
> May seem silly but when reviewing a lot of code, keeping things looking "normal"
> is a great benefit!
>
Makes sense. Will be fixed in v3.
> > +		iio_push_to_buffers_with_timestamp(indio_dev, data,
> > +						   iio_get_time_ns(indio_dev));
> > +
> > +	iio_trigger_notify_done(indio_dev->trig);
> > +
> > +	return IRQ_HANDLED;
> > +}
> 
> ...
> 
> > +
> > +static int __maybe_unused as6200_suspend(struct device *dev)
> > +{
> > +	struct i2c_client *client = to_i2c_client(dev);
> > +	struct as6200 *as = iio_priv(i2c_get_clientdata(client));
> > +
> > +	if (client->irq)
> > +		disable_irq(client->irq);
> > +
> > +	return regmap_update_bits(as->regmap, AS6200_CONFIG_REG,
> > +				  AS6200_CONFIG_SM, AS6200_CONFIG_SM);
> > +}
> > +
> > +static int __maybe_unused as6200_resume(struct device *dev)
> > +{
> > +	struct i2c_client *client = to_i2c_client(dev);
> > +	struct as6200 *as = iio_priv(i2c_get_clientdata(client));
> > +
> > +	if (client->irq)
> > +		enable_irq(client->irq);
> 
> I would normally expect suspend and resume to be mirror images. If that doesn't
> make sense for some reason and we do need to do the irq handling
> before the register write in both cases then add a comment.
No reason. Will be fixed in v3.
> 
> > +
> > +	return regmap_update_bits(as->regmap, AS6200_CONFIG_REG,
> > +				  AS6200_CONFIG_SM, 0);
> > +}
> > +
> > +static const struct dev_pm_ops as6200_pm_ops = {
> 
> DEFINE_SIMPLE_DEV_PM_OPS()
>
Will be fixed in v3.
> 
> > +	SET_SYSTEM_SLEEP_PM_OPS(as6200_suspend, as6200_resume)
> > +};
> > +
> > +static const struct i2c_device_id as6200_id_table[] = {
> > +	{ "as6200" },
> > +	{ }
> > +};
> > +MODULE_DEVICE_TABLE(i2c, as6200_id_table);
> > +
> > +static const struct of_device_id as6200_of_match[] = {
> > +	{ .compatible = "ams,as6200" },
> > +	{ }
> > +};
> > +MODULE_DEVICE_TABLE(of, as6200_of_match);
> > +
> > +static struct i2c_driver as6200_driver = {
> > +	.driver = {
> > +		.name = "as6200",
> > +		.pm = pm_sleep_ptr(&as6200_pm_ops),
> > +		.of_match_table = as6200_of_match,
> > +	},
> > +	.probe = as6200_probe,
> > +	.id_table = as6200_id_table,
> > +};
> > +module_i2c_driver(as6200_driver);
> > +
> > +MODULE_AUTHOR("Abdel Alkuor <alkuor@gmail.com");
> > +MODULE_DESCRIPTION("AMS AS6200 Temperature Sensor");
> > +MODULE_LICENSE("GPL");
>
Abdel Alkuor Dec. 5, 2023, 2:19 a.m. UTC | #4 
On Sun, Dec 03, 2023 at 11:24:31AM +0000, Conor Dooley wrote:
> On Fri, Dec 01, 2023 at 11:16:50PM -0500, Abdel Alkuor wrote:
> > as6200 is high accuracy temperature sensor of -/+ 0.4C degree
>
Hi Conor,
> Is +/- 0.4 degrees really "high accuracy"?
>
That's what the datasheet says :D. I'll remove it.
> > with a range between -40C to 125C degrees
> > 
> > Signed-off-by: Abdel Alkuor <alkuor@gmail.com>
> > ---
> > Changes in v2:
> >   - Add vdd-supply
> > 
> >  .../bindings/iio/temperature/ams,as6200.yaml  | 49 +++++++++++++++++++
> >  1 file changed, 49 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/iio/temperature/ams,as6200.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/iio/temperature/ams,as6200.yaml b/Documentation/devicetree/bindings/iio/temperature/ams,as6200.yaml
> > new file mode 100644
> > index 000000000000..a1817795cdca
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/temperature/ams,as6200.yaml
> > @@ -0,0 +1,49 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/iio/temperature/ams,as6200.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: AMS AS6200 Temperature Sensor
> > +
> > +maintainers:
> > +  - Abdel Alkuor <alkuor@gmail.com>
> > +
> > +description: |
> 
> Please add the text from your commit message (although perhaps dropping
> the "high accuracy" section) here.
> 
> Otherwise, this looks okay to me.
Sounds good. Will add it in v3.
> 
> Thanks,
> Conor.
> 
Thanks,
Abdel
> > +  https://ams.com/documents/20143/36005/AS6200_DS000449_4-00.pdf
> > +
> > +properties:
> > +  compatible:
> > +    const: ams,as6200
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  vdd-supply: true
> > +
> > +  interrupts:
> > +    maxItems: 1
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - vdd-supply
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/interrupt-controller/irq.h>
> > +    i2c {
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +
> > +        temperature-sensor@48 {
> > +            compatible = "ams,as6200";
> > +            reg = <0x48>;
> > +            vdd-supply = <&vdd>;
> > +            interrupt-parent = <&gpio1>;
> > +            interrupts = <17 IRQ_TYPE_EDGE_BOTH>;
> > +        };
> > +    };
> > +...
> > -- 
> > 2.34.1
> >
Jonathan Cameron Dec. 6, 2023, 5:36 p.m. UTC | #5 
On Mon, 4 Dec 2023 21:16:34 -0500
Abdel Alkuor <alkuor@gmail.com> wrote:

> On Mon, Dec 04, 2023 at 01:50:14PM +0000, Jonathan Cameron wrote:
> > On Fri,  1 Dec 2023 23:16:51 -0500
> > Abdel Alkuor <alkuor@gmail.com> wrote:
> >   
> > > as6200 is a high accuracy temperature sensor of 0.0625C with a range
> > > between -40 to 125 Celsius degrees.
> > > 
> > > The driver implements the alert trigger event in comparator mode where
> > > consecutive faults are converted into periods, high/low temperature
> > > thresholds require to be above/below the set limit for n seconds for
> > > the alert to be triggered/cleared. The alert is only cleared when the
> > > current temperature is below the low temperature threshold for n seconds.
> > > 
> > > The driver supports the following:
> > > - show available sampling frequencey
> > > - read/write sampling frequency
> > > - read raw temperature
> > > - read scaling factor
> > > - read/write temperature period that needs to be met for low/high
> > >   temperature thresholds to trigger an alert
> > > - show available temperature period thresholds
> > > - buffer trigger
> > > - temperature alert event trigger  
> > 
> > Hi Abdel,
> > 
> > A few comments inline. Looking good in general.
> >  
> Hi Jonathon,
> 
> Thank you for your time. I have a couple _silly_ questions about the tag
> and returning from if else. Other than that, your comments will be addressed
> in v3.
> > > 
> > > Datasheet: https://ams.com/documents/20143/36005/AS6200_DS000449_4-00.pdf
> > >   
> > 
> > No blank line here.  Tags block (and Datasheet is a tag) never has blank lines
> > as that breaks some existing tooling.
> >   
> Understood. 
> 
> P.S. when running checkpatch.pl on this patch, I get the following warning:
> 
> WARNING: Unknown link reference 'Datasheet:', use 'Link:' or 'Closes:' instead
> 
> should I use Link instead?

Nah. Just ignore checkpatch. :)

Also, if just accepting a comment, no need to say so.  Assumption of reviewer
is that if you keep quiet on a particular comment you will have it fixed in next
version.  Crop out that section of the email and only keep the bits that
you want the reviewer to see your comments on.

Hopefully I've cropped it appropriately!

> > > Signed-off-by: Abdel Alkuor <alkuor@gmail.com>
> > > +	if (info == IIO_EV_INFO_VALUE) {
> > > +		*val = sign_extend32(FIELD_GET(AS6200_TEMP_MASK, tmp), 11);
> > > +		ret = IIO_VAL_INT;  
> > return here.
> >   
> > > +	} else {
> > > +		cf = FIELD_GET(AS6200_CONFIG_CF, tmp);
> > > +		cr = FIELD_GET(AS6200_CONFIG_CR, tmp);
> > > +		*val = as6200_temp_thresh_periods[cr][cf][0];
> > > +		*val2 = as6200_temp_thresh_periods[cr][cf][1];
> > > +		ret = IIO_VAL_INT_PLUS_MICRO;  
> > 
> > and here.  If there is nothing more to be done, it simplifies the code
> > flow being read to just return as quick as possible.
> >  
> I did it as you mentioned, and when running check_patch.pl, it gives back a
> warning that else is not needed here because of the return in the if
> statement. So I opted into using ret instead, should I remove the else or ignore
> the warning?

Dropping the else is fine or take the view it's wrong and ignore it.
Checkpatch is providing hints and suggestions on where to double check.
There is no requirement to accept it's suggestions if you feel the
code ends up less readable.

> > > +	}
> > > +
> > > +	return ret;
Jonathan Cameron Dec. 6, 2023, 5:40 p.m. UTC | #6 
On Mon, 4 Dec 2023 21:19:22 -0500
Abdel Alkuor <alkuor@gmail.com> wrote:

> On Sun, Dec 03, 2023 at 11:24:31AM +0000, Conor Dooley wrote:
> > On Fri, Dec 01, 2023 at 11:16:50PM -0500, Abdel Alkuor wrote:  
> > > as6200 is high accuracy temperature sensor of -/+ 0.4C degree  
> >  
> Hi Conor,
> > Is +/- 0.4 degrees really "high accuracy"?
> >  
> That's what the datasheet says :D. I'll remove it.

I'd not noticed this.  Why is this an IIO driver rather than an hwmon one?

Mostly we do that only for very high accuracy or weird temperature sensors
(infrared ones for example or things with complex thermocouple handling).
Simpler devices meant for hardware monitoring type applications typically go
in hwmon.

We have the iio to hwmon bridge driver for things that naturally have uses
where IIO features are needed, but which get used for hwmon sometimes.
Not sure this is enough IIO focused though, so perhaps list out why you
think it should be in IIO?

Jonathan
Abdel Alkuor Dec. 9, 2023, 7:28 p.m. UTC | #7 
On Wed, Dec 06, 2023 at 05:40:50PM +0000, Jonathan Cameron wrote:
> On Mon, 4 Dec 2023 21:19:22 -0500
> Abdel Alkuor <alkuor@gmail.com> wrote:
>
Hi Jonathan,
> I'd not noticed this.  Why is this an IIO driver rather than an hwmon one?
> 
> Mostly we do that only for very high accuracy or weird temperature sensors
> (infrared ones for example or things with complex thermocouple handling).
> Simpler devices meant for hardware monitoring type applications typically go
> in hwmon.
> 
> We have the iio to hwmon bridge driver for things that naturally have uses
> where IIO features are needed, but which get used for hwmon sometimes.
> Not sure this is enough IIO focused though, so perhaps list out why you
> think it should be in IIO?
No specific reason. I thought IIO is the de facto for such sensors. I'll
use hwmon instead.

Thanks the clarification.

Abdel
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/iio/temperature/ams,as6200.yaml b/Documentation/devicetree/bindings/iio/temperature/ams,as6200.yaml
new file mode 100644
index 000000000000..a1817795cdca
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/temperature/ams,as6200.yaml
@@ -0,0 +1,49 @@ 
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/temperature/ams,as6200.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: AMS AS6200 Temperature Sensor
+
+maintainers:
+  - Abdel Alkuor <alkuor@gmail.com>
+
+description: |
+  https://ams.com/documents/20143/36005/AS6200_DS000449_4-00.pdf
+
+properties:
+  compatible:
+    const: ams,as6200
+
+  reg:
+    maxItems: 1
+
+  vdd-supply: true
+
+  interrupts:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - vdd-supply
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        temperature-sensor@48 {
+            compatible = "ams,as6200";
+            reg = <0x48>;
+            vdd-supply = <&vdd>;
+            interrupt-parent = <&gpio1>;
+            interrupts = <17 IRQ_TYPE_EDGE_BOTH>;
+        };
+    };
+...