| Message ID | 20200429163446.15390-2-rayagonda.kokatanur@broadcom.com |
|---|---|
| State | New |
| Headers | show |
| Series | extend pinctrl-single driver with APIs | expand |
Hi Rayagonda, +Stephen Warren On Wed, 29 Apr 2020 at 10:35, Rayagonda Kokatanur <rayagonda.kokatanur at broadcom.com> wrote: > > Add support to use different register read/write api's > based on register width. > > Signed-off-by: Rayagonda Kokatanur <rayagonda.kokatanur at broadcom.com> > --- > drivers/pinctrl/pinctrl-single.c | 98 ++++++++++++++++++++++++-------- > 1 file changed, 74 insertions(+), 24 deletions(-) > > diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c > index 738f5bd636..aed113b083 100644 > --- a/drivers/pinctrl/pinctrl-single.c > +++ b/drivers/pinctrl/pinctrl-single.c > @@ -10,12 +10,24 @@ > #include <linux/libfdt.h> > #include <asm/io.h> > > +/** > + * struct single_pdata - pinctrl device instance > + * @base first configuration register > + * @offset index of last configuration register > + * @mask configuration-value mask bits > + * @width configuration register bit width > + * @bits_per_mux > + * @read register read function to use > + * @write register write function to use > + */ > struct single_pdata { > fdt_addr_t base; /* first configuration register */ > int offset; /* index of last configuration register */ > u32 mask; /* configuration-value mask bits */ > int width; /* configuration register bit width */ > bool bits_per_mux; > + u32 (*read)(phys_addr_t reg); > + void (*write)(u32 val, phys_addr_t reg); Can't we just have a read & write function with a switch statement? Why do we need function pointers? Regards, Simon
On Wed, Apr 29, 2020 at 11:34 PM Simon Glass <sjg at chromium.org> wrote: > > Hi Rayagonda, > > +Stephen Warren > > On Wed, 29 Apr 2020 at 10:35, Rayagonda Kokatanur > <rayagonda.kokatanur at broadcom.com> wrote: > > > > Add support to use different register read/write api's > > based on register width. > > > > Signed-off-by: Rayagonda Kokatanur <rayagonda.kokatanur at broadcom.com> > > --- > > drivers/pinctrl/pinctrl-single.c | 98 ++++++++++++++++++++++++-------- > > 1 file changed, 74 insertions(+), 24 deletions(-) > > > > diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c > > index 738f5bd636..aed113b083 100644 > > --- a/drivers/pinctrl/pinctrl-single.c > > +++ b/drivers/pinctrl/pinctrl-single.c > > @@ -10,12 +10,24 @@ > > #include <linux/libfdt.h> > > #include <asm/io.h> > > > > +/** > > + * struct single_pdata - pinctrl device instance > > + * @base first configuration register > > + * @offset index of last configuration register > > + * @mask configuration-value mask bits > > + * @width configuration register bit width > > + * @bits_per_mux > > + * @read register read function to use > > + * @write register write function to use > > + */ > > struct single_pdata { > > fdt_addr_t base; /* first configuration register */ > > int offset; /* index of last configuration register */ > > u32 mask; /* configuration-value mask bits */ > > int width; /* configuration register bit width */ > > bool bits_per_mux; > > + u32 (*read)(phys_addr_t reg); > > + void (*write)(u32 val, phys_addr_t reg); > > Can't we just have a read & write function with a switch statement? > Why do we need function pointers? I referred to the linux pinctrl-single.c and kept code similar to linux. Please let me know. > > Regards, > Simon
Hi Rayagonda, On Thu, 30 Apr 2020 at 04:06, Rayagonda Kokatanur <rayagonda.kokatanur at broadcom.com> wrote: > > On Wed, Apr 29, 2020 at 11:34 PM Simon Glass <sjg at chromium.org> wrote: > > > > Hi Rayagonda, > > > > +Stephen Warren > > > > On Wed, 29 Apr 2020 at 10:35, Rayagonda Kokatanur > > <rayagonda.kokatanur at broadcom.com> wrote: > > > > > > Add support to use different register read/write api's > > > based on register width. > > > > > > Signed-off-by: Rayagonda Kokatanur <rayagonda.kokatanur at broadcom.com> > > > --- > > > drivers/pinctrl/pinctrl-single.c | 98 ++++++++++++++++++++++++-------- > > > 1 file changed, 74 insertions(+), 24 deletions(-) > > > > > > diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c > > > index 738f5bd636..aed113b083 100644 > > > --- a/drivers/pinctrl/pinctrl-single.c > > > +++ b/drivers/pinctrl/pinctrl-single.c > > > @@ -10,12 +10,24 @@ > > > #include <linux/libfdt.h> > > > #include <asm/io.h> > > > > > > +/** > > > + * struct single_pdata - pinctrl device instance > > > + * @base first configuration register > > > + * @offset index of last configuration register > > > + * @mask configuration-value mask bits > > > + * @width configuration register bit width > > > + * @bits_per_mux > > > + * @read register read function to use > > > + * @write register write function to use > > > + */ > > > struct single_pdata { > > > fdt_addr_t base; /* first configuration register */ > > > int offset; /* index of last configuration register */ > > > u32 mask; /* configuration-value mask bits */ > > > int width; /* configuration register bit width */ > > > bool bits_per_mux; > > > + u32 (*read)(phys_addr_t reg); > > > + void (*write)(u32 val, phys_addr_t reg); > > > > Can't we just have a read & write function with a switch statement? > > Why do we need function pointers? > > I referred to the linux pinctrl-single.c and kept code similar to linux. > Please let me know. See the regmap discussion here which is related: http://patchwork.ozlabs.org/project/uboot/patch/20191105114700.24989-3-jjhiblot at ti.com/ Should this driver use regmap, then? Regards, Simon
Hi Simon, On Fri, May 8, 2020 at 7:07 AM Simon Glass <sjg at chromium.org> wrote: > > Hi Rayagonda, > > On Thu, 30 Apr 2020 at 04:06, Rayagonda Kokatanur > <rayagonda.kokatanur at broadcom.com> wrote: > > > > On Wed, Apr 29, 2020 at 11:34 PM Simon Glass <sjg at chromium.org> wrote: > > > > > > Hi Rayagonda, > > > > > > +Stephen Warren > > > > > > On Wed, 29 Apr 2020 at 10:35, Rayagonda Kokatanur > > > <rayagonda.kokatanur at broadcom.com> wrote: > > > > > > > > Add support to use different register read/write api's > > > > based on register width. > > > > > > > > Signed-off-by: Rayagonda Kokatanur <rayagonda.kokatanur at broadcom.com> > > > > --- > > > > drivers/pinctrl/pinctrl-single.c | 98 ++++++++++++++++++++++++-------- > > > > 1 file changed, 74 insertions(+), 24 deletions(-) > > > > > > > > diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c > > > > index 738f5bd636..aed113b083 100644 > > > > --- a/drivers/pinctrl/pinctrl-single.c > > > > +++ b/drivers/pinctrl/pinctrl-single.c > > > > @@ -10,12 +10,24 @@ > > > > #include <linux/libfdt.h> > > > > #include <asm/io.h> > > > > > > > > +/** > > > > + * struct single_pdata - pinctrl device instance > > > > + * @base first configuration register > > > > + * @offset index of last configuration register > > > > + * @mask configuration-value mask bits > > > > + * @width configuration register bit width > > > > + * @bits_per_mux > > > > + * @read register read function to use > > > > + * @write register write function to use > > > > + */ > > > > struct single_pdata { > > > > fdt_addr_t base; /* first configuration register */ > > > > int offset; /* index of last configuration register */ > > > > u32 mask; /* configuration-value mask bits */ > > > > int width; /* configuration register bit width */ > > > > bool bits_per_mux; > > > > + u32 (*read)(phys_addr_t reg); > > > > + void (*write)(u32 val, phys_addr_t reg); > > > > > > Can't we just have a read & write function with a switch statement? > > > Why do we need function pointers? > > > > I referred to the linux pinctrl-single.c and kept code similar to linux. > > Please let me know. > > See the regmap discussion here which is related: > > http://patchwork.ozlabs.org/project/uboot/patch/20191105114700.24989-3-jjhiblot at ti.com/ > > Should this driver use regmap, then? I think using a function pointer is a better approach, we can check for errors in one place ie invalid register width. Right now it's been done in single_probe() function. Please let me know. Best regards, Rayagonda > > Regards, > Simon
Hi Rayagonda, On Sun, 24 May 2020 at 04:46, Rayagonda Kokatanur <rayagonda.kokatanur at broadcom.com> wrote: > > Hi Simon, > > On Fri, May 8, 2020 at 7:07 AM Simon Glass <sjg at chromium.org> wrote: > > > > Hi Rayagonda, > > > > On Thu, 30 Apr 2020 at 04:06, Rayagonda Kokatanur > > <rayagonda.kokatanur at broadcom.com> wrote: > > > > > > On Wed, Apr 29, 2020 at 11:34 PM Simon Glass <sjg at chromium.org> wrote: > > > > > > > > Hi Rayagonda, > > > > > > > > +Stephen Warren > > > > > > > > On Wed, 29 Apr 2020 at 10:35, Rayagonda Kokatanur > > > > <rayagonda.kokatanur at broadcom.com> wrote: > > > > > > > > > > Add support to use different register read/write api's > > > > > based on register width. > > > > > > > > > > Signed-off-by: Rayagonda Kokatanur <rayagonda.kokatanur at broadcom.com> > > > > > --- > > > > > drivers/pinctrl/pinctrl-single.c | 98 ++++++++++++++++++++++++-------- > > > > > 1 file changed, 74 insertions(+), 24 deletions(-) > > > > > > > > > > diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c > > > > > index 738f5bd636..aed113b083 100644 > > > > > --- a/drivers/pinctrl/pinctrl-single.c > > > > > +++ b/drivers/pinctrl/pinctrl-single.c > > > > > @@ -10,12 +10,24 @@ > > > > > #include <linux/libfdt.h> > > > > > #include <asm/io.h> > > > > > > > > > > +/** > > > > > + * struct single_pdata - pinctrl device instance > > > > > + * @base first configuration register > > > > > + * @offset index of last configuration register > > > > > + * @mask configuration-value mask bits > > > > > + * @width configuration register bit width > > > > > + * @bits_per_mux > > > > > + * @read register read function to use > > > > > + * @write register write function to use > > > > > + */ > > > > > struct single_pdata { > > > > > fdt_addr_t base; /* first configuration register */ > > > > > int offset; /* index of last configuration register */ > > > > > u32 mask; /* configuration-value mask bits */ > > > > > int width; /* configuration register bit width */ > > > > > bool bits_per_mux; > > > > > + u32 (*read)(phys_addr_t reg); > > > > > + void (*write)(u32 val, phys_addr_t reg); > > > > > > > > Can't we just have a read & write function with a switch statement? > > > > Why do we need function pointers? > > > > > > I referred to the linux pinctrl-single.c and kept code similar to linux. > > > Please let me know. > > > > See the regmap discussion here which is related: > > > > http://patchwork.ozlabs.org/project/uboot/patch/20191105114700.24989-3-jjhiblot at ti.com/ > > > > Should this driver use regmap, then? > > I think using a function pointer is a better approach, we can check > for errors in one place ie invalid register width. > Right now it's been done in single_probe() function. > Please let me know. What sort of errors? I'm sorry but I prefer the switch() for U-Boot. We have different constraints from Linux. After all, our file is 200 lines and in Linux this is 2k lines. Regards, Simon
Hi Simon, On Mon, May 25, 2020 at 7:45 AM Simon Glass <sjg at chromium.org> wrote: > > Hi Rayagonda, > > On Sun, 24 May 2020 at 04:46, Rayagonda Kokatanur > <rayagonda.kokatanur at broadcom.com> wrote: > > > > Hi Simon, > > > > On Fri, May 8, 2020 at 7:07 AM Simon Glass <sjg at chromium.org> wrote: > > > > > > Hi Rayagonda, > > > > > > On Thu, 30 Apr 2020 at 04:06, Rayagonda Kokatanur > > > <rayagonda.kokatanur at broadcom.com> wrote: > > > > > > > > On Wed, Apr 29, 2020 at 11:34 PM Simon Glass <sjg at chromium.org> wrote: > > > > > > > > > > Hi Rayagonda, > > > > > > > > > > +Stephen Warren > > > > > > > > > > On Wed, 29 Apr 2020 at 10:35, Rayagonda Kokatanur > > > > > <rayagonda.kokatanur at broadcom.com> wrote: > > > > > > > > > > > > Add support to use different register read/write api's > > > > > > based on register width. > > > > > > > > > > > > Signed-off-by: Rayagonda Kokatanur <rayagonda.kokatanur at broadcom.com> > > > > > > --- > > > > > > drivers/pinctrl/pinctrl-single.c | 98 ++++++++++++++++++++++++-------- > > > > > > 1 file changed, 74 insertions(+), 24 deletions(-) > > > > > > > > > > > > diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c > > > > > > index 738f5bd636..aed113b083 100644 > > > > > > --- a/drivers/pinctrl/pinctrl-single.c > > > > > > +++ b/drivers/pinctrl/pinctrl-single.c > > > > > > @@ -10,12 +10,24 @@ > > > > > > #include <linux/libfdt.h> > > > > > > #include <asm/io.h> > > > > > > > > > > > > +/** > > > > > > + * struct single_pdata - pinctrl device instance > > > > > > + * @base first configuration register > > > > > > + * @offset index of last configuration register > > > > > > + * @mask configuration-value mask bits > > > > > > + * @width configuration register bit width > > > > > > + * @bits_per_mux > > > > > > + * @read register read function to use > > > > > > + * @write register write function to use > > > > > > + */ > > > > > > struct single_pdata { > > > > > > fdt_addr_t base; /* first configuration register */ > > > > > > int offset; /* index of last configuration register */ > > > > > > u32 mask; /* configuration-value mask bits */ > > > > > > int width; /* configuration register bit width */ > > > > > > bool bits_per_mux; > > > > > > + u32 (*read)(phys_addr_t reg); > > > > > > + void (*write)(u32 val, phys_addr_t reg); > > > > > > > > > > Can't we just have a read & write function with a switch statement? > > > > > Why do we need function pointers? > > > > > > > > I referred to the linux pinctrl-single.c and kept code similar to linux. > > > > Please let me know. > > > > > > See the regmap discussion here which is related: > > > > > > http://patchwork.ozlabs.org/project/uboot/patch/20191105114700.24989-3-jjhiblot at ti.com/ > > > > > > Should this driver use regmap, then? > > > > I think using a function pointer is a better approach, we can check > > for errors in one place ie invalid register width. > > Right now it's been done in single_probe() function. > > Please let me know. > > What sort of errors? What I mean is, right now read/write function pointres are getting initialized in single_probe() based on pdata->width. If pdata->width is invalid, its erroring out there only. So if we use a single read and write function with switch statement then checking pdata->width should be part of this switch statement. This means every call to read/write should check for failure. Hence I am thinking function pointer is a better approach. Please let me know. Best regards, Rayagonda > > I'm sorry but I prefer the switch() for U-Boot. We have different > constraints from Linux. After all, our file is 200 lines and in Linux > this is 2k lines. > > Regards, > Simon
diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c index 738f5bd636..aed113b083 100644 --- a/drivers/pinctrl/pinctrl-single.c +++ b/drivers/pinctrl/pinctrl-single.c @@ -10,12 +10,24 @@ #include <linux/libfdt.h> #include <asm/io.h> +/** + * struct single_pdata - pinctrl device instance + * @base first configuration register + * @offset index of last configuration register + * @mask configuration-value mask bits + * @width configuration register bit width + * @bits_per_mux + * @read register read function to use + * @write register write function to use + */ struct single_pdata { fdt_addr_t base; /* first configuration register */ int offset; /* index of last configuration register */ u32 mask; /* configuration-value mask bits */ int width; /* configuration register bit width */ bool bits_per_mux; + u32 (*read)(phys_addr_t reg); + void (*write)(u32 val, phys_addr_t reg); }; struct single_fdt_pin_cfg { @@ -23,6 +35,36 @@ struct single_fdt_pin_cfg { fdt32_t val; /* configuration register value */ }; +static u32 __maybe_unused single_readb(phys_addr_t reg) +{ + return readb(reg); +} + +static u32 __maybe_unused single_readw(phys_addr_t reg) +{ + return readw(reg); +} + +static u32 __maybe_unused single_readl(phys_addr_t reg) +{ + return readl(reg); +} + +static void __maybe_unused single_writeb(u32 val, phys_addr_t reg) +{ + writeb(val, reg); +} + +static void __maybe_unused single_writew(u32 val, phys_addr_t reg) +{ + writew(val, reg); +} + +static void __maybe_unused single_writel(u32 val, phys_addr_t reg) +{ + writel(val, reg); +} + struct single_fdt_bits_cfg { fdt32_t reg; /* configuration register offset */ fdt32_t val; /* configuration register value */ @@ -60,18 +102,9 @@ static int single_configure_pins(struct udevice *dev, } reg += pdata->base; val = fdt32_to_cpu(pins->val) & pdata->mask; - switch (pdata->width) { - case 16: - writew((readw(reg) & ~pdata->mask) | val, reg); - break; - case 32: - writel((readl(reg) & ~pdata->mask) | val, reg); - break; - default: - dev_warn(dev, "unsupported register width %i\n", - pdata->width); - continue; - } + val |= (pdata->read(reg) & ~pdata->mask); + pdata->write(val, reg); + dev_dbg(dev, " reg/val 0x%pa/0x%08x\n", ®, val); } return 0; @@ -97,18 +130,8 @@ static int single_configure_bits(struct udevice *dev, mask = fdt32_to_cpu(pins->mask); val = fdt32_to_cpu(pins->val) & mask; - switch (pdata->width) { - case 16: - writew((readw(reg) & ~mask) | val, reg); - break; - case 32: - writel((readl(reg) & ~mask) | val, reg); - break; - default: - dev_warn(dev, "unsupported register width %i\n", - pdata->width); - continue; - } + val |= (pdata->read(reg) & ~mask); + pdata->write(val, reg); dev_dbg(dev, " reg/val 0x%pa/0x%08x\n", ®, val); } return 0; @@ -148,6 +171,32 @@ static int single_set_state(struct udevice *dev, return len; } +static int single_probe(struct udevice *dev) +{ + struct single_pdata *pdata = dev->platdata; + + switch (pdata->width) { + case 8: + pdata->read = single_readb; + pdata->write = single_writeb; + break; + case 16: + pdata->read = single_readw; + pdata->write = single_writew; + break; + case 32: + pdata->read = single_readl; + pdata->write = single_writel; + break; + default: + dev_warn(dev, "%s: unsupported register width %d\n", + __func__, pdata->width); + return -EINVAL; + } + + return 0; +} + static int single_ofdata_to_platdata(struct udevice *dev) { fdt_addr_t addr; @@ -193,4 +242,5 @@ U_BOOT_DRIVER(single_pinctrl) = { .ops = &single_pinctrl_ops, .platdata_auto_alloc_size = sizeof(struct single_pdata), .ofdata_to_platdata = single_ofdata_to_platdata, + .probe = single_probe, };
Add support to use different register read/write api's based on register width. Signed-off-by: Rayagonda Kokatanur <rayagonda.kokatanur at broadcom.com> --- drivers/pinctrl/pinctrl-single.c | 98 ++++++++++++++++++++++++-------- 1 file changed, 74 insertions(+), 24 deletions(-)