Message-ID: <56B17872.2050908@osg.samsung.com>
Date:	Wed, 3 Feb 2016 00:48:02 -0300
From:	Javier Martinez Canillas <javier@....samsung.com>
To:	Krzysztof Kozlowski <k.kozlowski@...sung.com>,
	Laxman Dewangan <ldewangan@...dia.com>, lee.jones@...aro.org,
	alexandre.belloni@...e-electrons.com
Cc:	cw00.choi@...sung.com, linux-kernel@...r.kernel.org,
	rtc-linux@...glegroups.com, Krzysztof Mazur <krzysiek@...lesie.net>
Subject: Re: [PATCH 4/4] rtc: max77686: move initialisation of rtc regmap, irq
 chip locally

Hello Laxman,

I'll comment on top of Krzysztof's answer since I agree with his remarks.

On 02/02/2016 10:17 PM, Krzysztof Kozlowski wrote:
> On 02.02.2016 22:16, Laxman Dewangan wrote:
>> To make RTC block of MAX77686/MAX77802 as independent driver,
>> move the registeration of i2c device, regmap for register access
>
> s/registeration/registration/
>
>> and irq_chip for interrupt support inside the RTC driver.
>> Removed the same initialisation from mfd driver.
>
> s/mfd/MFD/
>
> I would expect here also information answering to the "why" question.
> Why are you doing all of this? Why we want RTC driver to be independent?
>
>>
>> Signed-off-by: Laxman Dewangan <ldewangan@...dia.com>
>> CC: Krzysztof Mazur <krzysiek@...lesie.net>

I guess you meant Krzysztof Kozlowski <k.kozlowski@...sung.com> here?

Unless I missed some previous discussion and Krzysztof Mazur was involved?

