On 11/11/2020 1:05 AM, Christoph Hellwig wrote:
> On Tue, Nov 10, 2020 at 09:37:35AM -0600, Stuart Hayes wrote:
>> +Date: October 2020
>> +Contact: Stuart Hayes <stuart.w.hayes@xxxxxxxxx>
>> +Description: If the device supports the ACPI _DSM method to
control the
>> + PCIe SSD LED states, ssdleds_supported_states (read only)
>> + will show the LED states that are supported by the _DSM.
>> +
>> +What: /sys/bus/pci/devices/.../ssdleds_current_state
>> +Date: October 2020
>> +Contact: Stuart Hayes <stuart.w.hayes@xxxxxxxxx>
>> +Description: If the device supports the ACPI _DSM method to
control the
>> + PCIe SSD LED states, ssdleds_current_state will show or set
>> + the current LED states that are active.
>
> Is the supported file really required? Doesn't the current_state one
> also show which LEDs exist?
>
The current_state just shows which LED states are currently active, not
which
are supported by the system. I guess you could try to set all the states in
current_state and read it back to see which ones went on to see which states
are supported, but I'm not 100% if that would work, plus it might result in
the drive LEDs flashing in a strange way briefly.
>> +config PCI_SSDLEDS
>> + def_bool y if (ACPI && PCI)
>> + depends on PCI && ACPI
>
> We really should not default new code to y.
>
Good point, I agree.
>> + if (dsm_output->status != 0 &&
>> + !(dsm_output->status == 4 && dsm_output->function_specific_err
== 1)) {
>
> overly longline. But to make this a little more obvious, maybe you
> want to
>
> a) switch on dsm_output->status
> b) add symbolic names for the magic numbers
>
Good feedback, thanks.
