Re: [PATCH] pinctrl: single: add support for pinctrl-single, pins when #pinctrl-cells = 2

[Tom Rini <trini@konsulko.com>]

[-- Attachment #1: Type: text/plain, Size: 5562 bytes --]

On Fri, Dec 03, 2021 at 03:18:53PM +0000, AJ Bagwell wrote:

> Changes to the am33xx device (33e9021a) trees have been merged in from
> the upstream linux kernel which now means the device tree uses the new
> pins format (as of 5.10) where the confinguration can be stores as a
> separate configuration value and pin mux mode which are then OR'd
> together.
> 
> This patch adds support for the new format to u-boot so that
> pinctrl-cells is now respected when reading in pinctrl-single,pins
> ---
>  drivers/pinctrl/pinctrl-single.c | 55 +++++++++++++++++---------------
>  1 file changed, 29 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c
> index 8fc07e3498..bc9c17bce8 100644
> --- a/drivers/pinctrl/pinctrl-single.c
> +++ b/drivers/pinctrl/pinctrl-single.c
> @@ -28,6 +28,7 @@ struct single_pdata {
>  	int offset;
>  	u32 mask;
>  	u32 width;
> +	u32 args_count;
>  	bool bits_per_mux;
>  };
>  
> @@ -78,20 +79,6 @@ struct single_priv {
>  	struct list_head gpiofuncs;
>  };
>  
> -/**
> - * struct single_fdt_pin_cfg - pin configuration
> - *
> - * This structure is used for the pin configuration parameters in case
> - * the register controls only one pin.
> - *
> - * @reg: configuration register offset
> - * @val: configuration register value
> - */
> -struct single_fdt_pin_cfg {
> -	fdt32_t reg;
> -	fdt32_t val;
> -};
> -
>  /**
>   * struct single_fdt_bits_cfg - pin configuration
>   *
> @@ -314,25 +301,28 @@ static int single_pin_compare(const void *s1, const void *s2)
>   * @dev: Pointer to single pin configuration device which is the parent of
>   *       the pins node holding the pin configuration data.
>   * @pins: Pointer to the first element of an array of register/value pairs
> - *        of type 'struct single_fdt_pin_cfg'. Each such pair describes the
> - *        the pin to be configured and the value to be used for configuration.
> + *        of type 'u32'. Each such pair describes the pin to be configured 
> + *        and the value to be used for configuration.
> + *        The value can either be a simple value if #pinctrl-cells = 1
> + *        or a configuration value and a pin mux mode value if it is 2
>   *        This pointer points to a 'pinctrl-single,pins' property in the
>   *        device-tree.
>   * @size: Size of the 'pins' array in bytes.
> - *        The number of register/value pairs in the 'pins' array therefore
> - *        equals to 'size / sizeof(struct single_fdt_pin_cfg)'.
> + *        The number of cells in the array therefore equals to
> + *        'size / sizeof(u32)'.
>   * @fname: Function name.
>   */
>  static int single_configure_pins(struct udevice *dev,
> -				 const struct single_fdt_pin_cfg *pins,
> +				 const u32 *pins,
>  				 int size, const char *fname)
>  {
>  	struct single_pdata *pdata = dev_get_plat(dev);
>  	struct single_priv *priv = dev_get_priv(dev);
> -	int n, pin, count = size / sizeof(struct single_fdt_pin_cfg);
> +	int stride = pdata->args_count + 1;
> +	int n, pin, count = size / sizeof(u32);
>  	struct single_func *func;
>  	phys_addr_t reg;
> -	u32 offset, val;
> +	u32 offset, val, mux;
>  
>  	/* If function mask is null, needn't enable it. */
>  	if (!pdata->mask)
> @@ -344,16 +334,22 @@ static int single_configure_pins(struct udevice *dev,
>  
>  	func->name = fname;
>  	func->npins = 0;
> -	for (n = 0; n < count; n++, pins++) {
> -		offset = fdt32_to_cpu(pins->reg);
> +	for (n = 0; n < count; n += stride) {
> +		offset = fdt32_to_cpu(pins[n]);
>  		if (offset > pdata->offset) {
>  			dev_err(dev, "  invalid register offset 0x%x\n",
>  				offset);
>  			continue;
>  		}
>  
> +		/* if the pinctrl-cells is 2 then the second cell contains the mux */
> +		if (stride == 3)
> +			mux = fdt32_to_cpu(pins[n + 2]);
> +		else
> +			mux = 0;
> +
>  		reg = pdata->base + offset;
> -		val = fdt32_to_cpu(pins->val) & pdata->mask;
> +		val = (fdt32_to_cpu(pins[n + 1]) | mux) & pdata->mask;
>  		pin = single_get_pin_by_offset(dev, offset);
>  		if (pin < 0) {
>  			dev_err(dev, "  failed to get pin by offset %x\n",
> @@ -453,7 +449,7 @@ static int single_configure_bits(struct udevice *dev,
>  static int single_set_state(struct udevice *dev,
>  			    struct udevice *config)
>  {
> -	const struct single_fdt_pin_cfg *prop;
> +	const u32 *prop;
>  	const struct single_fdt_bits_cfg *prop_bits;
>  	int len;
>  
> @@ -461,7 +457,7 @@ static int single_set_state(struct udevice *dev,
>  
>  	if (prop) {
>  		dev_dbg(dev, "configuring pins for %s\n", config->name);
> -		if (len % sizeof(struct single_fdt_pin_cfg)) {
> +		if (len % sizeof(u32)) {
>  			dev_dbg(dev, "  invalid pin configuration in fdt\n");
>  			return -FDT_ERR_BADSTRUCTURE;
>  		}
> @@ -612,6 +608,13 @@ static int single_of_to_plat(struct udevice *dev)
>  
>  	pdata->bits_per_mux = dev_read_bool(dev, "pinctrl-single,bit-per-mux");
>  
> +	/* If no pinctrl-cells is present, default to old style of 2 cells with
> +	 * bits per mux and 1 cell otherwise.
> +	 */
> +	ret = dev_read_u32(dev, "#pinctrl-cells", &pdata->args_count);
> +	if (ret)
> +		pdata->args_count = pdata->bits_per_mux ? 2 : 1;
> +
>  	return 0;
>  }

While this change seems fine, it is missing a Signed-off-by line.
Please see https://developercertificate.org/ and if so, either repost or
reply with your line here as the change otherwise still applies fine,
thanks.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]