[PATCH v1 0/3] extend pinctrl-single driver with APIs

[PATCH v1 0/3] extend pinctrl-single driver with APIs

[Rayagonda Kokatanur]

This patch set adds following feature in pinctrl-single driver,

1. Support to use different register read/write api's 
   based on register width.

2. Parse different gpio properties from dt as part of the probe function. 
   This is required to enable pinctrl pad.

3. add pinctrl_ops->request api to configure pctrl pad register.

Rayagonda Kokatanur (3):
  drivers: pinctrl-single: handle different register width
  drivers: pinctrl-single: add support to parse gpio properties
  drivers: pinctrl-single: add request api

 drivers/pinctrl/pinctrl-single.c | 187 +++++++++++++++++++++++++++----
 1 file changed, 163 insertions(+), 24 deletions(-)

-- 
2.17.1

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

[Rayagonda Kokatanur]

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);
 };
 
 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,
 };
-- 
2.17.1

[PATCH v1 2/3] drivers: pinctrl-single: add support to parse gpio properties

[Rayagonda Kokatanur]

Parse different gpio properties from dt as part of probe
function. This detail will be used to enable pinctrl pad
later when gpio lines are requested.

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

diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c
index aed113b083..fe79a218ee 100644
--- a/drivers/pinctrl/pinctrl-single.c
+++ b/drivers/pinctrl/pinctrl-single.c
@@ -6,10 +6,26 @@
 #include <common.h>
 #include <dm.h>
 #include <dm/device_compat.h>
+#include <dm/devres.h>
+#include <dm/of_access.h>
 #include <dm/pinctrl.h>
 #include <linux/libfdt.h>
 #include <asm/io.h>
 
+/**
+ * struct single_gpiofunc_range - pin ranges with same mux value of gpio fun
+ * @offset:	offset base of pins
+ * @npins:	number pins with the same mux value of gpio function
+ * @gpiofunc:	mux value of gpio function
+ * @node:	list node
+ */
+struct single_gpiofunc_range {
+	u32 offset;
+	u32 npins;
+	u32 gpiofunc;
+	struct list_head node;
+};
+
 /**
  * struct single_pdata - pinctrl device instance
  * @base	first configuration register
@@ -17,6 +33,8 @@
  * @mask	configuration-value mask bits
  * @width	configuration register bit width
  * @bits_per_mux
+ * @mutex	mutex protecting the list
+ * @gpiofuncs	list of gpio functions
  * @read	register read function to use
  * @write	register write function to use
  */
@@ -26,6 +44,8 @@ struct single_pdata {
 	u32 mask;		/* configuration-value mask bits */
 	int width;		/* configuration register bit width */
 	bool bits_per_mux;
+	struct mutex mutex;
+	struct list_head gpiofuncs;
 	u32 (*read)(phys_addr_t reg);
 	void (*write)(u32 val, phys_addr_t reg);
 };
@@ -171,9 +191,42 @@ static int single_set_state(struct udevice *dev,
 	return len;
 }
 
+static int single_add_gpio_func(struct udevice *dev,
+				struct single_pdata *pdata)
+{
+	const char *propname = "pinctrl-single,gpio-range";
+	const char *cellname = "#pinctrl-single,gpio-range-cells";
+	struct single_gpiofunc_range *range;
+	struct ofnode_phandle_args gpiospec;
+	int ret, i;
+
+	for (i = 0; ; i++) {
+		ret = ofnode_parse_phandle_with_args(dev->node, propname,
+						     cellname, 0, i, &gpiospec);
+		/* Do not treat it as error. Only treat it as end condition. */
+		if (ret) {
+			ret = 0;
+			break;
+		}
+		range = devm_kzalloc(dev, sizeof(*range), GFP_KERNEL);
+		if (!range) {
+			ret = -ENOMEM;
+			break;
+		}
+		range->offset = gpiospec.args[0];
+		range->npins = gpiospec.args[1];
+		range->gpiofunc = gpiospec.args[2];
+		mutex_lock(&pdata->mutex);
+		list_add_tail(&range->node, &pdata->gpiofuncs);
+		mutex_unlock(&pdata->mutex);
+	}
+	return ret;
+}
+
 static int single_probe(struct udevice *dev)
 {
 	struct single_pdata *pdata = dev->platdata;
+	int ret;
 
 	switch (pdata->width) {
 	case 8:
@@ -194,7 +247,14 @@ static int single_probe(struct udevice *dev)
 		return -EINVAL;
 	}
 
-	return 0;
+	mutex_init(&pdata->mutex);
+	INIT_LIST_HEAD(&pdata->gpiofuncs);
+
+	ret = single_add_gpio_func(dev, pdata);
+	if (ret < 0)
+		dev_err(dev, "%s: Failed to add gpio functions\n", __func__);
+
+	return ret;
 }
 
 static int single_ofdata_to_platdata(struct udevice *dev)
