[PATCH v3 1/3] mfd: da9063: Add HWMON dependencies

[PATCH v3 1/3] mfd: da9063: Add HWMON dependencies

[Vincent Pelletier]

From: "Opensource [Steve Twiss]" <stwiss.opensource@diasemi.com>

Dependencies required for DA9063 HWMON support.

Signed-off-by: Opensource [Steve Twiss] <stwiss.opensource@diasemi.com>

Moved temperature offset reading to hwmon driver.

Signed-off-by: Vincent Pelletier <plr.vincent@gmail.com>
---
Changes in v3:
- moved temperature offset reading to hwmon driver

Changes in v2:
- registers.h changes moved from patch 2

Originally submitted by Steve Twiss in 2014:
  https://marc.info/?l=linux-kernel&m=139560864709852&w=2

 include/linux/mfd/da9063/registers.h | 34 ++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/include/linux/mfd/da9063/registers.h b/include/linux/mfd/da9063/registers.h
index 6e0f66a2e727..297631ddda39 100644
--- a/include/linux/mfd/da9063/registers.h
+++ b/include/linux/mfd/da9063/registers.h
@@ -512,6 +512,7 @@
 
 /* DA9063_REG_GPIO_0_1 (addr=0x15) */
 #define	DA9063_GPIO0_PIN_MASK			0x03
+#define	DA9063_GPIO0_PIN_MASK_SHIFT		0
 #define		DA9063_GPIO0_PIN_ADCIN1		0x00
 #define		DA9063_GPIO0_PIN_GPI		0x01
 #define		DA9063_GPIO0_PIN_GPO_OD		0x02
@@ -523,6 +524,7 @@
 #define		DA9063_GPIO0_TYPE_GPO_VDD_IO2	0x04
 #define	DA9063_GPIO0_NO_WAKEUP			0x08
 #define	DA9063_GPIO1_PIN_MASK			0x30
+#define	DA9063_GPIO1_PIN_MASK_SHIFT		4
 #define		DA9063_GPIO1_PIN_ADCIN2_COMP	0x00
 #define		DA9063_GPIO1_PIN_GPI		0x10
 #define		DA9063_GPIO1_PIN_GPO_OD		0x20
@@ -536,6 +538,7 @@
 
 /* DA9063_REG_GPIO_2_3 (addr=0x16) */
 #define	DA9063_GPIO2_PIN_MASK			0x03
+#define	DA9063_GPIO2_PIN_MASK_SHIFT		0
 #define		DA9063_GPIO2_PIN_ADCIN3		0x00
 #define		DA9063_GPIO2_PIN_GPI		0x01
 #define		DA9063_GPIO2_PIN_GPO_PSS	0x02
@@ -851,6 +854,7 @@
 #define	DA9063_VSYS_VAL_BASE			0x00
 
 /* DA9063_REG_ADC_RES_L (addr=0x37) */
+#define	DA9063_ADC_RES_L_SHIFT			6
 #define	DA9063_ADC_RES_L_BITS			2
 #define	DA9063_ADC_RES_L_MASK			0xC0
 
@@ -1014,6 +1018,36 @@
 #define DA9063_GPIO_DIM				0x80
 #define DA9063_GPIO_PWM_MASK			0x7F
 
+/* DA9063_REG_ADC_CFG (addr=0xC9) */
+#define DA9063_REG_ADCIN1_CUR_MASK		0x03
+#define DA9063_REG_ADCIN1_CUR_SHIFT		0
+#define		DA9063_ADCIN1_CUR_1UA		0x00
+#define		DA9063_ADCIN1_CUR_2UA		0x01
+#define		DA9063_ADCIN1_CUR_10UA		0x02
+#define		DA9063_ADCIN1_CUR_40UA		0x03
+#define DA9063_REG_ADCIN2_CUR_MASK		0x0C
+#define DA9063_REG_ADCIN2_CUR_SHIFT		2
+#define		DA9063_ADCIN2_CUR_1UA		0x00
+#define		DA9063_ADCIN2_CUR_2UA		0x01
+#define		DA9063_ADCIN2_CUR_10UA		0x02
+#define		DA9063_ADCIN2_CUR_40UA		0x03
+#define DA9063_REG_ADCIN3_CUR_MASK		0x10
+#define DA9063_REG_ADCIN3_CUR_SHIFT		4
+#define		DA9063_ADCIN3_CUR_10UA		0x00
+#define		DA9063_ADCIN3_CUR_40UA		0x01
+#define DA9063_REG_ADCIN1_DEB_MASK		0x20
+#define DA9063_REG_ADCIN1_DEB_SHIFT		5
+#define		DA9063_ADCIN1_DEB_OFF		0x00
+#define		DA9063_ADCIN1_DEB_ON		0x01
+#define DA9063_REG_ADCIN2_DEB_MASK		0x40
+#define DA9063_REG_ADCIN2_DEB_SHIFT		6
+#define		DA9063_ADCIN2_DEB_OFF		0x00
+#define		DA9063_ADCIN2_DEB_ON		0x01
+#define DA9063_REG_ADCIN3_DEB_MASK		0x80
+#define DA9063_REG_ADCIN3_DEB_SHIFT		7
+#define		DA9063_ADCIN3_DEB_OFF		0x00
+#define		DA9063_ADCIN3_DEB_ON		0x01
+
 /* DA9063_REG_CONFIG_H (addr=0x10D) */
 #define DA9063_PWM_CLK_MASK			0x01
 #define		DA9063_PWM_CLK_PWM2MHZ		0x00
-- 
2.32.0

[PATCH v3 2/3] hwmon: da9063: HWMON driver

[Vincent Pelletier]

Add the HWMON driver for DA9063

Originally-from: Opensource [Steve Twiss] <stwiss.opensource@diasemi.com>
Signed-off-by: Vincent Pelletier <plr.vincent@gmail.com>
---
Changes in v3:
- changed original author's Signed-off-by into an Originally-from.
- dropped license boilerplate
- only return ETIMEOUT if ADC result is not ready by the time the IRQ
  either triggered or timed out
- removed unnecessary lists
- changed a duplicate init_comptetion into a more useful reinit_completion
- dropped unused platform_set_drvdata
- moved temperature offset reading from mfd driver

Changes in v2:
- drop of_match_table: this should be meaningless in such sub-function
  driver (at least judging by other sub-function drivers for the da9063)
- sort includes
- switch to devm_hwmon_device_register_with_info
- registers.h changes moved to patch 1
- add SPDX header

This patch depends on patch 1/3.
Originally submitted by Steve Twiss in 2014:
  https://marc.info/?l=linux-kernel&m=139560868309857&w=2

 drivers/hwmon/Kconfig        |  10 ++
 drivers/hwmon/Makefile       |   1 +
 drivers/hwmon/da9063-hwmon.c | 260 +++++++++++++++++++++++++++++++++++
 3 files changed, 271 insertions(+)
 create mode 100644 drivers/hwmon/da9063-hwmon.c

diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 87624902ea80..17244cfaa855 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -515,6 +515,16 @@ config SENSORS_DA9055
 	  This driver can also be built as a module. If so, the module
 	  will be called da9055-hwmon.
 
+config SENSORS_DA9063
+	tristate "Dialog Semiconductor DA9063"
+	depends on MFD_DA9063
+	help
+	  If you say yes here you get support for the hardware
+	  monitoring features of the DA9063 Power Management IC.
+
+	  This driver can also be built as a module. If so, the module
+	  will be called da9063-hwmon.
+
 config SENSORS_I5K_AMB
 	tristate "FB-DIMM AMB temperature sensor on Intel 5000 series chipsets"
 	depends on PCI
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index 59e78bc212cf..6855711ed9ec 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -60,6 +60,7 @@ obj-$(CONFIG_SENSORS_CORSAIR_CPRO) += corsair-cpro.o
 obj-$(CONFIG_SENSORS_CORSAIR_PSU) += corsair-psu.o
 obj-$(CONFIG_SENSORS_DA9052_ADC)+= da9052-hwmon.o
 obj-$(CONFIG_SENSORS_DA9055)+= da9055-hwmon.o
+obj-$(CONFIG_SENSORS_DA9063)	+= da9063-hwmon.o
 obj-$(CONFIG_SENSORS_DELL_SMM)	+= dell-smm-hwmon.o
 obj-$(CONFIG_SENSORS_DME1737)	+= dme1737.o
 obj-$(CONFIG_SENSORS_DRIVETEMP)	+= drivetemp.o
