mbox series

[v7,0/6] Add support for the IEI WT61P803 PUZZLE MCU

Message ID 20201025005916.64747-1-luka.kovacic@sartura.hr (mailing list archive)
Headers show
Series Add support for the IEI WT61P803 PUZZLE MCU | expand

Message

Luka Kovacic Oct. 25, 2020, 12:59 a.m. UTC
This patchset adds support for the IEI WT61P803 PUZZLE microcontroller,
which enables some board specific features like fan and LED control,
system power management and temperature sensor reading on some IEI
Puzzle series boards.

The first board to use this functionality is IEI Puzzle-M801 1U
Rackmount Network Appliance and is since v4 sent separately, as a
standalone patch.

Changes for v2:
   - Use LAAs for local-mac-address and match reg values
   - Code styling changes
   - Error handling moved to the end of the function
   - Define all magic numbers in the main header file
   - Convert the driver to make it OF independent
   - Refactor hwmon to use devm_hwmon_device_register_with_info()
   - Reduce the number of mutex locks
   - Allocate memory once for the response buffer
   - Reduce managed memory allocations
Changes for v3:
   - Move iei-wt61p803-puzzle driver sysfs interface documentation to testing
   - Change some internal functions to static
   - Sync dt-bindings examples with the IEI Puzzle-M801 board dts
   - Remove obsolete device tree properties and correct LED functions
   - Reverse christmas tree variable declaration order, where possible
   - MAC address sysfs function rewrite
   - Fixed struct members size, where reasonable (MFD driver)
   - Add an error check for hwmon_dev
   - Use devm_led_classdev_register_ext() in the LED driver
Changes for v4:
   - Clean up sensible checks reported by checkpatch --strict
   - Document the mutex lock usage in the LED driver
   - Fix error handling and code styling issues in the HWMON driver
   - Break up the patchset and send the IEI Puzzle-M801 board support
patch separately
Changes for v5:
   - Remove the return before goto to also fwnode_handle_put(child)
when ret is 0 (LED driver)
   - Change unsigned char arrays to static where applicable
   - Fix unconventional line indentations
   - Remove unnecessary checks in the HWMON driver
   - Remove unnecessary type casts
   - Clear up command array assignments, where the command array is
modified before it is sent
   - Resolve a checksum calculation issue
   - Add Luka Perkov to MAINTAINERS
Changes for v6:
   - Use the container_of() macro to get the led_cdev parent struct
   - Use %u instead of %lu in a printf() (LED driver)
Changes for v7:
   - Use the correct vendor title (IEI instead of iEi)
   - Add missing properties to dt-bindings and fix styling issues
   - Styling changes in the IEI WT61P803 PUZZLE HWMON driver
   - Add missing commas in array definitions
   - Check reply_size, where possible
   - Clean up kernel-doc comments

Luka Kovacic (6):
  dt-bindings: Add IEI vendor prefix and IEI WT61P803 PUZZLE driver
    bindings
  drivers: mfd: Add a driver for IEI WT61P803 PUZZLE MCU
  drivers: hwmon: Add the IEI WT61P803 PUZZLE HWMON driver
  drivers: leds: Add the IEI WT61P803 PUZZLE LED driver
  Documentation/ABI: Add iei-wt61p803-puzzle driver sysfs interface
    documentation
  MAINTAINERS: Add an entry for the IEI WT61P803 PUZZLE driver

 .../testing/sysfs-driver-iei-wt61p803-puzzle  |   55 +
 .../hwmon/iei,wt61p803-puzzle-hwmon.yaml      |   53 +
 .../leds/iei,wt61p803-puzzle-leds.yaml        |   45 +
 .../bindings/mfd/iei,wt61p803-puzzle.yaml     |   83 ++
 .../devicetree/bindings/vendor-prefixes.yaml  |    2 +
 MAINTAINERS                                   |   14 +
 drivers/hwmon/Kconfig                         |    8 +
 drivers/hwmon/Makefile                        |    1 +
 drivers/hwmon/iei-wt61p803-puzzle-hwmon.c     |  412 +++++++
 drivers/leds/Kconfig                          |    8 +
 drivers/leds/Makefile                         |    1 +
 drivers/leds/leds-iei-wt61p803-puzzle.c       |  161 +++
 drivers/mfd/Kconfig                           |    8 +
 drivers/mfd/Makefile                          |    1 +
 drivers/mfd/iei-wt61p803-puzzle.c             | 1039 +++++++++++++++++
 include/linux/mfd/iei-wt61p803-puzzle.h       |   66 ++
 16 files changed, 1957 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-driver-iei-wt61p803-puzzle
 create mode 100644 Documentation/devicetree/bindings/hwmon/iei,wt61p803-puzzle-hwmon.yaml
 create mode 100644 Documentation/devicetree/bindings/leds/iei,wt61p803-puzzle-leds.yaml
 create mode 100644 Documentation/devicetree/bindings/mfd/iei,wt61p803-puzzle.yaml
 create mode 100644 drivers/hwmon/iei-wt61p803-puzzle-hwmon.c
 create mode 100644 drivers/leds/leds-iei-wt61p803-puzzle.c
 create mode 100644 drivers/mfd/iei-wt61p803-puzzle.c
 create mode 100644 include/linux/mfd/iei-wt61p803-puzzle.h