>> CC: Javier Martinez Canillas <javier@....samsung.com>
>> ---
>>   drivers/mfd/max77686.c               |  82 +------------
>>   drivers/rtc/Kconfig                  |   7 +-
>>   drivers/rtc/rtc-max77686.c           | 225 ++++++++++++++++++++++++++++++++---
>>   include/linux/mfd/max77686-private.h |  16 ---
>>   4 files changed, 211 insertions(+), 119 deletions(-)
>>
>> diff --git a/drivers/mfd/max77686.c b/drivers/mfd/max77686.c
>> index d959ebb..8254201 100644
>> --- a/drivers/mfd/max77686.c
>> +++ b/drivers/mfd/max77686.c
>> @@ -116,11 +116,6 @@ static const struct regmap_config max77686_regmap_config = {
>>   	.val_bits = 8,
>>   };
>>
>> -static const struct regmap_config max77686_rtc_regmap_config = {
>> -	.reg_bits = 8,
>> -	.val_bits = 8,
>> -};
>> -
>>   static const struct regmap_config max77802_regmap_config = {
>>   	.reg_bits = 8,
>>   	.val_bits = 8,
>> @@ -156,25 +151,6 @@ static const struct regmap_irq_chip max77686_irq_chip = {
>>   	.num_irqs		= ARRAY_SIZE(max77686_irqs),
>>   };
>>
>> -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, },
>> -};
>> -
>> -static const struct regmap_irq_chip max77686_rtc_irq_chip = {
>> -	.name			= "max77686-rtc",
>> -	.status_base		= MAX77686_RTC_INT,
>> -	.mask_base		= MAX77686_RTC_INTM,
>> -	.num_regs		= 1,
>> -	.irqs			= max77686_rtc_irqs,
>> -	.num_irqs		= ARRAY_SIZE(max77686_rtc_irqs),
>> -};
>> -
>>   static const struct regmap_irq_chip max77802_irq_chip = {
>>   	.name			= "max77802-pmic",
>>   	.status_base		= MAX77802_REG_INT1,
>> @@ -184,15 +160,6 @@ static const struct regmap_irq_chip max77802_irq_chip = {
>>   	.num_irqs		= ARRAY_SIZE(max77686_irqs),
>>   };
>>
>> -static const struct regmap_irq_chip max77802_rtc_irq_chip = {
>> -	.name			= "max77802-rtc",
>> -	.status_base		= MAX77802_RTC_INT,
>> -	.mask_base		= MAX77802_RTC_INTM,
>> -	.num_regs		= 1,
>> -	.irqs			= max77686_rtc_irqs, /* same masks as 77686 */
>> -	.num_irqs		= ARRAY_SIZE(max77686_rtc_irqs),
>> -};
>> -
>>   static const struct of_device_id max77686_pmic_dt_match[] = {
>>   	{
>>   		.compatible = "maxim,max77686",
>> @@ -214,8 +181,6 @@ static int max77686_i2c_probe(struct i2c_client *i2c,
>>   	int ret = 0;
>>   	const struct regmap_config *config;
>>   	const struct regmap_irq_chip *irq_chip;
>> -	const struct regmap_irq_chip *rtc_irq_chip;
>> -	struct regmap **rtc_regmap;
>>   	const struct mfd_cell *cells;
>>   	int n_devs;
>>
>> @@ -242,15 +207,11 @@ static int max77686_i2c_probe(struct i2c_client *i2c,
>>   	if (max77686->type == TYPE_MAX77686) {
>>   		config = &max77686_regmap_config;
>>   		irq_chip = &max77686_irq_chip;
>> -		rtc_irq_chip = &max77686_rtc_irq_chip;
>> -		rtc_regmap = &max77686->rtc_regmap;
>>   		cells =  max77686_devs;
>>   		n_devs = ARRAY_SIZE(max77686_devs);
>>   	} else {
>>   		config = &max77802_regmap_config;
>>   		irq_chip = &max77802_irq_chip;
>> -		rtc_irq_chip = &max77802_rtc_irq_chip;
>> -		rtc_regmap = &max77686->regmap;
>>   		cells =  max77802_devs;
>>   		n_devs = ARRAY_SIZE(max77802_devs);
>>   	}
>> @@ -270,60 +231,25 @@ static int max77686_i2c_probe(struct i2c_client *i2c,
>>   		return -ENODEV;
>>   	}
>>
>> -	if (max77686->type == TYPE_MAX77686) {
>> -		max77686->rtc = i2c_new_dummy(i2c->adapter, I2C_ADDR_RTC);
>> -		if (!max77686->rtc) {
>> -			dev_err(max77686->dev,
>> -				"Failed to allocate I2C device for RTC\n");
>> -			return -ENODEV;
>> -		}
>> -		i2c_set_clientdata(max77686->rtc, max77686);
>> -
>> -		max77686->rtc_regmap =
>> -			devm_regmap_init_i2c(max77686->rtc,
>> -					     &max77686_rtc_regmap_config);
>> -		if (IS_ERR(max77686->rtc_regmap)) {
>> -			ret = PTR_ERR(max77686->rtc_regmap);
>> -			dev_err(max77686->dev,
>> -				"failed to allocate RTC regmap: %d\n",
>> -				ret);
>> -			goto err_unregister_i2c;
>> -		}
>> -	}
>> -
>>   	ret = regmap_add_irq_chip(max77686->regmap, max77686->irq,
>>   				  IRQF_TRIGGER_FALLING | IRQF_ONESHOT |
>>   				  IRQF_SHARED, 0, irq_chip,
>>   				  &max77686->irq_data);
>>   	if (ret) {
>>   		dev_err(&i2c->dev, "failed to add PMIC irq chip: %d\n", ret);
>> -		goto err_unregister_i2c;
>> -	}
>> -
>> -	ret = regmap_add_irq_chip(*rtc_regmap, max77686->irq,
>> -				  IRQF_TRIGGER_FALLING | IRQF_ONESHOT |
>> -				  IRQF_SHARED, 0, rtc_irq_chip,
>> -				  &max77686->rtc_irq_data);
>> -	if (ret) {
>> -		dev_err(&i2c->dev, "failed to add RTC irq chip: %d\n", ret);
>> -		goto err_del_irqc;
>> +		return -ENODEV;
>
> return ret;
>
>>   	}
>>
>>   	ret = mfd_add_devices(max77686->dev, -1, cells, n_devs, NULL, 0, NULL);
>>   	if (ret < 0) {
>>   		dev_err(&i2c->dev, "failed to add MFD devices: %d\n", ret);
>> -		goto err_del_rtc_irqc;
>> +		goto err_del_irqc;
>>   	}
>>
>>   	return 0;
>>
>> -err_del_rtc_irqc:
>> -	regmap_del_irq_chip(max77686->irq, max77686->rtc_irq_data);
>>   err_del_irqc:
>>   	regmap_del_irq_chip(max77686->irq, max77686->irq_data);
>> -err_unregister_i2c:
>> -	if (max77686->type == TYPE_MAX77686)
>> -		i2c_unregister_device(max77686->rtc);
>>
>>   	return ret;
>>   }
>> @@ -334,12 +260,8 @@ static int max77686_i2c_remove(struct i2c_client *i2c)
>>
>>   	mfd_remove_devices(max77686->dev);
>>
>> -	regmap_del_irq_chip(max77686->irq, max77686->rtc_irq_data);
>>   	regmap_del_irq_chip(max77686->irq, max77686->irq_data);
>>
>> -	if (max77686->type == TYPE_MAX77686)
>> -		i2c_unregister_device(max77686->rtc);
>> -
>>   	return 0;
>>   }
>>
>> diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
>> index ec6a253..bdac624 100644
>> --- a/drivers/rtc/Kconfig
>> +++ b/drivers/rtc/Kconfig
>> @@ -325,14 +325,13 @@ config RTC_DRV_MAX8997
>>   	  will be called rtc-max8997.
>>
>>   config RTC_DRV_MAX77686
>> -	tristate "Maxim MAX77686"
>> -	depends on MFD_MAX77686
>
> It still depends on some parent. Instantiating this without main MFD
> probably would cause errors around max77686_init_rtc_regmap().
>

Indeed, if the RTC device can be registered by different MFD drivers,
then the driver should depend on X || Y

>> +	tristate "Maxim MAX77686/MAX77802"
>>   	help
>>   	  If you say yes here you will get support for the
>> -	  RTC of Maxim MAX77686 PMIC.
>> +	  RTC of Maxim MAX77686/MAX77802 PMIC.
>>
>>   	  This driver can also be built as a module. If so, the module
>> -	  will be called rtc-max77686.
>> +	  will be called rtc-max77686/MAX77802.
>
> The name of module does not change.
>
>>
>>   config RTC_DRV_RK808
>>   	tristate "Rockchip RK808 RTC"
>> diff --git a/drivers/rtc/rtc-max77686.c b/drivers/rtc/rtc-max77686.c
>> index ab1f2cd..21a88e2 100644
>> --- a/drivers/rtc/rtc-max77686.c
>> +++ b/drivers/rtc/rtc-max77686.c
>> @@ -12,16 +12,100 @@
>>    *
>>    */
>>
>> +#include <linux/i2c.h>
>>   #include <linux/slab.h>
>>   #include <linux/rtc.h>
>>   #include <linux/delay.h>
>>   #include <linux/mutex.h>
>>   #include <linux/module.h>
>>   #include <linux/platform_device.h>
>> -#include <linux/mfd/max77686-private.h>
>
> I think the include should remain. No need of duplicating here the
> register addresses. Although we wnat the driver to be independent from
> parent it is still coming from one parent or another.
>
>>   #include <linux/irqdomain.h>
>>   #include <linux/regmap.h>
>>
>> +#define MAX77686_REG_STATUS2		0x07
>> +
>> +enum max77686_rtc_reg {
>> +	MAX77686_RTC_INT		= 0x00,
>> +	MAX77686_RTC_INTM		= 0x01,
>> +	MAX77686_RTC_CONTROLM		= 0x02,
>> +	MAX77686_RTC_CONTROL		= 0x03,
>> +	MAX77686_RTC_UPDATE0		= 0x04,
>> +	/* Reserved: 0x5 */
>> +	MAX77686_WTSR_SMPL_CNTL		= 0x06,
>> +	MAX77686_RTC_SEC		= 0x07,
>> +	MAX77686_RTC_MIN		= 0x08,
>> +	MAX77686_RTC_HOUR		= 0x09,
>> +	MAX77686_RTC_WEEKDAY		= 0x0A,
>> +	MAX77686_RTC_MONTH		= 0x0B,
>> +	MAX77686_RTC_YEAR		= 0x0C,
>> +	MAX77686_RTC_DATE		= 0x0D,
>> +	MAX77686_ALARM1_SEC		= 0x0E,
>> +	MAX77686_ALARM1_MIN		= 0x0F,
>> +	MAX77686_ALARM1_HOUR		= 0x10,
>> +	MAX77686_ALARM1_WEEKDAY		= 0x11,
>> +	MAX77686_ALARM1_MONTH		= 0x12,
>> +	MAX77686_ALARM1_YEAR		= 0x13,
>> +	MAX77686_ALARM1_DATE		= 0x14,
>> +	MAX77686_ALARM2_SEC		= 0x15,
>> +	MAX77686_ALARM2_MIN		= 0x16,
>> +	MAX77686_ALARM2_HOUR		= 0x17,
>> +	MAX77686_ALARM2_WEEKDAY		= 0x18,
>> +	MAX77686_ALARM2_MONTH		= 0x19,
>> +	MAX77686_ALARM2_YEAR		= 0x1A,
>> +	MAX77686_ALARM2_DATE		= 0x1B,
>> +};
>> +
>> +enum max77802_rtc_reg {
>> +	MAX77802_RTC_INT		= 0xC0,
>> +	MAX77802_RTC_INTM		= 0xC1,
>> +	MAX77802_RTC_CONTROLM		= 0xC2,
>> +	MAX77802_RTC_CONTROL		= 0xC3,
>> +	MAX77802_RTC_UPDATE0		= 0xC4,
>> +	MAX77802_RTC_UPDATE1		= 0xC5,
>> +	MAX77802_WTSR_SMPL_CNTL		= 0xC6,
>> +	MAX77802_RTC_SEC		= 0xC7,
>> +	MAX77802_RTC_MIN		= 0xC8,
>> +	MAX77802_RTC_HOUR		= 0xC9,
>> +	MAX77802_RTC_WEEKDAY		= 0xCA,
>> +	MAX77802_RTC_MONTH		= 0xCB,
>> +	MAX77802_RTC_YEAR		= 0xCC,
>> +	MAX77802_RTC_DATE		= 0xCD,
>> +	MAX77802_RTC_AE1		= 0xCE,
>> +	MAX77802_ALARM1_SEC		= 0xCF,
>> +	MAX77802_ALARM1_MIN		= 0xD0,
>> +	MAX77802_ALARM1_HOUR		= 0xD1,
>> +	MAX77802_ALARM1_WEEKDAY		= 0xD2,
>> +	MAX77802_ALARM1_MONTH		= 0xD3,
>> +	MAX77802_ALARM1_YEAR		= 0xD4,
>> +	MAX77802_ALARM1_DATE		= 0xD5,
>> +	MAX77802_RTC_AE2		= 0xD6,
>> +	MAX77802_ALARM2_SEC		= 0xD7,
>> +	MAX77802_ALARM2_MIN		= 0xD8,
>> +	MAX77802_ALARM2_HOUR		= 0xD9,
>> +	MAX77802_ALARM2_WEEKDAY		= 0xDA,
>> +	MAX77802_ALARM2_MONTH		= 0xDB,
>> +	MAX77802_ALARM2_YEAR		= 0xDC,
>> +	MAX77802_ALARM2_DATE		= 0xDD,
>> +
>> +	MAX77802_RTC_END		= 0xDF,
>> +};
>
> All of these should come from existing max77686/max77802/max77620 headers.
>
>> +
>> +enum max77686_rtc_irq {
>> +	MAX77686_RTCIRQ_RTC60S = 0,
>> +	MAX77686_RTCIRQ_RTCA1,
>> +	MAX77686_RTCIRQ_RTCA2,
>> +	MAX77686_RTCIRQ_SMPL,
>> +	MAX77686_RTCIRQ_RTC1S,
>> +	MAX77686_RTCIRQ_WTSR,
>> +};
>
> ditto
>
>> +
>> +#define MAX77686_RTCINT_RTC60S_MSK	BIT(0)
>> +#define MAX77686_RTCINT_RTCA1_MSK	BIT(1)
>> +#define MAX77686_RTCINT_RTCA2_MSK	BIT(2)
>> +#define MAX77686_RTCINT_SMPL_MSK	BIT(3)
>> +#define MAX77686_RTCINT_RTC1S_MSK	BIT(4)
>> +#define MAX77686_RTCINT_WTSR_MSK	BIT(5)
>
> ditto
>
>> +
>>   /* RTC Control Register */
>>   #define BCD_EN_SHIFT			0
>>   #define BCD_EN_MASK			BIT(BCD_EN_SHIFT)
>> @@ -68,8 +152,10 @@ struct max77686_rtc_driver_data {
>>   	const unsigned int	*map;
>>   	/* Has a separate alarm enable register? */
>>   	bool			alarm_enable_reg;
>> -	/* Has a separate I2C regmap for the RTC? */
>> -	bool			separate_i2c_addr;
>> +	/* I2C address for RTC block */
>> +	u8			separate_i2c_addr;

The separate_i2c_addr field name made sense for a bool since it was
answering a question but now that is a u8, I believe that should be
called rtc_i2c_addr or something like that instead.

>> +	/* RTC IRQ CHIP for regmap */
>> +	const struct regmap_irq_chip *rtc_irq_chip;
>>   };
>>
>>   struct max77686_rtc_info {
>> @@ -82,7 +168,9 @@ struct max77686_rtc_info {
>>   	struct regmap		*rtc_regmap;
>>
>>   	const struct max77686_rtc_driver_data *drv_data;
>> +	struct regmap_irq_chip_data *rtc_irq_data;
>>
>> +	int rtc_irq;
>>   	int virq;
>>   	int rtc_24hr_mode;
>>   };
>> @@ -153,12 +241,32 @@ static const unsigned int max77686_map[REG_RTC_END] = {
>>   	[REG_RTC_AE1]	     = REG_RTC_NONE,
>>   };
>>
>> +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, },
>> +};
>> +
>> +static const struct regmap_irq_chip max77686_rtc_irq_chip = {
>> +	.name		= "max77686-rtc",
>> +	.status_base	= MAX77686_RTC_INT,
>> +	.mask_base	= MAX77686_RTC_INTM,
>> +	.num_regs	= 1,
>> +	.irqs		= max77686_rtc_irqs,
>> +	.num_irqs	= ARRAY_SIZE(max77686_rtc_irqs),
>> +};
>> +
>>   static const struct max77686_rtc_driver_data max77686_drv_data = {
>>   	.delay = 16000,
>>   	.mask  = 0x7f,
>>   	.map   = max77686_map,
>>   	.alarm_enable_reg  = false,
>> -	.separate_i2c_addr = true,
>> +	.separate_i2c_addr = 0xC >> 1,
>
> When the header will remain:
> I2C_ADDR_RTC
> (maybe it needs some prefix?)
>
>> +	.rtc_irq_chip = &max77686_rtc_irq_chip,
>>   };
>>
>>   static const unsigned int max77802_map[REG_RTC_END] = {
>> @@ -190,12 +298,22 @@ static const unsigned int max77802_map[REG_RTC_END] = {
>>   	[REG_RTC_AE1]	     = MAX77802_RTC_AE1,
>>   };
>>
>> +static const struct regmap_irq_chip max77802_rtc_irq_chip = {
>> +	.name		= "max77802-rtc",
>> +	.status_base	= MAX77802_RTC_INT,
>> +	.mask_base	= MAX77802_RTC_INTM,
>> +	.num_regs	= 1,
>> +	.irqs		= max77686_rtc_irqs, /* same masks as 77686 */
>> +	.num_irqs	= ARRAY_SIZE(max77686_rtc_irqs),
>> +};
>> +
>>   static const struct max77686_rtc_driver_data max77802_drv_data = {
>>   	.delay = 200,
>>   	.mask  = 0xff,
>>   	.map   = max77802_map,
>>   	.alarm_enable_reg  = true,
>> -	.separate_i2c_addr = false,
>> +	.separate_i2c_addr = 0,

AFAIK 0 is a reserved address for I2C (general call address) so maybe
it would be better to use an invalid address to specify that the RTC
doesn't have a separate I2C address for the registers and check this?

>> +	.rtc_irq_chip = &max77802_rtc_irq_chip,
>>   };
>>
>>   static void max77686_rtc_data_to_tm(u8 *data, struct rtc_time *tm,
>> @@ -599,9 +717,65 @@ static int max77686_rtc_init_reg(struct max77686_rtc_info *info)
>>   	return ret;
>>   }
>>
>> +static const struct regmap_config max77686_rtc_regmap_config = {
>> +	.reg_bits = 8,
>> +	.val_bits = 8,
>> +};
>> +
>> +static int max77686_init_rtc_regmap(struct max77686_rtc_info *info)
>> +{
>> +	struct device *parent = info->dev->parent;
>> +	struct i2c_client *parent_i2c = to_i2c_client(parent);
>> +	int ret;
>> +
>> +	info->rtc_irq =  parent_i2c->irq;
>
> Double space.
>
>> +
>> +	info->regmap = dev_get_regmap(parent, NULL);
>> +	if (!info->regmap) {
>> +		dev_err(info->dev, "Failed to get rtc regmap\n");
>> +		return -ENODEV;
>> +	}
>> +
>> +	if (!info->drv_data->separate_i2c_addr) {
>> +		info->rtc = parent_i2c;
>
> I think it should remain NULL in such case. You won't be using it
> anyway... but in case if you use it then it will be an error anyway
> (like in error path below).
>
>> +		goto add_rtc_irq;
>> +	}
>> +
>> +	info->rtc = i2c_new_dummy(parent_i2c->adapter,
>> +				info->drv_data->separate_i2c_addr);
>
> Please align the argument on new line.
>
>> +	if (!info->rtc) {
>> +		dev_err(info->dev, "Failed to allocate I2C device for RTC\n");
>> +		return -ENODEV;
>> +	}
>> +	i2c_set_clientdata(info->rtc, info);
>
> No need for this. There is no getter. Removal of this could be a
> separate patch (before this one).
>
>> +
>> +	info->rtc_regmap = devm_regmap_init_i2c(info->rtc,
>> +						&max77686_rtc_regmap_config);
>> +	if (IS_ERR(info->rtc_regmap)) {
>> +		ret = PTR_ERR(info->rtc_regmap);
>> +		dev_err(info->dev, "failed to allocate RTC regmap: %d\n", ret);
>
> While moving the messages can you unify the capital letter style (e.g.
> all of error msg starting with capital).
>
>> +		goto err_unregister_i2c;
>> +	}
>> +
>> +add_rtc_irq:
>> +	ret = regmap_add_irq_chip(info->rtc_regmap, info->rtc_irq,
>> +				  IRQF_TRIGGER_FALLING | IRQF_ONESHOT |
>> +				  IRQF_SHARED, 0, info->drv_data->rtc_irq_chip,
>> +				  &info->rtc_irq_data);
>> +	if (ret < 0) {
>> +		dev_err(info->dev, "failed to add RTC irq chip: %d\n", ret);
>> +		goto err_unregister_i2c;
>> +	}
>> +
>> +	return 0;
>> +
>> +err_unregister_i2c:
>> +	i2c_unregister_device(info->rtc);
>
> That would unregister the parent. You are missing "if (separate)".
>
>> +	return ret;
>> +}
>> +
>>   static int max77686_rtc_probe(struct platform_device *pdev)
>>   {
>> -	struct max77686_dev *max77686 = dev_get_drvdata(pdev->dev.parent);
>>   	struct max77686_rtc_info *info;
>>   	const struct platform_device_id *id = platform_get_device_id(pdev);
>>   	int ret;
>> @@ -613,18 +787,16 @@ static int max77686_rtc_probe(struct platform_device *pdev)
>>
>>   	mutex_init(&info->lock);
>>   	info->dev = &pdev->dev;
>> -	info->rtc = max77686->rtc;
>>   	info->drv_data = (const struct max77686_rtc_driver_data *)
>>   		id->driver_data;
>>
>> -	info->regmap = max77686->regmap;
>> -	info->rtc_regmap = (info->drv_data->separate_i2c_addr) ?
>> -			    max77686->rtc_regmap : info->regmap;
>> +	ret = max77686_init_rtc_regmap(info);
>> +	if (ret < 0)
>> +		return ret;
>>
>>   	platform_set_drvdata(pdev, info);
>>
>>   	ret = max77686_rtc_init_reg(info);
>> -
>>   	if (ret < 0) {
>>   		dev_err(&pdev->dev, "Failed to initialize RTC reg:%d\n", ret);
>>   		goto err_rtc;
>> @@ -643,13 +815,7 @@ static int max77686_rtc_probe(struct platform_device *pdev)
>>   		goto err_rtc;
>>   	}
>>
>> -	if (!max77686->rtc_irq_data) {
>> -		ret = -EINVAL;
>> -		dev_err(&pdev->dev, "No RTC regmap IRQ chip\n");
>> -		goto err_rtc;
>> -	}
>> -
>> -	info->virq = regmap_irq_get_virq(max77686->rtc_irq_data,
>> +	info->virq = regmap_irq_get_virq(info->rtc_irq_data,
>>   					 MAX77686_RTCIRQ_RTCA1);
>>   	if (!info->virq) {
>>   		ret = -ENXIO;
>> @@ -659,14 +825,34 @@ static int max77686_rtc_probe(struct platform_device *pdev)
>>   	ret = devm_request_threaded_irq(&pdev->dev, info->virq, NULL,
>>   					max77686_rtc_alarm_irq, 0,
>>   					"rtc-alarm1", info);
>> -	if (ret < 0)
>> +	if (ret < 0) {
>>   		dev_err(&pdev->dev, "Failed to request alarm IRQ: %d: %d\n",
>>   			info->virq, ret);
>> +		goto err_rtc;
>> +	}
>> +
>> +	return 0;
>>
>>   err_rtc:
>> +	if (info->drv_data->separate_i2c_addr)
>> +		i2c_unregister_device(info->rtc);
>> +	regmap_del_irq_chip(info->rtc_irq, info->rtc_irq_data);
>
> Cleanup in reverse-order of allocation. So first del irq_chip then
> unregister i2c.
>
> Best regards,
> Krzysztof
>

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

Powered by blists - more mailing lists