Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1674133 > unrolled thread
| Started by | Alexandre Belloni <alexandre.belloni@free-electrons.com> |
|---|---|
| First post | 2017-06-24 21:00 +0200 |
| Last post | 2017-06-26 23:50 +0200 |
| Articles | 4 — 2 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
Re: [PATCH 2/2] rtc: brcmstb-waketimer: Add Broadcom STB wake-timer Alexandre Belloni <alexandre.belloni@free-electrons.com> - 2017-06-24 21:00 +0200
Re: [PATCH 2/2] rtc: brcmstb-waketimer: Add Broadcom STB wake-timer Florian Fainelli <f.fainelli@gmail.com> - 2017-06-26 19:20 +0200
Re: [PATCH 2/2] rtc: brcmstb-waketimer: Add Broadcom STB wake-timer Alexandre Belloni <alexandre.belloni@free-electrons.com> - 2017-06-26 23:40 +0200
Re: [PATCH 2/2] rtc: brcmstb-waketimer: Add Broadcom STB wake-timer Florian Fainelli <f.fainelli@gmail.com> - 2017-06-26 23:50 +0200
| From | Alexandre Belloni <alexandre.belloni@free-electrons.com> |
|---|---|
| Date | 2017-06-24 21:00 +0200 |
| Subject | Re: [PATCH 2/2] rtc: brcmstb-waketimer: Add Broadcom STB wake-timer |
| Message-ID | <tVYrM-2Yp-31@gated-at.bofh.it> |
Hi,
This seems mostly good.
On 15/06/2017 at 12:59:04 -0700, Florian Fainelli wrote:
> +static void wktmr_read(struct brcmstb_waketmr *timer,
> + struct wktmr_time *t)
> +{
> + u32 tmp;
> +
To be sure, is this IP always 32bit, even on 64bit platforms?
> + do {
> + t->sec = readl_relaxed(timer->base + BRCMSTB_WKTMR_COUNTER);
> + tmp = readl_relaxed(timer->base + BRCMSTB_WKTMR_PRESCALER_VAL);
> + } while (tmp >= timer->rate);
> +
> + t->pre = timer->rate - tmp;
> +}
> +
[...]
> +static int brcmstb_waketmr_settime(struct device *dev,
> + struct rtc_time *tm)
> +{
> + struct brcmstb_waketmr *timer = dev_get_drvdata(dev);
> + unsigned long sec;
> + int ret;
> +
> + ret = rtc_valid_tm(tm);
> + if (ret)
> + return ret;
> +
There is no way this function can be called without a valid tm. The only
caller checks before calling.
> + rtc_tm_to_time(tm, &sec);
> +
> + dev_dbg(dev, "%s: sec=%ld\n", __func__, sec);
> + writel_relaxed(sec, timer->base + BRCMSTB_WKTMR_COUNTER);
> +
> + return 0;
> +}
> +
> +static int brcmstb_waketmr_getalarm(struct device *dev,
> + struct rtc_wkalrm *alarm)
> +{
> + struct brcmstb_waketmr *timer = dev_get_drvdata(dev);
> + unsigned long sec;
> + u32 reg;
> +
> + sec = readl_relaxed(timer->base + BRCMSTB_WKTMR_ALARM);
> + if (sec == 0) {
> + /* Alarm is disabled */
> + alarm->enabled = 0;
> + alarm->time.tm_mon = -1;
> + alarm->time.tm_mday = -1;
> + alarm->time.tm_year = -1;
> + alarm->time.tm_hour = -1;
> + alarm->time.tm_min = -1;
> + alarm->time.tm_sec = -1;
This is not needed since d68778b80dd7
> + dev_dbg(dev, "%s: alarm is disabled\n", __func__);
> + } else {
> + /* Alarm is enabled */
> + alarm->enabled = 1;
> + rtc_time_to_tm(sec, &alarm->time);
> + dev_dbg(dev, "%s: alarm is enabled\n", __func__);
> + }
> +
> + reg = readl_relaxed(timer->base + BRCMSTB_WKTMR_EVENT);
> + alarm->pending = !!(reg & 1);
> + dev_dbg(dev, "%s: alarm pending=%d\n", __func__, alarm->pending);
> +
> + return 0;
> +}
--
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
[toc] | [next] | [standalone]
| From | Florian Fainelli <f.fainelli@gmail.com> |
|---|---|
| Date | 2017-06-26 19:20 +0200 |
| Message-ID | <tWFQ5-5au-13@gated-at.bofh.it> |
| In reply to | #1674133 |
On 06/24/2017 11:59 AM, Alexandre Belloni wrote:
> Hi,
>
> This seems mostly good.
>
> On 15/06/2017 at 12:59:04 -0700, Florian Fainelli wrote:
>> +static void wktmr_read(struct brcmstb_waketmr *timer,
>> + struct wktmr_time *t)
>> +{
>> + u32 tmp;
>> +
>
> To be sure, is this IP always 32bit, even on 64bit platforms?
Correct, it's only 32-bit capable (saw the recent discussions about the
2038yr "problem"...).
>
>> + do {
>> + t->sec = readl_relaxed(timer->base + BRCMSTB_WKTMR_COUNTER);
>> + tmp = readl_relaxed(timer->base + BRCMSTB_WKTMR_PRESCALER_VAL);
>> + } while (tmp >= timer->rate);
>> +
>> + t->pre = timer->rate - tmp;
>> +}
>> +
>
> [...]
>
>> +static int brcmstb_waketmr_settime(struct device *dev,
>> + struct rtc_time *tm)
>> +{
>> + struct brcmstb_waketmr *timer = dev_get_drvdata(dev);
>> + unsigned long sec;
>> + int ret;
>> +
>> + ret = rtc_valid_tm(tm);
>> + if (ret)
>> + return ret;
>> +
>
> There is no way this function can be called without a valid tm. The only
> caller checks before calling.
OK.
>
>> + rtc_tm_to_time(tm, &sec);
>> +
>> + dev_dbg(dev, "%s: sec=%ld\n", __func__, sec);
>> + writel_relaxed(sec, timer->base + BRCMSTB_WKTMR_COUNTER);
>> +
>> + return 0;
>> +}
>> +
>> +static int brcmstb_waketmr_getalarm(struct device *dev,
>> + struct rtc_wkalrm *alarm)
>> +{
>> + struct brcmstb_waketmr *timer = dev_get_drvdata(dev);
>> + unsigned long sec;
>> + u32 reg;
>> +
>> + sec = readl_relaxed(timer->base + BRCMSTB_WKTMR_ALARM);
>> + if (sec == 0) {
>> + /* Alarm is disabled */
>> + alarm->enabled = 0;
>> + alarm->time.tm_mon = -1;
>> + alarm->time.tm_mday = -1;
>> + alarm->time.tm_year = -1;
>> + alarm->time.tm_hour = -1;
>> + alarm->time.tm_min = -1;
>> + alarm->time.tm_sec = -1;
>
> This is not needed since d68778b80dd7
Great, I will take that out.
Do you care whether some dev_dbg() prints are left in the driver or
should I remove those in v2?
>
>> + dev_dbg(dev, "%s: alarm is disabled\n", __func__);
>> + } else {
>> + /* Alarm is enabled */
>> + alarm->enabled = 1;
>> + rtc_time_to_tm(sec, &alarm->time);
>> + dev_dbg(dev, "%s: alarm is enabled\n", __func__);
>> + }
>> +
>> + reg = readl_relaxed(timer->base + BRCMSTB_WKTMR_EVENT);
>> + alarm->pending = !!(reg & 1);
>> + dev_dbg(dev, "%s: alarm pending=%d\n", __func__, alarm->pending);
>> +
>> + return 0;
>> +}
>
--
Florian
[toc] | [prev] | [next] | [standalone]
| From | Alexandre Belloni <alexandre.belloni@free-electrons.com> |
|---|---|
| Date | 2017-06-26 23:40 +0200 |
| Message-ID | <tWJTI-7Ec-13@gated-at.bofh.it> |
| In reply to | #1674956 |
On 26/06/2017 at 10:15:46 -0700, Florian Fainelli wrote: > > This is not needed since d68778b80dd7 > > Great, I will take that out. > > Do you care whether some dev_dbg() prints are left in the driver or > should I remove those in v2? > It could have stayed but it seems you removed it from v2. I'll applay that but it is ok if you change your mind. -- Alexandre Belloni, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com
[toc] | [prev] | [next] | [standalone]
| From | Florian Fainelli <f.fainelli@gmail.com> |
|---|---|
| Date | 2017-06-26 23:50 +0200 |
| Message-ID | <tWK3o-7Ki-33@gated-at.bofh.it> |
| In reply to | #1675118 |
On 06/26/2017 02:35 PM, Alexandre Belloni wrote: > On 26/06/2017 at 10:15:46 -0700, Florian Fainelli wrote: >>> This is not needed since d68778b80dd7 >> >> Great, I will take that out. >> >> Do you care whether some dev_dbg() prints are left in the driver or >> should I remove those in v2? >> > > It could have stayed but it seems you removed it from v2. > > I'll applay that but it is ok if you change your mind. No, I think they don't really belong in the driver anymore, thanks! -- Florian
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web