All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] platform/x86: thinkpad_acpi: Add support to detect hardware damage detection capability.
@ 2025-12-03  3:40 Nitin Joshi
  2025-12-03  7:52 ` Hans de Goede
  2025-12-03  9:44 ` Ilpo Järvinen
  0 siblings, 2 replies; 11+ messages in thread
From: Nitin Joshi @ 2025-12-03  3:40 UTC (permalink / raw)
  To: hansg, ilpo.jarvinen
  Cc: platform-driver-x86, linux-kernel, njoshi1, Nitin Joshi,
	Mark Pearson

Thinkpads are adding the ability to detect and report hardware damage
status. Add new sysfs interface to identify the impacted component
with status.
Initial support is available for the USB-C replaceable connector.

Reviewed-by: Mark Pearson <mpearson-lenovo@squebb.ca>
Signed-off-by: Nitin Joshi<nitjoshi@gmail.com>
---
 .../admin-guide/laptops/thinkpad-acpi.rst     |  26 +++
 drivers/platform/x86/lenovo/thinkpad_acpi.c   | 179 ++++++++++++++++++
 2 files changed, 205 insertions(+)

diff --git a/Documentation/admin-guide/laptops/thinkpad-acpi.rst b/Documentation/admin-guide/laptops/thinkpad-acpi.rst
index 4ab0fef7d440..4a3220529489 100644
--- a/Documentation/admin-guide/laptops/thinkpad-acpi.rst
+++ b/Documentation/admin-guide/laptops/thinkpad-acpi.rst
@@ -54,6 +54,7 @@ detailed description):
 	- Setting keyboard language
 	- WWAN Antenna type
 	- Auxmac
+	- Hardware damage detection capability
 
 A compatibility table by model and feature is maintained on the web
 site, http://ibm-acpi.sf.net/. I appreciate any success or failure
@@ -1576,6 +1577,31 @@ percentage level, above which charging will stop.
 The exact semantics of the attributes may be found in
 Documentation/ABI/testing/sysfs-class-power.
 
+Hardware damage detection capability
+-----------------
+
+sysfs attributes: hwdd_status
+
+Thinkpads are adding the ability to detect and report hardware damage.
+Add new sysfs interface to identify the impacted component.
+Initial support is available for the USB-C replaceable connector.
+
+The available commands are::
+
+        cat /sys/devices/platform/thinkpad_acpi/hwdd_status
+
+This value displays device type and location of device with damage status.
+For example:
+if no damage is detected:
+  No damage detected
+if damage detected:
+  Damage detected:
+  Device: TYPE-C
+  Location: Base, Right side, Center port
+
+The property is read-only. If feature is not supported then sysfs
+class is not created.
+
 Multiple Commands, Module Parameters
 ------------------------------------
 
diff --git a/drivers/platform/x86/lenovo/thinkpad_acpi.c b/drivers/platform/x86/lenovo/thinkpad_acpi.c
index cc19fe520ea9..c3629bed9a8e 100644
--- a/drivers/platform/x86/lenovo/thinkpad_acpi.c
+++ b/drivers/platform/x86/lenovo/thinkpad_acpi.c
@@ -11080,6 +11080,180 @@ static const struct attribute_group auxmac_attr_group = {
 	.attrs = auxmac_attributes,
 };
 
+/*************************************************************************
+ * HWDD subdriver, for the Lenovo Hardware Damage Detection feature.
+ */
+
+#define HWDD_GET_DMG_USBC 0x80000001
+#define HWDD_GET_CAP 0
+#define HWDD_NOT_SUPPORTED BIT(31)
+#define HWDD_SUPPORT_USBC BIT(0)
+#define HWDD_GET_CAP       0
+
+#define DAMAGE_STATUS_BIT  BIT(0)
+#define PORT_STATUS_OFFSET  4
+#define LID_STATUS_OFFSET  8
+#define BASE_STATUS_OFFSET 12
+#define PORT_DETAIL_OFFSET 16
+
+#define PORT_POS_OFFSET    2
+#define PORT_LOC_MASK      0x3
+
+#define PANEL_TOP   0
+#define PANEL_BASE  1
+#define PANEL_LEFT  2
+#define PANEL_RIGHT 3
+
+#define POS_LEFT    0
+#define POS_CENTER  1
+#define POS_RIGHT   2
+
+#define NUM_PORTS 4
+static bool hwdd_support_available;
+static bool ucdd_supported;
+static int hwdd_command(int command, int *output)
+{
+	acpi_handle hwdd_handle;
+
+	if (ACPI_FAILURE(acpi_get_handle(hkey_handle, "HWDD", &hwdd_handle)))
+		return -ENODEV;
+
+	if (!acpi_evalf(hwdd_handle, output, NULL, "dd", command))
+		return -EIO;
+
+	return 0;
+}
+
+static bool display_damage(char *buf, int *count, char *type, unsigned int dmg_status)
+{
+	unsigned char lid_status, base_status, port_status;
+	unsigned char loc_status, pos_status, panel_status;
+	bool damage_detected = false;
+	unsigned int i;
+
+	port_status = (dmg_status >> PORT_STATUS_OFFSET) & 0xF;
+	lid_status = (dmg_status >> LID_STATUS_OFFSET) & 0xF;
+	base_status = (dmg_status >> BASE_STATUS_OFFSET) & 0xF;
+	for (i = 0; i < NUM_PORTS; i++) {
+		if (dmg_status & BIT(i)) {
+			if (port_status & BIT(i)) {
+				*count += sysfs_emit_at(buf, *count, "%s damage detected:\n", type);
+				loc_status = (dmg_status >> (PORT_DETAIL_OFFSET + (4 * i))) & 0xF;
+				pos_status = (loc_status  >> PORT_POS_OFFSET) & PORT_LOC_MASK;
+				panel_status = loc_status & PORT_LOC_MASK;
+
+				*count += sysfs_emit_at(buf, *count, "Location: ");
+				if (lid_status & BIT(i))
+					*count += sysfs_emit_at(buf, *count, "Lid, ");
+				if (base_status & BIT(i))
+					*count += sysfs_emit_at(buf, *count, "Base, ");
+
+				switch (pos_status) {
+				case PANEL_TOP:
+					*count += sysfs_emit_at(buf, *count, "Top, ");
+					break;
+				case PANEL_BASE:
+					*count += sysfs_emit_at(buf, *count, "Bottom, ");
+					break;
+				case PANEL_LEFT:
+					*count += sysfs_emit_at(buf, *count, "Left, ");
+					break;
+				case PANEL_RIGHT:
+					*count += sysfs_emit_at(buf, *count, "Right, ");
+					break;
+				};
+
+				switch (panel_status) {
+				case POS_LEFT:
+					*count += sysfs_emit_at(buf, *count, "Left port\n");
+					break;
+				case POS_CENTER:
+					*count += sysfs_emit_at(buf, *count, "Center port\n");
+					break;
+				case POS_RIGHT:
+					*count += sysfs_emit_at(buf, *count, "Right port\n");
+					break;
+				default:
+					*count += sysfs_emit_at(buf, *count, "Undefined\n");
+					break;
+				};
+				damage_detected = true;
+			}
+		}
+	}
+	return damage_detected;
+}
+
+/* sysfs typc damage detection capability */
+static ssize_t hwdd_status_show(struct device *dev,
+				struct device_attribute *attr,
+				char *buf)
+{
+	bool damage_detected = false;
+	unsigned int damage_status;
+	int err, count = 0;
+
+	if (ucdd_supported) {
+		/* Get USB TYPE-C damage status */
+		err = hwdd_command(HWDD_GET_DMG_USBC, &damage_status);
+		if (err)
+			return err;
+
+		if (display_damage(buf, &count, "Type-C", damage_status))
+			damage_detected = true;
+	}
+
+	if (!damage_detected)
+		count  = count + sysfs_emit_at(buf, count, "No damage detected\n");
+
+	return count;
+}
+static DEVICE_ATTR_RO(hwdd_status);
+
+static struct attribute *hwdd_attributes[] = {
+	&dev_attr_hwdd_status.attr,
+	NULL
+};
+
+static umode_t hwdd_attr_is_visible(struct kobject *kobj,
+				struct attribute *attr, int n)
+{
+	return hwdd_support_available ? attr->mode : 0;
+}
+
+static const struct attribute_group hwdd_attr_group = {
+	.is_visible = hwdd_attr_is_visible,
+	.attrs = hwdd_attributes,
+};
+
+static int tpacpi_hwdd_init(struct ibm_init_struct *iibm)
+{
+	int err, output;
+
+	/* Below command checks the HWDD damage capability */
+	err = hwdd_command(HWDD_GET_CAP, &output);
+	if (err)
+		return err;
+
+	if (!(output & HWDD_NOT_SUPPORTED))
+		return -ENODEV;
+
+	hwdd_support_available = true;
+
+	/*
+	 * BIT(0) is assigned to check capability of damage detection is
+	 * supported for USB Type-C port or not.
+	 */
+	if (output & HWDD_SUPPORT_USBC)
+		ucdd_supported = true;
+
+	return err;
+}
+
+static struct ibm_struct hwdd_driver_data = {
+	.name = "hwdd",
+};
+
 /* --------------------------------------------------------------------- */
 
 static struct attribute *tpacpi_driver_attributes[] = {
@@ -11139,6 +11313,7 @@ static const struct attribute_group *tpacpi_groups[] = {
 	&kbdlang_attr_group,
 	&dprc_attr_group,
 	&auxmac_attr_group,
+	&hwdd_attr_group,
 	NULL,
 };
 
@@ -11752,6 +11927,10 @@ static struct ibm_init_struct ibms_init[] __initdata = {
 		.init = auxmac_init,
 		.data = &auxmac_data,
 	},
+	{
+		.init = tpacpi_hwdd_init,
+		.data = &hwdd_driver_data,
+	},
 };
 
 static int __init set_ibm_param(const char *val, const struct kernel_param *kp)
