[U-Boot] [PATCH v2] Add 16-bit single register pin controller support

[Felix Brack]

Hello James,

This patch does not compile without errors.

On 06.04.2017 07:38, James Balean wrote:
> Enables the pinctrl-single driver to support 16-bit registers. Only
> 32-bit registers were supported previously. Reduced width registers are
> required for some platforms, such as OMAP.
> 
> Signed-off-by: James Balean <james at balean.com.au>
> Cc: Felix Brack <fb at ltec.ch>
> Cc: Simon Glass <sjg at chromium.org>
> ---
> Changes for v2:
>   - Added explanation of why this patch is needed.
>   - Changed fdt32_t to ulong type.
>   - Removed 8-bit support.
>   - Now with a single read and write function, instead of one for each
>     register width.
> 
>  drivers/pinctrl/pinctrl-single.c | 45 ++++++++++++++++++++++++++--------------
>  1 file changed, 30 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c
> index d2dcec0..defb66f 100644
> --- a/drivers/pinctrl/pinctrl-single.c
> +++ b/drivers/pinctrl/pinctrl-single.c
> @@ -24,6 +24,30 @@ struct single_fdt_pin_cfg {
>  	fdt32_t val;		/* configuration register value */
>  };
>  
> +static ulong single_read(ulong reg, int width) {
> +	switch (size) {

Use 'width' instead of 'size' here.

> +	case 16:
> +		return readw(reg);
> +	case 32:
> +		return readl(reg);
> +	default:
> +		dev_warn(dev, "unsupported register width %i\n", width);

This function must return a value. What would you return here, i.e., in
case of failure?

> +	}
> +}
> +
> +static void single_write(ulong val, ulong reg, int width) {
> +	switch (width) {
> +	case 16:
> +		writew(val, reg);
> +		break;
> +	case 32:
> +		writel(val, reg);
> +		break;
> +	default:
> +		dev_warn(dev, "unsupported register width %i\n", width;

Missing closing parentheses.

> +	}
> +}
> +
>  /**
>   * single_configure_pins() - Configure pins based on FDT data
>   *
> @@ -47,28 +71,19 @@ static int single_configure_pins(struct udevice *dev,
>  	int n, reg;
>  	u32 val;
>  
> -	for (n = 0; n < count; n++) {
> +	for (n = 0; n < count; n++, pins++) {
>  		reg = fdt32_to_cpu(pins->reg);
>  		if ((reg < 0) || (reg > pdata->offset)) {
>  			dev_dbg(dev, "  invalid register offset 0x%08x\n", reg);
> -			pins++;
>  			continue;
>  		}
>  		reg += pdata->base;
> -		switch (pdata->width) {
> -		case 32:
> -			val = readl(reg) & ~pdata->mask;
> -			val |= fdt32_to_cpu(pins->val) & pdata->mask;
> -			writel(val, reg);
> -			dev_dbg(dev, "  reg/val 0x%08x/0x%08x\n",
> -				reg, val);
> -			break;
> -		default:
> -			dev_warn(dev, "unsupported register width %i\n",
> -				 pdata->width);
> -		}
> -		pins++;
> +		val = single_read(reg, pdata->width) & ~pdata->mask;

This is a no go as 'single_read' may fail (see above). You will have to
check the return value.
This is another reason for witch again I suggest you just keep the
switch statement as it was.

> +		val |= fdt32_to_cpu(pins->val) & pdata->mask;
> +		single_write(val, reg, pdata->width);
> +		dev_dbg(dev, "  reg/val 0x%08x/0x%08x\n", reg, val);
>  	}
> +
>  	return 0;
>  }
>  
> 

IMHO: You should make sure your patch is syntactically correct i.e. at
least run a build cycle. Furthermore and specifically for this patch (as
it directly deals with hardware registers) you should test it for '16
bit width' on your hardware.

regards Felix