mbox series

[RFC,0/7] pinctrl-scmi: Add GPIO support

Message ID cover.1746443762.git.dan.carpenter@linaro.org
Headers show
Series pinctrl-scmi: Add GPIO support | expand

Message

Dan Carpenter May 5, 2025, 11:36 a.m. UTC 
This patchset adds GPIO support to the SCMI pin control driver.
AKASHI Takahiro did some of that work earlier, but we decided to make
this a part of the pinctrl driver instead of a separate GPIO driver.

I'm not really sure how the device tree stuff wires it all in.  I've
been using a mock driver on SCP with virtio to test it.

Dan Carpenter (7):
  firmware: arm_scmi: move boiler plate code into the get info functions
  firmware: arm_scmi: add is_gpio() function
  pinctrl: introduce pinctrl_gpio_get_config()
  pinctrl-scmi: implement PIN_CONFIG_INPUT_VALUE
  pinctrl: Delete PIN_CONFIG_OUTPUT_IMPEDANCE_OHMS support
  pinctrl-scmi: Add GPIO support
  pinctrl-scmi: remove unused struct member

 drivers/firmware/arm_scmi/pinctrl.c     | 146 +++++++++-------
 drivers/pinctrl/core.c                  |  35 ++++
 drivers/pinctrl/pinctrl-scmi.c          | 213 +++++++++++++++++++++++-
 include/linux/pinctrl/consumer.h        |   9 +
 include/linux/pinctrl/pinconf-generic.h |   3 +
 include/linux/scmi_protocol.h           |   2 +
 6 files changed, 339 insertions(+), 69 deletions(-)

Comments

ALOK TIWARI May 5, 2025, 4:30 p.m. UTC | #1 
Hi Dan,


Thanks for your patch.

On 05-05-2025 17:07, Dan Carpenter wrote:
> From: AKASHI Takahiro <takahiro.akashi@linaro.org>
> 
> This is a counterpart of pinctrl_gpio_set_config(), which will initially
> be used to implement gpio_get interface in SCMI pinctrl based GPIO driver.
> 
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> ---
>   drivers/pinctrl/core.c           | 35 ++++++++++++++++++++++++++++++++
>   include/linux/pinctrl/consumer.h |  9 ++++++++
>   2 files changed, 44 insertions(+)
> 
> diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
> index 4bdbf6bb26e2..4310f9e2118b 100644
> --- a/drivers/pinctrl/core.c
> +++ b/drivers/pinctrl/core.c
> @@ -30,6 +30,7 @@
>   #include <linux/pinctrl/consumer.h>
>   #include <linux/pinctrl/devinfo.h>
>   #include <linux/pinctrl/machine.h>
> +#include <linux/pinctrl/pinconf.h>
>   #include <linux/pinctrl/pinctrl.h>
>   
>   #include "core.h"
> @@ -937,6 +938,40 @@ int pinctrl_gpio_set_config(struct gpio_chip *gc, unsigned int offset,
>   }
>   EXPORT_SYMBOL_GPL(pinctrl_gpio_set_config);
>   
> +/**
> + * pinctrl_gpio_get_config() - Get the config for a given GPIO pin
> + * @gc: GPIO chip structure from the GPIO subsystem
> + * @offset: hardware offset of the GPIO relative to the controller
> +  * @config: the configuration to query.  On success it holds the result

remove extra ' ' before * @config and On.

> + */
> +int pinctrl_gpio_get_config(struct gpio_chip *gc, unsigned int offset, unsigned long *config)
> +{
> +	struct pinctrl_gpio_range *range;
> +	const struct pinconf_ops *ops;
> +	struct pinctrl_dev *pctldev;
> +	int ret, pin;
> +
> +	ret = pinctrl_get_device_gpio_range(gc, offset, &pctldev, &range);
> +	if (ret)
> +		return ret;
> +
> +	ops = pctldev->desc->confops;
> +	if (!ops || !ops->pin_config_get)
> +		return -EINVAL;
> +
> +	mutex_lock(&pctldev->mutex);
> +	pin = gpio_to_pin(range, gc, offset);
> +	ret = ops->pin_config_get(pctldev, pin, config);

can we add reason here, as now we are not calling pin_config_get_for_pin()

https://lore.kernel.org/all/20231002021602.260100-3-takahiro.akashi@linaro.org/

> +	mutex_unlock(&pctldev->mutex);
> +
> +	if (ret)
> +		return ret;
> +
> +	*config = pinconf_to_config_argument(*config);

a '\n' before return.

> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(pinctrl_gpio_get_config);
> +
>   static struct pinctrl_state *find_state(struct pinctrl *p,
>   					const char *name)
>   {


Thanks,
Alok
Khaled Ali Ahmed May 7, 2025, 8:54 a.m. UTC | #2 
Good morning Dan,
Regarding the scmi_pinctrl stack design, what we have made in the SW is that the stack can communicate with multiple drivers with only two constraints:
1- Implement the interfacing APIs. which is declared by the object "struct mod_pinctrl_drv_api".
2- Integrate itself with the scmi_pinctrl HAL or backend as you prefer.
Also, we have an example I can discuss if you like.

thanks in advance
Dan Carpenter May 7, 2025, 11:32 a.m. UTC | #3 
On Mon, May 05, 2025 at 10:00:35PM +0530, ALOK TIWARI wrote:
> > +int pinctrl_gpio_get_config(struct gpio_chip *gc, unsigned int offset, unsigned long *config)
> > +{
> > +	struct pinctrl_gpio_range *range;
> > +	const struct pinconf_ops *ops;
> > +	struct pinctrl_dev *pctldev;
> > +	int ret, pin;
> > +
> > +	ret = pinctrl_get_device_gpio_range(gc, offset, &pctldev, &range);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ops = pctldev->desc->confops;
> > +	if (!ops || !ops->pin_config_get)
> > +		return -EINVAL;
> > +
> > +	mutex_lock(&pctldev->mutex);
> > +	pin = gpio_to_pin(range, gc, offset);
> > +	ret = ops->pin_config_get(pctldev, pin, config);
> 
> can we add reason here, as now we are not calling pin_config_get_for_pin()
> 
> https://lore.kernel.org/all/20231002021602.260100-3-takahiro.akashi@linaro.org/
> 

I don't even know why I changed that.  Using pin_config_get_for_pin()
works and it's cleaner.

regards,
dan carpenter