diff mbox series

[07/11] pinctrl: single: use function pointer for register access

Message ID 20210123182711.7177-8-dariobin@libero.it
State Superseded
Delegated to: Lokesh Vutla
Headers show
Series Add support for pinmux status command on beaglebone | expand

Commit Message

Dario Binacchi Jan. 23, 2021, 6:27 p.m. UTC 
The patch allows you to call the read/write functions set during probing
without having to check the type of access at runtime. It also adds
functions for 8-bit registers access.

Signed-off-by: Dario Binacchi <dariobin@libero.it>
---

 drivers/pinctrl/pinctrl-single.c | 98 ++++++++++++++++++++++++--------
 1 file changed, 73 insertions(+), 25 deletions(-)

Comments

Simon Glass Jan. 24, 2021, 2:03 a.m. UTC | #1 
Hi Dario,

On Sat, 23 Jan 2021 at 11:27, Dario Binacchi <dariobin@libero.it> wrote:
>
> The patch allows you to call the read/write functions set during probing
> without having to check the type of access at runtime. It also adds
> functions for 8-bit registers access.
>
> Signed-off-by: Dario Binacchi <dariobin@libero.it>
> ---
>
>  drivers/pinctrl/pinctrl-single.c | 98 ++++++++++++++++++++++++--------
>  1 file changed, 73 insertions(+), 25 deletions(-)

I'm not really keen on this. A switch() statement is not expensive and
function pointers add indirection/confusion and use more space.

Regards,
Simon
Dario Binacchi Jan. 24, 2021, 4:50 p.m. UTC | #2 
Hi Simon,

> Il 24/01/2021 03:03 Simon Glass <sjg@chromium.org> ha scritto:
> 
>  
> Hi Dario,
> 
> On Sat, 23 Jan 2021 at 11:27, Dario Binacchi <dariobin@libero.it> wrote:
> >
> > The patch allows you to call the read/write functions set during probing
> > without having to check the type of access at runtime. It also adds
> > functions for 8-bit registers access.
> >
> > Signed-off-by: Dario Binacchi <dariobin@libero.it>
> > ---
> >
> >  drivers/pinctrl/pinctrl-single.c | 98 ++++++++++++++++++++++++--------
> >  1 file changed, 73 insertions(+), 25 deletions(-)
> 
> I'm not really keen on this. A switch() statement is not expensive and
> function pointers add indirection/confusion and use more space.

I think however it is better to create two static functions for reading/writing
operations (with a switch() statement inside). So as not to replicate the code. 
Do you agree?

Regards,
Dario

> 
> Regards,
> Simon
Simon Glass Jan. 24, 2021, 6 p.m. UTC | #3 
Hi Dario,

On Sun, 24 Jan 2021 at 09:50, Dario Binacchi <dariobin@libero.it> wrote:
>
>
> Hi Simon,
>
> > Il 24/01/2021 03:03 Simon Glass <sjg@chromium.org> ha scritto:
> >
> >
> > Hi Dario,
> >
> > On Sat, 23 Jan 2021 at 11:27, Dario Binacchi <dariobin@libero.it> wrote:
> > >
> > > The patch allows you to call the read/write functions set during probing
> > > without having to check the type of access at runtime. It also adds
> > > functions for 8-bit registers access.
> > >
> > > Signed-off-by: Dario Binacchi <dariobin@libero.it>
> > > ---
> > >
> > >  drivers/pinctrl/pinctrl-single.c | 98 ++++++++++++++++++++++++--------
> > >  1 file changed, 73 insertions(+), 25 deletions(-)
> >
> > I'm not really keen on this. A switch() statement is not expensive and
> > function pointers add indirection/confusion and use more space.
>
> I think however it is better to create two static functions for reading/writing
> operations (with a switch() statement inside). So as not to replicate the code.
> Do you agree?

Yes that sounds good to me.

Regards,
Simon
diff mbox series

Patch

diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c
index 09bb883041..eb69e53096 100644
--- a/drivers/pinctrl/pinctrl-single.c
+++ b/drivers/pinctrl/pinctrl-single.c
@@ -26,6 +26,16 @@  struct single_pdata {
 	bool bits_per_mux;
 };
 
+/**
+ * struct single_priv - private data
+ * @read: function to read from a configuration register
+ * @write: function to write to a configuration register
+ */
+struct single_priv {
+	unsigned int (*read)(fdt_addr_t reg);
+	void (*write)(unsigned int val, fdt_addr_t reg);
+};
+
 /**
  * struct single_fdt_pin_cfg - pin configuration
  *
@@ -56,6 +66,36 @@  struct single_fdt_bits_cfg {
 	fdt32_t mask;
 };
 
+static unsigned int single_readb(fdt_addr_t reg)
+{
+	return readb(reg);
+}
+
+static unsigned int single_readw(fdt_addr_t reg)
+{
+	return readw(reg);
+}
+
+static unsigned int single_readl(fdt_addr_t reg)
+{
+	return readl(reg);
+}
+
+static void single_writeb(unsigned int val, fdt_addr_t reg)
+{
+	writeb(val, reg);
+}
+
+static void single_writew(unsigned int val, fdt_addr_t reg)
+{
+	writew(val, reg);
+}
+
+static void single_writel(unsigned int val, fdt_addr_t reg)
+{
+	writel(val, reg);
+}
+
 /**
  * single_configure_pins() - Configure pins based on FDT data
  *
@@ -75,6 +115,7 @@  static int single_configure_pins(struct udevice *dev,
 				 int size)
 {
 	struct single_pdata *pdata = dev_get_plat(dev);
+	struct single_priv *priv = dev_get_priv(dev);
 	int n, count = size / sizeof(struct single_fdt_pin_cfg);
 	phys_addr_t reg;
 	u32 offset, val;
@@ -93,19 +134,9 @@  static int single_configure_pins(struct udevice *dev,
 
 		reg = pdata->base + offset;
 		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;
-		}
+		priv->write((priv->read(reg) & ~pdata->mask) | val, reg);
 		dev_dbg(dev, "  reg/val %pa/0x%08x\n", &reg, val);
+
 	}
 	return 0;
 }
@@ -115,6 +146,7 @@  static int single_configure_bits(struct udevice *dev,
 				 int size)
 {
 	struct single_pdata *pdata = dev_get_plat(dev);
+	struct single_priv *priv = dev_get_priv(dev);
 	int n, count = size / sizeof(struct single_fdt_bits_cfg);
 	phys_addr_t reg;
 	u32 offset, val, mask;
@@ -131,19 +163,7 @@  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;
-		}
+		priv->write((priv->read(reg) & ~mask) | val, reg);
 		dev_dbg(dev, "  reg/val %pa/0x%08x\n", &reg, val);
 	}
 	return 0;
@@ -183,6 +203,32 @@  static int single_set_state(struct udevice *dev,
 	return len;
 }
 
+static int single_probe(struct udevice *dev)
+{
+	struct single_pdata *pdata = dev_get_plat(dev);
+	struct single_priv *priv = dev_get_priv(dev);
+
+	switch (pdata->width) {
+	case 8:
+		priv->read = single_readb;
+		priv->write = single_writeb;
+		break;
+	case 16:
+		priv->read = single_readw;
+		priv->write = single_writew;
+		break;
+	case 32:
+		priv->read = single_readl;
+		priv->write = single_writel;
+		break;
+	default:
+		dev_err(dev, "wrong register width\n");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 static int single_of_to_plat(struct udevice *dev)
 {
 	fdt_addr_t addr;
@@ -232,5 +278,7 @@  U_BOOT_DRIVER(single_pinctrl) = {
 	.of_match = single_pinctrl_match,
 	.ops = &single_pinctrl_ops,
 	.plat_auto	= sizeof(struct single_pdata),
+	.priv_auto = sizeof(struct single_priv),
 	.of_to_plat = single_of_to_plat,
+	.probe = single_probe,
 };