diff mbox series

hwmon: Add KEBA battery monitoring controller support

Message ID 20250308212346.51316-1-gerhard@engleder-embedded.com (mailing list archive)
State Changes Requested
Headers show
Series hwmon: Add KEBA battery monitoring controller support | expand

Commit Message

Gerhard Engleder March 8, 2025, 9:23 p.m. UTC 
From: Gerhard Engleder <eg@keba.com>

The KEBA battery monitoring controller is found in the system FPGA of
KEBA PLC devices. It puts a load on the coin cell battery to check the
state of the battery.

Signed-off-by: Gerhard Engleder <eg@keba.com>
---
 drivers/hwmon/Kconfig  |  12 ++++
 drivers/hwmon/Makefile |   1 +
 drivers/hwmon/kbatt.c  | 159 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 172 insertions(+)
 create mode 100644 drivers/hwmon/kbatt.c

Comments

Thomas Weißschuh March 8, 2025, 10:23 p.m. UTC | #1 
On 2025-03-08 22:23:46+0100, Gerhard Engleder wrote:
> From: Gerhard Engleder <eg@keba.com>
> 
> The KEBA battery monitoring controller is found in the system FPGA of
> KEBA PLC devices. It puts a load on the coin cell battery to check the
> state of the battery.

A hint that this will be instantiated from drivers/misc/keba/cp500.c
would be nice.

> Signed-off-by: Gerhard Engleder <eg@keba.com>
> ---
>  drivers/hwmon/Kconfig  |  12 ++++
>  drivers/hwmon/Makefile |   1 +
>  drivers/hwmon/kbatt.c  | 159 +++++++++++++++++++++++++++++++++++++++++

hwmon drivers commonly have an entry in Documentation/hwmon/.

>  3 files changed, 172 insertions(+)
>  create mode 100644 drivers/hwmon/kbatt.c
> 
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 4cbaba15d86e..ec396252cc18 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -335,6 +335,18 @@ config SENSORS_K10TEMP
>  	  This driver can also be built as a module. If so, the module
>  	  will be called k10temp.
>  
> +config SENSORS_KBATT
> +	tristate "KEBA battery controller support"
> +	depends on HAS_IOMEM
> +	depends on KEBA_CP500 || COMPILE_TEST

KEBA_CP500 already has a COMPILE_TEST variant.
Duplicating it here looks unnecessary.
Then the HAS_IOMEM and AUXILIARY_BUS references can go away.

> +	select AUXILIARY_BUS
> +	help
> +	  This driver supports the battery monitoring controller found in
> +	  KEBA system FPGA devices.
> +
> +	  This driver can also be built as a module. If so, the module
> +	  will be called kbatt.
> +
>  config SENSORS_FAM15H_POWER
>  	tristate "AMD Family 15h processor power"
>  	depends on X86 && PCI && CPU_SUP_AMD
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index b7ef0f0562d3..4671a9b77b55 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -108,6 +108,7 @@ obj-$(CONFIG_SENSORS_IT87)	+= it87.o
>  obj-$(CONFIG_SENSORS_JC42)	+= jc42.o
>  obj-$(CONFIG_SENSORS_K8TEMP)	+= k8temp.o
>  obj-$(CONFIG_SENSORS_K10TEMP)	+= k10temp.o
> +obj-$(CONFIG_SENSORS_KBATT)	+= kbatt.o
>  obj-$(CONFIG_SENSORS_LAN966X)	+= lan966x-hwmon.o
>  obj-$(CONFIG_SENSORS_LENOVO_EC)	+= lenovo-ec-sensors.o
>  obj-$(CONFIG_SENSORS_LINEAGE)	+= lineage-pem.o
> diff --git a/drivers/hwmon/kbatt.c b/drivers/hwmon/kbatt.c
> new file mode 100644
> index 000000000000..09a622a7d36a
> --- /dev/null
> +++ b/drivers/hwmon/kbatt.c
> @@ -0,0 +1,159 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) KEBA Industrial Automation Gmbh 2025

typo in "GmbH".

Normal copyright format would be:
Copyright (C) 2025 KEBA Industrial Automation GmbH

> + *
> + * Driver for KEBA battery monitoring controller FPGA IP core
> + */
> +
> +#include <linux/hwmon.h>
> +#include <linux/io.h>
> +#include <linux/delay.h>
> +#include <linux/module.h>
> +#include <linux/misc/keba.h>

#include <linux/auxiliary_bus.h>
#include <linux/device.h>
#include <linux/mutex.h>

