Re: [rtc-linux] [PATCH 2/2] rtc: mediatek: Add MT63xx RTC driver

Re: [rtc-linux] [PATCH 2/2] rtc: mediatek: Add MT63xx RTC driver

[Andrew Morton]

On Wed, 28 Jan 2015 17:27:56 +0800 Eddie Huang <eddie.huang@mediatek.com> wrote:

> From: Tianping Fang <tianping.fang@mediatek.com>
> 
> Add Mediatek MT63xx RTC driver

There are a couple of checkpatch warnings which should be addressed,
please:

WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#150: 
new file mode 100644

WARNING: DT compatible string "mediatek,mt6397-rtc" appears un-documented -- check ./Documentation/devicetree/bindings/
#488: FILE: drivers/rtc/rtc-mt6397.c:334:
+	{ .compatible = "mediatek,mt6397-rtc", },




>
> ...
>
> +static u16 rtc_read(struct mt6397_rtc *rtc, u32 offset)
> +{
> +	u32 rdata = 0;
> +	u32 addr = rtc->addr_base + offset;
> +
> +	if (offset < rtc->addr_range)
> +		regmap_read(rtc->regmap, addr, &rdata);
> +
> +	return (u16)rdata;
> +}
> +
> +static void rtc_write(struct mt6397_rtc *rtc, u32 offset, u32 data)
> +{
> +	u32 addr;
> +
> +	addr = rtc->addr_base + offset;
> +
> +	if (offset < rtc->addr_range)
> +		regmap_write(rtc->regmap, addr, data);
> +}

regmap_read() and regmap_write() can return errors.  There is no
checking for this.

> +static void rtc_write_trigger(struct mt6397_rtc *rtc)
> +{
> +	rtc_write(rtc, RTC_WRTGR, 1);
> +	while (rtc_read(rtc, RTC_BBPU) & RTC_BBPU_CBUSY)
> +		cpu_relax();
> +}
> +
>
> ...
>
> +static int mtk_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alm)
> +{
> +	struct rtc_time *tm = &alm->time;
> +	struct mt6397_rtc *rtc = dev_get_drvdata(dev);
> +	u16 irqen, pdn2;
> +
> +	mutex_lock(&rtc->lock);
> +	irqen = rtc_read(rtc, RTC_IRQ_EN);
> +	pdn2 = rtc_read(rtc, RTC_PDN2);
> +	tm->tm_sec  = rtc_read(rtc, RTC_AL_SEC);
> +	tm->tm_min  = rtc_read(rtc, RTC_AL_MIN);
> +	tm->tm_hour = rtc_read(rtc, RTC_AL_HOU) & RTC_AL_HOU_MASK;
> +	tm->tm_mday = rtc_read(rtc, RTC_AL_DOM) & RTC_AL_DOM_MASK;
> +	tm->tm_mon  = rtc_read(rtc, RTC_AL_MTH) & RTC_AL_MTH_MASK;
> +	tm->tm_year = rtc_read(rtc, RTC_AL_YEA);
> +	mutex_unlock(&rtc->lock);
> +
> +	alm->enabled = !!(irqen & RTC_IRQ_EN_AL);
> +	alm->pending = !!(pdn2 & RTC_PDN2_PWRON_ALARM);
> +
> +	tm->tm_year += RTC_MIN_YEAR_OFFSET;
> +	tm->tm_mon--;
> +
> +	return 0;
> +}
> +
> +static int mtk_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alm)
> +{
> +	struct rtc_time *tm = &alm->time;
> +	struct mt6397_rtc *rtc = dev_get_drvdata(dev);
> +	u16 irqen;
> +
> +	tm->tm_year -= RTC_MIN_YEAR_OFFSET;
> +	tm->tm_mon++;
> +
> +	if (alm->enabled) {
> +		mutex_lock(&rtc->lock);
> +		rtc_write(rtc, RTC_AL_YEA, tm->tm_year);
> +		rtc_write(rtc, RTC_AL_MTH, (rtc_read(rtc, RTC_AL_MTH) &
> +				RTC_NEW_SPARE3) | tm->tm_mon);
> +		rtc_write(rtc, RTC_AL_DOM, (rtc_read(rtc, RTC_AL_DOM) &
> +				RTC_NEW_SPARE1) | tm->tm_mday);
> +		rtc_write(rtc, RTC_AL_HOU, (rtc_read(rtc, RTC_AL_HOU) &
> +				RTC_NEW_SPARE_FG_MASK) | tm->tm_hour);
> +		rtc_write(rtc, RTC_AL_MIN, tm->tm_min);
> +		rtc_write(rtc, RTC_AL_SEC, tm->tm_sec);
> +		rtc_write(rtc, RTC_AL_MASK, RTC_AL_MASK_DOW);
> +		rtc_write_trigger(rtc);
> +		irqen = rtc_read(rtc, RTC_IRQ_EN) | RTC_IRQ_EN_ONESHOT_AL;
> +		rtc_write(rtc, RTC_IRQ_EN, irqen);
> +		rtc_write_trigger(rtc);
> +		mutex_unlock(&rtc->lock);
> +	}
> +
> +	return 0;
> +}

This all looks a bit racy.  Wouldn't it be better if the testing of and
modification of ->enabled and ->pending were protected by the mutex?

>
> ...
>
> +static int mtk_rtc_probe(struct platform_device *pdev)
> +{
> +	struct mt6397_chip *mt6397_chip = dev_get_drvdata(pdev->dev.parent);
> +	struct mt6397_rtc *rtc;
> +	u32 reg[2];
> +	int ret = 0;
> +
> +	rtc = devm_kzalloc(&pdev->dev, sizeof(struct mt6397_rtc), GFP_KERNEL);
> +	if (!rtc)
> +		return -ENOMEM;
> +
> +	ret = of_property_read_u32_array(pdev->dev.of_node, "reg", reg, 2);
> +	if (ret) {
> +		dev_err(&pdev->dev, "couldn't read rtc base address!\n");
> +		return -EINVAL;
> +	}
> +	rtc->addr_base = reg[0];
> +	rtc->addr_range = reg[1];
> +	rtc->regmap = mt6397_chip->regmap;
> +	rtc->dev = &pdev->dev;
> +	mutex_init(&rtc->lock);
> +
> +	platform_set_drvdata(pdev, rtc);
> +
> +	rtc->rtc_dev = rtc_device_register("mt6397-rtc", &pdev->dev,
> +				&mtk_rtc_ops, THIS_MODULE);
> +	if (IS_ERR(rtc->rtc_dev)) {
> +		dev_err(&pdev->dev, "register rtc device failed\n");
> +		return PTR_ERR(rtc->rtc_dev);
> +	}
> +
> +	rtc->irq = platform_get_irq(pdev, 0);
> +	if (rtc->irq < 0) {
> +		ret = rtc->irq;
> +		goto out_rtc;
> +	}
> +
> +	ret = devm_request_threaded_irq(&pdev->dev, rtc->irq, NULL,
> +			rtc_irq_handler_thread, IRQF_ONESHOT,
> +			"mt6397-rtc", rtc);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Failed to request alarm IRQ: %d: %d\n",
> +			rtc->irq, ret);
> +		goto out_rtc;
> +	}
> +
> +	device_init_wakeup(&pdev->dev, 1);
> +
> +	return 0;
> +
> +out_rtc:
> +	rtc_device_unregister(rtc->rtc_dev);
> +	return ret;
> +
> +}

