Re: [PATCH v2 1/2] hwmon: add ChromeOS EC driver

[Thomas Weißschuh]

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