| Message ID | 20250222161824.172511-7-lkml@antheas.dev (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | hwmon: (oxpsensors) Add devices, features, fix ABI and move to platform/x86 | expand |
On February 22, 2025 8:18:17 AM PST, Antheas Kapenekakis <lkml@antheas.dev> 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. > >As part of researching this topic, I verified that other OneXPlayer >devices do not have a turbo led, which makes this feature only >applicable to X1 and X1 mini devices. Antheas, Do you mean a turbo LED That can be set via EC? OXP devices have had an LED to indicate turbo all the way back to the 1S and mini AMD. I'm not sure if they can be set prior to X1, but this is incorrect as posted. >Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev> >--- > drivers/hwmon/oxp-sensors.c | 84 +++++++++++++++++++++++++++++++++++++ > 1 file changed, 84 insertions(+) > >diff --git a/drivers/hwmon/oxp-sensors.c b/drivers/hwmon/oxp-sensors.c >index 1c01636582d7..9c43ec0fc994 100644 >--- a/drivers/hwmon/oxp-sensors.c >+++ b/drivers/hwmon/oxp-sensors.c >@@ -101,6 +101,12 @@ static enum oxp_board board; > */ > #define OXP_X1_CHARGE_BYPASS_MASK_S3S5 0x02 > >+/* X1 Turbo LED */ >+#define OXP_X1_TURBO_LED_REG 0x57 >+ >+#define OXP_X1_TURBO_LED_OFF 0x01 >+#define OXP_X1_TURBO_LED_ON 0x02 >+ Not a blocker for me on this series, but we should consider looking at creating some enums in the future to capture functionality in a more concise way. There are quite a few define's at this point and enums offer a little bit of value validation. > enum charge_type_value_index { > CT_OFF, > CT_S0, >@@ -466,6 +472,73 @@ static ssize_t tt_toggle_show(struct device *dev, > > static DEVICE_ATTR_RW(tt_toggle); > >+/* Callbacks for turbo toggle 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; >+ int rval; >+ bool value; >+ >+ 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) >+ return rval; >+ >+ return count; >+} >+ >+static ssize_t tt_led_show(struct device *dev, >+ struct device_attribute *attr, char *buf) >+{ >+ int retval; >+ u8 reg; >+ long enval; >+ long val; >+ >+ switch (board) { >+ case oxp_x1: >+ reg = OXP_2_TURBO_SWITCH_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); >+ > /* Callbacks for turbo toggle attribute */ > static bool charge_control_supported(void) > { >@@ -894,8 +967,19 @@ static 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 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 > }; > - Derek
On Sat, 1 Mar 2025 at 16:14, Derek J. Clark <derekjohn.clark@gmail.com> wrote: > > > > On February 22, 2025 8:18:17 AM PST, Antheas Kapenekakis <lkml@antheas.dev> 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. > > > >As part of researching this topic, I verified that other OneXPlayer > >devices do not have a turbo led, which makes this feature only > >applicable to X1 and X1 mini devices. > > Antheas, > > Do you mean a turbo LED That can be set via EC? OXP devices have had an LED to indicate turbo all the way back to the 1S and mini AMD. I'm not sure if they can be set prior to X1, but this is incorrect as posted. Do not confuse the keyboard LED button with the turbo button. The X1 has two LEDs. Only the turbo one can be controlled. > >Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev> > >--- > > drivers/hwmon/oxp-sensors.c | 84 +++++++++++++++++++++++++++++++++++++ > > 1 file changed, 84 insertions(+) > > > >diff --git a/drivers/hwmon/oxp-sensors.c b/drivers/hwmon/oxp-sensors.c > >index 1c01636582d7..9c43ec0fc994 100644 > >--- a/drivers/hwmon/oxp-sensors.c > >+++ b/drivers/hwmon/oxp-sensors.c > >@@ -101,6 +101,12 @@ static enum oxp_board board; > > */ > > #define OXP_X1_CHARGE_BYPASS_MASK_S3S5 0x02 > > > >+/* X1 Turbo LED */ > >+#define OXP_X1_TURBO_LED_REG 0x57 > >+ > >+#define OXP_X1_TURBO_LED_OFF 0x01 > >+#define OXP_X1_TURBO_LED_ON 0x02 > >+ > > Not a blocker for me on this series, but we should consider looking at creating some enums in the future to capture functionality in a more concise way. There are quite a few define's at this point and enums offer a little bit of value validation. There should probably be a refactor in the future. Yes. I would tend towards using a driver struct... > > enum charge_type_value_index { > > CT_OFF, > > CT_S0, > >@@ -466,6 +472,73 @@ static ssize_t tt_toggle_show(struct device *dev, > > > > static DEVICE_ATTR_RW(tt_toggle); > > > >+/* Callbacks for turbo toggle 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; > >+ int rval; > >+ bool value; > >+ > >+ 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) > >+ return rval; > >+ > >+ return count; > >+} > >+ > >+static ssize_t tt_led_show(struct device *dev, > >+ struct device_attribute *attr, char *buf) > >+{ > >+ int retval; > >+ u8 reg; > >+ long enval; > >+ long val; > >+ > >+ switch (board) { > >+ case oxp_x1: > >+ reg = OXP_2_TURBO_SWITCH_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); > >+ > > /* Callbacks for turbo toggle attribute */ > > static bool charge_control_supported(void) > > { > >@@ -894,8 +967,19 @@ static 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 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 > > }; > > > - Derek
On March 1, 2025 7:54:22 AM PST, Antheas Kapenekakis <lkml@antheas.dev> wrote: >On Sat, 1 Mar 2025 at 16:14, Derek J. Clark <derekjohn.clark@gmail.com> wrote: >> >> >> >> On February 22, 2025 8:18:17 AM PST, Antheas Kapenekakis <lkml@antheas.dev> 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. >> > >> >As part of researching this topic, I verified that other OneXPlayer >> >devices do not have a turbo led, which makes this feature only >> >applicable to X1 and X1 mini devices. >> >> Antheas, >> >> Do you mean a turbo LED That can be set via EC? OXP devices have had an LED to indicate turbo all the way back to the 1S and mini AMD. I'm not sure if they can be set prior to X1, but this is incorrect as posted. > >Do not confuse the keyboard LED button with the turbo button. The X1 >has two LEDs. Only the turbo one can be controlled. > This would be pretty difficult to do. On the 1S it has the text TURBO. This video shows it at 09:40 https://youtu.be/AYrVKLD2J_k?si=bnwwQKY7MdqbbnCY It would be better to just point out that it became configurable on the X1. >> >Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev> >> >--- >> > drivers/hwmon/oxp-sensors.c | 84 +++++++++++++++++++++++++++++++++++++ >> > 1 file changed, 84 insertions(+) >> > >> >diff --git a/drivers/hwmon/oxp-sensors.c b/drivers/hwmon/oxp-sensors.c >> >index 1c01636582d7..9c43ec0fc994 100644 >> >--- a/drivers/hwmon/oxp-sensors.c >> >+++ b/drivers/hwmon/oxp-sensors.c >> >@@ -101,6 +101,12 @@ static enum oxp_board board; >> > */ >> > #define OXP_X1_CHARGE_BYPASS_MASK_S3S5 0x02 >> > >> >+/* X1 Turbo LED */ >> >+#define OXP_X1_TURBO_LED_REG 0x57 >> >+ >> >+#define OXP_X1_TURBO_LED_OFF 0x01 >> >+#define OXP_X1_TURBO_LED_ON 0x02 >> >+ >> >> Not a blocker for me on this series, but we should consider looking at creating some enums in the future to capture functionality in a more concise way. There are quite a few define's at this point and enums offer a little bit of value validation. > >There should probably be a refactor in the future. Yes. I would tend >towards using a driver struct... > >> > enum charge_type_value_index { >> > CT_OFF, >> > CT_S0, >> >@@ -466,6 +472,73 @@ static ssize_t tt_toggle_show(struct device *dev, >> > >> > static DEVICE_ATTR_RW(tt_toggle); >> > >> >+/* Callbacks for turbo toggle 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; >> >+ int rval; >> >+ bool value; >> >+ >> >+ 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) >> >+ return rval; >> >+ >> >+ return count; >> >+} >> >+ >> >+static ssize_t tt_led_show(struct device *dev, >> >+ struct device_attribute *attr, char *buf) >> >+{ >> >+ int retval; >> >+ u8 reg; >> >+ long enval; >> >+ long val; >> >+ >> >+ switch (board) { >> >+ case oxp_x1: >> >+ reg = OXP_2_TURBO_SWITCH_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); >> >+ >> > /* Callbacks for turbo toggle attribute */ >> > static bool charge_control_supported(void) >> > { >> >@@ -894,8 +967,19 @@ static 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 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 >> > }; >> > >> - Derek - Derek
On Sat, 1 Mar 2025 at 17:42, Derek J. Clark <derekjohn.clark@gmail.com> wrote: > > > > On March 1, 2025 7:54:22 AM PST, Antheas Kapenekakis <lkml@antheas.dev> wrote: > >On Sat, 1 Mar 2025 at 16:14, Derek J. Clark <derekjohn.clark@gmail.com> wrote: > >> > >> > >> > >> On February 22, 2025 8:18:17 AM PST, Antheas Kapenekakis <lkml@antheas.dev> 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. > >> > > >> >As part of researching this topic, I verified that other OneXPlayer > >> >devices do not have a turbo led, which makes this feature only > >> >applicable to X1 and X1 mini devices. > >> > >> Antheas, > >> > >> Do you mean a turbo LED That can be set via EC? OXP devices have had an LED to indicate turbo all the way back to the 1S and mini AMD. I'm not sure if they can be set prior to X1, but this is incorrect as posted. > > > >Do not confuse the keyboard LED button with the turbo button. The X1 > >has two LEDs. Only the turbo one can be controlled. > > > > This would be pretty difficult to do. On the 1S it has the text TURBO. This video shows it at 09:40 > > https://youtu.be/AYrVKLD2J_k?si=bnwwQKY7MdqbbnCY > > It would be better to just point out that it became configurable on the X1. I asked around and I think the OXP 2, MiniPro, and OneXFly models do not have it. I can edit the comment and say I added it for the X1. If you have an 1S, can you use rweverything and find the register for it? > >> >Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev> > >> >--- > >> > drivers/hwmon/oxp-sensors.c | 84 +++++++++++++++++++++++++++++++++++++ > >> > 1 file changed, 84 insertions(+) > >> > > >> >diff --git a/drivers/hwmon/oxp-sensors.c b/drivers/hwmon/oxp-sensors.c > >> >index 1c01636582d7..9c43ec0fc994 100644 > >> >--- a/drivers/hwmon/oxp-sensors.c > >> >+++ b/drivers/hwmon/oxp-sensors.c > >> >@@ -101,6 +101,12 @@ static enum oxp_board board; > >> > */ > >> > #define OXP_X1_CHARGE_BYPASS_MASK_S3S5 0x02 > >> > > >> >+/* X1 Turbo LED */ > >> >+#define OXP_X1_TURBO_LED_REG 0x57 > >> >+ > >> >+#define OXP_X1_TURBO_LED_OFF 0x01 > >> >+#define OXP_X1_TURBO_LED_ON 0x02 > >> >+ > >> > >> Not a blocker for me on this series, but we should consider looking at creating some enums in the future to capture functionality in a more concise way. There are quite a few define's at this point and enums offer a little bit of value validation. > > > >There should probably be a refactor in the future. Yes. I would tend > >towards using a driver struct... > > > >> > enum charge_type_value_index { > >> > CT_OFF, > >> > CT_S0, > >> >@@ -466,6 +472,73 @@ static ssize_t tt_toggle_show(struct device *dev, > >> > > >> > static DEVICE_ATTR_RW(tt_toggle); > >> > > >> >+/* Callbacks for turbo toggle 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; > >> >+ int rval; > >> >+ bool value; > >> >+ > >> >+ 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) > >> >+ return rval; > >> >+ > >> >+ return count; > >> >+} > >> >+ > >> >+static ssize_t tt_led_show(struct device *dev, > >> >+ struct device_attribute *attr, char *buf) > >> >+{ > >> >+ int retval; > >> >+ u8 reg; > >> >+ long enval; > >> >+ long val; > >> >+ > >> >+ switch (board) { > >> >+ case oxp_x1: > >> >+ reg = OXP_2_TURBO_SWITCH_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); > >> >+ > >> > /* Callbacks for turbo toggle attribute */ > >> > static bool charge_control_supported(void) > >> > { > >> >@@ -894,8 +967,19 @@ static 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 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 > >> > }; > >> > > >> - Derek > > - Derek
On March 1, 2025 8:52:38 AM PST, Antheas Kapenekakis <lkml@antheas.dev> wrote: >On Sat, 1 Mar 2025 at 17:42, Derek J. Clark <derekjohn.clark@gmail.com> wrote: >> >> >> >> On March 1, 2025 7:54:22 AM PST, Antheas Kapenekakis <lkml@antheas.dev> wrote: >> >On Sat, 1 Mar 2025 at 16:14, Derek J. Clark <derekjohn.clark@gmail.com> wrote: >> >> >> >> >> >> >> >> On February 22, 2025 8:18:17 AM PST, Antheas Kapenekakis <lkml@antheas.dev> 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. >> >> > >> >> >As part of researching this topic, I verified that other OneXPlayer >> >> >devices do not have a turbo led, which makes this feature only >> >> >applicable to X1 and X1 mini devices. >> >> >> >> Antheas, >> >> >> >> Do you mean a turbo LED That can be set via EC? OXP devices have had an LED to indicate turbo all the way back to the 1S and mini AMD. I'm not sure if they can be set prior to X1, but this is incorrect as posted. >> > >> >Do not confuse the keyboard LED button with the turbo button. The X1 >> >has two LEDs. Only the turbo one can be controlled. >> > >> >> This would be pretty difficult to do. On the 1S it has the text TURBO. This video shows it at 09:40 >> >> https://youtu.be/AYrVKLD2J_k?si=bnwwQKY7MdqbbnCY >> >> It would be better to just point out that it became configurable on the X1. > >I asked around and I think the OXP 2, MiniPro, and OneXFly models do >not have it. I can edit the comment and say I added it for the X1. > >If you have an 1S, can you use rweverything and find the register for it? Not necessary, 1S EC predates the one they started using in the Mini AMD and has no configurable options. The original mini AMD is also unlikely to support this as only my unit with an experimental BIOS nobody else installed can even do the takeover. X1+ should be fine. - Derek >> >> >Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev> >> >> >--- >> >> > drivers/hwmon/oxp-sensors.c | 84 +++++++++++++++++++++++++++++++++++++ >> >> > 1 file changed, 84 insertions(+) >> >> > >> >> >diff --git a/drivers/hwmon/oxp-sensors.c b/drivers/hwmon/oxp-sensors.c >> >> >index 1c01636582d7..9c43ec0fc994 100644 >> >> >--- a/drivers/hwmon/oxp-sensors.c >> >> >+++ b/drivers/hwmon/oxp-sensors.c >> >> >@@ -101,6 +101,12 @@ static enum oxp_board board; >> >> > */ >> >> > #define OXP_X1_CHARGE_BYPASS_MASK_S3S5 0x02 >> >> > >> >> >+/* X1 Turbo LED */ >> >> >+#define OXP_X1_TURBO_LED_REG 0x57 >> >> >+ >> >> >+#define OXP_X1_TURBO_LED_OFF 0x01 >> >> >+#define OXP_X1_TURBO_LED_ON 0x02 >> >> >+ >> >> >> >> Not a blocker for me on this series, but we should consider looking at creating some enums in the future to capture functionality in a more concise way. There are quite a few define's at this point and enums offer a little bit of value validation. >> > >> >There should probably be a refactor in the future. Yes. I would tend >> >towards using a driver struct... >> > >> >> > enum charge_type_value_index { >> >> > CT_OFF, >> >> > CT_S0, >> >> >@@ -466,6 +472,73 @@ static ssize_t tt_toggle_show(struct device *dev, >> >> > >> >> > static DEVICE_ATTR_RW(tt_toggle); >> >> > >> >> >+/* Callbacks for turbo toggle 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; >> >> >+ int rval; >> >> >+ bool value; >> >> >+ >> >> >+ 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) >> >> >+ return rval; >> >> >+ >> >> >+ return count; >> >> >+} >> >> >+ >> >> >+static ssize_t tt_led_show(struct device *dev, >> >> >+ struct device_attribute *attr, char *buf) >> >> >+{ >> >> >+ int retval; >> >> >+ u8 reg; >> >> >+ long enval; >> >> >+ long val; >> >> >+ >> >> >+ switch (board) { >> >> >+ case oxp_x1: >> >> >+ reg = OXP_2_TURBO_SWITCH_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); >> >> >+ >> >> > /* Callbacks for turbo toggle attribute */ >> >> > static bool charge_control_supported(void) >> >> > { >> >> >@@ -894,8 +967,19 @@ static 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 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 >> >> > }; >> >> > >> >> - Derek >> >> - Derek
diff --git a/drivers/hwmon/oxp-sensors.c b/drivers/hwmon/oxp-sensors.c index 1c01636582d7..9c43ec0fc994 100644 --- a/drivers/hwmon/oxp-sensors.c +++ b/drivers/hwmon/oxp-sensors.c @@ -101,6 +101,12 @@ static enum oxp_board board; */ #define OXP_X1_CHARGE_BYPASS_MASK_S3S5 0x02 +/* X1 Turbo LED */ +#define OXP_X1_TURBO_LED_REG 0x57 + +#define OXP_X1_TURBO_LED_OFF 0x01 +#define OXP_X1_TURBO_LED_ON 0x02 + enum charge_type_value_index { CT_OFF, CT_S0, @@ -466,6 +472,73 @@ static ssize_t tt_toggle_show(struct device *dev, static DEVICE_ATTR_RW(tt_toggle); +/* Callbacks for turbo toggle 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; + int rval; + bool value; + + 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) + return rval; + + return count; +} + +static ssize_t tt_led_show(struct device *dev, + struct device_attribute *attr, char *buf) +{ + int retval; + u8 reg; + long enval; + long val; + + switch (board) { + case oxp_x1: + reg = OXP_2_TURBO_SWITCH_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); + /* Callbacks for turbo toggle attribute */ static bool charge_control_supported(void) { @@ -894,8 +967,19 @@ static 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 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 };
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. As part of researching this topic, I verified that other OneXPlayer devices do not have a turbo led, which makes this feature only applicable to X1 and X1 mini devices. Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev> --- drivers/hwmon/oxp-sensors.c | 84 +++++++++++++++++++++++++++++++++++++ 1 file changed, 84 insertions(+)