linux-kernel - Re: [PATCH v3 7/7] regulator: bcm590xx: Add support for BCM59054 regulators

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] [day] [month] [year] [list]
Message-ID: <Z6O4F1pCqwK2774W@standask-GA-A55M-S2HP>
Date: Wed, 5 Feb 2025 20:12:23 +0100
From: Stanislav Jakubek <stano.jakubek@...il.com>
To: Artur Weber <aweber.kernel@...il.com>
Cc: Lee Jones <lee@...nel.org>, 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>,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org,
	~postmarketos/upstreaming@...ts.sr.ht
Subject: Re: [PATCH v3 7/7] regulator: bcm590xx: Add support for BCM59054
 regulators

Hi Artur,

some notes below.

On Fri, Jan 31, 2025 at 07:13:55PM +0100, Artur Weber wrote:
> The BCM59056 and BCM59054 are very similar in terms of regulators. Add
> the register definitions for the BCM59054 and enable support for it in
> the driver.
> 
> Signed-off-by: Artur Weber <aweber.kernel@...il.com>
> ---

[snip]

> +	{
> +		.type = BCM590XX_REG_TYPE_LDO,
> +		.regmap = BCM590XX_REG_REGMAP_PRI,
> +		.desc = {
> +			BCM59054_LDO_DESC(MICLDO, micldo, ldo_1_table),
> +		},
> +	},

Downstream has MICLDO fixed at 1.8V.
Not sure if that isn't just some vendor hack though :/

https://github.com/Samsung-KYLEPROXX/android_kernel_samsung_kyleproxx/blob/cm-13.0/drivers/regulator/bcmpmu-rgltr-bcm59xxx.c#L83

> +
> +	{
> +		.type = BCM590XX_REG_TYPE_LDO,
> +		.regmap = BCM590XX_REG_REGMAP_PRI,
> +		.desc = {
> +			BCM59054_LDO_DESC(USBLDO, usbldo, ldo_1_table),
> +		},
> +	},
> +
> +	{
> +		.type = BCM590XX_REG_TYPE_LDO,
> +		.regmap = BCM590XX_REG_REGMAP_PRI,
> +		.desc = {
> +			BCM59054_LDO_DESC(VIBLDO, vibldo, ldo_2_table),
> +		},
> +	},
> +
> +	{
> +		.type = BCM590XX_REG_TYPE_SR,
> +		.regmap = BCM590XX_REG_REGMAP_PRI,
> +		.desc = {
> +			BCM59054_SR_DESC(CSR, csr, dcdc_csr_ranges),
> +		},
> +	},
> +
> +	{
> +		.type = BCM590XX_REG_TYPE_SR,
> +		.regmap = BCM590XX_REG_REGMAP_PRI,
> +		.desc = {
> +			BCM59054_SR_DESC(IOSR1, iosr1, dcdc_sr_ranges),
> +		},
> +	},
> +
> +	{
> +		.type = BCM590XX_REG_TYPE_SR,
> +		.regmap = BCM590XX_REG_REGMAP_PRI,
> +		.desc = {
> +			BCM59054_SR_DESC(IOSR2, iosr2, dcdc_sr_ranges),
> +		},
> +	},
> +
> +	{
> +		.type = BCM590XX_REG_TYPE_SR,
> +		.regmap = BCM590XX_REG_REGMAP_PRI,
> +		.desc = {
> +			BCM59054_SR_DESC(MMSR, mmsr, dcdc_sr_ranges),
> +		},
> +	},
> +
> +	{
> +		.type = BCM590XX_REG_TYPE_SR,
> +		.regmap = BCM590XX_REG_REGMAP_PRI,
> +		.desc = {
> +			BCM59054_SR_DESC(SDSR1, sdsr1, dcdc_sr_ranges),
> +		},
> +	},
> +
> +	{
> +		.type = BCM590XX_REG_TYPE_SR,
> +		.regmap = BCM590XX_REG_REGMAP_PRI,
> +		.desc = {
> +			BCM59054_SR_DESC(SDSR2, sdsr2, dcdc_sr_ranges),
> +		},
> +	},
> +
> +	{
> +		.type = BCM590XX_REG_TYPE_SR,
> +		.regmap = BCM590XX_REG_REGMAP_PRI,
> +		.desc = {
> +			BCM59054_SR_DESC(VSR, vsr, dcdc_vsr_ranges),
> +		},
> +	},

