Re: [PATCH V3 1/2] pinctrl: bcm2835: Implement bcm2835_pinconf_get

[Chen-Yu Tsai <wens@xxxxxxxxxx>]

On Wed, Mar 6, 2024 at 4:38 PM Linus Walleij <linus.walleij@xxxxxxxxxx> wrote:
> On Wed, Mar 6, 2024 at 4:10 AM Chen-Yu Tsai <wens@xxxxxxxxxx> wrote:
> > On Wed, Mar 6, 2024 at 6:38 AM Linus Walleij <linus.walleij@xxxxxxxxxx> wrote:
> > > On Sun, Mar 3, 2024 at 3:19 PM Chen-Yu Tsai <wens@xxxxxxxxxx> wrote:
> > >
> > > > Hi Linus,
> > > (...)
> > > > I'd like to take this opportunity to ask about INPUT_ENABLE and
> > > > OUTPUT_ENABLE.
> > > >
> > > > AFAICT from existing comments in include/linux/pinctrl/pinconf-generic.h ,
> > > > these two options refer to input and output buffers or connections within
> > > > the general electric path, i.e. it allows the signal to pass through in
> > > > a certain direction. It does not refer to the current selected direction
> > > > of the GPIO function, which is covered by the PIN_CONFIG_OUTPUT option,
> > > > and by gpiolib, if and only if the pin has been allocated for gpiolib
> > > > use. But it seems multiple existing drivers do this.
> > > >
> > > > What's the correct thing to do here?
> > >
> > > It's really up to the driver author: the text in pinconf-generic.h does its best
> > > to describe the intended semantics, but sometimes hardware will not fully
> > > match what is said in the documentation.
> > >
> > > I guess in this case the PIN_CONFIG_OUTPUT_ENABLE and
> > > PIN_CONFIG_OUTPUT is not two distinctly different things for this
> > > hardware so a reasonable semantic is to implement both in the same
> > > case and do the same for both of them.
> >
> > But that doesn't really match the intended semantics. Maybe the driver
> > should just ignore PIN_CONFIG_OUTPUT / PIN_CONFIG_INPUT if the hardware
> > doesn't have matching toggles?

I meant PIN_CONFIG_OUTPUT_ENABLE and PIN_CONFIG_INPUT_ENABLE here. Sorry.

> > On MediaTek hardware, they have input enable controls, but not for output
> > enable. So mapping directly to GPIO direction doesn't quite make sense.
>
> I think you should look what will work in practice, given the state
> definitions in the device tree file and the runtime requirements from
> GPIO.
>
> If these things add up and work in practice it's fine.
>
> Rough consensus and running code.

I agree that we shouldn't needlessly break existing platforms. But at the
same time we should prevent new additions of incorrect usage. Note that
I am specifically referring to the {INPUT,OUTPUT}_ENABLE configs. I agree
that PIN_CONFIG_OUTPUT works OK, even though I think in some cases a
gpio-hog should be used instead.

For the MediaTek device trees, the only two occurrences of "output-enable"
actually describe conflicting information:

    pins-rts {
            pinmux = <PINMUX_GPIO47__FUNC_URTS1>;
            output-enable;
    };

The above asks for the UART function on this pin, but based on existing
driver definitions, switches the function to GPIO output because of the
"output-enable" property. Hence the confusion.


ChenYu