-- 
2.43.0


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH] platform/x86: thinkpad_acpi: Add support to detect hardware damage detection capability.
  2025-12-03  3:40 [PATCH] platform/x86: thinkpad_acpi: Add support to detect hardware damage detection capability Nitin Joshi
@ 2025-12-03  7:52 ` Hans de Goede
  2025-12-03 10:18   ` Nitin
  2025-12-03  9:44 ` Ilpo Järvinen
  1 sibling, 1 reply; 11+ messages in thread
From: Hans de Goede @ 2025-12-03  7:52 UTC (permalink / raw)
  To: Nitin Joshi, ilpo.jarvinen
  Cc: platform-driver-x86, linux-kernel, njoshi1, Mark Pearson

Hi,

Interesting new feature. A few small remarks on the proposed
sysfs API below.

On 3-Dec-25 4:40 AM, Nitin Joshi wrote:
> Thinkpads are adding the ability to detect and report hardware damage
> status. Add new sysfs interface to identify the impacted component
> with status.
> Initial support is available for the USB-C replaceable connector.
> 
> Reviewed-by: Mark Pearson <mpearson-lenovo@squebb.ca>
> Signed-off-by: Nitin Joshi<nitjoshi@gmail.com>
> ---
>  .../admin-guide/laptops/thinkpad-acpi.rst     |  26 +++
>  drivers/platform/x86/lenovo/thinkpad_acpi.c   | 179 ++++++++++++++++++
>  2 files changed, 205 insertions(+)
> 
> diff --git a/Documentation/admin-guide/laptops/thinkpad-acpi.rst b/Documentation/admin-guide/laptops/thinkpad-acpi.rst
> index 4ab0fef7d440..4a3220529489 100644
> --- a/Documentation/admin-guide/laptops/thinkpad-acpi.rst
> +++ b/Documentation/admin-guide/laptops/thinkpad-acpi.rst
> @@ -54,6 +54,7 @@ detailed description):
>  	- Setting keyboard language
>  	- WWAN Antenna type
>  	- Auxmac
> +	- Hardware damage detection capability
>  
>  A compatibility table by model and feature is maintained on the web
>  site, http://ibm-acpi.sf.net/. I appreciate any success or failure
> @@ -1576,6 +1577,31 @@ percentage level, above which charging will stop.
>  The exact semantics of the attributes may be found in
>  Documentation/ABI/testing/sysfs-class-power.
>  
> +Hardware damage detection capability
> +-----------------
> +
> +sysfs attributes: hwdd_status
> +
> +Thinkpads are adding the ability to detect and report hardware damage.
> +Add new sysfs interface to identify the impacted component.
> +Initial support is available for the USB-C replaceable connector.
> +
> +The available commands are::
> +
> +        cat /sys/devices/platform/thinkpad_acpi/hwdd_status
> +
> +This value displays device type and location of device with damage status.
> +For example:
> +if no damage is detected:
> +  No damage detected
> +if damage detected:
> +  Damage detected:
> +  Device: TYPE-C
> +  Location: Base, Right side, Center port
> +
> +The property is read-only. If feature is not supported then sysfs
> +class is not created.

Nitpick: s/class/attribute/ classes are standardized sysfs
interfaces living under /sys/class/ which this is not.

Besides the nitpick I'm wondering if we do not want to make
this a little bit more friendly / easy for software to parse ?

ATM this seems focused on a human directly reading
the output but what if we want some sort of automation,
like e.g. a Linux version of the Lenovo Vantage sw parsing
this in the future?

Note I've no specific suggestions for how to make this
easier to parse, this is just an observation.

Regards,

Hans



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] platform/x86: thinkpad_acpi: Add support to detect hardware damage detection capability.
  2025-12-03  3:40 [PATCH] platform/x86: thinkpad_acpi: Add support to detect hardware damage detection capability Nitin Joshi
  2025-12-03  7:52 ` Hans de Goede
@ 2025-12-03  9:44 ` Ilpo Järvinen
  2025-12-03 10:34   ` Nitin
  1 sibling, 1 reply; 11+ messages in thread
From: Ilpo Järvinen @ 2025-12-03  9:44 UTC (permalink / raw)
  To: Nitin Joshi
  Cc: Hans de Goede, platform-driver-x86, LKML, njoshi1, Mark Pearson

On Wed, 3 Dec 2025, Nitin Joshi wrote:

> Thinkpads are adding the ability to detect and report hardware damage
> status. Add new sysfs interface to identify the impacted component
> with status.
> Initial support is available for the USB-C replaceable connector.
> 
> Reviewed-by: Mark Pearson <mpearson-lenovo@squebb.ca>
> Signed-off-by: Nitin Joshi<nitjoshi@gmail.com>
> ---
>  .../admin-guide/laptops/thinkpad-acpi.rst     |  26 +++
>  drivers/platform/x86/lenovo/thinkpad_acpi.c   | 179 ++++++++++++++++++
>  2 files changed, 205 insertions(+)
> 
> diff --git a/Documentation/admin-guide/laptops/thinkpad-acpi.rst b/Documentation/admin-guide/laptops/thinkpad-acpi.rst
> index 4ab0fef7d440..4a3220529489 100644
> --- a/Documentation/admin-guide/laptops/thinkpad-acpi.rst
> +++ b/Documentation/admin-guide/laptops/thinkpad-acpi.rst
> @@ -54,6 +54,7 @@ detailed description):
>  	- Setting keyboard language
>  	- WWAN Antenna type
>  	- Auxmac
> +	- Hardware damage detection capability
>  
>  A compatibility table by model and feature is maintained on the web
>  site, http://ibm-acpi.sf.net/. I appreciate any success or failure
> @@ -1576,6 +1577,31 @@ percentage level, above which charging will stop.
>  The exact semantics of the attributes may be found in
>  Documentation/ABI/testing/sysfs-class-power.
>  
> +Hardware damage detection capability
> +-----------------
> +
> +sysfs attributes: hwdd_status
> +
> +Thinkpads are adding the ability to detect and report hardware damage.
> +Add new sysfs interface to identify the impacted component.
> +Initial support is available for the USB-C replaceable connector.
> +
> +The available commands are::
> +
> +        cat /sys/devices/platform/thinkpad_acpi/hwdd_status
> +
> +This value displays device type and location of device with damage status.
> +For example:
> +if no damage is detected:
> +  No damage detected
> +if damage detected:
> +  Damage detected:
> +  Device: TYPE-C
> +  Location: Base, Right side, Center port
> +
> +The property is read-only. If feature is not supported then sysfs
> +class is not created.
> +
>  Multiple Commands, Module Parameters
>  ------------------------------------
>  
> diff --git a/drivers/platform/x86/lenovo/thinkpad_acpi.c b/drivers/platform/x86/lenovo/thinkpad_acpi.c
> index cc19fe520ea9..c3629bed9a8e 100644
> --- a/drivers/platform/x86/lenovo/thinkpad_acpi.c
> +++ b/drivers/platform/x86/lenovo/thinkpad_acpi.c
> @@ -11080,6 +11080,180 @@ static const struct attribute_group auxmac_attr_group = {
>  	.attrs = auxmac_attributes,
>  };
>  
> +/*************************************************************************
> + * HWDD subdriver, for the Lenovo Hardware Damage Detection feature.
> + */
> +
> +#define HWDD_GET_DMG_USBC 0x80000001
> +#define HWDD_GET_CAP 0
> +#define HWDD_NOT_SUPPORTED BIT(31)
> +#define HWDD_SUPPORT_USBC BIT(0)

Align values.

> +#define HWDD_GET_CAP       0
> +
> +#define DAMAGE_STATUS_BIT  BIT(0)
> +#define PORT_STATUS_OFFSET  4
> +#define LID_STATUS_OFFSET  8
> +#define BASE_STATUS_OFFSET 12
> +#define PORT_DETAIL_OFFSET 16
> +
> +#define PORT_POS_OFFSET    2
> +#define PORT_LOC_MASK      0x3
> +
> +#define PANEL_TOP   0
> +#define PANEL_BASE  1
> +#define PANEL_LEFT  2
> +#define PANEL_RIGHT 3
> +
> +#define POS_LEFT    0
> +#define POS_CENTER  1
> +#define POS_RIGHT   2
> +
> +#define NUM_PORTS 4

