Re: [PATCH v8 07/14] platform/x86: oxpec: Add turbo led support to X1 devices

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 4091 bytes --]

On Sat, 22 Mar 2025, Antheas Kapenekakis wrote:

> The X1 and X1 mini lineups feature an LED nested within their turbo
> button. When turbo takeover is not enabled, the turbo button allows
> the device to switch from 18W to 25W TDP. When the device is in the
> 25W TDP mode, the LED is turned on.
> 
> However, when we engage turbo takeover, the turbo led remains on its
> last state, which might be illuminated and cannot be currently
> controlled. Therefore, add the register that controls it under sysfs,
> to allow userspace to turn it off once engaging turbo takeover and
> then control it as they wish.
> 
> 2024 OneXPlayer devices, other than the X1s, do not have a turbo LED.
> However, earlier models do, so this can be extended to them as well
> when the register for it is found.
> 
> Reviewed-by: Thomas Weißschuh <linux@weissschuh.net>
> Reviewed-by: Derek J. Clark <derekjohn.clark@gmail.com>
> Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
> ---
>  drivers/platform/x86/oxpec.c | 84 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 84 insertions(+)
> 
> diff --git a/drivers/platform/x86/oxpec.c b/drivers/platform/x86/oxpec.c
> index ee37070ec54fc..384d525402def 100644
> --- a/drivers/platform/x86/oxpec.c
> +++ b/drivers/platform/x86/oxpec.c
> @@ -87,6 +87,12 @@ static enum oxp_board board;
>  
>  #define OXP_TURBO_RETURN_VAL           0x00 /* Common return val */
>  
> +/* X1 Turbo LED */
> +#define OXP_X1_TURBO_LED_REG           0x57
> +
> +#define OXP_X1_TURBO_LED_OFF           0x01
> +#define OXP_X1_TURBO_LED_ON            0x02
> +
>  static const struct dmi_system_id dmi_table[] = {
>  	{
>  		.matches = {
> @@ -434,6 +440,73 @@ static ssize_t tt_toggle_show(struct device *dev,
>  
>  static DEVICE_ATTR_RW(tt_toggle);
>  
> +/* Callbacks for turbo LED attribute */
> +static umode_t tt_led_is_visible(struct kobject *kobj,
> +				    struct attribute *attr, int n)
> +{
> +	switch (board) {
> +	case oxp_x1:
> +		return attr->mode;
> +	default:
> +		break;
> +	}
> +	return 0;
> +}
> +
> +static ssize_t tt_led_store(struct device *dev,
> +			       struct device_attribute *attr, const char *buf,
> +			       size_t count)
> +{
> +	u8 reg, val;
> +	bool value;
> +	int rval;
> +
> +	rval = kstrtobool(buf, &value);
> +	if (rval)
> +		return rval;
> +
> +	switch (board) {
> +	case oxp_x1:
> +		reg = OXP_X1_TURBO_LED_REG;
> +		val = value ? OXP_X1_TURBO_LED_ON : OXP_X1_TURBO_LED_OFF;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +	rval = write_to_ec(reg, val);
> +
> +	if (rval)

Please don't put empty lines in between call and its error handling.

Please change "rval" to "ret".

(I'd add also a patch to convert the another rval to ret if it remains and 
change "retval" too to ret for consistency).

> +		return rval;
> +
> +	return count;
> +}
> +
> +static ssize_t tt_led_show(struct device *dev,
> +			      struct device_attribute *attr, char *buf)
> +{
> +	int retval;
> +	long enval;
> +	long val;
> +	u8 reg;
> +
> +	switch (board) {
> +	case oxp_x1:
> +		reg = OXP_X1_TURBO_LED_REG;
> +		enval = OXP_X1_TURBO_LED_ON;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	retval = read_from_ec(reg, 1, &val);
> +	if (retval)
> +		return retval;
> +
> +	return sysfs_emit(buf, "%d\n", val == enval);
> +}
> +
> +static DEVICE_ATTR_RW(tt_led);
> +
>  /* PWM enable/disable functions */
>  static int oxp_pwm_enable(void)
>  {
> @@ -691,8 +764,19 @@ static const struct attribute_group oxp_tt_toggle_attribute_group = {
>  	.attrs = oxp_tt_toggle_attrs,
>  };
>  
> +static struct attribute *oxp_tt_led_attrs[] = {
> +	&dev_attr_tt_led.attr,
> +	NULL
> +};
> +
> +static const struct attribute_group oxp_tt_led_attribute_group = {
> +	.is_visible = tt_led_is_visible,
> +	.attrs = oxp_tt_led_attrs,
> +};
> +
>  static const struct attribute_group *oxp_ec_groups[] = {
>  	&oxp_tt_toggle_attribute_group,
> +	&oxp_tt_led_attribute_group,
>  	NULL
>  };
>  
> 

-- 
 i.