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

[Jeff LaBundy]

Hi James,

On Wed, Nov 01, 2023 at 08:47:11PM +0000, James Ogletree wrote:
> 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.

I'm very much a proponent of using regmap_irq for a device whose register
map is organized in this way; my question was mostly why to map an entire
irq line to every possible interrupt source as opposed to reserving only
one line for L50 altogether.

I noted other such examples as well, and I think either method is functionally
equivalent. But considering many of these interrupts are related to events
that no customer would reasonably care about, and the ones that customers do
care about can easily be delineated by printing, a single handler is fine in
my opinion.

> 
> Best,
> James
> 

Kind regards,
Jeff LaBundy