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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Message-ID: <C3E444B1-4BC3-4C30-AD57-051B6B804662@cirrus.com>
Date:   Wed, 1 Nov 2023 20:47:11 +0000
From:   James Ogletree <James.Ogletree@...rus.com>
To:     Jeff LaBundy <jeff@...undy.com>
CC:     James Ogletree <james.ogletree@...nsource.cirrus.com>,
        Dmitry Torokhov <dmitry.torokhov@...il.com>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Lee Jones <lee@...nel.org>,
        Fred Treven <Fred.Treven@...rus.com>,
        Ben Bright <Ben.Bright@...rus.com>,
        "linux-input@...r.kernel.org" <linux-input@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v4 3/4] mfd: cs40l50: Add support for CS40L50 core driver

Hi Jeff,

> On Oct 24, 2023, at 9:56 PM, Jeff LaBundy <jeff@...undy.com> 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