-- 
2.17.1

[PATCH v1 3/3] drivers: pinctrl-single: add request api

[Rayagonda Kokatanur]

Add pinctrl_ops->request api to configure pctrl
pad register in gpio mode.

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

diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c
index fe79a218ee..2cdba1d338 100644
--- a/drivers/pinctrl/pinctrl-single.c
+++ b/drivers/pinctrl/pinctrl-single.c
@@ -130,6 +130,34 @@ static int single_configure_pins(struct udevice *dev,
 	return 0;
 }
 
+static int single_request(struct udevice *dev, int pin, int flags)
+{
+	struct single_pdata *pdata = dev->platdata;
+	struct single_gpiofunc_range *frange = NULL;
+	struct list_head *pos, *tmp;
+	int mux_bytes = 0;
+	u32 data;
+
+	if (!pdata->mask)
+		return -ENOTSUPP;
+
+	list_for_each_safe(pos, tmp, &pdata->gpiofuncs) {
+		frange = list_entry(pos, struct single_gpiofunc_range, node);
+		if ((pin >= frange->offset + frange->npins) ||
+		    pin < frange->offset)
+			continue;
+
+		mux_bytes = pdata->width / BITS_PER_BYTE;
+		data = pdata->read(pdata->base + pin * mux_bytes);
+		data &= ~pdata->mask;
+		data |= frange->gpiofunc;
+		pdata->write(data, pdata->base + pin * mux_bytes);
+		break;
+	}
+
+	return 0;
+}
+
 static int single_configure_bits(struct udevice *dev,
 				 const struct single_fdt_bits_cfg *pins,
 				 int size)
@@ -288,6 +316,7 @@ static int single_ofdata_to_platdata(struct udevice *dev)
 
 const struct pinctrl_ops single_pinctrl_ops = {
 	.set_state = single_set_state,
+	.request = single_request,
 };
 
 static const struct udevice_id single_pinctrl_match[] = {
-- 
2.17.1

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

[Simon Glass]

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

[PATCH v1 2/3] drivers: pinctrl-single: add support to parse gpio properties

[Simon Glass]

Hi Rayagonda,

+Stephen Warren

On Wed, 29 Apr 2020 at 10:35, Rayagonda Kokatanur
<rayagonda.kokatanur@broadcom.com> wrote:
>
> Parse different gpio properties from dt as part of probe
> function. This detail will be used to enable pinctrl pad
> later when gpio lines are requested.
>
> Signed-off-by: Rayagonda Kokatanur <rayagonda.kokatanur@broadcom.com>
> ---
>  drivers/pinctrl/pinctrl-single.c | 62 +++++++++++++++++++++++++++++++-
>  1 file changed, 61 insertions(+), 1 deletion(-)

Can you please add the binding and a test? Also I think this feature
should be behind a Kconfig flag to avoid code-size increase.

Regards,
Simon

[PATCH v1 3/3] drivers: pinctrl-single: add request api

[Simon Glass]

Hi Rayagonda,

+Stephen Warren

On Wed, 29 Apr 2020 at 10:36, Rayagonda Kokatanur
<rayagonda.kokatanur@broadcom.com> wrote:
>
> Add pinctrl_ops->request api to configure pctrl
> pad register in gpio mode.
>
> Signed-off-by: Rayagonda Kokatanur <rayagonda.kokatanur@broadcom.com>
> ---
>  drivers/pinctrl/pinctrl-single.c | 29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)

This should use the Kconfig also (and needs a test)

Regards,
Simon

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

[Rayagonda Kokatanur]

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

[PATCH v1 3/3] drivers: pinctrl-single: add request api

[Rayagonda Kokatanur]

Hi Simon,

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:36, Rayagonda Kokatanur
> <rayagonda.kokatanur@broadcom.com> wrote:
> >
> > Add pinctrl_ops->request api to configure pctrl
> > pad register in gpio mode.
> >
> > Signed-off-by: Rayagonda Kokatanur <rayagonda.kokatanur@broadcom.com>
> > ---
> >  drivers/pinctrl/pinctrl-single.c | 29 +++++++++++++++++++++++++++++
> >  1 file changed, 29 insertions(+)
>
> This should use the Kconfig also (and needs a test)

