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>
---
 drivers/pinctrl/pinctrl-scmi.c          | 3 +++
 include/linux/pinctrl/pinconf-generic.h | 3 +++
 2 files changed, 6 insertions(+)

diff --git a/drivers/pinctrl/pinctrl-scmi.c b/drivers/pinctrl/pinctrl-scmi.c
index 383681041e4c..d1f2f971cd96 100644
--- a/drivers/pinctrl/pinctrl-scmi.c
+++ b/drivers/pinctrl/pinctrl-scmi.c
@@ -250,6 +250,9 @@ static int pinctrl_scmi_map_pinconf_type(enum pin_config_param param,
 	case PIN_CONFIG_INPUT_SCHMITT_ENABLE:
 		*type = SCMI_PIN_INPUT_MODE;
 		break;
+	case PIN_CONFIG_INPUT_VALUE:
+		*type = SCMI_PIN_INPUT_VALUE;
+		break;
 	case PIN_CONFIG_MODE_LOW_POWER:
 		*type = SCMI_PIN_LOW_POWER_MODE;
 		break;
diff --git a/include/linux/pinctrl/pinconf-generic.h b/include/linux/pinctrl/pinconf-generic.h
index 1bcf071b860e..b37838171581 100644
--- a/include/linux/pinctrl/pinconf-generic.h
+++ b/include/linux/pinctrl/pinconf-generic.h
@@ -83,6 +83,8 @@ struct pinctrl_map;
  *      schmitt-trigger mode is disabled.
  * @PIN_CONFIG_INPUT_SCHMITT_UV: this will configure an input pin to run in
  *	schmitt-trigger mode. The argument is in uV.
+ * @PIN_CONFIG_INPUT_VALUE: This is used in SCMI to read the value from the
+ *	pin.
  * @PIN_CONFIG_MODE_LOW_POWER: this will configure the pin for low power
  *	operation, if several modes of operation are supported these can be
  *	passed in the argument on a custom form, else just use argument 1
@@ -135,6 +137,7 @@ enum pin_config_param {
 	PIN_CONFIG_INPUT_SCHMITT,
 	PIN_CONFIG_INPUT_SCHMITT_ENABLE,
 	PIN_CONFIG_INPUT_SCHMITT_UV,
+	PIN_CONFIG_INPUT_VALUE,
 	PIN_CONFIG_MODE_LOW_POWER,
 	PIN_CONFIG_MODE_PWM,
 	PIN_CONFIG_OUTPUT,
-- 
2.47.2

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.

Yours,
Linus Walleij

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.

Yours,
Linus Walleij

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

On Sun, Jul 20, 2025 at 02:38:59PM -0500, Dan Carpenter 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()

If this is the case, should not this commit have also a Fixes tag ?

Cheers,
Cristian