[PATCH 1/2] rtc: max77686: Add support for MAX20024/MAX77620 RTC IP

[PATCH 1/2] rtc: max77686: Add support for MAX20024/MAX77620 RTC IP

[Laxman Dewangan]

Maxim Semiconductor's PMIC MAX77686 has RTC IP which is
reused in the MAX77620/MAX20024 PMICs.

Add support for these devices in MAX77686 RTC driver. This
device does not have RTC alarm pending status outside of
RTC IP. The RTC IP is having separate I2C address for its
register access.

Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
---
 drivers/rtc/Kconfig        |  4 ++--
 drivers/rtc/rtc-max77686.c | 46 +++++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 45 insertions(+), 5 deletions(-)

diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
index 08df14b..1c8dadc 100644
--- a/drivers/rtc/Kconfig
+++ b/drivers/rtc/Kconfig
@@ -326,10 +326,10 @@ config RTC_DRV_MAX8997
 
 config RTC_DRV_MAX77686
 	tristate "Maxim MAX77686"
-	depends on MFD_MAX77686
+	depends on MFD_MAX77686 || MFD_MAX77620
 	help
 	  If you say yes here you will get support for the
-	  RTC of Maxim MAX77686 PMIC.
+	  RTC of Maxim MAX77686/MAX77620/MAX77802 PMIC.
 
 	  This driver can also be built as a module. If so, the module
 	  will be called rtc-max77686.
diff --git a/drivers/rtc/rtc-max77686.c b/drivers/rtc/rtc-max77686.c
index 5e924f3..39d529a 100644
--- a/drivers/rtc/rtc-max77686.c
+++ b/drivers/rtc/rtc-max77686.c
@@ -24,8 +24,15 @@
 #include <linux/regmap.h>
 
 #define MAX77686_I2C_ADDR_RTC		(0x0C >> 1)
+#define MAX77620_I2C_ADDR_RTC		0x68
 #define MAX77686_INVALID_I2C_ADDR	(-1)
 
+/* Alarm pending register */
+#define MAX77696_INVALID_REG		(-1)
+#define MAX77686_RTC_ALARM_PENDING_STATUS_REG	MAX77686_REG_STATUS2
+#define MAX77802_RTC_ALARM_PENDING_STATUS_REG	MAX77686_REG_STATUS2
+#define MAX77620_RTC_ALARM_PENDING_STATUS_REG	MAX77696_INVALID_REG
+
 /* RTC Control Register */
 #define BCD_EN_SHIFT			0
 #define BCD_EN_MASK			BIT(BCD_EN_SHIFT)
@@ -74,6 +81,10 @@ struct max77686_rtc_driver_data {
 	bool			alarm_enable_reg;
 	/* I2C address for RTC block */
 	int			rtc_i2c_addr;
+	/* RTC interrupt via platform resource */
+	bool rtc_irq_from_platform;
+	/* Pending alarm status register */
+	int alarm_pending_status_reg;
 	/* RTC IRQ CHIP for regmap */
 	const struct regmap_irq_chip *rtc_irq_chip;
 };
@@ -185,10 +196,23 @@ static const struct max77686_rtc_driver_data max77686_drv_data = {
 	.mask  = 0x7f,
 	.map   = max77686_map,
 	.alarm_enable_reg  = false,
+	.rtc_irq_from_platform = false,
+	.alarm_pending_status_reg = MAX77686_RTC_ALARM_PENDING_STATUS_REG,
 	.rtc_i2c_addr = MAX77686_I2C_ADDR_RTC,
 	.rtc_irq_chip = &max77686_rtc_irq_chip,
 };
 
+static const struct max77686_rtc_driver_data max77620_drv_data = {
+	.delay = 16000,
+	.mask  = 0x7f,
+	.map   = max77686_map,
+	.alarm_enable_reg  = false,
+	.rtc_irq_from_platform = true,
+	.alarm_pending_status_reg = MAX77620_RTC_ALARM_PENDING_STATUS_REG,
+	.rtc_i2c_addr = MAX77620_I2C_ADDR_RTC,
+	.rtc_irq_chip = &max77686_rtc_irq_chip,
+};
+
 static const unsigned int max77802_map[REG_RTC_END] = {
 	[REG_RTC_CONTROLM]   = MAX77802_RTC_CONTROLM,
 	[REG_RTC_CONTROL]    = MAX77802_RTC_CONTROL,
@@ -232,6 +256,8 @@ static const struct max77686_rtc_driver_data max77802_drv_data = {
 	.mask  = 0xff,
 	.map   = max77802_map,
 	.alarm_enable_reg  = true,
+	.rtc_irq_from_platform = false,
+	.alarm_pending_status_reg = MAX77802_RTC_ALARM_PENDING_STATUS_REG,
 	.rtc_i2c_addr = MAX77686_INVALID_I2C_ADDR,
 	.rtc_irq_chip = &max77802_rtc_irq_chip,
 };
@@ -427,9 +453,15 @@ static int max77686_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm)
 	}
 
 	alrm->pending = 0;
