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

From: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
To: Matti Vaittinen <mazziesaccount@gmail.com>
Cc: Mark Brown <broonie@kernel.org>,
	Linus Walleij <linus.walleij@linaro.org>,
	Bartosz Golaszewski <brgl@bgdev.pl>,
	Rob Herring <robh+dt@kernel.org>,
	krzysztof.kozlowski+dt@linaro.org, Chen-Yu Tsai <wens@csie.org>,
	jic23@kernel.org, Lee Jones <lee.jones@linaro.org>,
	Sebastian Reichel <sre@kernel.org>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Liam Girdwood <lgirdwood@gmail.com>,
	lars@metafoo.de, "Rafael J . Wysocki" <rafael@kernel.org>,
	quic_gurus@quicinc.com,
	"open list:GPIO SUBSYSTEM" <linux-gpio@vger.kernel.org>,
	devicetree <devicetree@vger.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	linux-iio@vger.kernel.org,
	Linux PM list <linux-pm@vger.kernel.org>
Subject: Re: [PATCH v2 02/17] regmap-irq: Add get_irq_reg to support unusual register layouts
Date: Thu, 23 Jun 2022 21:35:47 +0100	[thread overview]
Message-ID: <AX8g6eUgSrGJYBxU1YYNt2559CtK9b5G@localhost> (raw)
In-Reply-To: <CANhJrGMqUmnSvyNRgRyp40YnGQkD3N_2AZLn94NDp+4RG0_x5w@mail.gmail.com>

Matti Vaittinen <mazziesaccount@gmail.com> writes:

> Hi dee Ho peeps!
>
> Sorry for the late reply.
>
> pe 10. kesäk. 2022 klo 18.43 Aidan MacDonald
> (aidanmacdonald.0x0@gmail.com) kirjoitti:
>>
>> 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.
>>
>> - 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.
>
> Does this work for devices where a single main status bit can flag
> IRQs in more than one sub-registers?
>
> Best Regards
>  -- Matti

No, I realized once I got into the refactor that what I outlined here
wouldn't fit that use case well, which is what rohm-bd71828 needs.

There are some other complications with this approach, like how to
go between IRQs and register groups efficiently, and it's generally
a rather heavyweight solution. It might be useful for handling very
hierarchical chips, but I couldn't justify the added complexity when
most chips don't need it -- after all most chips behind slow busses
will have a small number of interrupts and a fairly flat structure.

In the end I went with Mark's suggestion to factor things out around
->get_irq_reg(). At first I thought there might be too many "gotchas"
that'd limit its usefulness, but in the end it proved to be a better
option and a lot easier to implement.