Messages in this thread Patch in this message | | | Subject | Re: [PATCH 4/4] rtc: max77686: move initialisation of rtc regmap, irq chip locally | From | Javier Martinez Canillas <> | Date | Wed, 3 Feb 2016 00:48:02 -0300 |
| |
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@nvidia.com> >> CC: Krzysztof Mazur <krzysiek@podlesie.net>
I guess you meant Krzysztof Kozlowski <k.kozlowski@samsung.com> here?
Unless I missed some previous discussion and Krzysztof Mazur was involved?
>> CC: Javier Martinez Canillas <javier@osg.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
| |