On Fri, Apr 29, 2011 at 08:29:55AM +0400, Igor Plyatov wrote:
>>> The latter will definitely mess things up, right?
>> I mean, one surely can register an IRQ resource with both flags set. And
>> if the underlying hardware works as it is described (i.e. raises an irq
>> on both edges) then it will be a problem.
>
> Please don't try to abandon one of ASSERT or CLEAR events!
> It is very useful to register both of them, because in this case its
> possible to measure pulse width and decode PPS signals like DCF77.
At this point I suppose we should add both ASSERT and CLEAR events...
Ciao,
Rodolfo
--
GNU/Linux Solutions e-mail: [email protected]
Linux Device Driver [email protected]
Embedded Systems phone: +39 349 2432127
UNIX programming skype: rodolfo.giometti
Freelance ICT Italia - Consulente ICT Italia - http://www.consulenti-ict.it
Thanks for you comments Igor, Alexander and Rodolfo.
You all make a good point that it would be useful to also register a
CLEAR event. Before I go ahead and implement any changes I would like
clarify a couple of things.
My original patch which only included the ASSERT event was simple
because all you needed to do was register an IRQ resource with a
particular edge trigger and you would get an ASSERT event on that
edge. It becomes a little more complicated when you want to capture
both ASSERT and CLEAR events because you also have to indicate to the
driver which edge is the ASSERT and which is the CLEAR. This is
because I presume you could have systems that generate a pulse which
transitions from low->high->low OR a pulse which transitions from
high->low->high, and we would want a driver which is generic enough to
handle both cases. Is my understanding correct that an ASSERT event is
simply the first edge of a single pulse, regardless of "polarity"?
I suppose the driver could be intelligent enough to figure out which
is the first edge on the assumption that the pulse width is always
very small - at least less than 0.5 second, more likely less than a
few hundred milliseconds. Obviously it would need at least 2 pulses
before it decides to register either an ASSERT or CLEAR event. Is it
safe enough to say that an ASSERT event will only be generated if the
previous edge was greater than 0.5 second ago, otherwise a CLEAR event
will be generated?
If I was to implement the driver this way then you would have exactly
3 ways to register a device to use this driver:
1) Register an IRQ with only IORESOURCE_IRQ_HIGHEDGE set:
This will generate an ASSERT event on rising edges (no CLEAR events)
2) Register an IRQ with only IORESOURCE_IRQ_LOWEDGE set:
This will generate an ASSERT event on falling edges (no CLEAR events)
3) Register an IRQ with both IORESOURCE_IRQ_LOWEDGE and
IORESOURCE_IRQ_HIGHEDGE set:
This will generate ASSERT and CLEAR events with the driver dynamically
determining which edge is the ASSERT based on the logic above.
I hope this covers all the potential use cases.
cheers,
James
On Fri, Apr 29, 2011 at 4:26 AM, Rodolfo Giometti <[email protected]> wrote:
> On Fri, Apr 29, 2011 at 08:29:55AM +0400, Igor Plyatov wrote:
>
>>>> The latter will definitely mess things up, right?
>>> I mean, one surely can register an IRQ resource with both flags set. And
>>> if the underlying hardware works as it is described (i.e. raises an irq
>>> on both edges) then it will be a problem.
>>
>> Please don't try to abandon one of ASSERT or CLEAR events!
>> It is very useful to register both of them, because in this case its
>> possible to measure pulse width and decode PPS signals like DCF77.
>
> At this point I suppose we should add both ASSERT and CLEAR events...
>
> Ciao,
>
> Rodolfo
>
> --
>
> GNU/Linux Solutions ? ? ? ? ? ? ? ? ?e-mail: [email protected]
> Linux Device Driver ? ? ? ? ? ? ? ? ? ? ? ? [email protected]
> Embedded Systems ? ? ? ? ? ? ? ? ? ? phone: ?+39 349 2432127
> UNIX programming ? ? ? ? ? ? ? ? ? ? skype: ?rodolfo.giometti
> Freelance ICT Italia - Consulente ICT Italia - http://www.consulenti-ict.it
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at ?http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at ?http://www.tux.org/lkml/
>
Hi James!
> Thanks for you comments Igor, Alexander and Rodolfo.
>
> You all make a good point that it would be useful to also register a
> CLEAR event. Before I go ahead and implement any changes I would like
> clarify a couple of things.
>
> My original patch which only included the ASSERT event was simple
> because all you needed to do was register an IRQ resource with a
> particular edge trigger and you would get an ASSERT event on that
> edge. It becomes a little more complicated when you want to capture
> both ASSERT and CLEAR events because you also have to indicate to the
> driver which edge is the ASSERT and which is the CLEAR. This is
> because I presume you could have systems that generate a pulse which
> transitions from low->high->low OR a pulse which transitions from
> high->low->high, and we would want a driver which is generic enough to
> handle both cases. Is my understanding correct that an ASSERT event is
> simply the first edge of a single pulse, regardless of "polarity"?
>
> I suppose the driver could be intelligent enough to figure out which
> is the first edge on the assumption that the pulse width is always
> very small - at least less than 0.5 second, more likely less than a
> few hundred milliseconds. Obviously it would need at least 2 pulses
> before it decides to register either an ASSERT or CLEAR event. Is it
> safe enough to say that an ASSERT event will only be generated if the
> previous edge was greater than 0.5 second ago, otherwise a CLEAR event
> will be generated?
This way can lead to problems when developer tries to debug PPS
subsystem with some specific signal parameters.
In my opinion it much safer to explicitly declare ".active_low = 0" or
".active_low = 1" in the platform initialization code.
See example:
/*
* pps client gpio
*/
static struct pps_client_gpio pps_client_gpios[] = {
{
.gpio = GPI_PPS0_IN,
.active_low = 0, /* ASSERT is a _/ */
.desc = "pps0 source",
.decoder_type = PPS_DECODER_GEOSIG_T1PPS,
},
{
.gpio = GPI_PPS1_IN,
.active_low = 1, /* ASSERT is a \_ */
.desc = "pps1 source",
}
};
static struct pps_client_gpio_platform_data pps_client_gpios_data = {
.pclient = pps_client_gpios,
.num_gpios = ARRAY_SIZE(pps_client_gpios),
};
static struct platform_device pps_client_gpio_device = {
.name = "pps-client-gpio",
.id = 0,
.dev = {
.platform_data = &pps_client_gpios_data,
}
};
> If I was to implement the driver this way then you would have exactly
> 3 ways to register a device to use this driver:
>
> 1) Register an IRQ with only IORESOURCE_IRQ_HIGHEDGE set:
> This will generate an ASSERT event on rising edges (no CLEAR events)
>
> 2) Register an IRQ with only IORESOURCE_IRQ_LOWEDGE set:
> This will generate an ASSERT event on falling edges (no CLEAR events)
>
> 3) Register an IRQ with both IORESOURCE_IRQ_LOWEDGE and
> IORESOURCE_IRQ_HIGHEDGE set:
> This will generate ASSERT and CLEAR events with the driver dynamically
> determining which edge is the ASSERT based on the logic above.
>
> I hope this covers all the potential use cases.
>
> cheers,
> James
>
>
> On Fri, Apr 29, 2011 at 4:26 AM, Rodolfo Giometti<[email protected]> wrote:
>> On Fri, Apr 29, 2011 at 08:29:55AM +0400, Igor Plyatov wrote:
>>
>>>>> The latter will definitely mess things up, right?
>>>> I mean, one surely can register an IRQ resource with both flags set. And
>>>> if the underlying hardware works as it is described (i.e. raises an irq
>>>> on both edges) then it will be a problem.
>>> Please don't try to abandon one of ASSERT or CLEAR events!
>>> It is very useful to register both of them, because in this case its
>>> possible to measure pulse width and decode PPS signals like DCF77.
>> At this point I suppose we should add both ASSERT and CLEAR events...
>>
>> Ciao,
>>
>> Rodolfo
>>
>> --
>>
>> GNU/Linux Solutions e-mail: [email protected]
>> Linux Device Driver [email protected]
>> Embedded Systems phone: +39 349 2432127
>> UNIX programming skype: rodolfo.giometti
>> Freelance ICT Italia - Consulente ICT Italia - http://www.consulenti-ict.it
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> the body of a message to [email protected]
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at http://www.tux.org/lkml/
>>
Best regards!
--
Igor Plyatov
Hi Igor,
I agree, platform_data is a good candidate for this type of parameter.
Where is the appropriate place to keep the header file containing the
struct declaration(s)? e.g in your case "struct
pps_client_gpio_platform_data" and "struct pps_client_gpio" Presumably
it needs to be in a readily accessible header file so the board setup
code can use it.
cheers,
James
>
> This way can lead to problems when developer tries to debug PPS subsystem
> with some specific signal parameters.
> In my opinion it much safer to explicitly declare ".active_low = 0" or
> ".active_low = 1" in the platform initialization code.
> See example:
>
> /*
> ?* pps client gpio
> ?*/
> static struct pps_client_gpio pps_client_gpios[] = {
> ? ?{
> ? ? ? ?.gpio ? ? ? ? ? ?= GPI_PPS0_IN,
> ? ? ? ?.active_low ? ? ? ?= 0, /* ASSERT is a _/ */
> ? ? ? ?.desc ? ? ? ? ? ?= "pps0 source",
> ? ? ? ?.decoder_type ? ? ? ?= PPS_DECODER_GEOSIG_T1PPS,
> ? ?},
> ? ?{
> ? ? ? ?.gpio ? ? ? ? ? ?= GPI_PPS1_IN,
> ? ? ? ?.active_low ? ? ? ?= 1, /* ASSERT is a \_ */
> ? ? ? ?.desc ? ? ? ? ? ?= "pps1 source",
> ? ?}
> };
>
> static struct pps_client_gpio_platform_data pps_client_gpios_data = {
> ? ?.pclient ? ?= pps_client_gpios,
> ? ?.num_gpios ? ?= ARRAY_SIZE(pps_client_gpios),
> };
>
> static struct platform_device pps_client_gpio_device = {
> ? ?.name ? ? ? ?= "pps-client-gpio",
> ? ?.id ? ? ? ?= 0,
> ? ?.dev ? ? ? ?= {
> ? ? ? ?.platform_data ? ?= &pps_client_gpios_data,
> ? ?}
> };
>
>
>> If I was to implement the driver this way then you would have exactly
>> 3 ways to register a device to use this driver:
>>
>> 1) Register an IRQ with only IORESOURCE_IRQ_HIGHEDGE set:
>> This will generate an ASSERT event on rising edges (no CLEAR events)
>>
>> 2) Register an IRQ with only IORESOURCE_IRQ_LOWEDGE set:
>> This will generate an ASSERT event on falling edges (no CLEAR events)
>>
>> 3) Register an IRQ with both IORESOURCE_IRQ_LOWEDGE and
>> IORESOURCE_IRQ_HIGHEDGE set:
>> This will generate ASSERT and CLEAR events with the driver dynamically
>> determining which edge is the ASSERT based on the logic above.
>>
>> I hope this covers all the potential use cases.
>>
>> cheers,
>> James
>>
>>
>> On Fri, Apr 29, 2011 at 4:26 AM, Rodolfo Giometti<[email protected]>
>> ?wrote:
>>>
>>> On Fri, Apr 29, 2011 at 08:29:55AM +0400, Igor Plyatov wrote:
>>>
>>>>>> The latter will definitely mess things up, right?
>>>>>
>>>>> I mean, one surely can register an IRQ resource with both flags set.
>>>>> And
>>>>> if the underlying hardware works as it is described (i.e. raises an irq
>>>>> on both edges) then it will be a problem.
>>>>
>>>> Please don't try to abandon one of ASSERT or CLEAR events!
>>>> It is very useful to register both of them, because in this case its
>>>> possible to measure pulse width and decode PPS signals like DCF77.
>>>
>>> At this point I suppose we should add both ASSERT and CLEAR events...
>>>
>>> Ciao,
>>>
>>> Rodolfo
>>>
>>> --
>>>
>>> GNU/Linux Solutions ? ? ? ? ? ? ? ? ?e-mail: [email protected]
>>> Linux Device Driver ? ? ? ? ? ? ? ? ? ? ? ? [email protected]
>>> Embedded Systems ? ? ? ? ? ? ? ? ? ? phone: ?+39 349 2432127
>>> UNIX programming ? ? ? ? ? ? ? ? ? ? skype: ?rodolfo.giometti
>>> Freelance ICT Italia - Consulente ICT Italia - http://www.consulenti-ict.it
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-kernel"
>>> in
>>> the body of a message to [email protected]
>>> More majordomo info at ?http://vger.kernel.org/majordomo-info.html
>>> Please read the FAQ at ?http://www.tux.org/lkml/
>>>
>
> Best regards!
>
> --
> Igor Plyatov
>
>
Hi James!
> Hi Igor,
>
> I agree, platform_data is a good candidate for this type of parameter.
> Where is the appropriate place to keep the header file containing the
> struct declaration(s)? e.g in your case "struct
> pps_client_gpio_platform_data" and "struct pps_client_gpio" Presumably
> it needs to be in a readily accessible header file so the board setup
> code can use it.
This header file must be "include/linux/pps-client-gpio.h".
Here is its content:
/*
* pps-client-gpio.h -- PPS client for IRQ capable GPIO
*
*
* Copyright (C) 2011 Igor Plyatov <[email protected]>
*
* This program is free software; you can redistribute it and/or modify
* it under the terms of the GNU General Public License as published by
* the Free Software Foundation; either version 2 of the License, or
* (at your option) any later version.
*
* This program is distributed in the hope that 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, write to the Free Software
* Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
*/
#ifndef _PPS_CLIENT_GPIO_H
#define _PPS_CLIENT_GPIO_H
struct pps_client_gpio {
/* Configuration parameters */
unsigned gpio;
unsigned active_low;
const char *desc;
unsigned char decoder_type;
};
struct pps_client_gpio_platform_data {
struct pps_client_gpio *pclient;
int num_gpios;
};
#endif
> cheers,
> James
>
>> This way can lead to problems when developer tries to debug PPS subsystem
>> with some specific signal parameters.
>> In my opinion it much safer to explicitly declare ".active_low = 0" or
>> ".active_low = 1" in the platform initialization code.
>> See example:
>>
>> /*
>> * pps client gpio
>> */
>> static struct pps_client_gpio pps_client_gpios[] = {
>> {
>> .gpio = GPI_PPS0_IN,
>> .active_low = 0, /* ASSERT is a _/ */
>> .desc = "pps0 source",
>> .decoder_type = PPS_DECODER_GEOSIG_T1PPS,
>> },
>> {
>> .gpio = GPI_PPS1_IN,
>> .active_low = 1, /* ASSERT is a \_ */
>> .desc = "pps1 source",
>> }
>> };
>>
>> static struct pps_client_gpio_platform_data pps_client_gpios_data = {
>> .pclient = pps_client_gpios,
>> .num_gpios = ARRAY_SIZE(pps_client_gpios),
>> };
>>
>> static struct platform_device pps_client_gpio_device = {
>> .name = "pps-client-gpio",
>> .id = 0,
>> .dev = {
>> .platform_data =&pps_client_gpios_data,
>> }
>> };
>>
>>
>>> If I was to implement the driver this way then you would have exactly
>>> 3 ways to register a device to use this driver:
>>>
>>> 1) Register an IRQ with only IORESOURCE_IRQ_HIGHEDGE set:
>>> This will generate an ASSERT event on rising edges (no CLEAR events)
>>>
>>> 2) Register an IRQ with only IORESOURCE_IRQ_LOWEDGE set:
>>> This will generate an ASSERT event on falling edges (no CLEAR events)
>>>
>>> 3) Register an IRQ with both IORESOURCE_IRQ_LOWEDGE and
>>> IORESOURCE_IRQ_HIGHEDGE set:
>>> This will generate ASSERT and CLEAR events with the driver dynamically
>>> determining which edge is the ASSERT based on the logic above.
>>>
>>> I hope this covers all the potential use cases.
>>>
>>> cheers,
>>> James
>>>
>>>
>>> On Fri, Apr 29, 2011 at 4:26 AM, Rodolfo Giometti<[email protected]>
>>> wrote:
>>>> On Fri, Apr 29, 2011 at 08:29:55AM +0400, Igor Plyatov wrote:
>>>>
>>>>>>> The latter will definitely mess things up, right?
>>>>>> I mean, one surely can register an IRQ resource with both flags set.
>>>>>> And
>>>>>> if the underlying hardware works as it is described (i.e. raises an irq
>>>>>> on both edges) then it will be a problem.
>>>>> Please don't try to abandon one of ASSERT or CLEAR events!
>>>>> It is very useful to register both of them, because in this case its
>>>>> possible to measure pulse width and decode PPS signals like DCF77.
>>>> At this point I suppose we should add both ASSERT and CLEAR events...
>>>>
>>>> Ciao,
>>>>
>>>> Rodolfo
>>>>
>>>> --
>>>>
>>>> GNU/Linux Solutions e-mail: [email protected]
>>>> Linux Device Driver [email protected]
>>>> Embedded Systems phone: +39 349 2432127
>>>> UNIX programming skype: rodolfo.giometti
>>>> Freelance ICT Italia - Consulente ICT Italia - http://www.consulenti-ict.it
>>>> --
>>>> To unsubscribe from this list: send the line "unsubscribe linux-kernel"
>>>> in
>>>> the body of a message to [email protected]
>>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>> Please read the FAQ at http://www.tux.org/lkml/
>>>>
>> Best regards!
>>
>> --
>> Igor Plyatov
>>
>>
Best regards!
--
Igor Plyatov