[PATCH v2] hwmon: (max77705) add initial support

Dzmitry Sankouski posted 1 patch 11 months, 1 week ago
There is a newer version of this series
Documentation/hwmon/index.rst    |   1 +
Documentation/hwmon/max77705.rst |  39 +++++++++++++++++++++++++++++++++++++++
MAINTAINERS                      |   7 +++++++
drivers/hwmon/Kconfig            |  10 ++++++++++
drivers/hwmon/Makefile           |   1 +
drivers/hwmon/max77705-hwmon.c   | 250 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
6 files changed, 308 insertions(+)
[PATCH v2] hwmon: (max77705) add initial support
Posted by Dzmitry Sankouski 11 months, 1 week ago
Add support for max77705 hwmon. Includes charger input, system bus, and
vbyp measurements.

Signed-off-by: Dzmitry Sankouski <dsankouski@gmail.com>
---
Maxim MAX77705 is a Companion Power Management and Type-C interface IC.
It includes charger and fuel gauge blocks, and is capable of measuring
charger input current, system bus volatage and current, and bypass
voltage.

Add support for mentioned measurements.
---
Changes in v2:
- EDITME: describe what is new in this series revision.
- EDITME: use bulletpoints and terse descriptions.
- Link to v1: https://lore.kernel.org/r/20250225-initial-support-for-max77705-sensors-v1-1-2be6467628b0@gmail.com
---
Changes in v2:
- sort headers alphabetically
- swap curr channel info, to align indeces with channel_desc struct
- reword coverletter
- fix checkpatch --strict warnings
- remove struct max77705_hwmon, use regmap directly
- move register validation logic to is_visible function
- move common register reading and converting logic to separate function
- remove unnessesary {} in if statement
- s/i2c->dev/pdev->dev in dev_err_probe
---
 Documentation/hwmon/index.rst    |   1 +
 Documentation/hwmon/max77705.rst |  39 +++++++++++++++++++++++++++++++++++++++
 MAINTAINERS                      |   7 +++++++
 drivers/hwmon/Kconfig            |  10 ++++++++++
 drivers/hwmon/Makefile           |   1 +
 drivers/hwmon/max77705-hwmon.c   | 250 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 308 insertions(+)

diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
index 874f8fd26325..444c7865f74f 100644
--- a/Documentation/hwmon/index.rst
+++ b/Documentation/hwmon/index.rst
@@ -158,6 +158,7 @@ Hardware Monitoring Kernel Drivers
    max6639
    max6650
    max6697
+   max77705
    max8688
    mc13783-adc
    mc34vr500
diff --git a/Documentation/hwmon/max77705.rst b/Documentation/hwmon/max77705.rst
new file mode 100644
index 000000000000..9037226c50b9
--- /dev/null
+++ b/Documentation/hwmon/max77705.rst
@@ -0,0 +1,39 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+Kernel driver max77705
+====================
+
+Supported chips:
+
+  * Maxim Integrated MAX77705
+
+    Prefix: 'max77705'
+
+    Addresses scanned: none
+
+    Datasheet: Not available
+
+Authors:
+      - Dzmitry Sankouski <dsankouski@gmail.com>
+
+Description
+-----------
+
+The MAX77705 PMIC provides current and voltage measurements besides fuelgauge:
+- chip input current
+- system bus current and voltage
+- VBYP voltage
+
+Sysfs Attributes
+----------------
+
+================= ========================================
+in1_label         "vbyp"
+in1_input         Measured chip vbyp voltage
+in2_label         "vsys"
+in2_input         Measured chip system bus voltage
+curr1_label       "iin"
+curr1_input       Measured chip input current.
+curr2_label       "isys"
+curr2_input       Measured chip system bus current.
+================= ========================================
diff --git a/MAINTAINERS b/MAINTAINERS
index 29e1a423eee5..0175f9f89325 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -18110,6 +18110,13 @@ S:	Maintained
 F:	Documentation/hwmon/pc87427.rst
 F:	drivers/hwmon/pc87427.c
 
