[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <f6b09d06-6fc5-4032-a960-856e8cbd1574@t-8ch.de>
Date: Mon, 17 Mar 2025 07:40:51 +0100
From: Thomas Weißschuh <thomas@...ssschuh.net>
To: "Sung-Chi, Li" <lschyi@...omium.org>
Cc: Jean Delvare <jdelvare@...e.com>, Guenter Roeck <linux@...ck-us.net>,
Benson Leung <bleung@...omium.org>, Guenter Roeck <groeck@...omium.org>,
chrome-platform@...ts.linux.dev, linux-hwmon@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/3] hwmon: (cros_ec) Add reading target fan RPM function
On 2025-03-17 11:51:19+0800, Sung-Chi, Li wrote:
> On Thu, Mar 13, 2025 at 05:24:28PM +0100, Thomas Weißschuh wrote:
> > On 2025-03-13 12:47:43+0800, Sung-Chi Li wrote:
> > > Implement the functionality of reading the target fan RPM setting from
> > > ChromeOS embedded controller under framework.
> > >
> > > Signed-off-by: Sung-Chi Li <lschyi@...omium.org>
> > > ---
> > > drivers/hwmon/cros_ec_hwmon.c | 18 ++++++++++++++++++
> > > 1 file changed, 18 insertions(+)
> > >
> > > diff --git a/drivers/hwmon/cros_ec_hwmon.c b/drivers/hwmon/cros_ec_hwmon.c
> > > index b2fec0768301f116f49c57b8dbfb042b98a573e1..73bfcbbaf9531be6b753cfef8045fd5dab5b2ab3 100644
> > > --- a/drivers/hwmon/cros_ec_hwmon.c
> > > +++ b/drivers/hwmon/cros_ec_hwmon.c
> > > @@ -36,6 +36,19 @@ static int cros_ec_hwmon_read_fan_speed(struct cros_ec_device *cros_ec, u8 index
> > > return 0;
> > > }
> > >
> > > +static int cros_ec_hwmon_read_fan_target(struct cros_ec_device *cros_ec, u8 index, int32_t *speed)
> >
> > int32_t is a userspace type. In the kernel use i32, or even better u32.
> >
> Sorry for missing this important detail, I will not use userspace type
> for following changes.
No need to be sorry.
<snip>
> > > +
> > > + ret = cros_ec_cmd(cros_ec, 0, EC_CMD_PWM_GET_FAN_TARGET_RPM, NULL, 0, &r, sizeof(r));
> > > + if (ret < 0)
> > > + return ret;
> > > +
> > > + *speed = le32_to_cpu(r.rpm);
> >
> > r.rpm is not marked as __le32, I'm not sure if sparse will complain
> > about the usage of le32_to_cpu().
> >
> It did. Currently, all devices are running little endians on both AP and EC, so
> I think it is ok not to explicitly call the le32_to_cpu?
I think on big endian none of the CrOS EC code in Linux would work.
But as the driver currently already uses leXX_to_cpu() it would be nice
to keep using it consistently.
The nicest solution would be to change the definition of
struct ec_response_pwm_get_fan_rpm to use __le32.
Or add a cast: le32_to_cpu((__force __le32)r.rpm);
Powered by blists - more mailing lists