Re: [PATCH RFC v2 4/7] pinctrl-scmi: add PIN_CONFIG_INPUT_VALUE

Linus Walleij
2025-09-05T14:33:12Z

On Fri, Sep 5, 2025 at 10:31 AM Linus Walleij <linus.walleij@linaro.org> wrote:
> On Fri, Sep 5, 2025 at 10:27 AM Linus Walleij <linus.walleij@linaro.org> wrote:
> > On Sun, Jul 20, 2025 at 9:39 PM Dan Carpenter <dan.carpenter@linaro.org> wrote:
> >
> > > In SCMI the value of the pin is just another configuration option. Add
> > > this as an option in the pin_config_param enum and creating a mapping to
> > > SCMI_PIN_INPUT_VALUE in pinctrl_scmi_map_pinconf_type()
> > >
> > > Since this is an RFC patch, I'm going to comment that I think the SCMI
> > > pinctrl driver misuses the PIN_CONFIG_OUTPUT enum. It should be for
> > > enabling and disabling output on pins which can serve as both input and
> > > output. Enabling it is supposed to write a 1 and disabling it is
> > > supposed to write a 0 but we use that side effect to write 1s and 0s. I
> > > did't change this because it would break userspace but I'd like to add a
> > > PIN_CONFIG_OUTPUT_VALUE enum as well and use that in the GPIO driver.
> > > But in this patchset I just use PIN_CONFIG_OUTPUT.
> > >
> > > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> >
> > I tweaked this patch around a bit and applied: removed the second comment
> > in the commit message and wrote the docs to be more generic since
> > in the future other things than SCMI might want to use this
> > config option.
>
> Then I thought about it some more. ...
>
> Isn't it more intuitive that we rewrite the curren PIN_CONFIG_OUTPUT_VALUE
> to just PIN_CONFIG_VALUE that can be used for both reading and
> writing binary low/high instead of having two different things like this?
>
> I will look over current users and maybe propose a patch.

I discovered that several in-tree drivers are already *reading* the
property PIN_CONFIG_OUTPUT_VALUE to get the logic level of
the line.

I sent a patch renaming this property to PIN_CONFIG_LEVEL so
it is clear that this can also be read, and you can drop this patch
and just read/write PIN_CONFIG_LEVEL instead for the GPIO
driver.

Yours,
Linus Walleij