It seems strange to request the IRQ after having registered the rtc. 
And possibly racy - I seem to recall another driver having issues with
this recently.

A lot of rtc drivers are requesting the IRQ after registration so
presumably it isn't a huge problem.  But still, wouldn't it be better
to defer registration until after the IRQ has been successfully
obtained?

>
> ...
>

Re: [rtc-linux] [PATCH 2/2] rtc: mediatek: Add MT63xx RTC driver

[Eddie Huang]

Hi Andrew,

On Mon, 2015-02-23 at 13:50 -0800, Andrew Morton wrote:
> On Wed, 28 Jan 2015 17:27:56 +0800 Eddie Huang <eddie.huang@mediatek.com> wrote:
> 
> > From: Tianping Fang <tianping.fang@mediatek.com>
> > 
> > Add Mediatek MT63xx RTC driver
> 
> There are a couple of checkpatch warnings which should be addressed,
> please:
> 
> WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
> #150: 
> new file mode 100644
> 
OK, I will update MAINTAINERS file

> WARNING: DT compatible string "mediatek,mt6397-rtc" appears un-documented -- check ./Documentation/devicetree/bindings/
> #488: FILE: drivers/rtc/rtc-mt6397.c:334:
> +	{ .compatible = "mediatek,mt6397-rtc", },
> 
> 
Do you include patch "[PATCH 1/2] dt: bindings: Add Mediatek RTC driver
binding document" in this series ?
http://lists.infradead.org/pipermail/linux-arm-kernel/2015-January/320425.html

I think this warning shouldn't happen if include this patch.


> >
> > ...
> >
> > +static void rtc_write(struct mt6397_rtc *rtc, u32 offset, u32 data)
> > +{
> > +	u32 addr;
> > +
> > +	addr = rtc->addr_base + offset;
> > +
> > +	if (offset < rtc->addr_range)
> > +		regmap_write(rtc->regmap, addr, data);
> > +}
> 
> regmap_read() and regmap_write() can return errors.  There is no
> checking for this.
Will fix it.

> > ...
> >
> > +static int mtk_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alm)
> > +{
> > +	struct rtc_time *tm = &alm->time;
> > +	struct mt6397_rtc *rtc = dev_get_drvdata(dev);
> > +	u16 irqen, pdn2;
> > +
> > +	mutex_lock(&rtc->lock);
> > +	irqen = rtc_read(rtc, RTC_IRQ_EN);
> > +	pdn2 = rtc_read(rtc, RTC_PDN2);
> > +	tm->tm_sec  = rtc_read(rtc, RTC_AL_SEC);
> > +	tm->tm_min  = rtc_read(rtc, RTC_AL_MIN);
> > +	tm->tm_hour = rtc_read(rtc, RTC_AL_HOU) & RTC_AL_HOU_MASK;
> > +	tm->tm_mday = rtc_read(rtc, RTC_AL_DOM) & RTC_AL_DOM_MASK;
> > +	tm->tm_mon  = rtc_read(rtc, RTC_AL_MTH) & RTC_AL_MTH_MASK;
> > +	tm->tm_year = rtc_read(rtc, RTC_AL_YEA);
> > +	mutex_unlock(&rtc->lock);
> > +
> > +	alm->enabled = !!(irqen & RTC_IRQ_EN_AL);
> > +	alm->pending = !!(pdn2 & RTC_PDN2_PWRON_ALARM);
> > +
> > +	tm->tm_year += RTC_MIN_YEAR_OFFSET;
> > +	tm->tm_mon--;
> > +
> > +	return 0;
> > +}
> > +
> > +static int mtk_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alm)
> > +{
> > +	struct rtc_time *tm = &alm->time;
> > +	struct mt6397_rtc *rtc = dev_get_drvdata(dev);
> > +	u16 irqen;
> > +
> > +	tm->tm_year -= RTC_MIN_YEAR_OFFSET;
> > +	tm->tm_mon++;
> > +
> > +	if (alm->enabled) {
> > +		mutex_lock(&rtc->lock);
> > +		rtc_write(rtc, RTC_AL_YEA, tm->tm_year);
> > +		rtc_write(rtc, RTC_AL_MTH, (rtc_read(rtc, RTC_AL_MTH) &
> > +				RTC_NEW_SPARE3) | tm->tm_mon);
> > +		rtc_write(rtc, RTC_AL_DOM, (rtc_read(rtc, RTC_AL_DOM) &
> > +				RTC_NEW_SPARE1) | tm->tm_mday);
> > +		rtc_write(rtc, RTC_AL_HOU, (rtc_read(rtc, RTC_AL_HOU) &
> > +				RTC_NEW_SPARE_FG_MASK) | tm->tm_hour);
> > +		rtc_write(rtc, RTC_AL_MIN, tm->tm_min);
> > +		rtc_write(rtc, RTC_AL_SEC, tm->tm_sec);
> > +		rtc_write(rtc, RTC_AL_MASK, RTC_AL_MASK_DOW);
> > +		rtc_write_trigger(rtc);
> > +		irqen = rtc_read(rtc, RTC_IRQ_EN) | RTC_IRQ_EN_ONESHOT_AL;
> > +		rtc_write(rtc, RTC_IRQ_EN, irqen);
> > +		rtc_write_trigger(rtc);
> > +		mutex_unlock(&rtc->lock);
> > +	}
> > +
> > +	return 0;
> > +}
> 
> This all looks a bit racy.  Wouldn't it be better if the testing of and
> modification of ->enabled and ->pending were protected by the mutex?
> 
Will fix it.

> >
> > ...
> >
> > +static int mtk_rtc_probe(struct platform_device *pdev)
> > +{
> > +	struct mt6397_chip *mt6397_chip = dev_get_drvdata(pdev->dev.parent);
> > +	struct mt6397_rtc *rtc;
> > +	u32 reg[2];
> > +	int ret = 0;
> > +
> > +	rtc = devm_kzalloc(&pdev->dev, sizeof(struct mt6397_rtc), GFP_KERNEL);
> > +	if (!rtc)
> > +		return -ENOMEM;
> > +
> > +	ret = of_property_read_u32_array(pdev->dev.of_node, "reg", reg, 2);
> > +	if (ret) {
> > +		dev_err(&pdev->dev, "couldn't read rtc base address!\n");
> > +		return -EINVAL;
> > +	}
> > +	rtc->addr_base = reg[0];
> > +	rtc->addr_range = reg[1];
> > +	rtc->regmap = mt6397_chip->regmap;
> > +	rtc->dev = &pdev->dev;
> > +	mutex_init(&rtc->lock);
> > +
> > +	platform_set_drvdata(pdev, rtc);
> > +
> > +	rtc->rtc_dev = rtc_device_register("mt6397-rtc", &pdev->dev,
> > +				&mtk_rtc_ops, THIS_MODULE);
> > +	if (IS_ERR(rtc->rtc_dev)) {
> > +		dev_err(&pdev->dev, "register rtc device failed\n");
> > +		return PTR_ERR(rtc->rtc_dev);
> > +	}
> > +
> > +	rtc->irq = platform_get_irq(pdev, 0);
> > +	if (rtc->irq < 0) {
> > +		ret = rtc->irq;
> > +		goto out_rtc;
> > +	}
> > +
> > +	ret = devm_request_threaded_irq(&pdev->dev, rtc->irq, NULL,
> > +			rtc_irq_handler_thread, IRQF_ONESHOT,
> > +			"mt6397-rtc", rtc);
> > +	if (ret) {
> > +		dev_err(&pdev->dev, "Failed to request alarm IRQ: %d: %d\n",
> > +			rtc->irq, ret);
> > +		goto out_rtc;
> > +	}
> > +
> > +	device_init_wakeup(&pdev->dev, 1);
> > +
> > +	return 0;
> > +
> > +out_rtc:
> > +	rtc_device_unregister(rtc->rtc_dev);
> > +	return ret;
> > +
> > +}
> 
> It seems strange to request the IRQ after having registered the rtc. 
> And possibly racy - I seem to recall another driver having issues with
> this recently.
> 
> A lot of rtc drivers are requesting the IRQ after registration so
> presumably it isn't a huge problem.  But still, wouldn't it be better
> to defer registration until after the IRQ has been successfully
> obtained?
> 
OK, will fix it.

