From mboxrd@z Thu Jan 1 00:00:00 1970 From: Felix Brack Date: Fri, 7 Apr 2017 15:16:16 +0200 Subject: [U-Boot] [PATCH v2] Add 16-bit single register pin controller support In-Reply-To: <1491457108-11185-1-git-send-email-james@balean.com.au> References: <1490594146-4562-1-git-send-email-james@balean.com.au> <760ef132-6b3b-b219-e5f6-00e95927dcbe@ltec.ch> <1491456859-11010-1-git-send-email-james@balean.com.au> <1491457108-11185-1-git-send-email-james@balean.com.au> Message-ID: <03776c3c-021b-b810-42bf-4938541c1c31@ltec.ch> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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 > Cc: Felix Brack > Cc: Simon Glass > --- > 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