| 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
| ||
|
Message-ID: <20250401125014.GD7190@google.com>
Date: Tue, 1 Apr 2025 13:50:14 +0100
From: Lee Jones <lee@...nel.org>
To: Artur Weber <aweber.kernel@...il.com>
Cc: Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley <conor+dt@...nel.org>,
Liam Girdwood <lgirdwood@...il.com>,
Mark Brown <broonie@...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>,
Stanislav Jakubek <stano.jakubek@...il.com>,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org,
~postmarketos/upstreaming@...ts.sr.ht
Subject: Re: [PATCH v7 06/10] mfd: bcm590xx: Add PMU ID/revision parsing
function
On Sun, 16 Mar 2025, Artur Weber wrote:
> The BCM590xx PMUs have two I2C registers for reading the PMU ID
> and revision. The revision is useful for subdevice drivers, since
> different revisions may have slight differences in behavior (for
> example - BCM59054 has different regulator configurations for
> revision A0 and A1).
>
> Check the PMU ID register and make sure it matches the DT compatible.
> Fetch the digital and analog revision from the PMUREV register
> so that it can be used in subdevice drivers.
>
> Also add some known revision values to bcm590xx.h, for convenience
> when writing subdevice drivers.
>
> Signed-off-by: Artur Weber <aweber.kernel@...il.com>
> ---
> Changes in v7:
> - Return -ENODEV on PMU ID mismatch
> - Drop "Check your DT compatible" from ID mismatch error message
>
> Changes in v6:
> - Adapt to PMUID being passed as device type value
> - Rename rev_dig and rev_ana to rev_digital and rev_analog
> - Rewrite commit message
>
> Changes in v5:
> - Add REG_ prefix to register offset constant names
>
> Changes in v4:
> - Added this commit
> ---
> drivers/mfd/bcm590xx.c | 63 ++++++++++++++++++++++++++++++++++++++++++++
> include/linux/mfd/bcm590xx.h | 14 ++++++++++
> 2 files changed, 77 insertions(+)
>
> diff --git a/drivers/mfd/bcm590xx.c b/drivers/mfd/bcm590xx.c
> index 4620eed0066fbf1dd691a2e392e967747b4d125b..140107263599777b30cce4cfc0f86a9278907d34 100644
> --- a/drivers/mfd/bcm590xx.c
> +++ b/drivers/mfd/bcm590xx.c
> @@ -17,6 +17,15 @@
> #include <linux/regmap.h>
> #include <linux/slab.h>
>
> +/* Under primary I2C address: */
> +#define BCM590XX_REG_PMUID 0x1e
> +
> +#define BCM590XX_REG_PMUREV 0x1f
> +#define BCM590XX_PMUREV_DIG_MASK 0xF
> +#define BCM590XX_PMUREV_DIG_SHIFT 0
> +#define BCM590XX_PMUREV_ANA_MASK 0xF0
> +#define BCM590XX_PMUREV_ANA_SHIFT 4
> +
> static const struct mfd_cell bcm590xx_devs[] = {
> {
> .name = "bcm590xx-vregs",
> @@ -37,6 +46,56 @@ static const struct regmap_config bcm590xx_regmap_config_sec = {
> .cache_type = REGCACHE_MAPLE,
> };
>
> +/* Map PMU ID value to model name string */
> +static const char * const bcm590xx_names[] = {
> + [BCM590XX_PMUID_BCM59054] = "BCM59054",
> + [BCM590XX_PMUID_BCM59056] = "BCM59056",
> +};
> +
> +/*
> + * Parse the version from version registers and make sure it matches
> + * the device type passed to the compatible.
> + */
Nit: I think that a lot of these comments are superfluous.
This is all normal stuff. No need for the extra commentary IMHO.
> +static int bcm590xx_parse_version(struct bcm590xx *bcm590xx)
> +{
> + unsigned int id, rev;
> + int ret;
> +
> + /* Get PMU ID and verify that it matches compatible */
> + ret = regmap_read(bcm590xx->regmap_pri, BCM590XX_REG_PMUID, &id);
> + if (ret) {
> + dev_err(bcm590xx->dev, "failed to read PMU ID: %d\n", ret);
> + return ret;
> + }
> +
> + if (id != bcm590xx->pmu_id) {
> + dev_err(bcm590xx->dev,
> + "Incorrect ID for %s: expected %x, got %x.\n",
> + bcm590xx_names[bcm590xx->pmu_id], bcm590xx->pmu_id, id);
> + return -ENODEV;
> + }
> +
> + /* Get PMU revision and store it in the info struct */
> + ret = regmap_read(bcm590xx->regmap_pri, BCM590XX_REG_PMUREV, &rev);
> + if (ret) {
> + dev_err(bcm590xx->dev, "failed to read PMU revision: %d\n",
> + ret);
No need to wrap for little things like this.
I'd take lines up to 100-chars if the aim is to make the code more readable.
> + return ret;
> + }
> +
> + bcm590xx->rev_digital = (rev & BCM590XX_PMUREV_DIG_MASK)
> + >> BCM590XX_PMUREV_DIG_SHIFT;
> +
> + bcm590xx->rev_analog = (rev & BCM590XX_PMUREV_ANA_MASK)
> + >> BCM590XX_PMUREV_ANA_SHIFT;
Here too, etc.
> +
> + dev_info(bcm590xx->dev, "PMU ID 0x%x (%s), revision: digital %d, analog %d",
> + id, bcm590xx_names[id],
> + bcm590xx->rev_digital, bcm590xx->rev_analog);
If this stuff actually useful to anyone?
> + return 0;
> +}
> +
> static int bcm590xx_i2c_probe(struct i2c_client *i2c_pri)
> {
> struct bcm590xx *bcm590xx;
> @@ -78,6 +137,10 @@ static int bcm590xx_i2c_probe(struct i2c_client *i2c_pri)
> goto err;
> }
>
> + ret = bcm590xx_parse_version(bcm590xx);
> + if (ret)
> + goto err;
> +
> ret = devm_mfd_add_devices(&i2c_pri->dev, -1, bcm590xx_devs,
> ARRAY_SIZE(bcm590xx_devs), NULL, 0, NULL);
> if (ret < 0) {
> diff --git a/include/linux/mfd/bcm590xx.h b/include/linux/mfd/bcm590xx.h
> index 8d146e3b102a7dbce6f4dbab9f8ae5a9c4e68c0e..fbc458e94bef923ca1b69afe2cac944adf6fedf8 100644
> --- a/include/linux/mfd/bcm590xx.h
> +++ b/include/linux/mfd/bcm590xx.h
> @@ -17,6 +17,16 @@
> #define BCM590XX_PMUID_BCM59054 0x54
> #define BCM590XX_PMUID_BCM59056 0x56
>
> +/* Known chip revision IDs */
> +#define BCM59054_REV_DIGITAL_A1 1
> +#define BCM59054_REV_ANALOG_A1 2
> +
> +#define BCM59056_REV_DIGITAL_A0 1
> +#define BCM59056_REV_ANALOG_A0 1
> +
> +#define BCM59056_REV_DIGITAL_B0 2
> +#define BCM59056_REV_ANALOG_B0 2
> +
> /* max register address */
> #define BCM590XX_MAX_REGISTER_PRI 0xe7
> #define BCM590XX_MAX_REGISTER_SEC 0xf0
> @@ -30,6 +40,10 @@ struct bcm590xx {
>
> /* PMU ID value; also used as device type */
> u8 pmu_id;
> +
> + /* Chip revision, read from PMUREV reg */
> + u8 rev_digital;
> + u8 rev_analog;
> };
>
> #endif /* __LINUX_MFD_BCM590XX_H */
>
> --
> 2.48.1
>
--
Lee Jones [李琼斯]
Powered by blists - more mailing lists