Eddie

Re: [rtc-linux] [PATCH 2/2] rtc: mediatek: Add MT63xx RTC driver

[Andrew Morton]

On Mon, 2 Mar 2015 16:20:36 +0800 Eddie Huang <eddie.huang@mediatek.com> wrote:

> > WARNING: DT compatible string "mediatek,mt6397-rtc" appears un-documented -- check ./Documentation/devicetree/bindings/
> > #488: FILE: drivers/rtc/rtc-mt6397.c:334:
> > +	{ .compatible = "mediatek,mt6397-rtc", },
> > 
> > 
> Do you include patch "[PATCH 1/2] dt: bindings: Add Mediatek RTC driver
> binding document" in this series ?
> http://lists.infradead.org/pipermail/linux-arm-kernel/2015-January/320425.html
> 
> I think this warning shouldn't happen if include this patch.

I can't find that patch on rtc-linux or linux-kernel.  Resend, please?

Re: [rtc-linux] [PATCH 2/2] rtc: mediatek: Add MT63xx RTC driver

[Eddie Huang]

Hi,

On Mon, 2015-02-23 at 13:50 -0800, Andrew Morton wrote:
> On Wed, 28 Jan 2015 17:27:56 +0800 Eddie Huang <eddie.huang@mediatek.com> wrote:
> 
> > From: Tianping Fang <tianping.fang@mediatek.com>
> > 
> > Add Mediatek MT63xx RTC driver
> 
> There are a couple of checkpatch warnings which should be addressed,
> please:
> 
> WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
> #150: 
> new file mode 100644
> 
> WARNING: DT compatible string "mediatek,mt6397-rtc" appears un-documented -- check ./Documentation/devicetree/bindings/
> #488: FILE: drivers/rtc/rtc-mt6397.c:334:
> +	{ .compatible = "mediatek,mt6397-rtc", },
> 
> 
> 
> 
> >
> > ...
> >
> > +static u16 rtc_read(struct mt6397_rtc *rtc, u32 offset)
> > +{
> > +	u32 rdata = 0;
> > +	u32 addr = rtc->addr_base + offset;
> > +
> > +	if (offset < rtc->addr_range)
> > +		regmap_read(rtc->regmap, addr, &rdata);
> > +
> > +	return (u16)rdata;
> > +}
> > +
> > +static void rtc_write(struct mt6397_rtc *rtc, u32 offset, u32 data)
> > +{
> > +	u32 addr;
> > +
> > +	addr = rtc->addr_base + offset;
> > +
> > +	if (offset < rtc->addr_range)
> > +		regmap_write(rtc->regmap, addr, data);
> > +}
> 
> regmap_read() and regmap_write() can return errors.  There is no
> checking for this.
> 

I encounter some trouble when I add code to check return value of
regmap_read and regmap_write. Every RTC register access through regmap,
and there are many register read/write in this driver. If I check every
return value, the driver will become ugly. I try to make this driver
clean using following macro.

static int __rtc_read(struct mt6397_rtc *rtc, u32 offset, u32 *data)
{
        u32 addr = rtc->addr_base + offset;

        if (offset < rtc->addr_range)
                return regmap_read(rtc->regmap, addr, data);

        return -EINVAL;
}

#define rtc_read(ret, rtc, offset, data)                \
({                                                      \
        ret = __rtc_read(rtc, offset, data);            \
        if (ret < 0)                                    \
                goto rtc_exit;                          \
})                                                      \


And function call rtc_read, rtc_write looks like:

static int mtk_rtc_read_time(struct device *dev, struct rtc_time *tm)
{
        unsigned long time;
        struct mt6397_rtc *rtc = dev_get_drvdata(dev);
        int ret = 0;
        u32 sec;

        mutex_lock(&rtc->lock);
        do {
                rtc_read(ret, rtc, RTC_TC_SEC, &tm->tm_sec);
                rtc_read(ret, rtc, RTC_TC_MIN, &tm->tm_min);
                rtc_read(ret, rtc, RTC_TC_HOU, &tm->tm_hour);
                rtc_read(ret, rtc, RTC_TC_DOM, &tm->tm_mday);
                rtc_read(ret, rtc, RTC_TC_MTH, &tm->tm_mon);
                rtc_read(ret, rtc, RTC_TC_YEA, &tm->tm_year);
                rtc_read(ret, rtc, RTC_TC_SEC, &sec);
        } while (sec < tm->tm_sec);
        mutex_unlock(&rtc->lock);

        tm->tm_year += RTC_MIN_YEAR_OFFSET;
        tm->tm_mon--;
        rtc_tm_to_time(tm, &time);

        tm->tm_wday = (time / 86400 + 4) % 7;

        return 0;

rtc_exit:
        mutex_unlock(&rtc->lock);
        return ret;
}

It's a little tricky, does anyone have good suggestion ?

Eddie

Re: [rtc-linux] [PATCH 2/2] rtc: mediatek: Add MT63xx RTC driver

[Sascha Hauer]

Hi Eddie,

On Fri, Mar 13, 2015 at 06:29:23PM +0800, Eddie Huang wrote:
> > regmap_read() and regmap_write() can return errors.  There is no
> > checking for this.
> > 
> 
> I encounter some trouble when I add code to check return value of
> regmap_read and regmap_write. Every RTC register access through regmap,
> and there are many register read/write in this driver. If I check every
> return value, the driver will become ugly. I try to make this driver
> clean using following macro.
> 
> static int __rtc_read(struct mt6397_rtc *rtc, u32 offset, u32 *data)
> {
>         u32 addr = rtc->addr_base + offset;
> 
>         if (offset < rtc->addr_range)
>                 return regmap_read(rtc->regmap, addr, data);
> 
>         return -EINVAL;
> }
> 
> #define rtc_read(ret, rtc, offset, data)                \
> ({                                                      \
>         ret = __rtc_read(rtc, offset, data);            \
>         if (ret < 0)                                    \
>                 goto rtc_exit;                          \
> })                                                      \

Hiding a goto (or return) in a macro is a very bad idea.

what you can do is

	ret |= regmap_read(rtc->regmap, RTC_TC_SEC, &tm->tm_sec);
	ret |= regmap_read(rtc->regmap, RTC_TC_MIN, &tm->tm_min);

	if (ret)
		return -EIO;