Add an empty line

> +static bool hwdd_support_available;
> +static bool ucdd_supported;

Add an empty line

> +static int hwdd_command(int command, int *output)
> +{
> +	acpi_handle hwdd_handle;
> +
> +	if (ACPI_FAILURE(acpi_get_handle(hkey_handle, "HWDD", &hwdd_handle)))
> +		return -ENODEV;
> +
> +	if (!acpi_evalf(hwdd_handle, output, NULL, "dd", command))
> +		return -EIO;
> +
> +	return 0;
> +}
> +
> +static bool display_damage(char *buf, int *count, char *type, unsigned int dmg_status)
> +{
> +	unsigned char lid_status, base_status, port_status;
> +	unsigned char loc_status, pos_status, panel_status;
> +	bool damage_detected = false;
> +	unsigned int i;
> +
> +	port_status = (dmg_status >> PORT_STATUS_OFFSET) & 0xF;
> +	lid_status = (dmg_status >> LID_STATUS_OFFSET) & 0xF;
> +	base_status = (dmg_status >> BASE_STATUS_OFFSET) & 0xF;

Define these as fields using GENMASK() and use FIELD_GET() here. Don't 
forget to check you also have the includes for those.

> +	for (i = 0; i < NUM_PORTS; i++) {
> +		if (dmg_status & BIT(i)) {
> +			if (port_status & BIT(i)) {

Reverse the logic in these and use continue to bring down indentation 
level.

> +				*count += sysfs_emit_at(buf, *count, "%s damage detected:\n", type);
> +				loc_status = (dmg_status >> (PORT_DETAIL_OFFSET + (4 * i))) & 0xF;
> +				pos_status = (loc_status  >> PORT_POS_OFFSET) & PORT_LOC_MASK;

Extra space & use FIELD_GET().

> +				panel_status = loc_status & PORT_LOC_MASK;
> +
> +				*count += sysfs_emit_at(buf, *count, "Location: ");
> +				if (lid_status & BIT(i))
> +					*count += sysfs_emit_at(buf, *count, "Lid, ");
> +				if (base_status & BIT(i))
> +					*count += sysfs_emit_at(buf, *count, "Base, ");
> +
> +				switch (pos_status) {
> +				case PANEL_TOP:
> +					*count += sysfs_emit_at(buf, *count, "Top, ");
> +					break;
> +				case PANEL_BASE:
> +					*count += sysfs_emit_at(buf, *count, "Bottom, ");
> +					break;
> +				case PANEL_LEFT:
> +					*count += sysfs_emit_at(buf, *count, "Left, ");
> +					break;
> +				case PANEL_RIGHT:
> +					*count += sysfs_emit_at(buf, *count, "Right, ");
> +					break;

Some checkers likely won't be happy if you're not having default: at all, 
even if it is unnecessary here. So add something like this:

				default:
					WARN_ON(1);
					return ...;

> +				};
> +
> +				switch (panel_status) {
> +				case POS_LEFT:
> +					*count += sysfs_emit_at(buf, *count, "Left port\n");
> +					break;
> +				case POS_CENTER:
> +					*count += sysfs_emit_at(buf, *count, "Center port\n");
> +					break;
> +				case POS_RIGHT:
> +					*count += sysfs_emit_at(buf, *count, "Right port\n");
> +					break;
> +				default:
> +					*count += sysfs_emit_at(buf, *count, "Undefined\n");
> +					break;
> +				};
> +				damage_detected = true;
> +			}
> +		}
> +	}
> +	return damage_detected;
> +}
> +
> +/* sysfs typc damage detection capability */
> +static ssize_t hwdd_status_show(struct device *dev,
> +				struct device_attribute *attr,
> +				char *buf)
> +{
> +	bool damage_detected = false;
> +	unsigned int damage_status;
> +	int err, count = 0;
> +
> +	if (ucdd_supported) {
> +		/* Get USB TYPE-C damage status */
> +		err = hwdd_command(HWDD_GET_DMG_USBC, &damage_status);
> +		if (err)
> +			return err;
> +
> +		if (display_damage(buf, &count, "Type-C", damage_status))
> +			damage_detected = true;
> +	}
> +
> +	if (!damage_detected)
> +		count  = count + sysfs_emit_at(buf, count, "No damage detected\n");

Extra space, please also use += instead.

> +
> +	return count;
> +}
> +static DEVICE_ATTR_RO(hwdd_status);
> +
> +static struct attribute *hwdd_attributes[] = {
> +	&dev_attr_hwdd_status.attr,
> +	NULL
> +};
> +
> +static umode_t hwdd_attr_is_visible(struct kobject *kobj,
> +				struct attribute *attr, int n)
> +{
> +	return hwdd_support_available ? attr->mode : 0;
> +}
> +
> +static const struct attribute_group hwdd_attr_group = {
> +	.is_visible = hwdd_attr_is_visible,
> +	.attrs = hwdd_attributes,
> +};
> +
> +static int tpacpi_hwdd_init(struct ibm_init_struct *iibm)
> +{
> +	int err, output;
> +
> +	/* Below command checks the HWDD damage capability */
> +	err = hwdd_command(HWDD_GET_CAP, &output);
> +	if (err)
> +		return err;
> +
> +	if (!(output & HWDD_NOT_SUPPORTED))
> +		return -ENODEV;
> +
> +	hwdd_support_available = true;
> +
> +	/*
> +	 * BIT(0) is assigned to check capability of damage detection is
> +	 * supported for USB Type-C port or not.
> +	 */
> +	if (output & HWDD_SUPPORT_USBC)
> +		ucdd_supported = true;
> +
> +	return err;
> +}
> +
> +static struct ibm_struct hwdd_driver_data = {
> +	.name = "hwdd",
> +};
> +
>  /* --------------------------------------------------------------------- */
>  
>  static struct attribute *tpacpi_driver_attributes[] = {
> @@ -11139,6 +11313,7 @@ static const struct attribute_group *tpacpi_groups[] = {
>  	&kbdlang_attr_group,
>  	&dprc_attr_group,
>  	&auxmac_attr_group,
> +	&hwdd_attr_group,
>  	NULL,
>  };
>  
> @@ -11752,6 +11927,10 @@ static struct ibm_init_struct ibms_init[] __initdata = {
>  		.init = auxmac_init,
>  		.data = &auxmac_data,
>  	},
> +	{
> +		.init = tpacpi_hwdd_init,
> +		.data = &hwdd_driver_data,
> +	},
>  };
>  
>  static int __init set_ibm_param(const char *val, const struct kernel_param *kp)
> 

-- 
 i.


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] platform/x86: thinkpad_acpi: Add support to detect hardware damage detection capability.
  2025-12-03  7:52 ` Hans de Goede
@ 2025-12-03 10:18   ` Nitin
  2025-12-03 13:16     ` Mark Pearson
  0 siblings, 1 reply; 11+ messages in thread
From: Nitin @ 2025-12-03 10:18 UTC (permalink / raw)
  To: Hans de Goede, ilpo.jarvinen
  Cc: platform-driver-x86, linux-kernel, njoshi1, Mark Pearson

Hi Hans,

Thank you for reviewing this patch.

On 12/3/25 16:52, Hans de Goede wrote:
> Hi,
>
> Interesting new feature. A few small remarks on the proposed
> sysfs API below.
>
> On 3-Dec-25 4:40 AM, Nitin Joshi wrote:
>> Thinkpads are adding the ability to detect and report hardware damage
>> status. Add new sysfs interface to identify the impacted component
>> with status.
>> Initial support is available for the USB-C replaceable connector.
>>
>> Reviewed-by: Mark Pearson <mpearson-lenovo@squebb.ca>
>> Signed-off-by: Nitin Joshi<nitjoshi@gmail.com>
>> ---
>>   .../admin-guide/laptops/thinkpad-acpi.rst     |  26 +++
>>   drivers/platform/x86/lenovo/thinkpad_acpi.c   | 179 ++++++++++++++++++
>>   2 files changed, 205 insertions(+)
>>
>> diff --git a/Documentation/admin-guide/laptops/thinkpad-acpi.rst b/Documentation/admin-guide/laptops/thinkpad-acpi.rst
>> index 4ab0fef7d440..4a3220529489 100644
>> --- a/Documentation/admin-guide/laptops/thinkpad-acpi.rst
>> +++ b/Documentation/admin-guide/laptops/thinkpad-acpi.rst
>> @@ -54,6 +54,7 @@ detailed description):
>>   	- Setting keyboard language
>>   	- WWAN Antenna type
>>   	- Auxmac
>> +	- Hardware damage detection capability
>>   
>>   A compatibility table by model and feature is maintained on the web
>>   site, http://ibm-acpi.sf.net/. I appreciate any success or failure
>> @@ -1576,6 +1577,31 @@ percentage level, above which charging will stop.
>>   The exact semantics of the attributes may be found in
>>   Documentation/ABI/testing/sysfs-class-power.
>>   
>> +Hardware damage detection capability
>> +-----------------
>> +
>> +sysfs attributes: hwdd_status
>> +
>> +Thinkpads are adding the ability to detect and report hardware damage.
>> +Add new sysfs interface to identify the impacted component.
>> +Initial support is available for the USB-C replaceable connector.
>> +
>> +The available commands are::
>> +
>> +        cat /sys/devices/platform/thinkpad_acpi/hwdd_status
>> +
>> +This value displays device type and location of device with damage status.
>> +For example:
>> +if no damage is detected:
>> +  No damage detected
>> +if damage detected:
>> +  Damage detected:
>> +  Device: TYPE-C
>> +  Location: Base, Right side, Center port
>> +
>> +The property is read-only. If feature is not supported then sysfs
>> +class is not created.
> Nitpick: s/class/attribute/ classes are standardized sysfs
> interfaces living under /sys/class/ which this is not.
Ack, I will modify it.
>
> Besides the nitpick I'm wondering if we do not want to make
> this a little bit more friendly / easy for software to parse ?
>
> ATM this seems focused on a human directly reading
> the output but what if we want some sort of automation,
> like e.g. a Linux version of the Lenovo Vantage sw parsing
> this in the future?
>
> Note I've no specific suggestions for how to make this
> easier to parse, this is just an observation.

Thank you for pointing this out.  I am open for suggestions and will 
re-check regarding its use case in lenovo vantage in future.

>
> Regards,
>
> Hans
>
>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] platform/x86: thinkpad_acpi: Add support to detect hardware damage detection capability.
  2025-12-03  9:44 ` Ilpo Järvinen
