Re: [RFC PATCH] rtc/nxp: add FTM alarm driver as the wakeup source

[Alexandre Belloni]

Hi,

On 24/10/2016 at 09:42:16 +0800, Meng Yi wrote:
> For the platforms including LS1021A, LS1043A that has the
> flextimer module, implementing alarm functions within RTC
> subsystem to wakeup the system when system going to sleep.
> Only Ftm0 can be used to wakeup the system.
> 
> Signed-off-by: Meng Yi 
> ---
>  drivers/rtc/Kconfig   |   9 ++
>  drivers/rtc/Makefile  |   1 +
>  drivers/rtc/rtc-nxp-ftm.c | 255 
> ++
>  3 files changed, 265 insertions(+)
>  create mode 100644 drivers/rtc/rtc-nxp-ftm.c
> 
> +static const struct of_device_id nxp_ftm_rtc_of_match[] = {
> + {
> + .compatible = "fsl,ftm-clock",
> + }, {
> + /* sentinel */
> + }
> +};
> +MODULE_DEVICE_TABLE(of, nxp_ftm_rtc_of_match);
> 

I think the patch is generally fine but I think you will get an issue
later if anybody is trying to use an FTM to actually do something else
(e.g. PWM, counter, quad decoder,...).

I'm asking because there is exactly this issue on atmel SoCs.

I think we may be missing a proper subsystem.

> +static int nxp_ftm_rtc_probe(struct platform_device *pdev)
> +{
> + struct device_node *np = pdev->dev.of_node;
> + struct resource *r;
> + int irq;
> + int ret;
> +
> + rtc.alarm_freq = (u32)FIXED_FREQ_CLK / (u32)MAX_FREQ_DIV;
> +
> + r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!r)
> + return -ENODEV;
> +
> + rtc.base = devm_ioremap_resource(>dev, r);
> + if (IS_ERR(rtc.base))
> + return PTR_ERR(rtc.base);
> +
> + irq = irq_of_parse_and_map(np, 0);
> + if (irq <= 0) {
> + pr_err("ftm: unable to get IRQ from DT, %d\n", irq);
> + return -EINVAL;
> + }
> +
> + rtc.endian = of_property_read_bool(np, "big-endian");
> +
Doesn't the platform know the endianness, is it really necessary to have
that in the device tree?


-- 
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [RFC PATCH] rtc/nxp: add FTM alarm driver as the wakeup source

[Alexandre Belloni]

Hi,

On 24/10/2016 at 09:42:16 +0800, Meng Yi wrote:
> For the platforms including LS1021A, LS1043A that has the
> flextimer module, implementing alarm functions within RTC
> subsystem to wakeup the system when system going to sleep.
> Only Ftm0 can be used to wakeup the system.
> 
> Signed-off-by: Meng Yi 
> ---
>  drivers/rtc/Kconfig   |   9 ++
>  drivers/rtc/Makefile  |   1 +
>  drivers/rtc/rtc-nxp-ftm.c | 255 
> ++
>  3 files changed, 265 insertions(+)
>  create mode 100644 drivers/rtc/rtc-nxp-ftm.c
> 
> +static const struct of_device_id nxp_ftm_rtc_of_match[] = {
> + {
> + .compatible = "fsl,ftm-clock",
> + }, {
> + /* sentinel */
> + }
> +};
> +MODULE_DEVICE_TABLE(of, nxp_ftm_rtc_of_match);
> 

I think the patch is generally fine but I think you will get an issue
later if anybody is trying to use an FTM to actually do something else
(e.g. PWM, counter, quad decoder,...).

I'm asking because there is exactly this issue on atmel SoCs.

I think we may be missing a proper subsystem.

> +static int nxp_ftm_rtc_probe(struct platform_device *pdev)
> +{
> + struct device_node *np = pdev->dev.of_node;
> + struct resource *r;
> + int irq;
> + int ret;
> +
> + rtc.alarm_freq = (u32)FIXED_FREQ_CLK / (u32)MAX_FREQ_DIV;
> +
> + r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!r)
> + return -ENODEV;
> +
> + rtc.base = devm_ioremap_resource(>dev, r);
> + if (IS_ERR(rtc.base))
> + return PTR_ERR(rtc.base);
> +
> + irq = irq_of_parse_and_map(np, 0);
> + if (irq <= 0) {
> + pr_err("ftm: unable to get IRQ from DT, %d\n", irq);
> + return -EINVAL;
> + }
> +
> + rtc.endian = of_property_read_bool(np, "big-endian");
> +
Doesn't the platform know the endianness, is it really necessary to have
that in the device tree?


-- 
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

RE: [RFC PATCH] rtc/nxp: add FTM alarm driver as the wakeup source

[Meng Yi]

> > > +
> > > +static int nxp_ftm_rtc_read_time(struct device *dev, struct
> > > +rtc_time
> > > +*tm) {
> > > + struct timeval time;
> > > + unsigned long local_time;
> > > +
> > > + do_gettimeofday();
> > > + local_time = (u32)(time.tv_sec - (sys_tz.tz_minuteswest * 60));
> > > + rtc_time_to_tm(local_time, tm);
> > > +
> > > + return 0;
> > > +}
> > > +
> >
> > This is not really getting time from the RTC since FTM is not a RTC
> > device, But we need to get the time to setup alarm, so we are using system
> time for now.
> > Anybody have better idea?
> >
> 
> No, that seems fine to me. I'll review the rest of the driver.

