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