-	ret = regmap_read(info->regmap, MAX77686_REG_STATUS2, &val);
+
+	if (info->drv_data->alarm_pending_status_reg == MAX77696_INVALID_REG)
+		goto out;
+
+	ret = regmap_read(info->regmap,
+			  info->drv_data->alarm_pending_status_reg, &val);
 	if (ret < 0) {
-		dev_err(info->dev, "Fail to read status2 reg(%d)\n", ret);
+		dev_err(info->dev,
+			"Fail to read alarm pending status reg(%d)\n", ret);
 		goto out;
 	}
 
@@ -648,7 +680,13 @@ static int max77686_init_rtc_regmap(struct max77686_rtc_info *info)
 	struct i2c_client *parent_i2c = to_i2c_client(parent);
 	int ret;
 
-	info->rtc_irq = parent_i2c->irq;
+	if (info->drv_data->rtc_irq_from_platform) {
+		struct platform_device *pdev = to_platform_device(info->dev);
+
+		info->rtc_irq = platform_get_irq(pdev, 0);
+	} else {
+		info->rtc_irq =  parent_i2c->irq;
+	}
 
 	info->regmap = dev_get_regmap(parent, NULL);
 	if (!info->regmap) {
@@ -802,6 +840,8 @@ static SIMPLE_DEV_PM_OPS(max77686_rtc_pm_ops,
 static const struct platform_device_id rtc_id[] = {
 	{ "max77686-rtc", .driver_data = (kernel_ulong_t)&max77686_drv_data, },
 	{ "max77802-rtc", .driver_data = (kernel_ulong_t)&max77802_drv_data, },
+	{ "max77620-rtc", .driver_data = (kernel_ulong_t)&max77620_drv_data, },
+	{ "max20024-rtc", .driver_data = (kernel_ulong_t)&max77620_drv_data, },
 	{},
 };
 MODULE_DEVICE_TABLE(platform, rtc_id);
-- 
2.1.4

[PATCH 2/2] rtc: max77686: Use REGMAP_IRQ_REG for regmap-rtc-irqs initialisation

[Laxman Dewangan]

use macro REGMAP_IRQ_REG from regmap.h to initialise the
regmap irq table for max77686.

Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
---
 drivers/rtc/rtc-max77686.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/rtc/rtc-max77686.c b/drivers/rtc/rtc-max77686.c
index 39d529a..0cb422b 100644
--- a/drivers/rtc/rtc-max77686.c
+++ b/drivers/rtc/rtc-max77686.c
@@ -174,12 +174,12 @@ static const unsigned int max77686_map[REG_RTC_END] = {
 
 static const struct regmap_irq max77686_rtc_irqs[] = {
 	/* RTC interrupts */
-	{ .reg_offset = 0, .mask = MAX77686_RTCINT_RTC60S_MSK, },
-	{ .reg_offset = 0, .mask = MAX77686_RTCINT_RTCA1_MSK, },
-	{ .reg_offset = 0, .mask = MAX77686_RTCINT_RTCA2_MSK, },
-	{ .reg_offset = 0, .mask = MAX77686_RTCINT_SMPL_MSK, },
-	{ .reg_offset = 0, .mask = MAX77686_RTCINT_RTC1S_MSK, },
-	{ .reg_offset = 0, .mask = MAX77686_RTCINT_WTSR_MSK, },
+	REGMAP_IRQ_REG(0, 0, MAX77686_RTCINT_RTC60S_MSK),
+	REGMAP_IRQ_REG(1, 0, MAX77686_RTCINT_RTCA1_MSK),
+	REGMAP_IRQ_REG(2, 0, MAX77686_RTCINT_RTCA2_MSK),
+	REGMAP_IRQ_REG(3, 0, MAX77686_RTCINT_SMPL_MSK),
+	REGMAP_IRQ_REG(4, 0, MAX77686_RTCINT_RTC1S_MSK),
+	REGMAP_IRQ_REG(5, 0, MAX77686_RTCINT_WTSR_MSK),
 };
 
 static const struct regmap_irq_chip max77686_rtc_irq_chip = {
-- 
2.1.4

Re: [PATCH 2/2] rtc: max77686: Use REGMAP_IRQ_REG for regmap-rtc-irqs initialisation

[Krzysztof Kozlowski]

On 29.02.2016 21:58, Laxman Dewangan wrote:
> use macro REGMAP_IRQ_REG from regmap.h to initialise the
> regmap irq table for max77686.

Why? Any benefits?

Best regards,
Krzysztof

> 
> Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
> ---
>  drivers/rtc/rtc-max77686.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/rtc/rtc-max77686.c b/drivers/rtc/rtc-max77686.c
> index 39d529a..0cb422b 100644
> --- a/drivers/rtc/rtc-max77686.c
> +++ b/drivers/rtc/rtc-max77686.c
> @@ -174,12 +174,12 @@ static const unsigned int max77686_map[REG_RTC_END] = {
>  
>  static const struct regmap_irq max77686_rtc_irqs[] = {
>  	/* RTC interrupts */
> -	{ .reg_offset = 0, .mask = MAX77686_RTCINT_RTC60S_MSK, },
> -	{ .reg_offset = 0, .mask = MAX77686_RTCINT_RTCA1_MSK, },
> -	{ .reg_offset = 0, .mask = MAX77686_RTCINT_RTCA2_MSK, },
> -	{ .reg_offset = 0, .mask = MAX77686_RTCINT_SMPL_MSK, },
> -	{ .reg_offset = 0, .mask = MAX77686_RTCINT_RTC1S_MSK, },
> -	{ .reg_offset = 0, .mask = MAX77686_RTCINT_WTSR_MSK, },
> +	REGMAP_IRQ_REG(0, 0, MAX77686_RTCINT_RTC60S_MSK),
> +	REGMAP_IRQ_REG(1, 0, MAX77686_RTCINT_RTCA1_MSK),
> +	REGMAP_IRQ_REG(2, 0, MAX77686_RTCINT_RTCA2_MSK),
> +	REGMAP_IRQ_REG(3, 0, MAX77686_RTCINT_SMPL_MSK),
> +	REGMAP_IRQ_REG(4, 0, MAX77686_RTCINT_RTC1S_MSK),
> +	REGMAP_IRQ_REG(5, 0, MAX77686_RTCINT_WTSR_MSK),
>  };
>  
>  static const struct regmap_irq_chip max77686_rtc_irq_chip = {
> 

Re: [PATCH 2/2] rtc: max77686: Use REGMAP_IRQ_REG for regmap-rtc-irqs initialisation

[Laxman Dewangan]

On Wednesday 02 March 2016 06:30 AM, Krzysztof Kozlowski wrote:
> On 29.02.2016 21:58, Laxman Dewangan wrote:
>> use macro REGMAP_IRQ_REG from regmap.h to initialise the
>> regmap irq table for max77686.
> Why? Any benefits?
>
>
I think it is better coding to use macro for member initializations 
instead of doing it locally and it improves readability.

I got the similar comment from Lee's in my MFD patches to use macro for 
such initialization and I agreed with him.

As I was here on this driver, thought to use macro.

Re: [PATCH 2/2] rtc: max77686: Use REGMAP_IRQ_REG for regmap-rtc-irqs initialisation

[Krzysztof Kozlowski]

On 02.03.2016 11:04, Laxman Dewangan wrote:
> 
> On Wednesday 02 March 2016 06:30 AM, Krzysztof Kozlowski wrote:
>> On 29.02.2016 21:58, Laxman Dewangan wrote:
>>> use macro REGMAP_IRQ_REG from regmap.h to initialise the
>>> regmap irq table for max77686.
>> Why? Any benefits?
>>
>>
> I think it is better coding to use macro for member initializations
> instead of doing it locally and it improves readability.
> 
> I got the similar comment from Lee's in my MFD patches to use macro for
> such initialization and I agreed with him.
> 
> As I was here on this driver, thought to use macro.

Sounds good, so this should be written in commit message.

BR,
Krzysztof

Re: [PATCH 1/2] rtc: max77686: Add support for MAX20024/MAX77620 RTC IP

[Krzysztof Kozlowski]

On 29.02.2016 21:58, Laxman Dewangan wrote:
> Maxim Semiconductor's PMIC MAX77686 has RTC IP which is
> reused in the MAX77620/MAX20024 PMICs.
> 
> Add support for these devices in MAX77686 RTC driver. This
> device does not have RTC alarm pending status outside of
> RTC IP. The RTC IP is having separate I2C address for its
> register access.
> 
> Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
> ---
>  drivers/rtc/Kconfig        |  4 ++--
>  drivers/rtc/rtc-max77686.c | 46 +++++++++++++++++++++++++++++++++++++++++++---
>  2 files changed, 45 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
> index 08df14b..1c8dadc 100644
> --- a/drivers/rtc/Kconfig
> +++ b/drivers/rtc/Kconfig
> @@ -326,10 +326,10 @@ config RTC_DRV_MAX8997
>  
>  config RTC_DRV_MAX77686
>  	tristate "Maxim MAX77686"
> -	depends on MFD_MAX77686
> +	depends on MFD_MAX77686 || MFD_MAX77620
>  	help
>  	  If you say yes here you will get support for the
> -	  RTC of Maxim MAX77686 PMIC.
> +	  RTC of Maxim MAX77686/MAX77620/MAX77802 PMIC.
>  
>  	  This driver can also be built as a module. If so, the module
>  	  will be called rtc-max77686.
> diff --git a/drivers/rtc/rtc-max77686.c b/drivers/rtc/rtc-max77686.c
> index 5e924f3..39d529a 100644
> --- a/drivers/rtc/rtc-max77686.c
> +++ b/drivers/rtc/rtc-max77686.c
> @@ -24,8 +24,15 @@
>  #include <linux/regmap.h>
>  
>  #define MAX77686_I2C_ADDR_RTC		(0x0C >> 1)
> +#define MAX77620_I2C_ADDR_RTC		0x68
>  #define MAX77686_INVALID_I2C_ADDR	(-1)
>  
> +/* Alarm pending register */
> +#define MAX77696_INVALID_REG		(-1)
MAX77686
...but actually why not using just 0x0 (remove it completely)? See
comment later.

> +#define MAX77686_RTC_ALARM_PENDING_STATUS_REG	MAX77686_REG_STATUS2
> +#define MAX77802_RTC_ALARM_PENDING_STATUS_REG	MAX77686_REG_STATUS2
> +#define MAX77620_RTC_ALARM_PENDING_STATUS_REG	MAX77696_INVALID_REG

These defines look useless. Just use directly the register.

> +
>  /* RTC Control Register */
>  #define BCD_EN_SHIFT			0
>  #define BCD_EN_MASK			BIT(BCD_EN_SHIFT)
> @@ -74,6 +81,10 @@ struct max77686_rtc_driver_data {
>  	bool			alarm_enable_reg;
>  	/* I2C address for RTC block */
>  	int			rtc_i2c_addr;
> +	/* RTC interrupt via platform resource */
> +	bool rtc_irq_from_platform;

Make indentation consistent.

> +	/* Pending alarm status register */
> +	int alarm_pending_status_reg;

ditto

>  	/* RTC IRQ CHIP for regmap */
>  	const struct regmap_irq_chip *rtc_irq_chip;
>  };
> @@ -185,10 +196,23 @@ static const struct max77686_rtc_driver_data max77686_drv_data = {
>  	.mask  = 0x7f,
>  	.map   = max77686_map,
>  	.alarm_enable_reg  = false,
> +	.rtc_irq_from_platform = false,
> +	.alarm_pending_status_reg = MAX77686_RTC_ALARM_PENDING_STATUS_REG,

Just:
.alarm_pending_status_reg = MAX77686_REG_STATUS2

>  	.rtc_i2c_addr = MAX77686_I2C_ADDR_RTC,
>  	.rtc_irq_chip = &max77686_rtc_irq_chip,
>  };
>  
> +static const struct max77686_rtc_driver_data max77620_drv_data = {
> +	.delay = 16000,
> +	.mask  = 0x7f,
> +	.map   = max77686_map,
> +	.alarm_enable_reg  = false,
> +	.rtc_irq_from_platform = true,
> +	.alarm_pending_status_reg = MAX77620_RTC_ALARM_PENDING_STATUS_REG,

Just skip the alarm_pending_status_reg (so it will be 0x0) and check for
non-zero value later?

It might be a little bit non consistent approach to how we map RTC
registers (REG_RTC_NONE)... so I don't have strong feelings about this.


> +	.rtc_i2c_addr = MAX77620_I2C_ADDR_RTC,
> +	.rtc_irq_chip = &max77686_rtc_irq_chip,
> +};
> +
>  static const unsigned int max77802_map[REG_RTC_END] = {
>  	[REG_RTC_CONTROLM]   = MAX77802_RTC_CONTROLM,
>  	[REG_RTC_CONTROL]    = MAX77802_RTC_CONTROL,
> @@ -232,6 +256,8 @@ static const struct max77686_rtc_driver_data max77802_drv_data = {
>  	.mask  = 0xff,
>  	.map   = max77802_map,
>  	.alarm_enable_reg  = true,
> +	.rtc_irq_from_platform = false,
> +	.alarm_pending_status_reg = MAX77802_RTC_ALARM_PENDING_STATUS_REG,
>  	.rtc_i2c_addr = MAX77686_INVALID_I2C_ADDR,
>  	.rtc_irq_chip = &max77802_rtc_irq_chip,
>  };
> @@ -427,9 +453,15 @@ static int max77686_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm)
>  	}
>  
>  	alrm->pending = 0;
> -	ret = regmap_read(info->regmap, MAX77686_REG_STATUS2, &val);
> +
> +	if (info->drv_data->alarm_pending_status_reg == MAX77696_INVALID_REG)
> +		goto out;
> +
> +	ret = regmap_read(info->regmap,
> +			  info->drv_data->alarm_pending_status_reg, &val);
>  	if (ret < 0) {
> -		dev_err(info->dev, "Fail to read status2 reg(%d)\n", ret);
> +		dev_err(info->dev,
> +			"Fail to read alarm pending status reg(%d)\n", ret);
>  		goto out;
>  	}
>  
> @@ -648,7 +680,13 @@ static int max77686_init_rtc_regmap(struct max77686_rtc_info *info)
>  	struct i2c_client *parent_i2c = to_i2c_client(parent);
>  	int ret;
>  
> -	info->rtc_irq = parent_i2c->irq;
> +	if (info->drv_data->rtc_irq_from_platform) {
> +		struct platform_device *pdev = to_platform_device(info->dev);
> +
> +		info->rtc_irq = platform_get_irq(pdev, 0);

It may return -ERRNO. What happens then?

> +	} else {
> +		info->rtc_irq =  parent_i2c->irq;
> +	}
>  
>  	info->regmap = dev_get_regmap(parent, NULL);
>  	if (!info->regmap) {
> @@ -802,6 +840,8 @@ static SIMPLE_DEV_PM_OPS(max77686_rtc_pm_ops,
>  static const struct platform_device_id rtc_id[] = {
>  	{ "max77686-rtc", .driver_data = (kernel_ulong_t)&max77686_drv_data, },
>  	{ "max77802-rtc", .driver_data = (kernel_ulong_t)&max77802_drv_data, },
> +	{ "max77620-rtc", .driver_data = (kernel_ulong_t)&max77620_drv_data, },
> +	{ "max20024-rtc", .driver_data = (kernel_ulong_t)&max77620_drv_data, },

There shouldn't be "max20024-rtc". This is exactly the same as
"max77620-rtc" so re-use existing id. No point of duplicating device
names for 100% compatible devices.

Best regards,
Krzysztof

Re: [PATCH 1/2] rtc: max77686: Add support for MAX20024/MAX77620 RTC IP

[Laxman Dewangan]

On Wednesday 02 March 2016 06:28 AM, Krzysztof Kozlowski wrote:
> On 29.02.2016 21:58, Laxman Dewangan wrote:
> +	.alarm_pending_status_reg = MAX77620_RTC_ALARM_PENDING_STATUS_REG,
> Just skip the alarm_pending_status_reg (so it will be 0x0) and check for
> non-zero value later?
>
> It might be a little bit non consistent approach to how we map RTC
> registers (REG_RTC_NONE)... so I don't have strong feelings about this.

I choose -1 because 0 is also valid.
So I can have macro for INVALID register which is -1 and use here, other 
places direct register as STATUS2.


>
>> +	if (info->drv_data->rtc_irq_from_platform) {
>> +		struct platform_device *pdev = to_platform_device(info->dev);
>> +
>> +		info->rtc_irq = platform_get_irq(pdev, 0);
> It may return -ERRNO. What happens then?

MFD is initializing the irq and so it will not fail on this particular case.
Even if error, the regmap_add_irq should fail.

Let me handle error at this point only to avoid any assumption and 
further processing with error, by returning error.


>
>> +	} else {
>> +		info->rtc_irq =  parent_i2c->irq;
>> +	}
>>   
>>   	info->regmap = dev_get_regmap(parent, NULL);
>>   	if (!info->regmap) {
>> @@ -802,6 +840,8 @@ static SIMPLE_DEV_PM_OPS(max77686_rtc_pm_ops,
>>   static const struct platform_device_id rtc_id[] = {
>>   	{ "max77686-rtc", .driver_data = (kernel_ulong_t)&max77686_drv_data, },
>>   	{ "max77802-rtc", .driver_data = (kernel_ulong_t)&max77802_drv_data, },
>> +	{ "max77620-rtc", .driver_data = (kernel_ulong_t)&max77620_drv_data, },
>> +	{ "max20024-rtc", .driver_data = (kernel_ulong_t)&max77620_drv_data, },
> There shouldn't be "max20024-rtc". This is exactly the same as
> "max77620-rtc" so re-use existing id. No point of duplicating device
> names for 100% compatible devices.
>
>
I am thinking that having compatible for each device which it supports 
is better.

In MFD, I have made all sub module of max20024 as max20024-<module>.
I have not mixed the sub module name for max20024 with max77620 module.

Re: [PATCH 1/2] rtc: max77686: Add support for MAX20024/MAX77620 RTC IP

[Krzysztof Kozlowski]

On 02.03.2016 11:15, Laxman Dewangan wrote:
> 
> On Wednesday 02 March 2016 06:28 AM, Krzysztof Kozlowski wrote:
>> On 29.02.2016 21:58, Laxman Dewangan wrote:
>> +    .alarm_pending_status_reg = MAX77620_RTC_ALARM_PENDING_STATUS_REG,
>> Just skip the alarm_pending_status_reg (so it will be 0x0) and check for
>> non-zero value later?
>>
>> It might be a little bit non consistent approach to how we map RTC
>> registers (REG_RTC_NONE)... so I don't have strong feelings about this.
> 
> I choose -1 because 0 is also valid.
> So I can have macro for INVALID register which is -1 and use here, other
> places direct register as STATUS2.

There is only one value used here so 0 not valid. But I don't mind that
approach.

> 
> 
>>
>>> +    if (info->drv_data->rtc_irq_from_platform) {
>>> +        struct platform_device *pdev = to_platform_device(info->dev);
>>> +
>>> +        info->rtc_irq = platform_get_irq(pdev, 0);
>> It may return -ERRNO. What happens then?
> 
> MFD is initializing the irq and so it will not fail on this particular
> case.
> Even if error, the regmap_add_irq should fail.
> 
> Let me handle error at this point only to avoid any assumption and
> further processing with error, by returning error.
> 
> 
>>
>>> +    } else {
>>> +        info->rtc_irq =  parent_i2c->irq;
>>> +    }
>>>         info->regmap = dev_get_regmap(parent, NULL);
>>>       if (!info->regmap) {
>>> @@ -802,6 +840,8 @@ static SIMPLE_DEV_PM_OPS(max77686_rtc_pm_ops,
>>>   static const struct platform_device_id rtc_id[] = {
>>>       { "max77686-rtc", .driver_data =
>>> (kernel_ulong_t)&max77686_drv_data, },
>>>       { "max77802-rtc", .driver_data =
>>> (kernel_ulong_t)&max77802_drv_data, },
>>> +    { "max77620-rtc", .driver_data =
>>> (kernel_ulong_t)&max77620_drv_data, },
>>> +    { "max20024-rtc", .driver_data =
>>> (kernel_ulong_t)&max77620_drv_data, },
>> There shouldn't be "max20024-rtc". This is exactly the same as
>> "max77620-rtc" so re-use existing id. No point of duplicating device
>> names for 100% compatible devices.
>>
>>
> I am thinking that having compatible for each device which it supports
> is better.
> 
> In MFD, I have made all sub module of max20024 as max20024-<module>.
> I have not mixed the sub module name for max20024 with max77620 module.

The point of compatible is to be... compatible so you don't create
compatibles for the same meaning!

However this is actually not a compatible but a matching name... which
should follow the same idea. You did not give any argument why this is
better.

For me, code like this:
{ "max77802-rtc", .driver_data = (kernel_ulong_t)&max77802_drv_data, },
{ "max77620-rtc", .driver_data = (kernel_ulong_t)&max77620_drv_data, },
{ "max77621-rtc", .driver_data = (kernel_ulong_t)&max77620_drv_data, },
{ "max77622-rtc", .driver_data = (kernel_ulong_t)&max77620_drv_data, },
{ "max77623-rtc", .driver_data = (kernel_ulong_t)&max77620_drv_data, },
{ "max776xx-some-other-rtc", .driver_data =
(kernel_ulong_t)&max77620_drv_data, },
{ "max77624-rtc", .driver_data = (kernel_ulong_t)&max77620_drv_data, },

is useless, ridiculous and obfuscated. It is duplication of code just
"because". The child driver is selected by matching mfd-cell or
compatible. We are reusing child drivers so reuse under the same name.

Best regards,
Krzysztof

Re: [PATCH 1/2] rtc: max77686: Add support for MAX20024/MAX77620 RTC IP

[Laxman Dewangan]

On Wednesday 02 March 2016 09:22 AM, Krzysztof Kozlowski wrote:
> On 02.03.2016 11:15, Laxman Dewangan wrote:
>
>>>> (kernel_ulong_t)&max77802_drv_data, },
>>>> +    { "max77620-rtc", .driver_data =
>>>> (kernel_ulong_t)&max77620_drv_data, },
>>>> +    { "max20024-rtc", .driver_data =
>>>> (kernel_ulong_t)&max77620_drv_data, },
>>> There shouldn't be "max20024-rtc". This is exactly the same as
>>> "max77620-rtc" so re-use existing id. No point of duplicating device
>>> names for 100% compatible devices.
>>>
>>>
>> I am thinking that having compatible for each device which it supports
>> is better.
>>
>> In MFD, I have made all sub module of max20024 as max20024-<module>.
>> I have not mixed the sub module name for max20024 with max77620 module.
> The point of compatible is to be... compatible so you don't create
> compatibles for the same meaning!
>
> However this is actually not a compatible but a matching name... which
> should follow the same idea. You did not give any argument why this is
> better.

My point is that if any driver supporting the any devices then it should 
be there in their compatibility although other everything is same.

This way, it is easy to find that the driver is available for the device 
or not. Also easy way to tell that someone has invested time to find out 
the driver corresponding to device and he confirmed that this driver is 
compatible with that device.
Otherwise, it is difficult to quickly find out the driver whether this 
is available/support or not for given device.

Datasheet never says that this device is same as some other device and 
hence this is the only place to tell for SW guys.



> For me, code like this:
> { "max77802-rtc", .driver_data = (kernel_ulong_t)&max77802_drv_data, },
> { "max77620-rtc", .driver_data = (kernel_ulong_t)&max77620_drv_data, },
> { "max77621-rtc", .driver_data = (kernel_ulong_t)&max77620_drv_data, },
> { "max77622-rtc", .driver_data = (kernel_ulong_t)&max77620_drv_data, },
> { "max77623-rtc", .driver_data = (kernel_ulong_t)&max77620_drv_data, },
> { "max776xx-some-other-rtc", .driver_data =
> (kernel_ulong_t)&max77620_drv_data, },
> { "max77624-rtc", .driver_data = (kernel_ulong_t)&max77620_drv_data, },
>
> is useless, ridiculous and obfuscated. It is duplication of code just
> "because". The child driver is selected by matching mfd-cell or
> compatible. We are reusing child drivers so reuse under the same name.
>
> Best regards,
> Krzysztof
>
>
>

Re: [PATCH 1/2] rtc: max77686: Add support for MAX20024/MAX77620 RTC IP

[Krzysztof Kozlowski]

On 02.03.2016 13:10, Laxman Dewangan wrote:
> 
> On Wednesday 02 March 2016 09:22 AM, Krzysztof Kozlowski wrote:
>> On 02.03.2016 11:15, Laxman Dewangan wrote:
>>
>>>>> (kernel_ulong_t)&max77802_drv_data, },
>>>>> +    { "max77620-rtc", .driver_data =
>>>>> (kernel_ulong_t)&max77620_drv_data, },
>>>>> +    { "max20024-rtc", .driver_data =
>>>>> (kernel_ulong_t)&max77620_drv_data, },
>>>> There shouldn't be "max20024-rtc". This is exactly the same as
>>>> "max77620-rtc" so re-use existing id. No point of duplicating device
>>>> names for 100% compatible devices.
>>>>
>>>>
>>> I am thinking that having compatible for each device which it supports
>>> is better.
>>>
>>> In MFD, I have made all sub module of max20024 as max20024-<module>.
>>> I have not mixed the sub module name for max20024 with max77620 module.
>> The point of compatible is to be... compatible so you don't create
>> compatibles for the same meaning!
>>
>> However this is actually not a compatible but a matching name... which
>> should follow the same idea. You did not give any argument why this is
>> better.
> 
> My point is that if any driver supporting the any devices then it should
> be there in their compatibility although other everything is same.

Nope. The driver can describe supported devices in comment, Kconfig,
module description, DT binding description but the compatible is one.
One compatible for all compatible devices.

> This way, it is easy to find that the driver is available for the device
> or not. Also easy way to tell that someone has invested time to find out
> the driver corresponding to device and he confirmed that this driver is
> compatible with that device.
> Otherwise, it is difficult to quickly find out the driver whether this
> is available/support or not for given device.

This is so specific, imaginated use case... Regular users don't write
DTS. This is strictly for developers and the engineer who develops
code/platforms using maxim devices has this problem? No way...

You want to create many artificial device ids for the same compatible
device just to make grepping easier for someone (I cannot even find out
for whom...)? Nooo, that is just wrong. These are the same
blocks/subdevices. They use the same driver. They should use the same
compatible or the same name of driver.

Best regards,
Krzysztof

Re: [PATCH 1/2] rtc: max77686: Add support for MAX20024/MAX77620 RTC IP

[Laxman Dewangan]

On Wednesday 02 March 2016 09:58 AM, Krzysztof Kozlowski wrote:
> On 02.03.2016 13:10, Laxman Dewangan wrote:
>> On Wednesday 02 March 2016 09:22 AM, Krzysztof Kozlowski wrote:
>>> On 02.03.2016 11:15, Laxman Dewangan wrote:
>>>
>>>>>> (kernel_ulong_t)&max77802_drv_data, },
>>>>>> +    { "max77620-rtc", .driver_data =
>>>>>> (kernel_ulong_t)&max77620_drv_data, },
>>>>>> +    { "max20024-rtc", .driver_data =
>>>>>> (kernel_ulong_t)&max77620_drv_data, },
>>>>> There shouldn't be "max20024-rtc". This is exactly the same as
>>>>> "max77620-rtc" so re-use existing id. No point of duplicating device
>>>>> names for 100% compatible devices.
>>>>>
>>>>>
>>>> I am thinking that having compatible for each device which it supports
>>>> is better.
>>>>
>>>> In MFD, I have made all sub module of max20024 as max20024-<module>.
>>>> I have not mixed the sub module name for max20024 with max77620 module.
>>> The point of compatible is to be... compatible so you don't create
>>> compatibles for the same meaning!
>>>
>>> However this is actually not a compatible but a matching name... which
>>> should follow the same idea. You did not give any argument why this is
>>> better.
>> My point is that if any driver supporting the any devices then it should
>> be there in their compatibility although other everything is same.
> Nope. The driver can describe supported devices in comment, Kconfig,
> module description, DT binding description but the compatible is one.
> One compatible for all compatible devices.

OK, got it.

>
>> This way, it is easy to find that the driver is available for the device
>> or not. Also easy way to tell that someone has invested time to find out
>> the driver corresponding to device and he confirmed that this driver is
>> compatible with that device.
>> Otherwise, it is difficult to quickly find out the driver whether this
>> is available/support or not for given device.
> This is so specific, imaginated use case... Regular users don't write
> DTS. This is strictly for developers and the engineer who develops
> code/platforms using maxim devices has this problem? No way...

It is generic and not very specific to Maxim.
As you suggested above, we can mention the same information in other places.


Will respin the patch to remove this extra compatibility.