From: "Thomas Weißschuh" <linux@weissschuh.net>
To: Guenter Roeck <groeck@google.com>
Cc: Jean Delvare <jdelvare@suse.com>,
Guenter Roeck <linux@roeck-us.net>,
Benson Leung <bleung@chromium.org>, Lee Jones <lee@kernel.org>,
Guenter Roeck <groeck@chromium.org>,
linux-kernel@vger.kernel.org, linux-hwmon@vger.kernel.org,
chrome-platform@lists.linux.dev,
Dustin Howett <dustin@howett.net>,
Mario Limonciello <mario.limonciello@amd.com>,
Moritz Fischer <mdf@kernel.org>
Subject: Re: [PATCH v2 1/2] hwmon: add ChromeOS EC driver
Date: Tue, 7 May 2024 23:59:00 +0200 [thread overview]
Message-ID: <572e968e-f1dc-4243-a895-9bc01b292a01@t-8ch.de> (raw)
In-Reply-To: <CABXOdTcabi6ab9FnfvR+3-q0sKQuk_sewD8ZT=80j5Ou+bHeKQ@mail.gmail.com>
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@weissschuh.net> 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@weissschuh.net>
> > ---
> > 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@weissschuh.net>
> > + */
> > +
> > +#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
next prev parent reply other threads:[~2024-05-07 21:59 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-05-07 16:29 [PATCH v2 0/2] ChromeOS Embedded controller hwmon driver Thomas Weißschuh
2024-05-07 16:29 ` [PATCH v2 1/2] hwmon: add ChromeOS EC driver Thomas Weißschuh
2024-05-07 20:55 ` Guenter Roeck
2024-05-07 21:59 ` Thomas Weißschuh [this message]
2024-05-07 16:29 ` [PATCH v2 2/2] mfd: cros_ec: Register hardware monitoring subdevice Thomas Weißschuh
2024-05-07 19:03 ` [PATCH v2 0/2] ChromeOS Embedded controller hwmon driver Mario Limonciello
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=572e968e-f1dc-4243-a895-9bc01b292a01@t-8ch.de \
--to=linux@weissschuh.net \
--cc=bleung@chromium.org \
--cc=chrome-platform@lists.linux.dev \
--cc=dustin@howett.net \
--cc=groeck@chromium.org \
--cc=groeck@google.com \
--cc=jdelvare@suse.com \
--cc=lee@kernel.org \
--cc=linux-hwmon@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux@roeck-us.net \
--cc=mario.limonciello@amd.com \
--cc=mdf@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).