[+cc Keith for visibility]
Hi Stuart,
> >cat /sys/class/leds/0000:88:00.0::pcie_ssd_status/supported_states
> ok 0x0004 [ ]
> locate 0x0008 [*]
> fail 0x0010 [ ]
> rebuild 0x0020 [ ]
> pfa 0x0040 [ ]
> hotspare 0x0080 [ ]
> criticalarray 0x0100 [ ]
> failedarray 0x0200 [ ]
> invaliddevice 0x0400 [ ]
> disabled 0x0800 [ ]
> --
> supported_states = 0x0008
>
> >cat /sys/class/leds/0000:88:00.0::pcie_ssd_status/current_states
> locate 0x0008 [ ]
As per what Keith already noted, this is a very elaborate output as far
as sysfs goes - very human-readable, but it would be complex to parse
should some software would be interested in showing this values in a way
or another.
I would propose output similar to this one:
$ cat /sys/block/sda/queue/scheduler
mq-deadline-nodefault [bfq] none
If you prefer to show the end-user a string, rather than a numeric
value. This approach could support both the supported and current
states (similarly to how it works for the I/O scheduler), thus there
would be no need to duplicate the code between the two attributes.
What do you think?
Also, and I appreciate it would be more work, it's customary to accept
a string value rather than a numeric value when updating state through
the sysfs object, especially when the end-user is presented with string
values on read. Having said that, this is not a requirement, so I am
leaving it at your discretion. :)
[...]
> +static struct led_state led_states[] = {
> + { .name = "ok", .mask = 1 << 2 },
> + { .name = "locate", .mask = 1 << 3 },
> + { .name = "fail", .mask = 1 << 4 },
> + { .name = "rebuild", .mask = 1 << 5 },
> + { .name = "pfa", .mask = 1 << 6 },
> + { .name = "hotspare", .mask = 1 << 7 },
> + { .name = "criticalarray", .mask = 1 << 8 },
> + { .name = "failedarray", .mask = 1 << 9 },
> + { .name = "invaliddevice", .mask = 1 << 10 },
> + { .name = "disabled", .mask = 1 << 11 },
> +};
Just a suggestion: how about using the BIT() macro in the above?
[...]
> +static bool pdev_has_dsm(struct pci_dev *pdev)
> +{
> + acpi_handle handle;
> +
> + handle = ACPI_HANDLE(&pdev->dev);
> + if (!handle)
> + return false;
> +
> + return !!acpi_check_dsm(handle, &pcie_ssdleds_dsm_guid, 0x1,
> + 1 << GET_SUPPORTED_STATES_DSM ||
> + 1 << GET_STATE_DSM ||
> + 1 << SET_STATE_DSM);
> +}
The acpi_check_dsm() function already returns a bool type, so you can
drop the conversion.
> +static ssize_t current_states_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct led_classdev *led_cdev = dev_get_drvdata(dev);
> + struct device *dsm_dev = led_cdev->dev->parent;
> + struct ssd_status_dev *ssd;
> + u32 val;
> + int err, i, result = 0;
> +
> + ssd = container_of(led_cdev, struct ssd_status_dev, led_cdev);
> +
> + err = dsm_get(dsm_dev, GET_STATE_DSM, &val);
> + if (err < 0)
> + return err;
> + for (i = 0; i < ARRAY_SIZE(led_states); i++)
> + if (led_states[i].mask & ssd->supported_states)
> + result += sprintf(buf + result, "%-25s\t0x%04X [%c]\n",
> + led_states[i].name,
> + led_states[i].mask,
> + (val & led_states[i].mask)
> + ? '*' : ' ');
> +
> + result += sprintf(buf + result, "--\ncurrent_states = 0x%04X\n", val);
> + return result;
> +}
This show() callback above might benefit from already using the
sysfs_emit_at() function instead of using the sprintf() here, see:
https://www.kernel.org/doc/html/latest/filesystems/sysfs.html?highlight=sysfs_emi#reading-writing-attribute-data
[...]
> +static ssize_t supported_states_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct led_classdev *led_cdev = dev_get_drvdata(dev);
> + struct ssd_status_dev *ssd;
> + int i, result = 0;
> +
> + ssd = container_of(led_cdev, struct ssd_status_dev, led_cdev);
> +
> + for (i = 0; i < ARRAY_SIZE(led_states); i++)
> + result += sprintf(buf + result, "%-25s\t0x%04X [%c]\n",
> + led_states[i].name,
> + led_states[i].mask,
> + (ssd->supported_states & led_states[i].mask)
> + ? '*' : ' ');
> +
> + result += sprintf(buf + result, "--\nsupported_states = 0x%04X\n",
> + ssd->supported_states);
> + return result;
> +}
Same as above. This might benefit from using sysfs_emit_at().
[...]
> +static DEVICE_ATTR_RW(current_states);
> +static DEVICE_ATTR_RO(supported_states);
A small nitpik. These are customary added immediately after the show()
and store() functions for a given attribute.
Krzysztof
