Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1206828 > unrolled thread

Re: [PATCH 2/2] soc/fsl: add ftm alarm driver for ls1021a platform

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.

Contents

  Re: [PATCH 2/2] soc/fsl: add ftm alarm driver for ls1021a platform Linus Walleij <linus.walleij@linaro.org> - 2015-08-13 16:00 +0200
    RE: [PATCH 2/2] soc/fsl: add ftm alarm driver for ls1021a platform Wang Dongsheng <Dongsheng.Wang@freescale.com> - 2015-08-14 05:20 +0200
      Re: [PATCH 2/2] soc/fsl: add ftm alarm driver for ls1021a platform Linus Walleij <linus.walleij@linaro.org> - 2015-08-14 12:10 +0200

#1206828 — Re: [PATCH 2/2] soc/fsl: add ftm alarm driver for ls1021a platform

On Wed, Aug 12, 2015 at 7:53 AM, Dongsheng Wang
<dongsheng.wang@freescale.com> wrote:

> From: Wang Dongsheng <dongsheng.wang@freescale.com>
>
> Only Ftm0 can be used when system going to deep sleep. So this driver
> to support ftm0 as a wakeup source.
>
> Signed-off-by: Wang Dongsheng <dongsheng.wang@freescale.com>
> ---
> *V2*
> Change Copyright 2014 to 2015.
(...)
> +config FTM_ALARM
> +       bool "FTM alarm driver"
> +       depends on SOC_LS1021A
> +       default n
> +       help
> +         Say y here to enable FTM alarm support.  The FTM alarm provides
> +         alarm functions for wakeup system from deep sleep.  There is only
> +         one FTM can be used in ALARM(FTM 0).
(...)
> +static u32 time_to_cycle(unsigned long time)
> +static u32 cycle_to_time(u32 cycle)
> +static int ftm_set_alarm(u64 cycle)
> +static irqreturn_t ftm_alarm_interrupt(int irq, void *dev_id)
> +static ssize_t ftm_alarm_show(struct device *dev,
> +                             struct device_attribute *attr,
> +                             char *buf)
> +static ssize_t ftm_alarm_store(struct device *dev,
> +                              struct device_attribute *attr,
> +                              const char *buf, size_t count)
(...)
> +static struct device_attribute ftm_alarm_attributes = __ATTR(ftm_alarm, 0644,
> +                       ftm_alarm_show, ftm_alarm_store);

If you're gonna invent ABIs, document then in Documentation/ABI/testing/*.

But I don't get it. Why is this driver not in drivers/rtc?

It does a subset of what an RTC does. The ioctl()'s of an RTC
can do what you want to do. And much much more.

If it can't do all an RTC can do, surely the RTC subsystem
can be augmented to host it anyway. It's way to close to
an RTC to have it's own random sysfs driver like this.

Unless I'm totally off, rewrite this to an RTC driver and post
it to the RTC maintainers.

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

[toc] | [next] | [standalone]

#1207314