> +
> +#define KBATT "kbatt"
> +
> +#define KBATT_CONTROL_REG		0x4
> +#define   KBATT_CONTROL_BAT_TEST	0x01
> +
> +#define KBATT_STATUS_REG		0x8
> +#define   KBATT_STATUS_BAT_OK		0x01
> +
> +#define KBATT_MAX_UPD_INTERVAL	(5 * HZ)
> +#define KBATT_SETTLE_TIME_MS	100
> +
> +struct kbatt {
> +	struct keba_batt_auxdev *auxdev;

Not needed.

> +	/* update lock */
> +	struct mutex lock;
> +	void __iomem *base;
> +	struct device *hwmon_dev;

Not needed.

> +
> +	bool valid;
> +	unsigned long last_updated; /* in jiffies */
> +	long alarm;

bool

> +};
> +
> +static long kbatt_alarm(struct kbatt *kbatt)
> +{
> +	mutex_lock(&kbatt->lock);
> +
> +	if (time_after(jiffies, kbatt->last_updated + KBATT_MAX_UPD_INTERVAL) ||
> +	    !kbatt->valid) {

kbatt->valid can be removed and instead check for
!kbatt->last_updated || time_after().

> +		/* switch load on */
> +		iowrite8(KBATT_CONTROL_BAT_TEST,
> +			 kbatt->base + KBATT_CONTROL_REG);
> +
> +		/* wait some time to let things settle */
> +		msleep(KBATT_SETTLE_TIME_MS);

Could use the recommended fsleep():
Documentation/timers/delay_sleep_functions.rst

> +
> +		/* check battery state */
> +		if (ioread8(kbatt->base + KBATT_STATUS_REG) &
> +		    KBATT_STATUS_BAT_OK)
> +			kbatt->alarm = 0;
> +		else
> +			kbatt->alarm = 1;
> +
> +		/* switch load off */
> +		iowrite8(0, kbatt->base + KBATT_CONTROL_REG);
> +
> +		kbatt->last_updated = jiffies;
> +		kbatt->valid = true;
> +	}
> +
> +	mutex_unlock(&kbatt->lock);
> +
> +	return kbatt->alarm;
> +}
> +
> +static int kbatt_read(struct device *dev, enum hwmon_sensor_types type,
> +		      u32 attr, int channel, long *val)
> +{
> +	struct kbatt *kbatt = dev_get_drvdata(dev);
> +
> +	if ((channel != 1) || (attr != hwmon_in_alarm))
> +		return -EOPNOTSUPP;

This condition is never true.

> +
> +	*val = kbatt_alarm(kbatt);
> +
> +	return 0;
> +}
> +
> +static umode_t kbatt_is_visible(const void *data, enum hwmon_sensor_types type,
> +				u32 attr, int channel)
> +{
> +	if ((channel == 1) && (attr == hwmon_in_alarm))
> +		return 0444;
> +
> +	return 0;
> +}
> +
> +static const struct hwmon_channel_info *kbatt_info[] = {
> +	HWMON_CHANNEL_INFO(in,
> +			   /* 0: dummy, skipped in is_visible */

Why?

> +			   HWMON_I_ALARM,
> +			   /* 1: input alarm channel */
> +			   HWMON_I_ALARM),
> +	NULL
> +};
> +
> +static const struct hwmon_ops kbatt_hwmon_ops = {
> +	.is_visible = kbatt_is_visible,
> +	.read = kbatt_read,
> +};
> +
> +static const struct hwmon_chip_info kbatt_chip_info = {
> +	.ops = &kbatt_hwmon_ops,
> +	.info = kbatt_info,
> +};
> +
> +static int kbatt_probe(struct auxiliary_device *auxdev,
> +		       const struct auxiliary_device_id *id)
> +{
> +	struct device *dev = &auxdev->dev;
> +	struct kbatt *kbatt;
> +
> +	kbatt = devm_kzalloc(dev, sizeof(struct kbatt), GFP_KERNEL);

sizeof(*kbatt)

> +	if (!kbatt)
> +		return -ENOMEM;
> +	kbatt->auxdev = container_of(auxdev, struct keba_batt_auxdev, auxdev);
> +	mutex_init(&kbatt->lock);

devm_mutex_init(), check the return value.

> +	auxiliary_set_drvdata(auxdev, kbatt);

Is this needed?

> +
> +	kbatt->base = devm_ioremap_resource(dev, &kbatt->auxdev->io);
> +	if (IS_ERR(kbatt->base))
> +		return PTR_ERR(kbatt->base);
> +
> +	kbatt->hwmon_dev = devm_hwmon_device_register_with_info(dev, KBATT,
> +							     kbatt,
> +							     &kbatt_chip_info,
> +							     NULL);
> +	if (IS_ERR(kbatt->hwmon_dev))
> +		return PTR_ERR(kbatt->hwmon_dev);
> +
> +	return 0;

Would be slightly shorter with 'return PTR_ERR_OR_ZERO()', but both
variant are fine.

> +}
> +
> +static void kbatt_remove(struct auxiliary_device *auxdev)
> +{
> +	auxiliary_set_drvdata(auxdev, NULL);

Unnecessary.

> +}
> +
> +static const struct auxiliary_device_id kbatt_devtype_aux[] = {
> +	{ .name = "keba.batt" },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(auxiliary, kbatt_devtype_aux);
> +
> +static struct auxiliary_driver kbatt_driver_aux = {
> +	.name = KBATT,
> +	.id_table = kbatt_devtype_aux,
> +	.probe = kbatt_probe,
> +	.remove = kbatt_remove,
> +};
> +module_auxiliary_driver(kbatt_driver_aux);
> +
> +MODULE_AUTHOR("Petar Bojanic <boja@keba.com>");
> +MODULE_AUTHOR("Gerhard Engleder <eg@keba.com>");
> +MODULE_DESCRIPTION("KEBA battery monitoring controller driver");
> +MODULE_LICENSE("GPL");
> -- 
> 2.39.5
> 
>
Guenter Roeck March 8, 2025, 10:32 p.m. UTC | #2 
On 3/8/25 13:23, Gerhard Engleder wrote:
> From: Gerhard Engleder <eg@keba.com>
> 
> The KEBA battery monitoring controller is found in the system FPGA of
> KEBA PLC devices. It puts a load on the coin cell battery to check the
> state of the battery.
> 
> Signed-off-by: Gerhard Engleder <eg@keba.com>

Looking into the keba code, that is kind of piece by piece approach.
I see a reference to fans in drivers/misc/keba/cp500.c, so I guess I'll see
a fan controller driver at some point in the future. I do not support
the idea of having multiple drivers for the hardware monitoring
functionality of a single device.

Either case, the attribute needs to be documented.

Some more technical comments inline.

Guenter

> ---
>   drivers/hwmon/Kconfig  |  12 ++++
>   drivers/hwmon/Makefile |   1 +
>   drivers/hwmon/kbatt.c  | 159 +++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 172 insertions(+)
>   create mode 100644 drivers/hwmon/kbatt.c
> 
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 4cbaba15d86e..ec396252cc18 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -335,6 +335,18 @@ config SENSORS_K10TEMP
>   	  This driver can also be built as a module. If so, the module
>   	  will be called k10temp.
>   
> +config SENSORS_KBATT
> +	tristate "KEBA battery controller support"
> +	depends on HAS_IOMEM
> +	depends on KEBA_CP500 || COMPILE_TEST
> +	select AUXILIARY_BUS
> +	help
> +	  This driver supports the battery monitoring controller found in
> +	  KEBA system FPGA devices.
> +
> +	  This driver can also be built as a module. If so, the module
> +	  will be called kbatt.
> +
>   config SENSORS_FAM15H_POWER
>   	tristate "AMD Family 15h processor power"
>   	depends on X86 && PCI && CPU_SUP_AMD
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index b7ef0f0562d3..4671a9b77b55 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -108,6 +108,7 @@ obj-$(CONFIG_SENSORS_IT87)	+= it87.o
>   obj-$(CONFIG_SENSORS_JC42)	+= jc42.o
>   obj-$(CONFIG_SENSORS_K8TEMP)	+= k8temp.o
>   obj-$(CONFIG_SENSORS_K10TEMP)	+= k10temp.o
> +obj-$(CONFIG_SENSORS_KBATT)	+= kbatt.o
>   obj-$(CONFIG_SENSORS_LAN966X)	+= lan966x-hwmon.o
>   obj-$(CONFIG_SENSORS_LENOVO_EC)	+= lenovo-ec-sensors.o
>   obj-$(CONFIG_SENSORS_LINEAGE)	+= lineage-pem.o
> diff --git a/drivers/hwmon/kbatt.c b/drivers/hwmon/kbatt.c
> new file mode 100644
> index 000000000000..09a622a7d36a
> --- /dev/null
> +++ b/drivers/hwmon/kbatt.c
> @@ -0,0 +1,159 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) KEBA Industrial Automation Gmbh 2025
> + *
> + * Driver for KEBA battery monitoring controller FPGA IP core
> + */
> +
> +#include <linux/hwmon.h>
> +#include <linux/io.h>
> +#include <linux/delay.h>
> +#include <linux/module.h>
> +#include <linux/misc/keba.h>
> +
> +#define KBATT "kbatt"
> +
> +#define KBATT_CONTROL_REG		0x4
> +#define   KBATT_CONTROL_BAT_TEST	0x01
> +
> +#define KBATT_STATUS_REG		0x8
> +#define   KBATT_STATUS_BAT_OK		0x01
> +
> +#define KBATT_MAX_UPD_INTERVAL	(5 * HZ)
> +#define KBATT_SETTLE_TIME_MS	100
> +
> +struct kbatt {
> +	struct keba_batt_auxdev *auxdev;
> +	/* update lock */
> +	struct mutex lock;
> +	void __iomem *base;
> +	struct device *hwmon_dev;
> +
> +	bool valid;
> +	unsigned long last_updated; /* in jiffies */
> +	long alarm;

bool

> +};
> +
> +static long kbatt_alarm(struct kbatt *kbatt)
> +{
> +	mutex_lock(&kbatt->lock);
> +
> +	if (time_after(jiffies, kbatt->last_updated + KBATT_MAX_UPD_INTERVAL) ||
> +	    !kbatt->valid) {
> +		/* switch load on */
> +		iowrite8(KBATT_CONTROL_BAT_TEST,
> +			 kbatt->base + KBATT_CONTROL_REG);
> +
> +		/* wait some time to let things settle */
> +		msleep(KBATT_SETTLE_TIME_MS);
> +
> +		/* check battery state */
> +		if (ioread8(kbatt->base + KBATT_STATUS_REG) &
> +		    KBATT_STATUS_BAT_OK)
> +			kbatt->alarm = 0;
> +		else
> +			kbatt->alarm = 1;
> +
> +		/* switch load off */
> +		iowrite8(0, kbatt->base + KBATT_CONTROL_REG);
> +
> +		kbatt->last_updated = jiffies;
> +		kbatt->valid = true;
> +	}
> +
> +	mutex_unlock(&kbatt->lock);
> +
> +	return kbatt->alarm;
> +}
> +
> +static int kbatt_read(struct device *dev, enum hwmon_sensor_types type,
> +		      u32 attr, int channel, long *val)
> +{
> +	struct kbatt *kbatt = dev_get_drvdata(dev);
> +
> +	if ((channel != 1) || (attr != hwmon_in_alarm))

Various unnecessary ( ) here and below.

> +		return -EOPNOTSUPP;
> +
> +	*val = kbatt_alarm(kbatt);
> +
> +	return 0;
> +}
> +
> +static umode_t kbatt_is_visible(const void *data, enum hwmon_sensor_types type,
> +				u32 attr, int channel)
> +{
> +	if ((channel == 1) && (attr == hwmon_in_alarm))
> +		return 0444;
> +
> +	return 0;
> +}
> +
> +static const struct hwmon_channel_info *kbatt_info[] = {
> +	HWMON_CHANNEL_INFO(in,
> +			   /* 0: dummy, skipped in is_visible */
> +			   HWMON_I_ALARM,
> +			   /* 1: input alarm channel */
> +			   HWMON_I_ALARM),

Not acceptable. The first voltage channel is channel 0, not 1.

> +	NULL
> +};
> +
> +static const struct hwmon_ops kbatt_hwmon_ops = {
> +	.is_visible = kbatt_is_visible,
> +	.read = kbatt_read,
> +};
> +
> +static const struct hwmon_chip_info kbatt_chip_info = {
> +	.ops = &kbatt_hwmon_ops,
> +	.info = kbatt_info,
> +};
> +
> +static int kbatt_probe(struct auxiliary_device *auxdev,
> +		       const struct auxiliary_device_id *id)
> +{
> +	struct device *dev = &auxdev->dev;
> +	struct kbatt *kbatt;
> +
> +	kbatt = devm_kzalloc(dev, sizeof(struct kbatt), GFP_KERNEL);
> +	if (!kbatt)
> +		return -ENOMEM;
> +	kbatt->auxdev = container_of(auxdev, struct keba_batt_auxdev, auxdev);
> +	mutex_init(&kbatt->lock);
> +	auxiliary_set_drvdata(auxdev, kbatt);
> +
> +	kbatt->base = devm_ioremap_resource(dev, &kbatt->auxdev->io);
> +	if (IS_ERR(kbatt->base))
> +		return PTR_ERR(kbatt->base);
> +
> +	kbatt->hwmon_dev = devm_hwmon_device_register_with_info(dev, KBATT,
> +							     kbatt,
> +							     &kbatt_chip_info,
> +							     NULL);
> +	if (IS_ERR(kbatt->hwmon_dev))
> +		return PTR_ERR(kbatt->hwmon_dev);
> +
> +	return 0;
> +}
> +
> +static void kbatt_remove(struct auxiliary_device *auxdev)
> +{
> +	auxiliary_set_drvdata(auxdev, NULL);
> +}
> +
> +static const struct auxiliary_device_id kbatt_devtype_aux[] = {
> +	{ .name = "keba.batt" },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(auxiliary, kbatt_devtype_aux);
> +
> +static struct auxiliary_driver kbatt_driver_aux = {
> +	.name = KBATT,
> +	.id_table = kbatt_devtype_aux,
> +	.probe = kbatt_probe,
> +	.remove = kbatt_remove,
> +};
> +module_auxiliary_driver(kbatt_driver_aux);
> +
> +MODULE_AUTHOR("Petar Bojanic <boja@keba.com>");
> +MODULE_AUTHOR("Gerhard Engleder <eg@keba.com>");
> +MODULE_DESCRIPTION("KEBA battery monitoring controller driver");
> +MODULE_LICENSE("GPL");
Gerhard Engleder March 9, 2025, 7:38 a.m. UTC | #3 
On 08.03.25 23:23, Thomas Weißschuh wrote:
> On 2025-03-08 22:23:46+0100, Gerhard Engleder wrote:
>> From: Gerhard Engleder <eg@keba.com>
>>
>> The KEBA battery monitoring controller is found in the system FPGA of
>> KEBA PLC devices. It puts a load on the coin cell battery to check the
>> state of the battery.
> 
> A hint that this will be instantiated from drivers/misc/keba/cp500.c
> would be nice.

I will add that hint.

> 
>> Signed-off-by: Gerhard Engleder <eg@keba.com>
>> ---
>>   drivers/hwmon/Kconfig  |  12 ++++
>>   drivers/hwmon/Makefile |   1 +
>>   drivers/hwmon/kbatt.c  | 159 +++++++++++++++++++++++++++++++++++++++++
> 
> hwmon drivers commonly have an entry in Documentation/hwmon/.

Will be added.

>>   3 files changed, 172 insertions(+)
>>   create mode 100644 drivers/hwmon/kbatt.c
>>
>> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
>> index 4cbaba15d86e..ec396252cc18 100644
>> --- a/drivers/hwmon/Kconfig
>> +++ b/drivers/hwmon/Kconfig
>> @@ -335,6 +335,18 @@ config SENSORS_K10TEMP
>>   	  This driver can also be built as a module. If so, the module
>>   	  will be called k10temp.
>>   
>> +config SENSORS_KBATT
>> +	tristate "KEBA battery controller support"
>> +	depends on HAS_IOMEM
>> +	depends on KEBA_CP500 || COMPILE_TEST
> 
> KEBA_CP500 already has a COMPILE_TEST variant.
> Duplicating it here looks unnecessary.
> Then the HAS_IOMEM and AUXILIARY_BUS references can go away.

With COMPILE_TEST here the driver can be compile tested individually.
Is this property not worth it? But I can change it if needed.

>> +	select AUXILIARY_BUS
>> +	help
>> +	  This driver supports the battery monitoring controller found in
>> +	  KEBA system FPGA devices.
>> +
>> +	  This driver can also be built as a module. If so, the module
>> +	  will be called kbatt.
>> +
>>   config SENSORS_FAM15H_POWER
>>   	tristate "AMD Family 15h processor power"
>>   	depends on X86 && PCI && CPU_SUP_AMD
>> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
>> index b7ef0f0562d3..4671a9b77b55 100644
>> --- a/drivers/hwmon/Makefile
>> +++ b/drivers/hwmon/Makefile
>> @@ -108,6 +108,7 @@ obj-$(CONFIG_SENSORS_IT87)	+= it87.o
>>   obj-$(CONFIG_SENSORS_JC42)	+= jc42.o
>>   obj-$(CONFIG_SENSORS_K8TEMP)	+= k8temp.o
>>   obj-$(CONFIG_SENSORS_K10TEMP)	+= k10temp.o
>> +obj-$(CONFIG_SENSORS_KBATT)	+= kbatt.o
>>   obj-$(CONFIG_SENSORS_LAN966X)	+= lan966x-hwmon.o
>>   obj-$(CONFIG_SENSORS_LENOVO_EC)	+= lenovo-ec-sensors.o
>>   obj-$(CONFIG_SENSORS_LINEAGE)	+= lineage-pem.o
>> diff --git a/drivers/hwmon/kbatt.c b/drivers/hwmon/kbatt.c
>> new file mode 100644
>> index 000000000000..09a622a7d36a
>> --- /dev/null
>> +++ b/drivers/hwmon/kbatt.c
>> @@ -0,0 +1,159 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (C) KEBA Industrial Automation Gmbh 2025
> 
> typo in "GmbH".

I will fix that.

> Normal copyright format would be:
> Copyright (C) 2025 KEBA Industrial Automation GmbH

I will check the copyright format.

>> + *
>> + * Driver for KEBA battery monitoring controller FPGA IP core
>> + */
>> +
>> +#include <linux/hwmon.h>
>> +#include <linux/io.h>
>> +#include <linux/delay.h>
>> +#include <linux/module.h>
>> +#include <linux/misc/keba.h>
> 
> #include <linux/auxiliary_bus.h>
> #include <linux/device.h>

Do I really have to include them explicitly? They are included
indirectly with linux/misc/keba.h.

> #include <linux/mutex.h>
> 
>> +
>> +#define KBATT "kbatt"
>> +
>> +#define KBATT_CONTROL_REG		0x4
>> +#define   KBATT_CONTROL_BAT_TEST	0x01
>> +
>> +#define KBATT_STATUS_REG		0x8
>> +#define   KBATT_STATUS_BAT_OK		0x01
>> +
>> +#define KBATT_MAX_UPD_INTERVAL	(5 * HZ)
>> +#define KBATT_SETTLE_TIME_MS	100
>> +
>> +struct kbatt {
>> +	struct keba_batt_auxdev *auxdev;
> 
> Not needed.

Will be fixed.

>> +	/* update lock */
>> +	struct mutex lock;
>> +	void __iomem *base;
>> +	struct device *hwmon_dev;
> 
> Not needed.

Will be fixed.

>> +
>> +	bool valid;
>> +	unsigned long last_updated; /* in jiffies */
>> +	long alarm;
> 
> bool

I choose long to match the hwmon API, hwmon_ops->read. Does it need to
be bool nevertheless?

>> +};
>> +
>> +static long kbatt_alarm(struct kbatt *kbatt)
>> +{
>> +	mutex_lock(&kbatt->lock);
>> +
>> +	if (time_after(jiffies, kbatt->last_updated + KBATT_MAX_UPD_INTERVAL) ||
>> +	    !kbatt->valid) {
> 
> kbatt->valid can be removed and instead check for
> !kbatt->last_updated || time_after().

You are right. I will rework.

>> +		/* switch load on */
>> +		iowrite8(KBATT_CONTROL_BAT_TEST,
>> +			 kbatt->base + KBATT_CONTROL_REG);
>> +
>> +		/* wait some time to let things settle */
>> +		msleep(KBATT_SETTLE_TIME_MS);
> 
> Could use the recommended fsleep():
> Documentation/timers/delay_sleep_functions.rst

Thank you for the hint! According to the documentation, I would still
choose the second option "Use `*sleep()` whenever possible", because
I want to prevent unecessary hrtimer work and interrupts.

>> +
>> +		/* check battery state */
>> +		if (ioread8(kbatt->base + KBATT_STATUS_REG) &
>> +		    KBATT_STATUS_BAT_OK)
>> +			kbatt->alarm = 0;
>> +		else
>> +			kbatt->alarm = 1;
>> +
>> +		/* switch load off */
>> +		iowrite8(0, kbatt->base + KBATT_CONTROL_REG);
>> +
>> +		kbatt->last_updated = jiffies;
>> +		kbatt->valid = true;
>> +	}
>> +
>> +	mutex_unlock(&kbatt->lock);
>> +
>> +	return kbatt->alarm;
>> +}
>> +
>> +static int kbatt_read(struct device *dev, enum hwmon_sensor_types type,
>> +		      u32 attr, int channel, long *val)
>> +{
>> +	struct kbatt *kbatt = dev_get_drvdata(dev);
>> +
>> +	if ((channel != 1) || (attr != hwmon_in_alarm))
>> +		return -EOPNOTSUPP;
> 
> This condition is never true.

I will remove that check.

>> +
>> +	*val = kbatt_alarm(kbatt);
>> +
>> +	return 0;
>> +}
>> +
>> +static umode_t kbatt_is_visible(const void *data, enum hwmon_sensor_types type,
>> +				u32 attr, int channel)
>> +{
>> +	if ((channel == 1) && (attr == hwmon_in_alarm))
>> +		return 0444;
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct hwmon_channel_info *kbatt_info[] = {
>> +	HWMON_CHANNEL_INFO(in,
>> +			   /* 0: dummy, skipped in is_visible */
> 
> Why?

For compatibility reasons, as the out of tree version of the driver did
start with index 1 and there is software which rely on that fact. But
I'm unsure if this is a valid argument for mainline code. Guenter Roeck
also commented that, so will discuss this in the other thread.

>> +			   HWMON_I_ALARM,
>> +			   /* 1: input alarm channel */
>> +			   HWMON_I_ALARM),
>> +	NULL
>> +};
>> +
>> +static const struct hwmon_ops kbatt_hwmon_ops = {
>> +	.is_visible = kbatt_is_visible,
>> +	.read = kbatt_read,
>> +};
>> +
>> +static const struct hwmon_chip_info kbatt_chip_info = {
>> +	.ops = &kbatt_hwmon_ops,
>> +	.info = kbatt_info,
>> +};
>> +
>> +static int kbatt_probe(struct auxiliary_device *auxdev,
>> +		       const struct auxiliary_device_id *id)
>> +{
>> +	struct device *dev = &auxdev->dev;
>> +	struct kbatt *kbatt;
>> +
>> +	kbatt = devm_kzalloc(dev, sizeof(struct kbatt), GFP_KERNEL);
> 
> sizeof(*kbatt)

I will change that.

>> +	if (!kbatt)
>> +		return -ENOMEM;
>> +	kbatt->auxdev = container_of(auxdev, struct keba_batt_auxdev, auxdev);
>> +	mutex_init(&kbatt->lock);
> 
> devm_mutex_init(), check the return value.

That is new to me. Seams to help with mutex debugging. I will use it.

>> +	auxiliary_set_drvdata(auxdev, kbatt);
> 
> Is this needed?

dev_get_drvdata() is used within kbatt_read().

>> +
>> +	kbatt->base = devm_ioremap_resource(dev, &kbatt->auxdev->io);
>> +	if (IS_ERR(kbatt->base))
>> +		return PTR_ERR(kbatt->base);
>> +
>> +	kbatt->hwmon_dev = devm_hwmon_device_register_with_info(dev, KBATT,
>> +							     kbatt,
>> +							     &kbatt_chip_info,
>> +							     NULL);
>> +	if (IS_ERR(kbatt->hwmon_dev))
>> +		return PTR_ERR(kbatt->hwmon_dev);
>> +
>> +	return 0;
> 
> Would be slightly shorter with 'return PTR_ERR_OR_ZERO()', but both
> variant are fine.

I will use it.

>> +}
>> +
>> +static void kbatt_remove(struct auxiliary_device *auxdev)
>> +{
>> +	auxiliary_set_drvdata(auxdev, NULL);
> 
> Unnecessary.

Then I can completely eliminate kbatt_remove(). I will remove it.

>> +}
>> +
>> +static const struct auxiliary_device_id kbatt_devtype_aux[] = {
>> +	{ .name = "keba.batt" },
>> +	{}
>> +};
>> +MODULE_DEVICE_TABLE(auxiliary, kbatt_devtype_aux);
>> +
>> +static struct auxiliary_driver kbatt_driver_aux = {
>> +	.name = KBATT,
>> +	.id_table = kbatt_devtype_aux,
>> +	.probe = kbatt_probe,
>> +	.remove = kbatt_remove,
>> +};
>> +module_auxiliary_driver(kbatt_driver_aux);
>> +
>> +MODULE_AUTHOR("Petar Bojanic <boja@keba.com>");
>> +MODULE_AUTHOR("Gerhard Engleder <eg@keba.com>");
>> +MODULE_DESCRIPTION("KEBA battery monitoring controller driver");
>> +MODULE_LICENSE("GPL");
>> -- 
>> 2.39.5
>>
>>
Gerhard Engleder March 9, 2025, 8:03 a.m. UTC | #4 
On 08.03.25 23:32, Guenter Roeck wrote:
> On 3/8/25 13:23, Gerhard Engleder wrote:
>> From: Gerhard Engleder <eg@keba.com>
>>
>> The KEBA battery monitoring controller is found in the system FPGA of
>> KEBA PLC devices. It puts a load on the coin cell battery to check the
>> state of the battery.
>>
>> Signed-off-by: Gerhard Engleder <eg@keba.com>
> 
> Looking into the keba code, that is kind of piece by piece approach.
> I see a reference to fans in drivers/misc/keba/cp500.c, so I guess I'll see
> a fan controller driver at some point in the future. I do not support
> the idea of having multiple drivers for the hardware monitoring
> functionality of a single device.

Yes, the fan controller will follow. The cp500 driver supports multiple
devices. All of them have the battery controller, but only some of them
have the fan controller. Fanless devices don't have a fan controller in
the FPGA. There are also devices with two fan controllers.

The battery controller and the fan controller are separate IP cores with
its own 4k address range (also I2C, SPI, ...). So I see them as separate
devices. There is also no guarantee if the address range of both
controllers is next to each other or not.

Does that help you to see the battery controller and the fan controller
as separate devices?

> Either case, the attribute needs to be documented.

You mean the documentation Documentation/hwmon/, which Thomas Weißschuh
also mentioned? I will add it.

> Some more technical comments inline.
> 
> Guenter
> 
>> ---
>>   drivers/hwmon/Kconfig  |  12 ++++
>>   drivers/hwmon/Makefile |   1 +
>>   drivers/hwmon/kbatt.c  | 159 +++++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 172 insertions(+)
>>   create mode 100644 drivers/hwmon/kbatt.c
>>
>> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
>> index 4cbaba15d86e..ec396252cc18 100644
>> --- a/drivers/hwmon/Kconfig
>> +++ b/drivers/hwmon/Kconfig
>> @@ -335,6 +335,18 @@ config SENSORS_K10TEMP
>>         This driver can also be built as a module. If so, the module
>>         will be called k10temp.
>> +config SENSORS_KBATT
>> +    tristate "KEBA battery controller support"
>> +    depends on HAS_IOMEM
>> +    depends on KEBA_CP500 || COMPILE_TEST
>> +    select AUXILIARY_BUS
>> +    help
>> +      This driver supports the battery monitoring controller found in
>> +      KEBA system FPGA devices.
>> +
>> +      This driver can also be built as a module. If so, the module
>> +      will be called kbatt.
>> +
>>   config SENSORS_FAM15H_POWER
>>       tristate "AMD Family 15h processor power"
>>       depends on X86 && PCI && CPU_SUP_AMD
>> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
>> index b7ef0f0562d3..4671a9b77b55 100644
>> --- a/drivers/hwmon/Makefile
>> +++ b/drivers/hwmon/Makefile
>> @@ -108,6 +108,7 @@ obj-$(CONFIG_SENSORS_IT87)    += it87.o
>>   obj-$(CONFIG_SENSORS_JC42)    += jc42.o
>>   obj-$(CONFIG_SENSORS_K8TEMP)    += k8temp.o
>>   obj-$(CONFIG_SENSORS_K10TEMP)    += k10temp.o
>> +obj-$(CONFIG_SENSORS_KBATT)    += kbatt.o
>>   obj-$(CONFIG_SENSORS_LAN966X)    += lan966x-hwmon.o
>>   obj-$(CONFIG_SENSORS_LENOVO_EC)    += lenovo-ec-sensors.o
>>   obj-$(CONFIG_SENSORS_LINEAGE)    += lineage-pem.o
>> diff --git a/drivers/hwmon/kbatt.c b/drivers/hwmon/kbatt.c
>> new file mode 100644
>> index 000000000000..09a622a7d36a
>> --- /dev/null
>> +++ b/drivers/hwmon/kbatt.c
>> @@ -0,0 +1,159 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (C) KEBA Industrial Automation Gmbh 2025
>> + *
>> + * Driver for KEBA battery monitoring controller FPGA IP core
>> + */
>> +
>> +#include <linux/hwmon.h>
>> +#include <linux/io.h>
>> +#include <linux/delay.h>
>> +#include <linux/module.h>
>> +#include <linux/misc/keba.h>
>> +
>> +#define KBATT "kbatt"
>> +
>> +#define KBATT_CONTROL_REG        0x4
>> +#define   KBATT_CONTROL_BAT_TEST    0x01
>> +
>> +#define KBATT_STATUS_REG        0x8
>> +#define   KBATT_STATUS_BAT_OK        0x01
>> +
>> +#define KBATT_MAX_UPD_INTERVAL    (5 * HZ)
>> +#define KBATT_SETTLE_TIME_MS    100
>> +
>> +struct kbatt {
>> +    struct keba_batt_auxdev *auxdev;
>> +    /* update lock */
>> +    struct mutex lock;
>> +    void __iomem *base;
>> +    struct device *hwmon_dev;
>> +
>> +    bool valid;
>> +    unsigned long last_updated; /* in jiffies */
>> +    long alarm;
> 
> bool

I will change it to bool.

>> +};
>> +
>> +static long kbatt_alarm(struct kbatt *kbatt)
>> +{
>> +    mutex_lock(&kbatt->lock);
>> +
>> +    if (time_after(jiffies, kbatt->last_updated + 
>> KBATT_MAX_UPD_INTERVAL) ||
>> +        !kbatt->valid) {
>> +        /* switch load on */
>> +        iowrite8(KBATT_CONTROL_BAT_TEST,
>> +             kbatt->base + KBATT_CONTROL_REG);
>> +
>> +        /* wait some time to let things settle */
>> +        msleep(KBATT_SETTLE_TIME_MS);
>> +
>> +        /* check battery state */
>> +        if (ioread8(kbatt->base + KBATT_STATUS_REG) &
>> +            KBATT_STATUS_BAT_OK)
>> +            kbatt->alarm = 0;
>> +        else
>> +            kbatt->alarm = 1;
>> +
>> +        /* switch load off */
>> +        iowrite8(0, kbatt->base + KBATT_CONTROL_REG);
>> +
>> +        kbatt->last_updated = jiffies;
>> +        kbatt->valid = true;
>> +    }
>> +
>> +    mutex_unlock(&kbatt->lock);
>> +
>> +    return kbatt->alarm;
>> +}
>> +
>> +static int kbatt_read(struct device *dev, enum hwmon_sensor_types type,
>> +              u32 attr, int channel, long *val)
>> +{
>> +    struct kbatt *kbatt = dev_get_drvdata(dev);
>> +
>> +    if ((channel != 1) || (attr != hwmon_in_alarm))
> 
> Various unnecessary ( ) here and below.

I will clean it up here and check the whole code for that.

>> +        return -EOPNOTSUPP;
>> +
>> +    *val = kbatt_alarm(kbatt);
>> +
>> +    return 0;
>> +}
>> +
>> +static umode_t kbatt_is_visible(const void *data, enum 
>> hwmon_sensor_types type,
>> +                u32 attr, int channel)
>> +{
>> +    if ((channel == 1) && (attr == hwmon_in_alarm))
>> +        return 0444;
>> +
>> +    return 0;
>> +}
>> +
>> +static const struct hwmon_channel_info *kbatt_info[] = {
>> +    HWMON_CHANNEL_INFO(in,
>> +               /* 0: dummy, skipped in is_visible */
>> +               HWMON_I_ALARM,
>> +               /* 1: input alarm channel */
>> +               HWMON_I_ALARM),
> 
> Not acceptable. The first voltage channel is channel 0, not 1.

I did that for compatibility reasons, as the out of tree version of the
driver did start with index 1 and there is software which rely on that
fact. I saw a similar dummy in ina3221.c, so I thought this is ok.

Usually out of tree code is no argument. So if it must start with 0,
then I will change it.


Thank you for the review!

Gerhard
Thomas Weißschuh March 9, 2025, 8:23 a.m. UTC | #5 
On 2025-03-09 08:38:06+0100, Gerhard Engleder wrote:
> On 08.03.25 23:23, Thomas Weißschuh wrote:
> > On 2025-03-08 22:23:46+0100, Gerhard Engleder wrote:

<snip>

> > > diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> > > index 4cbaba15d86e..ec396252cc18 100644
> > > --- a/drivers/hwmon/Kconfig
> > > +++ b/drivers/hwmon/Kconfig
> > > @@ -335,6 +335,18 @@ config SENSORS_K10TEMP
> > >   	  This driver can also be built as a module. If so, the module
> > >   	  will be called k10temp.
> > > +config SENSORS_KBATT
> > > +	tristate "KEBA battery controller support"
> > > +	depends on HAS_IOMEM
> > > +	depends on KEBA_CP500 || COMPILE_TEST
> > 
> > KEBA_CP500 already has a COMPILE_TEST variant.
> > Duplicating it here looks unnecessary.
> > Then the HAS_IOMEM and AUXILIARY_BUS references can go away.
> 
> With COMPILE_TEST here the driver can be compile tested individually.
> Is this property not worth it? But I can change it if needed.

COMPILE_TEST is meant to break dependencies on concrete platforms.
KEBA_CP500 itself is not a platform dependency.
The platform dependencies of KERBA_CP500 are already broken through
COMPILE_TEST.

> > > +	select AUXILIARY_BUS
> > > +	help
> > > +	  This driver supports the battery monitoring controller found in
> > > +	  KEBA system FPGA devices.
> > > +
> > > +	  This driver can also be built as a module. If so, the module
> > > +	  will be called kbatt.
> > > +
> > >   config SENSORS_FAM15H_POWER
> > >   	tristate "AMD Family 15h processor power"
> > >   	depends on X86 && PCI && CPU_SUP_AMD

<snip>

> > > + *
> > > + * Driver for KEBA battery monitoring controller FPGA IP core
> > > + */
> > > +
> > > +#include <linux/hwmon.h>
> > > +#include <linux/io.h>
> > > +#include <linux/delay.h>
> > > +#include <linux/module.h>
> > > +#include <linux/misc/keba.h>
> > 
> > #include <linux/auxiliary_bus.h>
> > #include <linux/device.h>
> 
> Do I really have to include them explicitly? They are included
> indirectly with linux/misc/keba.h.

You are using symbols from those headers in your own source code,
so there is a direct dependency on them.

> > #include <linux/mutex.h>

<snip>

> > > +
> > > +	bool valid;
> > > +	unsigned long last_updated; /* in jiffies */
> > > +	long alarm;
> > 
> > bool
> 
> I choose long to match the hwmon API, hwmon_ops->read. Does it need to
> be bool nevertheless?

hwmon_ops->read needs to deal with different kinds of attributes,
most of which need proper number support. Alarm is only a bool,
so the code specific to it can be simpler.

Guenter also mentioned it.

<snip>

> > > +		/* switch load on */
> > > +		iowrite8(KBATT_CONTROL_BAT_TEST,
> > > +			 kbatt->base + KBATT_CONTROL_REG);
> > > +
> > > +		/* wait some time to let things settle */
> > > +		msleep(KBATT_SETTLE_TIME_MS);
> > 
> > Could use the recommended fsleep():
> > Documentation/timers/delay_sleep_functions.rst
> 
> Thank you for the hint! According to the documentation, I would still
> choose the second option "Use `*sleep()` whenever possible", because
> I want to prevent unecessary hrtimer work and interrupts.

I read the docs as fsleep() being preferable.
The timer core should do the right thing to avoid unnecessary work.

<snip>

> > > +static const struct hwmon_channel_info *kbatt_info[] = {
> > > +	HWMON_CHANNEL_INFO(in,
> > > +			   /* 0: dummy, skipped in is_visible */
> > 
> > Why?
> 
> For compatibility reasons, as the out of tree version of the driver did
> start with index 1 and there is software which rely on that fact. But
> I'm unsure if this is a valid argument for mainline code. Guenter Roeck
> also commented that, so will discuss this in the other thread.

Ack, lets' discuss with Guenter.
However I don't think it's going to fly.

> > > +			   HWMON_I_ALARM,
> > > +			   /* 1: input alarm channel */
> > > +			   HWMON_I_ALARM),
> > > +	NULL
> > > +};

<snip>

> > > +	auxiliary_set_drvdata(auxdev, kbatt);
> > 
> > Is this needed?
> 
> dev_get_drvdata() is used within kbatt_read().

The dev_get_drvdata() in kbatt_read(), is used on the hwmon device, not
the aux device. The drvdata for that hwmon device is set in
devm_hwmon_device_register_with_info() below.

> 
> > > +
> > > +	kbatt->base = devm_ioremap_resource(dev, &kbatt->auxdev->io);
> > > +	if (IS_ERR(kbatt->base))
> > > +		return PTR_ERR(kbatt->base);
> > > +
> > > +	kbatt->hwmon_dev = devm_hwmon_device_register_with_info(dev, KBATT,
> > > +							     kbatt,
> > > +							     &kbatt_chip_info,
> > > +							     NULL);

<snip>
Gerhard Engleder March 9, 2025, 9:16 a.m. UTC | #6 
On 09.03.25 09:23, Thomas Weißschuh wrote:
> On 2025-03-09 08:38:06+0100, Gerhard Engleder wrote:
>> On 08.03.25 23:23, Thomas Weißschuh wrote:
>>> On 2025-03-08 22:23:46+0100, Gerhard Engleder wrote:
> 
> <snip>
> 
>>>> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
>>>> index 4cbaba15d86e..ec396252cc18 100644
>>>> --- a/drivers/hwmon/Kconfig
>>>> +++ b/drivers/hwmon/Kconfig
>>>> @@ -335,6 +335,18 @@ config SENSORS_K10TEMP
>>>>    	  This driver can also be built as a module. If so, the module
>>>>    	  will be called k10temp.
>>>> +config SENSORS_KBATT
>>>> +	tristate "KEBA battery controller support"
>>>> +	depends on HAS_IOMEM
>>>> +	depends on KEBA_CP500 || COMPILE_TEST
>>>
>>> KEBA_CP500 already has a COMPILE_TEST variant.
>>> Duplicating it here looks unnecessary.
>>> Then the HAS_IOMEM and AUXILIARY_BUS references can go away.
>>
>> With COMPILE_TEST here the driver can be compile tested individually.
>> Is this property not worth it? But I can change it if needed.
> 
> COMPILE_TEST is meant to break dependencies on concrete platforms.
> KEBA_CP500 itself is not a platform dependency.
> The platform dependencies of KERBA_CP500 are already broken through
> COMPILE_TEST.

Ok, I will change it.

>>>> + *
>>>> + * Driver for KEBA battery monitoring controller FPGA IP core
>>>> + */
>>>> +
>>>> +#include <linux/hwmon.h>
>>>> +#include <linux/io.h>
>>>> +#include <linux/delay.h>
>>>> +#include <linux/module.h>
>>>> +#include <linux/misc/keba.h>
>>>
>>> #include <linux/auxiliary_bus.h>
>>> #include <linux/device.h>
>>
>> Do I really have to include them explicitly? They are included
>> indirectly with linux/misc/keba.h.
> 
> You are using symbols from those headers in your own source code,
> so there is a direct dependency on them.

I will include them.

>>>> +
>>>> +	bool valid;
>>>> +	unsigned long last_updated; /* in jiffies */
>>>> +	long alarm;
>>>
>>> bool
>>
>> I choose long to match the hwmon API, hwmon_ops->read. Does it need to
>> be bool nevertheless?
> 
> hwmon_ops->read needs to deal with different kinds of attributes,
> most of which need proper number support. Alarm is only a bool,
> so the code specific to it can be simpler.
> 
> Guenter also mentioned it.

I will switch to bool.

> <snip>
> 
>>>> +		/* switch load on */
>>>> +		iowrite8(KBATT_CONTROL_BAT_TEST,
>>>> +			 kbatt->base + KBATT_CONTROL_REG);
>>>> +
>>>> +		/* wait some time to let things settle */
>>>> +		msleep(KBATT_SETTLE_TIME_MS);
>>>
>>> Could use the recommended fsleep():
>>> Documentation/timers/delay_sleep_functions.rst
>>
>> Thank you for the hint! According to the documentation, I would still
>> choose the second option "Use `*sleep()` whenever possible", because
>> I want to prevent unecessary hrtimer work and interrupts.
> 
> I read the docs as fsleep() being preferable.
> The timer core should do the right thing to avoid unnecessary work.

I read "Use `fsleep()` whenever _unsure_" and I'm sure that msleep() is
sufficient and I don't need hrtimer. But in this case fsleep() will end
up in msleep() anyway. I will switch to fsleep().

(...)

>>>> +	auxiliary_set_drvdata(auxdev, kbatt);
>>>
>>> Is this needed?
>>
>> dev_get_drvdata() is used within kbatt_read().
> 
> The dev_get_drvdata() in kbatt_read(), is used on the hwmon device, not
> the aux device. The drvdata for that hwmon device is set in
> devm_hwmon_device_register_with_info() below.

I will remove it.


Thank you for your detailed review! Will make the code much simpler.

Gerhard
Guenter Roeck March 9, 2025, 2:50 p.m. UTC | #7 
On 3/9/25 00:23, Thomas Weißschuh wrote:

>>>> +static const struct hwmon_channel_info *kbatt_info[] = {
>>>> +	HWMON_CHANNEL_INFO(in,
>>>> +			   /* 0: dummy, skipped in is_visible */
>>>
>>> Why?
>>
>> For compatibility reasons, as the out of tree version of the driver did
>> start with index 1 and there is software which rely on that fact. But
>> I'm unsure if this is a valid argument for mainline code. Guenter Roeck
>> also commented that, so will discuss this in the other thread.
> 
> Ack, lets' discuss with Guenter.
> However I don't think it's going to fly.

This kind of argument is often used by those who want to implement non-standard
code. Implement it out-of-tree first and then say "sorry, we have to do it,
the out-of-tree code does it and our userspace depends on it". That is completely
unacceptable. If that is what you want, and you are not willing to adjust your
userspace code, keep your code out of tree.

On top of that, I don't even know what the attribute means. An alarm attribute
is supposed to indicate that a value is out of range. The implementation suggests
that this is is not the case. What is "battery ok" ? Voltage out of range ?
Battery failed ? The term itself suggests that it may reflect a failure.
It might be a "fault" attribute, and even that would not be a good match.
I'll need to see the actual description to determine what if anything is
acceptable. It will most definitely not be in1_alarm.

Guenter
Guenter Roeck March 9, 2025, 3:04 p.m. UTC | #8 
On 3/9/25 00:03, Gerhard Engleder wrote:
> On 08.03.25 23:32, Guenter Roeck wrote:
>> On 3/8/25 13:23, Gerhard Engleder wrote:
>>> From: Gerhard Engleder <eg@keba.com>
>>>
>>> The KEBA battery monitoring controller is found in the system FPGA of
>>> KEBA PLC devices. It puts a load on the coin cell battery to check the
>>> state of the battery.
>>>
>>> Signed-off-by: Gerhard Engleder <eg@keba.com>
>>
>> Looking into the keba code, that is kind of piece by piece approach.
>> I see a reference to fans in drivers/misc/keba/cp500.c, so I guess I'll see
>> a fan controller driver at some point in the future. I do not support
>> the idea of having multiple drivers for the hardware monitoring
>> functionality of a single device.
> 
> Yes, the fan controller will follow. The cp500 driver supports multiple
> devices. All of them have the battery controller, but only some of them
> have the fan controller. Fanless devices don't have a fan controller in
> the FPGA. There are also devices with two fan controllers.
> 
> The battery controller and the fan controller are separate IP cores with
> its own 4k address range (also I2C, SPI, ...). So I see them as separate
> devices. There is also no guarantee if the address range of both
> controllers is next to each other or not.
> 
> Does that help you to see the battery controller and the fan controller
> as separate devices?
> 

Barely. At this point I am not convinced that this should be a hwmon driver
in the first place.

>> Either case, the attribute needs to be documented.
> 
> You mean the documentation Documentation/hwmon/, which Thomas Weißschuh
> also mentioned? I will add it.
> 

Yes.
...

>> Not acceptable. The first voltage channel is channel 0, not 1.
> 
> I did that for compatibility reasons, as the out of tree version of the
> driver did start with index 1 and there is software which rely on that
> fact. I saw a similar dummy in ina3221.c, so I thought this is ok.

Isn't this a nice thing in the Linux kernel: You can find almost everything
in there to use as precedence.

The ina3221 driver slipped this in when it was submitted and, yes, I didn't
notice. When it was converted to the with_info API, it was too late to
change it. That doesn't make it better, and it is most definitely not an
argument to make for a new driver doing the same.

Guenter
Guenter Roeck March 9, 2025, 3:22 p.m. UTC | #9 
On 3/9/25 01:16, Gerhard Engleder wrote:
> On 09.03.25 09:23, Thomas Weißschuh wrote:
>> On 2025-03-09 08:38:06+0100, Gerhard Engleder wrote:
>>> On 08.03.25 23:23, Thomas Weißschuh wrote:
>>>> On 2025-03-08 22:23:46+0100, Gerhard Engleder wrote:
>>
>> <snip>
>>
>>>>> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
>>>>> index 4cbaba15d86e..ec396252cc18 100644
>>>>> --- a/drivers/hwmon/Kconfig
>>>>> +++ b/drivers/hwmon/Kconfig
>>>>> @@ -335,6 +335,18 @@ config SENSORS_K10TEMP
>>>>>          This driver can also be built as a module. If so, the module
>>>>>          will be called k10temp.
>>>>> +config SENSORS_KBATT
>>>>> +    tristate "KEBA battery controller support"
>>>>> +    depends on HAS_IOMEM
>>>>> +    depends on KEBA_CP500 || COMPILE_TEST
>>>>
>>>> KEBA_CP500 already has a COMPILE_TEST variant.
>>>> Duplicating it here looks unnecessary.
>>>> Then the HAS_IOMEM and AUXILIARY_BUS references can go away.
>>>
>>> With COMPILE_TEST here the driver can be compile tested individually.
>>> Is this property not worth it? But I can change it if needed.
>>
>> COMPILE_TEST is meant to break dependencies on concrete platforms.
>> KEBA_CP500 itself is not a platform dependency.
>> The platform dependencies of KERBA_CP500 are already broken through
>> COMPILE_TEST.
> 
> Ok, I will change it.
> 

FWIW, all those COMPILE_TEST dependencies are pointless:

drivers/i2c/busses/Kconfig:     depends on KEBA_CP500 || COMPILE_TEST
drivers/misc/keba/Kconfig:      depends on KEBA_CP500 || COMPILE_TEST
drivers/spi/Kconfig:    depends on KEBA_CP500 || COMPILE_TEST

On top of that, both SPI_KSPI2 and I2C_KEBA select AUXILIARY_BUS
which is equally pointless because KEBA_CP500 already does that.
I2C_KEBA depends on HAS_IOMEM but I2C itself already depends on it.

It is also ... odd ... that KEBA_CP500 depends on I2C. So it isn't
possible to enable any of its sub-devices without also enabling I2C.
It is not immediately obvious why this would be necessary.

Guenter
Gerhard Engleder March 9, 2025, 8:57 p.m. UTC | #10 
On 09.03.25 15:50, Guenter Roeck wrote:
> On 3/9/25 00:23, Thomas Weißschuh wrote:
> 
>>>>> +static const struct hwmon_channel_info *kbatt_info[] = {
>>>>> +    HWMON_CHANNEL_INFO(in,
>>>>> +               /* 0: dummy, skipped in is_visible */
>>>>
>>>> Why?
>>>
>>> For compatibility reasons, as the out of tree version of the driver did
>>> start with index 1 and there is software which rely on that fact. But
>>> I'm unsure if this is a valid argument for mainline code. Guenter Roeck
>>> also commented that, so will discuss this in the other thread.
>>
>> Ack, lets' discuss with Guenter.
>> However I don't think it's going to fly.
> 
> This kind of argument is often used by those who want to implement non- 
> standard
> code. Implement it out-of-tree first and then say "sorry, we have to do it,
> the out-of-tree code does it and our userspace depends on it". That is 
> completely
> unacceptable. If that is what you want, and you are not willing to 
> adjust your
> userspace code, keep your code out of tree.

I'm sorry that I created the impression that I don't want to change
driver code and user space code. This is not the case, I'm will remove
that dummy and start with 0.

> On top of that, I don't even know what the attribute means. An alarm 
> attribute
> is supposed to indicate that a value is out of range. The implementation 
> suggests
> that this is is not the case. What is "battery ok" ? Voltage out of range ?
> Battery failed ? The term itself suggests that it may reflect a failure.
> It might be a "fault" attribute, and even that would not be a good match.
> I'll need to see the actual description to determine what if anything is
> acceptable. It will most definitely not be in1_alarm.

I will try to provide a clear picture in the other thread.

Gerhard
Gerhard Engleder March 9, 2025, 9:36 p.m. UTC | #11 
On 09.03.25 16:04, Guenter Roeck wrote:
> On 3/9/25 00:03, Gerhard Engleder wrote:
>> On 08.03.25 23:32, Guenter Roeck wrote:
>>> On 3/8/25 13:23, Gerhard Engleder wrote:
>>>> From: Gerhard Engleder <eg@keba.com>
>>>>
>>>> The KEBA battery monitoring controller is found in the system FPGA of
>>>> KEBA PLC devices. It puts a load on the coin cell battery to check the
>>>> state of the battery.
>>>>
>>>> Signed-off-by: Gerhard Engleder <eg@keba.com>
>>>
>>> Looking into the keba code, that is kind of piece by piece approach.
>>> I see a reference to fans in drivers/misc/keba/cp500.c, so I guess 
>>> I'll see
>>> a fan controller driver at some point in the future. I do not support
>>> the idea of having multiple drivers for the hardware monitoring
>>> functionality of a single device.
>>
>> Yes, the fan controller will follow. The cp500 driver supports multiple
>> devices. All of them have the battery controller, but only some of them
>> have the fan controller. Fanless devices don't have a fan controller in
>> the FPGA. There are also devices with two fan controllers.
>>
>> The battery controller and the fan controller are separate IP cores with
>> its own 4k address range (also I2C, SPI, ...). So I see them as separate
>> devices. There is also no guarantee if the address range of both
>> controllers is next to each other or not.
>>
>> Does that help you to see the battery controller and the fan controller
>> as separate devices?
>>
> 
> Barely. At this point I am not convinced that this should be a hwmon driver
> in the first place.

Here a more detailed description, which I would add to
Documentation/hwmon/ in the proper form:

The PLC devices use a coin cell battery for the RTC to keep the current
time. The goal is to provide information about the coin cell state to
user space. Actually the user space shall be informed that the coin cell
battery is nearly empty and needs to be replaced.

The coin cell must be tested actively to get to know if its nearly empty
or not. Therefore, a load is put on the coin cell and the resulting
voltage is evaluated. This evaluation is done by some hard wired analog
logic, which compares the voltage to a defined limit. If the voltage is
above the limit, then the coin cell is assumed to be ok. If the voltage
is below the limit, then the coin cell is nearly empty (or broken,
or removed, ...) and shall be replaced by a new one. The battery
controller allows to start the test of the coin cell and to get the
result if the voltage is above or below the limit. The actual voltage is
not available. Only the information if the voltage is below a limit is
available.

That's why I thought a voltage alarm is a good fit. But I'm not an
expert and I'm curious about your opinion.

(...)

>>> Not acceptable. The first voltage channel is channel 0, not 1.
>>
>> I did that for compatibility reasons, as the out of tree version of the
>> driver did start with index 1 and there is software which rely on that
>> fact. I saw a similar dummy in ina3221.c, so I thought this is ok.
> 
> Isn't this a nice thing in the Linux kernel: You can find almost everything
> in there to use as precedence.
> 
> The ina3221 driver slipped this in when it was submitted and, yes, I didn't
> notice. When it was converted to the with_info API, it was too late to
> change it. That doesn't make it better, and it is most definitely not an
> argument to make for a new driver doing the same.

I'm sorry, I only wanted to mention where the idea comes from. I will
start with channel 0.

Gerhard
Gerhard Engleder March 9, 2025, 10:04 p.m. UTC | #12 
On 09.03.25 16:22, Guenter Roeck wrote:
> On 3/9/25 01:16, Gerhard Engleder wrote:
>> On 09.03.25 09:23, Thomas Weißschuh wrote:
>>> On 2025-03-09 08:38:06+0100, Gerhard Engleder wrote:
>>>> On 08.03.25 23:23, Thomas Weißschuh wrote:
>>>>> On 2025-03-08 22:23:46+0100, Gerhard Engleder wrote:
>>>
>>> <snip>
>>>
>>>>>> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
>>>>>> index 4cbaba15d86e..ec396252cc18 100644
>>>>>> --- a/drivers/hwmon/Kconfig
>>>>>> +++ b/drivers/hwmon/Kconfig
>>>>>> @@ -335,6 +335,18 @@ config SENSORS_K10TEMP
>>>>>>          This driver can also be built as a module. If so, the module
>>>>>>          will be called k10temp.
>>>>>> +config SENSORS_KBATT
>>>>>> +    tristate "KEBA battery controller support"
>>>>>> +    depends on HAS_IOMEM
>>>>>> +    depends on KEBA_CP500 || COMPILE_TEST
>>>>>
>>>>> KEBA_CP500 already has a COMPILE_TEST variant.
>>>>> Duplicating it here looks unnecessary.
>>>>> Then the HAS_IOMEM and AUXILIARY_BUS references can go away.
>>>>
>>>> With COMPILE_TEST here the driver can be compile tested individually.
>>>> Is this property not worth it? But I can change it if needed.
>>>
>>> COMPILE_TEST is meant to break dependencies on concrete platforms.
>>> KEBA_CP500 itself is not a platform dependency.
>>> The platform dependencies of KERBA_CP500 are already broken through
>>> COMPILE_TEST.
>>
>> Ok, I will change it.
>>
> 
> FWIW, all those COMPILE_TEST dependencies are pointless:
> 
> drivers/i2c/busses/Kconfig:     depends on KEBA_CP500 || COMPILE_TEST
> drivers/misc/keba/Kconfig:      depends on KEBA_CP500 || COMPILE_TEST
> drivers/spi/Kconfig:    depends on KEBA_CP500 || COMPILE_TEST

Ok, I won't add them anymore.

> On top of that, both SPI_KSPI2 and I2C_KEBA select AUXILIARY_BUS
> which is equally pointless because KEBA_CP500 already does that.
> I2C_KEBA depends on HAS_IOMEM but I2C itself already depends on it.

I'm sorry, I didn't know that Kconfig must be strictly minimized.

> It is also ... odd ... that KEBA_CP500 depends on I2C. So it isn't
> possible to enable any of its sub-devices without also enabling I2C.
> It is not immediately obvious why this would be necessary.

The cp500 driver uses functions of the I2C subsystem to find a defined
EEPROM.

Gerhard
Guenter Roeck March 9, 2025, 10:37 p.m. UTC | #13 
On 3/9/25 15:04, Gerhard Engleder wrote:

>> FWIW, all those COMPILE_TEST dependencies are pointless:
>>
>> drivers/i2c/busses/Kconfig:     depends on KEBA_CP500 || COMPILE_TEST
>> drivers/misc/keba/Kconfig:      depends on KEBA_CP500 || COMPILE_TEST
>> drivers/spi/Kconfig:    depends on KEBA_CP500 || COMPILE_TEST
> 
> Ok, I won't add them anymore.
> 
>> On top of that, both SPI_KSPI2 and I2C_KEBA select AUXILIARY_BUS
>> which is equally pointless because KEBA_CP500 already does that.
>> I2C_KEBA depends on HAS_IOMEM but I2C itself already depends on it.
> 
> I'm sorry, I didn't know that Kconfig must be strictly minimized.
> 

It doesn't, it just has no value if it isn't, and it is to some degree
misleading. Following your logic, the unnecessary COMPILE_TEST don't
matter either. Of course, if there is some problem with those COMPILE_TEST
dependencies in the future, and someone will drop them, there will be
a bit of a surprise when the drivers are built anyway. _That_ is why one
would normally avoid redundant dependencies.

>> It is also ... odd ... that KEBA_CP500 depends on I2C. So it isn't
>> possible to enable any of its sub-devices without also enabling I2C.
>> It is not immediately obvious why this would be necessary.
> 
> The cp500 driver uses functions of the I2C subsystem to find a defined
> EEPROM.
> 

Ok.

Thanks,
Guenter
Guenter Roeck March 9, 2025, 11:22 p.m. UTC | #14 
On 3/9/25 14:36, Gerhard Engleder wrote:
> On 09.03.25 16:04, Guenter Roeck wrote:
>> On 3/9/25 00:03, Gerhard Engleder wrote:
>>> On 08.03.25 23:32, Guenter Roeck wrote:
>>>> On 3/8/25 13:23, Gerhard Engleder wrote:
>>>>> From: Gerhard Engleder <eg@keba.com>
>>>>>
>>>>> The KEBA battery monitoring controller is found in the system FPGA of
>>>>> KEBA PLC devices. It puts a load on the coin cell battery to check the
>>>>> state of the battery.
>>>>>
>>>>> Signed-off-by: Gerhard Engleder <eg@keba.com>
>>>>
>>>> Looking into the keba code, that is kind of piece by piece approach.
>>>> I see a reference to fans in drivers/misc/keba/cp500.c, so I guess I'll see
>>>> a fan controller driver at some point in the future. I do not support
>>>> the idea of having multiple drivers for the hardware monitoring
>>>> functionality of a single device.
>>>
>>> Yes, the fan controller will follow. The cp500 driver supports multiple
>>> devices. All of them have the battery controller, but only some of them
>>> have the fan controller. Fanless devices don't have a fan controller in
>>> the FPGA. There are also devices with two fan controllers.
>>>
>>> The battery controller and the fan controller are separate IP cores with
>>> its own 4k address range (also I2C, SPI, ...). So I see them as separate
>>> devices. There is also no guarantee if the address range of both
>>> controllers is next to each other or not.
>>>
>>> Does that help you to see the battery controller and the fan controller
>>> as separate devices?
>>>
>>
>> Barely. At this point I am not convinced that this should be a hwmon driver
>> in the first place.
> 
> Here a more detailed description, which I would add to
> Documentation/hwmon/ in the proper form:
> 
> The PLC devices use a coin cell battery for the RTC to keep the current
> time. The goal is to provide information about the coin cell state to
> user space. Actually the user space shall be informed that the coin cell
> battery is nearly empty and needs to be replaced.
> 
> The coin cell must be tested actively to get to know if its nearly empty
> or not. Therefore, a load is put on the coin cell and the resulting
> voltage is evaluated. This evaluation is done by some hard wired analog
> logic, which compares the voltage to a defined limit. If the voltage is
> above the limit, then the coin cell is assumed to be ok. If the voltage
> is below the limit, then the coin cell is nearly empty (or broken,
> or removed, ...) and shall be replaced by a new one. The battery
> controller allows to start the test of the coin cell and to get the
> result if the voltage is above or below the limit. The actual voltage is
> not available. Only the information if the voltage is below a limit is
> available.
> 
> That's why I thought a voltage alarm is a good fit. But I'm not an
> expert and I'm curious about your opinion.
> 

It is ok, though in0_min_alarm would be a better fit. And, yes, please
add the above to the documentation.

Given the above description, and the code itself, I'd be a bit concerned
that reading the value repeatedly will cause the battery to be drain faster
than necessary (otherwise it could be active all the time). If so, it might
make sense to reduce the frequency of such readings to, say, once a day.
 From the MAX6916 datasheet:

"The MAX6916 does not constantly monitor an attached battery because such
  monitoring would drastically reduce the life of the battery. As a result,
  the MAX6916 only tests the battery for 1s every 24hr"

Personally I'd have tried to rely on the rtc itself to read the battery
status (RTC_VL_READ ioctl), but of course not every rtc supports that,
and not every rtc driver supports actually reporting it. Just in case
the rtc in your system does support it, and the driver doesn't (such
as the above mentioned MAX6916), it might make sense to submit a patch
for the rtc driver and use that mechanism, since that would be a more
generic solution than relying on proprietary fpga functionality.

That is just a note; it is your call, obviously, to decide how and how
often to check the battery status.

Thanks,
Guenter
Gerhard Engleder March 10, 2025, 7:14 p.m. UTC | #15 
On 10.03.25 00:22, Guenter Roeck wrote:
> On 3/9/25 14:36, Gerhard Engleder wrote:
>> On 09.03.25 16:04, Guenter Roeck wrote:
>>> On 3/9/25 00:03, Gerhard Engleder wrote:
>>>> On 08.03.25 23:32, Guenter Roeck wrote:
>>>>> On 3/8/25 13:23, Gerhard Engleder wrote:
>>>>>> From: Gerhard Engleder <eg@keba.com>
>>>>>>
>>>>>> The KEBA battery monitoring controller is found in the system FPGA of
>>>>>> KEBA PLC devices. It puts a load on the coin cell battery to check 
>>>>>> the
>>>>>> state of the battery.
>>>>>>
>>>>>> Signed-off-by: Gerhard Engleder <eg@keba.com>
>>>>>
>>>>> Looking into the keba code, that is kind of piece by piece approach.
>>>>> I see a reference to fans in drivers/misc/keba/cp500.c, so I guess 
>>>>> I'll see
>>>>> a fan controller driver at some point in the future. I do not support
>>>>> the idea of having multiple drivers for the hardware monitoring
>>>>> functionality of a single device.
>>>>
>>>> Yes, the fan controller will follow. The cp500 driver supports multiple
>>>> devices. All of them have the battery controller, but only some of them
>>>> have the fan controller. Fanless devices don't have a fan controller in
>>>> the FPGA. There are also devices with two fan controllers.
>>>>
>>>> The battery controller and the fan controller are separate IP cores 
>>>> with
>>>> its own 4k address range (also I2C, SPI, ...). So I see them as 
>>>> separate
>>>> devices. There is also no guarantee if the address range of both
>>>> controllers is next to each other or not.
>>>>
>>>> Does that help you to see the battery controller and the fan controller
>>>> as separate devices?
>>>>
>>>
>>> Barely. At this point I am not convinced that this should be a hwmon 
>>> driver
>>> in the first place.
>>
>> Here a more detailed description, which I would add to
>> Documentation/hwmon/ in the proper form:
>>
>> The PLC devices use a coin cell battery for the RTC to keep the current
>> time. The goal is to provide information about the coin cell state to
>> user space. Actually the user space shall be informed that the coin cell
>> battery is nearly empty and needs to be replaced.
>>
>> The coin cell must be tested actively to get to know if its nearly empty
>> or not. Therefore, a load is put on the coin cell and the resulting
>> voltage is evaluated. This evaluation is done by some hard wired analog
>> logic, which compares the voltage to a defined limit. If the voltage is
>> above the limit, then the coin cell is assumed to be ok. If the voltage
>> is below the limit, then the coin cell is nearly empty (or broken,
>> or removed, ...) and shall be replaced by a new one. The battery
>> controller allows to start the test of the coin cell and to get the
>> result if the voltage is above or below the limit. The actual voltage is
>> not available. Only the information if the voltage is below a limit is
>> available.
>>
>> That's why I thought a voltage alarm is a good fit. But I'm not an
>> expert and I'm curious about your opinion.
>>
> 
> It is ok, though in0_min_alarm would be a better fit. And, yes, please
> add the above to the documentation.

I will take a look at in0_min_alarm. Thank you for your advice.

> Given the above description, and the code itself, I'd be a bit concerned
> that reading the value repeatedly will cause the battery to be drain faster
> than necessary (otherwise it could be active all the time). If so, it might
> make sense to reduce the frequency of such readings to, say, once a day.

You are right, this test would drain the battery too faster if done too
often. This is why the test is limited to every 5 seconds and this is
agreed with the electronics engineers. I will document that fact too.

> Personally I'd have tried to rely on the rtc itself to read the battery
> status (RTC_VL_READ ioctl), but of course not every rtc supports that,
> and not every rtc driver supports actually reporting it. Just in case
> the rtc in your system does support it, and the driver doesn't (such
> as the above mentioned MAX6916), it might make sense to submit a patch
> for the rtc driver and use that mechanism, since that would be a more
> generic solution than relying on proprietary fpga functionality.

That's possible for some designs yes. I will discuss that with the
electronics engineer for future designs with separate RTC chip.

Thank you for your feedback!

Gerhard
diff mbox series

Patch

diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 4cbaba15d86e..ec396252cc18 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -335,6 +335,18 @@  config SENSORS_K10TEMP
 	  This driver can also be built as a module. If so, the module
 	  will be called k10temp.
 
+config SENSORS_KBATT
+	tristate "KEBA battery controller support"
+	depends on HAS_IOMEM
+	depends on KEBA_CP500 || COMPILE_TEST
+	select AUXILIARY_BUS
+	help
+	  This driver supports the battery monitoring controller found in
+	  KEBA system FPGA devices.
+
+	  This driver can also be built as a module. If so, the module
+	  will be called kbatt.
+
 config SENSORS_FAM15H_POWER
 	tristate "AMD Family 15h processor power"
 	depends on X86 && PCI && CPU_SUP_AMD
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index b7ef0f0562d3..4671a9b77b55 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -108,6 +108,7 @@  obj-$(CONFIG_SENSORS_IT87)	+= it87.o
 obj-$(CONFIG_SENSORS_JC42)	+= jc42.o
 obj-$(CONFIG_SENSORS_K8TEMP)	+= k8temp.o
 obj-$(CONFIG_SENSORS_K10TEMP)	+= k10temp.o
+obj-$(CONFIG_SENSORS_KBATT)	+= kbatt.o
 obj-$(CONFIG_SENSORS_LAN966X)	+= lan966x-hwmon.o
 obj-$(CONFIG_SENSORS_LENOVO_EC)	+= lenovo-ec-sensors.o
 obj-$(CONFIG_SENSORS_LINEAGE)	+= lineage-pem.o
diff --git a/drivers/hwmon/kbatt.c b/drivers/hwmon/kbatt.c
new file mode 100644
index 000000000000..09a622a7d36a
--- /dev/null
+++ b/drivers/hwmon/kbatt.c
@@ -0,0 +1,159 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) KEBA Industrial Automation Gmbh 2025
+ *
+ * Driver for KEBA battery monitoring controller FPGA IP core
+ */
+
+#include <linux/hwmon.h>
+#include <linux/io.h>
+#include <linux/delay.h>
+#include <linux/module.h>
+#include <linux/misc/keba.h>
+
+#define KBATT "kbatt"
+
+#define KBATT_CONTROL_REG		0x4
+#define   KBATT_CONTROL_BAT_TEST	0x01
+
+#define KBATT_STATUS_REG		0x8
+#define   KBATT_STATUS_BAT_OK		0x01
+
+#define KBATT_MAX_UPD_INTERVAL	(5 * HZ)
+#define KBATT_SETTLE_TIME_MS	100
+
+struct kbatt {
+	struct keba_batt_auxdev *auxdev;
+	/* update lock */
+	struct mutex lock;
+	void __iomem *base;
+	struct device *hwmon_dev;
+
+	bool valid;
+	unsigned long last_updated; /* in jiffies */
+	long alarm;
+};
+
+static long kbatt_alarm(struct kbatt *kbatt)
+{
+	mutex_lock(&kbatt->lock);
+
+	if (time_after(jiffies, kbatt->last_updated + KBATT_MAX_UPD_INTERVAL) ||
+	    !kbatt->valid) {
+		/* switch load on */
+		iowrite8(KBATT_CONTROL_BAT_TEST,
+			 kbatt->base + KBATT_CONTROL_REG);
+
+		/* wait some time to let things settle */
+		msleep(KBATT_SETTLE_TIME_MS);
+
+		/* check battery state */
+		if (ioread8(kbatt->base + KBATT_STATUS_REG) &
+		    KBATT_STATUS_BAT_OK)
+			kbatt->alarm = 0;
+		else
+			kbatt->alarm = 1;
+
+		/* switch load off */
+		iowrite8(0, kbatt->base + KBATT_CONTROL_REG);
+
+		kbatt->last_updated = jiffies;
+		kbatt->valid = true;
+	}
+
+	mutex_unlock(&kbatt->lock);
+
+	return kbatt->alarm;
+}
+
+static int kbatt_read(struct device *dev, enum hwmon_sensor_types type,
+		      u32 attr, int channel, long *val)
+{
+	struct kbatt *kbatt = dev_get_drvdata(dev);
+
+	if ((channel != 1) || (attr != hwmon_in_alarm))
+		return -EOPNOTSUPP;
+
+	*val = kbatt_alarm(kbatt);
+
+	return 0;
+}
+
+static umode_t kbatt_is_visible(const void *data, enum hwmon_sensor_types type,
+				u32 attr, int channel)
+{
+	if ((channel == 1) && (attr == hwmon_in_alarm))
+		return 0444;
+
+	return 0;
+}
+
+static const struct hwmon_channel_info *kbatt_info[] = {
+	HWMON_CHANNEL_INFO(in,
+			   /* 0: dummy, skipped in is_visible */
+			   HWMON_I_ALARM,
+			   /* 1: input alarm channel */
+			   HWMON_I_ALARM),
+	NULL
+};
+
+static const struct hwmon_ops kbatt_hwmon_ops = {
+	.is_visible = kbatt_is_visible,
+	.read = kbatt_read,
+};
+
+static const struct hwmon_chip_info kbatt_chip_info = {
+	.ops = &kbatt_hwmon_ops,
+	.info = kbatt_info,
+};
+
+static int kbatt_probe(struct auxiliary_device *auxdev,
+		       const struct auxiliary_device_id *id)
+{
+	struct device *dev = &auxdev->dev;
+	struct kbatt *kbatt;
+
+	kbatt = devm_kzalloc(dev, sizeof(struct kbatt), GFP_KERNEL);
+	if (!kbatt)
+		return -ENOMEM;
+	kbatt->auxdev = container_of(auxdev, struct keba_batt_auxdev, auxdev);
+	mutex_init(&kbatt->lock);
+	auxiliary_set_drvdata(auxdev, kbatt);
+
+	kbatt->base = devm_ioremap_resource(dev, &kbatt->auxdev->io);
+	if (IS_ERR(kbatt->base))
+		return PTR_ERR(kbatt->base);
+
+	kbatt->hwmon_dev = devm_hwmon_device_register_with_info(dev, KBATT,
+							     kbatt,
+							     &kbatt_chip_info,
+							     NULL);
+	if (IS_ERR(kbatt->hwmon_dev))
+		return PTR_ERR(kbatt->hwmon_dev);
+
+	return 0;
+}
+
+static void kbatt_remove(struct auxiliary_device *auxdev)
+{
+	auxiliary_set_drvdata(auxdev, NULL);
+}
+
+static const struct auxiliary_device_id kbatt_devtype_aux[] = {
+	{ .name = "keba.batt" },
+	{}
+};
+MODULE_DEVICE_TABLE(auxiliary, kbatt_devtype_aux);
+
+static struct auxiliary_driver kbatt_driver_aux = {
+	.name = KBATT,
+	.id_table = kbatt_devtype_aux,
+	.probe = kbatt_probe,
+	.remove = kbatt_remove,
+};
+module_auxiliary_driver(kbatt_driver_aux);
+
+MODULE_AUTHOR("Petar Bojanic <boja@keba.com>");
+MODULE_AUTHOR("Gerhard Engleder <eg@keba.com>");
+MODULE_DESCRIPTION("KEBA battery monitoring controller driver");
+MODULE_LICENSE("GPL");