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