Message-ID: <5fb06bdd-b1d2-4625-9e9a-1679c5e69713@gmail.com>
Date: Tue, 2 Sep 2025 17:23:52 +0200
From: Artur Weber <aweber.kernel@...il.com>
To: Lee Jones <lee@...nel.org>
Cc: linux-kernel@...r.kernel.org, ~postmarketos/upstreaming@...ts.sr.ht,
 Stanislav Jakubek <stano.jakubek@...il.com>
Subject: Re: [PATCH] mfd: bcm590xx: Add support for interrupt handling



On 9/2/25 09:50, Lee Jones wrote:
> On Sat, 16 Aug 2025, Artur Weber wrote:
> 
>> The BCM590XX supports up to 128 internal interrupts, which are used by
>> various parts of the chip. Add regmap_irq-based interrupt handling and
>> helper functions to allow subdevice drivers to easily use the interrupts.
>>
>> Signed-off-by: Artur Weber <aweber.kernel@...il.com>
>> ---
>> This patch is a prerequisite for future subdevice additions, since
>> many of them rely on the interrupts; I have a power-on key driver and
>> an RTC driver ready which both use the IRQ data/helper functions included
>> in this patch (they will be sent in subsequent patch series), and more
>> are on the way.

(...)

>> diff --git a/drivers/mfd/bcm590xx.c b/drivers/mfd/bcm590xx.c
>> index 5a8456bbd63f65b9260f05ef6546c026bf822bae..d688abd08c12b621a38586650843e55bd71ca715 100644
>> --- a/drivers/mfd/bcm590xx.c
>> +++ b/drivers/mfd/bcm590xx.c
>> @@ -26,16 +26,30 @@
>>   #define BCM590XX_PMUREV_ANA_MASK	0xF0
>>   #define BCM590XX_PMUREV_ANA_SHIFT	4
>>   
>> +#define BCM590XX_REG_IRQ1		0x20
>> +#define BCM590XX_REG_IRQ1_MASK		0x30
> 
> REG and MASK mean different things.  What is it?

IRQ1_MASK is the corresponding IRQ mask register for IRQ1. I agree that
the naming is confusing, would "IRQ1MASK" (without the underscore
separator) sound better?

> 
>> +
>>   static const struct mfd_cell bcm590xx_devs[] = {
>>   	{
>>   		.name = "bcm590xx-vregs",
>>   	},
>>   };
>>   
>> +static bool bcm590xx_volatile_pri(struct device *dev, unsigned int reg)
> 
> What does pri mean?

Primary I2C subdev/regmap, named after the bcm590xx_regmap_config_pri
struct (where the BCM590XX_REGMAP_PRI/SEC constants got their name as
well). Should I add a comment here?

> 
>> +{
>> +	/*
>> +	 * IRQ registers are clear-on-read, make sure we don't cache them
>> +	 * so that they get read/cleared correctly
>> +	 */
>> +	return (reg >= BCM590XX_REG_IRQ1 &&
>> +		reg <= (BCM590XX_REG_IRQ1 + 15));
> 
> Use up to 100-chars to prevent these line feeds.

Ack, will fix this.

> 
>> +}
>> +
>>   static const struct regmap_config bcm590xx_regmap_config_pri = {
>>   	.reg_bits	= 8,
>>   	.val_bits	= 8,
>>   	.max_register	= BCM590XX_MAX_REGISTER_PRI,
>> +	.volatile_reg	= bcm590xx_volatile_pri,
>>   	.cache_type	= REGCACHE_MAPLE,
>>   };
>>   
>> @@ -46,6 +60,268 @@ static const struct regmap_config bcm590xx_regmap_config_sec = {
>>   	.cache_type	= REGCACHE_MAPLE,
>>   };
>>   
>> +/** Interrupt handling **/
> 
> This is obvious with the alerting header comment.
IMO it makes scrolling through the driver code a bit less confusing (the
interrupt list is clearly split from the regmap config and later model
detection and probe code), but I'll remove it if needed.>
>> +/* IRQ IDs in the MFD header follow the IRQ order in hardware. */
> 
> Not sure I the helpfulness of this comment.

