Hi Stephen,
On 2024-05-25 09:13:09+0000, Stephen Horvath wrote:
> I was the one to implement fan monitoring/control into Dustin's driver, and
> just had a quick comment for your driver:
>
> On 8/5/24 02:29, Thomas Weißschuh wrote:
> > The ChromeOS Embedded Controller exposes fan speed and temperature
> > readings.
> > Expose this data through the hwmon subsystem.
> >
> > The driver is designed to be probed via the cros_ec mfd device.
> >
> > Signed-off-by: Thomas Weißschuh <linux@xxxxxxxxxxxxxx>
> > ---
> > Documentation/hwmon/cros_ec_hwmon.rst | 26 ++++
> > Documentation/hwmon/index.rst | 1 +
> > MAINTAINERS | 8 +
> > drivers/hwmon/Kconfig | 11 ++
> > drivers/hwmon/Makefile | 1 +
> > drivers/hwmon/cros_ec_hwmon.c | 269 ++++++++++++++++++++++++++++++++++
> > 6 files changed, 316 insertions(+)
> >
<snip>
> > diff --git a/drivers/hwmon/cros_ec_hwmon.c b/drivers/hwmon/cros_ec_hwmon.c
> > new file mode 100644
> > index 000000000000..d59d39df2ac4
> > --- /dev/null
> > +++ b/drivers/hwmon/cros_ec_hwmon.c
> > @@ -0,0 +1,269 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * ChromesOS EC driver for hwmon
> > + *
> > + * Copyright (C) 2024 Thomas Weißschuh <linux@xxxxxxxxxxxxxx>
> > + */
> > +
> > +#include <linux/device.h>
> > +#include <linux/hwmon.h>
> > +#include <linux/kernel.h>
> > +#include <linux/mod_devicetable.h>
> > +#include <linux/module.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/platform_data/cros_ec_commands.h>
> > +#include <linux/platform_data/cros_ec_proto.h>
> > +#include <linux/units.h>
> > +
> > +#define DRV_NAME "cros-ec-hwmon"
> > +
> > +struct cros_ec_hwmon_priv {
> > + struct cros_ec_device *cros_ec;
> > + u8 thermal_version;
> > + const char *temp_sensor_names[EC_TEMP_SENSOR_ENTRIES + EC_TEMP_SENSOR_B_ENTRIES];
> > +};
> > +
> > +static int cros_ec_hwmon_read_fan_speed(struct cros_ec_device *cros_ec, u8 index, u16 *speed)
> > +{
> > + u16 data;
> > + int ret;
> > +
> > + ret = cros_ec->cmd_readmem(cros_ec, EC_MEMMAP_FAN + index * 2, 2, &data);
> > + if (ret < 0)
> > + return ret;
> > +
> > + data = le16_to_cpu(data);
> > +
> > + if (data == EC_FAN_SPEED_NOT_PRESENT)
> > + return -ENODEV;
> > +
>
> Don't forget it can also return `EC_FAN_SPEED_STALLED`.
Thanks for the hint. I'll need to think about how to handle this better.
> Like Guenter, I also don't like returning `-ENODEV`, but I don't have a
> problem with checking for `EC_FAN_SPEED_NOT_PRESENT` in case it was removed
> since init or something.
Ok.
> My approach was to return the speed as `0`, since the fan probably isn't
> spinning, but set HWMON_F_FAULT for `EC_FAN_SPEED_NOT_PRESENT` and
> HWMON_F_ALARM for `EC_FAN_SPEED_STALLED`.
> No idea if this is correct though.
I'm not a fan of returning a speed of 0 in case of errors.
Rather -EIO which can't be mistaken.
Maybe -EIO for both EC_FAN_SPEED_NOT_PRESENT (which should never happen)
and also for EC_FAN_SPEED_STALLED.
And EC_FAN_SPEED_STALLED also sets HWMON_F_FAULT.
HWMON_F_ALARM doesn't seem right to me.
But if Guenter has a preference, that will do, too.
>
> > + *speed = data;
> > + return 0;
> > +}
> > +
<snip>
> But feel free to ignore me if I'm completly wrong about this, since I really
> don't have much experience with kernel dev.
Thanks for your feedback!
Would you mind if I Cc you on further revisions?
Thomas
