Re: [PATCH] PCI: dwc: Add the sysfs property to provide the LTSSM status of the PCIe link

[Hans Zhang <18255117159@163.com>]

On 2025/1/24 00:16, Frank Li wrote:
>> +static char *dw_ltssm_sts_string(enum dw_pcie_ltssm ltssm)
>> +{
>> +	char *str;
>> +
>> +	switch (ltssm) {
>> +#define DW_PCIE_LTSSM_NAME(n) case n: str = #n; break
>> +	DW_PCIE_LTSSM_NAME(DW_PCIE_LTSSM_DETECT_QUIET);
>> +	DW_PCIE_LTSSM_NAME(DW_PCIE_LTSSM_DETECT_ACT);
>> +	DW_PCIE_LTSSM_NAME(DW_PCIE_LTSSM_POLL_ACTIVE);
>> +	DW_PCIE_LTSSM_NAME(DW_PCIE_LTSSM_POLL_COMPLIANCE);
>> +	...
>> +	default:
>> +		str = "DW_PCIE_LTSSM_UNKNOWN";
>> +		break;
> 
> I prefer
> const char * str[] =
> {
> 	"detect_quitet",
> 	"detect_act",
> 	...
> }
> 
> 	return str[ltssm];
> 
> Or
> 	#define DW_PCIE_LTSSM_NAME(n) case n: return #n;
> 	...
> 	default:
> 		return "DW_PCIE_LTSSM_UNKNOWN";
Hi Frank,

I considered the two methods you mentioned before I submitted this patch.

The first, I think, will increase the memory overhead.

+static const char * const dw_pcie_ltssm_str[] = {
+	[DW_PCIE_LTSSM_DETECT_QUIET] = "DETECT_QUIET",
+	[DW_PCIE_LTSSM_DETECT_ACT] = "DETECT_ACT",
+	[DW_PCIE_LTSSM_POLL_ACTIVE] = "POLL_ACTIVE",
+	[DW_PCIE_LTSSM_POLL_COMPLIANCE] = "POLL_COMPLIANCE",
	...


The second, ./scripts/checkpatch.pl checks will have a warning

WARNING: Macros with flow control statements should be avoided
#121: FILE: drivers/pci/controller/dwc/pcie-designware.h:329:
+#define DW_PCIE_LTSSM_NAME(n) case n: return #n


>> +static ssize_t ltssm_status_show(struct device *dev,
>> +				 struct device_attribute *attr, char *buf)
>> +{
>> +	struct pci_dev *pdev = to_pci_dev(dev);
>> +	struct pci_host_bridge *bridge = pci_find_host_bridge(pdev->bus);
>> +	struct dw_pcie_rp *pp = bridge->sysdata;
>> +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
>> +
>> +	return sysfs_emit(buf, "%s\n",
>> +			  dw_ltssm_sts_string(dw_pcie_get_ltssm(pci)));
> 
> Suggest dump raw value also
> 
> val = dw_pcie_get_ltssm(pci);
> return sysfs_emit(buf, "%s (0x%02x)\n",
> 		  dw_ltssm_sts_string(val), val);

Thanks, i think it's a good idea.

Best regards
Hans