Re: [PATCH v6 1/2] platform: Add driver for RAVE Supervisory Processor

[Andrey Smirnov]

On Wed, Aug 30, 2017 at 3:55 AM, Pavel Machek <pavel@xxxxxx> wrote:
> Hi!
>
>> Add a driver for RAVE Supervisory Processor, an MCU implementing
>> varoius bits of housekeeping functionality (watchdoging, backlight
>> control, LED control, etc) on RAVE family of products by Zodiac
>> Inflight Innovations.
>>
>> This driver implementes core MFD/serdev device as well as
>> communication subroutines necessary for commanding the device.
>
>> diff --git a/Documentation/ABI/testing/sysfs-platform-rave-sp b/Documentation/ABI/testing/sysfs-platform-rave-sp
>> new file mode 100644
>> index 000000000000..81bdc54ba857
>> --- /dev/null
>> +++ b/Documentation/ABI/testing/sysfs-platform-rave-sp
>> @@ -0,0 +1,35 @@
>> +What:                /sys/devices/platform/*/serial*/serial*-*/reset_reason
>> +Date:                Aug 2017
>> +Contact:     "Andrey Smirnov" <andrew.smirnov@xxxxxxxxx>
>> +Description:    (RO) Indicates reason for last reset.
>> +
>> +             The following values can be reported:
>> +                 * 0  -> Normal Power Off
>> +                 * 1  -> Hardware Watchdog
>> +                 * 2  -> Software Watchdog
>> +                 * 3  -> Input Voltage Out Of Range
>> +                 * 4  -> Host Requested
>> +                 * 5  -> Temperature Out Of Range
>> +                 * 6  -> User Requested (via long Power Button press)
>> +                 * 7  -> Illegal Configuration Word
>> +                 * 8  -> Illegal Insturction
>
> Typo here.
>
>> +                 * 9  -> Illegal Trap
>> +                 * 10 -> Unknown
>> +                 * 11 -> Crew Panel Requested
>
> Anyway... If you move management chip to .. I don't know, i2c, the
> path would change. Also it would be different path on N900. Userland
> should not have to deal with this.
>
> And... this should really be string, as the list will need to grow on
> different hardware.
>

I think we have a misunderstanding, with this part of the patch set I
am not trying to propose a generic ABI that would be useful for any
other driver but this one. Hence the lack of concern for different
hardware paths (it's not going to change for this device) and device
specific codes instead of generic strings. I can see how my choice of
generic name such as "reset_reason" might suggest that, so I apologize
for any confusion I might have caused. If said generic name is
unacceptable I can change it to "rave_reset_reason" or something
similar and if that is undesirable as well I am happy to drop this
part of the patch and re-visit this later.

> Plus we'll really need better explanations. What is difference between
> "normal power off" and "host requested"?
>

Short answer: I don't know, since this is as much information that ICD
for that device gave me.

Long answer:  It probably can be discerned from the source code of the
firmware/schematic as well as by bothering the right people, but since
I get a feeling that this attribute is not really desirable in its
current from, I'll punt doing that.

>> +What:                /sys/devices/platform/*/serial*/serial*-*/boot_source
>> +Date:                Aug 2017
>> +Contact:     "Andrey Smirnov" <andrew.smirnov@xxxxxxxxx>
>> +Description:    (RW) Indicates currently selected boot source.
>> +
>> +             The following values are valid:
>> +                 * 0  -> SD card
>> +                 * 1  -> eMMC
>> +                 * 2  -> SPI NOR
>> +
>> +             NOTE: Setting boot source on RDU1 hardware is
>> +             currently not implemented
>
> Same comments apply here.
>

Yep, same comment for me as well :-)

>> +RAVE_SP_DEFINE_SEQ_FILE_PRINT_FUNCTION(part_number_firmware);
>> +RAVE_SP_DEFINE_SEQ_FILE_PRINT_FUNCTION(part_number_bootloader);
>> +RAVE_SP_DEFINE_SEQ_FILE_PRINT_FUNCTION(copper_rev_deb);
>> +RAVE_SP_DEFINE_SEQ_FILE_PRINT_FUNCTION(copper_rev_rmb);
>> +RAVE_SP_DEFINE_SEQ_FILE_PRINT_FUNCTION(copper_mod_deb);
>> +RAVE_SP_DEFINE_SEQ_FILE_PRINT_FUNCTION(copper_mod_rmb);
>> +RAVE_SP_DEFINE_SEQ_FILE_PRINT_FUNCTION(silicon_devid);
>> +RAVE_SP_DEFINE_SEQ_FILE_PRINT_FUNCTION(silicon_devrev);
>
> Are these going to debugfs?

They already are there. See rave_sp_debugfs_create() where those
helper functions are being used.

Thanks,
Andrey Smirnov