[5/6] rtc: max77620: add support for max77620/max20024 RTC driver

Maxim Semiconductor's PMIC MAX77620/MAX20024 has on chip
RTC  module. This support for setting alarm and time.

Add RTC driver to access this chip's RTC module via RTC
APIs.

Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
Signed-off-by: Chaitanya Bandi <bandik@nvidia.com>
Tested-by: Venkat Reddy Talla <vreddytalla@nvidia.com>
---
 drivers/rtc/Kconfig        |   9 +
 drivers/rtc/Makefile       |   1 +
 drivers/rtc/rtc-max77620.c | 574 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 584 insertions(+)
 create mode 100644 drivers/rtc/rtc-max77620.c

Hi,

On Thursday 07 January 2016 08:08 PM, Laxman Dewangan wrote:

> Maxim Semiconductor's PMIC MAX77620/MAX20024 has on chip
> RTC  module. This support for setting alarm and time.
>
> Add RTC driver to access this chip's RTC module via RTC
> APIs.
>
> Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
> Signed-off-by: Chaitanya Bandi <bandik@nvidia.com>
> Tested-by: Venkat Reddy Talla <vreddytalla@nvidia.com>
> ---
>   drivers/rtc/Kconfig        |   9 +
>   drivers/rtc/Makefile       |   1 +
>   drivers/rtc/rtc-max77620.c | 574 +++++++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 584 insertions(+)
>   create mode 100644 drivers/rtc/rtc-max77620.c
>
(...)

> +static struct platform_driver max77620_rtc_driver = {
> +	.probe = max77620_rtc_probe,
> +	.remove = max77620_rtc_remove,
> +	.id_table = max77620_rtc_devtype,
> +	.driver = {
> +			.name = "max77620-rtc",
> +			.owner = THIS_MODULE,

Drop this line, .owner will be populated by driver core.

> +			.pm = &max77620_rtc_pm_ops,
> +	},
> +};
> +
> +module_platform_driver(max77620_rtc_driver);
> +
> +MODULE_DESCRIPTION("max77620 RTC driver");
> +MODULE_AUTHOR("Chaitanya Bandi <bandik@nvidia.com>");
> +MODULE_AUTHOR("Laxman Dewangan <ldewangan@nvidia.com>");
> +MODULE_ALIAS("platform:max77620-rtc");
> +MODULE_LICENSE("GPL v2");
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

2016-01-07 23:38 GMT+09:00 Laxman Dewangan <ldewangan@nvidia.com>:
> Maxim Semiconductor's PMIC MAX77620/MAX20024 has on chip
> RTC  module. This support for setting alarm and time.
>
> Add RTC driver to access this chip's RTC module via RTC
> APIs.
>
> Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
> Signed-off-by: Chaitanya Bandi <bandik@nvidia.com>
> Tested-by: Venkat Reddy Talla <vreddytalla@nvidia.com>

Same as for mfd patch. The tag should be given explicitly in a public way.

> ---
>  drivers/rtc/Kconfig        |   9 +
>  drivers/rtc/Makefile       |   1 +
>  drivers/rtc/rtc-max77620.c | 574 +++++++++++++++++++++++++++++++++++++++++++++

The driver (as most of Maxim's) looks quite similar to existing RTC
driver, like max77802. It is difficult to spot the exact differences
(I don't have 77620's datasheet) but I would suggest not duplicating
the work. Maybe the biggest difference is the way you configure the
regmap... but still sometimes the regmap config can be shared.

Anyway some minor comments below.


>  3 files changed, 584 insertions(+)
>  create mode 100644 drivers/rtc/rtc-max77620.c
>
> diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
> index 376322f..8723bf8 100644
> --- a/drivers/rtc/Kconfig
> +++ b/drivers/rtc/Kconfig
> @@ -315,6 +315,15 @@ config RTC_DRV_MAX8997
>           This driver can also be built as a module. If so, the module
>           will be called rtc-max8997.
>
> +config RTC_DRV_MAX77620
> +       tristate "Maxim MAX77620/MAX20024"
> +       depends on MFD_MAX77620
> +       help
> +         If you say yes here you will get support for the
> +         RTC of Maxim MAX77620/MAX20024 PMIC.
> +         This driver can also be built as a module. If so, the module
> +         will be called rtc-max77620.
> +
>  config RTC_DRV_MAX77686
>         tristate "Maxim MAX77686"
>         depends on MFD_MAX77686
> diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
> index 62d61b2..da5db0a 100644
> --- a/drivers/rtc/Makefile
> +++ b/drivers/rtc/Makefile
> @@ -85,6 +85,7 @@ obj-$(CONFIG_RTC_DRV_M48T59)  += rtc-m48t59.o
>  obj-$(CONFIG_RTC_DRV_M48T86)   += rtc-m48t86.o
>  obj-$(CONFIG_RTC_DRV_MAX6900)  += rtc-max6900.o
>  obj-$(CONFIG_RTC_DRV_MAX6902)  += rtc-max6902.o
> +obj-$(CONFIG_RTC_DRV_MAX77620) += rtc-max77620.o
>  obj-$(CONFIG_RTC_DRV_MAX77686) += rtc-max77686.o
>  obj-$(CONFIG_RTC_DRV_MAX77802) += rtc-max77802.o
>  obj-$(CONFIG_RTC_DRV_MAX8907)  += rtc-max8907.o
> diff --git a/drivers/rtc/rtc-max77620.c b/drivers/rtc/rtc-max77620.c
> new file mode 100644
> index 0000000..645c661
> --- /dev/null
> +++ b/drivers/rtc/rtc-max77620.c
> @@ -0,0 +1,574 @@
> +/*
> + * Max77620 RTC driver
> + *
> + * Copyright (c) 2016, NVIDIA CORPORATION.  All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +#include <linux/delay.h>
> +#include <linux/i2c.h>
> +#include <linux/rtc.h>
> +#include <linux/mfd/max77620.h>
> +
> +#define MAX77620_RTC60S_MASK           BIT(0)
> +#define MAX77620_RTCA1_MASK            BIT(1)
> +#define MAX77620_RTCA2_MASK            BIT(2)
> +#define MAX77620_RTC_SMPL_MASK         BIT(3)
> +#define MAX77620_RTC_RTC1S_MASK                BIT(4)
> +#define MAX77620_RTC_ALL_IRQ_MASK      0x1F
> +
> +#define MAX77620_BCDM_MASK             BIT(0)
> +#define MAX77620_HRMODEM_MASK          BIT(1)
> +
> +#define WB_UPDATE_MASK                 BIT(0)
> +#define FLAG_AUTO_CLEAR_MASK           BIT(1)
> +#define FREEZE_SEC_MASK                        BIT(2)
> +#define RTC_WAKE_MASK                  BIT(3)
> +#define RB_UPDATE_MASK                 BIT(4)
> +
> +#define MAX77620_UDF_MASK              BIT(0)
> +#define MAX77620_RBUDF_MASK            BIT(1)
> +
> +#define SEC_MASK                       0x7F
> +#define MIN_MASK                       0x7F
> +#define HOUR_MASK                      0x3F
> +#define WEEKDAY_MASK                   0x7F
> +#define MONTH_MASK                     0x1F
> +#define YEAR_MASK                      0xFF
> +#define MONTHDAY_MASK                  0x3F
> +
> +#define ALARM_EN_MASK                  0x80
> +#define ALARM_EN_SHIFT                 7
> +
> +#define RTC_YEAR_BASE                  100
> +#define RTC_YEAR_MAX                   99
> +
> +#define ONOFF_WK_ALARM1_MASK           BIT(2)
> +
> +enum {
> +       RTC_SEC,
> +       RTC_MIN,
> +       RTC_HOUR,
> +       RTC_WEEKDAY,
> +       RTC_MONTH,
> +       RTC_YEAR,
> +       RTC_MONTHDAY,
> +       RTC_NR
> +};
> +
> +struct max77620_rtc {
> +       struct rtc_device *rtc;
> +       struct device *dev;
> +
> +       struct mutex io_lock;
> +       int irq;
> +       u8 irq_mask;
> +};
> +
> +static inline struct device *_to_parent(struct max77620_rtc *rtc)

max77620_to_parent().

> +{
> +       return rtc->dev->parent;
> +}
> +
> +static inline int max77620_rtc_update_buffer(struct max77620_rtc *rtc,
> +                                            int write)

What's the purpose of declaring it inline?

> +{
> +       struct device *parent = _to_parent(rtc);
> +       u8 val =  FLAG_AUTO_CLEAR_MASK | RTC_WAKE_MASK;
> +       int ret;
> +
> +       if (write)
> +               val |= WB_UPDATE_MASK;
> +       else
> +               val |= RB_UPDATE_MASK;
> +
> +       dev_dbg(rtc->dev, "rtc_update_buffer: write=%d, addr=0x%x, val=0x%x\n",
> +               write, MAX77620_REG_RTCUPDATE0, val);
> +       ret = max77620_reg_write(parent, MAX77620_RTC_SLAVE,
> +                                               MAX77620_REG_RTCUPDATE0, val);
> +       if (ret < 0) {
> +               dev_err(rtc->dev, "Reg RTCUPDATE0 read failed: %d\n", ret);
> +               return ret;
> +       }
> +
> +       /* Must wait 16ms for buffer update */
> +       usleep_range(16000, 17000);
> +
> +       return 0;
> +}
> +
> +static inline int max77620_rtc_write(struct max77620_rtc *rtc, u8 addr,
> +                                    void *values, u32 len, int update_buffer)