Indeed I suppose it's not particularily helpful. I'll remove it.

> 
>> +#define BCM590XX_REGMAP_IRQ_REG(id)	REGMAP_IRQ_REG_LINE(id, 8)
> 
> What does the 8 mean?

IRQ register width - 8-bit wide registers. Each IRQ gets one bit for
status in the IRQx registers, and one bit for mask in the IRQxMASK
registers. See REGMAP_IRQ_REG_LINE in include/linux/regmap.h.

> 
>> +
>> +/* BCM59054 IRQs */
> 
> We can see this by the nomenclature.

(...)

>> +/* BCM59056 IRQs */
> 
> As above.
> 

Ack, will drop these.
>> +	switch (bcm590xx->pmu_id) {
>> +	case BCM590XX_PMUID_BCM59054:
>> +		irq_chip = &bcm59054_irq_chip;
>> +		break;
>> +	case BCM590XX_PMUID_BCM59056:
>> +		irq_chip = &bcm59056_irq_chip;
>> +		break;
>> +	default:
>> +		dev_err(bcm590xx->dev,
>> +			"Unknown device type, this is a driver bug!\n");
> 
> Prevent the wrap here.
> 
> No, this is not a driver bug.
> 
> Just "Unsupported device type %d" will be fine.
Ack, I'll reword the message.

> 
>> +		return -EINVAL;
>> +	}
>> +
>> +	ret = devm_regmap_add_irq_chip(bcm590xx->dev, bcm590xx->regmap_pri,
>> +			bcm590xx->irq, IRQF_TRIGGER_FALLING, 0,
>> +			irq_chip, &bcm590xx->irq_data);
>> +	if (ret) {
>> +		dev_err(bcm590xx->dev, "Failed to reguest IRQ %d: %d\n",
> 
> "Failed to add IRQ Chip for IRQ: %d (%d)"
> 
>> +			bcm590xx->irq, ret);
>> +		return ret;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +/** Chip version parsing **/
> 
> Not needed.
> 

See my reply to "/** Interrupt handling **/" comment part.

(...)
>> diff --git a/include/linux/mfd/bcm590xx.h b/include/linux/mfd/bcm590xx.h
>> index 5a5783abd47b9a6bb6f9bb3a8cafddbd01aa7fcc..e6ea643766ab1a9d579c94605b54c53dc1d742d7 100644
>> --- a/include/linux/mfd/bcm590xx.h
>> +++ b/include/linux/mfd/bcm590xx.h
>> @@ -50,6 +50,237 @@ struct bcm590xx {
>>   	/* Chip revision, read from PMUREV reg */
>>   	u8 rev_digital;
>>   	u8 rev_analog;
>> +
>> +	/* Interrupts */
>> +	int irq;
>> +	struct regmap_irq_chip_data *irq_data;
>> +};
>> +
>> +/* Interrupt handling helper functions */
>> +
>> +static inline int
>> +bcm590xx_devm_request_irq(struct device *dev, struct bcm590xx *bcm590xx, int irq,
>> +			  irq_handler_t handler, unsigned long flags,
>> +			  const char *name, void *data)
>> +{
>> +	if (!bcm590xx->irq_data)
>> +		return -EINVAL;
>> +
>> +	return devm_request_threaded_irq(dev,
>> +				regmap_irq_get_virq(bcm590xx->irq_data, irq),
>> +				NULL, handler, flags, name, data);
>> +}
>> +
>> +static inline void
>> +bcm590xx_devm_free_irq(struct device *dev, struct bcm590xx *bcm590xx, int irq,
>> +		       void *data)
>> +{
>> +	if (!bcm590xx->irq_data)
>> +		return;
>> +
>> +	devm_free_irq(dev, regmap_irq_get_virq(bcm590xx->irq_data, irq), data);
>> +}
> 
> These functions are abstracted for the sake of abstraction.  Please remove.

OK, will do.

Best regards
Artur

Powered by blists - more mailing lists