Re: [PATCH v2 3/4] dt-bindings: Add pinctrl bindings for mt65xx/mt81xx.

[Lucas Stach]

Am Donnerstag, den 02.10.2014, 16:00 +0200 schrieb Linus Walleij:
> On Tue, Sep 23, 2014 at 5:39 AM, Hongzhou.Yang
> <srv_hongzhou.yang@xxxxxxxxxxxx> wrote:
> 
> > From: Hongzhou Yang <hongzhou.yang@xxxxxxxxxxxx>
> >
> > Add devicetree bindings for Mediatek SoC pinctrl driver.
> >
> > Signed-off-by: Hongzhou Yang <hongzhou.yang@xxxxxxxxxxxx>
> 
> I have worked on generic pin control bindings a bit because it
> is getting out of hand with all these custom bindings.
> 
> See:
> http://marc.info/?l=devicetree&m=141223584006648&w=2
> 
> Especially.
> 
> > +- mediatek,pinfunc: List of gpio number and function to mux.
> 
> A "GPIO number" and a "pin number" is not the same thing at all,
> this is very confusing. Those are two separate number spaces.
> This is likely about the pin numbers.
> 
> > +The mediatek,pinfunc can use defines directly,
> > +which are already defind in boot/dts/mt8135-pinfunc.h.
> > +
> > +Optional subnode-properties:
> > +- generic pin configuration option to use, bias-disable, bias-pull-down,
> > +  bias-pull,up, output-low and output-high are valid.
> > +  Example :
> > +       i2c0_pins_a {
> > +               mediatek,pinfunc = <MT8135_PIN_195_SDA1__FUNC_SDA1>;
> > +               bias-disable;
> > +       };
> 
> I don't like this approach at all.
> 
> I prefer that pins are put into groups named by strings, like "i2c0-pos0"
> inside the driver and then connected to function with a certain
> device-related name, such as "i2c0".
> 
So we should create artificial software groups where there are none in
hardware? This sounds really backward to me. Almost every new SoC out
there has the ability to mux every pin on it's own.

By defining artificial software groups in the driver we are pushing
constraints in the binding that don't really exist in hardware. So if
someone comes up with a pin usage that isn't covered by the existing
groups we need to change the binding. Experience from working with lots
of hardware engineers tells us that if something can be done (if there
are no constraints in HW) it will be done sooner or later.
If the hardware allows this much freedom we should also allow it in the
pinctrl binding.

> Then put the pin configuration (bias etc) in a separate node in the same
> state definition like that:
> 
> i2c0_pins_a {
>          function = "i2c0";
>          groups = "i2c0-pos0";
> };
> i2c0_pins_b {
>          bias-disable;
> };

The problem with that is that different pins might need different
configuration for the same muxed function. To properly reflect this we
would need to duplicate the pin definitions. One popular example is the
MMC interface where part of the pins need to have a pull-up, while
others don't. How would you reflect this with the DT description above?

Regards,
Lucas

-- 
Pengutronix e.K.             | Lucas Stach                 |
Industrial Linux Solutions   | http://www.pengutronix.de/  |

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/