Re: [PATCH v3 2/2] drivers: rtc: add xilinx zynqmp rtc driver

[Moritz Fischer]

Hi Suneel

On Wed, Aug 19, 2015 at 9:37 PM,  <suneelgarapati@xxxxxxxxx> wrote:
> Hi Moritz,
>
> devm_ioremap_resource validates the argument pointer of struct resource.
> Hence no explicit validation of return value of platform_get_resource
> function.
> It seems to be common practice with all other drivers in drivers/rtc.

Fair enough.

Acked-by: Moritz Fischer <moritz.fischer@xxxxxxxxx>
>
> Regards,
> Suneel
>
> On Wednesday, August 19, 2015 at 11:49:49 PM UTC+5:30, Moritz Fischer wrote:
>>
>> Hi Suneel,
>>
>> looks good, minor nit below.
>>
>> On Wed, Aug 19, 2015 at 2:53 AM, Suneel Garapati
>> <suneel....@xxxxxxxxxx> wrote:
>> > adds support for RTC controller found on
>> > Xilinx Zynq Ultrascale+ MPSoC platform.
>> >
>> > Signed-off-by: Suneel Garapati <suneel....@xxxxxxxxxx>
>> > ---
>> > Changes v3
>> > - fix checkpatch errors
>> > - check time in secs arg against max sec val
>> > - order header files
>> > - use ptr_err_or_zero macro
>> > - use time64 variants
>> > Changes v2
>> > - change alm to alarm
>> > ---
>> >  drivers/rtc/Kconfig      |   7 ++
>> >  drivers/rtc/Makefile     |   1 +
>> >  drivers/rtc/rtc-zynqmp.c | 279
>> > +++++++++++++++++++++++++++++++++++++++++++++++
>> >  3 files changed, 287 insertions(+)
>> >  create mode 100644 drivers/rtc/rtc-zynqmp.c
>> >
>> > diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
>> > index 35ea04c..bc96277 100644
>> > --- a/drivers/rtc/Kconfig
>> > +++ b/drivers/rtc/Kconfig
>> > @@ -1116,6 +1116,13 @@ config RTC_DRV_OPAL
>> >           This driver can also be built as a module. If so, the module
>> >           will be called rtc-opal.
>> >
>> > +config RTC_DRV_ZYNQMP
>> > +       tristate "Xilinx Zynq Ultrascale+ MPSoC RTC"
>> > +       depends on OF
>> > +       help
>> > +         If you say yes here you get support for the RTC controller
>> > found on
>> > +         Xilinx Zynq Ultrascale+ MPSoC.
>> > +
>> >  comment "on-CPU RTC drivers"
>> >
>> >  config RTC_DRV_DAVINCI
>> > diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
>> > index 2797384..e491eb5 100644
>> > --- a/drivers/rtc/Makefile
>> > +++ b/drivers/rtc/Makefile
>> > @@ -159,3 +159,4 @@ obj-$(CONFIG_RTC_DRV_WM831X)        += rtc-wm831x.o
>> >  obj-$(CONFIG_RTC_DRV_WM8350)   += rtc-wm8350.o
>> >  obj-$(CONFIG_RTC_DRV_X1205)    += rtc-x1205.o
>> >  obj-$(CONFIG_RTC_DRV_XGENE)    += rtc-xgene.o
>> > +obj-$(CONFIG_RTC_DRV_ZYNQMP)   += rtc-zynqmp.o
>> > diff --git a/drivers/rtc/rtc-zynqmp.c b/drivers/rtc/rtc-zynqmp.c
>> > new file mode 100644
>> > index 0000000..8b28762
>> > --- /dev/null
>> > +++ b/drivers/rtc/rtc-zynqmp.c
>> > @@ -0,0 +1,279 @@
>> > +/*
>> > + * Xilinx Zynq Ultrascale+ MPSoC Real Time Clock Driver
>> > + *
>> > + * Copyright (C) 2015 Xilinx, Inc.
>> > + *
>> > + * 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.
>> > + *
>> > + * You should have received a copy of the GNU General Public License
>> > along with
>> > + * this program. If not, see <http://www.gnu.org/licenses/>.
>> > + *
>> > + */
>> > +
>> > +#include <linux/delay.h>
>> > +#include <linux/init.h>
>> > +#include <linux/io.h>
>> > +#include <linux/module.h>
>> > +#include <linux/of.h>
>> > +#include <linux/platform_device.h>
>> > +#include <linux/rtc.h>
>> > +
>> > +/* RTC Registers */
>> > +#define RTC_SET_TM_WR          0x00
>> > +#define RTC_SET_TM_RD          0x04
>> > +#define RTC_CALIB_WR           0x08
>> > +#define RTC_CALIB_RD           0x0C
>> > +#define RTC_CUR_TM             0x10
>> > +#define RTC_CUR_TICK           0x14
>> > +#define RTC_ALRM               0x18
>> > +#define RTC_INT_STS            0x20
>> > +#define RTC_INT_MASK           0x24
>> > +#define RTC_INT_EN             0x28
>> > +#define RTC_INT_DIS            0x2C
>> > +#define RTC_CTRL               0x40
>> > +
>> > +#define RTC_FR_EN              BIT(20)
>> > +#define RTC_FR_DATSHIFT                16
>> > +#define RTC_TICK_MASK          0xFFFF
>> > +#define RTC_INT_SEC            BIT(0)
>> > +#define RTC_INT_ALRM           BIT(1)
>> > +#define RTC_OSC_EN             BIT(24)
>> > +
>> > +#define RTC_CALIB_DEF          0x198233
>> > +#define RTC_CALIB_MASK         0x1FFFFF
>> > +#define RTC_SEC_MAX_VAL                0xFFFFFFFF
>> > +
>> > +struct xlnx_rtc_dev {
>> > +       struct rtc_device       *rtc;
>> > +       void __iomem            *reg_base;
>> > +       int                     alarm_irq;
>> > +       int                     sec_irq;
>> > +};
>> > +
>> > +static int xlnx_rtc_set_time(struct device *dev, struct rtc_time *tm)
>> > +{
>> > +       struct xlnx_rtc_dev *xrtcdev = dev_get_drvdata(dev);
>> > +       unsigned long new_time;
>> > +
>> > +       new_time = rtc_tm_to_time64(tm);
>> > +
>> > +       if (new_time > RTC_SEC_MAX_VAL)
>> > +               return -EINVAL;
>> > +
>> > +       writel(new_time, xrtcdev->reg_base + RTC_SET_TM_WR);
>> > +
>> > +       return 0;
>> > +}
>> > +
>> > +static int xlnx_rtc_read_time(struct device *dev, struct rtc_time *tm)
>> > +{
>> > +       struct xlnx_rtc_dev *xrtcdev = dev_get_drvdata(dev);
>> > +
>> > +       rtc_time64_to_tm(readl(xrtcdev->reg_base + RTC_CUR_TM), tm);
>> > +
>> > +       return rtc_valid_tm(tm);
>> > +}
>> > +
>> > +static int xlnx_rtc_read_alarm(struct device *dev, struct rtc_wkalrm
>> > *alrm)
>> > +{
>> > +       struct xlnx_rtc_dev *xrtcdev = dev_get_drvdata(dev);
>> > +
>> > +       rtc_time64_to_tm(readl(xrtcdev->reg_base + RTC_ALRM),
>> > &alrm->time);
>> > +       alrm->enabled = readl(xrtcdev->reg_base + RTC_INT_MASK) &
>> > RTC_INT_ALRM;
>> > +
>> > +       return 0;
>> > +}
>> > +
>> > +static int xlnx_rtc_alarm_irq_enable(struct device *dev, u32 enabled)
>> > +{
>> > +       struct xlnx_rtc_dev *xrtcdev = dev_get_drvdata(dev);
>> > +
>> > +       if (enabled)
>> > +               writel(RTC_INT_ALRM, xrtcdev->reg_base + RTC_INT_EN);
>> > +       else
>> > +               writel(RTC_INT_ALRM, xrtcdev->reg_base + RTC_INT_DIS);
>> > +
>> > +       return 0;
>> > +}
>> > +
>> > +static int xlnx_rtc_set_alarm(struct device *dev, struct rtc_wkalrm
>> > *alrm)
>> > +{
>> > +       struct xlnx_rtc_dev *xrtcdev = dev_get_drvdata(dev);
>> > +       unsigned long alarm_time;
>> > +
>> > +       alarm_time = rtc_tm_to_time64(&alrm->time);
>> > +
>> > +       if (alarm_time > RTC_SEC_MAX_VAL)
>> > +               return -EINVAL;
>> > +
>> > +       writel((u32)alarm_time, (xrtcdev->reg_base + RTC_ALRM));
>> > +
>> > +       xlnx_rtc_alarm_irq_enable(dev, alrm->enabled);
>> > +
>> > +       return 0;
>> > +}
>> > +
>> > +static void xlnx_init_rtc(struct xlnx_rtc_dev *xrtcdev, u32 calibval)
>> > +{
>> > +       /*
>> > +        * Based on crystal freq of 33.330 KHz
>> > +        * set the seconds counter and enable, set fractions counter
>> > +        * to default value suggested as per design spec
>> > +        * to correct RTC delay in frequency over period of time.
>> > +        */
>> > +       calibval &= RTC_CALIB_MASK;
>> > +       writel(calibval, (xrtcdev->reg_base + RTC_CALIB_WR));
>> > +}
>> > +
>> > +static const struct rtc_class_ops xlnx_rtc_ops = {
>> > +       .set_time         = xlnx_rtc_set_time,
>> > +       .read_time        = xlnx_rtc_read_time,
>> > +       .read_alarm       = xlnx_rtc_read_alarm,
>> > +       .set_alarm        = xlnx_rtc_set_alarm,
>> > +       .alarm_irq_enable = xlnx_rtc_alarm_irq_enable,
>> > +};
>> > +
>> > +static irqreturn_t xlnx_rtc_interrupt(int irq, void *id)
>> > +{
>> > +       struct xlnx_rtc_dev *xrtcdev = (struct xlnx_rtc_dev *)id;
>> > +       unsigned int status;
>> > +
>> > +       status = readl(xrtcdev->reg_base + RTC_INT_STS);
>> > +       /* Check if interrupt asserted */
>> > +       if (!(status & (RTC_INT_SEC | RTC_INT_ALRM)))
>> > +               return IRQ_NONE;
>> > +
>> > +       /* Clear interrupt */
>> > +       writel(status, xrtcdev->reg_base + RTC_INT_STS);
>> > +
>> > +       if (status & RTC_INT_SEC)
>> > +               rtc_update_irq(xrtcdev->rtc, 1, RTC_IRQF | RTC_UF);
>> > +       if (status & RTC_INT_ALRM)
>> > +               rtc_update_irq(xrtcdev->rtc, 1, RTC_IRQF | RTC_AF);
>> > +
>> > +       return IRQ_HANDLED;
>> > +}
>> > +
>> > +static int xlnx_rtc_probe(struct platform_device *pdev)
>> > +{
>> > +       struct xlnx_rtc_dev *xrtcdev;
>> > +       struct resource *res;
>> > +       int ret;
>> > +       unsigned int calibvalue;
>> > +
>> > +       xrtcdev = devm_kzalloc(&pdev->dev, sizeof(*xrtcdev),
>> > GFP_KERNEL);
>> > +       if (!xrtcdev)
>> > +               return -ENOMEM;
>> > +
>> > +       platform_set_drvdata(pdev, xrtcdev);
>> > +
>> > +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> Do you want to check for return value here?
>> > +
>> > +       xrtcdev->reg_base = devm_ioremap_resource(&pdev->dev, res);
>> > +       if (IS_ERR(xrtcdev->reg_base))
>> > +               return PTR_ERR(xrtcdev->reg_base);
>> > +
>> > +       xrtcdev->alarm_irq = platform_get_irq_byname(pdev, "alarm");
>> > +       if (xrtcdev->alarm_irq < 0) {
>> > +               dev_err(&pdev->dev, "no irq resource\n");
>> > +               return xrtcdev->alarm_irq;
>> > +       }
>> > +       ret = devm_request_irq(&pdev->dev, xrtcdev->alarm_irq,
>> > +                              xlnx_rtc_interrupt, 0,
>> > +                              dev_name(&pdev->dev), xrtcdev);
>> > +       if (ret) {
>> > +               dev_err(&pdev->dev, "request irq failed\n");
>> > +               return ret;
>> > +       }
>> > +
>> > +       xrtcdev->sec_irq = platform_get_irq_byname(pdev, "sec");
>> > +       if (xrtcdev->sec_irq < 0) {
>> > +               dev_err(&pdev->dev, "no irq resource\n");
>> > +               return xrtcdev->sec_irq;
>> > +       }
>> > +       ret = devm_request_irq(&pdev->dev, xrtcdev->sec_irq,
>> > +                              xlnx_rtc_interrupt, 0,
>> > +                              dev_name(&pdev->dev), xrtcdev);
>> > +       if (ret) {
>> > +               dev_err(&pdev->dev, "request irq failed\n");
>> > +               return ret;
>> > +       }
>> > +
>> > +       ret = of_property_read_u32(pdev->dev.of_node, "calibration",
>> > +                                  &calibvalue);
>> > +       if (ret)
>> > +               calibvalue = RTC_CALIB_DEF;
>> > +
>> > +       xlnx_init_rtc(xrtcdev, calibvalue);
>> > +
>> > +       device_init_wakeup(&pdev->dev, 1);
>> > +
>> > +       xrtcdev->rtc = devm_rtc_device_register(&pdev->dev, pdev->name,
>> > +                                        &xlnx_rtc_ops, THIS_MODULE);
>> > +       return PTR_ERR_OR_ZERO(xrtcdev->rtc);
>> > +}
>> > +
>> > +static int xlnx_rtc_remove(struct platform_device *pdev)
>> > +{
>> > +       xlnx_rtc_alarm_irq_enable(&pdev->dev, 0);
>> > +       device_init_wakeup(&pdev->dev, 0);
>> > +
>> > +       return 0;
>> > +}
>> > +
>> > +static int __maybe_unused xlnx_rtc_suspend(struct device *dev)
>> > +{
>> > +       struct platform_device *pdev = to_platform_device(dev);
>> > +       struct xlnx_rtc_dev *xrtcdev = platform_get_drvdata(pdev);
>> > +
>> > +       if (device_may_wakeup(&pdev->dev))
>> > +               enable_irq_wake(xrtcdev->alarm_irq);
>> > +       else
>> > +               xlnx_rtc_alarm_irq_enable(dev, 0);
>> > +
>> > +       return 0;
>> > +}
>> > +
>> > +static int __maybe_unused xlnx_rtc_resume(struct device *dev)
>> > +{
>> > +       struct platform_device *pdev = to_platform_device(dev);
>> > +       struct xlnx_rtc_dev *xrtcdev = platform_get_drvdata(pdev);
>> > +
>> > +       if (device_may_wakeup(&pdev->dev))
>> > +               disable_irq_wake(xrtcdev->alarm_irq);
>> > +       else
>> > +               xlnx_rtc_alarm_irq_enable(dev, 1);
>> > +
>> > +       return 0;
>> > +}
>> > +
>> > +static SIMPLE_DEV_PM_OPS(xlnx_rtc_pm_ops, xlnx_rtc_suspend,
>> > xlnx_rtc_resume);
>> > +
>> > +static const struct of_device_id xlnx_rtc_of_match[] = {
>> > +       {.compatible = "xlnx,zynqmp-rtc" },
>> > +       { }
>> > +};
>> > +MODULE_DEVICE_TABLE(of, xlnx_rtc_of_match);
>> > +
>> > +static struct platform_driver xlnx_rtc_driver = {
>> > +       .probe          = xlnx_rtc_probe,
>> > +       .remove         = xlnx_rtc_remove,
>> > +       .driver         = {
>> > +               .name   = KBUILD_MODNAME,
>> > +               .pm     = &xlnx_rtc_pm_ops,
>> > +               .of_match_table = xlnx_rtc_of_match,
>> > +       },
>> > +};
>> > +
>> > +module_platform_driver(xlnx_rtc_driver);
>> > +
>> > +MODULE_DESCRIPTION("Xilinx Zynq MPSoC RTC driver");
>> > +MODULE_AUTHOR("Xilinx Inc.");
>> > +MODULE_LICENSE("GPL v2");
>> > --
>> > 2.1.2
>> >
>> >
>> > _______________________________________________
>> > linux-arm-kernel mailing list
>> > linux-ar...@xxxxxxxxxxxxxxxxxxx
>> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>
>> Cheers,
>>
>> Moritz

Cheers,

Moritz
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/