+MAX77705 HARDWARE MONITORING DRIVER
+M:	Dzmitry Sankouski <dsankouski@gmail.com>
+L:	linux-hwmon@vger.kernel.org
+S:	Maintained
+F:	Documentation/hwmon/max77705.rst
+F:	drivers/hwmon/max77705-hwmon.c
+
 PCA9532 LED DRIVER
 M:	Riku Voipio <riku.voipio@iki.fi>
 S:	Maintained
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 56494ab85b83..c86fe094a978 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -1287,6 +1287,16 @@ config SENSORS_MAX31790
 	  This driver can also be built as a module. If so, the module
 	  will be called max31790.
 
+config SENSORS_MAX77705
+	tristate "MAX77705 current and voltage sensor"
+	depends on I2C
+	select REGMAP_I2C
+	help
+	  If you say yes here you get support for MAX77705 sensors connected with I2C.
+
+	  This driver can also be built as a module. If so, the module
+	  will be called max77705-hwmon.
+
 config SENSORS_MC34VR500
 	tristate "NXP MC34VR500 hardware monitoring driver"
 	depends on I2C
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index b7ef0f0562d3..ff69f45eca50 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -159,6 +159,7 @@ obj-$(CONFIG_SENSORS_MAX6650)	+= max6650.o
 obj-$(CONFIG_SENSORS_MAX6697)	+= max6697.o
 obj-$(CONFIG_SENSORS_MAX31790)	+= max31790.o
 obj-$(CONFIG_MAX31827) += max31827.o
+obj-$(CONFIG_SENSORS_MAX77705) += max77705-hwmon.o
 obj-$(CONFIG_SENSORS_MC13783_ADC)+= mc13783-adc.o
 obj-$(CONFIG_SENSORS_MC34VR500)	+= mc34vr500.o
 obj-$(CONFIG_SENSORS_MCP3021)	+= mcp3021.o
