Re: [PATCH v2 02/17] regmap-irq: Add get_irq_reg to support unusual register layouts

From: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
To: Mark Brown <broonie@kernel.org>
Cc: linus.walleij@linaro.org, brgl@bgdev.pl, robh+dt@kernel.org,
	krzysztof.kozlowski+dt@linaro.org, wens@csie.org,
	jic23@kernel.org, lee.jones@linaro.org, sre@kernel.org,
	gregkh@linuxfoundation.org, lgirdwood@gmail.com, lars@metafoo.de,
	rafael@kernel.org, quic_gurus@quicinc.com,
	linux-gpio@vger.kernel.org, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org, linux-iio@vger.kernel.org,
	linux-pm@vger.kernel.org
Subject: Re: [PATCH v2 02/17] regmap-irq: Add get_irq_reg to support unusual register layouts
Date: Fri, 10 Jun 2022 16:40:20 +0100	[thread overview]
Message-ID: <6YJcC5wyOg6x6Ny4Os8ujFbK2qB4alkU@localhost> (raw)
In-Reply-To: <YqDLflKTsYaupArl@sirena.org.uk>

Mark Brown <broonie@kernel.org> writes:

> On Tue, Jun 07, 2022 at 04:53:09PM +0100, Aidan MacDonald wrote:
>
>> -	if (!chip->sub_reg_offsets || !chip->not_fixed_stride) {
>> +	if (chip->get_irq_reg) {
>> +		reg = chip->get_irq_reg(base_reg, i);
>> +	} else if (!chip->sub_reg_offsets || !chip->not_fixed_stride) {
>
> It seems like it would be cleaner and clearer to refactor things so that
> we always have a get_irq_reg() with standard chips getting given a
> default implementation which implements the current behaviour.

I don't think that is a good way to clean things up. I only intended
get_irq_reg() to be a quick hack to solve a problem; in my opinion it
would be a poor abstraction to base the API around.

What I'd suggest is something that will simplify regmap-irq. Instead of
defining the base registers, etc. in the chip, introduce a new struct
to describe a register group:

    struct regmap_irq_reg_group {
        unsigned int status_base;
        unsigned int mask_base;
        ...

        unsigned int irq_reg_stride;

        int num_regs;
    };

The idea is that the registers in a group are linearly mapped using the
formula "base + (i * irq_reg_stride)". Then it's possible to allow for
multiple register groups in regmap_irq_chip:

    struct regmap_irq_chip {
        const struct regmap_irq_reg_group *groups;
        unsigned int num_groups;

        unsigned int main_status_base;
        unsigned int num_main_status_bits;
        int num_main_regs;

        ...
    };

It should be straightforward to fit existing chips into this model.

- "Normal" chips which do not use sub_reg_offsets or not_fixed_stride
  will have a single register group describing their register layout.

- Chips which use not_fixed_stride=1 (eg. qcom-pm8008.c) will define
  multiple register groups instead of using sub_reg_offsets, so they
  will look more like a normal chip.

- Chips that use a main status + sub-block IRQ layout will define
  one register group for each sub-block and continue to describe the
  location of the main status registers inside of regmap_irq_chip.
  A group will only get polled if the corresponding main status bit
  is set -- n'th group is polled if n'th bit is set.

I think this scheme is easier to understand than having three or four
different hacks to deal with minor deviations from the simple cases.
It's also more flexible because groups do not need to be homogenous,
unlike the way sub_reg_offsets works.

For the AXP192, I'd just need to add two register groups, one for IRQ0-3
and another with IRQ4 off by itself. So this would remove the need for
get_irq_reg() entirely.

On the other hand, basing the public API around get_irq_reg() doesn't
appear to simplify any existing use case.

What do you think? If you're happy with the idea I don't mind doing the
refactoring in a separate patch series.

I had hoped to avoid a big refactor just to add one chip, though.

Best regards,
Aidan