lkml.org 
[lkml]   [2022]   [Nov]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v3 3/9] gpio: 104-dio-48e: Migrate to the regmap-irq API
    On Tue, Nov 22, 2022 at 02:11:00AM -0500, William Breathitt Gray wrote:
    > The regmap API supports IO port accessors so we can take advantage of
    > regmap abstractions rather than handling access to the device registers
    > directly in the driver.
    >
    > For the 104-dio-48e we have the following IRQ registers (0xB and 0xF):
    >
    > Base Address +B (Write): Enable Interrupt
    > Base Address +B (Read): Disable Interrupt
    > Base Address +F (Read/Write): Clear Interrupt
    >
    > Any write to 0xB will enable interrupts, while any read will disable
    > interrupts. Interrupts are cleared by a read or any write to 0xF.
    > There's no IRQ status register, so software has to assume that if an
    > interrupt is raised then it was for the 104-DIO-48E device.

    ...

    > +/* only bit 3 on each respective Port C supports interrupts */
    > +#define DIO48E_REGMAP_IRQ(_ppi) \
    > + [19 + (_ppi) * 24] = { \
    > + .mask = BIT(_ppi), \
    > + .type = { .types_supported = IRQ_TYPE_EDGE_RISING, }, \

    When {} on a single line, the trailing comma is not needed.

    .type = { .types_supported = IRQ_TYPE_EDGE_RISING }, \

    would work as well.

    A nit: I would put \ on the same column by using TABs before each of them.

    > }

    ...

    > + /* if all previously masked, enable interrupts when unmasking */
    > + if (prev_mask == all_masked) {
    > + err = regmap_write(map, DIO48E_ENABLE_INTERRUPT, 0x00);
    > + if (err)
    > + return err;
    > + /* if all are currently masked, disable interrupts */
    > + } else if (mask_buf == all_masked) {
    > + err = regmap_read(map, DIO48E_DISABLE_INTERRUPT, &val);
    > + if (err)
    > + return err;
    > + }

    Haven't looked at the rest of the series, but if there is nothing with this
    code piece, the above can be optimized to

    if (prev_mask == all_masked)
    return regmap_write(map, DIO48E_ENABLE_INTERRUPT, 0x00);

    if (mask_buf == all_masked)
    return regmap_read(map, DIO48E_DISABLE_INTERRUPT, &val);

    ...

    > + /* Initialize device interrupt state */
    > + err = regmap_read(map, DIO48E_DISABLE_INTERRUPT, &val);
    > + if (err)
    > + return err;

    Use ->init_hw() callback for this.

    ...

    > + err = devm_regmap_add_irq_chip(dev, map, irq[id], 0, 0, chip,
    > + &chip_data);

    I would leave this on one line. It's only 82.

    > + if (err) {
    > + dev_err(dev, "IRQ registration failed (%d)\n", err);
    > + return err;
    > + }

    --
    With Best Regards,
    Andy Shevchenko


    \
     
     \ /
      Last update: 2022-11-23 16:04    [W:3.177 / U:0.048 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site