It seems you are using 0 or 1 for update_buffer. So bool.

> +{
> +       struct device *parent = _to_parent(rtc);
> +       int ret;
> +
> +       mutex_lock(&rtc->io_lock);
> +
> +       ret = max77620_reg_writes(parent, MAX77620_RTC_SLAVE,
> +                                               addr, len, values);
> +       if (ret < 0)
> +               goto out;
> +
> +       if (update_buffer)
> +               ret = max77620_rtc_update_buffer(rtc, 1);
> +
> +out:
> +       mutex_unlock(&rtc->io_lock);

New line, here and in other places.

> +       return ret;
> +}
> +
> +static inline int max77620_rtc_read(struct max77620_rtc *rtc, u8 addr,
> +                                   void *values, u32 len, int update_buffer)
> +{
> +       struct device *parent = _to_parent(rtc);
> +       int ret;
> +
> +       mutex_lock(&rtc->io_lock);
> +
> +       if (update_buffer) {
> +               ret = max77620_rtc_update_buffer(rtc, 0);
> +               if (ret < 0)
> +                       goto out;
> +       }
> +
> +       ret = max77620_reg_reads(parent, MAX77620_RTC_SLAVE, addr, len, values);
> +out:
> +       mutex_unlock(&rtc->io_lock);
> +       return ret;
> +}
> +
> +static inline int max77620_rtc_reg_to_tm(struct max77620_rtc *rtc, u8 *buf,
> +                                        struct rtc_time *tm)
> +{
> +       int wday = buf[RTC_WEEKDAY] & WEEKDAY_MASK;
> +
> +       if (unlikely(!wday)) {
> +               dev_err(rtc->dev,
> +                       "rtc_reg_to_tm: Invalid day of week, %d\n", wday);
> +               return -EINVAL;
> +       }
> +
> +       tm->tm_sec = (int)(buf[RTC_SEC] & SEC_MASK);
> +       tm->tm_min = (int)(buf[RTC_MIN] & MIN_MASK);
> +       tm->tm_hour = (int)(buf[RTC_HOUR] & HOUR_MASK);
> +       tm->tm_mday = (int)(buf[RTC_MONTHDAY] & MONTHDAY_MASK);
> +       tm->tm_mon = (int)(buf[RTC_MONTH] & MONTH_MASK) - 1;
> +       tm->tm_year = (int)(buf[RTC_YEAR] & YEAR_MASK) + RTC_YEAR_BASE;
> +       tm->tm_wday = ffs(wday) - 1;
> +
> +       return 0;
> +}
> +
> +static inline int max77620_rtc_tm_to_reg(struct max77620_rtc *rtc, u8 *buf,
> +                                        struct rtc_time *tm, int alarm)
> +{
> +       u8 alarm_mask = alarm ? ALARM_EN_MASK : 0;
> +
> +       if (unlikely((tm->tm_year < RTC_YEAR_BASE) ||
> +                       (tm->tm_year > RTC_YEAR_BASE + RTC_YEAR_MAX))) {
> +               dev_err(rtc->dev,
> +                       "rtc_tm_to_reg: Invalid year, %d\n", tm->tm_year);
> +               return -EINVAL;
> +       }
> +
> +       buf[RTC_SEC] = tm->tm_sec | alarm_mask;
> +       buf[RTC_MIN] = tm->tm_min | alarm_mask;
> +       buf[RTC_HOUR] = tm->tm_hour | alarm_mask;
> +       buf[RTC_MONTHDAY] = tm->tm_mday | alarm_mask;
> +       buf[RTC_MONTH] = (tm->tm_mon + 1) | alarm_mask;
> +       buf[RTC_YEAR] = (tm->tm_year - RTC_YEAR_BASE) | alarm_mask;
> +
> +       /* The wday is configured only when disabled alarm. */
> +       if (!alarm)
> +               buf[RTC_WEEKDAY] = (1 << tm->tm_wday);
> +       else {
> +       /* Configure its default reset value 0x01, and not enable it. */
> +               buf[RTC_WEEKDAY] = 0x01;
> +       }
> +       return 0;
> +}
> +
> +static inline int max77620_rtc_irq_mask(struct max77620_rtc *rtc, u8 irq)
> +{
> +       u8 irq_mask = rtc->irq_mask | irq;
> +       int ret = 0;
> +
> +       ret = max77620_rtc_write(rtc, MAX77620_REG_RTCINTM, &irq_mask, 1, 1);
> +       if (ret < 0) {
> +               dev_err(rtc->dev, "rtc_irq_mask: Failed to set rtc irq mask\n");

Again a chain of error messages for the same error. The error comes
either from max77620_reg_writes() or from
max77620_rtc_update_buffer(). Then you print it here as well polluting
the dmesg.

> +               goto out;
> +       }
> +       rtc->irq_mask = irq_mask;
> +
> +out:
> +       return ret;
> +}
> +
> +static inline int max77620_rtc_irq_unmask(struct max77620_rtc *rtc, u8 irq)
> +{
> +       u8 irq_mask = rtc->irq_mask & ~irq;
> +       int ret = 0;
> +
> +       ret = max77620_rtc_write(rtc, MAX77620_REG_RTCINTM, &irq_mask, 1, 1);
> +       if (ret < 0) {
> +               dev_err(rtc->dev,
> +                       "rtc_irq_unmask: Failed to set rtc irq mask\n");
> +               goto out;
> +       }
> +       rtc->irq_mask = irq_mask;
> +
> +out:
> +       return ret;
> +}
> +
> +static inline int max77620_rtc_do_irq(struct max77620_rtc *rtc)
> +{
> +       struct device *parent = _to_parent(rtc);
> +       u8 irq_status;
> +       int ret;
> +
> +       ret = max77620_reg_read(parent, MAX77620_RTC_SLAVE,
> +                                       MAX77620_REG_RTCINT, &irq_status);
> +       if (ret < 0) {
> +               dev_err(rtc->dev, "rtc_irq: Failed to get rtc irq status\n");
> +               return ret;
> +       }
> +
> +       dev_dbg(rtc->dev, "rtc_do_irq: irq_mask=0x%02x, irq_status=0x%02x\n",
> +               rtc->irq_mask, irq_status);
> +
> +       if (!(rtc->irq_mask & MAX77620_RTCA1_MASK) &&
> +                       (irq_status & MAX77620_RTCA1_MASK))
> +               rtc_update_irq(rtc->rtc, 1, RTC_IRQF | RTC_AF);
> +
> +       if (!(rtc->irq_mask & MAX77620_RTC_RTC1S_MASK) &&
> +                       (irq_status & MAX77620_RTC_RTC1S_MASK))
> +               rtc_update_irq(rtc->rtc, 1, RTC_IRQF | RTC_UF);
> +
> +       return ret;
> +}
> +
> +static irqreturn_t max77620_rtc_irq(int irq, void *data)
> +{
> +       struct max77620_rtc *rtc = (struct max77620_rtc *)data;
> +
> +       max77620_rtc_do_irq(rtc);
> +
> +       return IRQ_HANDLED;
> +}
> +
> +static int max77620_rtc_alarm_irq_enable(struct device *dev,
> +                                        unsigned int enabled)
> +{
> +       struct max77620_rtc *rtc = dev_get_drvdata(dev);
> +       int ret = 0;
> +
> +       if (rtc->irq < 0)
> +               return -ENXIO;
> +
> +       /* Handle pending interrupt */
> +       ret = max77620_rtc_do_irq(rtc);
> +       if (ret < 0)
> +               goto out;
> +
> +       /* Config alarm interrupt */

s/Config/Configure/ ?

> +       if (enabled) {
> +               ret = max77620_rtc_irq_unmask(rtc, MAX77620_RTCA1_MASK);
> +               if (ret < 0)
> +                       goto out;
> +       } else {
> +               ret = max77620_rtc_irq_mask(rtc, MAX77620_RTCA1_MASK);
> +               if (ret < 0)
> +                       goto out;
> +       }
> +out:
> +       return ret;
> +}
> +
> +static int max77620_rtc_read_time(struct device *dev, struct rtc_time *tm)
> +{
> +       struct max77620_rtc *rtc = dev_get_drvdata(dev);
> +       u8 buf[RTC_NR];
> +       int ret;
> +
> +       ret = max77620_rtc_read(rtc, MAX77620_REG_RTCSEC, buf, sizeof(buf), 1);
> +       if (ret < 0) {
> +               dev_err(rtc->dev, "Reg RTCSEC read failed: %d\n", ret);
> +               return ret;
> +       }
> +
> +       ret = max77620_rtc_reg_to_tm(rtc, buf, tm);
> +       if (ret < 0) {
> +               dev_err(rtc->dev, "Reg format to time format conv failed: %d\n",
> +                       ret);

The same pattern of error messages - max77620_rtc_reg_to_tm() will
print it. Show it only once.

> +               return ret;
> +       }
> +
> +       return ret;
> +}
> +
> +static int max77620_rtc_set_time(struct device *dev, struct rtc_time *tm)
> +{
> +       struct max77620_rtc *rtc = dev_get_drvdata(dev);
> +       u8 buf[RTC_NR];
> +       int ret;
> +
> +       ret = max77620_rtc_tm_to_reg(rtc, buf, tm, 0);
> +       if (ret < 0) {
> +               dev_err(rtc->dev, "Time format to Reg format conv failed: %d\n",
> +                       ret);
> +               return ret;
> +       }
> +
> +       return max77620_rtc_write(rtc, MAX77620_REG_RTCSEC,
> +                                       buf, sizeof(buf), 1);
> +}
> +
> +static int max77620_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm)
> +{
> +       struct max77620_rtc *rtc = dev_get_drvdata(dev);
> +       u8 buf[RTC_NR];
> +       int ret;
> +
> +       ret = max77620_rtc_read(rtc, MAX77620_REG_RTCSECA1,
> +                                       buf, sizeof(buf), 1);
> +       if (ret < 0) {
> +               dev_err(rtc->dev, "Reg RTCSECA1 read failed: %d\n", ret);
> +               return ret;
> +       }
> +

One line is enough.

> +
> +       buf[RTC_YEAR] &= ~ALARM_EN_MASK;
> +       ret = max77620_rtc_reg_to_tm(rtc, buf, &alrm->time);
> +       if (ret < 0) {
> +               dev_err(rtc->dev, "Reg format to time format conv failed: %d\n",
> +                       ret);
> +               return ret;
> +       }
> +
> +       if (rtc->irq_mask & MAX77620_RTCA1_MASK)
> +               alrm->enabled = 0;
> +       else
> +               alrm->enabled = 1;
> +
> +       return 0;
> +}
> +
> +static int max77620_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
> +{
> +       struct max77620_rtc *rtc = dev_get_drvdata(dev);
> +       u8 buf[RTC_NR];
> +       int ret;
> +
> +       ret = max77620_rtc_tm_to_reg(rtc, buf, &alrm->time, 1);
> +       if (ret < 0) {
> +               dev_err(rtc->dev, "Time format to reg format conv failed: %d\n",
> +                       ret);
> +               return ret;
> +       }
> +
> +       ret = max77620_rtc_write(rtc, MAX77620_REG_RTCSECA1, buf,
> +                       sizeof(buf), 1);
> +       if (ret < 0) {
> +               dev_err(rtc->dev, "Reg RTCSECA1 write failed: %d\n", ret);
> +               return ret;
> +       }
> +
> +       ret = max77620_rtc_alarm_irq_enable(dev, alrm->enabled);
> +       if (ret < 0) {
> +               dev_err(rtc->dev, "Enable rtc alarm failed: %d\n", ret);
> +               return ret;
> +       }
> +
> +       return ret;
> +}
> +
> +static const struct rtc_class_ops max77620_rtc_ops = {
> +       .read_time = max77620_rtc_read_time,
> +       .set_time = max77620_rtc_set_time,
> +       .read_alarm = max77620_rtc_read_alarm,
> +       .set_alarm = max77620_rtc_set_alarm,
> +       .alarm_irq_enable = max77620_rtc_alarm_irq_enable,
> +};
> +
> +static int max77620_rtc_preinit(struct max77620_rtc *rtc)
> +{
> +       struct device *parent = _to_parent(rtc);
> +       u8 val;
> +       int ret;
> +
> +       /* Mask all interrupts */
> +       rtc->irq_mask = 0xFF;
> +       ret = max77620_rtc_write(rtc, MAX77620_REG_RTCINTM,
> +                                               &rtc->irq_mask, 1, 1);
> +       if (ret < 0) {
> +               dev_err(rtc->dev, "preinit: Failed to set rtc irq mask\n");
> +               return ret;
> +       }
> +
> +       max77620_rtc_read(rtc, MAX77620_REG_RTCINT, &val, 1, 0);
> +
> +       /* Configure Binary mode and 24hour mode */
> +       val = MAX77620_HRMODEM_MASK;
> +       ret = max77620_rtc_write(rtc, MAX77620_REG_RTCCNTL, &val, 1, 1);
> +       if (ret < 0) {
> +               dev_err(rtc->dev, "preinit: Failed to set rtc control\n");
> +               return ret;
> +       }
> +
> +       /* It should be disabled alarm wakeup to wakeup from sleep
> +        * by EN1 input signal.
> +        */

Please use Linux coding style for multi-line comments. I also can't
get the meaning. It should be disabled wakeup to wakeup?

> +       ret = max77620_reg_update(parent, MAX77620_PWR_SLAVE,
> +               MAX77620_REG_ONOFFCNFG2, ONOFF_WK_ALARM1_MASK, 0);
> +       if (ret < 0) {
> +               dev_err(rtc->dev, "preinit: Failed to set onoff cfg2\n");
> +               return ret;
> +       }
> +
> +       return 0;
> +}
> +
> +static int max77620_rtc_probe(struct platform_device *pdev)
> +{
> +       static struct max77620_rtc *rtc;
> +       int ret = 0;
> +
> +       rtc = devm_kzalloc(&pdev->dev, sizeof(*rtc), GFP_KERNEL);
> +       if (!rtc)
> +               return -ENOMEM;
> +
> +       dev_set_drvdata(&pdev->dev, rtc);
> +       rtc->dev = &pdev->dev;
> +       mutex_init(&rtc->io_lock);
> +
> +       ret = max77620_rtc_preinit(rtc);
> +       if (ret) {
> +               dev_err(&pdev->dev, "probe: Failed to rtc preinit\n");
> +               goto fail_preinit;
> +       }
> +
> +       device_init_wakeup(&pdev->dev, 1);
> +
> +       rtc->rtc = devm_rtc_device_register(&pdev->dev, "max77620-rtc",
> +                                      &max77620_rtc_ops, THIS_MODULE);
> +       if (IS_ERR(rtc->rtc)) {
> +               dev_err(&pdev->dev, "probe: Failed to register rtc\n");
> +               ret = PTR_ERR(rtc->rtc);
> +               goto fail_preinit;
> +       }
> +
> +       rtc->irq = platform_get_irq(pdev, 0);
> +       ret = devm_request_threaded_irq(&pdev->dev, rtc->irq, NULL,
> +                       max77620_rtc_irq, IRQF_ONESHOT | IRQF_EARLY_RESUME,

Why early resume?

> +                       "max77620-rtc", rtc);
> +       if (ret < 0) {
> +               dev_err(rtc->dev, "probe: Failed to request irq %d\n",
> +                       rtc->irq);
> +               rtc->irq = -1;

Why? The probe will fail. Will it be used somewhere?

> +               goto fail_preinit;
> +       }
> +
> +       device_init_wakeup(rtc->dev, 1);

This shouldn't be needed.

> +       enable_irq_wake(rtc->irq);
> +
> +       return 0;
> +
> +fail_preinit:
> +       mutex_destroy(&rtc->io_lock);
> +       return ret;
> +}
> +
> +static int max77620_rtc_remove(struct platform_device *pdev)
> +{
> +       struct max77620_rtc *rtc = dev_get_drvdata(&pdev->dev);
> +
> +       mutex_destroy(&rtc->io_lock);
> +       return 0;
> +}
> +
> +#ifdef CONFIG_PM_SLEEP
> +static int max77620_rtc_suspend(struct device *dev)
> +{
> +       struct max77620_rtc *max77620_rtc = dev_get_drvdata(dev);
> +
> +       if (device_may_wakeup(dev)) {
> +               int ret;
> +               struct rtc_wkalrm alm;
> +
> +               enable_irq_wake(max77620_rtc->irq);
> +               ret = max77620_rtc_read_alarm(dev, &alm);
> +               if (!ret)
> +                       dev_info(dev, "%s() alrm %d time %d %d %d %d %d %d\n",
> +                               __func__, alm.enabled,
> +                               alm.time.tm_year, alm.time.tm_mon,
> +                               alm.time.tm_mday, alm.time.tm_hour,
> +                               alm.time.tm_min, alm.time.tm_sec);

Nope. On each suspend you want to print current time? This does not
even classifies for dev_dbg.

> +       }
> +
> +       return 0;
> +}
> +
> +static int max77620_rtc_resume(struct device *dev)
> +{
> +       struct max77620_rtc *max77620_rtc = dev_get_drvdata(dev);
> +
> +       if (device_may_wakeup(dev)) {
> +               struct rtc_time tm;
> +               int ret;
> +
> +               disable_irq_wake(max77620_rtc->irq);
> +               ret = max77620_rtc_read_time(dev, &tm);
> +               if (!ret)
> +                       dev_info(dev, "%s() %d %d %d %d %d %d\n",
> +                               __func__, tm.tm_year, tm.tm_mon, tm.tm_mday,
> +                               tm.tm_hour, tm.tm_min, tm.tm_sec);

Ditto.

> +       }
> +
> +       return 0;
> +}
> +#endif
> +
> +static const struct dev_pm_ops max77620_rtc_pm_ops = {
> +       SET_SYSTEM_SLEEP_PM_OPS(max77620_rtc_suspend, max77620_rtc_resume)
> +};
> +
> +static struct platform_device_id max77620_rtc_devtype[] = {

const?

Best regards,
Krzysztof

> +       {
> +               .name = "max77620-rtc",
> +       },
> +       {
> +               .name = "max20024-rtc",
> +       },
> +};
> +
> +static struct platform_driver max77620_rtc_driver = {
> +       .probe = max77620_rtc_probe,
> +       .remove = max77620_rtc_remove,
> +       .id_table = max77620_rtc_devtype,
> +       .driver = {
> +                       .name = "max77620-rtc",
> +                       .owner = THIS_MODULE,
> +                       .pm = &max77620_rtc_pm_ops,
> +       },
> +};
> +
> +module_platform_driver(max77620_rtc_driver);
> +
> +MODULE_DESCRIPTION("max77620 RTC driver");
> +MODULE_AUTHOR("Chaitanya Bandi <bandik@nvidia.com>");
> +MODULE_AUTHOR("Laxman Dewangan <ldewangan@nvidia.com>");
> +MODULE_ALIAS("platform:max77620-rtc");
> +MODULE_LICENSE("GPL v2");
> --
> 2.1.4
>
> --
> --
> You received this message because you are subscribed to "rtc-linux".
> Membership options at http://groups.google.com/group/rtc-linux .
> Please read http://groups.google.com/group/rtc-linux/web/checklist
> before submitting a driver.
> ---
> You received this message because you are subscribed to the Google Groups "rtc-linux" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Hi Krzysztof,
Thanks for review.

