linux-kernel - Re: [PATCH 2/2] regulator: max20339: add Maxim MAX20339 regulator 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-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZulFBQzRdOdw9cfV@finisterre.sirena.org.uk>
Date: Tue, 17 Sep 2024 10:59:49 +0200
From: Mark Brown <broonie@...nel.org>
To: Krzysztof Kozlowski <krzk@...nel.org>
Cc: André Draszik <andre.draszik@...aro.org>,
	Liam Girdwood <lgirdwood@...il.com>, Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Michael Walle <mwalle@...nel.org>,
	Peter Griffin <peter.griffin@...aro.org>,
	Tudor Ambarus <tudor.ambarus@...aro.org>,
	Will McVicker <willmcvicker@...gle.com>, kernel-team@...roid.com,
	linux-kernel@...r.kernel.org, devicetree@...r.kernel.org
Subject: Re: [PATCH 2/2] regulator: max20339: add Maxim MAX20339 regulator
 driver

On Mon, Sep 16, 2024 at 10:06:37PM +0200, Krzysztof Kozlowski wrote:
> On 16/09/2024 18:48, André Draszik wrote:

> > +	/* INSW status */
> > +	if ((status[3] & MAX20339_VINVALID)
> > +	    && !(status[0] & MAX20339_VINVALID)) {
> > +		dev_warn(dev, "Vin over- or undervoltage\n");

> Same with all these. What happens if interrupt is triggered constantly?

Logs on physical error conditions are a lot more appropriate than debug
logs, they should basically never be triggered in normal operation and 
often it's a priorty to get information out about a failure in case
someone might actually see something going wrong - especially with
regulators, the system might be about to fall over if we're failing to
regulate except in cases like SD cards.  However in the case of the
regulator API where you're telling the core about the error it's good to
defer this to the core.  We should probably be doing a better job here
and logging something in the core.

> > +	if (val & MAX20339_LSWxSHORTFAULT)
> > +		*flags |= REGULATOR_ERROR_OVER_CURRENT;
> > +
> > +	if (val & MAX20339_LSWxOVFAULT)
> > +		*flags |= REGULATOR_ERROR_OVER_VOLTAGE_WARN;
> > +
> > +	if (val & MAX20339_LSWxOCFAULT)
> > +		*flags |= REGULATOR_ERROR_OVER_CURRENT;

These should be notified to the core too, especially over voltage.

> > +	irq_flags = IRQF_ONESHOT | IRQF_SHARED;

> Why shared?

Why not?  In general if a driver can support a shared interrupt it's
polite for it to do so.

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)