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

[Stanislav Jakubek]

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@xxxxxxxxx>
> ---

[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
>