Re: [PATCH v4 3/4] mfd: cs40l50: Add support for CS40L50 core driver

[James Ogletree]

Hi Jeff,

> On Oct 24, 2023, at 9:56 PM, Jeff LaBundy <jeff@xxxxxxxxxxx> wrote:
> 
> On Wed, Oct 18, 2023 at 05:57:24PM +0000, James Ogletree wrote:
>> 
>> +static irqreturn_t cs40l50_error(int irq, void *data);
>> +
>> +static const struct cs40l50_irq cs40l50_irqs[] = {
>> +	CS40L50_IRQ(AMP_SHORT,		"Amp short",		error),
>> +	CS40L50_IRQ(VIRT2_MBOX,		"Mailbox",		process_mbox),
>> +	CS40L50_IRQ(TEMP_ERR,		"Overtemperature",	error),
>> +	CS40L50_IRQ(BST_UVP,		"Boost undervoltage",	error),
>> +	CS40L50_IRQ(BST_SHORT,		"Boost short",		error),
>> +	CS40L50_IRQ(BST_ILIMIT,		"Boost current limit",	error),
>> +	CS40L50_IRQ(UVLO_VDDBATT,	"Boost UVLO",		error),
>> +	CS40L50_IRQ(GLOBAL_ERROR,	"Global",		error),
>> +};
>> +
>> +static irqreturn_t cs40l50_error(int irq, void *data)
>> +{
>> +	struct cs40l50_private *cs40l50 = data;
>> +
>> +	dev_err(cs40l50->dev, "%s error\n", cs40l50_irqs[irq].name);
>> +
>> +	return IRQ_RETVAL(!cs40l50_error_release(cs40l50));
>> +}
>> +
>> +static const struct regmap_irq cs40l50_reg_irqs[] = {
>> +	CS40L50_REG_IRQ(IRQ1_INT_1,	AMP_SHORT),
>> +	CS40L50_REG_IRQ(IRQ1_INT_2,	VIRT2_MBOX),
>> +	CS40L50_REG_IRQ(IRQ1_INT_8,	TEMP_ERR),
>> +	CS40L50_REG_IRQ(IRQ1_INT_9,	BST_UVP),
>> +	CS40L50_REG_IRQ(IRQ1_INT_9,	BST_SHORT),
>> +	CS40L50_REG_IRQ(IRQ1_INT_9,	BST_ILIMIT),
>> +	CS40L50_REG_IRQ(IRQ1_INT_10,	UVLO_VDDBATT),
>> +	CS40L50_REG_IRQ(IRQ1_INT_18,	GLOBAL_ERROR),
>> +};
>> +
>> +static struct regmap_irq_chip cs40l50_irq_chip = {
>> +	.name =			"CS40L50 IRQ Controller",
>> +
>> +	.status_base =		CS40L50_IRQ1_INT_1,
>> +	.mask_base =		CS40L50_IRQ1_MASK_1,
>> +	.ack_base =		CS40L50_IRQ1_INT_1,
>> +	.num_regs =		22,
>> +
>> +	.irqs =			cs40l50_reg_irqs,
>> +	.num_irqs =		ARRAY_SIZE(cs40l50_reg_irqs),
>> +
>> +	.runtime_pm =		true,
>> +};
>> +
>> +static int cs40l50_irq_init(struct cs40l50_private *cs40l50)
>> +{
>> +	struct device *dev = cs40l50->dev;
>> +	int error, i, irq;
>> +
>> +	error = devm_regmap_add_irq_chip(dev, cs40l50->regmap, cs40l50->irq,
>> +					 IRQF_ONESHOT | IRQF_SHARED, 0,
>> +					 &cs40l50_irq_chip, &cs40l50->irq_data);
>> +	if (error)
>> +		return error;
>> +
>> +	for (i = 0; i < ARRAY_SIZE(cs40l50_irqs); i++) {
>> +		irq = regmap_irq_get_virq(cs40l50->irq_data, cs40l50_irqs[i].irq);
>> +		if (irq < 0) {
>> +			dev_err(dev, "Failed getting %s\n", cs40l50_irqs[i].name);
>> +			return irq;
>> +		}
>> +
>> +		error = devm_request_threaded_irq(dev, irq, NULL,
>> +						  cs40l50_irqs[i].handler,
>> +						  IRQF_ONESHOT | IRQF_SHARED,
>> +						  cs40l50_irqs[i].name, cs40l50);
>> +		if (error) {
>> +			dev_err(dev, "Failed requesting %s\n", cs40l50_irqs[i].name);
>> +			return error;
>> +		}
>> +	}
> 
> This is kind of an uncommon design pattern; if anyone reads /proc/interrupts
> on their system, it's going to be full of L50 interrupts. Normally we declare
> a single IRQ, read the status register(s) from inside its handler and then
> act accordingly.
> 
> What is the motivation for having one handler per interrupt status bit? If
> multiple bits are set at once, does the register get read multiple times and
> if so, does doing so clear any pending status? Or are the status registers
> write-to-clear instead of read-to-clear?

The reason I used the regmap_irq framework is that it takes care of
the reading and clearing of the status register, and yes it handles the
situation of multiple bits getting set at once. I think I will merge the IRQ
handlers into one for the next version. The fact of /proc/interrupts filling
up with these interrupts is not great and was something I overlooked,
though I think I see instances of drivers with similar amount of interrupts
upstream.

Best,
James