Comments

Luka Kovacic Nov. 1, 2020, 9:56 a.m. UTC | #1
Hello Pavel,

On Thu, Oct 29, 2020 at 7:01 PM Pavel Machek <pavel@ucw.cz> wrote:
>
> Hi!
>
> > +What:                /sys/bus/serial/devices/.../iei_wt61p803_puzzle_core/power_status
> > +Date:                September 2020
> > +Contact:     Luka Kovacic <luka.kovacic@sartura.hr>
> > +Description: (RO) Power status indicates the host platform power on method.
> > +             Value mapping (bitwise list):
> > +             0x80 - Null
> > +             0x40 - Firmware flag
> > +             0x20 - Power loss detection flag (powered off)
> > +             0x10 - Power loss detection flag (AC mode)
> > +             0x08 - Button power on
> > +             0x04 - WOL power on
> > +             0x02 - RTC alarm power on
> > +             0x01 - AC recover power on
>
> It would be nice to put this into standard place somewhere. Many
> machines will want to expose this information.

As this is specific to this microcontroller and to how it encodes
these values, I don't see a need to change this.
This isn't used anywhere else.

>
> If not, at least spell out WoL, as it is not that common of acronym.

Okay.

>
> > +What:                /sys/bus/serial/devices/.../iei_wt61p803_puzzle_core/ac_recovery_status
> > +Date:                September 2020
> > +Contact:     Luka Kovacic <luka.kovacic@sartura.hr>
> > +Description: (RO) Host platform AC recovery status value
>
> I can not tell what this is from documentation...

I'll expand the description.

>
> Best regards,
>                                                                 Pavel
>
> --
> http://www.livejournal.com/~pavelmachek

Kind regards,
Luka
Dan Murphy Nov. 2, 2020, 6:29 p.m. UTC | #2
Hello

On 11/1/20 3:56 AM, Luka Kovacic wrote:
> Hello Pavel,
>
> On Thu, Oct 29, 2020 at 7:01 PM Pavel Machek <pavel@ucw.cz> wrote:
>> Hi!
>>
>>> +What:                /sys/bus/serial/devices/.../iei_wt61p803_puzzle_core/power_status
>>> +Date:                September 2020
>>> +Contact:     Luka Kovacic <luka.kovacic@sartura.hr>
>>> +Description: (RO) Power status indicates the host platform power on method.
>>> +             Value mapping (bitwise list):
>>> +             0x80 - Null
>>> +             0x40 - Firmware flag
>>> +             0x20 - Power loss detection flag (powered off)
>>> +             0x10 - Power loss detection flag (AC mode)
>>> +             0x08 - Button power on
>>> +             0x04 - WOL power on
>>> +             0x02 - RTC alarm power on
>>> +             0x01 - AC recover power on
>> It would be nice to put this into standard place somewhere. Many
>> machines will want to expose this information.
> As this is specific to this microcontroller and to how it encodes
> these values, I don't see a need to change this.
> This isn't used anywhere else.
>
>> If not, at least spell out WoL, as it is not that common of acronym.
> Okay.

WoL is a very common acronym especially in the networking space

But the overall this section does not make sense

The description says that it indicates platform power on method but what 
is NULL power on? There are flags for power loss detection.

Does the RTC mean that the processor real time clock woke up the uC? Or 
that the internal RTC woke up the controller?

And for the 
/sys/bus/serial/devices/.../iei_wt61p803_puzzle_core/ac_recovery_status 
what are those values?

It seems like some ABI's are documented well with formats and others are 
just described without a format.

For instance

/sys/bus/serial/devices/.../iei_wt61p803_puzzle_core/version the format 
of this version is not described but 
/sys/bus/serial/devices/.../iei_wt61p803_puzzle_core/build_info is.


Dan
Pavel Machek Nov. 2, 2020, 7:03 p.m. UTC | #3
On Mon 2020-11-02 12:29:59, Dan Murphy wrote:
> Hello
> 
> On 11/1/20 3:56 AM, Luka Kovacic wrote:
> > Hello Pavel,
> > 
> > On Thu, Oct 29, 2020 at 7:01 PM Pavel Machek <pavel@ucw.cz> wrote:
> > > Hi!
> > > 
> > > > +What:                /sys/bus/serial/devices/.../iei_wt61p803_puzzle_core/power_status
> > > > +Date:                September 2020
> > > > +Contact:     Luka Kovacic <luka.kovacic@sartura.hr>
> > > > +Description: (RO) Power status indicates the host platform power on method.
> > > > +             Value mapping (bitwise list):
> > > > +             0x80 - Null
> > > > +             0x40 - Firmware flag
> > > > +             0x20 - Power loss detection flag (powered off)
> > > > +             0x10 - Power loss detection flag (AC mode)
> > > > +             0x08 - Button power on
> > > > +             0x04 - WOL power on
> > > > +             0x02 - RTC alarm power on
> > > > +             0x01 - AC recover power on
> > > It would be nice to put this into standard place somewhere. Many
> > > machines will want to expose this information.
> > As this is specific to this microcontroller and to how it encodes
> > these values, I don't see a need to change this.
> > This isn't used anywhere else.
> > 
> > > If not, at least spell out WoL, as it is not that common of acronym.
> > Okay.
> 
> WoL is a very common acronym especially in the networking space

