Re: [PATCH v2 11/12] regmap-irq: Add get_irq_reg() callback

Aidan MacDonald
2022-07-03T11:05:57Z

Marek Szyprowski <m.szyprowski@samsung.com> writes:

> Hi All,
>
> On 23.06.2022 23:14, Aidan MacDonald wrote:
>> Replace the internal sub_irq_reg() function with a public callback
>> that drivers can use when they have more complex register layouts.
>> The default implementation is regmap_irq_get_irq_reg_linear(), used
>> if the chip doesn't provide its own callback.
>>
>> Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
>> ---
>
> This patch landed in today's linux next-20220701 as commit bdf9b86cd3ad
> ("regmap-irq: Add get_irq_reg() callback"). I've noticed that it causes
> a regression on my test systems: the RTC alarm stopped working on all
> boards with Samsung PMICs (drivers/mfd/sec*.c). There are no
> warnings/oopses/etc. Waitng for the RTC alarm lasts forever, so it looks
> that something is broken with interrupts. Reverting it on top of
> linux-next fixes the issue.
>
> Unfortunately I'm going for a holidays for the whole next week and I'm
> not able to analyze this issue further today. Krzysztof: maybe You will
> be able to provide some more hints which regmap irq variant is broken?

Fortunately it's nothing complicated -- just a typo. Thanks for testing.

>> drivers/base/regmap/regmap-irq.c | 126 ++++++++++++++++++++-----------
>> include/linux/regmap.h | 15 +++-
>> 2 files changed, 93 insertions(+), 48 deletions(-)
>> [..]
>> +unsigned int regmap_irq_get_irq_reg_linear(struct regmap_irq_chip_data *data,
>> + unsigned int base, int index)
>> +{
>> + const struct regmap_irq_chip *chip = data->chip;
>> + struct regmap *map = data->map;
>> +
>> + /*
>> + * FIXME: This is for backward compatibility and should be removed
>> + * when not_fixed_stride is dropped (it's only used by qcom-pm8008).
>> + */
>> + if (chip->not_fixed_stride && chip->sub_reg_offsets) {
>> + struct regmap_irq_sub_irq_map *subreg;
>> +
>> + subreg = &chip->sub_reg_offsets[0];
>> + return base + subreg->offset[0];
>> + }
>> +
>> + return base + index * map->reg_stride * chip->irq_reg_stride;

chip->irq_reg_stride is usually 0. This should be data->irq_reg_stride,
which will be corrected to the default value of 1.

>> +}
>> +EXPORT_SYMBOL_GPL(regmap_irq_get_irq_reg_linear);
>> +
>> /**
>> * regmap_irq_set_type_config_simple() - Simple IRQ type configuration callback.
>> * @buf: Buffer containing configuration register values, this is a 2D array of
>> @@ -862,6 +891,11 @@ int regmap_add_irq_chip_fwnode(struct fwnode_handle *fwnode,
>
> Best regards