Hello James, hello Simon,
On 01.04.2017 06:22, Simon Glass wrote:
Hi James,
On 26 March 2017 at 23:55, James Balean james@balean.com.au wrote:
Enables the pinctrl-single driver to support 8 and 16-bit registers. Only 32-bit registers were supported previously.
Can you explain in your commit message why we want this?
Signed-off-by: James Balean james@balean.com.au Cc: Felix Brack fb@ltec.ch Cc: Simon Glass sjg@chromium.org
drivers/pinctrl/pinctrl-single.c | 58 ++++++++++++++++++++++++++++++++++------ 1 file changed, 50 insertions(+), 8 deletions(-)
diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c index d2dcec0..63fec8d 100644 --- a/drivers/pinctrl/pinctrl-single.c +++ b/drivers/pinctrl/pinctrl-single.c @@ -24,6 +24,36 @@ struct single_fdt_pin_cfg { fdt32_t val; /* configuration register value */ };
+static fdt32_t pcs_readb(fdt32_t reg)
I think ulong is better than fdt32_t, which is associated with devices tree.
+{
return readb(reg);+}
+static fdt32_t pcs_readw(fdt32_t reg) +{
return readw(reg);+}
+static fdt32_t pcs_readl(fdt32_t reg) +{
return readl(reg);+}
+static void pcs_writeb(fdt32_t val, fdt32_t reg) +{
writeb(val, reg);+}
+static void pcs_writew(fdt32_t val, fdt32_t reg) +{
writew(val, reg);+}
+static void pcs_writel(fdt32_t val, fdt32_t reg) +{
writel(val, reg);+}
Instead of lots of little functions, could you have:
pcs_read(ulong reg, int size) { switch (size) { case 8: return readb(reg); ...
?
I also prefer this. The corresponding switch is already there in 'single_configure_pins(..)', i.e. no need for an additional function. Using the existing function also eliminates the 'pcs_' prefix which I would have preferred to be 'single_' due to naming consistency (nitpicking, I admit).
/**
- single_configure_pins() - Configure pins based on FDT data
@@ -46,28 +76,40 @@ static int single_configure_pins(struct udevice *dev, int count = size / sizeof(struct single_fdt_pin_cfg); int n, reg; u32 val;
fdt32_t (*pcs_read)(fdt32_t reg);void (*pcs_write)(fdt32_t val, fdt32_t reg);
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 8:pcs_read = pcs_readb;pcs_write = pcs_writeb;break;case 16:pcs_read = pcs_readw;pcs_write = pcs_writew;break; 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);
pcs_read = pcs_readl;pcs_write = pcs_writel; break; default: dev_warn(dev, "unsupported register width %i\n", pdata->width);continue; }
pins++;
val = pcs_read(reg) & ~pdata->mask;val |= fdt32_to_cpu(pins->val) & pdata->mask;pcs_write(val, reg);dev_dbg(dev, " reg/val 0x%08x/0x%08x\n",reg, val); } return 0;}
2.7.4
Regards, Simon
regards, Felix