(Don't return ret in this case though as it might contain different
error codes orred together)

Another possibilty at least for contiguous registers would be
regmap_bulk_read().

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [rtc-linux] [PATCH 2/2] rtc: mediatek: Add MT63xx RTC driver

[Eddie Huang]

Hi Sascha,

On Fri, 2015-03-13 at 11:57 +0100, Sascha Hauer wrote:
> Hi Eddie,
> 
> On Fri, Mar 13, 2015 at 06:29:23PM +0800, Eddie Huang wrote:
> > > regmap_read() and regmap_write() can return errors.  There is no
> > > checking for this.
> > > 
> > 
> > I encounter some trouble when I add code to check return value of
> > regmap_read and regmap_write. Every RTC register access through regmap,
> > and there are many register read/write in this driver. If I check every
> > return value, the driver will become ugly. I try to make this driver
> > clean using following macro.
> > 
> > static int __rtc_read(struct mt6397_rtc *rtc, u32 offset, u32 *data)
> > {
> >         u32 addr = rtc->addr_base + offset;
> > 
> >         if (offset < rtc->addr_range)
> >                 return regmap_read(rtc->regmap, addr, data);
> > 
> >         return -EINVAL;
> > }
> > 
> > #define rtc_read(ret, rtc, offset, data)                \
> > ({                                                      \
> >         ret = __rtc_read(rtc, offset, data);            \
> >         if (ret < 0)                                    \
> >                 goto rtc_exit;                          \
> > })                                                      \
> 
> Hiding a goto (or return) in a macro is a very bad idea.
> 
> what you can do is
> 
> 	ret |= regmap_read(rtc->regmap, RTC_TC_SEC, &tm->tm_sec);
> 	ret |= regmap_read(rtc->regmap, RTC_TC_MIN, &tm->tm_min);
> 
> 	if (ret)
> 		return -EIO;
> 
> (Don't return ret in this case though as it might contain different
> error codes orred together)
> 

OK, I will drop macro, and check regmap_read, regmap_write return value
in each function.

> Another possibilty at least for contiguous registers would be
> regmap_bulk_read().
> 
> Sascha
> 

Contiguous registers access occurs in reading and writing time. I think
Matthias's suggestion is a good way: 

do {
	ret = __mtk_rtc_read_time(rtc, &tm, &sec);
	if (ret < 0)
		goto rtc_exit;
} while (sec < tm->tm_sec);

Eddie

Re: [rtc-linux] [PATCH 2/2] rtc: mediatek: Add MT63xx RTC driver

[Matthias Brugger]

On 13/03/15 11:29, Eddie Huang wrote:
> 
> I encounter some trouble when I add code to check return value of
> regmap_read and regmap_write. Every RTC register access through regmap,
> and there are many register read/write in this driver. If I check every
> return value, the driver will become ugly. I try to make this driver
> clean using following macro.
> 
> static int __rtc_read(struct mt6397_rtc *rtc, u32 offset, u32 *data)
> {
>         u32 addr = rtc->addr_base + offset;
> 
>         if (offset < rtc->addr_range)
>                 return regmap_read(rtc->regmap, addr, data);
> 
>         return -EINVAL;
> }
> 
> #define rtc_read(ret, rtc, offset, data)                \
> ({                                                      \
>         ret = __rtc_read(rtc, offset, data);            \
>         if (ret < 0)                                    \
>                 goto rtc_exit;                          \
> })                                                      \
> 

I agree with Sascha on hiding a goto statement in a macro is not a good idea.

