On 18-11-06 20:50, Trent Piepho wrote:
> On Mon, 2018-11-05 at 09:19 +0100, Marco Felsch wrote:
> > On 18-11-02 23:05, Trent Piepho wrote:
> > > On Fri, 2018-11-02 at 07:38 +0100, Marco Felsch wrote:
> > > >
> > > > > Interrupts types are specific to each interrupt controller, but there
> > > > > is a standard set of flags that, AFAIK, every Linux controller uses.
> > > > > These include IRQ_TYPE_EDGE_BOTH, IRQ_TYPE_EDGE_RISING,
> > > > > IRQ_TYPE_LEVEL_HIGH, and so on.
> > > > >
> > > > > So you can support hardware that is inherently edge or level triggered.
> > > >
> > > > I've been spoken about gpio-controllers and AFAIK there are no edge
> > > > types. Interrupt-Controller are a different story, as you pointed out
> > > > above.
> > >
> > > You can use edge triggering with gpios. Just try writing "rising" or
> > > "falling" into /sys/class/gpio/gpioX/edge
> >
> > Can we access the gpios trough the sysfs if they are requested by a
> > driver?
>
> When I first did the sysfs interface for gpios, you could do that, but
> David Brownell wanted it so that you can't access gpios via sysfs if a
> driver requested them. The compromise was that *kernel* code can
> explicitly export to sysfs a gpio that is used by a driver (ie. also
> requested in kernel code), but you couldn't do it just from userspace.
>
> But that's irrelevant here. The point is that you can get edge
> triggered interrupts on a gpio and if you don't believe me, just try it
> for yourself and you'll see it works. The sysfs interface just
> translates into the same calls a kernel driver could make.
>
> > > It's level you can't do sysfs. The irq masking necessary isn't
> > > supported to get it to work in a useful way, i.e. without a live-lock
> > > IRQ loop.
> > >
> > > But you can in the kernel.
> > >
> > > Normal process is to call gpiod_to_irq() and then use standard IRQF
> > > flags to select level, edge, etc.
> >
> > Currently I using the gpiod_to_irq() function to convert the sense gpio
> > into a irq, but I do some magic to determine the edge. I tought there
> > might be reasons why there are no edge defines in
> > include/dt-bindings/gpio/gpio.h.
>
> Just request the interrupt with IRQF_TRIGGER_RISING and it will work on
> almost any SoC. The reason you see no edge defines with gpio handles
> is that edge and level triggering is a interrupt concept, not a gpio
> concept. There are no level triggers defined for gpios either. The
> active low/high flags just define what voltage should be considered
> "asserted". They aren't intended to be related to interrupts.
Okay, thanks for this hint. So a gpio marked as GPIO_ACTIVE_LOW will
trigger a IRQF_TRIGGER_RISING requested interrupt if the gpio level
(electrical) goes from 1->0? I didn't knew that.
>
> > Okay, so no polling for the current solution. Let me summarize our
> > solution:
> > - no polling (currently)
> > - dt-node specifies a gpio instead of a interrupt
> > -> gpio <-> irq mapping is done by gpiod_to_irq() and fails if gpio
> > doesn't support irq's
> > - more alarms per sensor
> >
> > Only one last thing I tought about:
> >
> > Using a flat design like you mentioned would lead into a "virtual"
> > address conflict, since both sensors are on the same level. I tought
> > about i2c/spi/muxes/graph-devices which don't support such "addressing"
> > scheme.
>
> You mean a temp alarm and a voltage alarm could both be reg = <1>? I
> don't think anything complains about that. But it does seem
> undesirable.
Yes, because both types are on the same hierarchy level. As I said it is
more a dt-convention decision.
> > hwmon_dev {
> > compatible = "gpio-alarm";
> > bat@0 {
> > reg = <0>;
> > label = "Battery Pack1 Voltage";
> > type = "voltage";
> > alarm-type = <GPIO_ALARM_LCRIT, GPIO_ALARM_CRIT>;
>
> Would have to be <GPIO_ALARM_LCRIT>, <GPIO_ALARM_CRIT>;
>
> I'm not sure if dt bindings prefer symbolic integer constants vs
> strings for something which is an enumeration like this. strings seem
> more common to me, e.g. alarm-types = "lcrit", "crit";
That's a good question. I term of parsing, the non string variant should
be faster. I don't have any preference, but will try the string variant
first ;)
Regards,
Marco
> > alarm-gpios = <&gpio3 15 GPIO_ACTIVE_LOW
> > &gpio3 16 GPIO_ACTIVE_LOW>;
> > };
> > bat@1 {
> > reg = <1>;
> > label = "Battery Pack2 Voltage";
> > alarm-type = <GPIO_ALARM_LCRIT, GPIO_ALARM_CRIT>;
> > alarm-gpios = <&gpio3 9 GPIO_ACTIVE_LOW
> > &gpio3 1 GPIO_ACTIVE_LOW>;
> > };
> > cputemp@0 {
> > reg = <0>;
> > label = "CPU Temperature Critical";
> > type = "temperature";
> > alarm-type = <GPIO_ALARM_GENRIC>;
> > alarm-gpios = <&gpio4 17 GPIO_ACTIVE_LOW>;
> > };
> > };
> >
> > Where a more structured layout don't have this "issue".
> >
> > hwmon_dev {
> > compatible = "gpio-alarm";
> >
> > voltage {
> > bat@0 {
> > reg = <0>;
> > label = "Battery Pack1 Voltage";
> > alarm-type = <GPIO_ALARM_LCRIT, GPIO_ALARM_CRIT>;
> > alarm-gpios = <&gpio3 15 GPIO_ACTIVE_LOW
> > &gpio3 16 GPIO_ACTIVE_LOW>;
> > };
> > bat@1 {
> > reg = <1>;
> > label = "Battery Pack2 Voltage";
> > alarm-type = <GPIO_ALARM_LCRIT, GPIO_ALARM_CRIT>;
> > alarm-gpios = <&gpio3 9 GPIO_ACTIVE_LOW
> > &gpio3 1 GPIO_ACTIVE_LOW>;
> > };
> > };
> > temperature {
> > cputemp {
> > label = "CPU Temperature Critical";
> > alarm-type = <GPIO_ALARM_GENRIC>;
> > alarm-gpios = <&gpio4 17 GPIO_ACTIVE_LOW>;
> > };
> > };
> > };
> >
> > We don't have to take this layout, we can also consider about devices:
> >
> > hwmon_dev {
> > compatible = "gpio-alarm";
> >
> > dev@0 {
> > reg = <0>;
> > voltage {
> > label = "Battery Pack1 Voltage";
> > alarm-type = <GPIO_ALARM_LCRIT, GPIO_ALARM_CRIT>;
> > alarm-gpios = <&gpio3 15 GPIO_ACTIVE_LOW
> > &gpio3 16 GPIO_ACTIVE_LOW>;
> > };
> > temperature {
> > label = "Battery Pack1 Temperature Critical";
> > alarm-type = <GPIO_ALARM_GENRIC>;
> > alarm-gpios = <&gpio4 17 GPIO_ACTIVE_LOW>;
> > };
> > };
> > dev@1 {
> > reg = <1>;
> > temperature {
> > label = "CPU Temperature Critical";
> > alarm-type = <GPIO_ALARM_GENRIC>;
> > alarm-gpios = <&gpio4 19 GPIO_ACTIVE_LOW>;
> > };
> > };
> > };
> >
> > I don't think that is a issue at all, but I don't know the dt
> > maintainers opinion of this theme.
> >
> > Regards,
> > Marco
