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

[Aidan MacDonald <aidanmacdonald.0x0@gmail.com>]

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