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 <ja...@balean.com.au>
> Cc: Felix Brack <f...@ltec.ch>
> Cc: Simon Glass <s...@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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
