Re: [PATCH v3 2/5] dt-bindings: mfd: Add support for Airoha EN7581 GPIO System Controller

[Lee Jones <lee@xxxxxxxxxx>]

On Tue, 03 Sep 2024, Christian Marangi wrote:

> On Tue, Sep 03, 2024 at 04:33:53PM +0100, Lee Jones wrote:
> > On Sat, 31 Aug 2024, Lorenzo Bianconi wrote:
> > 
> > > From: Christian Marangi <ansuelsmth@xxxxxxxxx>
> > > 
> > > Add support for Airoha EN7581 GPIO System Controller which provide a
> > > register map for controlling the GPIO, pinctrl and PWM of the SoC.
> > > 
> > > Schema define cells for both gpio/interrupt controller and PWM.
> > > Moreover it provides a dedicated pinctrl node for pins and config
> > > definitions.
> > > 
> > > Signed-off-by: Christian Marangi <ansuelsmth@xxxxxxxxx>
> > > Co-developed-by: Lorenzo Bianconi <lorenzo@xxxxxxxxxx>
> > > Signed-off-by: Lorenzo Bianconi <lorenzo@xxxxxxxxxx>
> > > ---
> > >  .../bindings/mfd/airoha,en7581-gpio-sysctl.yaml    | 433 +++++++++++++++++++++
> > >  1 file changed, 433 insertions(+)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/mfd/airoha,en7581-gpio-sysctl.yaml b/Documentation/devicetree/bindings/mfd/airoha,en7581-gpio-sysctl.yaml
> > > new file mode 100644
> > > index 000000000000..a9080c7f50f9
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/mfd/airoha,en7581-gpio-sysctl.yaml
> > > @@ -0,0 +1,433 @@
> > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/mfd/airoha,en7581-gpio-sysctl.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: Airoha EN7581 GPIO System Controller
> > > +
> > > +maintainers:
> > > +  - Christian Marangi <ansuelsmth@xxxxxxxxx>
> > > +  - Lorenzo Bianconi <lorenzo@xxxxxxxxxx>
> > > +
> > > +description:
> > > +  Airoha EN7581 SoC GPIO system controller which provided a register map
> > > +  for controlling the GPIO, pins and PWM of the SoC.
> > 
> > This whole thing is just about pins.
> > 
> > The MFD portion of this submission doesn't do anything.
> >
> 
> Hi Lee,
> 
> thanks for the review. I think you missed the other series as it was
> requested to use MFD implementation due to shared register map.
> 
> > Please rework this to omit the MFD driver.
> 
> I'm a bit confused by this you mean in the schema? Putting PWM property
> in a pinctrl schema looks wrong to me :(
> 
> > 
> > After just a glance, it looks like simple-mfd _might_ work.
> 
> Simple-mfd works if register map are well defined and you can have
> something like
> - parent define the whole register
> - child can user reg property to register offset and subsection of the
>   parent register
> 
> Here it's all mixed and scrambled and also it was requested to have a
> very simple node that include both pwm and pinctrl property (cause that
> is how the HW register block is designed and schema must reflect HW)
> 
> Hope you can understand these reasons.

Thinking very quickly before I have to rush off.

Have you considered syscon?

-- 
Lee Jones [李琼斯]