Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1750941AbdH3TA2 (ORCPT ); Wed, 30 Aug 2017 15:00:28 -0400 Received: from mail-qt0-f195.google.com ([209.85.216.195]:38579 "EHLO mail-qt0-f195.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750803AbdH3TA0 (ORCPT ); Wed, 30 Aug 2017 15:00:26 -0400 MIME-Version: 1.0 In-Reply-To: <20170830105515.GA4926@amd> References: <20170828163131.24815-1-andrew.smirnov@gmail.com> <20170828163131.24815-2-andrew.smirnov@gmail.com> <20170830105515.GA4926@amd> From: Andrey Smirnov Date: Wed, 30 Aug 2017 12:00:25 -0700 Message-ID: Subject: Re: [PATCH v6 1/2] platform: Add driver for RAVE Supervisory Processor To: Pavel Machek Cc: linux-kernel , Chris Healy , Lucas Stach , Nikita Yushchenko , Lee Jones , Greg Kroah-Hartman Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4248 Lines: 104 On Wed, Aug 30, 2017 at 3:55 AM, Pavel Machek 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" >> +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" >> +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