lkml.org 
[lkml]   [2021]   [Jun]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v1] hwmon: (lm90) Use edge-triggered interrupt
    On Thu, Jun 17, 2021 at 05:46:33PM +0300, Dmitry Osipenko wrote:
    > 17.06.2021 17:13, Guenter Roeck пишет:
    > ...
    > >> This is a device-tree based system, in particular it's NVIDIA Tegra30
    > >> Nexus 7. The interrupt support was originally added to the lm90 driver
    > >> by Wei Ni who works at NVIDIA and did it for the Tegra boards. The Tegra
    > >> device-trees are specifying the trigger mask and apparently they all are
    > >> cargo-culted and wrong because they use IRQ_TYPE_LEVEL_HIGH, while it
    > >
    > > Be fair, no one is perfect.
    >
    > This is a very minor problem, so no wonder that nobody noticed or
    > bothered to fix it yet. I'm just clarifying the status here.
    >
    > >> should be IRQ_TYPE_EDGE_FALLING.
    > >
    > > It should probably be both IRQ_TYPE_EDGE_FALLING and IRQ_TYPE_EDGE_RISING,
    >
    > For now I see that the rising edge isn't needed, the TEMP_ALERT goes
    > HIGH by itself when temperature backs to normal. But I will try to
    > double check.
    >
    The point is that a sysfs event should be sent to userspace on both
    edges, not only when an alarm is raised. But, you are correct,
    IRQ_TYPE_EDGE_RISING is currently not needed since sysfs events
    are not generated.

    > > and the interrupt handler should call hwmon_notify_event() instead of
    > > clogging the kernel log, but that should be done in a separate patch.
    >
    > Thank you for suggestion, I will take a look.
    >
    > > Anyway, the tegra30 dts files in the upstream kernel either use
    > > IRQ_TYPE_LEVEL_LOW or no interrupts for nct1008. The Nexus 7 dts file
    > > in the upstream kernel has no interrupt configured (and coincidentally
    > > it was you who added that entry). Where do you see IRQ_TYPE_LEVEL_HIGH ?
    >
    > I have a patch that will add the interrupt property, it's stashed
    > locally for the next kernel release.
    >
    > IIUC, it's not only the Tegra30 dts, but all the TegraXXX boards that
    > use IRQ_TYPE_LEVEL_LOW are in the same position.

    I still don't see a IRQ_TYPE_LEVEL_HIGH, though.

    Thanks,
    Guenter

    >
    > >> The IRQF flag in devm_request_threaded_irq() overrides the trigger mask
    > >> specified in a device-tree. IIUC, the interrupt is used only by OF-based
    > >> devices, hence I think we could simply remove the IRQF flag from the
    > >> code and fix the device-trees. Does it sound good to you?
    > >
    > > Yes, that is a better approach.
    >
    > Thank you for reviewing this patch. I'll prepare v2.

    \
     
     \ /
      Last update: 2021-06-17 17:13    [W:4.352 / U:0.140 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site