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

[Thomas Weißschuh]

Hi Guenter,

thanks for your quick responses!

On 2024-05-07 14:55:44+0000, Guenter Roeck wrote:
> On Tue, May 7, 2024 at 10:29 AM Thomas Weißschuh <linux@xxxxxxxxxxxxxx> 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(+)
> >

[..]

> > 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;
> > +
> I don't have time for a full review right now, but this looks wrong:
> If a fan is not present, its sysfs attribute should not be instantiated.
> Returning -ENODEV here is most definitely wrong.

This is also called from cros_ec_hwmon_is_visible(),

I think the special value should be handled in the read function
anyways, although it should never happen after probing.

Otherwise the magic, nonsensical value will be shown to the user as
sensor reading.

The sysfs hiding works.
Out of 4 fans and 24 temp sensors known by the driver,
on my device only 1 fan and 4 temp sensors exist and are probed.

If you prefer another error code please let me know.

Please let me know if I got something wrong.

> > +       *speed = data;
> > +       return 0;
> > +}
> > +
> > +static int cros_ec_hwmon_read_temp(struct cros_ec_device *cros_ec, u8 thermal_version,
> > +                                  u8 index, u8 *data)
> > +{
> > +       unsigned int offset;
> > +       int ret;
> > +
> > +       if (index < EC_TEMP_SENSOR_ENTRIES)
> > +               offset = EC_MEMMAP_TEMP_SENSOR + index;
> > +       else
> > +               offset = EC_MEMMAP_TEMP_SENSOR_B + index - EC_TEMP_SENSOR_ENTRIES;
> > +
> > +       ret = cros_ec->cmd_readmem(cros_ec, offset, 1, data);
> > +       if (ret < 0)
> > +               return ret;
> > +
> > +       if (*data == EC_TEMP_SENSOR_NOT_PRESENT ||
> > +           *data == EC_TEMP_SENSOR_ERROR ||
> > +           *data == EC_TEMP_SENSOR_NOT_POWERED ||
> > +           *data == EC_TEMP_SENSOR_NOT_CALIBRATED)
> > +               return -ENODEV;
> > +
> Same as above.

cros_ec_hwmon_probe()
  cros_ec_hwmon_probe_temp_sensors()
    cros_ec_hwmon_read_temp()

if _read_temp() fails here, the sensor name remains NULL and
is_visible() will hide that sensor.
      
As for the general reasoning, see above.

[..]


Thanks,
Thomas