pinctrl: pinctrl-imx: add support for set bits for general purpose registers

PATCH Superseded Dong Aisheng 6 participants 14 messages Jul 12, 2012 – Jul 17, 2012

Subsystems (5)

Device tree bindings Arm i.MX and Layerscape Pin control Freescale SoC drivers +1 more subsystems

lore

Share & tools

lore

Dong Aisheng

author

2012-07-12 09:07 UTC #0 lore raw

PATCH v2 1/2 pinctrl: pinctrl-imx: add support for set bits for general purpose registers

From: Dong Aisheng <[email protected]>

The General Purpose Registers (GPR) is used to select operating modes for general features in the SoC, usually not related to the IOMUX itself, but it does belong to IOMUX controller.
We simply provide an convient API for driver to call to write/read the general purpose register bits if needed.

Signed-off-by Dong Aisheng <[email protected]>

---
ChangeLog v1->v2:
* add gpr read api
* change api name a bit to *_write and *_read
* add -EPROBE_DEFER support
* define macros for gpr registers for imx6q
* change driver loadding priority to postcore_init at satisfy clients driver to use imx_pinctrl_gpr_{read | write} APIs at best

---
 drivers/pinctrl/pinctrl-imx.c   |   29 ++++
 drivers/pinctrl/pinctrl-imx51.c |    2 +-
 drivers/pinctrl/pinctrl-imx53.c |    2 +-
 drivers/pinctrl/pinctrl-imx6q.c |    2 +-
 include/linux/fsl/imx-pinctrl.h |  340 +++++++++++++++++++++++++++++++++++++++
 5 files changed, 372 insertions(+), 3 deletions(-)
 create mode 100644 include/linux/fsl/imx-pinctrl.h

drivers/pinctrl/pinctrl-imx.c +29 -0

index 44e9726..1725e07 100644
@@ -54,6 +54,34 @@ struct imx_pinctrl {
 	const struct imx_pinctrl_soc_info *info;
 };
 
