Re: [PATCH v3 3/9] gpio: 104-dio-48e: Migrate to the regmap-irq API

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

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