> 
> And function call rtc_read, rtc_write looks like:
> 
> static int mtk_rtc_read_time(struct device *dev, struct rtc_time *tm)
> {
>         unsigned long time;
>         struct mt6397_rtc *rtc = dev_get_drvdata(dev);
>         int ret = 0;
>         u32 sec;
> 
>         mutex_lock(&rtc->lock);
>         do {
>                 rtc_read(ret, rtc, RTC_TC_SEC, &tm->tm_sec);
>                 rtc_read(ret, rtc, RTC_TC_MIN, &tm->tm_min);
>                 rtc_read(ret, rtc, RTC_TC_HOU, &tm->tm_hour);
>                 rtc_read(ret, rtc, RTC_TC_DOM, &tm->tm_mday);
>                 rtc_read(ret, rtc, RTC_TC_MTH, &tm->tm_mon);
>                 rtc_read(ret, rtc, RTC_TC_YEA, &tm->tm_year);
>                 rtc_read(ret, rtc, RTC_TC_SEC, &sec);
>         } while (sec < tm->tm_sec);

What about introducing
static int __mtk_rtc_read_time(struct mt6397_rtc *rtc, struct rtc_time *tm, u32 *sec)
and hide the checks of return values from regmap_read and the offset check in there. You return the error code or 0.

This way the while loop would look like this:

do {
	ret = __mtk_rtc_read_time(rtc, &tm, &sec);
	if (ret < 0)
		goto rtc_exit;
} while (sec < tm->tm_sec);

Best regards,
Matthias

Re: [PATCH 2/2] rtc: mediatek: Add MT63xx RTC driver

[Uwe Kleine-König]

Hello Eddie,

On Wed, Jan 28, 2015 at 05:27:56PM +0800, Eddie Huang wrote:
> From: Tianping Fang <tianping.fang@mediatek.com>
> 
> Add Mediatek MT63xx RTC driver
MT6397?

> diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
> index f15cddf..8ac52d8 100644
> --- a/drivers/rtc/Kconfig
> +++ b/drivers/rtc/Kconfig
> @@ -1427,6 +1427,16 @@ config RTC_DRV_MOXART
>  	   This driver can also be built as a module. If so, the module
>  	   will be called rtc-moxart
>  
> +config RTC_DRV_MT63XX
> +	tristate "Mediatek Real Time Clock driver"
> +	depends on MFD_MT6397
I suggest:

	depends on MFD_MT6397 || COMPILE_TEST

(maybe + any hard dependencies you need for compilation).

> +	help
> +	  This selects the Mediatek(R) RTC driver, you should add support
> +	  for Mediatek MT6397 PMIC before select Mediatek(R) RTC driver.
> +
> +	  If you want to use Mediatek(R) RTC interface, select Y or M here.
> +	  If unsure, Please select N.
Given the dependency above I'd say choosing y here is fine. Instead of
recommending that I'd just drop this line.

> [...]
> +static u16 rtc_read(struct mt6397_rtc *rtc, u32 offset)
rtc_read is a bad name for a driver. There are already 6 functions with
this name in the kernel. Better use a unique prefix.

> [...]
> +static irqreturn_t rtc_irq_handler_thread(int irq, void *data)
> +{
> +	struct mt6397_rtc *rtc = data;
> +	u16 irqsta, irqen;
> +
> +	mutex_lock(&rtc->lock);
> +	irqsta = rtc_read(rtc, RTC_IRQ_STA);
Do you really need to lock for a single read access?

> +	mutex_unlock(&rtc->lock);
> +
> +	if (irqsta & RTC_IRQ_STA_AL) {
> +		rtc_update_irq(rtc->rtc_dev, 1, RTC_IRQF | RTC_AF);
> +		irqen = irqsta & ~RTC_IRQ_EN_AL;
> +		rtc_write(rtc, RTC_IRQ_EN, irqen);
> +		rtc_write_trigger(rtc);
> +		return IRQ_HANDLED;
> +	}
> +
> +	return IRQ_NONE;
> +}
> +
> +static int mtk_rtc_read_time(struct device *dev, struct rtc_time *tm)
> +{
> +	unsigned long time;
> +	struct mt6397_rtc *rtc = dev_get_drvdata(dev);
> +
> +	mutex_lock(&rtc->lock);
> +	do {
> +		tm->tm_sec = rtc_read(rtc, RTC_TC_SEC);
> +		tm->tm_min = rtc_read(rtc, RTC_TC_MIN);
> +		tm->tm_hour = rtc_read(rtc, RTC_TC_HOU);
> +		tm->tm_mday = rtc_read(rtc, RTC_TC_DOM);
> +		tm->tm_mon = rtc_read(rtc, RTC_TC_MTH);
> +		tm->tm_year = rtc_read(rtc, RTC_TC_YEA);
> +	} while (rtc_read(rtc, RTC_TC_SEC) < tm->tm_sec);
> +	mutex_unlock(&rtc->lock);
> +
> +	tm->tm_year += RTC_MIN_YEAR_OFFSET;
> +	tm->tm_mon--;
> +	rtc_tm_to_time(tm, &time);
rtc_tm_to_time is deprecated, better use rtc_tm_to_time64.

> +	tm->tm_wday = (time / 86400 + 4) % 7;
> +
> +	return 0;
> +}
> +
> +static int mtk_rtc_set_time(struct device *dev, struct rtc_time *tm)
> +{
> +	struct mt6397_rtc *rtc = dev_get_drvdata(dev);
> +
> +	tm->tm_year -= RTC_MIN_YEAR_OFFSET;
> +	tm->tm_mon++;
> +	mutex_lock(&rtc->lock);
> +	rtc_write(rtc, RTC_TC_YEA, tm->tm_year);
> +	rtc_write(rtc, RTC_TC_MTH, tm->tm_mon);
> +	rtc_write(rtc, RTC_TC_DOM, tm->tm_mday);
> +	rtc_write(rtc, RTC_TC_HOU, tm->tm_hour);
> +	rtc_write(rtc, RTC_TC_MIN, tm->tm_min);
> +	rtc_write(rtc, RTC_TC_SEC, tm->tm_sec);
Is this racy? I.e. what happens if RTC_TC_SEC overflows just before you
write to it but after you wrote RTC_TC_MIN?

> +	rtc_write_trigger(rtc);
> +	mutex_unlock(&rtc->lock);
> +
> +	return 0;
> +}
> +
> +static int mtk_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alm)
> +{
> +	struct rtc_time *tm = &alm->time;
> +	struct mt6397_rtc *rtc = dev_get_drvdata(dev);
> +	u16 irqen, pdn2;
> +
> +	mutex_lock(&rtc->lock);
> +	irqen = rtc_read(rtc, RTC_IRQ_EN);
> +	pdn2 = rtc_read(rtc, RTC_PDN2);
> +	tm->tm_sec  = rtc_read(rtc, RTC_AL_SEC);
> +	tm->tm_min  = rtc_read(rtc, RTC_AL_MIN);
> +	tm->tm_hour = rtc_read(rtc, RTC_AL_HOU) & RTC_AL_HOU_MASK;
> +	tm->tm_mday = rtc_read(rtc, RTC_AL_DOM) & RTC_AL_DOM_MASK;
> +	tm->tm_mon  = rtc_read(rtc, RTC_AL_MTH) & RTC_AL_MTH_MASK;
> +	tm->tm_year = rtc_read(rtc, RTC_AL_YEA);
> +	mutex_unlock(&rtc->lock);
> +
> +	alm->enabled = !!(irqen & RTC_IRQ_EN_AL);
> +	alm->pending = !!(pdn2 & RTC_PDN2_PWRON_ALARM);
> +
> +	tm->tm_year += RTC_MIN_YEAR_OFFSET;
> +	tm->tm_mon--;
> +
> +	return 0;
> +}
> +
> +static int mtk_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alm)
> +{
> +	struct rtc_time *tm = &alm->time;
> +	struct mt6397_rtc *rtc = dev_get_drvdata(dev);
> +	u16 irqen;
> +
> +	tm->tm_year -= RTC_MIN_YEAR_OFFSET;
> +	tm->tm_mon++;
> +
> +	if (alm->enabled) {
> +		mutex_lock(&rtc->lock);
> +		rtc_write(rtc, RTC_AL_YEA, tm->tm_year);
> +		rtc_write(rtc, RTC_AL_MTH, (rtc_read(rtc, RTC_AL_MTH) &
> +				RTC_NEW_SPARE3) | tm->tm_mon);
This looks strange. Why doesn't RTC_NEW_SPARE3 contain the register
name? I would have expected:

	(rtc_read(rtc, RTC_AL_MTH) & ~RTC_AL_MTH_MASK) | tm->tm_mon;


> +		rtc_write(rtc, RTC_AL_DOM, (rtc_read(rtc, RTC_AL_DOM) &
> +				RTC_NEW_SPARE1) | tm->tm_mday);
> +		rtc_write(rtc, RTC_AL_HOU, (rtc_read(rtc, RTC_AL_HOU) &
> +				RTC_NEW_SPARE_FG_MASK) | tm->tm_hour);
> +		rtc_write(rtc, RTC_AL_MIN, tm->tm_min);
> +		rtc_write(rtc, RTC_AL_SEC, tm->tm_sec);
> +		rtc_write(rtc, RTC_AL_MASK, RTC_AL_MASK_DOW);
Is this racy? I.e. if the previous set alarm is

	2015-03-13 14:15:00

and you write

	2015-03.14 17:17:00

is it possible that this triggers an alarm if the update happens at

	2015-03-14 14:15:00

?

> +		rtc_write_trigger(rtc);
> +		irqen = rtc_read(rtc, RTC_IRQ_EN) | RTC_IRQ_EN_ONESHOT_AL;
> +		rtc_write(rtc, RTC_IRQ_EN, irqen);
> +		rtc_write_trigger(rtc);
> +		mutex_unlock(&rtc->lock);
	} else {
		/* disable alarm here */

> +	}
> +
> +	return 0;
> +}
> +
> +static struct rtc_class_ops mtk_rtc_ops = {
> +	.read_time  = mtk_rtc_read_time,
> +	.set_time   = mtk_rtc_set_time,
> +	.read_alarm = mtk_rtc_read_alarm,
> +	.set_alarm  = mtk_rtc_set_alarm,
> +};
> +
> +static int mtk_rtc_probe(struct platform_device *pdev)
> +{
> +	struct mt6397_chip *mt6397_chip = dev_get_drvdata(pdev->dev.parent);
> +	struct mt6397_rtc *rtc;
> +	u32 reg[2];
> +	int ret = 0;
> +
> +	rtc = devm_kzalloc(&pdev->dev, sizeof(struct mt6397_rtc), GFP_KERNEL);
> +	if (!rtc)
> +		return -ENOMEM;
> +
> +	ret = of_property_read_u32_array(pdev->dev.of_node, "reg", reg, 2);
> +	if (ret) {
> +		dev_err(&pdev->dev, "couldn't read rtc base address!\n");
> +		return -EINVAL;
> +	}
> +	rtc->addr_base = reg[0];
> +	rtc->addr_range = reg[1];
This looks strange, but maybe that's right as you reuse the parent's
regmap.

> +	rtc->regmap = mt6397_chip->regmap;
> +	rtc->dev = &pdev->dev;
> +	mutex_init(&rtc->lock);
> +
> +	platform_set_drvdata(pdev, rtc);
> +
> +	rtc->rtc_dev = rtc_device_register("mt6397-rtc", &pdev->dev,
> +				&mtk_rtc_ops, THIS_MODULE);
> +	if (IS_ERR(rtc->rtc_dev)) {
> +		dev_err(&pdev->dev, "register rtc device failed\n");
> +		return PTR_ERR(rtc->rtc_dev);
> +	}
> +
> +	rtc->irq = platform_get_irq(pdev, 0);
> +	if (rtc->irq < 0) {
platform_get_irq(pdev, 0) = 0 should be treated as error, too.

> +		ret = rtc->irq;
> +		goto out_rtc;
> +	}

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH 2/2] rtc: mediatek: Add MT63xx RTC driver