Please elaborate "need a test".
Do you mean a testing procedure as part of the commit message ?

I feel we don't need the Kconfig option, looking at Linux pinctrl-single driver.
Please let me know.

>
> Regards,
> Simon

[PATCH v1 2/3] drivers: pinctrl-single: add support to parse gpio properties

[Rayagonda Kokatanur]

Hi Simon,

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:
> >
> > Parse different gpio properties from dt as part of probe
> > function. This detail will be used to enable pinctrl pad
> > later when gpio lines are requested.
> >
> > Signed-off-by: Rayagonda Kokatanur <rayagonda.kokatanur@broadcom.com>
> > ---
> >  drivers/pinctrl/pinctrl-single.c | 62 +++++++++++++++++++++++++++++++-
> >  1 file changed, 61 insertions(+), 1 deletion(-)
>
> Can you please add the binding and a test? Also I think this feature
> should be behind a Kconfig flag to avoid code-size increase.

Sorry I didn't get it, please elaborate "binding and a test".
You mean dt-binding document and test procedure.

This feature is added by referring to linux pinctrl-single driver and
code is in align with linux driver.
This feature is going to be used in most of the gpio controllers where
they have pin controllers to select
different modes of gpio lines. I feel this feature should be part of
the driver by default.

>
> Regards,
> Simon

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

[Simon Glass]

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 at ti.com/

Should this driver use regmap, then?

Regards,
Simon

[PATCH v1 2/3] drivers: pinctrl-single: add support to parse gpio properties

[Simon Glass]

Hi Rayagonda,

On Thu, 30 Apr 2020 at 05:10, Rayagonda Kokatanur
<rayagonda.kokatanur@broadcom.com> wrote:
>
> Hi Simon,
>
> 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:
> > >
> > > Parse different gpio properties from dt as part of probe
> > > function. This detail will be used to enable pinctrl pad
> > > later when gpio lines are requested.
> > >
> > > Signed-off-by: Rayagonda Kokatanur <rayagonda.kokatanur@broadcom.com>
> > > ---
> > >  drivers/pinctrl/pinctrl-single.c | 62 +++++++++++++++++++++++++++++++-
> > >  1 file changed, 61 insertions(+), 1 deletion(-)
> >
> > Can you please add the binding and a test? Also I think this feature
> > should be behind a Kconfig flag to avoid code-size increase.
>
> Sorry I didn't get it, please elaborate "binding and a test".
> You mean dt-binding document and test procedure.

That's right. For the tests, it seems we do not have a
test/dm/pinctrl.c but we need one, something simple to start.

The binding will help explain what is going on.

>
> This feature is added by referring to linux pinctrl-single driver and
> code is in align with linux driver.
> This feature is going to be used in most of the gpio controllers where
> they have pin controllers to select
> different modes of gpio lines. I feel this feature should be part of
> the driver by default.

OK makes sense.

Re the Kconfig flag, U-Boot SPL runs in a tight environment. So when
we add new features we sometimes put them behind a flag so that it
does not bloat SPL.

Regards,
Simon

[PATCH v1 3/3] drivers: pinctrl-single: add request api

[Simon Glass]

Hi Rayagonda,

On Thu, 30 Apr 2020 at 05:03, Rayagonda Kokatanur
<rayagonda.kokatanur@broadcom.com> wrote:
>
> Hi Simon,
>
> 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:36, Rayagonda Kokatanur
> > <rayagonda.kokatanur@broadcom.com> wrote:
> > >
> > > Add pinctrl_ops->request api to configure pctrl
> > > pad register in gpio mode.
> > >
> > > Signed-off-by: Rayagonda Kokatanur <rayagonda.kokatanur@broadcom.com>
> > > ---
> > >  drivers/pinctrl/pinctrl-single.c | 29 +++++++++++++++++++++++++++++
> > >  1 file changed, 29 insertions(+)
> >
> > This should use the Kconfig also (and needs a test)
>
> Please elaborate "need a test".
> Do you mean a testing procedure as part of the commit message ?

No I mean U-Boot has automated tests. So when we add code we need to
add tests for that code. See test/dm/rtc.c for an example. You can run
them with:

   make qcheck

or a single one with:



>
> I feel we don't need the Kconfig option, looking at Linux pinctrl-single driver.
> Please let me know.

OK we can go without one. It is easy to add later if needed.

Regards,
Simon

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

[Rayagonda Kokatanur]

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 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

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

[Simon Glass]

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 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

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

[Rayagonda Kokatanur]

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 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