Re: [PATCH 1/4] pinctrl: Create a select_state variant with the ofnode

[Roger Quadros <rogerq@kernel.org>]

On 20/07/2023 12:55, Maxime Ripard wrote:
> Some drivers might not follow entirely the driver/device association,
> and thus might support what should be multiple devices in a single
> driver.
> 
> Such a driver is am65-cpsw-nuss, where the MAC and MDIO controllers are
> different device tree nodes, with their own resources (including pinctrl
> pins) but supported by a single driver tied to the MAC device in U-Boot.
> 
> In order to get the proper pinctrl state, we would need to get the
> state from the MDIO device tree node, but tie it to the MAC device since
> it's the only thing we have access to.
> 
> Signed-off-by: Maxime Ripard <mripard@kernel.org>
> ---
>  drivers/pinctrl/pinctrl-uclass.c | 15 +++++++++------
>  include/dm/pinctrl.h             | 26 ++++++++++++++++++++------
>  2 files changed, 29 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/pinctrl/pinctrl-uclass.c b/drivers/pinctrl/pinctrl-uclass.c
> index 73dd7b1038bb..9e28f1858cbd 100644
> --- a/drivers/pinctrl/pinctrl-uclass.c
> +++ b/drivers/pinctrl/pinctrl-uclass.c
> @@ -53,7 +53,8 @@ static int pinctrl_config_one(struct udevice *config)
>   * @statename: state name, like "default"
>   * @return: 0 on success, or negative error code on failure
>   */
> -static int pinctrl_select_state_full(struct udevice *dev, const char *statename)
> +static int pinctrl_select_state_full(struct udevice *dev, ofnode node,
> +				     const char *statename)
>  {
>  	char propname[32]; /* long enough */
>  	const fdt32_t *list;
> @@ -61,7 +62,7 @@ static int pinctrl_select_state_full(struct udevice *dev, const char *statename)
>  	struct udevice *config;
>  	int state, size, i, ret;
>  
> -	state = dev_read_stringlist_search(dev, "pinctrl-names", statename);
> +	state = ofnode_stringlist_search(node, "pinctrl-names", statename);
>  	if (state < 0) {
>  		char *end;
>  		/*
> @@ -74,7 +75,7 @@ static int pinctrl_select_state_full(struct udevice *dev, const char *statename)
>  	}
>  
>  	snprintf(propname, sizeof(propname), "pinctrl-%d", state);
> -	list = dev_read_prop(dev, propname, &size);
> +	list = ofnode_get_property(node, propname, &size);
>  	if (!list)
>  		return -ENOSYS;
>  
> @@ -293,20 +294,22 @@ static int pinctrl_select_state_simple(struct udevice *dev)
>  	return ops->set_state_simple(pctldev, dev);
>  }
>  
> -int pinctrl_select_state(struct udevice *dev, const char *statename)
> +int pinctrl_select_state_by_ofnode(struct udevice *dev, ofnode node,
> +				   const char *statename)
>  {
>  	/*
>  	 * Some device which is logical like mmc.blk, do not have
>  	 * a valid ofnode.
>  	 */
> -	if (!dev_has_ofnode(dev))
> +	if (!ofnode_valid(node))
>  		return 0;
> +
>  	/*
>  	 * Try full-implemented pinctrl first.
>  	 * If it fails or is not implemented, try simple one.
>  	 */
>  	if (CONFIG_IS_ENABLED(PINCTRL_FULL))
> -		return pinctrl_select_state_full(dev, statename);
> +		return pinctrl_select_state_full(dev, node, statename);
>  
>  	return pinctrl_select_state_simple(dev);
>  }
> diff --git a/include/dm/pinctrl.h b/include/dm/pinctrl.h
> index e3e50afeaff0..be4679b7f20d 100644
> --- a/include/dm/pinctrl.h
> +++ b/include/dm/pinctrl.h
> @@ -502,6 +502,24 @@ static inline int pinctrl_generic_set_state(struct udevice *pctldev,
>  #endif
>  
>  #if CONFIG_IS_ENABLED(PINCTRL)
> +/**
> + * pinctrl_select_state_by_ofnode() - Set a device to a given state using the given ofnode
> + * @dev:	Peripheral device
> + * @ofnode:	Device Tree node to get the state from
> + * @statename:	State name, like "default"
> + *
> + * Return: 0 on success, or negative error code on failure
> + */
> +int pinctrl_select_state_by_ofnode(struct udevice *dev, ofnode node, const char *statename);
> +#else
> +static inline int pinctrl_select_state_by_ofnode(struct udevice *dev,
> +						 ofnode node,
> +						 const char *statename)
> +{
> +	return -ENOSYS;
> +}

This allows & encourages one device driver to change another devices pinctrl state
which doesn't look like a good idea to me.

Please see if an alternative solution pointed out in patch2 works
so we don't have to need this patch.

> +#endif
> +
>  /**
>   * pinctrl_select_state() - Set a device to a given state
>   * @dev:	Peripheral device
> @@ -509,14 +527,10 @@ static inline int pinctrl_generic_set_state(struct udevice *pctldev,
>   *
>   * Return: 0 on success, or negative error code on failure
>   */
> -int pinctrl_select_state(struct udevice *dev, const char *statename);
> -#else
> -static inline int pinctrl_select_state(struct udevice *dev,
> -				       const char *statename)
> +static inline int pinctrl_select_state(struct udevice *dev, const char *statename)
>  {
> -	return -ENOSYS;
> +	return pinctrl_select_state_by_ofnode(dev, dev_ofnode(dev), statename);
>  }
> -#endif
>  
>  /**
>   * pinctrl_request() - Request a particular pinctrl function
> 

-- 
cheers,
-roger