VSR seems to be different between BCM59054(A0?) and BCM59054A1.
These are the A1 ranges.
Should we differentiate between the two versions, same as downstream?

https://github.com/Samsung-KYLEPROXX/android_kernel_samsung_kyleproxx/blob/cm-13.0/drivers/regulator/bcmpmu-rgltr-bcm59xxx.c#L736

> +
> +	{
> +		.type = BCM590XX_REG_TYPE_GPLDO,
> +		.regmap = BCM590XX_REG_REGMAP_SEC,
> +		.desc = {
> +			BCM59054_LDO_DESC(GPLDO1, gpldo1, ldo_1_table),
> +		},
> +	},
> +
> +	{
> +		.type = BCM590XX_REG_TYPE_GPLDO,
> +		.regmap = BCM590XX_REG_REGMAP_SEC,
> +		.desc = {
> +			BCM59054_LDO_DESC(GPLDO2, gpldo2, ldo_1_table),
> +		},
> +	},
> +
> +	{
> +		.type = BCM590XX_REG_TYPE_GPLDO,
> +		.regmap = BCM590XX_REG_REGMAP_SEC,
> +		.desc = {
> +			BCM59054_LDO_DESC(GPLDO3, gpldo3, ldo_1_table),
> +		},
> +	},
> +
> +	{
> +		.type = BCM590XX_REG_TYPE_GPLDO,
> +		.regmap = BCM590XX_REG_REGMAP_SEC,
> +		.desc = {
> +			BCM59054_LDO_DESC(TCXLDO, tcxldo, ldo_1_table),
> +		},
> +	},
> +
> +	{
> +		.type = BCM590XX_REG_TYPE_GPLDO,
> +		.regmap = BCM590XX_REG_REGMAP_SEC,
> +		.desc = {
> +			BCM59054_LDO_DESC(LVLDO1, lvldo1, ldo_1_table),

According to downstream, this should be ldo_3_table.

https://github.com/Samsung-KYLEPROXX/android_kernel_samsung_kyleproxx/blob/cm-13.0/drivers/regulator/bcmpmu-rgltr-bcm59xxx.c#L683

> +		},
> +	},
> +
> +	{
> +		.type = BCM590XX_REG_TYPE_GPLDO,
> +		.regmap = BCM590XX_REG_REGMAP_SEC,
> +		.desc = {
> +			BCM59054_LDO_DESC(LVLDO2, lvldo2, ldo_3_table),
> +		},
> +	},
> +
> +	{
> +		.type = BCM590XX_REG_TYPE_VBUS,
> +		.regmap = BCM590XX_REG_REGMAP_SEC,
> +		.desc = {
> +			BCM59054_REG_DESC(VBUS, vbus),
> +			.ops = &bcm590xx_ops_vbus,
> +			.n_voltages = 1,
> +			.fixed_uV = 5000000,
> +			.enable_reg = BCM59054_OTG_CTRL,
> +			.enable_mask = BCM590XX_VBUS_ENABLE,
> +		},
> +	},
> +};
> +
>  static int bcm590xx_probe(struct platform_device *pdev)
>  {
>  	struct bcm590xx *bcm590xx = dev_get_drvdata(pdev->dev.parent);
> @@ -487,8 +864,21 @@ static int bcm590xx_probe(struct platform_device *pdev)
>  		return -ENOMEM;
>  
>  	pmu->mfd = bcm590xx;
> -	pmu->n_regulators = BCM59056_NUM_REGS;
> -	pmu->regs = bcm59056_regs;
> +
> +	switch (pmu->mfd->device_type) {
> +	case BCM59054_TYPE:
> +		pmu->n_regulators = BCM59054_NUM_REGS;
> +		pmu->regs = bcm59054_regs;
> +		break;
> +	case BCM59056_TYPE:
> +		pmu->n_regulators = BCM59056_NUM_REGS;
> +		pmu->regs = bcm59056_regs;
> +		break;
> +	default:
> +		dev_err(bcm590xx->dev,
> +			"unknown device type, could not initialize");
> +		return -EINVAL;

Missing a '\n' in the error message.
Could use dev_err_probe as well. Maybe in other places in the driver too.

Regards,
Stanislav

> +	}
>  
>  	platform_set_drvdata(pdev, pmu);
>  
> 
> -- 
> 2.48.1
>