Re: [PATCH v6 2/3] mfd: sprd-sc27xx: Add SC2730 regulator cell

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] The preexisting 'sc2731-regulator' driver fails to bind and cannot parse Device Tree properties.
- [High] The custom SPI read function `sprd_pmic_spi_read()` is fundamentally broken, reading corrupted data and failing to transmit register addresses.
--

commit 64895ca35a5a54c5838030a81a11a1b90576b82b
Author: Otto Pflüger <otto.pflueger@abscue.de>
Subject: mfd: sprd-sc27xx: Add SC2730 regulator cell

Add an MFD cell to register the SC2730 PMIC's regulators.

> diff --git a/drivers/mfd/sprd-sc27xx-spi.c b/drivers/mfd/sprd-sc27xx-spi.c
> index aa052f6466231..214bcbef0c271 100644
> --- a/drivers/mfd/sprd-sc27xx-spi.c
> +++ b/drivers/mfd/sprd-sc27xx-spi.c
> @@ -61,6 +61,7 @@ static const struct mfd_cell sc2730_devices[] = {
>  	MFD_CELL_OF("sc2730-efuse", NULL, NULL, 0, 0, "sprd,sc2730-efuse"),
>  	MFD_CELL_OF("sc2730-eic", NULL, NULL, 0, 0, "sprd,sc2730-eic"),
>  	MFD_CELL_OF("sc2730-fgu", NULL, NULL, 0, 0, "sprd,sc2730-fgu"),
> +	MFD_CELL_NAME("sc2730-regulator"),
>  	MFD_CELL_OF("sc2730-rtc", NULL, NULL, 0, 0, "sprd,sc2730-rtc"),
>  	MFD_CELL_OF("sc2730-vibrator", NULL, NULL, 0, 0, "sprd,sc2730-vibrator"),
>  };

While reviewing this patch, I noticed a couple of pre-existing regressions in
this file:

drivers/mfd/sprd-sc27xx-spi.c:sc2731_devices() {
    ...
	MFD_CELL_OF("sc2731-fgu", NULL, NULL, 0, 0, "sprd,sc2731-fgu"),
	MFD_CELL_NAME("sc2731-poweroff"),
	MFD_CELL_NAME("sc2731-regulator"),

[Severity: High]
This is a pre-existing issue, but could the sc2731-regulator MFD cell fail to
bind to its platform driver due to a name mismatch?

The platform driver in drivers/regulator/sc2731-regulator.c is named
"sc27xx-regulator" and lacks an id_table, which means the platform bus will
not match them and the driver will remain unbound.

Furthermore, even if it did bind, sc2731_regulator_probe() sets
config.dev = &pdev->dev. Because MFD_CELL_NAME() creates the device without
an of_node, pdev->dev.of_node is NULL. Does this prevent the regulator
core from parsing any DT constraints or bindings, breaking regulator
initialization?
    ...
}

drivers/mfd/sprd-sc27xx-spi.c:sprd_pmic_spi_read() {
    ...
	/* Copy address to read from into first element of SPI buffer. */
	memcpy(rx_buf, reg, sizeof(u32));
	ret = spi_read(spi, rx_buf, 1);

[Severity: High]
This isn't a bug introduced by this patch, but is the custom SPI read
function sprd_pmic_spi_read() fundamentally broken?

It attempts to read a 4-byte PMIC register by copying the 4-byte register
address into rx_buf and then calling spi_read(spi, rx_buf, 1). The core
spi_read() API performs a receive-only transfer (tx_buf is NULL), meaning
the register address is never actually sent to the SPI slave.

Additionally, the length is set to 1 byte instead of 4, so only 1 byte is
clocked in from the slave.

	if (ret < 0)
		return ret;

	memcpy(val, rx_buf, val_size);

Finally, copying 4 bytes back to the caller returns the 1 byte read from the
bus concatenated with the remaining 3 bytes of the register address that were
initially placed into rx_buf. Does this silently return garbage data for
every register read?
    ...
}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260620-sc2730-regulators-v6-0-bbd2db395231@abscue.de?part=2