Re: [RFC PATCH v1 0/2] platform: toradex: Add toradex embedded controller

[Francesco Dolcini <francesco@dolcini.it>]

Hello Hans,
first thanks for the feedback.

On Thu, Mar 13, 2025 at 04:08:14PM +0100, Hans de Goede wrote:
> On 13-Mar-25 3:43 PM, Francesco Dolcini wrote:
> > From: Francesco Dolcini <francesco.dolcini@toradex.com>
> > 
> > This series adds support for the Toradex Embedded Controller, currently used
> > on Toradex SMARC iMX95 and iMX8MP boards, with more to come in the future.
> > 
> > The EC provides board power-off, reset and GPIO expander functionalities.
> > 
> > Sending it as an RFC to gather initial feedback on it before investing more
> > time in testing and adding the remaining functionalities, with that said both
> > the code and the binding are in condition to be wholly reviewed.
> > 
> > Emanuele Ghidoli (2):
> >   dt-bindings: firmware: add toradex,embedded-controller
> >   platform: toradex: add preliminary support for Embedded Controller
> 
> Thank you for your patches.
> 
> 2 remarks, as Andy already hinted at drivers/platform/arm64/ likely
> is a better location for this.

Ack.

This driver is not going to be specific of ARM64, but today we have only
ARM64 systems that would benefit from it. We might as well use it on a
RISCV based SoM in a few years.

With that said we'll move it there, we can always move it out if
anything changes on this regard.

> The reason for having ARM EC drivers there is that these are for
> x86-pc-like laptops with all the typical laptops bells and whistles
> like EC handled battery charging limits / spk/mic mute-leds built
> into keys on the keyboards. Special key handling (like mute, kbd
> backlight) done by the EC etc.
> 
> Since all the experience for dealing with those laptop-esque features
> and exporting them to userspace with a consistent userspace API is
> in hands of the maintainers of drivers/platform/x86 it was decided to
> add a new drivers/platform/arm64 directory maintained by the same folks.
> 
> If this EC driver's only functionality is: "The EC provides board
> power-off, reset and GPIO expander functionalities." I'm not sure
> that drivers/platform/arm64 is the best place for this.

The directory decision / architecture was mainly inspired by
`drivers/platform/cznic`.

This EC is used on a SMARC SoM [1][2], so we are not talking about a
laptop nor a device with a keyboard.

But we do have a power button, a LID switch, some handling and
coordination of low power mode and more.
This device is between the SOC, the PMIC, and various IOs used for
low-power, power-up/down, boot configuration (selecting the boot
device), ...

The short term goal is just the 2 basic functionalities mentioned in
the cover letter available to the driver:
 - power/reset (already implemented)
 - GPIO (working on it as we speak)

Starting small, and adding features when/if required.

> Also you mention GPIO expander, but that does not seem to be
> supported yet?
Correct, coming soon.

> 1. A drivers/mfd/ MFD driver with the regmap stuff,
>    registering "board-reset" and "gpio" cells

So, we considered the idea of going with an MFD driver, but looking at
drivers/platform/cznic, that is doing something relatively close to what
we are doing (just more feature rich, as of now), drivers/platform/
seemed a better fit.

I am not 100% sure what's Andy opinion on this topic, from what I can
understand his concerns are about the toradex directory (that we'll get
rid of), not the drivers/platform/ parent you are concerned about.

Francesco