[Eddie Huang]

Hi Uwe,

Thanks your review.

On Mon, 2015-03-16 at 16:30 +0100, Uwe Kleine-König wrote:
> Hello Eddie,
> 
> On Wed, Jan 28, 2015 at 05:27:56PM +0800, Eddie Huang wrote:
> > From: Tianping Fang <tianping.fang@mediatek.com>
> > 
> > Add Mediatek MT63xx RTC driver
> MT6397?

Yes, it is better to use MT6397

> > diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
> > index f15cddf..8ac52d8 100644
> > --- a/drivers/rtc/Kconfig
> > +++ b/drivers/rtc/Kconfig
> > @@ -1427,6 +1427,16 @@ config RTC_DRV_MOXART
> >  	   This driver can also be built as a module. If so, the module
> >  	   will be called rtc-moxart
> >  
> > +config RTC_DRV_MT63XX
> > +	tristate "Mediatek Real Time Clock driver"
> > +	depends on MFD_MT6397
> I suggest:
> 
> 	depends on MFD_MT6397 || COMPILE_TEST
> 
> (maybe + any hard dependencies you need for compilation).

OK, will fix it

> 
> > +	help
> > +	  This selects the Mediatek(R) RTC driver, you should add support
> > +	  for Mediatek MT6397 PMIC before select Mediatek(R) RTC driver.
> > +
> > +	  If you want to use Mediatek(R) RTC interface, select Y or M here.
> > +	  If unsure, Please select N.
> Given the dependency above I'd say choosing y here is fine. Instead of
> recommending that I'd just drop this line.

ok, will drop.

> 
> > [...]
> > +static u16 rtc_read(struct mt6397_rtc *rtc, u32 offset)
> rtc_read is a bad name for a driver. There are already 6 functions with
> this name in the kernel. Better use a unique prefix.

I will use prefix mtk_