Thanks for your review, any feedback is welcomed!

> 
> 
> --
> Alexandre Belloni, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com

Meng

RE: [RFC PATCH] rtc/nxp: add FTM alarm driver as the wakeup source

[Meng Yi]

> > > +
> > > +static int nxp_ftm_rtc_read_time(struct device *dev, struct
> > > +rtc_time
> > > +*tm) {
> > > + struct timeval time;
> > > + unsigned long local_time;
> > > +
> > > + do_gettimeofday();
> > > + local_time = (u32)(time.tv_sec - (sys_tz.tz_minuteswest * 60));
> > > + rtc_time_to_tm(local_time, tm);
> > > +
> > > + return 0;
> > > +}
> > > +
> >
> > This is not really getting time from the RTC since FTM is not a RTC
> > device, But we need to get the time to setup alarm, so we are using system
> time for now.
> > Anybody have better idea?
> >
> 
> No, that seems fine to me. I'll review the rest of the driver.

Thanks for your review, any feedback is welcomed!

> 
> 
> --
> Alexandre Belloni, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com

Meng

Re: [RFC PATCH] rtc/nxp: add FTM alarm driver as the wakeup source

[Alexandre Belloni]

On 02/11/2016 at 08:23:32 +, Meng Yi wrote :
> > +
> > +static int ftm_alarm_irq_enable(struct device *dev, unsigned int
> > +enabled) {
> > +   if (enabled)
> > +   ftm_irq_enable(true);
> > +   else
> > +   ftm_irq_enable(false);
> > +
> > +   return 0;
> > +}
> > +
> > +static int nxp_ftm_rtc_read_time(struct device *dev, struct rtc_time
> > +*tm) {
> > +   struct timeval time;
> > +   unsigned long local_time;
> > +
> > +   do_gettimeofday();
> > +   local_time = (u32)(time.tv_sec - (sys_tz.tz_minuteswest * 60));
> > +   rtc_time_to_tm(local_time, tm);
> > +
> > +   return 0;
> > +}
> > +
> 
> This is not really getting time from the RTC since FTM is not a RTC device,
> But we need to get the time to setup alarm, so we are using system time for 
> now.
> Anybody have better idea?
> 

No, that seems fine to me. I'll review the rest of the driver.


-- 
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [RFC PATCH] rtc/nxp: add FTM alarm driver as the wakeup source

[Alexandre Belloni]

On 02/11/2016 at 08:23:32 +, Meng Yi wrote :
> > +
> > +static int ftm_alarm_irq_enable(struct device *dev, unsigned int
> > +enabled) {
> > +   if (enabled)
> > +   ftm_irq_enable(true);
> > +   else
> > +   ftm_irq_enable(false);
> > +
> > +   return 0;
> > +}
> > +
> > +static int nxp_ftm_rtc_read_time(struct device *dev, struct rtc_time
> > +*tm) {
> > +   struct timeval time;
> > +   unsigned long local_time;
> > +
> > +   do_gettimeofday();
> > +   local_time = (u32)(time.tv_sec - (sys_tz.tz_minuteswest * 60));
> > +   rtc_time_to_tm(local_time, tm);
> > +
> > +   return 0;
> > +}
> > +
> 
> This is not really getting time from the RTC since FTM is not a RTC device,
> But we need to get the time to setup alarm, so we are using system time for 
> now.
> Anybody have better idea?
> 

No, that seems fine to me. I'll review the rest of the driver.


-- 
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

RE: [RFC PATCH] rtc/nxp: add FTM alarm driver as the wakeup source

[Meng Yi]

> +
> +static int ftm_alarm_irq_enable(struct device *dev, unsigned int
> +enabled) {
> + if (enabled)
> + ftm_irq_enable(true);
> + else
> + ftm_irq_enable(false);
> +
> + return 0;
> +}
> +
> +static int nxp_ftm_rtc_read_time(struct device *dev, struct rtc_time
> +*tm) {
> + struct timeval time;
> + unsigned long local_time;
> +
> + do_gettimeofday();
> + local_time = (u32)(time.tv_sec - (sys_tz.tz_minuteswest * 60));
> + rtc_time_to_tm(local_time, tm);
> +
> + return 0;
> +}
> +

This is not really getting time from the RTC since FTM is not a RTC device,
But we need to get the time to setup alarm, so we are using system time for now.
Anybody have better idea?

Meng

RE: [RFC PATCH] rtc/nxp: add FTM alarm driver as the wakeup source

[Meng Yi]

> +
> +static int ftm_alarm_irq_enable(struct device *dev, unsigned int
> +enabled) {
> + if (enabled)
> + ftm_irq_enable(true);
> + else
> + ftm_irq_enable(false);
> +
> + return 0;
> +}
> +
> +static int nxp_ftm_rtc_read_time(struct device *dev, struct rtc_time
> +*tm) {
> + struct timeval time;
> + unsigned long local_time;
> +
> + do_gettimeofday();
> + local_time = (u32)(time.tv_sec - (sys_tz.tz_minuteswest * 60));
> + rtc_time_to_tm(local_time, tm);
> +
> + return 0;
> +}
> +

This is not really getting time from the RTC since FTM is not a RTC device,
But we need to get the time to setup alarm, so we are using system time for now.
Anybody have better idea?

Meng