@ 2025-12-03 10:34   ` Nitin
  2025-12-03 10:39     ` Ilpo Järvinen
  0 siblings, 1 reply; 11+ messages in thread
From: Nitin @ 2025-12-03 10:34 UTC (permalink / raw)
  To: Ilpo Järvinen
  Cc: Hans de Goede, platform-driver-x86, LKML, njoshi1, Mark Pearson

Hi Ilpo,

Thank you for reviewing the patch.

On 12/3/25 18:44, Ilpo Järvinen wrote:
> On Wed, 3 Dec 2025, Nitin Joshi wrote:
>
>> Thinkpads are adding the ability to detect and report hardware damage
>> status. Add new sysfs interface to identify the impacted component
>> with status.
>> Initial support is available for the USB-C replaceable connector.
>>
>> Reviewed-by: Mark Pearson <mpearson-lenovo@squebb.ca>
>> Signed-off-by: Nitin Joshi<nitjoshi@gmail.com>
>> ---
>>   .../admin-guide/laptops/thinkpad-acpi.rst     |  26 +++
>>   drivers/platform/x86/lenovo/thinkpad_acpi.c   | 179 ++++++++++++++++++
>>   2 files changed, 205 insertions(+)
>>
>> diff --git a/Documentation/admin-guide/laptops/thinkpad-acpi.rst b/Documentation/admin-guide/laptops/thinkpad-acpi.rst
>> index 4ab0fef7d440..4a3220529489 100644
>> --- a/Documentation/admin-guide/laptops/thinkpad-acpi.rst
>> +++ b/Documentation/admin-guide/laptops/thinkpad-acpi.rst
>> @@ -54,6 +54,7 @@ detailed description):
>>   	- Setting keyboard language
>>   	- WWAN Antenna type
>>   	- Auxmac
>> +	- Hardware damage detection capability
>>   
>>   A compatibility table by model and feature is maintained on the web
>>   site, http://ibm-acpi.sf.net/. I appreciate any success or failure
>> @@ -1576,6 +1577,31 @@ percentage level, above which charging will stop.
>>   The exact semantics of the attributes may be found in
>>   Documentation/ABI/testing/sysfs-class-power.
>>   
>> +Hardware damage detection capability
>> +-----------------
>> +
>> +sysfs attributes: hwdd_status
>> +
>> +Thinkpads are adding the ability to detect and report hardware damage.
>> +Add new sysfs interface to identify the impacted component.
>> +Initial support is available for the USB-C replaceable connector.
>> +
>> +The available commands are::
>> +
>> +        cat /sys/devices/platform/thinkpad_acpi/hwdd_status
>> +
>> +This value displays device type and location of device with damage status.
>> +For example:
>> +if no damage is detected:
>> +  No damage detected
>> +if damage detected:
>> +  Damage detected:
>> +  Device: TYPE-C
>> +  Location: Base, Right side, Center port
>> +
>> +The property is read-only. If feature is not supported then sysfs
>> +class is not created.
>> +
>>   Multiple Commands, Module Parameters
>>   ------------------------------------
>>   
>> diff --git a/drivers/platform/x86/lenovo/thinkpad_acpi.c b/drivers/platform/x86/lenovo/thinkpad_acpi.c
>> index cc19fe520ea9..c3629bed9a8e 100644
>> --- a/drivers/platform/x86/lenovo/thinkpad_acpi.c
>> +++ b/drivers/platform/x86/lenovo/thinkpad_acpi.c
>> @@ -11080,6 +11080,180 @@ static const struct attribute_group auxmac_attr_group = {
>>   	.attrs = auxmac_attributes,
>>   };
>>   
>> +/*************************************************************************
>> + * HWDD subdriver, for the Lenovo Hardware Damage Detection feature.
>> + */
>> +
>> +#define HWDD_GET_DMG_USBC 0x80000001
>> +#define HWDD_GET_CAP 0
>> +#define HWDD_NOT_SUPPORTED BIT(31)
>> +#define HWDD_SUPPORT_USBC BIT(0)
> Align values.

Ack, i will align values like below:

#define HWDD_GET_CAP 0
#define HWDD_GET_DMG_USBC 0x80000001
#define HWDD_SUPPORT_USBC BIT(0)
#define HWDD_NOT_SUPPORTED BIT(31)

