Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 23 Nov 2022 17:01:53 +0200
From:   Andy Shevchenko <andriy.shevchenko@linux.intel.com>
To:     William Breathitt Gray <william.gray@linaro.org>
Cc:     linus.walleij@linaro.org, brgl@bgdev.pl,
        linux-gpio@vger.kernel.org, linux-kernel@vger.kernel.org,
        michael@walle.cc, broonie@kernel.org
Subject: Re: [PATCH v3 3/9] gpio: 104-dio-48e: Migrate to the regmap-irq API
Message-ID: <Y3414YhVjqKakddV@smile.fi.intel.com>
References: <cover.1669100542.git.william.gray@linaro.org>
 <80fc819bcafe9697b6e02c0750d3cf0ea4ec9e1b.1669100542.git.william.gray@linaro.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <80fc819bcafe9697b6e02c0750d3cf0ea4ec9e1b.1669100542.git.william.gray@linaro.org>
Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo
Precedence: bulk

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