* [PATCH 1/3] drivers: base: add support for stmp-style devices
2011-11-16 10:47 [PATCH 0/3] make stmp-style devices mach-independent Wolfram Sang
@ 2011-11-16 10:47 ` Wolfram Sang
2011-11-16 17:44 ` Arnd Bergmann
2011-11-16 10:47 ` [PATCH 2/3] i2c: mxs: use global reset function Wolfram Sang
` (3 subsequent siblings)
4 siblings, 1 reply; 11+ messages in thread
From: Wolfram Sang @ 2011-11-16 10:47 UTC (permalink / raw)
To: linux-arm-kernel
Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
---
drivers/base/Kconfig | 3 ++
drivers/base/Makefile | 1 +
drivers/base/stmp_device.c | 80 +++++++++++++++++++++++++++++++++++++++++++
include/linux/stmp_device.h | 19 ++++++++++
4 files changed, 103 insertions(+), 0 deletions(-)
create mode 100644 drivers/base/stmp_device.c
create mode 100644 include/linux/stmp_device.h
diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig
index 21cf46f..331f78f 100644
--- a/drivers/base/Kconfig
+++ b/drivers/base/Kconfig
@@ -172,6 +172,9 @@ config SYS_HYPERVISOR
bool
default n
+config STMP_DEVICE
+ bool
+
source "drivers/base/regmap/Kconfig"
endmenu
diff --git a/drivers/base/Makefile b/drivers/base/Makefile
index 99a375a..b865072 100644
--- a/drivers/base/Makefile
+++ b/drivers/base/Makefile
@@ -17,6 +17,7 @@ ifeq ($(CONFIG_SYSFS),y)
obj-$(CONFIG_MODULES) += module.o
endif
obj-$(CONFIG_SYS_HYPERVISOR) += hypervisor.o
+obj-$(CONFIG_STMP_DEVICE) += stmp_device.o
obj-$(CONFIG_REGMAP) += regmap/
ccflags-$(CONFIG_DEBUG_DRIVER) := -DDEBUG
diff --git a/drivers/base/stmp_device.c b/drivers/base/stmp_device.c
new file mode 100644
index 0000000..3e261e6
--- /dev/null
+++ b/drivers/base/stmp_device.c
@@ -0,0 +1,80 @@
+/*
+ * Copyright (C) 1999 ARM Limited
+ * Copyright (C) 2000 Deep Blue Solutions Ltd
+ * Copyright 2006-2007,2010 Freescale Semiconductor, Inc. All Rights Reserved.
+ * Copyright 2008 Juergen Beisert, kernel at pengutronix.de
+ * Copyright 2009 Ilya Yanok, Emcraft Systems Ltd, yanok at emcraft.com
+ * Copyright (C) 2011 Wolfram Sang, Pengutronix e.K.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include <linux/io.h>
+#include <linux/errno.h>
+#include <linux/delay.h>
+#include <linux/module.h>
+#include <linux/stmp_device.h>
+
+#define STMP_MODULE_CLKGATE (1 << 30)
+#define STMP_MODULE_SFTRST (1 << 31)
+
+/*
+ * Clear the bit and poll it cleared. This is usually called with
+ * a reset address and mask being either SFTRST(bit 31) or CLKGATE
+ * (bit 30).
+ */
+static int stmp_clear_poll_bit(void __iomem *addr, u32 mask)
+{
+ int timeout = 0x400;
+
+ writel(mask, addr + STMP_CLR_ADDR);
+ udelay(1);
+ while ((readl(addr) & mask) && --timeout)
+ /* nothing */;
+
+ return !timeout;
+}
+
+int stmp_reset_block(void __iomem *reset_addr)
+{
+ int ret;
+ int timeout = 0x400;
+
+ /* clear and poll SFTRST */
+ ret = stmp_clear_poll_bit(reset_addr, STMP_MODULE_SFTRST);
+ if (unlikely(ret))
+ goto error;
+
+ /* clear CLKGATE */
+ writel(STMP_MODULE_CLKGATE, reset_addr + STMP_CLR_ADDR);
+
+ /* set SFTRST to reset the block */
+ writel(STMP_MODULE_SFTRST, reset_addr + STMP_SET_ADDR);
+ udelay(1);
+
+ /* poll CLKGATE becoming set */
+ while ((!(readl(reset_addr) & STMP_MODULE_CLKGATE)) && --timeout)
+ /* nothing */;
+ if (unlikely(!timeout))
+ goto error;
+
+ /* clear and poll SFTRST */
+ ret = stmp_clear_poll_bit(reset_addr, STMP_MODULE_SFTRST);
+ if (unlikely(ret))
+ goto error;
+
+ /* clear and poll CLKGATE */
+ ret = stmp_clear_poll_bit(reset_addr, STMP_MODULE_CLKGATE);
+ if (unlikely(ret))
+ goto error;
+
+ return 0;
+
+error:
+ pr_err("%s(%p): module reset timeout\n", __func__, reset_addr);
+ return -ETIMEDOUT;
+}
+EXPORT_SYMBOL(stmp_reset_block);
diff --git a/include/linux/stmp_device.h b/include/linux/stmp_device.h
new file mode 100644
index 0000000..330f8d8
--- /dev/null
+++ b/include/linux/stmp_device.h
@@ -0,0 +1,19 @@
+/*
+ * basic functions for devices following the "stmp" style register layout
+ *
+ * Copyright (C) 2011 Wolfram Sang, Pengutronix e.K.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 2 of the License.
+ */
+
+#ifndef __STMP_DEVICE_H__
+#define __STMP_DEVICE_H__
+
+#define STMP_SET_ADDR 0x4
+#define STMP_CLR_ADDR 0x8
+#define STMP_TOG_ADDR 0xc
+
+extern int stmp_reset_block(void __iomem *);
+#endif /* __STMP_DEVICE_H__ */
--
1.7.7.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 1/3] drivers: base: add support for stmp-style devices
2011-11-16 10:47 ` [PATCH 1/3] drivers: base: add support for stmp-style devices Wolfram Sang
@ 2011-11-16 17:44 ` Arnd Bergmann
2011-11-16 17:57 ` Michał Mirosław
2011-11-16 19:19 ` Wolfram Sang
0 siblings, 2 replies; 11+ messages in thread
From: Arnd Bergmann @ 2011-11-16 17:44 UTC (permalink / raw)
To: linux-arm-kernel
On Wednesday 16 November 2011, Wolfram Sang wrote:
> Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
Introducing new core code definitely requires a long patch description.
How about copying the description from the introductory mail here?
> drivers/base/Kconfig | 3 ++
> drivers/base/Makefile | 1 +
> drivers/base/stmp_device.c | 80 +++++++++++++++++++++++++++++++++++++++++++
> include/linux/stmp_device.h | 19 ++++++++++
> 4 files changed, 103 insertions(+), 0 deletions(-)
> create mode 100644 drivers/base/stmp_device.c
> create mode 100644 include/linux/stmp_device.h
I'm not convinced that drivers/base is the right place, but I also can't
think of anything better right now.
> diff --git a/drivers/base/stmp_device.c b/drivers/base/stmp_device.c
> +/*
> + * Clear the bit and poll it cleared. This is usually called with
> + * a reset address and mask being either SFTRST(bit 31) or CLKGATE
> + * (bit 30).
> + */
> +static int stmp_clear_poll_bit(void __iomem *addr, u32 mask)
> +{
> + int timeout = 0x400;
> +
> + writel(mask, addr + STMP_CLR_ADDR);
> + udelay(1);
> + while ((readl(addr) & mask) && --timeout)
> + /* nothing */;
> +
> + return !timeout;
> +}
For portable code, you should use cpu_relax() inside of the loop.
Is the udelay() actually necessary here?
> +int stmp_reset_block(void __iomem *reset_addr)
> +{
> + int ret;
> + int timeout = 0x400;
> +
> + /* clear and poll SFTRST */
> + ret = stmp_clear_poll_bit(reset_addr, STMP_MODULE_SFTRST);
> + if (unlikely(ret))
> + goto error;
Please don't use likely()/unlikely() in code that is not very
performance sensitive. It will usually just increase the code size
but not actually have a measurable benefit.
> + /* clear CLKGATE */
> + writel(STMP_MODULE_CLKGATE, reset_addr + STMP_CLR_ADDR);
> +
> + /* set SFTRST to reset the block */
> + writel(STMP_MODULE_SFTRST, reset_addr + STMP_SET_ADDR);
> + udelay(1);
> +
> + /* poll CLKGATE becoming set */
> + while ((!(readl(reset_addr) & STMP_MODULE_CLKGATE)) && --timeout)
> + /* nothing */;
> + if (unlikely(!timeout))
> + goto error;
Since the run-time of a readl() may vary greatly, counting to 400
for a timeout seems completely arbitrary and unhelpful.
A better construct is
long timeout = jiffies + HZ / 10; /* wait for at most 100ms */
do {
...
} while (time_before(jiffies, timeout));
or the ktime equivalent of that if you are waiting for very short times.
> + /* clear and poll SFTRST */
> + ret = stmp_clear_poll_bit(reset_addr, STMP_MODULE_SFTRST);
> + if (unlikely(ret))
> + goto error;
> +
> + /* clear and poll CLKGATE */
> + ret = stmp_clear_poll_bit(reset_addr, STMP_MODULE_CLKGATE);
> + if (unlikely(ret))
> + goto error;
> +
> + return 0;
> +
> +error:
> + pr_err("%s(%p): module reset timeout\n", __func__, reset_addr);
> + return -ETIMEDOUT;
> +}
> +EXPORT_SYMBOL(stmp_reset_block);
EXPORT_SYMBOL_GPL?
> diff --git a/include/linux/stmp_device.h b/include/linux/stmp_device.h
> new file mode 100644
> index 0000000..330f8d8
> --- /dev/null
> +++ b/include/linux/stmp_device.h
> @@ -0,0 +1,19 @@
> +#ifndef __STMP_DEVICE_H__
> +#define __STMP_DEVICE_H__
> +
> +#define STMP_SET_ADDR 0x4
> +#define STMP_CLR_ADDR 0x8
> +#define STMP_TOG_ADDR 0xc
The register numbers should probably go into the implementation file,
they are not an interface.
> +extern int stmp_reset_block(void __iomem *);
> +#endif /* __STMP_DEVICE_H__ */
Arnd
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 1/3] drivers: base: add support for stmp-style devices
2011-11-16 17:44 ` Arnd Bergmann
@ 2011-11-16 17:57 ` Michał Mirosław
2011-11-16 19:19 ` Wolfram Sang
2011-11-16 19:19 ` Wolfram Sang
1 sibling, 1 reply; 11+ messages in thread
From: Michał Mirosław @ 2011-11-16 17:57 UTC (permalink / raw)
To: linux-arm-kernel
2011/11/16 Arnd Bergmann <arnd@arndb.de>:
> On Wednesday 16 November 2011, Wolfram Sang wrote:
[...]
>> --- /dev/null
>> +++ b/include/linux/stmp_device.h
>> @@ -0,0 +1,19 @@
>> +#ifndef __STMP_DEVICE_H__
>> +#define __STMP_DEVICE_H__
>> +
>> +#define STMP_SET_ADDR ? ? ? ? ? ? ? ?0x4
>> +#define STMP_CLR_ADDR ? ? ? ? ? ? ? ?0x8
>> +#define STMP_TOG_ADDR ? ? ? ? ? ? ? ?0xc
>
> The register numbers should probably go into the implementation file,
> they are not an interface.
Those are offsets - for every register there are "shadow" registers
that do implicit OR, AND NOT, XOR of that register with value written.
There might be better names for those.
Best Regards,
Micha? Miros?aw
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 1/3] drivers: base: add support for stmp-style devices
2011-11-16 17:44 ` Arnd Bergmann
2011-11-16 17:57 ` Michał Mirosław
@ 2011-11-16 19:19 ` Wolfram Sang
1 sibling, 0 replies; 11+ messages in thread
From: Wolfram Sang @ 2011-11-16 19:19 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, Nov 16, 2011 at 05:44:19PM +0000, Arnd Bergmann wrote:
> On Wednesday 16 November 2011, Wolfram Sang wrote:
> > Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
>
> Introducing new core code definitely requires a long patch description.
> How about copying the description from the introductory mail here?
What I mainly forgot was s/PATCH/RFC/, and I'm sorry about that. I know this
series is not suitable to be applied directly, I was mainly looking for
comments from the arm people who are affected by this change.
> > +static int stmp_clear_poll_bit(void __iomem *addr, u32 mask)
> > +{
> > + int timeout = 0x400;
> > +
> > + writel(mask, addr + STMP_CLR_ADDR);
> > + udelay(1);
> > + while ((readl(addr) & mask) && --timeout)
> > + /* nothing */;
> > +
> > + return !timeout;
> > +}
>
> For portable code, you should use cpu_relax() inside of the loop.
>
> Is the udelay() actually necessary here?
I am just copying the code from the current mxs-implementation. I think fixups
(yes, needed!) should go in with seperate patches. Should have said so
explicitly.
> > + ret = stmp_clear_poll_bit(reset_addr, STMP_MODULE_SFTRST);
> > + if (unlikely(ret))
> > + goto error;
>
> Please don't use likely()/unlikely() in code that is not very
> performance sensitive. It will usually just increase the code size
> but not actually have a measurable benefit.
Ditto.
> > + if (unlikely(!timeout))
> > + goto error;
>
> Since the run-time of a readl() may vary greatly, counting to 400
> for a timeout seems completely arbitrary and unhelpful.
Ditto, I know. I talked about such things in Prague this year :)
> long timeout = jiffies + HZ / 10; /* wait for at most 100ms */
>
> do {
> ...
> } while (time_before(jiffies, timeout));
Better, but not perfect ;) But I'll skip the discussion here...
> > +EXPORT_SYMBOL(stmp_reset_block);
>
> EXPORT_SYMBOL_GPL?
Fine with me.
> > +#define STMP_SET_ADDR 0x4
> > +#define STMP_CLR_ADDR 0x8
> > +#define STMP_TOG_ADDR 0xc
>
> The register numbers should probably go into the implementation file,
> they are not an interface.
As said, those are offsets. Especially useful for:
offset = enabled ? STMP_SET_ADDR : STMP_CLR_ADDR;
writel(bits1, reg1 + offset);
writel(bits2, reg2 + offset);
...
That will either set or clear bits, depending on 'enabled'.
Regards,
Wolfram
--
Pengutronix e.K. | Wolfram Sang |
Industrial Linux Solutions | http://www.pengutronix.de/ |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20111116/64590afd/attachment.sig>
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 2/3] i2c: mxs: use global reset function
2011-11-16 10:47 [PATCH 0/3] make stmp-style devices mach-independent Wolfram Sang
2011-11-16 10:47 ` [PATCH 1/3] drivers: base: add support for stmp-style devices Wolfram Sang
@ 2011-11-16 10:47 ` Wolfram Sang
2011-11-16 10:47 ` [PATCH 3/3] rtc: stmp3xxx: use global stmp_device functionality Wolfram Sang
` (2 subsequent siblings)
4 siblings, 0 replies; 11+ messages in thread
From: Wolfram Sang @ 2011-11-16 10:47 UTC (permalink / raw)
To: linux-arm-kernel
The former mach specific reset_block function has been converted to a global
one. Use the new one to remove mach dependency from the driver.
Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
---
drivers/i2c/busses/Kconfig | 1 +
drivers/i2c/busses/i2c-mxs.c | 9 ++-------
2 files changed, 3 insertions(+), 7 deletions(-)
diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index a3afac4..76714a8 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -467,6 +467,7 @@ config I2C_MV64XXX
config I2C_MXS
tristate "Freescale i.MX28 I2C interface"
depends on SOC_IMX28
+ select STMP_DEVICE
help
Say Y here if you want to use the I2C bus controller on
the Freescale i.MX28 processors.
diff --git a/drivers/i2c/busses/i2c-mxs.c b/drivers/i2c/busses/i2c-mxs.c
index 7e78f7c..8b92e40 100644
--- a/drivers/i2c/busses/i2c-mxs.c
+++ b/drivers/i2c/busses/i2c-mxs.c
@@ -26,8 +26,7 @@
#include <linux/platform_device.h>
#include <linux/jiffies.h>
#include <linux/io.h>
-
-#include <mach/common.h>
+#include <linux/stmp_device.h>
#define DRIVER_NAME "mxs-i2c"
@@ -110,13 +109,9 @@ struct mxs_i2c_dev {
struct i2c_adapter adapter;
};
-/*
- * TODO: check if calls to here are really needed. If not, we could get rid of
- * mxs_reset_block and the mach-dependency. Needs an I2C analyzer, probably.
- */
static void mxs_i2c_reset(struct mxs_i2c_dev *i2c)
{
- mxs_reset_block(i2c->regs);
+ stmp_reset_block(i2c->regs);
writel(MXS_I2C_IRQ_MASK << 8, i2c->regs + MXS_I2C_CTRL1_SET);
writel(MXS_I2C_QUEUECTRL_PIO_QUEUE_MODE,
i2c->regs + MXS_I2C_QUEUECTRL_SET);
--
1.7.7.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 3/3] rtc: stmp3xxx: use global stmp_device functionality
2011-11-16 10:47 [PATCH 0/3] make stmp-style devices mach-independent Wolfram Sang
2011-11-16 10:47 ` [PATCH 1/3] drivers: base: add support for stmp-style devices Wolfram Sang
2011-11-16 10:47 ` [PATCH 2/3] i2c: mxs: use global reset function Wolfram Sang
@ 2011-11-16 10:47 ` Wolfram Sang
2011-11-17 1:49 ` [PATCH 0/3] make stmp-style devices mach-independent Shawn Guo
2011-11-17 6:57 ` Dong Aisheng-B29396
4 siblings, 0 replies; 11+ messages in thread
From: Wolfram Sang @ 2011-11-16 10:47 UTC (permalink / raw)
To: linux-arm-kernel
The former mach specific reset_block function has been converted to a global
one. Use the new one to remove mach dependency from the driver. Also simplify
code using the new STMP_REG_* macros.
Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
---
drivers/rtc/Kconfig | 1 +
drivers/rtc/rtc-stmp3xxx.c | 29 ++++++++++-------------------
2 files changed, 11 insertions(+), 19 deletions(-)
diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
index 53eb4e5..babdd5d 100644
--- a/drivers/rtc/Kconfig
+++ b/drivers/rtc/Kconfig
@@ -983,6 +983,7 @@ config RTC_DRV_COH901331
config RTC_DRV_STMP
tristate "Freescale STMP3xxx/i.MX23/i.MX28 RTC"
depends on ARCH_MXS
+ select STMP_DEVICE
help
If you say yes here you will get support for the onboard
STMP3xxx/i.MX23/i.MX28 RTC.
diff --git a/drivers/rtc/rtc-stmp3xxx.c b/drivers/rtc/rtc-stmp3xxx.c
index 7315068..23db0ed 100644
--- a/drivers/rtc/rtc-stmp3xxx.c
+++ b/drivers/rtc/rtc-stmp3xxx.c
@@ -25,11 +25,9 @@
#include <linux/interrupt.h>
#include <linux/rtc.h>
#include <linux/slab.h>
-
-#include <mach/common.h>
+#include <linux/stmp_device.h>
#define STMP3XXX_RTC_CTRL 0x0
-#define STMP3XXX_RTC_CTRL_SET 0x4
#define STMP3XXX_RTC_CTRL_CLR 0x8
#define STMP3XXX_RTC_CTRL_ALARM_IRQ_EN 0x00000001
#define STMP3XXX_RTC_CTRL_ONEMSEC_IRQ_EN 0x00000002
@@ -44,7 +42,6 @@
#define STMP3XXX_RTC_ALARM 0x40
#define STMP3XXX_RTC_PERSISTENT0 0x60
-#define STMP3XXX_RTC_PERSISTENT0_SET 0x64
#define STMP3XXX_RTC_PERSISTENT0_CLR 0x68
#define STMP3XXX_RTC_PERSISTENT0_ALARM_WAKE_EN 0x00000002
#define STMP3XXX_RTC_PERSISTENT0_ALARM_EN 0x00000004
@@ -106,20 +103,14 @@ static irqreturn_t stmp3xxx_rtc_interrupt(int irq, void *dev_id)
static int stmp3xxx_alarm_irq_enable(struct device *dev, unsigned int enabled)
{
struct stmp3xxx_rtc_data *rtc_data = dev_get_drvdata(dev);
+ u32 set_clr_offset = enabled ? STMP_SET_ADDR : STMP_CLR_ADDR;
+
+ writel(STMP3XXX_RTC_PERSISTENT0_ALARM_EN |
+ STMP3XXX_RTC_PERSISTENT0_ALARM_WAKE_EN,
+ rtc_data->io + STMP3XXX_RTC_PERSISTENT0 + set_clr_offset);
+ writel(STMP3XXX_RTC_CTRL_ALARM_IRQ_EN,
+ rtc_data->io + STMP3XXX_RTC_CTRL + set_clr_offset);
- if (enabled) {
- writel(STMP3XXX_RTC_PERSISTENT0_ALARM_EN |
- STMP3XXX_RTC_PERSISTENT0_ALARM_WAKE_EN,
- rtc_data->io + STMP3XXX_RTC_PERSISTENT0_SET);
- writel(STMP3XXX_RTC_CTRL_ALARM_IRQ_EN,
- rtc_data->io + STMP3XXX_RTC_CTRL_SET);
- } else {
- writel(STMP3XXX_RTC_PERSISTENT0_ALARM_EN |
- STMP3XXX_RTC_PERSISTENT0_ALARM_WAKE_EN,
- rtc_data->io + STMP3XXX_RTC_PERSISTENT0_CLR);
- writel(STMP3XXX_RTC_CTRL_ALARM_IRQ_EN,
- rtc_data->io + STMP3XXX_RTC_CTRL_CLR);
- }
return 0;
}
@@ -206,7 +197,7 @@ static int stmp3xxx_rtc_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, rtc_data);
- mxs_reset_block(rtc_data->io);
+ stmp_reset_block(rtc_data->io);
writel(STMP3XXX_RTC_PERSISTENT0_ALARM_EN |
STMP3XXX_RTC_PERSISTENT0_ALARM_WAKE_EN |
STMP3XXX_RTC_PERSISTENT0_ALARM_WAKE,
@@ -253,7 +244,7 @@ static int stmp3xxx_rtc_resume(struct platform_device *dev)
{
struct stmp3xxx_rtc_data *rtc_data = platform_get_drvdata(dev);
- mxs_reset_block(rtc_data->io);
+ stmp_reset_block(rtc_data->io);
writel(STMP3XXX_RTC_PERSISTENT0_ALARM_EN |
STMP3XXX_RTC_PERSISTENT0_ALARM_WAKE_EN |
STMP3XXX_RTC_PERSISTENT0_ALARM_WAKE,
--
1.7.7.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 0/3] make stmp-style devices mach-independent
2011-11-16 10:47 [PATCH 0/3] make stmp-style devices mach-independent Wolfram Sang
` (2 preceding siblings ...)
2011-11-16 10:47 ` [PATCH 3/3] rtc: stmp3xxx: use global stmp_device functionality Wolfram Sang
@ 2011-11-17 1:49 ` Shawn Guo
2011-11-17 10:32 ` Wolfram Sang
2011-11-17 6:57 ` Dong Aisheng-B29396
4 siblings, 1 reply; 11+ messages in thread
From: Shawn Guo @ 2011-11-17 1:49 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, Nov 16, 2011 at 11:47:36AM +0100, Wolfram Sang wrote:
> It seems I haven't been following the lists too closely lately, so I might have
> missed similar patches. Yet, I noticed some patches dealing with drivers using
> mxs-specific features, so they need to include mach-specific files which is
> troublesome. I started to address the problem as well some time ago, so I push
> out the patches now as RFC to enrich the discussion. The patches are rebased to
> linux 3.2-rc2 and are boot tested.
>
> The basics: mach-mxs uses ip-cores which follow a register layout I have
> first seen on STMP SoCs. That means, every register has four incarnations:
> - one u32 to store a value
> - a SET register where every 1-bit sets the corresponding bit,
> others are unaffected
> - similar with a CLR register
> - and a TGL (toggle) register
>
> Also, the 2 MSBs in register 0 are always the same and can be used to reset the
> block.
>
> All this is strictly speaking not mach-specific (but ip-core specific) and,
> thus, doesn't need to be in mach-mxs/include. As I have been told, mx50 and
> mx6(?) might also use IP cores following the STMP-style; so I wondered if it
> can't be exported like the following patch series does:
>
> Introduce a stmp-style device, put the code around that in a public place
> (currently drivers/base/; dunno if that is the best place, though), and let
> drivers for stmp-style devices select that code.
>
> Voila, mach dependency gone, reusable code introduced. Note that I didn't
> remove the duplicated code from mach-mxs yet, first all drivers have to be
> converted.
>
> Right thing to do?
I think it's the right thing to do. But I do not like the name as
stmp-style device. Why not just mxs-style device? Name mxs is born
to replace stmp, and keeping using two names may confuse people.
Also sticking to mxs may save the diffs like the one below.
- mxs_reset_block(i2c->regs);
+ stmp_reset_block(i2c->regs);
Regards,
Shawn
> If so, I'll repost properly to the right lists. Fishing for
> feedback right now :)
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 0/3] make stmp-style devices mach-independent
2011-11-16 10:47 [PATCH 0/3] make stmp-style devices mach-independent Wolfram Sang
` (3 preceding siblings ...)
2011-11-17 1:49 ` [PATCH 0/3] make stmp-style devices mach-independent Shawn Guo
@ 2011-11-17 6:57 ` Dong Aisheng-B29396
4 siblings, 0 replies; 11+ messages in thread
From: Dong Aisheng-B29396 @ 2011-11-17 6:57 UTC (permalink / raw)
To: linux-arm-kernel
> -----Original Message-----
> From: Wolfram Sang [mailto:w.sang at pengutronix.de]
> Sent: Wednesday, November 16, 2011 6:48 PM
> To: linux-arm-kernel at lists.infradead.org
> Cc: Sascha Hauer; Guo Shawn-R65073; Dong Aisheng-B29396; Arnd Bergmann;
> Wolfram Sang
> Subject: [PATCH 0/3] make stmp-style devices mach-independent
>
> It seems I haven't been following the lists too closely lately, so I
> might have missed similar patches. Yet, I noticed some patches dealing
> with drivers using mxs-specific features, so they need to include mach-
> specific files which is troublesome. I started to address the problem as
> well some time ago, so I push out the patches now as RFC to enrich the
> discussion. The patches are rebased to linux 3.2-rc2 and are boot tested.
>
> The basics: mach-mxs uses ip-cores which follow a register layout I have
> first seen on STMP SoCs. That means, every register has four incarnations:
> - one u32 to store a value
> - a SET register where every 1-bit sets the corresponding bit,
> others are unaffected
> - similar with a CLR register
> - and a TGL (toggle) register
>
> Also, the 2 MSBs in register 0 are always the same and can be used to
> reset the block.
>
> All this is strictly speaking not mach-specific (but ip-core specific)
> and, thus, doesn't need to be in mach-mxs/include. As I have been told,
> mx50 and
> mx6(?) might also use IP cores following the STMP-style; so I wondered if
> it can't be exported like the following patch series does:
>
> Introduce a stmp-style device, put the code around that in a public place
> (currently drivers/base/; dunno if that is the best place, though), and
> let drivers for stmp-style devices select that code.
>
> Voila, mach dependency gone, reusable code introduced. Note that I didn't
> remove the duplicated code from mach-mxs yet, first all drivers have to
> be converted.
>
> Right thing to do? If so, I'll repost properly to the right lists.
> Fishing for feedback right now :)
Good thing to do.
I think it addressed my concern which raised in another patch
[PATCH 1/1] gpio: gpio-mxs: remove the duplicated micro define.
One thing is that for some people not know the history
between smtp and mxs, it maybe a little confused to see calling smtp_xx code
in mxs code.
Just as Shawn said, if we will final move to mxs in the long term,
maybe mxs name is better, right?
> Thanks,
>
> Wolfram
>
> Wolfram Sang (3):
> drivers: base: add support for stmp-style devices
> i2c: mxs: use global reset function
> rtc: stmp3xxx: use global stmp_device functionality
>
> drivers/base/Kconfig | 3 ++
> drivers/base/Makefile | 1 +
> drivers/base/stmp_device.c | 80
> ++++++++++++++++++++++++++++++++++++++++++
> drivers/i2c/busses/Kconfig | 1 +
> drivers/i2c/busses/i2c-mxs.c | 9 +----
> drivers/rtc/Kconfig | 1 +
> drivers/rtc/rtc-stmp3xxx.c | 29 +++++----------
> include/linux/stmp_device.h | 19 ++++++++++
> 8 files changed, 117 insertions(+), 26 deletions(-) create mode 100644
> drivers/base/stmp_device.c create mode 100644
> include/linux/stmp_device.h
>
> --
> 1.7.7.1
>
^ permalink raw reply [flat|nested] 11+ messages in thread