Accepted most of review comment and will update in next patch.

Answer to query:


On Friday 08 January 2016 07:33 AM, Krzysztof Kozlowski wrote:
> 2016-01-07 23:38 GMT+09:00 Laxman Dewangan <ldewangan@nvidia.com>:
> ---
>   drivers/rtc/Kconfig        |   9 +
>   drivers/rtc/Makefile       |   1 +
>   drivers/rtc/rtc-max77620.c | 574 +++++++++++++++++++++++++++++++++++++++++++++
> The driver (as most of Maxim's) looks quite similar to existing RTC
> driver, like max77802. It is difficult to spot the exact differences
> (I don't have 77620's datasheet) but I would suggest not duplicating
> the work. Maybe the biggest difference is the way you configure the
> regmap... but still sometimes the regmap config can be shared.
Yaah, that is the issue on IP based PMIC system and it happen with the 
Maxim and TI.
The MFD and its sub module drivers are too much couped  and hence 
dificult to use the IP driver across PMIC. For almost all Maxim PMIC, we 
end up of same type of RTC driver.

Probably, we need to enhance the mfd sub system to allow sub module 
driver to decoupe from its APIs.


+
+       rtc->irq = platform_get_irq(pdev, 0);
+       ret = devm_request_threaded_irq(&pdev->dev, rtc->irq, NULL,
+                       max77620_rtc_irq, IRQF_ONESHOT | IRQF_EARLY_RESUME,

> Why early resume?

When we go on suspend, the interrupts are masked by framework. For 
regmap-irq, it is stored on local reg value but not written into the 
PMIC register.
In Nvidia Tegra system, ARM GIC controller registered before the other 
interrupt controller like GPIO, PMIC etc.
When wakeup happened through RTC alarm, we get the interrupt from PMIC 
and on resume path, the PMIC isr handler get called after PMIC interrupt 
to the GIC is unmasked. But at this time, PMIC RTC alarm is still in 
masked state by regmap-irq which causes the interrupt to RTC ignore in 
the PMIC ISR handler and so RTC Isr did not get called and not cleared 
interrupt. This causes PMIC ISr handler to stuck on loop.

The need is to unmask the RTC alarm interrupt on PMIC interrupt driver 
before PMIC interrupt served so that we can have proper interrupt status 
in handler and rtc isr can get called.

For this, RTC alarm interrupt need to be EARLY_RESUME.


--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Fri, Jan 08, 2016 at 03:50:46PM +0530, Laxman Dewangan wrote:

> Yaah, that is the issue on IP based PMIC system and it happen with the Maxim
> and TI.
> The MFD and its sub module drivers are too much couped  and hence dificult
> to use the IP driver across PMIC. For almost all Maxim PMIC, we end up of
> same type of RTC driver.

> Probably, we need to enhance the mfd sub system to allow sub module driver
> to decoupe from its APIs.

Could you be more specific about the issues here please?  I can't think
of any barriers and you've not mentioned any barriers...

On Friday 08 January 2016 06:21 PM, Mark Brown wrote:
> * PGP Signed by an unknown key
>
> On Fri, Jan 08, 2016 at 03:50:46PM +0530, Laxman Dewangan wrote:
>
>> Yaah, that is the issue on IP based PMIC system and it happen with the Maxim
>> and TI.
>> The MFD and its sub module drivers are too much couped  and hence dificult
>> to use the IP driver across PMIC. For almost all Maxim PMIC, we end up of
>> same type of RTC driver.
>> Probably, we need to enhance the mfd sub system to allow sub module driver
>> to decoupe from its APIs.
> Could you be more specific about the issues here please?  I can't think
> of any barriers and you've not mentioned any barriers...
>

I took MAX77686 and MAX77620. They are almost same IP on both PMIC and I 
think it can be used if we decouple it. Typical codes are same as 
follows: (I posted here for quick reference, excuse me if it is long).


If we get the parent device, regmap handle and interrupt number from mfd 
core independent of the PMIC (MAX77620 or MAX77686), then same driver 
can be used here.
Two way which I can think of here:

1: We need to make independent maxim RTC IP driver which may be platform 
driver and get above information from the platform data.

This way both mfd driver can pass the platform data with required 
information and decoupled.

2. Other way is that rtc driver will register as independent platform 
driver and pass the mfd dt handle to this driver. Later maxim rtc driver 
can query for getting the regmap, interrupt number etc.

Any other approach are welcome.


sample code:
************************8
Typical function from max77686:

static int max77686_rtc_set_time(struct device *dev, struct rtc_time *tm)
{
         struct max77686_rtc_info *info = dev_get_drvdata(dev);
         u8 data[RTC_NR_TIME];
         int ret;

         ret = max77686_rtc_tm_to_data(tm, data);
         if (ret < 0)
                 return ret;

         mutex_lock(&info->lock);

         ret = regmap_bulk_write(info->max77686->rtc_regmap,
                                  MAX77686_RTC_SEC, data, RTC_NR_TIME);
         if (ret < 0) {
                 dev_err(info->dev, "%s: fail to write time reg(%d)\n", 
__func__,
                                 ret);
                 goto out;
         }

         ret = max77686_rtc_update(info, MAX77686_RTC_WRITE);

out:
         mutex_unlock(&info->lock);
         return ret;
}


static int max77620_rtc_set_time(struct device *dev, struct rtc_time *tm)
{
         struct max77620_rtc *rtc = dev_get_drvdata(dev);
         struct device *parent = max77620_to_parent(rtc);
         u8 buf[RTC_NR];
         int ret;

         ret = max77620_rtc_tm_to_reg(rtc, buf, tm, 0);
         if (ret < 0)
                 return ret;

         mutex_lock(&rtc->io_lock);

         ret = max77620_reg_writes(parent, MAX77620_RTC_SLAVE, addr, 
len, vals);
         if (ret < 0) {
                 dev_err(rtc->dev, "Reg 0x%02x write failed: %d\n", 
addr, ret);
                 goto out;
         }

         ret = max77620_rtc_update_buffer(rtc, 1);

out:
         mutex_unlock(&rtc->io_lock);

         return ret;
}




--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

W dniu 08.01.2016 o 19:20, Laxman Dewangan pisze:
> Hi Krzysztof,
> Thanks for review.
> 
> Accepted most of review comment and will update in next patch.
> 
> Answer to query:
> 
> 
> On Friday 08 January 2016 07:33 AM, Krzysztof Kozlowski wrote:
>> 2016-01-07 23:38 GMT+09:00 Laxman Dewangan <ldewangan@nvidia.com>:
>> ---
>>   drivers/rtc/Kconfig        |   9 +
>>   drivers/rtc/Makefile       |   1 +
>>   drivers/rtc/rtc-max77620.c | 574
>> +++++++++++++++++++++++++++++++++++++++++++++
>> The driver (as most of Maxim's) looks quite similar to existing RTC
>> driver, like max77802. It is difficult to spot the exact differences
>> (I don't have 77620's datasheet) but I would suggest not duplicating
>> the work. Maybe the biggest difference is the way you configure the
>> regmap... but still sometimes the regmap config can be shared.
> Yaah, that is the issue on IP based PMIC system and it happen with the
> Maxim and TI.
> The MFD and its sub module drivers are too much couped  and hence
> dificult to use the IP driver across PMIC. For almost all Maxim PMIC, we
> end up of same type of RTC driver.
> 
> Probably, we need to enhance the mfd sub system to allow sub module
> driver to decoupe from its APIs.

Actually the MFD and other subsystems are quite decoupled already and
support sharing drivers for common IP block. The problem is that we
develop drivers in a coupled way. This is not only issue of this
particular set of drivers. Others were developed in a same way.

So instead I would be happy to see this driver developed in the
decoupled way so existing RTC driver could be reused. If this requires
changing existing MFD driver like max77686 then please go ahead. I may
provide testing for that purpose. Something similar I made recently to
the max77693 and max77843. Both devices have different parent MFD
drivers but share some of the specific subsystem drivers: regulator and
input/haptic.

> 
> 
> +
> +       rtc->irq = platform_get_irq(pdev, 0);
> +       ret = devm_request_threaded_irq(&pdev->dev, rtc->irq, NULL,
> +                       max77620_rtc_irq, IRQF_ONESHOT | IRQF_EARLY_RESUME,
> 
>> Why early resume?
> 
> When we go on suspend, the interrupts are masked by framework. For
> regmap-irq, it is stored on local reg value but not written into the
> PMIC register.
> In Nvidia Tegra system, ARM GIC controller registered before the other
> interrupt controller like GPIO, PMIC etc.
> When wakeup happened through RTC alarm, we get the interrupt from PMIC
> and on resume path, the PMIC isr handler get called after PMIC interrupt
> to the GIC is unmasked. But at this time, PMIC RTC alarm is still in
> masked state by regmap-irq which causes the interrupt to RTC ignore in
> the PMIC ISR handler and so RTC Isr did not get called and not cleared
> interrupt. This causes PMIC ISr handler to stuck on loop.
> 
> The need is to unmask the RTC alarm interrupt on PMIC interrupt driver
> before PMIC interrupt served so that we can have proper interrupt status
> in handler and rtc isr can get called.
> 
> For this, RTC alarm interrupt need to be EARLY_RESUME.

Okay, this is quite common issue and I think this can be solved by
disabling the IRQ in suspend:
http://lxr.free-electrons.com/source/drivers/mfd/max77843.c#L205

IMHO this would be preferred way because you won't be moving device
suspend/resume callbacks to a syscore level.

Best regards,
Krzysztof
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Friday 08 January 2016 06:35 PM, Krzysztof Kozlowski wrote:
> W dniu 08.01.2016 o 19:20, Laxman Dewangan pisze:
>> Hi Krzysztof,
>> Thanks for review.
>>
>> Accepted most of review comment and will update in next patch.
>>
>> Answer to query:
>>
>>
>> On Friday 08 January 2016 07:33 AM, Krzysztof Kozlowski wrote:
>>
> Actually the MFD and other subsystems are quite decoupled already and
> support sharing drivers for common IP block. The problem is that we
> develop drivers in a coupled way. This is not only issue of this
> particular set of drivers. Others were developed in a same way.
>
> So instead I would be happy to see this driver developed in the
> decoupled way so existing RTC driver could be reused. If this requires
> changing existing MFD driver like max77686 then please go ahead. I may
> provide testing for that purpose. Something similar I made recently to
> the max77693 and max77843. Both devices have different parent MFD
> drivers but share some of the specific subsystem drivers: regulator and
> input/haptic.
I replied on other thread. I can decouple the RTC driver for max77620 
and will be available for review.
If it looks fine then we can modify the max6868 mfd driver to use this 
new driver for test purpose.


>>
>> For this, RTC alarm interrupt need to be EARLY_RESUME.
> Okay, this is quite common issue and I think this can be solved by
> disabling the IRQ in suspend:
> http://lxr.free-electrons.com/source/drivers/mfd/max77843.c#L205
>
> IMHO this would be preferred way because you won't be moving device
> suspend/resume callbacks to a syscore level.

I will try for disable_irq() in suspend and enable_irq() in resume.
I hope this will not affect the wakeup property from suspend.

--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Friday 08 January 2016 07:06 PM, Mark Brown wrote:
> * PGP Signed by an unknown key
>
> On Fri, Jan 08, 2016 at 06:34:29PM +0530, Laxman Dewangan wrote:
>
>> If we get the parent device, regmap handle and interrupt number from mfd
>> core independent of the PMIC (MAX77620 or MAX77686), then same driver can be
>> used here.
>> Two way which I can think of here:
> Parent device is just dev->parent, you can use dev_get_regmap() to get a
> regmap given a struct device and you can use platform resources to pass
> the interrupts to the children from the MFD (there's some examples,
> wm831x is one).
>
>

I think it should work with named regmap. mfd whould init regmap with 
name and rtc driver should ask with same name.

I saw three drivers which looks same:
rtc-max77620.c (new from me) and already available rtc-max77686.c, 
rtc-max77802.c

Seems I can develop IP based rtc driver as rtc-max77xxx.c


--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Fri, Jan 08, 2016 at 06:34:29PM +0530, Laxman Dewangan wrote:

> If we get the parent device, regmap handle and interrupt number from mfd
> core independent of the PMIC (MAX77620 or MAX77686), then same driver can be
> used here.
> Two way which I can think of here:

Parent device is just dev->parent, you can use dev_get_regmap() to get a
regmap given a struct device and you can use platform resources to pass
the interrupts to the children from the MFD (there's some examples,
wm831x is one).

From: Linux Kernel <linuxkernelmails@gmail.com>

Really?  Why the shroud of secrecy?

Any reason why you're not using your real name?

On Fri, 08 Jan 2016, Linux Kernel wrote:
> On Thursday 07 January 2016 08:08 PM, Laxman Dewangan wrote:
> 
> >Maxim Semiconductor's PMIC MAX77620/MAX20024 has on chip
> >RTC  module. This support for setting alarm and time.
> >
> >Add RTC driver to access this chip's RTC module via RTC
> >APIs.
> >
> >Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
> >Signed-off-by: Chaitanya Bandi <bandik@nvidia.com>
> >Tested-by: Venkat Reddy Talla <vreddytalla@nvidia.com>
> >---
> >  drivers/rtc/Kconfig        |   9 +
> >  drivers/rtc/Makefile       |   1 +
> >  drivers/rtc/rtc-max77620.c | 574 +++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 584 insertions(+)
> >  create mode 100644 drivers/rtc/rtc-max77620.c
> >
> (...)
> 
> >+static struct platform_driver max77620_rtc_driver = {
> >+	.probe = max77620_rtc_probe,
> >+	.remove = max77620_rtc_remove,
> >+	.id_table = max77620_rtc_devtype,
> >+	.driver = {
> >+			.name = "max77620-rtc",
> >+			.owner = THIS_MODULE,
> 
> Drop this line, .owner will be populated by driver core.
> 
> >+			.pm = &max77620_rtc_pm_ops,
> >+	},
> >+};
> >+
> >+module_platform_driver(max77620_rtc_driver);
> >+
> >+MODULE_DESCRIPTION("max77620 RTC driver");
> >+MODULE_AUTHOR("Chaitanya Bandi <bandik@nvidia.com>");
> >+MODULE_AUTHOR("Laxman Dewangan <ldewangan@nvidia.com>");
> >+MODULE_ALIAS("platform:max77620-rtc");
> >+MODULE_LICENSE("GPL v2");

On Friday 08 January 2016 07:06 PM, Laxman Dewangan wrote:
>
> On Friday 08 January 2016 07:06 PM, Mark Brown wrote:
>> * PGP Signed by an unknown key
>>
>> On Fri, Jan 08, 2016 at 06:34:29PM +0530, Laxman Dewangan wrote:
>>
>>> If we get the parent device, regmap handle and interrupt number from 
>>> mfd
>>> core independent of the PMIC (MAX77620 or MAX77686), then same 
>>> driver can be
>>> used here.
>>> Two way which I can think of here:
>> Parent device is just dev->parent, you can use dev_get_regmap() to get a
>> regmap given a struct device and you can use platform resources to pass
>> the interrupts to the children from the MFD (there's some examples,
>> wm831x is one).
>>
>>
>
> I think it should work with named regmap. mfd whould init regmap with 
> name and rtc driver should ask with same name.
>
> I saw three drivers which looks same:
> rtc-max77620.c (new from me) and already available rtc-max77686.c, 
> rtc-max77802.c
>
> Seems I can develop IP based rtc driver as rtc-max77xxx.c

I came with one of issue when doing this.

The RTC driver parent is not the same parent for which i2c slave address 
get registered.
There is two slave address from max77620, 0x3C (for general) and 0x68 
for RTC.

In max77620 mfd driver, we make dummy i2c client for 0x68 and initialize 
regmap with this address.

Now on mfd_add_devices, we pass the device for 0x3c and hence the RTC 
driver treat the parent as the 0x3c device but actually it should be 
0x68 to get the proper regmap.


Two approach:
1. If we add the option to pass parent_dev when adding cells form 
mfd_add_devices and select the parent device based on this option then 
it can be easily handle.
     Add parent_dev structure in struct mfd_cell and then change the 
parent in mfd_add_device() if cells has parent device.

2. Register the RTC driver with different mfd_add_devices with dummy i2c 
client device.
So two times mfd_add_devices.


IMO, approach 1 looks good to me.

Any opinion?


--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On 11/01/2016 at 18:47:34 +0530, Laxman Dewangan wrote :
> 
> On Friday 08 January 2016 07:06 PM, Laxman Dewangan wrote:
> >
> >On Friday 08 January 2016 07:06 PM, Mark Brown wrote:
> >>* PGP Signed by an unknown key
> >>
> >>On Fri, Jan 08, 2016 at 06:34:29PM +0530, Laxman Dewangan wrote:
> >>
> >>>If we get the parent device, regmap handle and interrupt number from
> >>>mfd
> >>>core independent of the PMIC (MAX77620 or MAX77686), then same driver
> >>>can be
> >>>used here.
> >>>Two way which I can think of here:
> >>Parent device is just dev->parent, you can use dev_get_regmap() to get a
> >>regmap given a struct device and you can use platform resources to pass
> >>the interrupts to the children from the MFD (there's some examples,
> >>wm831x is one).
> >>
> >>
> >
> >I think it should work with named regmap. mfd whould init regmap with name
> >and rtc driver should ask with same name.
> >
> >I saw three drivers which looks same:
> >rtc-max77620.c (new from me) and already available rtc-max77686.c,
> >rtc-max77802.c
> >
> >Seems I can develop IP based rtc driver as rtc-max77xxx.c
> 
> I came with one of issue when doing this.
> 
> The RTC driver parent is not the same parent for which i2c slave address get
> registered.
> There is two slave address from max77620, 0x3C (for general) and 0x68 for
> RTC.
> 
> In max77620 mfd driver, we make dummy i2c client for 0x68 and initialize
> regmap with this address.
> 
> Now on mfd_add_devices, we pass the device for 0x3c and hence the RTC driver
> treat the parent as the 0x3c device but actually it should be 0x68 to get
> the proper regmap.
> 
> 
> Two approach:
> 1. If we add the option to pass parent_dev when adding cells form
> mfd_add_devices and select the parent device based on this option then it
> can be easily handle.
>     Add parent_dev structure in struct mfd_cell and then change the parent
> in mfd_add_device() if cells has parent device.
> 
> 2. Register the RTC driver with different mfd_add_devices with dummy i2c
> client device.
> So two times mfd_add_devices.
> 
> 
> IMO, approach 1 looks good to me.
> 
> Any opinion?
> 

If the RTC is not at the same address, I'd say this is not an mfd
anymore, can't you probe it directly from DT?

On Monday 11 January 2016 09:34 PM, Alexandre Belloni wrote:
> On 11/01/2016 at 18:47:34 +0530, Laxman Dewangan wrote :
>> On Friday 08 January 2016 07:06 PM, Laxman Dewangan wrote:
>>> On Friday 08 January 2016 07:06 PM, Mark Brown wrote:
>>>> * PGP Signed by an unknown key
>>>>
>>>> On Fri, Jan 08, 2016 at 06:34:29PM +0530, Laxman Dewangan wrote:
>>>>
>>>>> If we get the parent device, regmap handle and interrupt number from
>>>>> mfd
>>>>> core independent of the PMIC (MAX77620 or MAX77686), then same driver
>>>>> can be
>>>>> used here.
>>>>> Two way which I can think of here:
>>>> Parent device is just dev->parent, you can use dev_get_regmap() to get a
>>>> regmap given a struct device and you can use platform resources to pass
>>>> the interrupts to the children from the MFD (there's some examples,
>>>> wm831x is one).
>>>>
>>>>
>>> I think it should work with named regmap. mfd whould init regmap with name
>>> and rtc driver should ask with same name.
>>>
>>> I saw three drivers which looks same:
>>> rtc-max77620.c (new from me) and already available rtc-max77686.c,
>>> rtc-max77802.c
>>>
>>> Seems I can develop IP based rtc driver as rtc-max77xxx.c
>> I came with one of issue when doing this.
>>
>> The RTC driver parent is not the same parent for which i2c slave address get
>> registered.
>> There is two slave address from max77620, 0x3C (for general) and 0x68 for
>> RTC.
>>
>> In max77620 mfd driver, we make dummy i2c client for 0x68 and initialize
>> regmap with this address.
>>
>> Now on mfd_add_devices, we pass the device for 0x3c and hence the RTC driver
>> treat the parent as the 0x3c device but actually it should be 0x68 to get
>> the proper regmap.
>>
>>
>> Two approach:
>> 1. If we add the option to pass parent_dev when adding cells form
>> mfd_add_devices and select the parent device based on this option then it
>> can be easily handle.
>>      Add parent_dev structure in struct mfd_cell and then change the parent
>> in mfd_add_device() if cells has parent device.
>>
>> 2. Register the RTC driver with different mfd_add_devices with dummy i2c
>> client device.
>> So two times mfd_add_devices.
>>
>>
>> IMO, approach 1 looks good to me.
>>
>> Any opinion?
>>
> If the RTC is not at the same address, I'd say this is not an mfd
> anymore, can't you probe it directly from DT?
>
>
This approach is also possible but,

although this is independent IP with separate i2c address but became it 
is inside the PMIC and its interrupt depends on PMIC internals, I like 
to register this rtc device from the mfd core.
So that when mfd core is ready with their interrupts and initial 
setting, it can register rtc device.


--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On 12.01.2016 02:07, Laxman Dewangan wrote:
> 
> On Monday 11 January 2016 09:34 PM, Alexandre Belloni wrote:
>> On 11/01/2016 at 18:47:34 +0530, Laxman Dewangan wrote :
>>> On Friday 08 January 2016 07:06 PM, Laxman Dewangan wrote:
>>>> On Friday 08 January 2016 07:06 PM, Mark Brown wrote:
>>>>> * PGP Signed by an unknown key
>>>>>
>>>>> On Fri, Jan 08, 2016 at 06:34:29PM +0530, Laxman Dewangan wrote:
>>>>>
>>>>>> If we get the parent device, regmap handle and interrupt number from
>>>>>> mfd
>>>>>> core independent of the PMIC (MAX77620 or MAX77686), then same driver
>>>>>> can be
>>>>>> used here.
>>>>>> Two way which I can think of here:
>>>>> Parent device is just dev->parent, you can use dev_get_regmap() to
>>>>> get a
>>>>> regmap given a struct device and you can use platform resources to
>>>>> pass
>>>>> the interrupts to the children from the MFD (there's some examples,
>>>>> wm831x is one).
>>>>>
>>>>>
>>>> I think it should work with named regmap. mfd whould init regmap
>>>> with name
>>>> and rtc driver should ask with same name.
>>>>
>>>> I saw three drivers which looks same:
>>>> rtc-max77620.c (new from me) and already available rtc-max77686.c,
>>>> rtc-max77802.c
>>>>
>>>> Seems I can develop IP based rtc driver as rtc-max77xxx.c
>>> I came with one of issue when doing this.
>>>
>>> The RTC driver parent is not the same parent for which i2c slave
>>> address get
>>> registered.
>>> There is two slave address from max77620, 0x3C (for general) and 0x68
>>> for
>>> RTC.
>>>
>>> In max77620 mfd driver, we make dummy i2c client for 0x68 and initialize
>>> regmap with this address.
>>>
>>> Now on mfd_add_devices, we pass the device for 0x3c and hence the RTC
>>> driver
>>> treat the parent as the 0x3c device but actually it should be 0x68 to
>>> get
>>> the proper regmap.
>>>
>>>
>>> Two approach:
>>> 1. If we add the option to pass parent_dev when adding cells form
>>> mfd_add_devices and select the parent device based on this option
>>> then it
>>> can be easily handle.
>>>      Add parent_dev structure in struct mfd_cell and then change the
>>> parent
>>> in mfd_add_device() if cells has parent device.
>>>
>>> 2. Register the RTC driver with different mfd_add_devices with dummy i2c
>>> client device.
>>> So two times mfd_add_devices.

Lexman,

I don't quite get the problem. This looks exactly the same as for
max77686. What is the difference? I don't see any need to change the
mfd_cell for current drivers...


>>>
>>>
>>> IMO, approach 1 looks good to me.
>>>
>>> Any opinion?
>>>
>> If the RTC is not at the same address, I'd say this is not an mfd
>> anymore, can't you probe it directly from DT?
>>
>>
> This approach is also possible but,
> 
> although this is independent IP with separate i2c address but became it
> is inside the PMIC and its interrupt depends on PMIC internals, I like
> to register this rtc device from the mfd core.
> So that when mfd core is ready with their interrupts and initial
> setting, it can register rtc device.

Alexandre,

All of these devices have some of the blocks under separate I2C address.
It is still a MFD because for example interrupts are shared and governed
by parent.

Best regards,
Krzysztof


--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Tuesday 12 January 2016 05:43 AM, Krzysztof Kozlowski wrote:
> On 12.01.2016 02:07, Laxman Dewangan wrote:
>> On Monday 11 January 2016 09:34 PM, Alexandre Belloni wrote:
>>> On 11/01/2016 at 18:47:34 +0530, Laxman Dewangan wrote :
>>>> On Friday 08 January 2016 07:06 PM, Laxman Dewangan wrote:
>>>>> On Friday 08 January 2016 07:06 PM, Mark Brown wrote:
>>>>>> * PGP Signed by an unknown key
>>>>>>
>>>>>> On Fri, Jan 08, 2016 at 06:34:29PM +0530, Laxman Dewangan wrote:
>>>>>>
>>>>>>> If we get the parent device, regmap handle and interrupt number from
>>>>>>> mfd
>>>>>>> core independent of the PMIC (MAX77620 or MAX77686), then same driver
>>>>>>> can be
>>>>>>> used here.
>>>>>>> Two way which I can think of here:
>>>>>> Parent device is just dev->parent, you can use dev_get_regmap() to
>>>>>> get a
>>>>>> regmap given a struct device and you can use platform resources to
>>>>>> pass
>>>>>> the interrupts to the children from the MFD (there's some examples,
>>>>>> wm831x is one).
>>>>>>
>>>>>>
>>>>> I think it should work with named regmap. mfd whould init regmap
>>>>> with name
>>>>> and rtc driver should ask with same name.
>>>>>
>>>>> I saw three drivers which looks same:
>>>>> rtc-max77620.c (new from me) and already available rtc-max77686.c,
>>>>> rtc-max77802.c
>>>>>
>>>>> Seems I can develop IP based rtc driver as rtc-max77xxx.c
>>>> I came with one of issue when doing this.
>>>>
>>>> The RTC driver parent is not the same parent for which i2c slave
>>>> address get
>>>> registered.
>>>> There is two slave address from max77620, 0x3C (for general) and 0x68
>>>> for
>>>> RTC.
>>>>
>>>> In max77620 mfd driver, we make dummy i2c client for 0x68 and initialize
>>>> regmap with this address.
>>>>
>>>> Now on mfd_add_devices, we pass the device for 0x3c and hence the RTC
>>>> driver
>>>> treat the parent as the 0x3c device but actually it should be 0x68 to
>>>> get
>>>> the proper regmap.
>>>>
>>>>
>>>> Two approach:
>>>> 1. If we add the option to pass parent_dev when adding cells form
>>>> mfd_add_devices and select the parent device based on this option
>>>> then it
>>>> can be easily handle.
>>>>       Add parent_dev structure in struct mfd_cell and then change the
>>>> parent
>>>> in mfd_add_device() if cells has parent device.
>>>>
>>>> 2. Register the RTC driver with different mfd_add_devices with dummy i2c
>>>> client device.
>>>> So two times mfd_add_devices.
> Lexman,
>
> I don't quite get the problem. This looks exactly the same as for
> max77686. What is the difference? I don't see any need to change the
> mfd_cell for current drivers...
>
>

Here the change is only required to pass the regmap handle from mfd to 
the rtc driver. There is no change for rest of rtc driver.

RTC i2c regmap registered with i2c dummy client device, not with actual 
parent device and hence we need to register RTC with this dummy i2c 
client device. This way we can get regmap handle by using the 
dev_get_regmap(pdev->dev.parent).

--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On 12.01.2016 11:32, Laxman Dewangan wrote:
> 
> On Tuesday 12 January 2016 05:43 AM, Krzysztof Kozlowski wrote:
>> On 12.01.2016 02:07, Laxman Dewangan wrote:
>>>>> The RTC driver parent is not the same parent for which i2c slave
>>>>> address get
>>>>> registered.
>>>>> There is two slave address from max77620, 0x3C (for general) and 0x68
>>>>> for
>>>>> RTC.
>>>>>
>>>>> In max77620 mfd driver, we make dummy i2c client for 0x68 and
>>>>> initialize
>>>>> regmap with this address.
>>>>>
>>>>> Now on mfd_add_devices, we pass the device for 0x3c and hence the RTC
>>>>> driver
>>>>> treat the parent as the 0x3c device but actually it should be 0x68 to
>>>>> get
>>>>> the proper regmap.
>>>>>
>>>>>
>>>>> Two approach:
>>>>> 1. If we add the option to pass parent_dev when adding cells form
>>>>> mfd_add_devices and select the parent device based on this option
>>>>> then it
>>>>> can be easily handle.
>>>>>       Add parent_dev structure in struct mfd_cell and then change the
>>>>> parent
>>>>> in mfd_add_device() if cells has parent device.
>>>>>
>>>>> 2. Register the RTC driver with different mfd_add_devices with
>>>>> dummy i2c
>>>>> client device.
>>>>> So two times mfd_add_devices.
>> Lexman,
>>
>> I don't quite get the problem. This looks exactly the same as for
>> max77686. What is the difference? I don't see any need to change the
>> mfd_cell for current drivers...
>>
>>
> 
> Here the change is only required to pass the regmap handle from mfd to
> the rtc driver. There is no change for rest of rtc driver.
> 
> RTC i2c regmap registered with i2c dummy client device, not with actual
> parent device and hence we need to register RTC with this dummy i2c
> client device. This way we can get regmap handle by using the
> dev_get_regmap(pdev->dev.parent).

It seems that both max77620 and max77686 use separate I2C addresses for
RTC block. The max77802 does not. This means that probably each child
driver should be responsible for its own regmap. The max77620 and
max77686 will create new I2C dummy devices and new regmaps. The max77802
will re-use references passed by parent (MFD).

Best regards,
Krzysztof
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Mon, Jan 11, 2016 at 6:46 AM, Lee Jones <lee.jones@linaro.org> wrote:
> From: Linux Kernel <linuxkernelmails@gmail.com>
>
> Really?  Why the shroud of secrecy?
>
> Any reason why you're not using your real name?

I wonder the same... You can use the mailadress linuxkernelmails@gmail.com
if you like, but atleast put in a real name.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html