+static struct imx_pinctrl *imx_pinctrl;
+/*
+ * Set bits for general purpose registers
+ */
+int imx_pinctrl_gpr_write(u8 gpr, u32 mask, u32 value)
+{
+	u32 reg;
+
+	if (!imx_pinctrl)
+		return -EPROBE_DEFER;
+
+	reg = readl(imx_pinctrl->base + gpr);
+	reg &= ~mask;
+	writel(reg | value, imx_pinctrl->base + gpr);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(imx_pinctrl_gpr_write);
+
+int imx_pinctrl_gpr_read(u8 gpr)
+{
+	if (!imx_pinctrl)
+		return -EPROBE_DEFER;
+
+	return readl(imx_pinctrl->base + gpr);
+}
+EXPORT_SYMBOL_GPL(imx_pinctrl_gpr_read);
+
 static const struct imx_pin_reg *imx_find_pin_reg(
 				const struct imx_pinctrl_soc_info *info,
 				unsigned pin, bool is_mux, unsigned mux)

@@ -587,6 +615,7 @@ int __devinit imx_pinctrl_probe(struct platform_device *pdev,
 	if (!ipctl->base)
 		return -EBUSY;
 
+	imx_pinctrl = ipctl;
 	imx_pinctrl_desc.name = dev_name(&pdev->dev);
 	imx_pinctrl_desc.pins = info->pins;
 	imx_pinctrl_desc.npins = info->npins;

drivers/pinctrl/pinctrl-imx51.c +1 -1

index 689b3c8..aa37634 100644
@@ -1310,7 +1310,7 @@ static int __init imx51_pinctrl_init(void)
 {
 	return platform_driver_register(&imx51_pinctrl_driver);
 }
-arch_initcall(imx51_pinctrl_init);
+postcore_initcall(imx51_pinctrl_init);
 
 static void __exit imx51_pinctrl_exit(void)
 {

drivers/pinctrl/pinctrl-imx53.c +1 -1

index 1f49e16..cff42a1 100644
@@ -1637,7 +1637,7 @@ static int __init imx53_pinctrl_init(void)
 {
 	return platform_driver_register(&imx53_pinctrl_driver);
 }
-arch_initcall(imx53_pinctrl_init);
+postcore_initcall(imx53_pinctrl_init);
 
 static void __exit imx53_pinctrl_exit(void)
 {

drivers/pinctrl/pinctrl-imx6q.c +1 -1

index 7737d4d..6974238 100644
@@ -2319,7 +2319,7 @@ static int __init imx6q_pinctrl_init(void)
 {
 	return platform_driver_register(&imx6q_pinctrl_driver);
 }
-arch_initcall(imx6q_pinctrl_init);
+postcore_initcall(imx6q_pinctrl_init);
 
 static void __exit imx6q_pinctrl_exit(void)
 {

include/linux/fsl/imx-pinctrl.h new file +340 -0

340 lines not on this page · raw message

Signature · 1 line

1.7.0.4

Dong Aisheng

author

2012-07-12 09:08 UTC #1 lore raw

PATCH v2 2/2 pinctrl: pinctrl-imx6q: add missed mux function for USBOTG_ID

From: Dong Aisheng <[email protected]>

The original pin registers table is derived from u-boot mainline, but somehow it was found missing some mux functions for USBOTG_ID. We added it at the bottom by following the exist pin function ids, then it will not break the exist using of pin function id in dts file.

Reported-by Richard Zhao <[email protected]>

Signed-off-by Dong Aisheng <[email protected]>

---
ChangeLog v1->v2: No changes

---
 .../bindings/pinctrl/fsl,imx6q-pinctrl.txt         |    2 ++
 drivers/pinctrl/pinctrl-imx6q.c                    |    2 ++
 2 files changed, 4 insertions(+), 0 deletions(-)

Documentation/devicetree/bindings/pinctrl/fsl,imx6q-pinctrl.txt +2 -0

index 82b43f9..a4119f6 100644
@@ -1626,3 +1626,5 @@ MX6Q_PAD_SD2_DAT3__PCIE_CTRL_MUX_11		1587
 MX6Q_PAD_SD2_DAT3__GPIO_1_12			1588
 MX6Q_PAD_SD2_DAT3__SJC_DONE			1589
 MX6Q_PAD_SD2_DAT3__ANATOP_TESTO_3		1590
+MX6Q_PAD_ENET_RX_ER__ANATOP_USBOTG_ID		1591
+MX6Q_PAD_GPIO_1__ANATOP_USBOTG_ID		1592

drivers/pinctrl/pinctrl-imx6q.c +2 -0

index 6974238..4d9bc07 100644
@@ -1950,6 +1950,8 @@ static struct imx_pin_reg imx6q_pin_regs[] = {
 	IMX_PIN_REG(MX6Q_PAD_SD2_DAT3, 0x0744, 0x035C, 5, 0x0000, 0), /* MX6Q_PAD_SD2_DAT3__GPIO_1_12 */
 	IMX_PIN_REG(MX6Q_PAD_SD2_DAT3, 0x0744, 0x035C, 6, 0x0000, 0), /* MX6Q_PAD_SD2_DAT3__SJC_DONE */
 	IMX_PIN_REG(MX6Q_PAD_SD2_DAT3, 0x0744, 0x035C, 7, 0x0000, 0), /* MX6Q_PAD_SD2_DAT3__ANATOP_TESTO_3 */
+	IMX_PIN_REG(MX6Q_PAD_ENET_RX_ER, 0x04EC, 0x01D8, 0, 0x0000, 0), /* MX6Q_PAD_ENET_RX_ER__ANATOP_USBOTG_ID */
+	IMX_PIN_REG(MX6Q_PAD_GPIO_1, 0x05F4, 0x0224, 3, 0x0000, 0), /* MX6Q_PAD_GPIO_1__ANATOP_USBOTG_ID */
 };
 
 /* Pad names for the pinmux subsystem */

Signature · 1 line

1.7.0.4

Richard Zhao

2012-07-12 09:31 UTC #2 lore raw

PATCH v2 1/2 pinctrl: pinctrl-imx: add support for set bits for general purpose registers

In reply to Dong Aisheng

Quoted 34 lines · Dong Aisheng

From: Dong Aisheng <[email protected]>
The General Purpose Registers (GPR) is used to select operating modes for general features in the SoC, usually not related to the IOMUX itself, but it does belong to IOMUX controller.
We simply provide an convient API for driver to call to write/read the general purpose register bits if needed.
Signed-off-by Dong Aisheng <[email protected]>
---
ChangeLog v1->v2:
* add gpr read api
* change api name a bit to *_write and *_read
* add -EPROBE_DEFER support
* define macros for gpr registers for imx6q
* change driver loadding priority to postcore_init at satisfy clients driver to use imx_pinctrl_gpr_{read | write} APIs at best
---
 drivers/pinctrl/pinctrl-imx.c   |   29 ++++
 drivers/pinctrl/pinctrl-imx51.c |    2 +-
 drivers/pinctrl/pinctrl-imx53.c |    2 +-
 drivers/pinctrl/pinctrl-imx6q.c |    2 +-
 include/linux/fsl/imx-pinctrl.h |  340 +++++++++++++++++++++++++++++++++++++++
 5 files changed, 372 insertions(+), 3 deletions(-)
 create mode 100644 include/linux/fsl/imx-pinctrl.h
drivers/pinctrl/pinctrl-imx.c +0 -0
index 44e9726..1725e07 100644
@@ -54,6 +54,34 @@ struct imx_pinctrl {
 	const struct imx_pinctrl_soc_info *info;
 };
 

drivers/pinctrl/pinctrl-imx.c struct imx_pinctrl { · line 66

Quoted 10 lines

drivers/pinctrl/pinctrl-imx.c +10 -0

+static struct imx_pinctrl *imx_pinctrl;
+/*
+ * Set bits for general purpose registers
+ */
+int imx_pinctrl_gpr_write(u8 gpr, u32 mask, u32 value)
+{
+	u32 reg;
+
+	if (!imx_pinctrl)
+		return -EPROBE_DEFER;

	value &= mask;

And add a spinlock to protect it?

Thanks
Richard

Hui Wang

2012-07-12 09:48 UTC #3 lore raw

PATCH v2 1/2 pinctrl: pinctrl-imx: add support for set bits for general purpose registers

In reply to Dong Aisheng

Quoted 50 lines · Dong Aisheng

From: Dong Aisheng <[email protected]>
The General Purpose Registers (GPR) is used to select operating modes for general features in the SoC, usually not related to the IOMUX itself, but it does belong to IOMUX controller.
We simply provide an convient API for driver to call to write/read the general purpose register bits if needed.
Signed-off-by Dong Aisheng <[email protected]>
---
ChangeLog v1->v2:
* add gpr read api
* change api name a bit to *_write and *_read
* add -EPROBE_DEFER support
* define macros for gpr registers for imx6q
* change driver loadding priority to postcore_init at satisfy clients driver to use imx_pinctrl_gpr_{read | write} APIs at best
---
 drivers/pinctrl/pinctrl-imx.c   |   29 ++++
 drivers/pinctrl/pinctrl-imx51.c |    2 +-
 drivers/pinctrl/pinctrl-imx53.c |    2 +-
 drivers/pinctrl/pinctrl-imx6q.c |    2 +-
 include/linux/fsl/imx-pinctrl.h |  340 +++++++++++++++++++++++++++++++++++++++
 5 files changed, 372 insertions(+), 3 deletions(-)
 create mode 100644 include/linux/fsl/imx-pinctrl.h
drivers/pinctrl/pinctrl-imx.c +16 -0
index 44e9726..1725e07 100644
@@ -54,6 +54,34 @@ struct imx_pinctrl {
 	const struct imx_pinctrl_soc_info *info;
 };
 
+static struct imx_pinctrl *imx_pinctrl;
+/*
+ * Set bits for general purpose registers
+ */
+int imx_pinctrl_gpr_write(u8 gpr, u32 mask, u32 value)
+{
+	u32 reg;
+
+	if (!imx_pinctrl)
+		return -EPROBE_DEFER;
+
+	reg = readl(imx_pinctrl->base + gpr);
+	reg &= ~mask;
+	writel(reg | value, imx_pinctrl->base + gpr);
+
+	return 0;

drivers/pinctrl/pinctrl-imx.c struct imx_pinctrl { · line 82

Quoted 10 lines

drivers/pinctrl/pinctrl-imx.c +9 -0

+}
+EXPORT_SYMBOL_GPL(imx_pinctrl_gpr_write);
+
+int imx_pinctrl_gpr_read(u8 gpr)
+{
+	if (!imx_pinctrl)
+		return -EPROBE_DEFER;
+
+	return readl(imx_pinctrl->base + gpr);

Suppose the MSB of the register content is 1, how does the caller distinguish if the function return an error or return a register content?

Richard Zhao

2012-07-12 10:24 UTC #4 lore raw

PATCH v2 1/2 pinctrl: pinctrl-imx: add support for set bits for general purpose registers

Quoted 54 lines · Hui Wang

Quoted 53 lines · Dong Aisheng
From: Dong Aisheng <[email protected]>
The General Purpose Registers (GPR) is used to select operating modes for general features in the SoC, usually not related to the IOMUX itself, but it does belong to IOMUX controller.
We simply provide an convient API for driver to call to write/read the general purpose register bits if needed.
Signed-off-by Dong Aisheng <[email protected]>
---
ChangeLog v1->v2:
* add gpr read api
* change api name a bit to *_write and *_read
* add -EPROBE_DEFER support
* define macros for gpr registers for imx6q
* change driver loadding priority to postcore_init at satisfy clients driver to use imx_pinctrl_gpr_{read | write} APIs at best
---
drivers/pinctrl/pinctrl-imx.c   |   29 ++++
drivers/pinctrl/pinctrl-imx51.c |    2 +-
drivers/pinctrl/pinctrl-imx53.c |    2 +-
drivers/pinctrl/pinctrl-imx6q.c |    2 +-
include/linux/fsl/imx-pinctrl.h |  340 +++++++++++++++++++++++++++++++++++++++
5 files changed, 372 insertions(+), 3 deletions(-)
create mode 100644 include/linux/fsl/imx-pinctrl.h
drivers/pinctrl/pinctrl-imx.c +20 -0
index 44e9726..1725e07 100644
@@ -54,6 +54,34 @@ struct imx_pinctrl {
 	const struct imx_pinctrl_soc_info *info;
 };
+static struct imx_pinctrl *imx_pinctrl;
+/*
+ * Set bits for general purpose registers
+ */
+int imx_pinctrl_gpr_write(u8 gpr, u32 mask, u32 value)
+{
+	u32 reg;
+
+	if (!imx_pinctrl)
+		return -EPROBE_DEFER;
+
+	reg = readl(imx_pinctrl->base + gpr);
+	reg &= ~mask;
+	writel(reg | value, imx_pinctrl->base + gpr);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(imx_pinctrl_gpr_write);
+
+int imx_pinctrl_gpr_read(u8 gpr)

Quoted 8 lines

drivers/pinctrl/pinctrl-imx.c struct imx_pinctrl { · line 81
Quoted 5 lines
drivers/pinctrl/pinctrl-imx.c +5 -0
+{
+	if (!imx_pinctrl)
+		return -EPROBE_DEFER;
+
+	return readl(imx_pinctrl->base + gpr);
Suppose the MSB of the register content is 1, how does the caller distinguish if the function return an error or return a register content?

I think it worth a BUG() here.

Thanks
Richard

Dong Aisheng

author

2012-07-12 11:04 UTC #5 lore raw

PATCH v2 1/2 pinctrl: pinctrl-imx: add support for set bits for general purpose registers

In reply to Hui Wang

On Thu, Jul 12, 2012 at 05:48:46PM +0800, Hui Wang wrote:
...

Quoted 11 lines

drivers/pinctrl/pinctrl-imx.c struct imx_pinctrl { · line 81
Quoted 8 lines
drivers/pinctrl/pinctrl-imx.c +7 -0
+
+int imx_pinctrl_gpr_read(u8 gpr)
+{
+	if (!imx_pinctrl)
+		return -EPROBE_DEFER;
+
+	return readl(imx_pinctrl->base + gpr);
Suppose the MSB of the register content is 1, how does the caller distinguish if the function return an error or return a register content?

Good catch, it's indeed an issue.
We need find a way to fix it, probably i would add a parameter for read API to return a u32 value.

Thanks

Regards
Dong Aisheng

Linus Walleij

2012-07-14 20:43 UTC #6 lore raw

PATCH v2 1/2 pinctrl: pinctrl-imx: add support for set bits for general purpose registers

In reply to Dong Aisheng

On Thu, Jul 12, 2012 at 11:07 AM, Dong Aisheng <[email protected]> wrote:

Hm, hm. This makes be ever more hesitant to have this in pinctrl.

Remeber again that nothing stops you from remapping the same register range in another driver.

Quoted 1 line

+#define IMX6Q_GPR0_CLOCK_8_MUX_SEL_MASK (0x3 << 30)

This belongs in drivers/clk/*

Why funnel these register writes through pinctrl? Just remap that address offset in the clk driver.

Quoted 1 line

+#define IMX6Q_GPR0_DMAREQ_MUX_SEL7_MASK BIT(7)

This belongs in drivers/dma/*

Same comments.

Quoted 1 line

+#define IMX6Q_GPR1_PCIE_REQ_MASK (0x3 << 30)

Looks like it belongs in some PCI driver or glue layer.

include/linux/fsl/imx-pinctrl.h line 123

Quoted 2 lines

include/linux/fsl/imx-pinctrl.h new file +2 -0

+#define IMX6Q_GPR1_MIPI_COLOR_SW               BIT(25)
+#define IMX6Q_GPR1_DPI_OFF                     BIT(24)

Looks related to some test or something...

And so on.

If you really wants a "funnel driver" doing all these diverse things, I'd put it in drivers/mfd.

This whole issue appears in other systems so you're not alone on this, for example the ux500 PRCMU driver has exactly these properties.

I'm also contemplating drivers/syscon again, but
I have a hard time seeing it would be much more than another drivers/mfd-similar construct.

I would really like input from Arnd and Samuel and other clever people on the placement of drivers like this one :-/

But close address range proximity to the pin controller is not a reason to have it in pinctrl.

Yours,
Linus Walleij

Richard Zhao

2012-07-16 08:17 UTC #7 lore raw

PATCH v2 1/2 pinctrl: pinctrl-imx: add support for set bits for general purpose registers

Quoted 29 lines · Linus Walleij

On Thu, Jul 12, 2012 at 11:07 AM, Dong Aisheng <[email protected]> wrote:
Hm, hm. This makes be ever more hesitant to have this in pinctrl.
Remeber again that nothing stops you from remapping the same register range in another driver.
Quoted 1 line
+#define IMX6Q_GPR0_CLOCK_8_MUX_SEL_MASK (0x3 << 30)
This belongs in drivers/clk/*
Why funnel these register writes through pinctrl? Just remap that address offset in the clk driver.
Quoted 1 line
+#define IMX6Q_GPR0_DMAREQ_MUX_SEL7_MASK BIT(7)
This belongs in drivers/dma/*
Same comments.
Quoted 1 line
+#define IMX6Q_GPR1_PCIE_REQ_MASK (0x3 << 30)
Looks like it belongs in some PCI driver or glue layer.
include/linux/fsl/imx-pinctrl.h line 123
Quoted 2 lines
include/linux/fsl/imx-pinctrl.h new file +2 -0
+#define IMX6Q_GPR1_MIPI_COLOR_SW               BIT(25)
+#define IMX6Q_GPR1_DPI_OFF                     BIT(24)
Looks related to some test or something...

Quoted 4 lines

And so on.
If you really wants a "funnel driver" doing all these diverse things, I'd put it in drivers/mfd.

It's like driver drivers/mfd/anatop-mfd.c. They both store misc bits. We may need a generic driver to simply provide register accessor.

Thanks
Richard

Quoted 18 lines

This whole issue appears in other systems so you're not alone on this, for example the ux500 PRCMU driver has exactly these properties.
I'm also contemplating drivers/syscon again, but
I have a hard time seeing it would be much more than another drivers/mfd-similar construct.
I would really like input from Arnd and Samuel and other clever people on the placement of drivers like this one :-/
But close address range proximity to the pin controller is not a reason to have it in pinctrl.
Yours,
Linus Walleij

Arnd Bergmann

2012-07-16 13:40 UTC #8 lore raw

PATCH v2 1/2 pinctrl: pinctrl-imx: add support for set bits for general purpose registers

Quoted 5 lines · Richard Zhao

Quoted 2 lines
If you really wants a "funnel driver" doing all these diverse things, I'd put it in drivers/mfd.
It's like driver drivers/mfd/anatop-mfd.c. They both store misc bits. We may need a generic driver to simply provide register accessor.

We already have the "regmap" framework that deals with sets of registers on spi, i2c or memory mapped buses. One thing I've thought about before is to create some kind of very simple "system controller" framework based on regmap that lets you register a single global "struct regmap" pointer from an SoC specific driver, and have drivers call a function into that framework to get a reference to that regmap.

Arnd

Linus Walleij

2012-07-16 22:15 UTC #9 lore raw

PATCH v2 1/2 pinctrl: pinctrl-imx: add support for set bits for general purpose registers

Quoted 6 lines · Arnd Bergmann

One thing I've thought about
before is to create some kind of very simple "system controller" framework based on regmap that lets you register a single global "struct regmap" pointer from an SoC specific driver, and have drivers call a function into that framework to get a reference to that regmap.

This sounds like a good idea. "misc-bit-poking" is something a lot of systems are doing.

I haven't fully grasped regmap so that I can say whether that is the proper abstraction or not though. But I trust you on this...

Yours,
Linus Walleij

Dong Aisheng

author

2012-07-17 03:04 UTC #10 lore raw

PATCH v2 1/2 pinctrl: pinctrl-imx: add support for set bits for general purpose registers

In reply to Arnd Bergmann

Quoted 9 lines · Arnd Bergmann

Quoted 5 lines · Richard Zhao
Quoted 2 lines
If you really wants a "funnel driver" doing all these diverse things, I'd put it in drivers/mfd.
It's like driver drivers/mfd/anatop-mfd.c. They both store misc bits. We may need a generic driver to simply provide register accessor.
We already have the "regmap" framework that deals with sets of registers on spi, i2c or memory mapped buses. One thing I've thought about

Thanks for the hints.
Will look into it.

Quoted 6 lines

before is to create some kind of very simple "system controller" framework based on regmap that lets you register a single global "struct regmap" pointer from an SoC specific driver, and have drivers call a function into that framework to get a reference to that regmap.

That would be a good idea to make people easy use.

Regards
Dong Aisheng

Dong Aisheng

author

2012-07-17 03:02 UTC #11 lore raw

PATCH v2 1/2 pinctrl: pinctrl-imx: add support for set bits for general purpose registers

In reply to Linus Walleij

Quoted 7 lines · Linus Walleij

On Thu, Jul 12, 2012 at 11:07 AM, Dong Aisheng <[email protected]> wrote:
Hm, hm. This makes be ever more hesitant to have this in pinctrl.
Remeber again that nothing stops you from remapping the same register range in another driver.

Well, i understand and agree with your point. We will try and see if we can find a better place.

Quoted 7 lines

Quoted 1 line
+#define IMX6Q_GPR0_CLOCK_8_MUX_SEL_MASK (0x3 << 30)
This belongs in drivers/clk/*
Why funnel these register writes through pinctrl? Just remap that address offset in the clk driver.

Hmm, i looked a bit more, not sure they could be put in driver/clk/* The mux here are module internal clocks, like:
Selects the source of asrck_clock_3 in ASRC according to clock muxing scheme:
00 audmux.amx_output_rxclk_p7 muxed with ssi3.ssi_srck
01 audmux.amx_output_rxclk_p7
10 ssi3.ssi_srck
11 ssi3.rx_bit_clk

I'm not sure we should abstract them in clk framework since usually they're internally controlled by module driver itself.

Quoted 8 lines

Quoted 1 line
+#define IMX6Q_GPR0_DMAREQ_MUX_SEL7_MASK BIT(7)
This belongs in drivers/dma/*
Same comments.
Quoted 1 line
+#define IMX6Q_GPR1_PCIE_REQ_MASK (0x3 << 30)

Quoted 9 lines

Looks like it belongs in some PCI driver or glue layer.
include/linux/fsl/imx-pinctrl.h line 123
Quoted 2 lines
include/linux/fsl/imx-pinctrl.h new file +2 -0
+#define IMX6Q_GPR1_MIPI_COLOR_SW               BIT(25)
+#define IMX6Q_GPR1_DPI_OFF                     BIT(24)
Looks related to some test or something...
And so on.

Yes, most of them are modules specific.

Quoted 2 lines

If you really wants a "funnel driver" doing all these diverse things, I'd put it in drivers/mfd.

Yes, we may wants this "funnel driver".
What i'm thinking is we may only provides setting api for conveniently use and how drivers use it or abstract it is driver specific.

Quoted 5 lines

This whole issue appears in other systems so you're not alone on this, for example the ux500 PRCMU driver has exactly these properties.

So how does ux500 PRCMU handle this?

Quoted 4 lines

I'm also contemplating drivers/syscon again, but
I have a hard time seeing it would be much more than another drivers/mfd-similar construct.

yes, that may be a proper place.

Quoted 6 lines

I would really like input from Arnd and Samuel and other clever people on the placement of drivers like this one :-/
But close address range proximity to the pin controller is not a reason to have it in pinctrl.

Well, understand.

Regards
Dong Aisheng

Linus Walleij

2012-07-17 18:15 UTC #12 lore raw

PATCH v2 1/2 pinctrl: pinctrl-imx: add support for set bits for general purpose registers

Quoted 7 lines · Dong Aisheng

Quoted 4 lines · Linus Walleij
This whole issue appears in other systems so you're not alone on this, for example the ux500 PRCMU driver has exactly these properties.
So how does ux500 PRCMU handle this?

First, this driver is in MFD which is the most apropriate place I can think of right now.

Then we have accessor functions, but when we just want to read/write a register we have this API:

u32 db8500_prcmu_read(unsigned int reg);
void db8500_prcmu_write(unsigned int reg, u32 value);
void db8500_prcmu_write_masked(unsigned int reg, u32 mask, u32 value);

The third function is an oddity which performs read-modify-write on the other side (firmware).

Yours,
Linus Walleij

Mark Brown

2012-07-17 18:24 UTC #13 lore raw

PATCH v2 1/2 pinctrl: pinctrl-imx: add support for set bits for general purpose registers

Quoted 6 lines · Linus Walleij

u32 db8500_prcmu_read(unsigned int reg);
void db8500_prcmu_write(unsigned int reg, u32 value);
void db8500_prcmu_write_masked(unsigned int reg, u32 mask, u32 value);
The third function is an oddity which performs read-modify-write on the other side (firmware).

Not so odd - a very high proportion of APIs like this have a r/m/w operation as there's usually a requirement for doing the updates atomically.