WoL is common. WOL is not. Better spell it out.

Best regards,
								Pavel
Dan Murphy Nov. 2, 2020, 7:04 p.m. UTC | #4
Hello

On 11/2/20 1:03 PM, Pavel Machek wrote:
> On Mon 2020-11-02 12:29:59, Dan Murphy wrote:
>> Hello
>>
>> On 11/1/20 3:56 AM, Luka Kovacic wrote:
>>> Hello Pavel,
>>>
>>> On Thu, Oct 29, 2020 at 7:01 PM Pavel Machek <pavel@ucw.cz> wrote:
>>>> Hi!
>>>>
>>>>> +What:                /sys/bus/serial/devices/.../iei_wt61p803_puzzle_core/power_status
>>>>> +Date:                September 2020
>>>>> +Contact:     Luka Kovacic <luka.kovacic@sartura.hr>
>>>>> +Description: (RO) Power status indicates the host platform power on method.
>>>>> +             Value mapping (bitwise list):
>>>>> +             0x80 - Null
>>>>> +             0x40 - Firmware flag
>>>>> +             0x20 - Power loss detection flag (powered off)
>>>>> +             0x10 - Power loss detection flag (AC mode)
>>>>> +             0x08 - Button power on
>>>>> +             0x04 - WOL power on
>>>>> +             0x02 - RTC alarm power on
>>>>> +             0x01 - AC recover power on
>>>> It would be nice to put this into standard place somewhere. Many
>>>> machines will want to expose this information.
>>> As this is specific to this microcontroller and to how it encodes
>>> these values, I don't see a need to change this.
>>> This isn't used anywhere else.
>>>
>>>> If not, at least spell out WoL, as it is not that common of acronym.
>>> Okay.
>> WoL is a very common acronym especially in the networking space
> WoL is common. WOL is not. Better spell it out.

Agreed. Especially if WOL does not mean Wake On Lan

Dan
Luka Kovacic Nov. 2, 2020, 10:36 p.m. UTC | #5
Hello,

On Mon, Nov 2, 2020 at 7:30 PM Dan Murphy <dmurphy@ti.com> wrote:
>
> Hello
>
> On 11/1/20 3:56 AM, Luka Kovacic wrote:
> > Hello Pavel,
> >
> > On Thu, Oct 29, 2020 at 7:01 PM Pavel Machek <pavel@ucw.cz> wrote:
> >> Hi!
> >>
> >>> +What:                /sys/bus/serial/devices/.../iei_wt61p803_puzzle_core/power_status
> >>> +Date:                September 2020
> >>> +Contact:     Luka Kovacic <luka.kovacic@sartura.hr>
> >>> +Description: (RO) Power status indicates the host platform power on method.
> >>> +             Value mapping (bitwise list):
> >>> +             0x80 - Null
> >>> +             0x40 - Firmware flag
> >>> +             0x20 - Power loss detection flag (powered off)
> >>> +             0x10 - Power loss detection flag (AC mode)
> >>> +             0x08 - Button power on
> >>> +             0x04 - WOL power on
> >>> +             0x02 - RTC alarm power on
> >>> +             0x01 - AC recover power on
> >> It would be nice to put this into standard place somewhere. Many
> >> machines will want to expose this information.
> > As this is specific to this microcontroller and to how it encodes
> > these values, I don't see a need to change this.
> > This isn't used anywhere else.
> >
> >> If not, at least spell out WoL, as it is not that common of acronym.
> > Okay.
>
> WoL is a very common acronym especially in the networking space

By WOL I meant Wake-on-LAN, I will spell out the whole acronym.

>
> But the overall this section does not make sense
>
> The description says that it indicates platform power on method but what
> is NULL power on? There are flags for power loss detection.

I will clarify the value mapping and try to replicate some of these states
so I can write a better description.

>
> Does the RTC mean that the processor real time clock woke up the uC? Or
> that the internal RTC woke up the controller?

These are all related to the platform as a whole.
So the Marvell SoC and all of the required peripherals are turned on.

>
> And for the
> /sys/bus/serial/devices/.../iei_wt61p803_puzzle_core/ac_recovery_status
> what are those values?

These values indicate whether the board has been shut down gracefully and
whether it has been powered on automatically (when power came back) or by
pressing the power button.
I will also extend the documentation with the value mapping for this.

>
> It seems like some ABI's are documented well with formats and others are
> just described without a format.
>
> For instance
>
> /sys/bus/serial/devices/.../iei_wt61p803_puzzle_core/version the format
> of this version is not described but
> /sys/bus/serial/devices/.../iei_wt61p803_puzzle_core/build_info is.

I left out the version format descriptions as they are in the recognizable
format and all of them are quite arbitrary (e.g. v1.000).

>
>
> Dan
>

Kind regards,
Luka