[PATCH] pinctrl: rockchip: fix offset of mux registers for rk3188

[PATCH] pinctrl: rockchip: fix offset of mux registers for rk3188

[Beniamino Galvani]

The correct value of .mux_offset for rk3188 seems to be 0x60
instead of 0x68.

Signed-off-by: Beniamino Galvani <b.galvani@gmail.com>
---
 drivers/pinctrl/pinctrl-rockchip.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pinctrl/pinctrl-rockchip.c b/drivers/pinctrl/pinctrl-rockchip.c
index 46dddc1..23e8812 100644
--- a/drivers/pinctrl/pinctrl-rockchip.c
+++ b/drivers/pinctrl/pinctrl-rockchip.c
@@ -1534,7 +1534,7 @@ static struct rockchip_pin_ctrl rk3188_pin_ctrl = {
 		.nr_banks		= ARRAY_SIZE(rk3188_pin_banks),
 		.label			= "RK3188-GPIO",
 		.type			= RK3188,
-		.mux_offset		= 0x68,
+		.mux_offset		= 0x60,
 		.pull_calc_reg		= rk3188_calc_pull_reg_and_bit,
 };
 
-- 
1.7.10.4

[PATCH] pinctrl: rockchip: fix offset of mux registers for rk3188

[Heiko Stübner]

Am Montag, 24. M?rz 2014, 23:36:01 schrieb Beniamino Galvani:
> The correct value of .mux_offset for rk3188 seems to be 0x60
> instead of 0x68.

Executive summary: the offset-change itself is correct, therefore

Reviewed-by: Heiko Stuebner <heiko@sntech.de>



That is what one gets when the only source is a vendor tree.
I've looked it up again, and it seems you're right with the offset, but there 
seems to be more to it ;-)

GPIO0 only has the second two IOMUX registers:
- GRF_GPIO0C_IOMUX at 0x68
- GRF_GPIO0D_IOMUX at 0x6c
which I guess is where my mistake comes from.

It looks like there does no iomux register exist at all for the first 16 pins.

In any case, the current number is wrong, and the 0x60 offset is the correct 
one, but I guess we need to determine what the affected pins do - do they 
always have a gpio mux or such?


Thanks for catching the mistake.

Heiko

> Signed-off-by: Beniamino Galvani <b.galvani@gmail.com>
> ---
>  drivers/pinctrl/pinctrl-rockchip.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/pinctrl/pinctrl-rockchip.c
> b/drivers/pinctrl/pinctrl-rockchip.c index 46dddc1..23e8812 100644
> --- a/drivers/pinctrl/pinctrl-rockchip.c
> +++ b/drivers/pinctrl/pinctrl-rockchip.c
> @@ -1534,7 +1534,7 @@ static struct rockchip_pin_ctrl rk3188_pin_ctrl = {
>  		.nr_banks		= ARRAY_SIZE(rk3188_pin_banks),
>  		.label			= "RK3188-GPIO",
>  		.type			= RK3188,
> -		.mux_offset		= 0x68,
> +		.mux_offset		= 0x60,
>  		.pull_calc_reg		= rk3188_calc_pull_reg_and_bit,
>  };

[PATCH] pinctrl: rockchip: fix offset of mux registers for rk3188

[Beniamino Galvani]

On Tue, Mar 25, 2014 at 12:14:42AM +0100, Heiko St?bner wrote:
> Am Montag, 24. M?rz 2014, 23:36:01 schrieb Beniamino Galvani:
> > The correct value of .mux_offset for rk3188 seems to be 0x60
> > instead of 0x68.
> 
> Executive summary: the offset-change itself is correct, therefore
> 
> Reviewed-by: Heiko Stuebner <heiko@sntech.de>
> 
> That is what one gets when the only source is a vendor tree.
> I've looked it up again, and it seems you're right with the offset, but there 
> seems to be more to it ;-)
> 
> GPIO0 only has the second two IOMUX registers:
> - GRF_GPIO0C_IOMUX at 0x68
> - GRF_GPIO0D_IOMUX at 0x6c
> which I guess is where my mistake comes from.
> 
> It looks like there does no iomux register exist at all for the first 16 pins.
> 
> In any case, the current number is wrong, and the 0x60 offset is the correct 
> one, but I guess we need to determine what the affected pins do - do they 
> always have a gpio mux or such?

On radxa rock schematic pins GPIO0A* and GPIO0B* are labeled only as
gpios, without alternate functions like other pins; my guess is that
on rk3188 they can only act as gpios and so mux registers are not
needed for them.

Beniamino

> 
> Thanks for catching the mistake.
> 
> Heiko
> 
> > Signed-off-by: Beniamino Galvani <b.galvani@gmail.com>
> > ---
> >  drivers/pinctrl/pinctrl-rockchip.c |    2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/pinctrl/pinctrl-rockchip.c
> > b/drivers/pinctrl/pinctrl-rockchip.c index 46dddc1..23e8812 100644
> > --- a/drivers/pinctrl/pinctrl-rockchip.c
> > +++ b/drivers/pinctrl/pinctrl-rockchip.c
> > @@ -1534,7 +1534,7 @@ static struct rockchip_pin_ctrl rk3188_pin_ctrl = {
> >  		.nr_banks		= ARRAY_SIZE(rk3188_pin_banks),
> >  		.label			= "RK3188-GPIO",
> >  		.type			= RK3188,
> > -		.mux_offset		= 0x68,
> > +		.mux_offset		= 0x60,
> >  		.pull_calc_reg		= rk3188_calc_pull_reg_and_bit,
> >  };
> 