diff --git a/drivers/hwmon/da9063-hwmon.c b/drivers/hwmon/da9063-hwmon.c
new file mode 100644
index 000000000000..6367685536a1
--- /dev/null
+++ b/drivers/hwmon/da9063-hwmon.c
@@ -0,0 +1,260 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/* da9063-hwmon.c - Hardware monitor support for DA9063
+ * Copyright (C) 2014 Dialog Semiconductor Ltd.
+ * Copyright (C) 2021 Vincent Pelletier <plr.vincent@gmail.com>
+ */
+
+#include <linux/delay.h>
+#include <linux/err.h>
+#include <linux/hwmon.h>
+#include <linux/hwmon-sysfs.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/mfd/da9063/core.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
+#include <linux/string.h>
+
+#define DA9063_ADC_RES	(1 << (DA9063_ADC_RES_L_BITS + DA9063_ADC_RES_M_BITS))
+#define DA9063_ADC_MAX	(DA9063_ADC_RES - 1)
+#define DA9063_2V5	2500
+#define DA9063_5V0	5000
+#define DA9063_5V5	5500
+#define DA9063_TJUNC_M	-398
+#define DA9063_TJUNC_O	330000
+#define DA9063_VBBAT_M	2048
+
+enum da9063_adc {
+	DA9063_CHAN_VSYS = DA9063_ADC_MUX_VSYS,
+	DA9063_CHAN_ADCIN1 = DA9063_ADC_MUX_ADCIN1,
+	DA9063_CHAN_ADCIN2 = DA9063_ADC_MUX_ADCIN2,
+	DA9063_CHAN_ADCIN3 = DA9063_ADC_MUX_ADCIN3,
+	DA9063_CHAN_TJUNC = DA9063_ADC_MUX_T_SENSE,
+	DA9063_CHAN_VBBAT = DA9063_ADC_MUX_VBBAT,
+	DA9063_CHAN_LDO_G1 = DA9063_ADC_MUX_LDO_G1,
+	DA9063_CHAN_LDO_G2 = DA9063_ADC_MUX_LDO_G2,
+	DA9063_CHAN_LDO_G3 = DA9063_ADC_MUX_LDO_G3
+};
+
+struct da9063_hwmon {
+	struct da9063 *da9063;
+	struct mutex hwmon_mutex;
+	struct completion adc_ready;
+	signed char tjunc_offset;
+};
+
+static int da9063_adc_manual_read(struct da9063_hwmon *hwmon, int channel)
+{
+	int ret;
+	unsigned char val;
+	unsigned char data[2];
+	int adc_man;
+
+	mutex_lock(&hwmon->hwmon_mutex);
+
+	val = (channel & DA9063_ADC_MUX_MASK) | DA9063_ADC_MAN;
+	ret = regmap_update_bits(hwmon->da9063->regmap, DA9063_REG_ADC_MAN,
+				 DA9063_ADC_MUX_MASK | DA9063_ADC_MAN, val);
+	if (ret < 0)
+		goto err_mread;
+
+	ret = wait_for_completion_timeout(&hwmon->adc_ready,
+					  msecs_to_jiffies(100));
+	reinit_completion(&hwmon->adc_ready);
+	if (ret == 0)
+		dev_dbg(hwmon->da9063->dev,
+			"Timeout while waiting for ADC completion IRQ\n");
+
+	ret = regmap_read(hwmon->da9063->regmap, DA9063_REG_ADC_MAN, &adc_man);
+	if (ret < 0)
+		goto err_mread;
+
+	/* data value is not ready */
+	if (adc_man & DA9063_ADC_MAN) {
+		ret = -ETIMEDOUT;
+		goto err_mread;
+	}
+
+	ret = regmap_bulk_read(hwmon->da9063->regmap,
+			       DA9063_REG_ADC_RES_L, data, 2);
+	if (ret < 0)
+		goto err_mread;
+
+	ret = (data[0] & DA9063_ADC_RES_L_MASK) >> DA9063_ADC_RES_L_SHIFT;
+	ret |= data[1] << DA9063_ADC_RES_L_BITS;
+err_mread:
+	mutex_unlock(&hwmon->hwmon_mutex);
+	return ret;
+}
+
+static irqreturn_t da9063_hwmon_irq_handler(int irq, void *irq_data)
+{
+	struct da9063_hwmon *hwmon = irq_data;
+
+	complete(&hwmon->adc_ready);
+	return IRQ_HANDLED;
+}
+
+static umode_t da9063_is_visible(const void *drvdata, enum
+				 hwmon_sensor_types type, u32 attr, int channel)
+{
+	return 0444;
+}
+
+static const enum da9063_adc da9063_in_index[] = {
+	DA9063_CHAN_VSYS, DA9063_CHAN_VBBAT
+};
+
+static int da9063_read(struct device *dev, enum hwmon_sensor_types type,
+		       u32 attr, int channel, long *val)
+{
+	struct da9063_hwmon *hwmon = dev_get_drvdata(dev);
+	enum da9063_adc adc_channel;
+	int tmp;
+
+	switch (type) {
+	case hwmon_in:
+		if (attr != hwmon_in_input)
+			return -EOPNOTSUPP;
+		adc_channel = da9063_in_index[channel];
+		break;
+	case hwmon_temp:
+		if (attr != hwmon_temp_input)
+			return -EOPNOTSUPP;
+		adc_channel = DA9063_CHAN_TJUNC;
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	tmp = da9063_adc_manual_read(hwmon, adc_channel);
+	if (tmp < 0)
+		return tmp;
+
+	switch (adc_channel) {
+	case DA9063_CHAN_VSYS:
+		*val = ((DA9063_5V5 - DA9063_2V5) * tmp) / DA9063_ADC_MAX +
+			DA9063_2V5;
+		break;
+	case DA9063_CHAN_TJUNC:
+		tmp -= hwmon->tjunc_offset;
+		*val = DA9063_TJUNC_M * tmp + DA9063_TJUNC_O;
+		break;
+	case DA9063_CHAN_VBBAT:
+		*val = (DA9063_5V0 * tmp) / DA9063_ADC_MAX;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static const char * const da9063_in_name[] = {
+	"VSYS", "VBBAT"
+};
+
+static int da9063_read_string(struct device *dev, enum hwmon_sensor_types type,
+			      u32 attr, int channel, const char **str)
+{
+	switch (type) {
+	case hwmon_in:
+		if (attr != hwmon_in_label)
+			return -EOPNOTSUPP;
+		*str = da9063_in_name[channel];
+		break;
+	case hwmon_temp:
+		if (attr != hwmon_temp_label)
+			return -EOPNOTSUPP;
+		*str = "TJUNC";
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	return 0;
+}
+
+static const struct hwmon_ops da9063_ops = {
+	.is_visible = da9063_is_visible,
+	.read = da9063_read,
+	.read_string = da9063_read_string,
+};
+
+static const struct hwmon_channel_info *da9063_channel_info[] = {
+	HWMON_CHANNEL_INFO(chip, HWMON_C_REGISTER_TZ),
+	HWMON_CHANNEL_INFO(in,
+			   HWMON_I_INPUT | HWMON_I_LABEL,
+			   HWMON_I_INPUT | HWMON_I_LABEL),
+	HWMON_CHANNEL_INFO(temp,
+			   HWMON_T_INPUT | HWMON_T_LABEL),
+	NULL
+};
+
+static const struct hwmon_chip_info da9063_chip_info = {
+	.ops = &da9063_ops,
+	.info = da9063_channel_info,
+};
+
+static int da9063_hwmon_probe(struct platform_device *pdev)
+{
+	struct da9063 *da9063 = dev_get_drvdata(pdev->dev.parent);
+	struct da9063_hwmon *hwmon;
+	struct device *hwmon_dev;
+	unsigned int tmp;
+	int irq;
+	int ret;
+
+	hwmon = devm_kzalloc(&pdev->dev, sizeof(struct da9063_hwmon),
+			     GFP_KERNEL);
+	if (!hwmon)
+		return -ENOMEM;
+
+	mutex_init(&hwmon->hwmon_mutex);
+	init_completion(&hwmon->adc_ready);
+	hwmon->da9063 = da9063;
+
+	irq = platform_get_irq_byname(pdev, DA9063_DRVNAME_HWMON);
+	if (irq < 0)
+		return irq;
+
+	ret = devm_request_threaded_irq(&pdev->dev, irq, NULL,
+					da9063_hwmon_irq_handler,
+					IRQF_TRIGGER_LOW | IRQF_ONESHOT,
+					"HWMON", hwmon);
+	if (ret) {
+		dev_err(&pdev->dev, "Failed to request IRQ.\n");
+		return ret;
+	}
+
+	ret = regmap_read(da9063->regmap, DA9063_REG_T_OFFSET, &tmp);
+	if (ret < 0) {
+		tmp = 0;
+		dev_warn(&pdev->dev,
+			 "Temperature trimming value cannot be read (defaulting to 0)\n");
+	}
+	hwmon->tjunc_offset = (signed char) tmp;
+
+	hwmon_dev = devm_hwmon_device_register_with_info(&pdev->dev, "da9063",
+							 hwmon,
+							 &da9063_chip_info,
+							 NULL);
+
+	return PTR_ERR_OR_ZERO(hwmon_dev);
+}
+
+static struct platform_driver da9063_hwmon_driver = {
+	.probe = da9063_hwmon_probe,
+	.driver = {
+		.name = DA9063_DRVNAME_HWMON,
+	},
+};
+module_platform_driver(da9063_hwmon_driver);
+
+MODULE_DESCRIPTION("Hardware monitor support device driver for Dialog DA9063");
+MODULE_AUTHOR("Vincent Pelletier <plr.vincent@gmail.com>");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:" DA9063_DRVNAME_HWMON);
-- 
2.32.0

Re: [PATCH v3 2/3] hwmon: da9063: HWMON driver

[Guenter Roeck]

On Wed, Jul 07, 2021 at 01:25:03PM +0000, Vincent Pelletier wrote:
> Add the HWMON driver for DA9063
> 
> Originally-from: Opensource [Steve Twiss] <stwiss.opensource@diasemi.com>
> Signed-off-by: Vincent Pelletier <plr.vincent@gmail.com>
> ---
> Changes in v3:
> - changed original author's Signed-off-by into an Originally-from.
> - dropped license boilerplate
> - only return ETIMEOUT if ADC result is not ready by the time the IRQ
>   either triggered or timed out
> - removed unnecessary lists
> - changed a duplicate init_comptetion into a more useful reinit_completion
> - dropped unused platform_set_drvdata
> - moved temperature offset reading from mfd driver
> 
> Changes in v2:
> - drop of_match_table: this should be meaningless in such sub-function
>   driver (at least judging by other sub-function drivers for the da9063)
> - sort includes
> - switch to devm_hwmon_device_register_with_info
> - registers.h changes moved to patch 1
> - add SPDX header
> 
> This patch depends on patch 1/3.
> Originally submitted by Steve Twiss in 2014:
>   https://marc.info/?l=linux-kernel&m=139560868309857&w=2
> 
>  drivers/hwmon/Kconfig        |  10 ++
>  drivers/hwmon/Makefile       |   1 +
>  drivers/hwmon/da9063-hwmon.c | 260 +++++++++++++++++++++++++++++++++++
>  3 files changed, 271 insertions(+)
>  create mode 100644 drivers/hwmon/da9063-hwmon.c
> 
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 87624902ea80..17244cfaa855 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -515,6 +515,16 @@ config SENSORS_DA9055
>  	  This driver can also be built as a module. If so, the module
>  	  will be called da9055-hwmon.
>  
> +config SENSORS_DA9063
> +	tristate "Dialog Semiconductor DA9063"
> +	depends on MFD_DA9063
> +	help
> +	  If you say yes here you get support for the hardware
> +	  monitoring features of the DA9063 Power Management IC.
> +
> +	  This driver can also be built as a module. If so, the module
> +	  will be called da9063-hwmon.
> +
>  config SENSORS_I5K_AMB
>  	tristate "FB-DIMM AMB temperature sensor on Intel 5000 series chipsets"
>  	depends on PCI
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index 59e78bc212cf..6855711ed9ec 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -60,6 +60,7 @@ obj-$(CONFIG_SENSORS_CORSAIR_CPRO) += corsair-cpro.o
>  obj-$(CONFIG_SENSORS_CORSAIR_PSU) += corsair-psu.o
>  obj-$(CONFIG_SENSORS_DA9052_ADC)+= da9052-hwmon.o
>  obj-$(CONFIG_SENSORS_DA9055)+= da9055-hwmon.o
> +obj-$(CONFIG_SENSORS_DA9063)	+= da9063-hwmon.o
>  obj-$(CONFIG_SENSORS_DELL_SMM)	+= dell-smm-hwmon.o
>  obj-$(CONFIG_SENSORS_DME1737)	+= dme1737.o
>  obj-$(CONFIG_SENSORS_DRIVETEMP)	+= drivetemp.o
> diff --git a/drivers/hwmon/da9063-hwmon.c b/drivers/hwmon/da9063-hwmon.c
> new file mode 100644
> index 000000000000..6367685536a1
> --- /dev/null
> +++ b/drivers/hwmon/da9063-hwmon.c
> @@ -0,0 +1,260 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/* da9063-hwmon.c - Hardware monitor support for DA9063
> + * Copyright (C) 2014 Dialog Semiconductor Ltd.
> + * Copyright (C) 2021 Vincent Pelletier <plr.vincent@gmail.com>
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/hwmon.h>
> +#include <linux/hwmon-sysfs.h>

Unnecessary include.

> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/mfd/da9063/core.h>
> +#include <linux/mod_devicetable.h>

I don't immediately see where this include is needed. Is this a
leftover ?

> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/slab.h>
> +#include <linux/string.h>

Same here.

> +
> +#define DA9063_ADC_RES	(1 << (DA9063_ADC_RES_L_BITS + DA9063_ADC_RES_M_BITS))
> +#define DA9063_ADC_MAX	(DA9063_ADC_RES - 1)
> +#define DA9063_2V5	2500
> +#define DA9063_5V0	5000
> +#define DA9063_5V5	5500
> +#define DA9063_TJUNC_M	-398

It doesn't matter here (I think), but it would be better to surround the
above with () to ensure that the '-' is not interpreted as arithmetic
operator.

> +#define DA9063_TJUNC_O	330000
> +#define DA9063_VBBAT_M	2048
> +
> +enum da9063_adc {
> +	DA9063_CHAN_VSYS = DA9063_ADC_MUX_VSYS,
> +	DA9063_CHAN_ADCIN1 = DA9063_ADC_MUX_ADCIN1,
> +	DA9063_CHAN_ADCIN2 = DA9063_ADC_MUX_ADCIN2,
> +	DA9063_CHAN_ADCIN3 = DA9063_ADC_MUX_ADCIN3,
> +	DA9063_CHAN_TJUNC = DA9063_ADC_MUX_T_SENSE,
> +	DA9063_CHAN_VBBAT = DA9063_ADC_MUX_VBBAT,
> +	DA9063_CHAN_LDO_G1 = DA9063_ADC_MUX_LDO_G1,
> +	DA9063_CHAN_LDO_G2 = DA9063_ADC_MUX_LDO_G2,
> +	DA9063_CHAN_LDO_G3 = DA9063_ADC_MUX_LDO_G3

Many of the above defines are not used. Do you plan a follow-up commit
to use them ? Otherwise please drop unused defines.

> +};
> +
> +struct da9063_hwmon {
> +	struct da9063 *da9063;
> +	struct mutex hwmon_mutex;
> +	struct completion adc_ready;
> +	signed char tjunc_offset;

I am curious: 'char' implies 'signed'. Any reason for using 'signed' ?

Also, note that on most architectures the resulting code is more complex
when using 'char' instead of 'int'. This is seen easily by compiling the
driver for arm64: Replacing the above 'signed char' with 'int' reduces
code size by 32 bytes.

> +};
> +
> +static int da9063_adc_manual_read(struct da9063_hwmon *hwmon, int channel)
> +{
> +	int ret;
> +	unsigned char val;
> +	unsigned char data[2];
> +	int adc_man;

Should this be unsigned int ?

> +
> +	mutex_lock(&hwmon->hwmon_mutex);
> +
> +	val = (channel & DA9063_ADC_MUX_MASK) | DA9063_ADC_MAN;
> +	ret = regmap_update_bits(hwmon->da9063->regmap, DA9063_REG_ADC_MAN,
> +				 DA9063_ADC_MUX_MASK | DA9063_ADC_MAN, val);
> +	if (ret < 0)
> +		goto err_mread;
> +
> +	ret = wait_for_completion_timeout(&hwmon->adc_ready,
> +					  msecs_to_jiffies(100));
> +	reinit_completion(&hwmon->adc_ready);

This is unusual. Normally I see init_completion() or reinit_completion()
ahead of calls to wait functions.

If a request timed out and an interrupt happened after the timeout,
the next request would return immediately with the previous result,
since complete() would be called on the re-initialized completion
handler. That doesn't seem to be correct to me.

> +	if (ret == 0)
> +		dev_dbg(hwmon->da9063->dev,
> +			"Timeout while waiting for ADC completion IRQ\n");
> +
> +	ret = regmap_read(hwmon->da9063->regmap, DA9063_REG_ADC_MAN, &adc_man);
> +	if (ret < 0)
> +		goto err_mread;
> +
> +	/* data value is not ready */
> +	if (adc_man & DA9063_ADC_MAN) {
> +		ret = -ETIMEDOUT;
> +		goto err_mread;
> +	}
> +
> +	ret = regmap_bulk_read(hwmon->da9063->regmap,
> +			       DA9063_REG_ADC_RES_L, data, 2);
> +	if (ret < 0)
> +		goto err_mread;
> +
> +	ret = (data[0] & DA9063_ADC_RES_L_MASK) >> DA9063_ADC_RES_L_SHIFT;
> +	ret |= data[1] << DA9063_ADC_RES_L_BITS;
> +err_mread:
> +	mutex_unlock(&hwmon->hwmon_mutex);
> +	return ret;
> +}
> +
> +static irqreturn_t da9063_hwmon_irq_handler(int irq, void *irq_data)
> +{
> +	struct da9063_hwmon *hwmon = irq_data;
> +
> +	complete(&hwmon->adc_ready);
> +	return IRQ_HANDLED;
> +}
> +
> +static umode_t da9063_is_visible(const void *drvdata, enum
> +				 hwmon_sensor_types type, u32 attr, int channel)
> +{
> +	return 0444;
> +}
> +
> +static const enum da9063_adc da9063_in_index[] = {
> +	DA9063_CHAN_VSYS, DA9063_CHAN_VBBAT
> +};
> +
> +static int da9063_read(struct device *dev, enum hwmon_sensor_types type,
> +		       u32 attr, int channel, long *val)
> +{
> +	struct da9063_hwmon *hwmon = dev_get_drvdata(dev);
> +	enum da9063_adc adc_channel;
> +	int tmp;
> +
> +	switch (type) {
> +	case hwmon_in:
> +		if (attr != hwmon_in_input)
> +			return -EOPNOTSUPP;
> +		adc_channel = da9063_in_index[channel];
> +		break;
> +	case hwmon_temp:
> +		if (attr != hwmon_temp_input)
> +			return -EOPNOTSUPP;
> +		adc_channel = DA9063_CHAN_TJUNC;
> +		break;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +
> +	tmp = da9063_adc_manual_read(hwmon, adc_channel);
> +	if (tmp < 0)
> +		return tmp;
> +
> +	switch (adc_channel) {
> +	case DA9063_CHAN_VSYS:
> +		*val = ((DA9063_5V5 - DA9063_2V5) * tmp) / DA9063_ADC_MAX +
> +			DA9063_2V5;
> +		break;
> +	case DA9063_CHAN_TJUNC:
> +		tmp -= hwmon->tjunc_offset;
> +		*val = DA9063_TJUNC_M * tmp + DA9063_TJUNC_O;
> +		break;
> +	case DA9063_CHAN_VBBAT:
> +		*val = (DA9063_5V0 * tmp) / DA9063_ADC_MAX;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static const char * const da9063_in_name[] = {
> +	"VSYS", "VBBAT"
> +};
> +
> +static int da9063_read_string(struct device *dev, enum hwmon_sensor_types type,
> +			      u32 attr, int channel, const char **str)
> +{
> +	switch (type) {
> +	case hwmon_in:
> +		if (attr != hwmon_in_label)
> +			return -EOPNOTSUPP;
> +		*str = da9063_in_name[channel];
> +		break;
> +	case hwmon_temp:
> +		if (attr != hwmon_temp_label)
> +			return -EOPNOTSUPP;
> +		*str = "TJUNC";
> +		break;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct hwmon_ops da9063_ops = {
> +	.is_visible = da9063_is_visible,
> +	.read = da9063_read,
> +	.read_string = da9063_read_string,
> +};
> +
> +static const struct hwmon_channel_info *da9063_channel_info[] = {
> +	HWMON_CHANNEL_INFO(chip, HWMON_C_REGISTER_TZ),
> +	HWMON_CHANNEL_INFO(in,
> +			   HWMON_I_INPUT | HWMON_I_LABEL,
> +			   HWMON_I_INPUT | HWMON_I_LABEL),
> +	HWMON_CHANNEL_INFO(temp,
> +			   HWMON_T_INPUT | HWMON_T_LABEL),
> +	NULL
> +};
> +
> +static const struct hwmon_chip_info da9063_chip_info = {
> +	.ops = &da9063_ops,
> +	.info = da9063_channel_info,
> +};
> +
> +static int da9063_hwmon_probe(struct platform_device *pdev)
> +{
> +	struct da9063 *da9063 = dev_get_drvdata(pdev->dev.parent);
> +	struct da9063_hwmon *hwmon;
> +	struct device *hwmon_dev;
> +	unsigned int tmp;
> +	int irq;
> +	int ret;
> +
> +	hwmon = devm_kzalloc(&pdev->dev, sizeof(struct da9063_hwmon),
> +			     GFP_KERNEL);
> +	if (!hwmon)
> +		return -ENOMEM;
> +
> +	mutex_init(&hwmon->hwmon_mutex);
> +	init_completion(&hwmon->adc_ready);
> +	hwmon->da9063 = da9063;
> +
> +	irq = platform_get_irq_byname(pdev, DA9063_DRVNAME_HWMON);
> +	if (irq < 0)
> +		return irq;
> +
> +	ret = devm_request_threaded_irq(&pdev->dev, irq, NULL,
> +					da9063_hwmon_irq_handler,
> +					IRQF_TRIGGER_LOW | IRQF_ONESHOT,
> +					"HWMON", hwmon);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Failed to request IRQ.\n");
> +		return ret;
> +	}
> +
> +	ret = regmap_read(da9063->regmap, DA9063_REG_T_OFFSET, &tmp);
> +	if (ret < 0) {
> +		tmp = 0;
> +		dev_warn(&pdev->dev,
> +			 "Temperature trimming value cannot be read (defaulting to 0)\n");
> +	}
> +	hwmon->tjunc_offset = (signed char) tmp;

Nit: Unnecessary space after typecast (checkpatch --strict would tell you).

Also, I am curious: The temperature offset is a standard hwmon attribute.
Is it an oversight to not report it, or is it on purpose ?

> +
> +	hwmon_dev = devm_hwmon_device_register_with_info(&pdev->dev, "da9063",
> +							 hwmon,
> +							 &da9063_chip_info,
> +							 NULL);
> +
> +	return PTR_ERR_OR_ZERO(hwmon_dev);
> +}
> +
> +static struct platform_driver da9063_hwmon_driver = {
> +	.probe = da9063_hwmon_probe,
> +	.driver = {
> +		.name = DA9063_DRVNAME_HWMON,
> +	},
> +};
> +module_platform_driver(da9063_hwmon_driver);
> +
> +MODULE_DESCRIPTION("Hardware monitor support device driver for Dialog DA9063");
> +MODULE_AUTHOR("Vincent Pelletier <plr.vincent@gmail.com>");
> +MODULE_LICENSE("GPL v2");
> +MODULE_ALIAS("platform:" DA9063_DRVNAME_HWMON);

Re: [PATCH v3 2/3] hwmon: da9063: HWMON driver

[Vincent Pelletier]

Thanks a 

On Sat, 10 Jul 2021 09:08:13 -0700, Guenter Roeck <linux@roeck-us.net> wrote:
> On Wed, Jul 07, 2021 at 01:25:03PM +0000, Vincent Pelletier wrote:
> > Add the HWMON driver for DA9063
> > 
> > Originally-from: Opensource [Steve Twiss] <stwiss.opensource@diasemi.com>
> > Signed-off-by: Vincent Pelletier <plr.vincent@gmail.com>
> > ---
> > Changes in v3:
> > - changed original author's Signed-off-by into an Originally-from.
> > - dropped license boilerplate
> > - only return ETIMEOUT if ADC result is not ready by the time the IRQ
> >   either triggered or timed out
> > - removed unnecessary lists
> > - changed a duplicate init_comptetion into a more useful reinit_completion
> > - dropped unused platform_set_drvdata
> > - moved temperature offset reading from mfd driver
> > 
> > Changes in v2:
> > - drop of_match_table: this should be meaningless in such sub-function
> >   driver (at least judging by other sub-function drivers for the da9063)
> > - sort includes
> > - switch to devm_hwmon_device_register_with_info
> > - registers.h changes moved to patch 1
> > - add SPDX header
> > 
> > This patch depends on patch 1/3.
> > Originally submitted by Steve Twiss in 2014:
> >   https://marc.info/?l=linux-kernel&m=139560868309857&w=2
> > 
> >  drivers/hwmon/Kconfig        |  10 ++
> >  drivers/hwmon/Makefile       |   1 +
> >  drivers/hwmon/da9063-hwmon.c | 260 +++++++++++++++++++++++++++++++++++
> >  3 files changed, 271 insertions(+)
> >  create mode 100644 drivers/hwmon/da9063-hwmon.c
> > 
> > diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> > index 87624902ea80..17244cfaa855 100644
> > --- a/drivers/hwmon/Kconfig
> > +++ b/drivers/hwmon/Kconfig
> > @@ -515,6 +515,16 @@ config SENSORS_DA9055
> >  	  This driver can also be built as a module. If so, the module
> >  	  will be called da9055-hwmon.
> >  
> > +config SENSORS_DA9063
> > +	tristate "Dialog Semiconductor DA9063"
> > +	depends on MFD_DA9063
> > +	help
> > +	  If you say yes here you get support for the hardware
> > +	  monitoring features of the DA9063 Power Management IC.
> > +
> > +	  This driver can also be built as a module. If so, the module
> > +	  will be called da9063-hwmon.
> > +
> >  config SENSORS_I5K_AMB
> >  	tristate "FB-DIMM AMB temperature sensor on Intel 5000 series chipsets"
> >  	depends on PCI
> > diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> > index 59e78bc212cf..6855711ed9ec 100644
> > --- a/drivers/hwmon/Makefile
> > +++ b/drivers/hwmon/Makefile
> > @@ -60,6 +60,7 @@ obj-$(CONFIG_SENSORS_CORSAIR_CPRO) += corsair-cpro.o
> >  obj-$(CONFIG_SENSORS_CORSAIR_PSU) += corsair-psu.o
> >  obj-$(CONFIG_SENSORS_DA9052_ADC)+= da9052-hwmon.o
> >  obj-$(CONFIG_SENSORS_DA9055)+= da9055-hwmon.o
> > +obj-$(CONFIG_SENSORS_DA9063)	+= da9063-hwmon.o
> >  obj-$(CONFIG_SENSORS_DELL_SMM)	+= dell-smm-hwmon.o
> >  obj-$(CONFIG_SENSORS_DME1737)	+= dme1737.o
> >  obj-$(CONFIG_SENSORS_DRIVETEMP)	+= drivetemp.o
> > diff --git a/drivers/hwmon/da9063-hwmon.c b/drivers/hwmon/da9063-hwmon.c
> > new file mode 100644
> > index 000000000000..6367685536a1
> > --- /dev/null
> > +++ b/drivers/hwmon/da9063-hwmon.c
> > @@ -0,0 +1,260 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/* da9063-hwmon.c - Hardware monitor support for DA9063
> > + * Copyright (C) 2014 Dialog Semiconductor Ltd.
> > + * Copyright (C) 2021 Vincent Pelletier <plr.vincent@gmail.com>
> > + */
> > +
> > +#include <linux/delay.h>
> > +#include <linux/err.h>
> > +#include <linux/hwmon.h>
> > +#include <linux/hwmon-sysfs.h>  
> 
> Unnecessary include.
> 
> > +#include <linux/init.h>
> > +#include <linux/kernel.h>
> > +#include <linux/mfd/da9063/core.h>
> > +#include <linux/mod_devicetable.h>  
> 
> I don't immediately see where this include is needed. Is this a
> leftover ?
> 
> > +#include <linux/module.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/regmap.h>
> > +#include <linux/slab.h>
> > +#include <linux/string.h>  
> 
> Same here.
> 
> > +
> > +#define DA9063_ADC_RES	(1 << (DA9063_ADC_RES_L_BITS + DA9063_ADC_RES_M_BITS))
> > +#define DA9063_ADC_MAX	(DA9063_ADC_RES - 1)
> > +#define DA9063_2V5	2500
> > +#define DA9063_5V0	5000
> > +#define DA9063_5V5	5500
> > +#define DA9063_TJUNC_M	-398  
> 
> It doesn't matter here (I think), but it would be better to surround the
> above with () to ensure that the '-' is not interpreted as arithmetic
> operator.
> 
> > +#define DA9063_TJUNC_O	330000
> > +#define DA9063_VBBAT_M	2048
> > +
> > +enum da9063_adc {
> > +	DA9063_CHAN_VSYS = DA9063_ADC_MUX_VSYS,
> > +	DA9063_CHAN_ADCIN1 = DA9063_ADC_MUX_ADCIN1,
> > +	DA9063_CHAN_ADCIN2 = DA9063_ADC_MUX_ADCIN2,
> > +	DA9063_CHAN_ADCIN3 = DA9063_ADC_MUX_ADCIN3,
> > +	DA9063_CHAN_TJUNC = DA9063_ADC_MUX_T_SENSE,
> > +	DA9063_CHAN_VBBAT = DA9063_ADC_MUX_VBBAT,
> > +	DA9063_CHAN_LDO_G1 = DA9063_ADC_MUX_LDO_G1,
> > +	DA9063_CHAN_LDO_G2 = DA9063_ADC_MUX_LDO_G2,
> > +	DA9063_CHAN_LDO_G3 = DA9063_ADC_MUX_LDO_G3  
> 
> Many of the above defines are not used. Do you plan a follow-up commit
> to use them ? Otherwise please drop unused defines.
> 
> > +};
> > +
> > +struct da9063_hwmon {
> > +	struct da9063 *da9063;
> > +	struct mutex hwmon_mutex;
> > +	struct completion adc_ready;
> > +	signed char tjunc_offset;  
> 
> I am curious: 'char' implies 'signed'. Any reason for using 'signed' ?
> 
> Also, note that on most architectures the resulting code is more complex
> when using 'char' instead of 'int'. This is seen easily by compiling the
> driver for arm64: Replacing the above 'signed char' with 'int' reduces
> code size by 32 bytes.
> 
> > +};
> > +
> > +static int da9063_adc_manual_read(struct da9063_hwmon *hwmon, int channel)
> > +{
> > +	int ret;
> > +	unsigned char val;
> > +	unsigned char data[2];
> > +	int adc_man;  
> 
> Should this be unsigned int ?
> 
> > +
> > +	mutex_lock(&hwmon->hwmon_mutex);
> > +
> > +	val = (channel & DA9063_ADC_MUX_MASK) | DA9063_ADC_MAN;
> > +	ret = regmap_update_bits(hwmon->da9063->regmap, DA9063_REG_ADC_MAN,
> > +				 DA9063_ADC_MUX_MASK | DA9063_ADC_MAN, val);
> > +	if (ret < 0)
> > +		goto err_mread;
> > +
> > +	ret = wait_for_completion_timeout(&hwmon->adc_ready,
> > +					  msecs_to_jiffies(100));
> > +	reinit_completion(&hwmon->adc_ready);  
> 
> This is unusual. Normally I see init_completion() or reinit_completion()
> ahead of calls to wait functions.
> 
> If a request timed out and an interrupt happened after the timeout,
> the next request would return immediately with the previous result,
> since complete() would be called on the re-initialized completion
> handler. That doesn't seem to be correct to me.
> 
> > +	if (ret == 0)
> > +		dev_dbg(hwmon->da9063->dev,
> > +			"Timeout while waiting for ADC completion IRQ\n");
> > +
> > +	ret = regmap_read(hwmon->da9063->regmap, DA9063_REG_ADC_MAN, &adc_man);
> > +	if (ret < 0)
> > +		goto err_mread;
> > +
> > +	/* data value is not ready */
> > +	if (adc_man & DA9063_ADC_MAN) {
> > +		ret = -ETIMEDOUT;
> > +		goto err_mread;
> > +	}
> > +
> > +	ret = regmap_bulk_read(hwmon->da9063->regmap,
> > +			       DA9063_REG_ADC_RES_L, data, 2);
> > +	if (ret < 0)
> > +		goto err_mread;
> > +
> > +	ret = (data[0] & DA9063_ADC_RES_L_MASK) >> DA9063_ADC_RES_L_SHIFT;
> > +	ret |= data[1] << DA9063_ADC_RES_L_BITS;
> > +err_mread:
> > +	mutex_unlock(&hwmon->hwmon_mutex);
> > +	return ret;
> > +}
> > +
> > +static irqreturn_t da9063_hwmon_irq_handler(int irq, void *irq_data)
> > +{
> > +	struct da9063_hwmon *hwmon = irq_data;
> > +
> > +	complete(&hwmon->adc_ready);
> > +	return IRQ_HANDLED;
> > +}
> > +
> > +static umode_t da9063_is_visible(const void *drvdata, enum
> > +				 hwmon_sensor_types type, u32 attr, int channel)
> > +{
> > +	return 0444;
> > +}
> > +
> > +static const enum da9063_adc da9063_in_index[] = {
> > +	DA9063_CHAN_VSYS, DA9063_CHAN_VBBAT
> > +};
> > +
> > +static int da9063_read(struct device *dev, enum hwmon_sensor_types type,
> > +		       u32 attr, int channel, long *val)
> > +{
> > +	struct da9063_hwmon *hwmon = dev_get_drvdata(dev);
> > +	enum da9063_adc adc_channel;
> > +	int tmp;
> > +
> > +	switch (type) {
> > +	case hwmon_in:
> > +		if (attr != hwmon_in_input)
> > +			return -EOPNOTSUPP;
> > +		adc_channel = da9063_in_index[channel];
> > +		break;
> > +	case hwmon_temp:
> > +		if (attr != hwmon_temp_input)
> > +			return -EOPNOTSUPP;
> > +		adc_channel = DA9063_CHAN_TJUNC;
> > +		break;
> > +	default:
> > +		return -EOPNOTSUPP;
> > +	}
> > +
> > +	tmp = da9063_adc_manual_read(hwmon, adc_channel);
> > +	if (tmp < 0)
> > +		return tmp;
> > +
> > +	switch (adc_channel) {
> > +	case DA9063_CHAN_VSYS:
> > +		*val = ((DA9063_5V5 - DA9063_2V5) * tmp) / DA9063_ADC_MAX +
> > +			DA9063_2V5;
> > +		break;
> > +	case DA9063_CHAN_TJUNC:
> > +		tmp -= hwmon->tjunc_offset;
> > +		*val = DA9063_TJUNC_M * tmp + DA9063_TJUNC_O;
> > +		break;
> > +	case DA9063_CHAN_VBBAT:
> > +		*val = (DA9063_5V0 * tmp) / DA9063_ADC_MAX;
> > +		break;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static const char * const da9063_in_name[] = {
> > +	"VSYS", "VBBAT"
> > +};
> > +
> > +static int da9063_read_string(struct device *dev, enum hwmon_sensor_types type,
> > +			      u32 attr, int channel, const char **str)
> > +{
> > +	switch (type) {
> > +	case hwmon_in:
> > +		if (attr != hwmon_in_label)
> > +			return -EOPNOTSUPP;
> > +		*str = da9063_in_name[channel];
> > +		break;
> > +	case hwmon_temp:
> > +		if (attr != hwmon_temp_label)
> > +			return -EOPNOTSUPP;
> > +		*str = "TJUNC";
> > +		break;
> > +	default:
> > +		return -EOPNOTSUPP;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct hwmon_ops da9063_ops = {
> > +	.is_visible = da9063_is_visible,
> > +	.read = da9063_read,
> > +	.read_string = da9063_read_string,
> > +};
> > +
> > +static const struct hwmon_channel_info *da9063_channel_info[] = {
> > +	HWMON_CHANNEL_INFO(chip, HWMON_C_REGISTER_TZ),
> > +	HWMON_CHANNEL_INFO(in,
> > +			   HWMON_I_INPUT | HWMON_I_LABEL,
> > +			   HWMON_I_INPUT | HWMON_I_LABEL),
> > +	HWMON_CHANNEL_INFO(temp,
> > +			   HWMON_T_INPUT | HWMON_T_LABEL),
> > +	NULL
> > +};
> > +
> > +static const struct hwmon_chip_info da9063_chip_info = {
> > +	.ops = &da9063_ops,
> > +	.info = da9063_channel_info,
> > +};
> > +
> > +static int da9063_hwmon_probe(struct platform_device *pdev)
> > +{
> > +	struct da9063 *da9063 = dev_get_drvdata(pdev->dev.parent);
> > +	struct da9063_hwmon *hwmon;
> > +	struct device *hwmon_dev;
> > +	unsigned int tmp;
> > +	int irq;
> > +	int ret;
> > +
> > +	hwmon = devm_kzalloc(&pdev->dev, sizeof(struct da9063_hwmon),
> > +			     GFP_KERNEL);
> > +	if (!hwmon)
> > +		return -ENOMEM;
> > +
> > +	mutex_init(&hwmon->hwmon_mutex);
> > +	init_completion(&hwmon->adc_ready);
> > +	hwmon->da9063 = da9063;
> > +
> > +	irq = platform_get_irq_byname(pdev, DA9063_DRVNAME_HWMON);
> > +	if (irq < 0)
> > +		return irq;
> > +
> > +	ret = devm_request_threaded_irq(&pdev->dev, irq, NULL,
> > +					da9063_hwmon_irq_handler,
> > +					IRQF_TRIGGER_LOW | IRQF_ONESHOT,
> > +					"HWMON", hwmon);
> > +	if (ret) {
> > +		dev_err(&pdev->dev, "Failed to request IRQ.\n");
> > +		return ret;
> > +	}
> > +
> > +	ret = regmap_read(da9063->regmap, DA9063_REG_T_OFFSET, &tmp);
> > +	if (ret < 0) {
> > +		tmp = 0;
> > +		dev_warn(&pdev->dev,
> > +			 "Temperature trimming value cannot be read (defaulting to 0)\n");
> > +	}
> > +	hwmon->tjunc_offset = (signed char) tmp;  
> 
> Nit: Unnecessary space after typecast (checkpatch --strict would tell you).
> 
> Also, I am curious: The temperature offset is a standard hwmon attribute.
> Is it an oversight to not report it, or is it on purpose ?
> 
> > +
> > +	hwmon_dev = devm_hwmon_device_register_with_info(&pdev->dev, "da9063",
> > +							 hwmon,
> > +							 &da9063_chip_info,
> > +							 NULL);
> > +
> > +	return PTR_ERR_OR_ZERO(hwmon_dev);
> > +}
> > +
> > +static struct platform_driver da9063_hwmon_driver = {
> > +	.probe = da9063_hwmon_probe,
> > +	.driver = {
> > +		.name = DA9063_DRVNAME_HWMON,
> > +	},
> > +};
> > +module_platform_driver(da9063_hwmon_driver);
> > +
> > +MODULE_DESCRIPTION("Hardware monitor support device driver for Dialog DA9063");
> > +MODULE_AUTHOR("Vincent Pelletier <plr.vincent@gmail.com>");
> > +MODULE_LICENSE("GPL v2");
> > +MODULE_ALIAS("platform:" DA9063_DRVNAME_HWMON);  




-- 
Vincent Pelletier
GPG fingerprint 983A E8B7 3B91 1598 7A92 3845 CAC9 3691 4257 B0C1

Re: [PATCH v3 2/3] hwmon: da9063: HWMON driver

[Vincent Pelletier]

Hello,

Thanks a lot for this new review (and sorry for the previous
very-incomplete send, unfortunate keyboard shortcut and sleepy fingers).

On Sat, 10 Jul 2021 09:08:13 -0700, Guenter Roeck <linux@roeck-us.net> wrote:
> Unnecessary include.
[...]
> I don't immediately see where this include is needed. Is this a
> leftover ?
[...]
> Same here.

Are there ways to systematically tell which includes are useless
besides commenting them out all and uncommenting until it compiles ?
(if that is even a good idea)

> > +enum da9063_adc {
> > +	DA9063_CHAN_VSYS = DA9063_ADC_MUX_VSYS,
> > +	DA9063_CHAN_ADCIN1 = DA9063_ADC_MUX_ADCIN1,
> > +	DA9063_CHAN_ADCIN2 = DA9063_ADC_MUX_ADCIN2,
> > +	DA9063_CHAN_ADCIN3 = DA9063_ADC_MUX_ADCIN3,
> > +	DA9063_CHAN_TJUNC = DA9063_ADC_MUX_T_SENSE,
> > +	DA9063_CHAN_VBBAT = DA9063_ADC_MUX_VBBAT,
> > +	DA9063_CHAN_LDO_G1 = DA9063_ADC_MUX_LDO_G1,
> > +	DA9063_CHAN_LDO_G2 = DA9063_ADC_MUX_LDO_G2,
> > +	DA9063_CHAN_LDO_G3 = DA9063_ADC_MUX_LDO_G3  
> 
> Many of the above defines are not used. Do you plan a follow-up commit
> to use them ? Otherwise please drop unused defines.

I'm not sure (would like to, but for this I think I need to add
devicetree controls, and I am not sure how this should look like), so in
doubt I will drop them from this patch set.

There are also #defines in this patchset related to ADCIN channels,
which are hence unused. Should I also drop these ? In my (short)
experience, there seem to regularly be unused #defines in headers, so I
left them be.

> > +struct da9063_hwmon {
> > +	struct da9063 *da9063;
> > +	struct mutex hwmon_mutex;
> > +	struct completion adc_ready;
> > +	signed char tjunc_offset;  
> 
> I am curious: 'char' implies 'signed'. Any reason for using 'signed' ?

We are again getting into my "erring on the status-quo side" as this
comes from the original patchset. My reading of this is that using a
char for holding an integer is somewhat unusual (as opposed to a holding
character) and the non-essential "signed" would signal that there is
something maybe a bit unusual going on here.

But this all becomes moot with your next point:

> Also, note that on most architectures the resulting code is more complex
> when using 'char' instead of 'int'. This is seen easily by compiling the
> driver for arm64: Replacing the above 'signed char' with 'int' reduces
> code size by 32 bytes.

This is reaching outside of the parts of C that I am comfortable in:
what is the correct way to sign-extend an 8-bits value into an int ?

In regmap_read() fills "int *value" with the read bytes, not
sign-extended (which looks sane):
	ret = regmap_read(da9063->regmap, DA9063_REG_T_OFFSET, &tmp);
	dev_warn(&pdev->dev, "da9063_hwmon_probe offset=%d\n", tmp);
->
[Jul11 01:53] da9063-hwmon da9063-hwmon: da9063_hwmon_probe offset=247

My naïve "(int)((char)tmp)" produces 247, instead of -9.
"(int)hwmon->tjunc_offset" does sign-extend, but going through an
intermediate variable looks overcomplex to me (for a tiny definition of
"overcomplex").
I see sign_extend*() functions but seeing their bitshift arguments I
feel these may not be intended for such no-shift-needed use.

> > +static int da9063_adc_manual_read(struct da9063_hwmon *hwmon, int channel)
[...]
> > +	ret = wait_for_completion_timeout(&hwmon->adc_ready,
> > +					  msecs_to_jiffies(100));
> > +	reinit_completion(&hwmon->adc_ready);  
> 
> This is unusual. Normally I see init_completion() or reinit_completion()
> ahead of calls to wait functions.
> 
> If a request timed out and an interrupt happened after the timeout,
> the next request would return immediately with the previous result,
> since complete() would be called on the re-initialized completion
> handler. That doesn't seem to be correct to me.

To confirm my comprehension: the issue is that if somehow the irq
handler fires outside a conversion request, it will mark adf_ready as
completed, so wait_for_completion_timeout() will immediately return.
The follow-up consequences being that the ADC, having just been asked
to do a new conversion, will still be busy, leading to a spurious
ETIMEDOUT.
Is this correct ?

With this in mind, could the time from regmap_update_bits() to
{,re}init_completion() be longer than the time the IRQ could take to
trigger ? In which case adc_ready would be marked as completed, then it
would be cleared, and wait_for_completion_timeout() would reach its
timeout despite the conversion being already over.

> > +static int da9063_hwmon_probe(struct platform_device *pdev)
[...]
> > +	ret = regmap_read(da9063->regmap, DA9063_REG_T_OFFSET, &tmp);
> > +	if (ret < 0) {
> > +		tmp = 0;
> > +		dev_warn(&pdev->dev,
> > +			 "Temperature trimming value cannot be read (defaulting to 0)\n");
> > +	}
> > +	hwmon->tjunc_offset = (signed char) tmp;  
> 
> Nit: Unnecessary space after typecast (checkpatch --strict would tell you).
> 
> Also, I am curious: The temperature offset is a standard hwmon attribute.
> Is it an oversight to not report it, or is it on purpose ?

It was an oversight, but now that I know about it I am not sure this
should be used: the offset is in chip-internal ADC units, so userland
cannot make use of it for temperature measurement unless the raw ADC
output is also exposed.
Is this attribute used to give an insight as to how the chip was
calibrated in-factory or otherwise good practice to expose ?

I can of course expose it and apply the same formula as for the
temperature attribute, to get the expected m°C unit.

Regards,
-- 
Vincent Pelletier
GPG fingerprint 983A E8B7 3B91 1598 7A92 3845 CAC9 3691 4257 B0C1

Re: [PATCH v3 2/3] hwmon: da9063: HWMON driver

[Guenter Roeck]

On 7/10/21 7:55 PM, Vincent Pelletier wrote:
> Hello,
> 
> Thanks a lot for this new review (and sorry for the previous
> very-incomplete send, unfortunate keyboard shortcut and sleepy fingers).
> 
> On Sat, 10 Jul 2021 09:08:13 -0700, Guenter Roeck <linux@roeck-us.net> wrote:
>> Unnecessary include.
> [...]
>> I don't immediately see where this include is needed. Is this a
>> leftover ?
> [...]
>> Same here.
> 
> Are there ways to systematically tell which includes are useless
> besides commenting them out all and uncommenting until it compiles ?
> (if that is even a good idea)
> 

I am sure there are, but I don't know any pointers. Either case, commenting
out include files until it fails to compile is not a good idea.
The driver then may compile with one architecture but fail with another.

>>> +enum da9063_adc {
>>> +	DA9063_CHAN_VSYS = DA9063_ADC_MUX_VSYS,
>>> +	DA9063_CHAN_ADCIN1 = DA9063_ADC_MUX_ADCIN1,
>>> +	DA9063_CHAN_ADCIN2 = DA9063_ADC_MUX_ADCIN2,
>>> +	DA9063_CHAN_ADCIN3 = DA9063_ADC_MUX_ADCIN3,
>>> +	DA9063_CHAN_TJUNC = DA9063_ADC_MUX_T_SENSE,
>>> +	DA9063_CHAN_VBBAT = DA9063_ADC_MUX_VBBAT,
>>> +	DA9063_CHAN_LDO_G1 = DA9063_ADC_MUX_LDO_G1,
>>> +	DA9063_CHAN_LDO_G2 = DA9063_ADC_MUX_LDO_G2,
>>> +	DA9063_CHAN_LDO_G3 = DA9063_ADC_MUX_LDO_G3
>>
>> Many of the above defines are not used. Do you plan a follow-up commit
>> to use them ? Otherwise please drop unused defines.
> 
> I'm not sure (would like to, but for this I think I need to add
> devicetree controls, and I am not sure how this should look like), so in
> doubt I will drop them from this patch set.
> 
> There are also #defines in this patchset related to ADCIN channels,
> which are hence unused. Should I also drop these ? In my (short)
> experience, there seem to regularly be unused #defines in headers, so I
> left them be.
> 

Please drop them. They can be added back as needed.

>>> +struct da9063_hwmon {
>>> +	struct da9063 *da9063;
>>> +	struct mutex hwmon_mutex;
>>> +	struct completion adc_ready;
>>> +	signed char tjunc_offset;
>>
>> I am curious: 'char' implies 'signed'. Any reason for using 'signed' ?
> 
> We are again getting into my "erring on the status-quo side" as this
> comes from the original patchset. My reading of this is that using a
> char for holding an integer is somewhat unusual (as opposed to a holding
> character) and the non-essential "signed" would signal that there is
> something maybe a bit unusual going on here.
> 
> But this all becomes moot with your next point:
> 
>> Also, note that on most architectures the resulting code is more complex
>> when using 'char' instead of 'int'. This is seen easily by compiling the
>> driver for arm64: Replacing the above 'signed char' with 'int' reduces
>> code size by 32 bytes.
> 
> This is reaching outside of the parts of C that I am comfortable in:
> what is the correct way to sign-extend an 8-bits value into an int ?
> 
> In regmap_read() fills "int *value" with the read bytes, not
> sign-extended (which looks sane):
> 	ret = regmap_read(da9063->regmap, DA9063_REG_T_OFFSET, &tmp);
> 	dev_warn(&pdev->dev, "da9063_hwmon_probe offset=%d\n", tmp);
> ->
> [Jul11 01:53] da9063-hwmon da9063-hwmon: da9063_hwmon_probe offset=247
> 
> My naïve "(int)((char)tmp)" produces 247, instead of -9.
> "(int)hwmon->tjunc_offset" does sign-extend, but going through an
> intermediate variable looks overcomplex to me (for a tiny definition of
> "overcomplex").
> I see sign_extend*() functions but seeing their bitshift arguments I
> feel these may not be intended for such no-shift-needed use.
> 
Sorry, you lost me there. Those functions use shift operations to move
the sign bit where it belongs, and the shift back retains the sign bit.
What is wrong with that ?

Also:

int main()
{
         unsigned int v1 = 247;
         int v2;
         int v3;

         v2 = (char)v1;
         v3 = (int)((char)v1);

         printf("%d %d %d\n", v1, v2, v3);

         return 0;
}

produces 247 -9 -9, so I don't fully follow what your (int)((char)tmp)
looks like. Besides, the outer typecast is not necessary.
In general,
	v2 = (char)v1;
is good enough since the char -> int conversion is automatic (and sign_extend32()
is indeed overkill for this situation).

Either case, please feel free to use 'char' if you like; I won't insist
on a change to int. However, please drop the "signed".

>>> +static int da9063_adc_manual_read(struct da9063_hwmon *hwmon, int channel)
> [...]
>>> +	ret = wait_for_completion_timeout(&hwmon->adc_ready,
>>> +					  msecs_to_jiffies(100));
>>> +	reinit_completion(&hwmon->adc_ready);
>>
>> This is unusual. Normally I see init_completion() or reinit_completion()
>> ahead of calls to wait functions.
>>
>> If a request timed out and an interrupt happened after the timeout,
>> the next request would return immediately with the previous result,
>> since complete() would be called on the re-initialized completion
>> handler. That doesn't seem to be correct to me.
> 
> To confirm my comprehension: the issue is that if somehow the irq
> handler fires outside a conversion request, it will mark adf_ready as
> completed, so wait_for_completion_timeout() will immediately return.
> The follow-up consequences being that the ADC, having just been asked
> to do a new conversion, will still be busy, leading to a spurious
> ETIMEDOUT.
> Is this correct ?
> 
I don't know what exactly happens. Why don't you try by setting the
timeout to a really small value, one that _does_ result in this
situation ?

> With this in mind, could the time from regmap_update_bits() to
> {,re}init_completion() be longer than the time the IRQ could take to
> trigger ? In which case adc_ready would be marked as completed, then it
> would be cleared, and wait_for_completion_timeout() would reach its
> timeout despite the conversion being already over.
> 
... but what I do know is that I don't understand why you insist having
the reinit_completion() _after_ the  wait call. The above doesn't explain
that. I see it as potentially racy, so if you want to keep the code as-is
I'll want to see a comment in the code explaining why it has to be done
this way, and how it is not racy.

Also: a return value of 0 from wait_for_completion_timeout()
already indicates a timeout. The subsequent regmap_read() to check
if the conversion is complete should not be necessary. If it does,
it really indicates a non-timeout problem. Are there situations
(other than the race condition I am concerned about) where
an interrupt can happen but DA9063_ADC_MAN is still set ?

If so, I think this needs a comment in the code, especially since there
is an extra i2c read which, after all, is costly. Also, this should
probably generate a different error code (-EIO, maybe), and
-ETIMEDOUT should be the result of wait_for_completion_timeout()
returning 0.

>>> +static int da9063_hwmon_probe(struct platform_device *pdev)
> [...]
>>> +	ret = regmap_read(da9063->regmap, DA9063_REG_T_OFFSET, &tmp);
>>> +	if (ret < 0) {
>>> +		tmp = 0;
>>> +		dev_warn(&pdev->dev,
>>> +			 "Temperature trimming value cannot be read (defaulting to 0)\n");
>>> +	}
>>> +	hwmon->tjunc_offset = (signed char) tmp;
>>
>> Nit: Unnecessary space after typecast (checkpatch --strict would tell you).
>>
>> Also, I am curious: The temperature offset is a standard hwmon attribute.
>> Is it an oversight to not report it, or is it on purpose ?
> 
> It was an oversight, but now that I know about it I am not sure this
> should be used: the offset is in chip-internal ADC units, so userland
> cannot make use of it for temperature measurement unless the raw ADC
> output is also exposed.

One would not report the raw value, but convert it to m°C.

> Is this attribute used to give an insight as to how the chip was
> calibrated in-factory or otherwise good practice to expose ?
> 

It can be exposed as read-only value if it is a read-only
register/value. Ultimately it is your call if it is indeed read-only.
It still provides some value in that case, but not much.

Thanks,
Guenter

Re: [PATCH v3 2/3] hwmon: da9063: HWMON driver

[Vincent Pelletier]

Hello,

On Sat, 10 Jul 2021 21:22:35 -0700, Guenter Roeck <linux@roeck-us.net> wrote:
> int main()
> {
>          unsigned int v1 = 247;
>          int v2;
>          int v3;
> 
>          v2 = (char)v1;
>          v3 = (int)((char)v1);
> 
>          printf("%d %d %d\n", v1, v2, v3);
> 
>          return 0;
> }
> 
> produces 247 -9 -9, so I don't fully follow what your (int)((char)tmp)
> looks like.

On the riscv machine I am writing this driver for (and the only one I
have with this chip), I get:
  $ gcc test.c
  $ ./a.out
  247 247 247
  $ file a.out
  a.out: ELF 64-bit LSB pie executable, UCB RISC-V, version 1 (SYSV), dynamically linked, interpreter /lib/ld-linux-riscv64-lp64d.so.1, BuildID[sha1]=0a146933fa8f9ab982a7aedb91b6e43b1bd8c668, for GNU/Linux 4.15.0, not stripped

It turns out that "char", without specifiers, is unsigned in the riscv
ABI:
  https://github.com/riscv/riscv-elf-psabi-doc/blob/master/riscv-elf.md#c-type-representations

And indeed with:
  v2 = (signed char)v1;
  v3 = (int)((signed char)v1);
I get the expected output:
  247 -9 -9

This means I will be leaving a (signed char) in the code, and I am
unsure if it needs anything else:
- someone eventually dropping the apparently useless qualifier will
  break the code on riscv, so a comment would be good
- OTOH, if this is an ABI-level specificity and not something unique to
  this driver, then such comment would surely be needed in a lot of
  places, which would just get in the way.

What is your opinion ?

> > With this in mind, could the time from regmap_update_bits() to
> > {,re}init_completion() be longer than the time the IRQ could take to
> > trigger ? In which case adc_ready would be marked as completed, then it
> > would be cleared, and wait_for_completion_timeout() would reach its
> > timeout despite the conversion being already over.
> >   
> ... but what I do know is that I don't understand why you insist having
> the reinit_completion() _after_ the  wait call.

Sorry that I gave you this impression, as this is definitely not my
intention.
I am rather trying to understand why moving {,re}init_completion() just
before the wait call is enough to fix the issue, as I am under the
impression that I may need to do more:
The hardware IRQ could have been received before the DA9063_ADC_MAN
write, and I guess the threaded handler can be delayed. So what is
preventing the interrupt handler from running right between
{,re}init_completion() and the wait ?

I'm leaning towards masking the interrupt when outside
da9063_adc_manual_read:
- acquire measure lock
- if ADC is not ready, return some error (-EIO ? -EAGAIN ? -EBUSY ?)
  as there does not seem to be a way to cancel an already triggered
  conversion, so no way to prevent an interrupt triggering at an
  unexpected time
- clear any pending ADC IRQ
- unmask ADC IRQ
- clear completion
- trigger measure
- wait for completion
- if timeout, return -ETIMEDOUT
- decode measure
- mask ADC IRQ
- release measure lock

(plus a few gotos to cleanup code, and register read/write error
propagation)
This looks race-free to me, at the cost of a 3 extra register writes.

> Also: a return value of 0 from wait_for_completion_timeout()
> already indicates a timeout. The subsequent regmap_read() to check
> if the conversion is complete should not be necessary. If it does,
> it really indicates a non-timeout problem. Are there situations
> (other than the race condition I am concerned about) where
> an interrupt can happen but DA9063_ADC_MAN is still set ?

Not as far as I know: only the ADC triggers this interrupt, and only
this driver should trigger an ADC conversion.
The chip can trigger the ADC internally, but these should not trigger
the IRQ according to the chip's documentation.

> If so, I think this needs a comment in the code, especially since there
> is an extra i2c read which, after all, is costly.

I was curious about the cost, so I checked with regmap events in
debug/tracing (hopefully this is representative enough). Here is the
breakdown (as of patchset v3, so without the IRQ masking scheme I am
considering):
- writing to DA9063_ADC_MAN to select the channel and initiate the
  measure (3 reads, 1 write): 3.5ms
- ADC measure, based on the time between the end of DA9063_ADC_MAN
  write and when the GPIO driver masks its interrupt line: 1.6ms
- clearing the IRQ in the DA9063 (3 reads, 1 write): 2ms
- reading DA9063_ADC_MAN back (2 reads): 1ms
- reading the conversion result (2 reads): 1ms
Total (including scheduling to and from the threaded interrupt
handler): 9.3ms

Regards,
-- 
Vincent Pelletier
GPG fingerprint 983A E8B7 3B91 1598 7A92 3845 CAC9 3691 4257 B0C1

Re: [PATCH v3 2/3] hwmon: da9063: HWMON driver

[Guenter Roeck]

On 7/11/21 4:39 AM, Vincent Pelletier wrote:
> Hello,
> 
> On Sat, 10 Jul 2021 21:22:35 -0700, Guenter Roeck <linux@roeck-us.net> wrote:
>> int main()
>> {
>>           unsigned int v1 = 247;
>>           int v2;
>>           int v3;
>>
>>           v2 = (char)v1;
>>           v3 = (int)((char)v1);
>>
>>           printf("%d %d %d\n", v1, v2, v3);
>>
>>           return 0;
>> }
>>
>> produces 247 -9 -9, so I don't fully follow what your (int)((char)tmp)
>> looks like.
> 
> On the riscv machine I am writing this driver for (and the only one I
> have with this chip), I get:
>    $ gcc test.c
>    $ ./a.out
>    247 247 247
>    $ file a.out
>    a.out: ELF 64-bit LSB pie executable, UCB RISC-V, version 1 (SYSV), dynamically linked, interpreter /lib/ld-linux-riscv64-lp64d.so.1, BuildID[sha1]=0a146933fa8f9ab982a7aedb91b6e43b1bd8c668, for GNU/Linux 4.15.0, not stripped
> 
> It turns out that "char", without specifiers, is unsigned in the riscv
> ABI:
>    https://github.com/riscv/riscv-elf-psabi-doc/blob/master/riscv-elf.md#c-type-representations
> 
> And indeed with:
>    v2 = (signed char)v1;
>    v3 = (int)((signed char)v1);
> I get the expected output:
>    247 -9 -9
> 
> This means I will be leaving a (signed char) in the code, and I am
> unsure if it needs anything else:
> - someone eventually dropping the apparently useless qualifier will
>    break the code on riscv, so a comment would be good
> - OTOH, if this is an ABI-level specificity and not something unique to
>    this driver, then such comment would surely be needed in a lot of
>    places, which would just get in the way.
> 
> What is your opinion ?
> 

If char, as it appears to be, is not well defined in C, it should be
avoided to use it to express a number which could be negative.
Fortunately this seems to be the only place at least in hwmon drivers
where someone insistss using a char to express such a value.

>>> With this in mind, could the time from regmap_update_bits() to
>>> {,re}init_completion() be longer than the time the IRQ could take to
>>> trigger ? In which case adc_ready would be marked as completed, then it
>>> would be cleared, and wait_for_completion_timeout() would reach its
>>> timeout despite the conversion being already over.
>>>    
>> ... but what I do know is that I don't understand why you insist having
>> the reinit_completion() _after_ the  wait call.
> 
> Sorry that I gave you this impression, as this is definitely not my
> intention.
> I am rather trying to understand why moving {,re}init_completion() just
> before the wait call is enough to fix the issue, as I am under the
> impression that I may need to do more:
> The hardware IRQ could have been received before the DA9063_ADC_MAN
> write, and I guess the threaded handler can be delayed. So what is
> preventing the interrupt handler from running right between
> {,re}init_completion() and the wait ?
> 
> I'm leaning towards masking the interrupt when outside
> da9063_adc_manual_read:
> - acquire measure lock
> - if ADC is not ready, return some error (-EIO ? -EAGAIN ? -EBUSY ?)
>    as there does not seem to be a way to cancel an already triggered
>    conversion, so no way to prevent an interrupt triggering at an
>    unexpected time
> - clear any pending ADC IRQ
> - unmask ADC IRQ
> - clear completion
> - trigger measure
> - wait for completion
> - if timeout, return -ETIMEDOUT
> - decode measure
> - mask ADC IRQ
> - release measure lock
> 
> (plus a few gotos to cleanup code, and register read/write error
> propagation)
> This looks race-free to me, at the cost of a 3 extra register writes.
> 

Seems to me that da9063 is similar to da9052 and da9055, except for
conversion constants, and that it should be sufficient to follow the
guidance from that code.

Guenter

Re: [PATCH v3 2/3] hwmon: da9063: HWMON driver

[Vincent Pelletier]

On Sun, 11 Jul 2021 02:55:02 +0000, Vincent Pelletier <plr.vincent@gmail.com> wrote:
> On Sat, 10 Jul 2021 09:08:13 -0700, Guenter Roeck <linux@roeck-us.net> wrote:
> > Unnecessary include.  
> [...]
> > I don't immediately see where this include is needed. Is this a
> > leftover ?  
> [...]
> > Same here.  
> 
> Are there ways to systematically tell which includes are useless
> besides commenting them out all and uncommenting until it compiles ?
> (if that is even a good idea)

I tried this, just to get a baseline: the module compiles with just
  linux/hwmon.h
  linux/mfd/da9063/core.h
  linux/module.h
  linux/platform_device.h
  linux/regmap.h

Beyond what you suggested this also gets rid of:
- seems reasonable:
  - linux/delay.h
  - linux/init.h
  - linux/slab.h
- looks suspicious to me:
  - linux/err.h, which means the error constants are indirectly
    imported. Removing it feels brittle.
  - linux/kernel.h, although to my surprise a lot of c files do not
    include it.

By default I'll drop the former and keep the latter in the
next version, please let me know if another combination is preferred.
-- 
Vincent Pelletier
GPG fingerprint 983A E8B7 3B91 1598 7A92 3845 CAC9 3691 4257 B0C1

Re: [PATCH v3 2/3] hwmon: da9063: HWMON driver

[Randy Dunlap]

On 7/10/21 9:44 PM, Vincent Pelletier wrote:
> On Sun, 11 Jul 2021 02:55:02 +0000, Vincent Pelletier <plr.vincent@gmail.com> wrote:
>> On Sat, 10 Jul 2021 09:08:13 -0700, Guenter Roeck <linux@roeck-us.net> wrote:
>>> Unnecessary include.
>> [...]
>>> I don't immediately see where this include is needed. Is this a
>>> leftover ?
>> [...]
>>> Same here.
>>
>> Are there ways to systematically tell which includes are useless
>> besides commenting them out all and uncommenting until it compiles ?
>> (if that is even a good idea)
> 
> I tried this, just to get a baseline: the module compiles with just
>    linux/hwmon.h
>    linux/mfd/da9063/core.h
>    linux/module.h
>    linux/platform_device.h
>    linux/regmap.h
> 
> Beyond what you suggested this also gets rid of:
> - seems reasonable:
>    - linux/delay.h
>    - linux/init.h
>    - linux/slab.h
> - looks suspicious to me:
>    - linux/err.h, which means the error constants are indirectly
>      imported. Removing it feels brittle.
>    - linux/kernel.h, although to my surprise a lot of c files do not
>      include it.
> 
> By default I'll drop the former and keep the latter in the
> next version, please let me know if another combination is preferred.
> 

Hi,

Please use Rule #1 from Documentation/process/submit-checklist.rst:

1) If you use a facility then #include the file that defines/declares
    that facility.  Don't depend on other header files pulling in ones
    that you use.


so if Enumbers (error numbers) are used, then #include the header file
for that.

etc.

thanks.

-- 
~Randy

Re: [PATCH v3 2/3] hwmon: da9063: HWMON driver

[Vincent Pelletier]

Hello,

On Sat, 10 Jul 2021 21:58:33 -0700, Randy Dunlap <rdunlap@infradead.org> wrote:
> > Beyond what you suggested this also gets rid of:
> > - seems reasonable:
> >    - linux/delay.h
> >    - linux/init.h
> >    - linux/slab.h
> > - looks suspicious to me:
> >    - linux/err.h, which means the error constants are indirectly
> >      imported. Removing it feels brittle.
> >    - linux/kernel.h, although to my surprise a lot of c files do not
> >      include it.
> > 
> > By default I'll drop the former and keep the latter in the
> > next version, please let me know if another combination is preferred.
> >   
> 
> Hi,
> 
> Please use Rule #1 from Documentation/process/submit-checklist.rst:
> 
> 1) If you use a facility then #include the file that defines/declares
>     that facility.  Don't depend on other header files pulling in ones
>     that you use.
> 
> so if Enumbers (error numbers) are used, then #include the header file
> for that.

My mail was poorly phrased, sorry.
I meant that I intend to drop the includes from the "seems reasonable"
group, and to keep those from the "looks suspicious" group.

Regards,
-- 
Vincent Pelletier
GPG fingerprint 983A E8B7 3B91 1598 7A92 3845 CAC9 3691 4257 B0C1

Re: [PATCH v3 2/3] hwmon: da9063: HWMON driver

[Guenter Roeck]

On 7/10/21 9:44 PM, Vincent Pelletier wrote:
> On Sun, 11 Jul 2021 02:55:02 +0000, Vincent Pelletier <plr.vincent@gmail.com> wrote:
>> On Sat, 10 Jul 2021 09:08:13 -0700, Guenter Roeck <linux@roeck-us.net> wrote:
>>> Unnecessary include.
>> [...]
>>> I don't immediately see where this include is needed. Is this a
>>> leftover ?
>> [...]
>>> Same here.
>>
>> Are there ways to systematically tell which includes are useless
>> besides commenting them out all and uncommenting until it compiles ?
>> (if that is even a good idea)
> 
> I tried this, just to get a baseline: the module compiles with just
>    linux/hwmon.h
>    linux/mfd/da9063/core.h
>    linux/module.h
>    linux/platform_device.h
>    linux/regmap.h
> 
> Beyond what you suggested this also gets rid of:
> - seems reasonable:
>    - linux/delay.h
>    - linux/init.h
>    - linux/slab.h

... except that slab.h was probably originally included for kzalloc().
The driver now uses devm_kzalloc() which is defined in linux/device.h,
so you'll need to include that instead.

> - looks suspicious to me:
>    - linux/err.h, which means the error constants are indirectly
>      imported. Removing it feels brittle.

It also defines PTR_ERR_OR_ZERO(), so it is definitely needed.

Guenter

>    - linux/kernel.h, although to my surprise a lot of c files do not
>      include it.
> 
> By default I'll drop the former and keep the latter in the
> next version, please let me know if another combination is preferred.
> 

[PATCH v3 3/3] Documentation: hwmon: New information for DA9063

[Vincent Pelletier]

From: "Opensource [Steve Twiss]" <stwiss.opensource@diasemi.com>

Addition of HWMON documentation for the DA9063 driver.

Signed-off-by: Opensource [Steve Twiss] <stwiss.opensource@diasemi.com>

Updated temperature formula, as of datasheet rev 2.3.
Converted to ReStructuredText.

Signed-off-by: Vincent Pelletier <plr.vincent@gmail.com>
---
Changes in v3:
- Reference added to index.rst

Changes in v2:
- ReST-ified

Originally submitted by Steve Twiss in 2014:
  https://marc.info/?l=linux-kernel&m=139560868209856&w=2

 Documentation/hwmon/da9063.rst | 73 ++++++++++++++++++++++++++++++++++
 Documentation/hwmon/index.rst  |  1 +
 2 files changed, 74 insertions(+)
 create mode 100644 Documentation/hwmon/da9063.rst

diff --git a/Documentation/hwmon/da9063.rst b/Documentation/hwmon/da9063.rst
new file mode 100644
index 000000000000..aae69c58a5d6
--- /dev/null
+++ b/Documentation/hwmon/da9063.rst
@@ -0,0 +1,73 @@
+Kernel driver da9063-hwmon
+==========================
+
+Supported chips:
+
+  * Dialog Semiconductor DA9063 PMIC
+
+    Prefix: 'da9063'
+
+    Datasheet: Publicly available at the Dialog Semiconductor website:
+
+	http://www.dialog-semiconductor.com/products/power-management/DA9063
+
+Authors:
+	- S Twiss <stwiss.opensource@diasemi.com>
+	- Vincent Pelletier <plr.vincent@gmail.com>
+
+Description
+-----------
+
+The DA9063 PMIC provides a general purpose ADC with 10 bits of resolution.
+It uses track and hold circuitry with an analogue input multiplexer which
+allows the conversion of up to 9 different inputs:
+
+- Channel  0: VSYS_RES	measurement of the system VDD (2.5 - 5.5V)
+- Channel  1: ADCIN1_RES	high impedance input (0 - 2.5V)
+- Channel  2: ADCIN2_RES	high impedance input (0 - 2.5V)
+- Channel  3: ADCIN3_RES	high impedance input (0 - 2.5V)
+- Channel  4: Tjunc	measurement of internal temperature sensor
+- Channel  5: VBBAT	measurement of the backup battery voltage (0 - 5.0V)
+- Channel  6: N/A	Reserved
+- Channel  7: N/A	Reserved
+- Channel  8: MON1_RES	group 1 internal regulators voltage (0 - 5.0V)
+- Channel  9: MON2_RES	group 2 internal regulators voltage (0 - 5.0V)
+- Channel 10: MON3_RES	group 3 internal regulators voltage (0 - 5.0V)
+
+The MUX selects from and isolates the 9 inputs and presents the channel to
+be measured to the ADC input. When selected, an input amplifier on the VSYS
+channel subtracts the VDDCORE reference voltage and scales the signal to the
+correct value for the ADC.
+
+The analog ADC includes current sources at ADC_IN1, ADC_IN2 and ADC_IN3 to
+support resistive measurements.
+
+Channels 1, 2 and 3 current source capability can be set through the ADC
+thresholds ADC_CFG register and values for ADCIN1_CUR, ADCIN2_CUR and
+ADCIN3_CUR. Settings for ADCIN1_CUR and ADCIN2_CUR are 1.0, 2.0, 10 and
+40 micro Amps. The setting for ADCIN3_CUR is 10 micro Amps.
+
+Voltage Monitoring
+------------------
+
+The manual measurement allows monitoring of the system voltage VSYS, the
+auxiliary channels ADCIN1, ADCIN2 and ADCIN3, and a VBBAT measurement of
+the backup battery voltage (0 - 5.0V). The manual measurements store 10
+bits of ADC resolution.
+
+The manual ADC measurements attributes described above are supported by
+the driver.
+
+The automatic ADC measurement is not supported by the driver.
+
+Temperature Monitoring
+----------------------
+
+Temperatures are sampled by a 10 bit ADC.  Junction temperatures
+are monitored by the ADC channels.
+
+The junction temperature is calculated:
+
+	Degrees celsius = -0.398 * (ADC_RES - T_OFFSET) + 330;
+
+The junction temperature attribute is supported by the driver.
diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
index 9ed60fa84cbe..b3aba6525157 100644
--- a/Documentation/hwmon/index.rst
+++ b/Documentation/hwmon/index.rst
@@ -51,6 +51,7 @@ Hardware Monitoring Kernel Drivers
    corsair-psu
    da9052
    da9055
+   da9063
    dell-smm-hwmon
    dme1737
    drivetemp
-- 
2.32.0

Re: [PATCH v3 3/3] Documentation: hwmon: New information for DA9063

[Guenter Roeck]

On 7/7/21 6:25 AM, Vincent Pelletier wrote:
> From: "Opensource [Steve Twiss]" <stwiss.opensource@diasemi.com>
> 
> Addition of HWMON documentation for the DA9063 driver.
> 
> Signed-off-by: Opensource [Steve Twiss] <stwiss.opensource@diasemi.com>
> 
> Updated temperature formula, as of datasheet rev 2.3.
> Converted to ReStructuredText.
> 
> Signed-off-by: Vincent Pelletier <plr.vincent@gmail.com>
> ---
> Changes in v3:
> - Reference added to index.rst
> 
> Changes in v2:
> - ReST-ified
> 
> Originally submitted by Steve Twiss in 2014:
>    https://marc.info/?l=linux-kernel&m=139560868209856&w=2
> 
>   Documentation/hwmon/da9063.rst | 73 ++++++++++++++++++++++++++++++++++
>   Documentation/hwmon/index.rst  |  1 +
>   2 files changed, 74 insertions(+)
>   create mode 100644 Documentation/hwmon/da9063.rst
> 
> diff --git a/Documentation/hwmon/da9063.rst b/Documentation/hwmon/da9063.rst
> new file mode 100644
> index 000000000000..aae69c58a5d6
> --- /dev/null
> +++ b/Documentation/hwmon/da9063.rst
> @@ -0,0 +1,73 @@
> +Kernel driver da9063-hwmon
> +==========================
> +
> +Supported chips:
> +
> +  * Dialog Semiconductor DA9063 PMIC
> +
> +    Prefix: 'da9063'
> +
> +    Datasheet: Publicly available at the Dialog Semiconductor website:
> +
> +	http://www.dialog-semiconductor.com/products/power-management/DA9063
> +
> +Authors:
> +	- S Twiss <stwiss.opensource@diasemi.com>
> +	- Vincent Pelletier <plr.vincent@gmail.com>
> +
> +Description
> +-----------
> +
> +The DA9063 PMIC provides a general purpose ADC with 10 bits of resolution.
> +It uses track and hold circuitry with an analogue input multiplexer which
> +allows the conversion of up to 9 different inputs:
> +
> +- Channel  0: VSYS_RES	measurement of the system VDD (2.5 - 5.5V)
> +- Channel  1: ADCIN1_RES	high impedance input (0 - 2.5V)
> +- Channel  2: ADCIN2_RES	high impedance input (0 - 2.5V)
> +- Channel  3: ADCIN3_RES	high impedance input (0 - 2.5V)
> +- Channel  4: Tjunc	measurement of internal temperature sensor
> +- Channel  5: VBBAT	measurement of the backup battery voltage (0 - 5.0V)
> +- Channel  6: N/A	Reserved
> +- Channel  7: N/A	Reserved
> +- Channel  8: MON1_RES	group 1 internal regulators voltage (0 - 5.0V)
> +- Channel  9: MON2_RES	group 2 internal regulators voltage (0 - 5.0V)
> +- Channel 10: MON3_RES	group 3 internal regulators voltage (0 - 5.0V)
> +
> +The MUX selects from and isolates the 9 inputs and presents the channel to
> +be measured to the ADC input. When selected, an input amplifier on the VSYS
> +channel subtracts the VDDCORE reference voltage and scales the signal to the
> +correct value for the ADC.
> +
> +The analog ADC includes current sources at ADC_IN1, ADC_IN2 and ADC_IN3 to
> +support resistive measurements.
> +
> +Channels 1, 2 and 3 current source capability can be set through the ADC
> +thresholds ADC_CFG register and values for ADCIN1_CUR, ADCIN2_CUR and
> +ADCIN3_CUR. Settings for ADCIN1_CUR and ADCIN2_CUR are 1.0, 2.0, 10 and
> +40 micro Amps. The setting for ADCIN3_CUR is 10 micro Amps.
> +
> +Voltage Monitoring
> +------------------
> +
> +The manual measurement allows monitoring of the system voltage VSYS, the
> +auxiliary channels ADCIN1, ADCIN2 and ADCIN3, and a VBBAT measurement of
> +the backup battery voltage (0 - 5.0V). The manual measurements store 10
> +bits of ADC resolution.
> +
> +The manual ADC measurements attributes described above are supported by
> +the driver.
> +

Not really. The driver only reports VSYS and VBBAT plus the temperature,
so this documentation is very misleading. Please provide a list of supported
attributes and their association to the ADC channels.

> +The automatic ADC measurement is not supported by the driver.
> +
> +Temperature Monitoring
> +----------------------
> +
> +Temperatures are sampled by a 10 bit ADC.  Junction temperatures
> +are monitored by the ADC channels.
> +
> +The junction temperature is calculated:
> +
> +	Degrees celsius = -0.398 * (ADC_RES - T_OFFSET) + 330;
> +
> +The junction temperature attribute is supported by the driver.
> diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
> index 9ed60fa84cbe..b3aba6525157 100644
> --- a/Documentation/hwmon/index.rst
> +++ b/Documentation/hwmon/index.rst
> @@ -51,6 +51,7 @@ Hardware Monitoring Kernel Drivers
>      corsair-psu
>      da9052
>      da9055
> +   da9063
>      dell-smm-hwmon
>      dme1737
>      drivetemp
> 

Re: [PATCH v3 1/3] mfd: da9063: Add HWMON dependencies

[Lee Jones]

On Wed, 07 Jul 2021, Vincent Pelletier wrote:

> From: "Opensource [Steve Twiss]" <stwiss.opensource@diasemi.com>
> 
> Dependencies required for DA9063 HWMON support.
> 
> Signed-off-by: Opensource [Steve Twiss] <stwiss.opensource@diasemi.com>
> 
> Moved temperature offset reading to hwmon driver.
> 
> Signed-off-by: Vincent Pelletier <plr.vincent@gmail.com>


Nit: Please remove the empty lines between tags.

And format like this please:

[vincent: Moved temperature offset reading to hwmon driver]

> ---
> Changes in v3:
> - moved temperature offset reading to hwmon driver
> 
> Changes in v2:
> - registers.h changes moved from patch 2
> 
> Originally submitted by Steve Twiss in 2014:
>   https://marc.info/?l=linux-kernel&m=139560864709852&w=2
> 
>  include/linux/mfd/da9063/registers.h | 34 ++++++++++++++++++++++++++++
>  1 file changed, 34 insertions(+)

Once fixed:

Acked-by: Lee Jones <lee.jones@linaro.org>

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog