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

[Chen-Yu Tsai <wens@xxxxxxxxxx>]

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?

On MediaTek hardware, they have input enable controls, but not for output
enable. So mapping directly to GPIO direction doesn't quite make sense.

ChenYu

> +       case PIN_CONFIG_OUTPUT_ENABLE:
> +       case PIN_CONFIG_OUTPUT:
> +               if (fsel != BCM2835_FSEL_GPIO_OUT)
> +                       return -EINVAL;
> +
> +               val = bcm2835_gpio_get_bit(pc, GPLEV0, pin);
> +               *config = pinconf_to_config_packed(param, val);
> +               break;
>
> Does it seem reasonable?
>
> Yours,
> Linus Walleij