On Fri, Jan 24, 2025 at 09:12:49PM +0800, Hans Zhang wrote:
>
>
> 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
>
Okay, it is not big deal
can you return
str + strlen("DW_PCIE_LTSSM_");
Or trim useless "DW_PCIE_LTSSM_" information.
Frank
>
> > > +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
>