>> +#define HWDD_GET_CAP       0
>> +
>> +#define DAMAGE_STATUS_BIT  BIT(0)
>> +#define PORT_STATUS_OFFSET  4
>> +#define LID_STATUS_OFFSET  8
>> +#define BASE_STATUS_OFFSET 12
>> +#define PORT_DETAIL_OFFSET 16
>> +
>> +#define PORT_POS_OFFSET    2
>> +#define PORT_LOC_MASK      0x3
>> +
>> +#define PANEL_TOP   0
>> +#define PANEL_BASE  1
>> +#define PANEL_LEFT  2
>> +#define PANEL_RIGHT 3
>> +
>> +#define POS_LEFT    0
>> +#define POS_CENTER  1
>> +#define POS_RIGHT   2
>> +
>> +#define NUM_PORTS 4
> Add an empty line
Ack.
>
>> +static bool hwdd_support_available;
>> +static bool ucdd_supported;
> Add an empty line
Ack.
>
>> +static int hwdd_command(int command, int *output)
>> +{
>> +	acpi_handle hwdd_handle;
>> +
>> +	if (ACPI_FAILURE(acpi_get_handle(hkey_handle, "HWDD", &hwdd_handle)))
>> +		return -ENODEV;
>> +
>> +	if (!acpi_evalf(hwdd_handle, output, NULL, "dd", command))
>> +		return -EIO;
>> +
>> +	return 0;
>> +}
>> +
>> +static bool display_damage(char *buf, int *count, char *type, unsigned int dmg_status)
>> +{
>> +	unsigned char lid_status, base_status, port_status;
>> +	unsigned char loc_status, pos_status, panel_status;
>> +	bool damage_detected = false;
>> +	unsigned int i;
>> +
>> +	port_status = (dmg_status >> PORT_STATUS_OFFSET) & 0xF;
>> +	lid_status = (dmg_status >> LID_STATUS_OFFSET) & 0xF;
>> +	base_status = (dmg_status >> BASE_STATUS_OFFSET) & 0xF;
> Define these as fields using GENMASK() and use FIELD_GET() here. Don't
> forget to check you also have the includes for those.
Ack. I will recheck and modify it.
>
>> +	for (i = 0; i < NUM_PORTS; i++) {
>> +		if (dmg_status & BIT(i)) {
>> +			if (port_status & BIT(i)) {
> Reverse the logic in these and use continue to bring down indentation
> level.
Ack. I will recheck and modify it.
>
>> +				*count += sysfs_emit_at(buf, *count, "%s damage detected:\n", type);
>> +				loc_status = (dmg_status >> (PORT_DETAIL_OFFSET + (4 * i))) & 0xF;
>> +				pos_status = (loc_status  >> PORT_POS_OFFSET) & PORT_LOC_MASK;
> Extra space & use FIELD_GET().
Ack.
>
>> +				panel_status = loc_status & PORT_LOC_MASK;
>> +
>> +				*count += sysfs_emit_at(buf, *count, "Location: ");
>> +				if (lid_status & BIT(i))
>> +					*count += sysfs_emit_at(buf, *count, "Lid, ");
>> +				if (base_status & BIT(i))
>> +					*count += sysfs_emit_at(buf, *count, "Base, ");
>> +
>> +				switch (pos_status) {
>> +				case PANEL_TOP:
>> +					*count += sysfs_emit_at(buf, *count, "Top, ");
>> +					break;
>> +				case PANEL_BASE:
>> +					*count += sysfs_emit_at(buf, *count, "Bottom, ");
>> +					break;
>> +				case PANEL_LEFT:
>> +					*count += sysfs_emit_at(buf, *count, "Left, ");
>> +					break;
>> +				case PANEL_RIGHT:
>> +					*count += sysfs_emit_at(buf, *count, "Right, ");
>> +					break;
> Some checkers likely won't be happy if you're not having default: at all,
> even if it is unnecessary here. So add something like this:
>
> 				default:
> 					WARN_ON(1);
> 					return ...;
Ack, I will recheck and modify it.
>> +				};
>> +
>> +				switch (panel_status) {
>> +				case POS_LEFT:
>> +					*count += sysfs_emit_at(buf, *count, "Left port\n");
>> +					break;
>> +				case POS_CENTER:
>> +					*count += sysfs_emit_at(buf, *count, "Center port\n");
>> +					break;
>> +				case POS_RIGHT:
>> +					*count += sysfs_emit_at(buf, *count, "Right port\n");
>> +					break;
>> +				default:
>> +					*count += sysfs_emit_at(buf, *count, "Undefined\n");
>> +					break;
>> +				};
>> +				damage_detected = true;
>> +			}
>> +		}
>> +	}
>> +	return damage_detected;
>> +}
>> +
>> +/* sysfs typc damage detection capability */
>> +static ssize_t hwdd_status_show(struct device *dev,
>> +				struct device_attribute *attr,
>> +				char *buf)
>> +{
>> +	bool damage_detected = false;
>> +	unsigned int damage_status;
>> +	int err, count = 0;
>> +
>> +	if (ucdd_supported) {
>> +		/* Get USB TYPE-C damage status */
>> +		err = hwdd_command(HWDD_GET_DMG_USBC, &damage_status);
>> +		if (err)
>> +			return err;
>> +
>> +		if (display_damage(buf, &count, "Type-C", damage_status))
>> +			damage_detected = true;
>> +	}
>> +
>> +	if (!damage_detected)
>> +		count  = count + sysfs_emit_at(buf, count, "No damage detected\n");
> Extra space, please also use += instead.
Ack.
>
>> +
>> +	return count;
>> +}
>> +static DEVICE_ATTR_RO(hwdd_status);
>> +
>> +static struct attribute *hwdd_attributes[] = {
>> +	&dev_attr_hwdd_status.attr,
>> +	NULL
>> +};
>> +
>> +static umode_t hwdd_attr_is_visible(struct kobject *kobj,
>> +				struct attribute *attr, int n)
>> +{
>> +	return hwdd_support_available ? attr->mode : 0;
>> +}
>> +
>> +static const struct attribute_group hwdd_attr_group = {
>> +	.is_visible = hwdd_attr_is_visible,
>> +	.attrs = hwdd_attributes,
>> +};
>> +
>> +static int tpacpi_hwdd_init(struct ibm_init_struct *iibm)
>> +{
>> +	int err, output;
>> +
>> +	/* Below command checks the HWDD damage capability */
>> +	err = hwdd_command(HWDD_GET_CAP, &output);
>> +	if (err)
>> +		return err;
>> +
>> +	if (!(output & HWDD_NOT_SUPPORTED))
>> +		return -ENODEV;
>> +
>> +	hwdd_support_available = true;
>> +
>> +	/*
>> +	 * BIT(0) is assigned to check capability of damage detection is
>> +	 * supported for USB Type-C port or not.
>> +	 */
>> +	if (output & HWDD_SUPPORT_USBC)
>> +		ucdd_supported = true;
>> +
>> +	return err;
>> +}
>> +
>> +static struct ibm_struct hwdd_driver_data = {
>> +	.name = "hwdd",
>> +};
>> +
>>   /* --------------------------------------------------------------------- */
>>   
>>   static struct attribute *tpacpi_driver_attributes[] = {
>> @@ -11139,6 +11313,7 @@ static const struct attribute_group *tpacpi_groups[] = {
>>   	&kbdlang_attr_group,
>>   	&dprc_attr_group,
>>   	&auxmac_attr_group,
>> +	&hwdd_attr_group,
>>   	NULL,
>>   };
>>   
>> @@ -11752,6 +11927,10 @@ static struct ibm_init_struct ibms_init[] __initdata = {
>>   		.init = auxmac_init,
>>   		.data = &auxmac_data,
>>   	},
>> +	{
>> +		.init = tpacpi_hwdd_init,
>> +		.data = &hwdd_driver_data,
>> +	},
>>   };
>>   
>>   static int __init set_ibm_param(const char *val, const struct kernel_param *kp)
>>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] platform/x86: thinkpad_acpi: Add support to detect hardware damage detection capability.
  2025-12-03 10:34   ` Nitin
@ 2025-12-03 10:39     ` Ilpo Järvinen
  2025-12-03 10:53       ` Nitin
  0 siblings, 1 reply; 11+ messages in thread
From: Ilpo Järvinen @ 2025-12-03 10:39 UTC (permalink / raw)
  To: Nitin; +Cc: Hans de Goede, platform-driver-x86, LKML, njoshi1, Mark Pearson

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

On Wed, 3 Dec 2025, Nitin wrote:

> Hi Ilpo,
> 
> Thank you for reviewing the patch.
> 
> On 12/3/25 18:44, Ilpo Järvinen wrote:
> > On Wed, 3 Dec 2025, Nitin Joshi wrote:
> > 
> > > Thinkpads are adding the ability to detect and report hardware damage
> > > status. Add new sysfs interface to identify the impacted component
> > > with status.
> > > Initial support is available for the USB-C replaceable connector.
> > > 
> > > Reviewed-by: Mark Pearson <mpearson-lenovo@squebb.ca>
> > > Signed-off-by: Nitin Joshi<nitjoshi@gmail.com>
> > > ---
> > >   .../admin-guide/laptops/thinkpad-acpi.rst     |  26 +++
> > >   drivers/platform/x86/lenovo/thinkpad_acpi.c   | 179 ++++++++++++++++++
> > >   2 files changed, 205 insertions(+)
> > > 
> > > diff --git a/Documentation/admin-guide/laptops/thinkpad-acpi.rst
> > > b/Documentation/admin-guide/laptops/thinkpad-acpi.rst
> > > index 4ab0fef7d440..4a3220529489 100644
> > > --- a/Documentation/admin-guide/laptops/thinkpad-acpi.rst
> > > +++ b/Documentation/admin-guide/laptops/thinkpad-acpi.rst
> > > @@ -54,6 +54,7 @@ detailed description):
> > >   	- Setting keyboard language
> > >   	- WWAN Antenna type
> > >   	- Auxmac
> > > +	- Hardware damage detection capability
> > >     A compatibility table by model and feature is maintained on the web
> > >   site, http://ibm-acpi.sf.net/. I appreciate any success or failure
> > > @@ -1576,6 +1577,31 @@ percentage level, above which charging will stop.
> > >   The exact semantics of the attributes may be found in
> > >   Documentation/ABI/testing/sysfs-class-power.
> > >   +Hardware damage detection capability
> > > +-----------------
> > > +
> > > +sysfs attributes: hwdd_status
> > > +
> > > +Thinkpads are adding the ability to detect and report hardware damage.
> > > +Add new sysfs interface to identify the impacted component.
> > > +Initial support is available for the USB-C replaceable connector.
> > > +
> > > +The available commands are::
> > > +
> > > +        cat /sys/devices/platform/thinkpad_acpi/hwdd_status
> > > +
> > > +This value displays device type and location of device with damage
> > > status.
> > > +For example:
> > > +if no damage is detected:
> > > +  No damage detected
> > > +if damage detected:
> > > +  Damage detected:
> > > +  Device: TYPE-C
> > > +  Location: Base, Right side, Center port
> > > +
> > > +The property is read-only. If feature is not supported then sysfs
> > > +class is not created.
> > > +
> > >   Multiple Commands, Module Parameters
> > >   ------------------------------------
> > >   diff --git a/drivers/platform/x86/lenovo/thinkpad_acpi.c
> > > b/drivers/platform/x86/lenovo/thinkpad_acpi.c
> > > index cc19fe520ea9..c3629bed9a8e 100644
> > > --- a/drivers/platform/x86/lenovo/thinkpad_acpi.c
> > > +++ b/drivers/platform/x86/lenovo/thinkpad_acpi.c
> > > @@ -11080,6 +11080,180 @@ static const struct attribute_group
> > > auxmac_attr_group = {
> > >   	.attrs = auxmac_attributes,
> > >   };
> > >   +/*************************************************************************
> > > + * HWDD subdriver, for the Lenovo Hardware Damage Detection feature.
> > > + */
> > > +
> > > +#define HWDD_GET_DMG_USBC 0x80000001
> > > +#define HWDD_GET_CAP 0
> > > +#define HWDD_NOT_SUPPORTED BIT(31)
> > > +#define HWDD_SUPPORT_USBC BIT(0)
> > Align values.
> 
> Ack, i will align values like below:
> 
> #define HWDD_GET_CAP 0
> #define HWDD_GET_DMG_USBC 0x80000001
> #define HWDD_SUPPORT_USBC BIT(0)
> #define HWDD_NOT_SUPPORTED BIT(31)

These are not aligned as values starts from different columns. Please 
put a tab char there in between so the values start at the same column (or 
varying number of tabs depending on the length of the name).

There are plenty of good examples in the code, including in this file 
you're modifying.

-- 
 i.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] platform/x86: thinkpad_acpi: Add support to detect hardware damage detection capability.
  2025-12-03 10:39     ` Ilpo Järvinen