> 
> > [...]
> > +static irqreturn_t rtc_irq_handler_thread(int irq, void *data)
> > +{
> > +	struct mt6397_rtc *rtc = data;
> > +	u16 irqsta, irqen;
> > +
> > +	mutex_lock(&rtc->lock);
> > +	irqsta = rtc_read(rtc, RTC_IRQ_STA);
> Do you really need to lock for a single read access?

I think this lock is necessary, because other thread may access rtc
register at the same time, for example, call mtk_rtc_set_alarm to modify
alarm time.

> 
> > +	mutex_unlock(&rtc->lock);
> > +
> > +	if (irqsta & RTC_IRQ_STA_AL) {
> > +		rtc_update_irq(rtc->rtc_dev, 1, RTC_IRQF | RTC_AF);
> > +		irqen = irqsta & ~RTC_IRQ_EN_AL;
> > +		rtc_write(rtc, RTC_IRQ_EN, irqen);
> > +		rtc_write_trigger(rtc);
> > +		return IRQ_HANDLED;
> > +	}
> > +
> > +	return IRQ_NONE;
> > +}
> > +
> > +static int mtk_rtc_read_time(struct device *dev, struct rtc_time *tm)
> > +{
> > +	unsigned long time;
> > +	struct mt6397_rtc *rtc = dev_get_drvdata(dev);
> > +
> > +	mutex_lock(&rtc->lock);
> > +	do {
> > +		tm->tm_sec = rtc_read(rtc, RTC_TC_SEC);
> > +		tm->tm_min = rtc_read(rtc, RTC_TC_MIN);
> > +		tm->tm_hour = rtc_read(rtc, RTC_TC_HOU);
> > +		tm->tm_mday = rtc_read(rtc, RTC_TC_DOM);
> > +		tm->tm_mon = rtc_read(rtc, RTC_TC_MTH);
> > +		tm->tm_year = rtc_read(rtc, RTC_TC_YEA);
> > +	} while (rtc_read(rtc, RTC_TC_SEC) < tm->tm_sec);
> > +	mutex_unlock(&rtc->lock);
> > +
> > +	tm->tm_year += RTC_MIN_YEAR_OFFSET;
> > +	tm->tm_mon--;
> > +	rtc_tm_to_time(tm, &time);
> rtc_tm_to_time is deprecated, better use rtc_tm_to_time64.

OK, will change to rtc_tm_to_time64

> 
> > +	tm->tm_wday = (time / 86400 + 4) % 7;
> > +
> > +	return 0;
> > +}
> > +
> > +static int mtk_rtc_set_time(struct device *dev, struct rtc_time *tm)
> > +{
> > +	struct mt6397_rtc *rtc = dev_get_drvdata(dev);
> > +
> > +	tm->tm_year -= RTC_MIN_YEAR_OFFSET;
> > +	tm->tm_mon++;
> > +	mutex_lock(&rtc->lock);
> > +	rtc_write(rtc, RTC_TC_YEA, tm->tm_year);
> > +	rtc_write(rtc, RTC_TC_MTH, tm->tm_mon);
> > +	rtc_write(rtc, RTC_TC_DOM, tm->tm_mday);
> > +	rtc_write(rtc, RTC_TC_HOU, tm->tm_hour);
> > +	rtc_write(rtc, RTC_TC_MIN, tm->tm_min);
> > +	rtc_write(rtc, RTC_TC_SEC, tm->tm_sec);
> Is this racy? I.e. what happens if RTC_TC_SEC overflows just before you
> write to it but after you wrote RTC_TC_MIN?

register value will write to hardware after rtc_write_trigger, so the
racy condition not exist.

> 
> > +	rtc_write_trigger(rtc);
> > +	mutex_unlock(&rtc->lock);
> > +
> > +	return 0;
> > +}
> > +
> > +static int mtk_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alm)
> > +{
> > +	struct rtc_time *tm = &alm->time;
> > +	struct mt6397_rtc *rtc = dev_get_drvdata(dev);
> > +	u16 irqen, pdn2;
> > +
> > +	mutex_lock(&rtc->lock);
> > +	irqen = rtc_read(rtc, RTC_IRQ_EN);
> > +	pdn2 = rtc_read(rtc, RTC_PDN2);
> > +	tm->tm_sec  = rtc_read(rtc, RTC_AL_SEC);
> > +	tm->tm_min  = rtc_read(rtc, RTC_AL_MIN);
> > +	tm->tm_hour = rtc_read(rtc, RTC_AL_HOU) & RTC_AL_HOU_MASK;
> > +	tm->tm_mday = rtc_read(rtc, RTC_AL_DOM) & RTC_AL_DOM_MASK;
> > +	tm->tm_mon  = rtc_read(rtc, RTC_AL_MTH) & RTC_AL_MTH_MASK;
> > +	tm->tm_year = rtc_read(rtc, RTC_AL_YEA);
> > +	mutex_unlock(&rtc->lock);
> > +
> > +	alm->enabled = !!(irqen & RTC_IRQ_EN_AL);
> > +	alm->pending = !!(pdn2 & RTC_PDN2_PWRON_ALARM);
> > +
> > +	tm->tm_year += RTC_MIN_YEAR_OFFSET;
> > +	tm->tm_mon--;
> > +
> > +	return 0;
> > +}
> > +
> > +static int mtk_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alm)
> > +{
> > +	struct rtc_time *tm = &alm->time;
> > +	struct mt6397_rtc *rtc = dev_get_drvdata(dev);
> > +	u16 irqen;
> > +
> > +	tm->tm_year -= RTC_MIN_YEAR_OFFSET;
> > +	tm->tm_mon++;
> > +
> > +	if (alm->enabled) {
> > +		mutex_lock(&rtc->lock);
> > +		rtc_write(rtc, RTC_AL_YEA, tm->tm_year);
> > +		rtc_write(rtc, RTC_AL_MTH, (rtc_read(rtc, RTC_AL_MTH) &
> > +				RTC_NEW_SPARE3) | tm->tm_mon);
> This looks strange. Why doesn't RTC_NEW_SPARE3 contain the register
> name? I would have expected:
> 
> 	(rtc_read(rtc, RTC_AL_MTH) & ~RTC_AL_MTH_MASK) | tm->tm_mon;

I will remove RTC_NEW_SPARE3. Hardware will return 0 if register bit is
useless. So the bit clear is redundant. 

> 
> > +		rtc_write(rtc, RTC_AL_DOM, (rtc_read(rtc, RTC_AL_DOM) &
> > +				RTC_NEW_SPARE1) | tm->tm_mday);
> > +		rtc_write(rtc, RTC_AL_HOU, (rtc_read(rtc, RTC_AL_HOU) &
> > +				RTC_NEW_SPARE_FG_MASK) | tm->tm_hour);
> > +		rtc_write(rtc, RTC_AL_MIN, tm->tm_min);
> > +		rtc_write(rtc, RTC_AL_SEC, tm->tm_sec);
> > +		rtc_write(rtc, RTC_AL_MASK, RTC_AL_MASK_DOW);
> Is this racy? I.e. if the previous set alarm is
> 
> 	2015-03-13 14:15:00
> 
> and you write
> 
> 	2015-03.14 17:17:00
> 
> is it possible that this triggers an alarm if the update happens at
> 
> 	2015-03-14 14:15:00
> 
> ?
> 
All rtc register value write to RTC hardware after rtc_write_trigger.
Race condition should not happen. 


> > +		rtc_write_trigger(rtc);
> > +		irqen = rtc_read(rtc, RTC_IRQ_EN) | RTC_IRQ_EN_ONESHOT_AL;
> > +		rtc_write(rtc, RTC_IRQ_EN, irqen);
> > +		rtc_write_trigger(rtc);
> > +		mutex_unlock(&rtc->lock);
> 	} else {
> 		/* disable alarm here */
> 
OK, should clear RTC_IRQ_EN

> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static struct rtc_class_ops mtk_rtc_ops = {
> > +	.read_time  = mtk_rtc_read_time,
> > +	.set_time   = mtk_rtc_set_time,
> > +	.read_alarm = mtk_rtc_read_alarm,
> > +	.set_alarm  = mtk_rtc_set_alarm,
> > +};
> > +
> > +static int mtk_rtc_probe(struct platform_device *pdev)
> > +{
> > +	struct mt6397_chip *mt6397_chip = dev_get_drvdata(pdev->dev.parent);
> > +	struct mt6397_rtc *rtc;
> > +	u32 reg[2];
> > +	int ret = 0;
> > +
> > +	rtc = devm_kzalloc(&pdev->dev, sizeof(struct mt6397_rtc), GFP_KERNEL);
> > +	if (!rtc)
> > +		return -ENOMEM;
> > +
> > +	ret = of_property_read_u32_array(pdev->dev.of_node, "reg", reg, 2);
> > +	if (ret) {
> > +		dev_err(&pdev->dev, "couldn't read rtc base address!\n");
> > +		return -EINVAL;
> > +	}
> > +	rtc->addr_base = reg[0];
> > +	rtc->addr_range = reg[1];
> This looks strange, but maybe that's right as you reuse the parent's
> regmap.

According Sascha and Mark Brown's discussion:
http://lists.infradead.org/pipermail/linux-arm-kernel/2015-February/323239.html

Address and interrupt will move from device tree to mfd_cell in
mt6397-core.c

> 
> > +	rtc->regmap = mt6397_chip->regmap;
> > +	rtc->dev = &pdev->dev;
> > +	mutex_init(&rtc->lock);
> > +
> > +	platform_set_drvdata(pdev, rtc);
> > +
> > +	rtc->rtc_dev = rtc_device_register("mt6397-rtc", &pdev->dev,
> > +				&mtk_rtc_ops, THIS_MODULE);
> > +	if (IS_ERR(rtc->rtc_dev)) {
> > +		dev_err(&pdev->dev, "register rtc device failed\n");
> > +		return PTR_ERR(rtc->rtc_dev);
> > +	}
> > +
> > +	rtc->irq = platform_get_irq(pdev, 0);
> > +	if (rtc->irq < 0) {
> platform_get_irq(pdev, 0) = 0 should be treated as error, too.

OK, will fix it

> 
> > +		ret = rtc->irq;
> > +		goto out_rtc;
> > +	}
> 
> Best regards
> Uwe
> 

Re: [PATCH 2/2] rtc: mediatek: Add MT63xx RTC driver

[Uwe Kleine-König]

Hello Eddie,

On Tue, Mar 17, 2015 at 08:31:14PM +0800, Eddie Huang wrote:
> On Mon, 2015-03-16 at 16:30 +0100, Uwe Kleine-König wrote:
> > On Wed, Jan 28, 2015 at 05:27:56PM +0800, Eddie Huang wrote:
> > > [...]
> > > +static u16 rtc_read(struct mt6397_rtc *rtc, u32 offset)
> > rtc_read is a bad name for a driver. There are already 6 functions with
> > this name in the kernel. Better use a unique prefix.
> 
> I will use prefix mtk_
I would prefer a prefix that is unique to the driver. "mtk_" doesn't
work to distinguish between the rtc and a (say) spi driver. What you
want here is that if someone reports a bug on any mailinglist with a
backtrace you are able to immediately see which driver is affected.

> > > [...]
> > > +static irqreturn_t rtc_irq_handler_thread(int irq, void *data)
> > > +{
> > > +	struct mt6397_rtc *rtc = data;
> > > +	u16 irqsta, irqen;
> > > +
> > > +	mutex_lock(&rtc->lock);
> > > +	irqsta = rtc_read(rtc, RTC_IRQ_STA);
> > Do you really need to lock for a single read access?
> 
> I think this lock is necessary, because other thread may access rtc
> register at the same time, for example, call mtk_rtc_set_alarm to modify
> alarm time.
That would be a valid reason if mtk_rtc_set_alarm touched that register
twice in a single critical section and the handler must not read the
value of the first write. Otherwise it should be fine, shouldn't it?

> > > +static int mtk_rtc_set_time(struct device *dev, struct rtc_time *tm)
> > > +{
> > > +	struct mt6397_rtc *rtc = dev_get_drvdata(dev);
> > > +
> > > +	tm->tm_year -= RTC_MIN_YEAR_OFFSET;
> > > +	tm->tm_mon++;
> > > +	mutex_lock(&rtc->lock);
> > > +	rtc_write(rtc, RTC_TC_YEA, tm->tm_year);
> > > +	rtc_write(rtc, RTC_TC_MTH, tm->tm_mon);
> > > +	rtc_write(rtc, RTC_TC_DOM, tm->tm_mday);
> > > +	rtc_write(rtc, RTC_TC_HOU, tm->tm_hour);
> > > +	rtc_write(rtc, RTC_TC_MIN, tm->tm_min);
> > > +	rtc_write(rtc, RTC_TC_SEC, tm->tm_sec);
> > Is this racy? I.e. what happens if RTC_TC_SEC overflows just before you
> > write to it but after you wrote RTC_TC_MIN?
> 
> register value will write to hardware after rtc_write_trigger, so the
> racy condition not exist.
Ah, it seems the hardware guys did their job. Nice.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH 2/2] rtc: mediatek: Add MT63xx RTC driver

[Eddie Huang]

Hi Uwe,

On Tue, 2015-03-17 at 14:43 +0100, Uwe Kleine-König wrote:
> Hello Eddie,
> 
> On Tue, Mar 17, 2015 at 08:31:14PM +0800, Eddie Huang wrote:
> > On Mon, 2015-03-16 at 16:30 +0100, Uwe Kleine-König wrote:
> > > On Wed, Jan 28, 2015 at 05:27:56PM +0800, Eddie Huang wrote:
> > > > [...]
> > > > +static u16 rtc_read(struct mt6397_rtc *rtc, u32 offset)
> > > rtc_read is a bad name for a driver. There are already 6 functions with
> > > this name in the kernel. Better use a unique prefix.
> > 
> > I will use prefix mtk_
> I would prefer a prefix that is unique to the driver. "mtk_" doesn't
> work to distinguish between the rtc and a (say) spi driver. What you
> want here is that if someone reports a bug on any mailinglist with a
> backtrace you are able to immediately see which driver is affected.
> 

My meaning is mtk_rtc_read, mtk_rtc_write.

> > > > [...]
> > > > +static irqreturn_t rtc_irq_handler_thread(int irq, void *data)
> > > > +{
> > > > +	struct mt6397_rtc *rtc = data;
> > > > +	u16 irqsta, irqen;
> > > > +
> > > > +	mutex_lock(&rtc->lock);
> > > > +	irqsta = rtc_read(rtc, RTC_IRQ_STA);
> > > Do you really need to lock for a single read access?
> > 
> > I think this lock is necessary, because other thread may access rtc
> > register at the same time, for example, call mtk_rtc_set_alarm to modify
> > alarm time.
> That would be a valid reason if mtk_rtc_set_alarm touched that register
> twice in a single critical section and the handler must not read the
> value of the first write. Otherwise it should be fine, shouldn't it?
> 

My original though is if disable alarm in mtk_rtc_set_alarm function,
RTC_IRQ_STA may be affected, this is why I add mutex. After checking
with designer, RTC_IRQ_STA will not be affected. I will remove the
mutex.

> > > > +static int mtk_rtc_set_time(struct device *dev, struct rtc_time *tm)
> > > > +{
> > > > +	struct mt6397_rtc *rtc = dev_get_drvdata(dev);
> > > > +
> > > > +	tm->tm_year -= RTC_MIN_YEAR_OFFSET;
> > > > +	tm->tm_mon++;
> > > > +	mutex_lock(&rtc->lock);
> > > > +	rtc_write(rtc, RTC_TC_YEA, tm->tm_year);
> > > > +	rtc_write(rtc, RTC_TC_MTH, tm->tm_mon);
> > > > +	rtc_write(rtc, RTC_TC_DOM, tm->tm_mday);
> > > > +	rtc_write(rtc, RTC_TC_HOU, tm->tm_hour);
> > > > +	rtc_write(rtc, RTC_TC_MIN, tm->tm_min);
> > > > +	rtc_write(rtc, RTC_TC_SEC, tm->tm_sec);
> > > Is this racy? I.e. what happens if RTC_TC_SEC overflows just before you
> > > write to it but after you wrote RTC_TC_MIN?
> > 
> > register value will write to hardware after rtc_write_trigger, so the
> > racy condition not exist.
> Ah, it seems the hardware guys did their job. Nice.
> 
> Best regards
> Uwe
> 

Re: [PATCH 2/2] rtc: mediatek: Add MT63xx RTC driver

[Uwe Kleine-König]

Hello Eddie,

On Wed, Mar 18, 2015 at 11:27:40AM +0800, Eddie Huang wrote:
> On Tue, 2015-03-17 at 14:43 +0100, Uwe Kleine-König wrote:
> > On Tue, Mar 17, 2015 at 08:31:14PM +0800, Eddie Huang wrote:
> > > On Mon, 2015-03-16 at 16:30 +0100, Uwe Kleine-König wrote:
> > > > On Wed, Jan 28, 2015 at 05:27:56PM +0800, Eddie Huang wrote:
> > > > > [...]
> > > > > +static irqreturn_t rtc_irq_handler_thread(int irq, void *data)
> > > > > +{
> > > > > +	struct mt6397_rtc *rtc = data;
> > > > > +	u16 irqsta, irqen;
> > > > > +
> > > > > +	mutex_lock(&rtc->lock);
> > > > > +	irqsta = rtc_read(rtc, RTC_IRQ_STA);
> > > > Do you really need to lock for a single read access?
> > > 
> > > I think this lock is necessary, because other thread may access rtc
> > > register at the same time, for example, call mtk_rtc_set_alarm to modify
> > > alarm time.
> > That would be a valid reason if mtk_rtc_set_alarm touched that register
> > twice in a single critical section and the handler must not read the
> > value of the first write. Otherwise it should be fine, shouldn't it?
> > 
> 
> My original though is if disable alarm in mtk_rtc_set_alarm function,
> RTC_IRQ_STA may be affected, this is why I add mutex. After checking
> with designer, RTC_IRQ_STA will not be affected. I will remove the
> mutex.
Even if IRQ_STA is changed there is no problem. With the lock the
following can happen:

Either the irq thread comes first:

	thread1			thread2

	lock
	rtc_read
	unlock   ------------>  lock
				dosomething with IRQ_STA
				unlock

or the other thread comes first:

	thread1			thread2

				lock
				dosomething with IRQ_STA
	lock   <-------------	unlock
	rtc_read
	unlock

So thread1 either reads the old or the new value of IRQ_STA.

Without locking you have the same! Either the write in thread2 comes
first or not. And depending on that you either read the new or the old
value respectively.

Of course this assumes that a write is atomic in the sense that if you
update a value from 0x01 to 0x10 there is no point in time a reader can
see 0x00 or 0x11. But this is usually given. Also reading the register
in question must not have side effects that affect the other threads.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |