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, 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.
> 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";
> 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