diff --git a/drivers/hwmon/max77705-hwmon.c b/drivers/hwmon/max77705-hwmon.c
new file mode 100644
index 000000000000..6d2161421ac7
--- /dev/null
+++ b/drivers/hwmon/max77705-hwmon.c
@@ -0,0 +1,250 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ *  MAX77705 voltage and current hwmon driver.
+ *
+ *  Copyright (C) 2025 Dzmitry Sankouski <dsankouski@gmail.com>
+ */
+
+#include <linux/err.h>
+#include <linux/hwmon-sysfs.h>
+#include <linux/hwmon.h>
+#include <linux/i2c.h>
+#include <linux/jiffies.h>
+#include <linux/kernel.h>
+#include <linux/mfd/max77705-private.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
+
+struct channel_desc {
+	u8 reg;
+	u8 avg_reg;
+	const char *const label;
+	// register resolution. nano Volts for voltage, nano Amperes for current
+	u64 resolution;
+};
+
+static const struct channel_desc current_channel_desc[] = {
+	{
+		.reg = IIN_REG,
+		.label = "IIN_REG",
+		.resolution = 125000
+	},
+	{
+		.reg = ISYS_REG,
+		.avg_reg = AVGISYS_REG,
+		.label = "ISYS_REG",
+		.resolution = 312500
+	}
+};
+
+static const struct channel_desc voltage_channel_desc[] = {
+	{
+		.reg = VBYP_REG,
+		.label = "VBYP_REG",
+		.resolution = 427246
+	},
+	{
+		.reg = VSYS_REG,
+		.label = "VSYS_REG",
+		.resolution = 156250
+	}
+};
+
+static const struct regmap_range max77705_hwmon_readable_ranges[] = {
+	regmap_reg_range(AVGISYS_REG,	AVGISYS_REG + 1),
+	regmap_reg_range(IIN_REG,	IIN_REG + 1),
+	regmap_reg_range(ISYS_REG,	ISYS_REG + 1),
+	regmap_reg_range(VBYP_REG,	VBYP_REG + 1),
+	regmap_reg_range(VSYS_REG,	VSYS_REG + 1),
+};
+
+static const struct regmap_access_table max77705_hwmon_readable_table = {
+	.yes_ranges = max77705_hwmon_readable_ranges,
+	.n_yes_ranges = ARRAY_SIZE(max77705_hwmon_readable_ranges),
+};
+
+static const struct regmap_config max77705_hwmon_regmap_config = {
+	.name = "max77705_hwmon",
+	.reg_bits = 8,
+	.val_bits = 16,
+	.rd_table = &max77705_hwmon_readable_table,
+	.max_register = MAX77705_FG_END,
+	.val_format_endian = REGMAP_ENDIAN_LITTLE
+};
+
+static int max77705_read_and_convert(struct regmap *regmap, u8 reg, u64 res, long *val)
+{
+	int ret;
+	u32 regval;
+
+	ret = regmap_read(regmap, reg, &regval);
+	if (ret < 0)
+		return ret;
+	*val = mult_frac((long)regval, res, 1000000);
+
+	return 0;
+}
+
+static umode_t max77705_is_visible(const void *data,
+				   enum hwmon_sensor_types type,
+				   u32 attr, int channel)
+{
+	switch (type) {
+	case hwmon_in:
+		if (channel >= ARRAY_SIZE(voltage_channel_desc))
+			return 0;
+
+		switch (attr) {
+		case hwmon_in_input:
+		case hwmon_in_label:
+			return 0444;
+		default:
+			break;
+		}
+		break;
+	case hwmon_curr:
+		if (channel >= ARRAY_SIZE(current_channel_desc))
+			return 0;
+
+		switch (attr) {
+		case hwmon_curr_input:
+		case hwmon_in_label:
+			return 0444;
+		case hwmon_curr_average:
+			if (current_channel_desc[channel].avg_reg)
+				return 0444;
+		default:
+			break;
+		}
+		break;
+	default:
+		break;
+	}
+	return 0;
+}
+
+static int max77705_read_string(struct device *dev, enum hwmon_sensor_types type, u32 attr,
+				int channel, const char **buf)
+{
+	switch (type) {
+	case hwmon_curr:
+		switch (attr) {
+		case hwmon_in_label:
+			*buf = current_channel_desc[channel].label;
+			return 0;
+		default:
+			return -EOPNOTSUPP;
+		}
+
+	case hwmon_in:
+		switch (attr) {
+		case hwmon_in_label:
+			*buf = voltage_channel_desc[channel].label;
+			return 0;
+		default:
+			return -EOPNOTSUPP;
+		}
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static int max77705_read(struct device *dev, enum hwmon_sensor_types type,
+			 u32 attr, int channel, long *val)
+{
+	struct regmap *regmap = dev_get_drvdata(dev);
+	u8 reg;
+	u64 res;
+
+	switch (type) {
+	case hwmon_curr:
+		switch (attr) {
+		case hwmon_curr_input:
+			reg = current_channel_desc[channel].reg;
+			res = current_channel_desc[channel].resolution;
+
+			return max77705_read_and_convert(regmap, reg, res, val);
+		case hwmon_curr_average:
+			reg = current_channel_desc[channel].avg_reg;
+			res = current_channel_desc[channel].resolution;
+
+			return max77705_read_and_convert(regmap, reg, res, val);
+		default:
+			return -EOPNOTSUPP;
+		}
+
+	case hwmon_in:
+		switch (attr) {
+		case hwmon_in_input:
+			reg = voltage_channel_desc[channel].reg;
+			res = voltage_channel_desc[channel].resolution;
+
+			return max77705_read_and_convert(regmap, reg, res, val);
+		default:
+			return -EOPNOTSUPP;
+		}
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	return 0;
+}
+
+static const struct hwmon_ops max77705_hwmon_ops = {
+	.is_visible = max77705_is_visible,
+	.read = max77705_read,
+	.read_string = max77705_read_string,
+};
+
+static const struct hwmon_channel_info *max77705_info[] = {
+	HWMON_CHANNEL_INFO(in,
+			   HWMON_I_INPUT | HWMON_I_LABEL,
+			   HWMON_I_INPUT | HWMON_I_LABEL
+			),
+	HWMON_CHANNEL_INFO(curr,
+			   HWMON_C_INPUT | HWMON_C_LABEL,
+			   HWMON_C_INPUT | HWMON_C_AVERAGE | HWMON_C_LABEL
+			),
+	NULL
+};
+
+static const struct hwmon_chip_info max77705_chip_info = {
+	.ops = &max77705_hwmon_ops,
+	.info = max77705_info,
+};
+
+static int max77705_hwmon_probe(struct platform_device *pdev)
+{
+	struct i2c_client *i2c;
+	struct device *hwmon_dev;
+	struct regmap *regmap;
+
+	i2c = to_i2c_client(pdev->dev.parent);
+	regmap = devm_regmap_init_i2c(i2c, &max77705_hwmon_regmap_config);
+	if (IS_ERR(regmap))
+		return dev_err_probe(&pdev->dev, PTR_ERR(regmap),
+				"Failed to register max77705 hwmon regmap\n");
+
+	hwmon_dev = devm_hwmon_device_register_with_info(&pdev->dev, "max77705", regmap,
+							 &max77705_chip_info, NULL);
+	if (IS_ERR(hwmon_dev))
+		return dev_err_probe(&pdev->dev, PTR_ERR(hwmon_dev),
+				"Unable to register hwmon device\n");
+
+	return 0;
+};
+
+static struct platform_driver max77705_hwmon_driver = {
+	.driver = {
+		.name = "max77705-hwmon",
+	},
+	.probe = max77705_hwmon_probe,
+};
+
+module_platform_driver(max77705_hwmon_driver);
+
+MODULE_AUTHOR("Dzmitry Sankouski <dsankouski@gmail.com>");
+MODULE_DESCRIPTION("MAX77705 monitor driver");
+MODULE_LICENSE("GPL");
+

---
base-commit: 20d5c66e1810e6e8805ec0d01373afb2dba9f51a
change-id: 20250123-initial-support-for-max77705-sensors-ad0170ac1ec5

Best regards,
-- 
Dzmitry Sankouski <dsankouski@gmail.com>
Re: [PATCH v2] hwmon: (max77705) add initial support
Posted by Guenter Roeck 11 months, 1 week ago
On 3/4/25 03:08, Dzmitry Sankouski wrote:
> Add support for max77705 hwmon. Includes charger input, system bus, and
> vbyp measurements.
> 
> Signed-off-by: Dzmitry Sankouski <dsankouski@gmail.com>
> ---
> Maxim MAX77705 is a Companion Power Management and Type-C interface IC.
> It includes charger and fuel gauge blocks, and is capable of measuring
> charger input current, system bus volatage and current, and bypass
> voltage.

That makes me wonder if this should be implemented in drivers/power/supply/
and utilize the power_supply->hwmon bridge. That seems to make more sense
to me than implementing a separate hwmon driver.

> 
> Add support for mentioned measurements.
> ---
> Changes in v2:
> - EDITME: describe what is new in this series revision.
> - EDITME: use bulletpoints and terse descriptions.
> - Link to v1: https://lore.kernel.org/r/20250225-initial-support-for-max77705-sensors-v1-1-2be6467628b0@gmail.com

???

> ---
> Changes in v2:
> - sort headers alphabetically
> - swap curr channel info, to align indeces with channel_desc struct
> - reword coverletter
> - fix checkpatch --strict warnings
> - remove struct max77705_hwmon, use regmap directly
> - move register validation logic to is_visible function
> - move common register reading and converting logic to separate function
> - remove unnessesary {} in if statement
> - s/i2c->dev/pdev->dev in dev_err_probe
> ---
>   Documentation/hwmon/index.rst    |   1 +
>   Documentation/hwmon/max77705.rst |  39 +++++++++++++++++++++++++++++++++++++++
>   MAINTAINERS                      |   7 +++++++
>   drivers/hwmon/Kconfig            |  10 ++++++++++
>   drivers/hwmon/Makefile           |   1 +
>   drivers/hwmon/max77705-hwmon.c   | 250 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>   6 files changed, 308 insertions(+)
> 
> diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
> index 874f8fd26325..444c7865f74f 100644
> --- a/Documentation/hwmon/index.rst
> +++ b/Documentation/hwmon/index.rst
> @@ -158,6 +158,7 @@ Hardware Monitoring Kernel Drivers
>      max6639
>      max6650
>      max6697
> +   max77705
>      max8688
>      mc13783-adc
>      mc34vr500
> diff --git a/Documentation/hwmon/max77705.rst b/Documentation/hwmon/max77705.rst
> new file mode 100644
> index 000000000000..9037226c50b9
> --- /dev/null
> +++ b/Documentation/hwmon/max77705.rst
> @@ -0,0 +1,39 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +
> +Kernel driver max77705
> +====================
> +
> +Supported chips:
> +
> +  * Maxim Integrated MAX77705
> +
> +    Prefix: 'max77705'
> +
> +    Addresses scanned: none
> +
> +    Datasheet: Not available
> +
> +Authors:
> +      - Dzmitry Sankouski <dsankouski@gmail.com>
> +
> +Description
> +-----------
> +
> +The MAX77705 PMIC provides current and voltage measurements besides fuelgauge:
> +- chip input current
> +- system bus current and voltage
> +- VBYP voltage
> +
> +Sysfs Attributes
> +----------------
> +
> +================= ========================================
> +in1_label         "vbyp"
> +in1_input         Measured chip vbyp voltage
> +in2_label         "vsys"
> +in2_input         Measured chip system bus voltage
> +curr1_label       "iin"
> +curr1_input       Measured chip input current.
> +curr2_label       "isys"
> +curr2_input       Measured chip system bus current.
> +================= ========================================
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 29e1a423eee5..0175f9f89325 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -18110,6 +18110,13 @@ S:	Maintained
>   F:	Documentation/hwmon/pc87427.rst
>   F:	drivers/hwmon/pc87427.c
>   
> +MAX77705 HARDWARE MONITORING DRIVER
> +M:	Dzmitry Sankouski <dsankouski@gmail.com>
> +L:	linux-hwmon@vger.kernel.org
> +S:	Maintained
> +F:	Documentation/hwmon/max77705.rst
> +F:	drivers/hwmon/max77705-hwmon.c
> +
>   PCA9532 LED DRIVER
>   M:	Riku Voipio <riku.voipio@iki.fi>
>   S:	Maintained
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 56494ab85b83..c86fe094a978 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -1287,6 +1287,16 @@ config SENSORS_MAX31790
>   	  This driver can also be built as a module. If so, the module
>   	  will be called max31790.
>   
> +config SENSORS_MAX77705
> +	tristate "MAX77705 current and voltage sensor"
> +	depends on I2C
> +	select REGMAP_I2C
> +	help
> +	  If you say yes here you get support for MAX77705 sensors connected with I2C.
> +
> +	  This driver can also be built as a module. If so, the module
> +	  will be called max77705-hwmon.
> +
>   config SENSORS_MC34VR500
>   	tristate "NXP MC34VR500 hardware monitoring driver"
>   	depends on I2C
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index b7ef0f0562d3..ff69f45eca50 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -159,6 +159,7 @@ obj-$(CONFIG_SENSORS_MAX6650)	+= max6650.o
>   obj-$(CONFIG_SENSORS_MAX6697)	+= max6697.o
>   obj-$(CONFIG_SENSORS_MAX31790)	+= max31790.o
>   obj-$(CONFIG_MAX31827) += max31827.o
> +obj-$(CONFIG_SENSORS_MAX77705) += max77705-hwmon.o
>   obj-$(CONFIG_SENSORS_MC13783_ADC)+= mc13783-adc.o
>   obj-$(CONFIG_SENSORS_MC34VR500)	+= mc34vr500.o
>   obj-$(CONFIG_SENSORS_MCP3021)	+= mcp3021.o
> diff --git a/drivers/hwmon/max77705-hwmon.c b/drivers/hwmon/max77705-hwmon.c
> new file mode 100644
> index 000000000000..6d2161421ac7
> --- /dev/null
> +++ b/drivers/hwmon/max77705-hwmon.c
> @@ -0,0 +1,250 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + *  MAX77705 voltage and current hwmon driver.
> + *
> + *  Copyright (C) 2025 Dzmitry Sankouski <dsankouski@gmail.com>
> + */
> +
> +#include <linux/err.h>
> +#include <linux/hwmon-sysfs.h>

Not needed.

> +#include <linux/hwmon.h>
> +#include <linux/i2c.h>
> +#include <linux/jiffies.h>
> +#include <linux/kernel.h>
> +#include <linux/mfd/max77705-private.h>

Doesn't exist.

> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/slab.h>
> +
> +struct channel_desc {
> +	u8 reg;
> +	u8 avg_reg;
> +	const char *const label;
> +	// register resolution. nano Volts for voltage, nano Amperes for current
> +	u64 resolution;

u64 is unnecessary. See below.

> +};
> +
> +static const struct channel_desc current_channel_desc[] = {
> +	{
> +		.reg = IIN_REG,
> +		.label = "IIN_REG",
> +		.resolution = 125000
> +	},
> +	{
> +		.reg = ISYS_REG,
> +		.avg_reg = AVGISYS_REG,
> +		.label = "ISYS_REG",
> +		.resolution = 312500
> +	}
> +};
> +
> +static const struct channel_desc voltage_channel_desc[] = {
> +	{
> +		.reg = VBYP_REG,
> +		.label = "VBYP_REG",
> +		.resolution = 427246
> +	},
> +	{
> +		.reg = VSYS_REG,
> +		.label = "VSYS_REG",
> +		.resolution = 156250
> +	}
> +};
> +
> +static const struct regmap_range max77705_hwmon_readable_ranges[] = {
> +	regmap_reg_range(AVGISYS_REG,	AVGISYS_REG + 1),
> +	regmap_reg_range(IIN_REG,	IIN_REG + 1),
> +	regmap_reg_range(ISYS_REG,	ISYS_REG + 1),
> +	regmap_reg_range(VBYP_REG,	VBYP_REG + 1),
> +	regmap_reg_range(VSYS_REG,	VSYS_REG + 1),
> +};
> +
> +static const struct regmap_access_table max77705_hwmon_readable_table = {
> +	.yes_ranges = max77705_hwmon_readable_ranges,
> +	.n_yes_ranges = ARRAY_SIZE(max77705_hwmon_readable_ranges),
> +};
> +
> +static const struct regmap_config max77705_hwmon_regmap_config = {
> +	.name = "max77705_hwmon",
> +	.reg_bits = 8,
> +	.val_bits = 16,
> +	.rd_table = &max77705_hwmon_readable_table,
> +	.max_register = MAX77705_FG_END,
> +	.val_format_endian = REGMAP_ENDIAN_LITTLE
> +};
> +
> +static int max77705_read_and_convert(struct regmap *regmap, u8 reg, u64 res, long *val)

u64 is unnecessary for res.

> +{
> +	int ret;
> +	u32 regval;
> +
> +	ret = regmap_read(regmap, reg, &regval);
> +	if (ret < 0)
> +		return ret;
> +	*val = mult_frac((long)regval, res, 1000000);

This won't build on 32-bit systems. You'll need to use DIV_ROUND_CLOSEST_ULL(),
and the conversion to 64-bit can be done with
	*val = DIV_ROUND_CLOSEST_ULL((u64)regval * res, 1000000);

> +
> +	return 0;
> +}
> +
> +static umode_t max77705_is_visible(const void *data,
> +				   enum hwmon_sensor_types type,
> +				   u32 attr, int channel)
> +{
> +	switch (type) {
> +	case hwmon_in:
> +		if (channel >= ARRAY_SIZE(voltage_channel_desc))
> +			return 0;
> +
> +		switch (attr) {
> +		case hwmon_in_input:
> +		case hwmon_in_label:
> +			return 0444;
> +		default:
> +			break;
> +		}
> +		break;
> +	case hwmon_curr:
> +		if (channel >= ARRAY_SIZE(current_channel_desc))
> +			return 0;
> +
> +		switch (attr) {
> +		case hwmon_curr_input:
> +		case hwmon_in_label:
> +			return 0444;
> +		case hwmon_curr_average:
> +			if (current_channel_desc[channel].avg_reg)
> +				return 0444;
> +		default:
> +			break;
> +		}
> +		break;

The chip provides temperature measurements. Why not support it ?

Also, how about limits ? Doesn't the chip support any ? There do
seem to be some threshold registers.

> +	default:
> +		break;
> +	}
> +	return 0;
> +}
> +
> +static int max77705_read_string(struct device *dev, enum hwmon_sensor_types type, u32 attr,
> +				int channel, const char **buf)
> +{
> +	switch (type) {
> +	case hwmon_curr:
> +		switch (attr) {
> +		case hwmon_in_label:
> +			*buf = current_channel_desc[channel].label;
> +			return 0;
> +		default:
> +			return -EOPNOTSUPP;
> +		}
> +
> +	case hwmon_in:
> +		switch (attr) {
> +		case hwmon_in_label:
> +			*buf = voltage_channel_desc[channel].label;
> +			return 0;
> +		default:
> +			return -EOPNOTSUPP;
> +		}
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}
> +
> +static int max77705_read(struct device *dev, enum hwmon_sensor_types type,
> +			 u32 attr, int channel, long *val)
> +{
> +	struct regmap *regmap = dev_get_drvdata(dev);
> +	u8 reg;
> +	u64 res;
> +
> +	switch (type) {
> +	case hwmon_curr:
> +		switch (attr) {
> +		case hwmon_curr_input:
> +			reg = current_channel_desc[channel].reg;
> +			res = current_channel_desc[channel].resolution;
> +
> +			return max77705_read_and_convert(regmap, reg, res, val);
> +		case hwmon_curr_average:
> +			reg = current_channel_desc[channel].avg_reg;
> +			res = current_channel_desc[channel].resolution;
> +
> +			return max77705_read_and_convert(regmap, reg, res, val);
> +		default:
> +			return -EOPNOTSUPP;
> +		}
> +
> +	case hwmon_in:
> +		switch (attr) {
> +		case hwmon_in_input:
> +			reg = voltage_channel_desc[channel].reg;
> +			res = voltage_channel_desc[channel].resolution;
> +
> +			return max77705_read_and_convert(regmap, reg, res, val);
> +		default:
> +			return -EOPNOTSUPP;
> +		}
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct hwmon_ops max77705_hwmon_ops = {
> +	.is_visible = max77705_is_visible,
> +	.read = max77705_read,
> +	.read_string = max77705_read_string,
> +};
> +
> +static const struct hwmon_channel_info *max77705_info[] = {
> +	HWMON_CHANNEL_INFO(in,
> +			   HWMON_I_INPUT | HWMON_I_LABEL,
> +			   HWMON_I_INPUT | HWMON_I_LABEL
> +			),
> +	HWMON_CHANNEL_INFO(curr,
> +			   HWMON_C_INPUT | HWMON_C_LABEL,
> +			   HWMON_C_INPUT | HWMON_C_AVERAGE | HWMON_C_LABEL
> +			),
> +	NULL
> +};
> +
> +static const struct hwmon_chip_info max77705_chip_info = {
> +	.ops = &max77705_hwmon_ops,
> +	.info = max77705_info,
> +};
> +
> +static int max77705_hwmon_probe(struct platform_device *pdev)
> +{
> +	struct i2c_client *i2c;
> +	struct device *hwmon_dev;
> +	struct regmap *regmap;
> +
> +	i2c = to_i2c_client(pdev->dev.parent);
> +	regmap = devm_regmap_init_i2c(i2c, &max77705_hwmon_regmap_config);

I still think this is unnecessarily restrictive, and that the mfd driver
should register the regmap.

> +	if (IS_ERR(regmap))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(regmap),
> +				"Failed to register max77705 hwmon regmap\n");
> +
> +	hwmon_dev = devm_hwmon_device_register_with_info(&pdev->dev, "max77705", regmap,
> +							 &max77705_chip_info, NULL);
> +	if (IS_ERR(hwmon_dev))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(hwmon_dev),
> +				"Unable to register hwmon device\n");
> +
> +	return 0;
> +};
> +
> +static struct platform_driver max77705_hwmon_driver = {
> +	.driver = {
> +		.name = "max77705-hwmon",
> +	},
> +	.probe = max77705_hwmon_probe,
> +};
> +
> +module_platform_driver(max77705_hwmon_driver);
> +
> +MODULE_AUTHOR("Dzmitry Sankouski <dsankouski@gmail.com>");
> +MODULE_DESCRIPTION("MAX77705 monitor driver");
> +MODULE_LICENSE("GPL");
> +
> 
> ---
> base-commit: 20d5c66e1810e6e8805ec0d01373afb2dba9f51a
> change-id: 20250123-initial-support-for-max77705-sensors-ad0170ac1ec5
> 
> Best regards,
Re: [PATCH v2] hwmon: (max77705) add initial support
Posted by Dzmitry Sankouski 11 months, 1 week ago
вт, 4 мар. 2025 г. в 14:38, Guenter Roeck <linux@roeck-us.net>:
>
> On 3/4/25 03:08, Dzmitry Sankouski wrote:
> > Add support for max77705 hwmon. Includes charger input, system bus, and
> > vbyp measurements.
> >
> > Signed-off-by: Dzmitry Sankouski <dsankouski@gmail.com>
> > ---
> > Maxim MAX77705 is a Companion Power Management and Type-C interface IC.
> > It includes charger and fuel gauge blocks, and is capable of measuring
> > charger input current, system bus volatage and current, and bypass
> > voltage.
>
> That makes me wonder if this should be implemented in drivers/power/supply/
> and utilize the power_supply->hwmon bridge. That seems to make more sense
> to me than implementing a separate hwmon driver.
>

This chip incorporates charger and fuel gauge, which are modeled as power
supply class drivers. These measurements, however, doesn't belong to any of
mentioned drivers. For example, ISYS is the system bus current, but it
not charger attribute, because charger output is split between battery and
system bus.

Powering diagram:

----------              ---------      ------------      --------------
|usb port|<--[input]--> |charger| <--> |fuel gauge| <--> |battery pack|
----------              ---------      ------------      --------------
                            |
                            |
                            |---> [system bus]

See also https://patches.linaro.org/project/linux-leds/patch/20241217-starqltechn_integration_upstream-v12-2-ed840944f948@gmail.com/#952337

> >
> > Add support for mentioned measurements.
> > ---
> > Changes in v2:
> > - EDITME: describe what is new in this series revision.
> > - EDITME: use bulletpoints and terse descriptions.
> > - Link to v1: https://lore.kernel.org/r/20250225-initial-support-for-max77705-sensors-v1-1-2be6467628b0@gmail.com
>
> ???
>

sorry, I forgot to edit that

(...)
> > +static umode_t max77705_is_visible(const void *data,
> > +                                enum hwmon_sensor_types type,
> > +                                u32 attr, int channel)
> > +{
> > +     switch (type) {
> > +     case hwmon_in:
> > +             if (channel >= ARRAY_SIZE(voltage_channel_desc))
> > +                     return 0;
> > +
> > +             switch (attr) {
> > +             case hwmon_in_input:
> > +             case hwmon_in_label:
> > +                     return 0444;
> > +             default:
> > +                     break;
> > +             }
> > +             break;
> > +     case hwmon_curr:
> > +             if (channel >= ARRAY_SIZE(current_channel_desc))
> > +                     return 0;
> > +
> > +             switch (attr) {
> > +             case hwmon_curr_input:
> > +             case hwmon_in_label:
> > +                     return 0444;
> > +             case hwmon_curr_average:
> > +                     if (current_channel_desc[channel].avg_reg)
> > +                             return 0444;
> > +             default:
> > +                     break;
> > +             }
> > +             break;
>
> The chip provides temperature measurements. Why not support it ?
>

It provides temperature sensing in the fuel gauge block, so it is handled there.

> Also, how about limits ? Doesn't the chip support any ? There do
> seem to be some threshold registers.
>

It has a register in the charger block to limit IIN current. It also has VBYP
interrupt, but I didn't found how the limit is set. Actually, I have a feeling
these measurements are implemented by chip firmware, rather than some IP
solution.

(...)

-- 
Best regards and thanks for review,
Dzmitry