On 08.03.25 23:32, Guenter Roeck wrote:
On 3/8/25 13:23, 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.
Signed-off-by: Gerhard Engleder <eg@xxxxxxxx>
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
