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@xxxxxxxx>
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@xxxxxxxx>
---
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@xxxxxxxx>");
+MODULE_AUTHOR("Gerhard Engleder <eg@xxxxxxxx>");
+MODULE_DESCRIPTION("KEBA battery monitoring controller driver");
+MODULE_LICENSE("GPL");
--
2.39.5
