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>