@ 2025-12-03 10:53       ` Nitin
  0 siblings, 0 replies; 11+ messages in thread
From: Nitin @ 2025-12-03 10:53 UTC (permalink / raw)
  To: Ilpo Järvinen
  Cc: Hans de Goede, platform-driver-x86, LKML, njoshi1, Mark Pearson


On 12/3/25 19:39, Ilpo Järvinen wrote:
> On Wed, 3 Dec 2025, Nitin wrote:
>
>> Hi Ilpo,
>>
>> Thank you for reviewing the patch.
>>
>> On 12/3/25 18:44, Ilpo Järvinen wrote:
>>> On Wed, 3 Dec 2025, Nitin Joshi wrote:
>>>
>>>> Thinkpads are adding the ability to detect and report hardware damage
>>>> status. Add new sysfs interface to identify the impacted component
>>>> with status.
>>>> Initial support is available for the USB-C replaceable connector.
>>>>
>>>> Reviewed-by: Mark Pearson <mpearson-lenovo@squebb.ca>
>>>> Signed-off-by: Nitin Joshi<nitjoshi@gmail.com>
>>>> ---
>>>>    .../admin-guide/laptops/thinkpad-acpi.rst     |  26 +++
>>>>    drivers/platform/x86/lenovo/thinkpad_acpi.c   | 179 ++++++++++++++++++
>>>>    2 files changed, 205 insertions(+)
>>>>
>>>> diff --git a/Documentation/admin-guide/laptops/thinkpad-acpi.rst
>>>> b/Documentation/admin-guide/laptops/thinkpad-acpi.rst
>>>> index 4ab0fef7d440..4a3220529489 100644
>>>> --- a/Documentation/admin-guide/laptops/thinkpad-acpi.rst
>>>> +++ b/Documentation/admin-guide/laptops/thinkpad-acpi.rst
>>>> @@ -54,6 +54,7 @@ detailed description):
>>>>    	- Setting keyboard language
>>>>    	- WWAN Antenna type
>>>>    	- Auxmac
>>>> +	- Hardware damage detection capability
>>>>      A compatibility table by model and feature is maintained on the web
>>>>    site, http://ibm-acpi.sf.net/. I appreciate any success or failure
>>>> @@ -1576,6 +1577,31 @@ percentage level, above which charging will stop.
>>>>    The exact semantics of the attributes may be found in
>>>>    Documentation/ABI/testing/sysfs-class-power.
>>>>    +Hardware damage detection capability
>>>> +-----------------
>>>> +
>>>> +sysfs attributes: hwdd_status
>>>> +
>>>> +Thinkpads are adding the ability to detect and report hardware damage.
>>>> +Add new sysfs interface to identify the impacted component.
>>>> +Initial support is available for the USB-C replaceable connector.
>>>> +
>>>> +The available commands are::
>>>> +
>>>> +        cat /sys/devices/platform/thinkpad_acpi/hwdd_status
>>>> +
>>>> +This value displays device type and location of device with damage
>>>> status.
>>>> +For example:
>>>> +if no damage is detected:
>>>> +  No damage detected
>>>> +if damage detected:
>>>> +  Damage detected:
>>>> +  Device: TYPE-C
>>>> +  Location: Base, Right side, Center port
>>>> +
>>>> +The property is read-only. If feature is not supported then sysfs
>>>> +class is not created.
>>>> +
>>>>    Multiple Commands, Module Parameters
>>>>    ------------------------------------
>>>>    diff --git a/drivers/platform/x86/lenovo/thinkpad_acpi.c
>>>> b/drivers/platform/x86/lenovo/thinkpad_acpi.c
>>>> index cc19fe520ea9..c3629bed9a8e 100644
>>>> --- a/drivers/platform/x86/lenovo/thinkpad_acpi.c
>>>> +++ b/drivers/platform/x86/lenovo/thinkpad_acpi.c
>>>> @@ -11080,6 +11080,180 @@ static const struct attribute_group
>>>> auxmac_attr_group = {
>>>>    	.attrs = auxmac_attributes,
>>>>    };
>>>>    +/*************************************************************************
>>>> + * HWDD subdriver, for the Lenovo Hardware Damage Detection feature.
>>>> + */
>>>> +
>>>> +#define HWDD_GET_DMG_USBC 0x80000001
>>>> +#define HWDD_GET_CAP 0
>>>> +#define HWDD_NOT_SUPPORTED BIT(31)
>>>> +#define HWDD_SUPPORT_USBC BIT(0)
>>> Align values.
>> Ack, i will align values like below:
>>
>> #define HWDD_GET_CAP 0
>> #define HWDD_GET_DMG_USBC 0x80000001
>> #define HWDD_SUPPORT_USBC BIT(0)
>> #define HWDD_NOT_SUPPORTED BIT(31)
> These are not aligned as values starts from different columns. Please
> put a tab char there in between so the values start at the same column (or
> varying number of tabs depending on the length of the name).
>
> There are plenty of good examples in the code, including in this file
> you're modifying.
Ack, i will modify it . Thank you !
>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] platform/x86: thinkpad_acpi: Add support to detect hardware damage detection capability.
  2025-12-03 10:18   ` Nitin
@ 2025-12-03 13:16     ` Mark Pearson
  2025-12-08  7:19       ` Nitin
  0 siblings, 1 reply; 11+ messages in thread
From: Mark Pearson @ 2025-12-03 13:16 UTC (permalink / raw)
  To: Nitin Joshi, Hans de Goede, Ilpo Järvinen
  Cc: platform-driver-x86@vger.kernel.org, linux-kernel, Nitin Joshi1

On Wed, Dec 3, 2025, at 5:18 AM, Nitin wrote:
> Hi Hans,
>
> Thank you for reviewing this patch.
>
> On 12/3/25 16:52, Hans de Goede wrote:
>> Hi,
>>
>> Interesting new feature. A few small remarks on the proposed
>> sysfs API below.
>>
>> On 3-Dec-25 4:40 AM, Nitin Joshi wrote:
>>> Thinkpads are adding the ability to detect and report hardware damage
>>> status. Add new sysfs interface to identify the impacted component
>>> with status.
>>> Initial support is available for the USB-C replaceable connector.
>>>
>>> Reviewed-by: Mark Pearson <mpearson-lenovo@squebb.ca>
>>> Signed-off-by: Nitin Joshi<nitjoshi@gmail.com>
>>> ---
>>>   .../admin-guide/laptops/thinkpad-acpi.rst     |  26 +++
>>>   drivers/platform/x86/lenovo/thinkpad_acpi.c   | 179 ++++++++++++++++++
>>>   2 files changed, 205 insertions(+)
>>>
>>> diff --git a/Documentation/admin-guide/laptops/thinkpad-acpi.rst b/Documentation/admin-guide/laptops/thinkpad-acpi.rst
>>> index 4ab0fef7d440..4a3220529489 100644
>>> --- a/Documentation/admin-guide/laptops/thinkpad-acpi.rst
>>> +++ b/Documentation/admin-guide/laptops/thinkpad-acpi.rst
>>> @@ -54,6 +54,7 @@ detailed description):
>>>   	- Setting keyboard language
>>>   	- WWAN Antenna type
>>>   	- Auxmac
>>> +	- Hardware damage detection capability
>>>   
>>>   A compatibility table by model and feature is maintained on the web
>>>   site, http://ibm-acpi.sf.net/. I appreciate any success or failure
>>> @@ -1576,6 +1577,31 @@ percentage level, above which charging will stop.
>>>   The exact semantics of the attributes may be found in
>>>   Documentation/ABI/testing/sysfs-class-power.
>>>   
>>> +Hardware damage detection capability
>>> +-----------------
>>> +
>>> +sysfs attributes: hwdd_status
>>> +
>>> +Thinkpads are adding the ability to detect and report hardware damage.
>>> +Add new sysfs interface to identify the impacted component.
>>> +Initial support is available for the USB-C replaceable connector.
>>> +
>>> +The available commands are::
>>> +
>>> +        cat /sys/devices/platform/thinkpad_acpi/hwdd_status
>>> +
>>> +This value displays device type and location of device with damage status.
>>> +For example:
>>> +if no damage is detected:
>>> +  No damage detected
>>> +if damage detected:
>>> +  Damage detected:
>>> +  Device: TYPE-C
>>> +  Location: Base, Right side, Center port
>>> +
>>> +The property is read-only. If feature is not supported then sysfs
>>> +class is not created.
>> Nitpick: s/class/attribute/ classes are standardized sysfs
>> interfaces living under /sys/class/ which this is not.
> Ack, I will modify it.
>>
>> Besides the nitpick I'm wondering if we do not want to make
>> this a little bit more friendly / easy for software to parse ?
>>
>> ATM this seems focused on a human directly reading
>> the output but what if we want some sort of automation,
>> like e.g. a Linux version of the Lenovo Vantage sw parsing
>> this in the future?
>>
>> Note I've no specific suggestions for how to make this
>> easier to parse, this is just an observation.
>
> Thank you for pointing this out.  I am open for suggestions and will 
> re-check regarding its use case in lenovo vantage in future.
>
I'd not considered it from this perspective.

