Re: [PATCH v3 3/9] gpio: 104-dio-48e: Migrate to the regmap-irq API
From: Andy Shevchenko
Date: Wed Nov 23 2022 - 10:02:13 EST
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