Re: [PATCH 2/2] mfd: sprd-sc27xx-spi: Add PMICs support for UMS9621 SoC

[Krzysztof Kozlowski]

On 22/08/2023 09:51, Jiansheng Wu wrote:
> There are three PMICs (UMP9620/21/22) on Unisoc's UMS9621 chip.
> UMP9620 is a master PMIC, the others are slave ones. Slave PMICs
> don't have irq functions, which is different from master device,
> such as SC27xx series and UMP9620, etc.
> 
> Signed-off-by: Jiansheng Wu <jiansheng.wu@xxxxxxxxxx>
> ---
>  drivers/mfd/sprd-sc27xx-spi.c | 104 +++++++++++++++++++++++++---------
>  1 file changed, 77 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/mfd/sprd-sc27xx-spi.c b/drivers/mfd/sprd-sc27xx-spi.c
> index d21f32cc784d..aa91301568a9 100644
> --- a/drivers/mfd/sprd-sc27xx-spi.c
> +++ b/drivers/mfd/sprd-sc27xx-spi.c
> @@ -24,6 +24,10 @@
>  #define SPRD_SC2731_IRQ_BASE		0x140
>  #define SPRD_SC2731_IRQ_NUMS		16
>  #define SPRD_SC2731_CHG_DET		0xedc
> +#define SPRD_UMP9620_IRQ_BASE           0x80
> +#define SPRD_UMP9620_IRQ_NUMS           11
> +#define SPRD_UMP9621_SLAVE_ID           0x8000
> +#define SPRD_UMP9622_SLAVE_ID           0xc000
>  
>  /* PMIC charger detection definition */
>  #define SPRD_PMIC_CHG_DET_DELAY_US	200000
> @@ -45,6 +49,7 @@ struct sprd_pmic {
>  };
>  
>  struct sprd_pmic_data {
> +	u32 slave_id;

See coding style about such wording. You know, it is not 2010 anymore...

>  	u32 irq_base;
>  	u32 num_irqs;
>  	u32 charger_det;
> @@ -67,6 +72,19 @@ static const struct sprd_pmic_data sc2731_data = {
>  	.charger_det = SPRD_SC2731_CHG_DET,
>  };
>  
> +static const struct sprd_pmic_data ump9620_data = {
> +	.irq_base = SPRD_UMP9620_IRQ_BASE,
> +	.num_irqs = SPRD_UMP9620_IRQ_NUMS,
> +};
> +
> +static const struct sprd_pmic_data ump9621_data = {
> +	.slave_id = SPRD_UMP9621_SLAVE_ID,
> +};
> +
> +static const struct sprd_pmic_data ump9622_data = {
> +	.slave_id = SPRD_UMP9622_SLAVE_ID,
> +};
> +
>  enum usb_charger_type sprd_pmic_detect_charger_type(struct device *dev)
>  {
>  	struct spi_device *spi = to_spi_device(dev);
> @@ -108,8 +126,27 @@ static int sprd_pmic_spi_write(void *context, const void *data, size_t count)
>  {
>  	struct device *dev = context;
>  	struct spi_device *spi = to_spi_device(dev);
> +	const struct sprd_pmic_data *pdata;
> +	int ret;
> +	u32 *pmdata;
> +
> +	if (!pdata->slave_id) {
> +		ret = spi_write(spi, data, count);
> +	} else {
> +		pdata = ((struct sprd_pmic *)spi_get_drvdata(spi))->pdata;
> +
> +		pmdata = kzalloc(count, GFP_KERNEL);
> +		if (!pmdata)
> +			return -ENOMEM;
> +		memcpy(pmdata, data, count);
> +		*pmdata += pdata->slave_id;
> +		ret = spi_write(spi, (const void *)pmdata, count);
> +		kfree(pmdata);
> +	}
> +	if (ret)
> +		pr_err("pmic mfd write failed!\n");
>  
> -	return spi_write(spi, data, count);
> +	return ret;
>  }
>  
>  static int sprd_pmic_spi_read(void *context,
> @@ -118,6 +155,7 @@ static int sprd_pmic_spi_read(void *context,
>  {
>  	struct device *dev = context;
>  	struct spi_device *spi = to_spi_device(dev);
> +	const struct sprd_pmic_data *pdata;
>  	u32 rx_buf[2] = { 0 };
>  	int ret;
>  
> @@ -125,11 +163,16 @@ static int sprd_pmic_spi_read(void *context,
>  	if (reg_size != sizeof(u32) || val_size != sizeof(u32))
>  		return -EINVAL;
>  
> +	pdata = ((struct sprd_pmic *)spi_get_drvdata(spi))->pdata;
>  	/* Copy address to read from into first element of SPI buffer. */
>  	memcpy(rx_buf, reg, sizeof(u32));
> +	if (!pdata->slave_id)
> +		rx_buf[0] += pdata->slave_id;
>  	ret = spi_read(spi, rx_buf, 1);
> -	if (ret < 0)
> +	if (ret < 0) {
> +		pr_err("pmic mfd read failed!\n");

Eh... drivers do not use pr_ but dev_. Which you can easily see within
this driver, so before posting changes please look at the driver and its
style, then learn from it and use similar coding convention.

>  		return ret;
> +	}
>  
>  	memcpy(val, rx_buf, val_size);
>  	return 0;

...


>  
>  	ret = devm_of_platform_populate(&spi->dev);
> @@ -240,6 +284,9 @@ static DEFINE_SIMPLE_DEV_PM_OPS(sprd_pmic_pm_ops,
>  static const struct of_device_id sprd_pmic_match[] = {
>  	{ .compatible = "sprd,sc2730", .data = &sc2730_data },
>  	{ .compatible = "sprd,sc2731", .data = &sc2731_data },
> +	{ .compatible = "sprd,ump9620", .data = &ump9620_data },
> +	{ .compatible = "sprd,ump9621", .data = &ump9621_data },
> +	{ .compatible = "sprd,ump9622", .data = &ump9622_data },
>  	{},
>  };
>  MODULE_DEVICE_TABLE(of, sprd_pmic_match);
> @@ -247,6 +294,9 @@ MODULE_DEVICE_TABLE(of, sprd_pmic_match);
>  static const struct spi_device_id sprd_pmic_spi_ids[] = {
>  	{ .name = "sc2730", .driver_data = (unsigned long)&sc2730_data },
>  	{ .name = "sc2731", .driver_data = (unsigned long)&sc2731_data },
> +	{ .name = "ump9620", .driver_data = (unsigned long)&ump9620_data },
> +	{ .name = "ump9621", .driver_data = (unsigned long)&ump9621_data },
> +	{ .name = "ump9622", .driver_data = (unsigned long)&ump9622_data },

So here you sneaked new compatibles... Sorry, adding new compatibles is
not the same as converting old ones. Entirely separate patch.

Best regards,
Krzysztof