Re: [RESEND PATCH] rtc: rk808: rename rtc-rk808.c to rtc-rk8xx.c
Hi,
On 05/01/2016 at 16:31:38 +0800, Huang, Tao wrote :
> You and Alexandre are right. The rename is just make the driver more
> readable, i.e. let people know this driver suit for more PMIC no just
> rk808. In fact, I don't care the name is rk808 or rk8xx.
>
For this purpose, you can add the name of the supported PMICs in the
Kconfig.
> The key change of this patch is try to dis-coupling rk808 driver and RTC
> driver. Because of register offset and function is vary between
> different PMIC, we believe it is hard to write one PMIC driver to suit
> all PMIC. So we hope RTC driver can share between all PMIC from rockchip.
>
> Please review this code:
>
> -static int rk808_rtc_probe(struct platform_device *pdev)
> +static int rk8xx_rtc_probe(struct platform_device *pdev)
> {
> - struct rk808 *rk808 = dev_get_drvdata(pdev->dev.parent);
> ...
> + struct i2c_client *client = to_i2c_client(pdev->dev.parent);
>
> ...
>
> - rk808_rtc->rk808 = rk808;
> + rk8xx_rtc->regmap = devm_regmap_init_i2c(client,
> + &rk8xx_rtc_regmap_config);
> ...
> + rk8xx_rtc->i2c = client;
>
> Old driver have struct rk808 pointer, which defined on
> include/linux/mfd/rk808.h
> If we write new PMIC driver, for example rk818, define a new struct
> rk818. How can we get this pointer from RTC driver?
>
> So another way to solve this problem is introduce common struct share
> between all PMIC driver. For example rk8xx.
>
That is probably the best solution. If you want to reuse drivers between
pmic, then have a common structure that you can pass to those drivers.
I believe the regmap_init_i2c should stay in the MFD driver.
--
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RESEND PATCH] rtc: rk808: rename rtc-rk808.c to rtc-rk8xx.c
Hi, Alessandro:
On 2016年01月04日 21:59, Alessandro Zummo wrote:
> On Mon, 4 Jan 2016 10:45:46 +0100
> Alexandre Belloni wrote:
>
>> I'm not sure it is useful to do that renaming. It is usual to have one
>> driver that supports multiple chips named with the forst chip it
>> supported.
>>
>> Also, what would happen if for example rk855 is not compatible at all
>> with the previous implementations?
>
> Alexandre is absolutely right. There's no need to rename a driver,
> it would just piss off people who are used to that name and
> have it in their scripts. Like when your eth0 gets renamed
> to some obscure enXXX .
>
You and Alexandre are right. The rename is just make the driver more
readable, i.e. let people know this driver suit for more PMIC no just
rk808. In fact, I don't care the name is rk808 or rk8xx.
The key change of this patch is try to dis-coupling rk808 driver and RTC
driver. Because of register offset and function is vary between
different PMIC, we believe it is hard to write one PMIC driver to suit
all PMIC. So we hope RTC driver can share between all PMIC from rockchip.
Please review this code:
-static int rk808_rtc_probe(struct platform_device *pdev)
+static int rk8xx_rtc_probe(struct platform_device *pdev)
{
- struct rk808 *rk808 = dev_get_drvdata(pdev->dev.parent);
...
+ struct i2c_client *client = to_i2c_client(pdev->dev.parent);
...
- rk808_rtc->rk808 = rk808;
+ rk8xx_rtc->regmap = devm_regmap_init_i2c(client,
+&rk8xx_rtc_regmap_config);
...
+ rk8xx_rtc->i2c = client;
Old driver have struct rk808 pointer, which defined on
include/linux/mfd/rk808.h
If we write new PMIC driver, for example rk818, define a new struct
rk818. How can we get this pointer from RTC driver?
So another way to solve this problem is introduce common struct share
between all PMIC driver. For example rk8xx.
We solve this problem by create new regmap to access PMIC. As I say
before, it make RTC driver independent of PMIC driver. Do you agree this
change?
Thanks!
Huang, Tao
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RESEND PATCH] rtc: rk808: rename rtc-rk808.c to rtc-rk8xx.c
On Mon, 4 Jan 2016 10:45:46 +0100 Alexandre Belloni wrote: > I'm not sure it is useful to do that renaming. It is usual to have one > driver that supports multiple chips named with the forst chip it > supported. > > Also, what would happen if for example rk855 is not compatible at all > with the previous implementations? Alexandre is absolutely right. There's no need to rename a driver, it would just piss off people who are used to that name and have it in their scripts. Like when your eth0 gets renamed to some obscure enXXX . -- Best regards, Alessandro Zummo - CEO, Tower Technologies - Torino, Italy http://www.towertech.it -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RESEND PATCH] rtc: rk808: rename rtc-rk808.c to rtc-rk8xx.c
Hi,
On 31/12/2015 at 06:14:55 -0800, zhangqing wrote :
> make rtc-rk8xx.c compatible for all pmic chips.
> for pmic chips(rk808\rk807\rk816\rk818) in the future.
>
I'm not sure it is useful to do that renaming. It is usual to have one
driver that supports multiple chips named with the forst chip it
supported.
Also, what would happen if for example rk855 is not compatible at all
with the previous implementations?
> Signed-off-by: zhangqing
> ---
> drivers/mfd/rk808.c | 2 +-
> drivers/rtc/Kconfig | 8 +-
> drivers/rtc/Makefile | 2 +-
> drivers/rtc/{rtc-rk808.c => rtc-rk8xx.c} | 218
> ++-
> 4 files changed, 131 insertions(+), 99 deletions(-)
> rename drivers/rtc/{rtc-rk808.c => rtc-rk8xx.c} (64%)
>
> diff --git a/drivers/mfd/rk808.c b/drivers/mfd/rk808.c
> index 4b1e439..21da22b 100644
> --- a/drivers/mfd/rk808.c
> +++ b/drivers/mfd/rk808.c
> @@ -77,7 +77,7 @@ static const struct mfd_cell rk808s[] = {
> { .name = "rk808-clkout", },
> { .name = "rk808-regulator", },
> {
> - .name = "rk808-rtc",
> + .name = "rk8xx-rtc",
> .num_resources = ARRAY_SIZE(rtc_resources),
> .resources = &rtc_resources[0],
> },
> diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
> index 376322f..d669473d 100644
> --- a/drivers/rtc/Kconfig
> +++ b/drivers/rtc/Kconfig
> @@ -325,15 +325,15 @@ config RTC_DRV_MAX77686
> This driver can also be built as a module. If so, the module
> will be called rtc-max77686.
>
> -config RTC_DRV_RK808
> - tristate "Rockchip RK808 RTC"
> +config RTC_DRV_RK8XX
> + tristate "Rockchip RK8XX RTC"
> depends on MFD_RK808
> help
> If you say yes here you will get support for the
> - RTC of RK808 PMIC.
> + RTC of RK8XX PMIC.
>
> This driver can also be built as a module. If so, the module
> - will be called rk808-rtc.
> + will be called rk8xx-rtc.
>
> config RTC_DRV_MAX77802
> tristate "Maxim 77802 RTC"
> diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
> index 62d61b2..5b1384a 100644
> --- a/drivers/rtc/Makefile
> +++ b/drivers/rtc/Makefile
> @@ -120,7 +120,7 @@ obj-$(CONFIG_RTC_DRV_PUV3)+= rtc-puv3.o
> obj-$(CONFIG_RTC_DRV_PXA)+= rtc-pxa.o
> obj-$(CONFIG_RTC_DRV_R9701) += rtc-r9701.o
> obj-$(CONFIG_RTC_DRV_RC5T583)+= rtc-rc5t583.o
> -obj-$(CONFIG_RTC_DRV_RK808) += rtc-rk808.o
> +obj-$(CONFIG_RTC_DRV_RK8XX) += rtc-rk8xx.o
> obj-$(CONFIG_RTC_DRV_RP5C01) += rtc-rp5c01.o
> obj-$(CONFIG_RTC_DRV_RS5C313)+= rtc-rs5c313.o
> obj-$(CONFIG_RTC_DRV_RS5C348)+= rtc-rs5c348.o
> diff --git a/drivers/rtc/rtc-rk808.c b/drivers/rtc/rtc-rk8xx.c
> similarity index 64%
> rename from drivers/rtc/rtc-rk808.c
> rename to drivers/rtc/rtc-rk8xx.c
> index 35c9aad..5d946bf 100644
> --- a/drivers/rtc/rtc-rk808.c
> +++ b/drivers/rtc/rtc-rk8xx.c
> @@ -1,5 +1,5 @@
> /*
> - * RTC driver for Rockchip RK808
> + * RTC driver for Rockchip RK8XX
> *
> * Copyright (c) 2014, Fuzhou Rockchip Electronics Co., Ltd
> *
> @@ -20,14 +20,33 @@
> #include
> #include
> #include
> -#include
> #include
> #include
> +#include
> +
> +#define RK8XX_SECONDS_REG0x00
> +#define RK8XX_MINUTES_REG0x01
> +#define RK8XX_HOURS_REG 0x02
> +#define RK8XX_DAYS_REG 0x03
> +#define RK8XX_MONTHS_REG 0x04
> +#define RK8XX_YEARS_REG 0x05
> +#define RK8XX_WEEKS_REG 0x06
> +#define RK8XX_ALARM_SECONDS_REG 0x08
> +#define RK8XX_ALARM_MINUTES_REG 0x09
> +#define RK8XX_ALARM_HOURS_REG0x0A
> +#define RK8XX_ALARM_DAYS_REG 0x0B
> +#define RK8XX_ALARM_MONTHS_REG 0x0C
> +#define RK8XX_ALARM_YEARS_REG0x0D
> +#define RK8XX_RTC_CTRL_REG 0x10
> +#define RK8XX_RTC_STATUS_REG 0x11
> +#define RK8XX_RTC_INT_REG0x12
> +#define RK8XX_RTC_COMP_LSB_REG 0x13
> +#define RK8XX_RTC_COMP_MSB_REG 0x14
>
> /* RTC_CTRL_REG bitfields */
> #define BIT_RTC_CTRL_REG_STOP_RTC_M BIT(0)
>
> -/* RK808 has a shadowed register for saving a "frozen" RTC time.
> +/* RK8xx has a shadowed register for saving a "frozen" RTC time.
> * When user setting "GET_TIME" to 1, the time will save in this shadowed
> * register. If set "READSEL" to 1, user read rtc time register, actually
> * get the time of that moment. If we need the real time, clr this bit.
> @@ -47,17 +66,25 @@
>
> /* REG_SECONDS_REG through REG_YEARS_REG is how many registers? */
>
> -#define NUM_TIME_REGS(RK808_WEEKS_REG - RK808_SECONDS_REG + 1)
> -#define NUM_ALARM_REGS (RK808_ALARM_YEA
Re: [RESEND PATCH] rtc: rk808: rename rtc-rk808.c to rtc-rk8xx.c
Hi Zhang Qing,
On 12/31/2015 10:14 PM, zhangqing wrote:
make rtc-rk8xx.c compatible for all pmic chips.
for pmic chips(rk808\rk807\rk816\rk818) in the future.
The commit message will be better like this:
Rename the file to rtc-rk8xx.c to compatible other Rockchip PMIC chips
like rk807/rk816/rk818 and other chips in the the future.
Also rename the variable, MACRO, function and etc. names in relate file.
Signed-off-by: zhangqing
Prefer to use: Zhang Qing
---
drivers/mfd/rk808.c | 2 +-
drivers/rtc/Kconfig | 8 +-
drivers/rtc/Makefile | 2 +-
drivers/rtc/{rtc-rk808.c => rtc-rk8xx.c} | 218 ++-
4 files changed, 131 insertions(+), 99 deletions(-)
rename drivers/rtc/{rtc-rk808.c => rtc-rk8xx.c} (64%)
diff --git a/drivers/mfd/rk808.c b/drivers/mfd/rk808.c
index 4b1e439..21da22b 100644
--- a/drivers/mfd/rk808.c
+++ b/drivers/mfd/rk808.c
@@ -77,7 +77,7 @@ static const struct mfd_cell rk808s[] = {
{ .name = "rk808-clkout", },
{ .name = "rk808-regulator", },
{
- .name = "rk808-rtc",
+ .name = "rk8xx-rtc",
.num_resources = ARRAY_SIZE(rtc_resources),
.resources = &rtc_resources[0],
},
diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
index 376322f..d669473d 100644
--- a/drivers/rtc/Kconfig
+++ b/drivers/rtc/Kconfig
@@ -325,15 +325,15 @@ config RTC_DRV_MAX77686
This driver can also be built as a module. If so, the module
will be called rtc-max77686.
-config RTC_DRV_RK808
- tristate "Rockchip RK808 RTC"
+config RTC_DRV_RK8XX
+ tristate "Rockchip RK8XX RTC"
depends on MFD_RK808
help
If you say yes here you will get support for the
- RTC of RK808 PMIC.
+ RTC of RK8XX PMIC.
This driver can also be built as a module. If so, the module
- will be called rk808-rtc.
+ will be called rk8xx-rtc.
config RTC_DRV_MAX77802
tristate "Maxim 77802 RTC"
diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
index 62d61b2..5b1384a 100644
--- a/drivers/rtc/Makefile
+++ b/drivers/rtc/Makefile
@@ -120,7 +120,7 @@ obj-$(CONFIG_RTC_DRV_PUV3) += rtc-puv3.o
obj-$(CONFIG_RTC_DRV_PXA) += rtc-pxa.o
obj-$(CONFIG_RTC_DRV_R9701) += rtc-r9701.o
obj-$(CONFIG_RTC_DRV_RC5T583) += rtc-rc5t583.o
-obj-$(CONFIG_RTC_DRV_RK808)+= rtc-rk808.o
+obj-$(CONFIG_RTC_DRV_RK8XX)+= rtc-rk8xx.o
obj-$(CONFIG_RTC_DRV_RP5C01) += rtc-rp5c01.o
obj-$(CONFIG_RTC_DRV_RS5C313) += rtc-rs5c313.o
obj-$(CONFIG_RTC_DRV_RS5C348) += rtc-rs5c348.o
diff --git a/drivers/rtc/rtc-rk808.c b/drivers/rtc/rtc-rk8xx.c
similarity index 64%
rename from drivers/rtc/rtc-rk808.c
rename to drivers/rtc/rtc-rk8xx.c
index 35c9aad..5d946bf 100644
--- a/drivers/rtc/rtc-rk808.c
+++ b/drivers/rtc/rtc-rk8xx.c
@@ -1,5 +1,5 @@
/*
- * RTC driver for Rockchip RK808
+ * RTC driver for Rockchip RK8XX
*
* Copyright (c) 2014, Fuzhou Rockchip Electronics Co., Ltd
*
@@ -20,14 +20,33 @@
#include
#include
#include
-#include
#include
#include
+#include
+
+#define RK8XX_SECONDS_REG 0x00
+#define RK8XX_MINUTES_REG 0x01
+#define RK8XX_HOURS_REG0x02
+#define RK8XX_DAYS_REG 0x03
+#define RK8XX_MONTHS_REG 0x04
+#define RK8XX_YEARS_REG0x05
+#define RK8XX_WEEKS_REG0x06
+#define RK8XX_ALARM_SECONDS_REG0x08
+#define RK8XX_ALARM_MINUTES_REG0x09
+#define RK8XX_ALARM_HOURS_REG 0x0A
+#define RK8XX_ALARM_DAYS_REG 0x0B
+#define RK8XX_ALARM_MONTHS_REG 0x0C
+#define RK8XX_ALARM_YEARS_REG 0x0D
+#define RK8XX_RTC_CTRL_REG 0x10
+#define RK8XX_RTC_STATUS_REG 0x11
+#define RK8XX_RTC_INT_REG 0x12
+#define RK8XX_RTC_COMP_LSB_REG 0x13
+#define RK8XX_RTC_COMP_MSB_REG 0x14
/* RTC_CTRL_REG bitfields */
#define BIT_RTC_CTRL_REG_STOP_RTC_M BIT(0)
-/* RK808 has a shadowed register for saving a "frozen" RTC time.
+/* RK8xx has a shadowed register for saving a "frozen" RTC time.
* When user setting "GET_TIME" to 1, the time will save in this shadowed
* register. If set "READSEL" to 1, user read rtc time register, actually
* get the time of that moment. If we need the real time, clr this bit.
@@ -47,17 +66,25 @@
/* REG_SECONDS_REG through REG_YEARS_REG is how many registers? */
-#define NUM_TIME_REGS (RK808_WEEKS_REG - RK808_SECONDS_REG + 1)
-#define NUM_ALARM_REGS (RK808_ALARM_YEARS_REG - RK808_ALARM_SECONDS_REG + 1)
+#define NUM_TIME_REGS (RK8XX_WEEKS_REG - RK8XX_SECONDS_REG + 1)
+#define NUM_ALARM_REGS (RK8XX_ALARM_YEARS_REG - RK8XX_ALARM_SECONDS_REG
[RESEND PATCH] rtc: rk808: rename rtc-rk808.c to rtc-rk8xx.c
make rtc-rk8xx.c compatible for all pmic chips.
for pmic chips(rk808\rk807\rk816\rk818) in the future.
Signed-off-by: zhangqing
---
drivers/mfd/rk808.c | 2 +-
drivers/rtc/Kconfig | 8 +-
drivers/rtc/Makefile | 2 +-
drivers/rtc/{rtc-rk808.c => rtc-rk8xx.c} | 218 ++-
4 files changed, 131 insertions(+), 99 deletions(-)
rename drivers/rtc/{rtc-rk808.c => rtc-rk8xx.c} (64%)
diff --git a/drivers/mfd/rk808.c b/drivers/mfd/rk808.c
index 4b1e439..21da22b 100644
--- a/drivers/mfd/rk808.c
+++ b/drivers/mfd/rk808.c
@@ -77,7 +77,7 @@ static const struct mfd_cell rk808s[] = {
{ .name = "rk808-clkout", },
{ .name = "rk808-regulator", },
{
- .name = "rk808-rtc",
+ .name = "rk8xx-rtc",
.num_resources = ARRAY_SIZE(rtc_resources),
.resources = &rtc_resources[0],
},
diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
index 376322f..d669473d 100644
--- a/drivers/rtc/Kconfig
+++ b/drivers/rtc/Kconfig
@@ -325,15 +325,15 @@ config RTC_DRV_MAX77686
This driver can also be built as a module. If so, the module
will be called rtc-max77686.
-config RTC_DRV_RK808
- tristate "Rockchip RK808 RTC"
+config RTC_DRV_RK8XX
+ tristate "Rockchip RK8XX RTC"
depends on MFD_RK808
help
If you say yes here you will get support for the
- RTC of RK808 PMIC.
+ RTC of RK8XX PMIC.
This driver can also be built as a module. If so, the module
- will be called rk808-rtc.
+ will be called rk8xx-rtc.
config RTC_DRV_MAX77802
tristate "Maxim 77802 RTC"
diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
index 62d61b2..5b1384a 100644
--- a/drivers/rtc/Makefile
+++ b/drivers/rtc/Makefile
@@ -120,7 +120,7 @@ obj-$(CONFIG_RTC_DRV_PUV3) += rtc-puv3.o
obj-$(CONFIG_RTC_DRV_PXA) += rtc-pxa.o
obj-$(CONFIG_RTC_DRV_R9701)+= rtc-r9701.o
obj-$(CONFIG_RTC_DRV_RC5T583) += rtc-rc5t583.o
-obj-$(CONFIG_RTC_DRV_RK808)+= rtc-rk808.o
+obj-$(CONFIG_RTC_DRV_RK8XX)+= rtc-rk8xx.o
obj-$(CONFIG_RTC_DRV_RP5C01) += rtc-rp5c01.o
obj-$(CONFIG_RTC_DRV_RS5C313) += rtc-rs5c313.o
obj-$(CONFIG_RTC_DRV_RS5C348) += rtc-rs5c348.o
diff --git a/drivers/rtc/rtc-rk808.c b/drivers/rtc/rtc-rk8xx.c
similarity index 64%
rename from drivers/rtc/rtc-rk808.c
rename to drivers/rtc/rtc-rk8xx.c
index 35c9aad..5d946bf 100644
--- a/drivers/rtc/rtc-rk808.c
+++ b/drivers/rtc/rtc-rk8xx.c
@@ -1,5 +1,5 @@
/*
- * RTC driver for Rockchip RK808
+ * RTC driver for Rockchip RK8XX
*
* Copyright (c) 2014, Fuzhou Rockchip Electronics Co., Ltd
*
@@ -20,14 +20,33 @@
#include
#include
#include
-#include
#include
#include
+#include
+
+#define RK8XX_SECONDS_REG 0x00
+#define RK8XX_MINUTES_REG 0x01
+#define RK8XX_HOURS_REG0x02
+#define RK8XX_DAYS_REG 0x03
+#define RK8XX_MONTHS_REG 0x04
+#define RK8XX_YEARS_REG0x05
+#define RK8XX_WEEKS_REG0x06
+#define RK8XX_ALARM_SECONDS_REG0x08
+#define RK8XX_ALARM_MINUTES_REG0x09
+#define RK8XX_ALARM_HOURS_REG 0x0A
+#define RK8XX_ALARM_DAYS_REG 0x0B
+#define RK8XX_ALARM_MONTHS_REG 0x0C
+#define RK8XX_ALARM_YEARS_REG 0x0D
+#define RK8XX_RTC_CTRL_REG 0x10
+#define RK8XX_RTC_STATUS_REG 0x11
+#define RK8XX_RTC_INT_REG 0x12
+#define RK8XX_RTC_COMP_LSB_REG 0x13
+#define RK8XX_RTC_COMP_MSB_REG 0x14
/* RTC_CTRL_REG bitfields */
#define BIT_RTC_CTRL_REG_STOP_RTC_MBIT(0)
-/* RK808 has a shadowed register for saving a "frozen" RTC time.
+/* RK8xx has a shadowed register for saving a "frozen" RTC time.
* When user setting "GET_TIME" to 1, the time will save in this shadowed
* register. If set "READSEL" to 1, user read rtc time register, actually
* get the time of that moment. If we need the real time, clr this bit.
@@ -47,17 +66,25 @@
/* REG_SECONDS_REG through REG_YEARS_REG is how many registers? */
-#define NUM_TIME_REGS (RK808_WEEKS_REG - RK808_SECONDS_REG + 1)
-#define NUM_ALARM_REGS (RK808_ALARM_YEARS_REG - RK808_ALARM_SECONDS_REG + 1)
+#define NUM_TIME_REGS (RK8XX_WEEKS_REG - RK8XX_SECONDS_REG + 1)
+#define NUM_ALARM_REGS (RK8XX_ALARM_YEARS_REG - RK8XX_ALARM_SECONDS_REG + 1)
-struct rk808_rtc {
- struct rk808 *rk808;
+static const struct regmap_config rk8xx_rtc_regmap_config = {
+ .reg_bits = 8,
+ .val_bits = 8,
+ .max_register = RK8XX_RTC_COMP_MSB_REG,
+ .cache_type = REGCACHE_RBTREE,
+};
+
+struct rk8xx_rtc {
struct rtc_device *rtc;
+ struct i2c_client *i2c;
+ struct regmap *re