One concern is that it could be multiple devices in multiple location - so breaking it up into too many pieces becomes difficult.

What if we did:
  hwdd_status - 1 or 0 if damage detected
  hwdd_detail - list of damaged items with their location (could be multiple). e.g.
    TYPE-C: Base, Right side, Center port
If hwdd_status is 0 then hwdd_detail would display "No damage detected"

Mark

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] platform/x86: thinkpad_acpi: Add support to detect hardware damage detection capability.
  2025-12-03 13:16     ` Mark Pearson
@ 2025-12-08  7:19       ` Nitin
  2025-12-08  9:32         ` Hans de Goede
  0 siblings, 1 reply; 11+ messages in thread
From: Nitin @ 2025-12-08  7:19 UTC (permalink / raw)
  To: Mark Pearson, Hans de Goede, Ilpo Järvinen
  Cc: platform-driver-x86@vger.kernel.org, linux-kernel, Nitin Joshi1


On 12/3/25 22:16, Mark Pearson wrote:
> On Wed, Dec 3, 2025, at 5:18 AM, Nitin wrote:
>> Hi Hans,
>>
>> Thank you for reviewing this patch.
>>
>> On 12/3/25 16:52, Hans de Goede wrote:
>>> Hi,
>>>
>>> Interesting new feature. A few small remarks on the proposed
>>> sysfs API below.
>>>
>>> On 3-Dec-25 4:40 AM, Nitin Joshi wrote:
>>>> Thinkpads are adding the ability to detect and report hardware damage
>>>> status. Add new sysfs interface to identify the impacted component
>>>> with status.
>>>> Initial support is available for the USB-C replaceable connector.
>>>>
>>>> Reviewed-by: Mark Pearson <mpearson-lenovo@squebb.ca>
>>>> Signed-off-by: Nitin Joshi<nitjoshi@gmail.com>
>>>> ---
>>>>    .../admin-guide/laptops/thinkpad-acpi.rst     |  26 +++
>>>>    drivers/platform/x86/lenovo/thinkpad_acpi.c   | 179 ++++++++++++++++++
>>>>    2 files changed, 205 insertions(+)
>>>>
>>>> diff --git a/Documentation/admin-guide/laptops/thinkpad-acpi.rst b/Documentation/admin-guide/laptops/thinkpad-acpi.rst
>>>> index 4ab0fef7d440..4a3220529489 100644
>>>> --- a/Documentation/admin-guide/laptops/thinkpad-acpi.rst
>>>> +++ b/Documentation/admin-guide/laptops/thinkpad-acpi.rst
>>>> @@ -54,6 +54,7 @@ detailed description):
>>>>    	- Setting keyboard language
>>>>    	- WWAN Antenna type
>>>>    	- Auxmac
>>>> +	- Hardware damage detection capability
>>>>    
>>>>    A compatibility table by model and feature is maintained on the web
>>>>    site, http://ibm-acpi.sf.net/. I appreciate any success or failure
>>>> @@ -1576,6 +1577,31 @@ percentage level, above which charging will stop.
>>>>    The exact semantics of the attributes may be found in
>>>>    Documentation/ABI/testing/sysfs-class-power.
>>>>    
>>>> +Hardware damage detection capability
>>>> +-----------------
>>>> +
>>>> +sysfs attributes: hwdd_status
>>>> +
>>>> +Thinkpads are adding the ability to detect and report hardware damage.
>>>> +Add new sysfs interface to identify the impacted component.
>>>> +Initial support is available for the USB-C replaceable connector.
>>>> +
>>>> +The available commands are::
>>>> +
>>>> +        cat /sys/devices/platform/thinkpad_acpi/hwdd_status
>>>> +
>>>> +This value displays device type and location of device with damage status.
>>>> +For example:
>>>> +if no damage is detected:
>>>> +  No damage detected
>>>> +if damage detected:
>>>> +  Damage detected:
>>>> +  Device: TYPE-C
>>>> +  Location: Base, Right side, Center port
>>>> +
>>>> +The property is read-only. If feature is not supported then sysfs
>>>> +class is not created.
>>> Nitpick: s/class/attribute/ classes are standardized sysfs
>>> interfaces living under /sys/class/ which this is not.
>> Ack, I will modify it.
>>> Besides the nitpick I'm wondering if we do not want to make
>>> this a little bit more friendly / easy for software to parse ?
>>>
>>> ATM this seems focused on a human directly reading
>>> the output but what if we want some sort of automation,
>>> like e.g. a Linux version of the Lenovo Vantage sw parsing
>>> this in the future?
>>>
>>> Note I've no specific suggestions for how to make this
>>> easier to parse, this is just an observation.
>> Thank you for pointing this out.  I am open for suggestions and will
>> re-check regarding its use case in lenovo vantage in future.
>>
> I'd not considered it from this perspective.
>
> One concern is that it could be multiple devices in multiple location - so breaking it up into too many pieces becomes difficult.
>
> What if we did:
>    hwdd_status - 1 or 0 if damage detected
>    hwdd_detail - list of damaged items with their location (could be multiple). e.g.
>      TYPE-C: Base, Right side, Center port
> If hwdd_status is 0 then hwdd_detail would display "No damage detected"

Thank you, Mark ! I am OK with this suggestion.

Hans / Ilpo - Any comment on this ? Thanks

> Mark

Thank you !

Nitin Joshi


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] platform/x86: thinkpad_acpi: Add support to detect hardware damage detection capability.
  2025-12-08  7:19       ` Nitin
@ 2025-12-08  9:32         ` Hans de Goede
  2025-12-08 12:04           ` Nitin
  0 siblings, 1 reply; 11+ messages in thread
From: Hans de Goede @ 2025-12-08  9:32 UTC (permalink / raw)
  To: Nitin, Mark Pearson, Ilpo Järvinen
  Cc: platform-driver-x86@vger.kernel.org, linux-kernel, Nitin Joshi1

Hi,

