Re: [PATCH v3 2/3] hwmon: (cros_ec) add PWM control over fans

[Guenter Roeck]

On 6/18/25 00:26, Sung-Chi Li wrote:
> On Mon, May 12, 2025 at 09:30:39AM +0200, Thomas Weißschuh wrote:
>
> Sorry for the late reply, I missed mails for this series.
>
> > On 2025-05-12 15:11:56+0800, Sung-Chi Li via B4 Relay wrote:
> > > From: Sung-Chi Li <lschyi@xxxxxxxxxxxx>
> > >   static int cros_ec_hwmon_read_temp(struct cros_ec_device *cros_ec, u8 index, u8 *temp)
> > >   {
> > >   	unsigned int offset;
> > > @@ -73,7 +117,9 @@ static long cros_ec_hwmon_temp_to_millicelsius(u8 temp)
> > >   static int cros_ec_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
> > >   			      u32 attr, int channel, long *val)
> > >   {
> > > +	u8 control_method;
> > >   	struct cros_ec_hwmon_priv *priv = dev_get_drvdata(dev);
> > > +	u8 pwm_value;
> > >   	int ret = -EOPNOTSUPP;
> > >   	u16 speed;
> > >   	u8 temp;
> >
> > Ordering again.
> >
> > This should be:
> >
> > struct cros_ec_hwmon_priv *priv = dev_get_drvdata(dev);
> > int ret = -EOPNOTSUPP;
> > u8 control_method;
> > u8 pwm_value;
> > u16 speed;
> > u8 temp;
> >
> > or:
> >
> > struct cros_ec_hwmon_priv *priv = dev_get_drvdata(dev);
> > u8 control_method, pwm_value, temp;
> > int ret = -EOPNOTSUPP;
> > u16 speed;
> >
> > <snip>
>
> Would you mind to share the sorting logic, so I do not bother you with checking
> these styling issue? Initially, I thought the sorting is based on the variable
> name, but after seeing your example (which I am appreciated), I am not sure how
> the sorting works. Is it sorted along with the variable types (
> "u8 control_method", "u16 speed", etc)? If that is the case, why "u16 speed" is
> in the middle of the u8 group variables?


It is called "reverse christmas tree". Longer lines first.

Guenter