DQoNCj4gLS0tLS1PcmlnaW5hbCBNZXNzYWdlLS0tLS0NCj4gRnJvbTogTGludXMgV2FsbGVpaiBb
bWFpbHRvOmxpbnVzLndhbGxlaWpAbGluYXJvLm9yZ10NCj4gU2VudDogVGh1cnNkYXksIEF1Z3Vz
dCAxMywgMjAxNSA5OjU0IFBNDQo+IFRvOiBXYW5nIERvbmdzaGVuZy1CNDA1MzQ7IEpvaG4gU3R1
bHR6OyBBbGVzc2FuZHJvIFp1bW1vOyBBbGV4YW5kcmUgQmVsbG9uaQ0KPiBDYzogU2hhd24gR3Vv
OyBOYWlyLCBTYW5kZWVwOyBIYW5zIGRlIEdvZWRlOyBXYW5nIEh1YW4tQjE4OTY1OyBsaW51eC1h
cm0tDQo+IGtlcm5lbEBsaXN0cy5pbmZyYWRlYWQub3JnOyBsaW51eC1rZXJuZWxAdmdlci5rZXJu
ZWwub3JnOyBydGMtDQo+IGxpbnV4QGdvb2dsZWdyb3Vwcy5jb20NCj4gU3ViamVjdDogUmU6IFtQ
QVRDSCAyLzJdIHNvYy9mc2w6IGFkZCBmdG0gYWxhcm0gZHJpdmVyIGZvciBsczEwMjFhIHBsYXRm
b3JtDQo+IA0KPiBPbiBXZWQsIEF1ZyAxMiwgMjAxNSBhdCA3OjUzIEFNLCBEb25nc2hlbmcgV2Fu
Zw0KPiA8ZG9uZ3NoZW5nLndhbmdAZnJlZXNjYWxlLmNvbT4gd3JvdGU6DQo+IA0KPiA+IEZyb206
IFdhbmcgRG9uZ3NoZW5nIDxkb25nc2hlbmcud2FuZ0BmcmVlc2NhbGUuY29tPg0KPiA+DQo+ID4g
T25seSBGdG0wIGNhbiBiZSB1c2VkIHdoZW4gc3lzdGVtIGdvaW5nIHRvIGRlZXAgc2xlZXAuIFNv
IHRoaXMgZHJpdmVyDQo+ID4gdG8gc3VwcG9ydCBmdG0wIGFzIGEgd2FrZXVwIHNvdXJjZS4NCj4g
Pg0KPiA+IFNpZ25lZC1vZmYtYnk6IFdhbmcgRG9uZ3NoZW5nIDxkb25nc2hlbmcud2FuZ0BmcmVl
c2NhbGUuY29tPg0KPiA+IC0tLQ0KPiA+ICpWMioNCj4gPiBDaGFuZ2UgQ29weXJpZ2h0IDIwMTQg
dG8gMjAxNS4NCj4gKC4uLikNCj4gPiArY29uZmlnIEZUTV9BTEFSTQ0KPiA+ICsgICAgICAgYm9v
bCAiRlRNIGFsYXJtIGRyaXZlciINCj4gPiArICAgICAgIGRlcGVuZHMgb24gU09DX0xTMTAyMUEN
Cj4gPiArICAgICAgIGRlZmF1bHQgbg0KPiA+ICsgICAgICAgaGVscA0KPiA+ICsgICAgICAgICBT
YXkgeSBoZXJlIHRvIGVuYWJsZSBGVE0gYWxhcm0gc3VwcG9ydC4gIFRoZSBGVE0gYWxhcm0gcHJv
dmlkZXMNCj4gPiArICAgICAgICAgYWxhcm0gZnVuY3Rpb25zIGZvciB3YWtldXAgc3lzdGVtIGZy
b20gZGVlcCBzbGVlcC4gIFRoZXJlIGlzIG9ubHkNCj4gPiArICAgICAgICAgb25lIEZUTSBjYW4g
YmUgdXNlZCBpbiBBTEFSTShGVE0gMCkuDQo+ICguLi4pDQo+ID4gK3N0YXRpYyB1MzIgdGltZV90
b19jeWNsZSh1bnNpZ25lZCBsb25nIHRpbWUpDQo+ID4gK3N0YXRpYyB1MzIgY3ljbGVfdG9fdGlt
ZSh1MzIgY3ljbGUpDQo+ID4gK3N0YXRpYyBpbnQgZnRtX3NldF9hbGFybSh1NjQgY3ljbGUpDQo+
ID4gK3N0YXRpYyBpcnFyZXR1cm5fdCBmdG1fYWxhcm1faW50ZXJydXB0KGludCBpcnEsIHZvaWQg
KmRldl9pZCkNCj4gPiArc3RhdGljIHNzaXplX3QgZnRtX2FsYXJtX3Nob3coc3RydWN0IGRldmlj
ZSAqZGV2LA0KPiA+ICsgICAgICAgICAgICAgICAgICAgICAgICAgICAgIHN0cnVjdCBkZXZpY2Vf
YXR0cmlidXRlICphdHRyLA0KPiA+ICsgICAgICAgICAgICAgICAgICAgICAgICAgICAgIGNoYXIg
KmJ1ZikNCj4gPiArc3RhdGljIHNzaXplX3QgZnRtX2FsYXJtX3N0b3JlKHN0cnVjdCBkZXZpY2Ug
KmRldiwNCj4gPiArICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgc3RydWN0IGRldmljZV9h
dHRyaWJ1dGUgKmF0dHIsDQo+ID4gKyAgICAgICAgICAgICAgICAgICAgICAgICAgICAgIGNvbnN0
IGNoYXIgKmJ1Ziwgc2l6ZV90IGNvdW50KQ0KPiAoLi4uKQ0KPiA+ICtzdGF0aWMgc3RydWN0IGRl
dmljZV9hdHRyaWJ1dGUgZnRtX2FsYXJtX2F0dHJpYnV0ZXMgPSBfX0FUVFIoZnRtX2FsYXJtLCAw
NjQ0LA0KPiA+ICsgICAgICAgICAgICAgICAgICAgICAgIGZ0bV9hbGFybV9zaG93LCBmdG1fYWxh
cm1fc3RvcmUpOw0KPiANCj4gSWYgeW91J3JlIGdvbm5hIGludmVudCBBQklzLCBkb2N1bWVudCB0
aGVuIGluIERvY3VtZW50YXRpb24vQUJJL3Rlc3RpbmcvKi4NCj4gDQo+IEJ1dCBJIGRvbid0IGdl
dCBpdC4gV2h5IGlzIHRoaXMgZHJpdmVyIG5vdCBpbiBkcml2ZXJzL3J0Yz8NCj4gDQo+IEl0IGRv
ZXMgYSBzdWJzZXQgb2Ygd2hhdCBhbiBSVEMgZG9lcy4gVGhlIGlvY3RsKCkncyBvZiBhbiBSVEMN
Cj4gY2FuIGRvIHdoYXQgeW91IHdhbnQgdG8gZG8uIEFuZCBtdWNoIG11Y2ggbW9yZS4NCj4gDQo+
IElmIGl0IGNhbid0IGRvIGFsbCBhbiBSVEMgY2FuIGRvLCBzdXJlbHkgdGhlIFJUQyBzdWJzeXN0
ZW0NCj4gY2FuIGJlIGF1Z21lbnRlZCB0byBob3N0IGl0IGFueXdheS4gSXQncyB3YXkgdG8gY2xv
c2UgdG8NCj4gYW4gUlRDIHRvIGhhdmUgaXQncyBvd24gcmFuZG9tIHN5c2ZzIGRyaXZlciBsaWtl
IHRoaXMuDQo+IA0KPiBVbmxlc3MgSSdtIHRvdGFsbHkgb2ZmLCByZXdyaXRlIHRoaXMgdG8gYW4g
UlRDIGRyaXZlciBhbmQgcG9zdA0KPiBpdCB0byB0aGUgUlRDIG1haW50YWluZXJzLg0KPiANCg0K
RmxleFRpbWVyIGlzIG5vdCBhIFJUQyBkZXZpY2UgYW5kIG5vdCBoYXZlIGFueSBydGMgZGVpdmNl
IGZ1bmN0aW9uLiBUaGV5IGJlbG9uZyB0bw0KZGlmZmVyZW50IGRldmljZXMsIHdoeSB3ZSBuZWVk
IHRvIHJlZ2lzdGVyIHRoaXMgdG8gUlRDIGZyYW1ld29yaz8gSSBhbSBjb25mdXNlZCBhYm91dCB0
aGlzLg0KDQpOb3cgaW4gZnJlZXNjYWxlIGxheWVyc2NhcGUgcGxhdGZvcm0gdGhpcyBkcml2ZXIg
aXMgb25seSBmb3IgRmxleFRpbWVyMCwgYW5kIG5vdA0KZml0IGZvciBlYWNoIGZsZXh0aW1lci4g
QmVjYXVzZSBvbmx5IEZsZXhUaW1lcjAgc3RpbGwgdHVybi1vbiB3aGVuIHN5c3RlbSBpbiB0aGUg
RGVlcCBTbGVlcC4NCg0KSWYgdGhlICJhbGFybSIgbWFrZSB5b3UgZmVlbCBjb25mdXNlZCBvciBt
aXNsZWFkIHlvdSB0aGluayB0aGlzIGlzIGEgUlRDIGRldmljZXMuIEkgdGhpbmsNCkkgbmVlZCB0
byBjaGFuZ2UgdGhlICJhbGFybSIgdG8gInRpbWVyIi4NCg0KUmVnYXJkcywNCi1Eb25nc2hlbmcN
Cg==
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1207461

