diff mbox series

[v1,1/3] drivers: pinctrl-single: handle different register width

Message ID 20200429163446.15390-2-rayagonda.kokatanur@broadcom.com
State New
Delegated to: Tom Rini
Headers show
Series extend pinctrl-single driver with APIs | expand

Commit Message

Rayagonda Kokatanur April 29, 2020, 4:34 p.m. UTC
Add support to use different register read/write api's
based on register width.

Signed-off-by: Rayagonda Kokatanur <rayagonda.kokatanur@broadcom.com>
---
 drivers/pinctrl/pinctrl-single.c | 98 ++++++++++++++++++++++++--------
 1 file changed, 74 insertions(+), 24 deletions(-)

Comments

Simon Glass April 29, 2020, 6:04 p.m. UTC | #1
Hi Rayagonda,

+Stephen Warren

On Wed, 29 Apr 2020 at 10:35, Rayagonda Kokatanur
<rayagonda.kokatanur@broadcom.com> wrote:
>
> Add support to use different register read/write api's
> based on register width.
>
> Signed-off-by: Rayagonda Kokatanur <rayagonda.kokatanur@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
Rayagonda Kokatanur April 30, 2020, 10:06 a.m. UTC | #2
On Wed, Apr 29, 2020 at 11:34 PM Simon Glass <sjg@chromium.org> wrote:
>
> Hi Rayagonda,
>
> +Stephen Warren
>
> On Wed, 29 Apr 2020 at 10:35, Rayagonda Kokatanur
> <rayagonda.kokatanur@broadcom.com> wrote:
> >
> > Add support to use different register read/write api's
> > based on register width.
> >
> > Signed-off-by: Rayagonda Kokatanur <rayagonda.kokatanur@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
Simon Glass May 8, 2020, 1:36 a.m. UTC | #3
Hi Rayagonda,

On Thu, 30 Apr 2020 at 04:06, Rayagonda Kokatanur
<rayagonda.kokatanur@broadcom.com> wrote:
>
> On Wed, Apr 29, 2020 at 11:34 PM Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Rayagonda,
> >
> > +Stephen Warren
> >
> > On Wed, 29 Apr 2020 at 10:35, Rayagonda Kokatanur
> > <rayagonda.kokatanur@broadcom.com> wrote:
> > >
> > > Add support to use different register read/write api's
> > > based on register width.
> > >
> > > Signed-off-by: Rayagonda Kokatanur <rayagonda.kokatanur@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@ti.com/

Should this driver use regmap, then?

Regards,
Simon
Rayagonda Kokatanur May 24, 2020, 10:46 a.m. UTC | #4
Hi Simon,

On Fri, May 8, 2020 at 7:07 AM Simon Glass <sjg@chromium.org> wrote:
>
> Hi Rayagonda,
>
> On Thu, 30 Apr 2020 at 04:06, Rayagonda Kokatanur
> <rayagonda.kokatanur@broadcom.com> wrote:
> >
> > On Wed, Apr 29, 2020 at 11:34 PM Simon Glass <sjg@chromium.org> wrote:
> > >
> > > Hi Rayagonda,
> > >
> > > +Stephen Warren
> > >
> > > On Wed, 29 Apr 2020 at 10:35, Rayagonda Kokatanur
> > > <rayagonda.kokatanur@broadcom.com> wrote:
> > > >
> > > > Add support to use different register read/write api's
> > > > based on register width.
> > > >
> > > > Signed-off-by: Rayagonda Kokatanur <rayagonda.kokatanur@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@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
Simon Glass May 25, 2020, 2:15 a.m. UTC | #5
Hi Rayagonda,

On Sun, 24 May 2020 at 04:46, Rayagonda Kokatanur
<rayagonda.kokatanur@broadcom.com> wrote:
>
> Hi Simon,
>
> On Fri, May 8, 2020 at 7:07 AM Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Rayagonda,
> >
> > On Thu, 30 Apr 2020 at 04:06, Rayagonda Kokatanur
> > <rayagonda.kokatanur@broadcom.com> wrote:
> > >
> > > On Wed, Apr 29, 2020 at 11:34 PM Simon Glass <sjg@chromium.org> wrote:
> > > >
> > > > Hi Rayagonda,
> > > >
> > > > +Stephen Warren
> > > >
> > > > On Wed, 29 Apr 2020 at 10:35, Rayagonda Kokatanur
> > > > <rayagonda.kokatanur@broadcom.com> wrote:
> > > > >
> > > > > Add support to use different register read/write api's
> > > > > based on register width.
> > > > >
> > > > > Signed-off-by: Rayagonda Kokatanur <rayagonda.kokatanur@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@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
Rayagonda Kokatanur May 26, 2020, 6:57 p.m. UTC | #6
Hi Simon,

On Mon, May 25, 2020 at 7:45 AM Simon Glass <sjg@chromium.org> wrote:
>
> Hi Rayagonda,
>
> On Sun, 24 May 2020 at 04:46, Rayagonda Kokatanur
> <rayagonda.kokatanur@broadcom.com> wrote:
> >
> > Hi Simon,
> >
> > On Fri, May 8, 2020 at 7:07 AM Simon Glass <sjg@chromium.org> wrote:
> > >
> > > Hi Rayagonda,
> > >
> > > On Thu, 30 Apr 2020 at 04:06, Rayagonda Kokatanur
> > > <rayagonda.kokatanur@broadcom.com> wrote:
> > > >
> > > > On Wed, Apr 29, 2020 at 11:34 PM Simon Glass <sjg@chromium.org> wrote:
> > > > >
> > > > > Hi Rayagonda,
> > > > >
> > > > > +Stephen Warren
> > > > >
> > > > > On Wed, 29 Apr 2020 at 10:35, Rayagonda Kokatanur
> > > > > <rayagonda.kokatanur@broadcom.com> wrote:
> > > > > >
> > > > > > Add support to use different register read/write api's
> > > > > > based on register width.
> > > > > >
> > > > > > Signed-off-by: Rayagonda Kokatanur <rayagonda.kokatanur@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@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 mbox series

Patch

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", &reg, 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", &reg, 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,
 };