linux-kernel - Re: [PATCH v2 5/6] regulator: bcm590xx: Add support for BCM59054

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: <53aae3a6-9e6f-4156-b807-082d11d858ed@sirena.org.uk>
Date:   Tue, 31 Oct 2023 12:41:43 +0000
From:   Mark Brown <broonie@...nel.org>
To:     Artur Weber <aweber.kernel@...il.com>
Cc:     Lee Jones <lee@...nel.org>, Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Conor Dooley <conor+dt@...nel.org>,
        Florian Fainelli <florian.fainelli@...adcom.com>,
        Ray Jui <rjui@...adcom.com>,
        Scott Branden <sbranden@...adcom.com>,
        Broadcom internal kernel review list 
        <bcm-kernel-feedback-list@...adcom.com>,
        Liam Girdwood <lgirdwood@...il.com>,
        Stanislav Jakubek <stano.jakubek@...il.com>,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v2 5/6] regulator: bcm590xx: Add support for BCM59054

On Mon, Oct 30, 2023 at 08:41:47PM +0100, Artur Weber wrote:
> The BCM59054 is fairly similar in terms of regulators to the already
> supported BCM59056, as included in the BCM590XX driver.

> Add support for the BCM59054's regulators to the BCM590XX driver.
> Switch from using defines for common checks to using functions which
> return different values depending on the identified MFD model.

> While we're at it, fix a bug where the enable/vsel register
> offsets for GPLDO and LDO regulators were calculated incorrectly.

> Also, change the regulator enable bitmask to cover the entire PMMODE
> register.

As is very clear from the commit log this is a whole bunch of changes
which really should be split out into multiple patches.  There's the
bug fix, there's the multiple refactorings to support the new device and
the new device itself.  As covered in submitting-patches.rst you should
do one change per patch, this makes things much easier to follow.

> +	if (pmu->mfd->device_type == BCM59054_TYPE) {
> +		info = bcm59054_regs;
> +		n_regulators = BCM59054_NUM_REGS;
> +	} else if (pmu->mfd->device_type == BCM59056_TYPE) {
> +		info = bcm59056_regs;
> +		n_regulators = BCM59056_NUM_REGS;
> +	}

There's no error handling here if there's an unknown type.

> -		if ((BCM590XX_REG_IS_LDO(i)) || (BCM590XX_REG_IS_GPLDO(i))) {
> +		if (bcm590xx_reg_is_ldo(pmu, i) || \
> +				bcm590xx_reg_is_gpldo(pmu, i)) {
>  			pmu->desc[i].ops = &bcm590xx_ops_ldo;
>  			pmu->desc[i].vsel_mask = BCM590XX_LDO_VSEL_MASK;
> -		} else if (BCM590XX_REG_IS_VBUS(i))
> -			pmu->desc[i].ops = &bcm590xx_ops_vbus;
> -		else {
> +		} else if (bcm590xx_reg_is_static(pmu, i)) {
> +			pmu->desc[i].ops = &bcm590xx_ops_static;
> +		} else {
>  			pmu->desc[i].ops = &bcm590xx_ops_dcdc;
>  			pmu->desc[i].vsel_mask = BCM590XX_SR_VSEL_MASK;
>  		}

It seems like everything would be a lot simpler and clearer to just have
tables of regulators per device rather than have all these conditionals.

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