On Fri, Aug 14, 2015 at 5:12 AM, Wang Dongsheng
<Dongsheng.Wang@freescale.com> wrote:
>> On Wed, Aug 12, 2015 at 7:53 AM, Dongsheng Wang
>> <dongsheng.wang@freescale.com> wrote:
>>
>> > From: Wang Dongsheng <dongsheng.wang@freescale.com>
>> >
>> > Only Ftm0 can be used when system going to deep sleep. So this driver
>> > to support ftm0 as a wakeup source.
>> >
>> > Signed-off-by: Wang Dongsheng <dongsheng.wang@freescale.com>
>> > ---
>> > *V2*
>> > Change Copyright 2014 to 2015.
>> (...)
>> > +config FTM_ALARM
>> > +       bool "FTM alarm driver"
>> > +       depends on SOC_LS1021A
>> > +       default n
>> > +       help
>> > +         Say y here to enable FTM alarm support.  The FTM alarm provides
>> > +         alarm functions for wakeup system from deep sleep.  There is only
>> > +         one FTM can be used in ALARM(FTM 0).
>> (...)
>> > +static u32 time_to_cycle(unsigned long time)
>> > +static u32 cycle_to_time(u32 cycle)
>> > +static int ftm_set_alarm(u64 cycle)
>> > +static irqreturn_t ftm_alarm_interrupt(int irq, void *dev_id)
>> > +static ssize_t ftm_alarm_show(struct device *dev,
>> > +                             struct device_attribute *attr,
>> > +                             char *buf)
>> > +static ssize_t ftm_alarm_store(struct device *dev,
>> > +                              struct device_attribute *attr,
>> > +                              const char *buf, size_t count)
>> (...)
>> > +static struct device_attribute ftm_alarm_attributes = __ATTR(ftm_alarm, 0644,
>> > +                       ftm_alarm_show, ftm_alarm_store);
>>
>> If you're gonna invent ABIs, document then in Documentation/ABI/testing/*.
>>
>> But I don't get it. Why is this driver not in drivers/rtc?
>>
>> It does a subset of what an RTC does. The ioctl()'s of an RTC
>> can do what you want to do. And much much more.
>>
>> If it can't do all an RTC can do, surely the RTC subsystem
>> can be augmented to host it anyway. It's way to close to
>> an RTC to have it's own random sysfs driver like this.
>>
>> Unless I'm totally off, rewrite this to an RTC driver and post
>> it to the RTC maintainers.
>
> FlexTimer is not a RTC device and not have any rtc deivce function. They belong to
> different devices, why we need to register this to RTC framework? I am confused about this.
>
> Now in freescale layerscape platform this driver is only for FlexTimer0, and not
> fit for each flextimer. Because only FlexTimer0 still turn-on when system in the Deep Sleep.
>
> If the "alarm" make you feel confused or mislead you think this is a RTC devices. I think
> I need to change the "alarm" to "timer".