[PATCH 0/2] pinctrl: rockchip: fix handling of first pinbank

[Heiko Stübner]

Am Dienstag, 25. M?rz 2014, 20:43:59 schrieb Beniamino Galvani:
> On Tue, Mar 25, 2014 at 12:14:42AM +0100, Heiko St?bner wrote:
> > GPIO0 only has the second two IOMUX registers:
> > - GRF_GPIO0C_IOMUX at 0x68
> > - GRF_GPIO0D_IOMUX at 0x6c
> > which I guess is where my mistake comes from.

[...] 

> On radxa rock schematic pins GPIO0A* and GPIO0B* are labeled only as
> gpios, without alternate functions like other pins; my guess is that
> on rk3188 they can only act as gpios and so mux registers are not
> needed for them.

That was my guess too - especially as the registers are also missing.

Therefore I put together the following two patches to go on top of
your patch and also make rockchip_set_mux honor this situation.


Heiko Stuebner (2):
  pinctrl: rockchip: add return value to rockchip_set_mux
  pinctrl: rockchip: handle first half of rk3188-bank0 correctly

 drivers/pinctrl/pinctrl-rockchip.c | 46 ++++++++++++++++++++++++++++++++------
 1 file changed, 39 insertions(+), 7 deletions(-)

-- 
1.9.0

[PATCH 1/2] pinctrl: rockchip: add return value to rockchip_set_mux

[Heiko Stübner]

In a following change, rockchip_set_mux gets the possibility to fail.
Therefore add a return value to it and honor error codes in functions
using rockchip_set_mux.

Signed-off-by: Heiko Stuebner <heiko@sntech.de>
---
 drivers/pinctrl/pinctrl-rockchip.c | 32 +++++++++++++++++++++++++-------
 1 file changed, 25 insertions(+), 7 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-rockchip.c b/drivers/pinctrl/pinctrl-rockchip.c
index 23e8812..80e28e5 100644
--- a/drivers/pinctrl/pinctrl-rockchip.c
+++ b/drivers/pinctrl/pinctrl-rockchip.c
@@ -342,7 +342,7 @@ static const struct pinctrl_ops rockchip_pctrl_ops = {
  * @pin: pin to change
  * @mux: new mux function to set
  */
-static void rockchip_set_mux(struct rockchip_pin_bank *bank, int pin, int mux)
+static int rockchip_set_mux(struct rockchip_pin_bank *bank, int pin, int mux)
 {
 	struct rockchip_pinctrl *info = bank->drvdata;
 	void __iomem *reg = info->reg_base + info->ctrl->mux_offset;
@@ -365,6 +365,8 @@ static void rockchip_set_mux(struct rockchip_pin_bank *bank, int pin, int mux)
 	writel(data, reg);
 
 	spin_unlock_irqrestore(&bank->slock, flags);
+
+	return 0;
 }
 
 #define RK2928_PULL_OFFSET		0x118
@@ -560,7 +562,7 @@ static int rockchip_pmx_enable(struct pinctrl_dev *pctldev, unsigned selector,
 	const unsigned int *pins = info->groups[group].pins;
 	const struct rockchip_pin_config *data = info->groups[group].data;
 	struct rockchip_pin_bank *bank;
-	int cnt;
+	int cnt, ret = 0;
 
 	dev_dbg(info->dev, "enable function %s group %s\n",
 		info->functions[selector].name, info->groups[group].name);
@@ -571,8 +573,18 @@ static int rockchip_pmx_enable(struct pinctrl_dev *pctldev, unsigned selector,
 	 */
 	for (cnt = 0; cnt < info->groups[group].npins; cnt++) {
 		bank = pin_to_bank(info, pins[cnt]);
-		rockchip_set_mux(bank, pins[cnt] - bank->pin_base,
-				 data[cnt].func);
+		ret = rockchip_set_mux(bank, pins[cnt] - bank->pin_base,
+				       data[cnt].func);
+		if (ret)
+			break;
+	}
+
+	if (ret) {
+		/* revert the already done pin settings */
+		for (cnt--; cnt >= 0; cnt--)
+			rockchip_set_mux(bank, pins[cnt] - bank->pin_base, 0);
+
+		return ret;
 	}
 
 	return 0;
@@ -607,7 +619,7 @@ static int rockchip_pmx_gpio_set_direction(struct pinctrl_dev *pctldev,
 	struct rockchip_pinctrl *info = pinctrl_dev_get_drvdata(pctldev);
 	struct rockchip_pin_bank *bank;
 	struct gpio_chip *chip;
-	int pin;
+	int pin, ret;
 	u32 data;
 
 	chip = range->gc;
@@ -617,7 +629,9 @@ static int rockchip_pmx_gpio_set_direction(struct pinctrl_dev *pctldev,
 	dev_dbg(info->dev, "gpio_direction for pin %u as %s-%d to %s\n",
 		 offset, range->name, pin, input ? "input" : "output");
 
-	rockchip_set_mux(bank, pin, RK_FUNC_GPIO);
+	ret = rockchip_set_mux(bank, pin, RK_FUNC_GPIO);
+	if (ret < 0)
+		return ret;
 
 	data = readl_relaxed(bank->reg_base + GPIO_SWPORT_DDR);
 	/* set bit to 1 for output, 0 for input */
@@ -1144,9 +1158,13 @@ static int rockchip_irq_set_type(struct irq_data *d, unsigned int type)
 	u32 polarity;
 	u32 level;
 	u32 data;
+	int ret;
 
 	/* make sure the pin is configured as gpio input */
-	rockchip_set_mux(bank, d->hwirq, RK_FUNC_GPIO);
+	ret = rockchip_set_mux(bank, d->hwirq, RK_FUNC_GPIO);
+	if (ret < 0)
+		return ret;
+
 	data = readl_relaxed(bank->reg_base + GPIO_SWPORT_DDR);
 	data &= ~mask;
 	writel_relaxed(data, bank->reg_base + GPIO_SWPORT_DDR);
-- 
1.9.0

[PATCH 2/2] pinctrl: rockchip: handle first half of rk3188-bank0 correctly

[Heiko Stübner]

The first half of pinbank 0 only has one muxing function (as gpios) and
does not have a special mux-register.

Therefore ensure that no other mux function can be selected and also do not
write to a non-existent register.

Signed-off-by: Heiko Stuebner <heiko@sntech.de>
---
 drivers/pinctrl/pinctrl-rockchip.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/pinctrl/pinctrl-rockchip.c b/drivers/pinctrl/pinctrl-rockchip.c
index 80e28e5..ffcd4f4 100644
--- a/drivers/pinctrl/pinctrl-rockchip.c
+++ b/drivers/pinctrl/pinctrl-rockchip.c
@@ -350,6 +350,20 @@ static int rockchip_set_mux(struct rockchip_pin_bank *bank, int pin, int mux)
 	u8 bit;
 	u32 data;
 
+	/*
+	 * The first 16 pins of rk3188_bank0 are always gpios and do not have
+	 * a mux register at all.
+	 */
+	if (bank->bank_type == RK3188_BANK0 && pin < 16) {
+		if (mux != RK_FUNC_GPIO) {
+			dev_err(info->dev,
+				"pin %d only supports a gpio mux\n", pin);
+			return -ENOTSUPP;
+		} else {
+			return 0;
+		}
+	}
+
 	dev_dbg(info->dev, "setting mux of GPIO%d-%d to %d\n",
 						bank->bank_num, pin, mux);
 
-- 
1.9.0

[PATCH 0/2] pinctrl: rockchip: fix handling of first pinbank

[Linus Walleij]

On Wed, Mar 26, 2014 at 12:56 AM, Heiko St?bner <heiko@sntech.de> wrote:

> Therefore I put together the following two patches to go on top of
> your patch and also make rockchip_set_mux honor this situation.

Both patches applied for fixes.

Yours,
Linus Walleij

[PATCH] pinctrl: rockchip: fix offset of mux registers for rk3188

[Linus Walleij]

On Mon, Mar 24, 2014 at 11:36 PM, Beniamino Galvani <b.galvani@gmail.com> wrote:

> The correct value of .mux_offset for rk3188 seems to be 0x60
> instead of 0x68.
>
> Signed-off-by: Beniamino Galvani <b.galvani@gmail.com>

Patch applied to fixes with Heiko's Review tag and also
copied some info from the conversation into the commit message.

Yours,
Linus Walleij

[PATCH] pinctrl: rockchip: fix offset of mux registers for rk3188

[Heiko Stübner]

Am Donnerstag, 3. April 2014, 16:10:46 schrieb Linus Walleij:
> On Mon, Mar 24, 2014 at 11:36 PM, Beniamino Galvani <b.galvani@gmail.com> 
wrote:
> > The correct value of .mux_offset for rk3188 seems to be 0x60
> > instead of 0x68.
> > 
> > Signed-off-by: Beniamino Galvani <b.galvani@gmail.com>
> 
> Patch applied to fixes with Heiko's Review tag and also
> copied some info from the conversation into the commit message.

small protest note

"Heikki adds:" - sounds somehow finish or swedish, but who is this guy? ;-)


But it doesn't really matter, so no need to roll it back

Heiko

[PATCH] pinctrl: rockchip: fix offset of mux registers for rk3188

[Linus Walleij]

On Thu, Apr 3, 2014 at 4:20 PM, Heiko St?bner <heiko@sntech.de> wrote:

> small protest note
>
> "Heikki adds:" - sounds somehow finish or swedish, but who is this guy? ;-)

Haha sorry Heiko, I'll fix.

Yours,
Linus Walleij