On 8-Dec-25 8:19 AM, Nitin wrote:
> 
> On 12/3/25 22:16, Mark Pearson wrote:
>> On Wed, Dec 3, 2025, at 5:18 AM, Nitin wrote:
>>> Hi Hans,
>>>
>>> Thank you for reviewing this patch.
>>>
>>> On 12/3/25 16:52, Hans de Goede wrote:
>>>> Hi,
>>>>
>>>> Interesting new feature. A few small remarks on the proposed
>>>> sysfs API below.
>>>>
>>>> On 3-Dec-25 4:40 AM, Nitin Joshi wrote:
>>>>> Thinkpads are adding the ability to detect and report hardware damage
>>>>> status. Add new sysfs interface to identify the impacted component
>>>>> with status.
>>>>> Initial support is available for the USB-C replaceable connector.
>>>>>
>>>>> Reviewed-by: Mark Pearson <mpearson-lenovo@squebb.ca>
>>>>> Signed-off-by: Nitin Joshi<nitjoshi@gmail.com>
>>>>> ---
>>>>>    .../admin-guide/laptops/thinkpad-acpi.rst     |  26 +++
>>>>>    drivers/platform/x86/lenovo/thinkpad_acpi.c   | 179 ++++++++++++++++++
>>>>>    2 files changed, 205 insertions(+)
>>>>>
>>>>> diff --git a/Documentation/admin-guide/laptops/thinkpad-acpi.rst b/Documentation/admin-guide/laptops/thinkpad-acpi.rst
>>>>> index 4ab0fef7d440..4a3220529489 100644
>>>>> --- a/Documentation/admin-guide/laptops/thinkpad-acpi.rst
>>>>> +++ b/Documentation/admin-guide/laptops/thinkpad-acpi.rst
>>>>> @@ -54,6 +54,7 @@ detailed description):
>>>>>        - Setting keyboard language
>>>>>        - WWAN Antenna type
>>>>>        - Auxmac
>>>>> +    - Hardware damage detection capability
>>>>>       A compatibility table by model and feature is maintained on the web
>>>>>    site, http://ibm-acpi.sf.net/. I appreciate any success or failure
>>>>> @@ -1576,6 +1577,31 @@ percentage level, above which charging will stop.
>>>>>    The exact semantics of the attributes may be found in
>>>>>    Documentation/ABI/testing/sysfs-class-power.
>>>>>    +Hardware damage detection capability
>>>>> +-----------------
>>>>> +
>>>>> +sysfs attributes: hwdd_status
>>>>> +
>>>>> +Thinkpads are adding the ability to detect and report hardware damage.
>>>>> +Add new sysfs interface to identify the impacted component.
>>>>> +Initial support is available for the USB-C replaceable connector.
>>>>> +
>>>>> +The available commands are::
>>>>> +
>>>>> +        cat /sys/devices/platform/thinkpad_acpi/hwdd_status
>>>>> +
>>>>> +This value displays device type and location of device with damage status.
>>>>> +For example:
>>>>> +if no damage is detected:
>>>>> +  No damage detected
>>>>> +if damage detected:
>>>>> +  Damage detected:
>>>>> +  Device: TYPE-C
>>>>> +  Location: Base, Right side, Center port
>>>>> +
>>>>> +The property is read-only. If feature is not supported then sysfs
>>>>> +class is not created.
>>>> Nitpick: s/class/attribute/ classes are standardized sysfs
>>>> interfaces living under /sys/class/ which this is not.
>>> Ack, I will modify it.
>>>> Besides the nitpick I'm wondering if we do not want to make
>>>> this a little bit more friendly / easy for software to parse ?
>>>>
>>>> ATM this seems focused on a human directly reading
>>>> the output but what if we want some sort of automation,
>>>> like e.g. a Linux version of the Lenovo Vantage sw parsing
>>>> this in the future?
>>>>
>>>> Note I've no specific suggestions for how to make this
>>>> easier to parse, this is just an observation.
>>> Thank you for pointing this out.  I am open for suggestions and will
>>> re-check regarding its use case in lenovo vantage in future.
>>>
>> I'd not considered it from this perspective.
>>
>> One concern is that it could be multiple devices in multiple location - so breaking it up into too many pieces becomes difficult.
>>
>> What if we did:
>>    hwdd_status - 1 or 0 if damage detected
>>    hwdd_detail - list of damaged items with their location (could be multiple). e.g.
>>      TYPE-C: Base, Right side, Center port

So I assume this would then output one line per damaged "item", it would
be good to document this (1 line per damaged item) in the sysfs ABI docs.

>> If hwdd_status is 0 then hwdd_detail would display "No damage detected"
> 
> Thank you, Mark ! I am OK with this suggestion.
> 
> Hans / Ilpo - Any comment on this ? Thanks

Yes this sounds good to me. 

Regards,

Hans



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] platform/x86: thinkpad_acpi: Add support to detect hardware damage detection capability.
  2025-12-08  9:32         ` Hans de Goede
@ 2025-12-08 12:04           ` Nitin
  0 siblings, 0 replies; 11+ messages in thread
From: Nitin @ 2025-12-08 12:04 UTC (permalink / raw)
  To: Hans de Goede, Mark Pearson, Ilpo Järvinen
  Cc: platform-driver-x86@vger.kernel.org, linux-kernel, Nitin Joshi1

Hi,

On 12/8/25 18:32, Hans de Goede wrote:
> Hi,
>
> On 8-Dec-25 8:19 AM, Nitin wrote:
>> On 12/3/25 22:16, Mark Pearson wrote:
>>> On Wed, Dec 3, 2025, at 5:18 AM, Nitin wrote:
>>>> Hi Hans,
>>>>
>>>> Thank you for reviewing this patch.
>>>>
>>>> On 12/3/25 16:52, Hans de Goede wrote:
>>>>> Hi,
>>>>>
>>>>> Interesting new feature. A few small remarks on the proposed
>>>>> sysfs API below.
>>>>>
>>>>> On 3-Dec-25 4:40 AM, Nitin Joshi wrote:
>>>>>> Thinkpads are adding the ability to detect and report hardware damage
>>>>>> status. Add new sysfs interface to identify the impacted component
>>>>>> with status.
>>>>>> Initial support is available for the USB-C replaceable connector.
>>>>>>
>>>>>> Reviewed-by: Mark Pearson <mpearson-lenovo@squebb.ca>
>>>>>> Signed-off-by: Nitin Joshi<nitjoshi@gmail.com>
>>>>>> ---
>>>>>>     .../admin-guide/laptops/thinkpad-acpi.rst     |  26 +++
>>>>>>     drivers/platform/x86/lenovo/thinkpad_acpi.c   | 179 ++++++++++++++++++
>>>>>>     2 files changed, 205 insertions(+)
>>>>>>
>>>>>> diff --git a/Documentation/admin-guide/laptops/thinkpad-acpi.rst b/Documentation/admin-guide/laptops/thinkpad-acpi.rst
>>>>>> index 4ab0fef7d440..4a3220529489 100644
>>>>>> --- a/Documentation/admin-guide/laptops/thinkpad-acpi.rst
>>>>>> +++ b/Documentation/admin-guide/laptops/thinkpad-acpi.rst
>>>>>> @@ -54,6 +54,7 @@ detailed description):
>>>>>>         - Setting keyboard language
>>>>>>         - WWAN Antenna type
>>>>>>         - Auxmac
>>>>>> +    - Hardware damage detection capability
>>>>>>        A compatibility table by model and feature is maintained on the web
>>>>>>     site, http://ibm-acpi.sf.net/. I appreciate any success or failure
>>>>>> @@ -1576,6 +1577,31 @@ percentage level, above which charging will stop.
>>>>>>     The exact semantics of the attributes may be found in
>>>>>>     Documentation/ABI/testing/sysfs-class-power.
>>>>>>     +Hardware damage detection capability
>>>>>> +-----------------
>>>>>> +
>>>>>> +sysfs attributes: hwdd_status
>>>>>> +
>>>>>> +Thinkpads are adding the ability to detect and report hardware damage.
>>>>>> +Add new sysfs interface to identify the impacted component.
>>>>>> +Initial support is available for the USB-C replaceable connector.
>>>>>> +
>>>>>> +The available commands are::
>>>>>> +
>>>>>> +        cat /sys/devices/platform/thinkpad_acpi/hwdd_status
>>>>>> +
>>>>>> +This value displays device type and location of device with damage status.
>>>>>> +For example:
>>>>>> +if no damage is detected:
>>>>>> +  No damage detected
>>>>>> +if damage detected:
>>>>>> +  Damage detected:
>>>>>> +  Device: TYPE-C
>>>>>> +  Location: Base, Right side, Center port
>>>>>> +
>>>>>> +The property is read-only. If feature is not supported then sysfs
>>>>>> +class is not created.
>>>>> Nitpick: s/class/attribute/ classes are standardized sysfs
>>>>> interfaces living under /sys/class/ which this is not.
>>>> Ack, I will modify it.
>>>>> Besides the nitpick I'm wondering if we do not want to make
>>>>> this a little bit more friendly / easy for software to parse ?
>>>>>
>>>>> ATM this seems focused on a human directly reading
>>>>> the output but what if we want some sort of automation,
>>>>> like e.g. a Linux version of the Lenovo Vantage sw parsing
>>>>> this in the future?
>>>>>
>>>>> Note I've no specific suggestions for how to make this
>>>>> easier to parse, this is just an observation.
>>>> Thank you for pointing this out.  I am open for suggestions and will
>>>> re-check regarding its use case in lenovo vantage in future.
>>>>
>>> I'd not considered it from this perspective.
>>>
>>> One concern is that it could be multiple devices in multiple location - so breaking it up into too many pieces becomes difficult.
>>>
>>> What if we did:
>>>     hwdd_status - 1 or 0 if damage detected
>>>     hwdd_detail - list of damaged items with their location (could be multiple). e.g.
>>>       TYPE-C: Base, Right side, Center port
> So I assume this would then output one line per damaged "item", it would
> be good to document this (1 line per damaged item) in the sysfs ABI docs.

yes, we will have one line per damaged "item".  I will update ABI docs 
accordingly.

Thanks

>
>>> If hwdd_status is 0 then hwdd_detail would display "No damage detected"
>> Thank you, Mark ! I am OK with this suggestion.
>>
>> Hans / Ilpo - Any comment on this ? Thanks
> Yes this sounds good to me.
>
> Regards,
>
> Hans

Thanks & Regards,

Nitin

>
>

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2025-12-08 12:04 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-12-03  3:40 [PATCH] platform/x86: thinkpad_acpi: Add support to detect hardware damage detection capability Nitin Joshi
2025-12-03  7:52 ` Hans de Goede
2025-12-03 10:18   ` Nitin
2025-12-03 13:16     ` Mark Pearson
2025-12-08  7:19       ` Nitin
2025-12-08  9:32         ` Hans de Goede
2025-12-08 12:04           ` Nitin
2025-12-03  9:44 ` Ilpo Järvinen
2025-12-03 10:34   ` Nitin
2025-12-03 10:39     ` Ilpo Järvinen
2025-12-03 10:53       ` Nitin

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.