I think it is an RTC, it is just that the hardware engineer
designed it with a wakeup usecase in mind and did not call
it an RTC. Wakeup is one of the things RTCs do.

If you inspect a few drivers in drivers/rtc such as drivers/rtc/rtc-pl030.c
you will find that they are just as crude as this "alarm" thing.

It has a counter that counts cycles, it has a comparator
and an alarm function. It is an on-chip RTC, just like PL030
no matter what the datasheet or hardware engineer thinks it
should be called, the Linux kernel calls this an RTC, and it
has a subsystem for handling it, so we should use it and
not invent random new stuff.

If the hardware is really so strange that the counter can only
be started if you also put an alarm at the same time (I doubt
it, but OK if you say so) it is a subset of an RTC that can
only be used for alarms but not timekeeping, but it should
*still* live in drivers/rtc.

Think for a moment on the huge effort that John Stultz put into
integrating Android alarm timers with POSIX and the RTC
subsystem and fixing it all from the smallest handset to
the largest S360 supercomputer. The approach of a custom
device just throws all of that out the window and reinvents the
mechanism in userspace, forcing all standardized userspace to
have special code to handle this special alarm with its
special sysfs ABI.

Check
commit ff3ead96d17f47ee70c294a5cc2cce9b61e82f0f
"timers: Introduce in-kernel alarm-timer interface"
for example.

Even if you persist on keeping it in its own magic driver
like this, it should implement the alarm timer interface
from <linux/alarmtimer.h> and I bet after that you don't
need the sysfs files anymore, as the system will sleep
and wake up from the regular syscalls instead of using
magic poking in sysfs from userspace. AFAICT this hardware
is designed for exactly this usecase.

tools/testing/selftests/timers/alarmtimer-suspend.c
is there for you to test your driver with alarmtimer
support.

Needless to say: if you implement it as an RTC you get the
alarmtimer interaction for free. That is why we have the
subsystem after all: to be helpful.

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web