[2/2] mfd: add new driver for P1 PMIC from SpacemiT

Commit Message

Troy Mitchell Dec. 30, 2024, 10:02 a.m. UTC

Add the core MFD driver for P1 PMIC. I define four sub-devices
for which the drivers will be added in subsequent patches.

For this patch, It supports `reboot` and `shutdown`.

Signed-off-by: Troy Mitchell <[e-mail]>
---
 drivers/mfd/Kconfig                        |  14 +
 drivers/mfd/Makefile                       |   1 +
 drivers/mfd/spacemit-pmic.c                | 159 ++++++++++
 include/linux/mfd/spacemit/spacemit-p1.h   | 491 +++++++++++++++++++++++++++++
 include/linux/mfd/spacemit/spacemit-pmic.h |  39 +++
 5 files changed, 704 insertions(+)

Comments

Troy Mitchell Dec. 30, 2024, 10:15 a.m. UTC | #1

On 2024/12/30 18:02, Troy Mitchell wrote:
> Add the core MFD driver for P1 PMIC. I define four sub-devices
> for which the drivers will be added in subsequent patches.
> 
> For this patch, It supports `reboot` and `shutdown`.
> 
> Signed-off-by: Troy Mitchell <[e-mail]>
> ---
>  drivers/mfd/Kconfig                        |  14 +
>  drivers/mfd/Makefile                       |   1 +
>  drivers/mfd/spacemit-pmic.c                | 159 ++++++++++
>  include/linux/mfd/spacemit/spacemit-p1.h   | 491 +++++++++++++++++++++++++++++
>  include/linux/mfd/spacemit/spacemit-pmic.h |  39 +++
>  5 files changed, 704 insertions(+)
> 
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index ae23b317a64e49f0cb529ae6bd1becbb90b7c282..c062bf6b11fd23d420a6d5f6ee51b3ec97f9fcbb 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -1173,6 +1173,20 @@ config MFD_QCOM_RPM
>  	  Say M here if you want to include support for the Qualcomm RPM as a
>  	  module. This will build a module called "qcom_rpm".
>  
> +config MFD_SPACEMIT_PMIC
> +	tristate "SpacemiT PMIC"
> +	depends on ARCH_SPACEMIT || COMPILE_TEST
> +	depends on I2C && OF
> +	select MFD_CORE
> +	select REGMAP_I2C
> +	select REGMAP_IRQ
> +	help
> +	  If this option is turned on, the P1 chip produced by SpacemiT will
> +	  be supported.
> +
> +	  This driver can also be compiled as a module. If you choose to build
> +	  it as a module, the resulting kernel module will be named `spacemit-pmic`.
> +
>  config MFD_SPMI_PMIC
>  	tristate "Qualcomm SPMI PMICs"
>  	depends on ARCH_QCOM || COMPILE_TEST
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index e057d6d6faef5c1d639789e2560f336fa26cd872..284dbb8fe2ef83bdd994a598504fe315f2eabbdf 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -266,6 +266,7 @@ obj-$(CONFIG_MFD_SUN4I_GPADC)	+= sun4i-gpadc.o
>  obj-$(CONFIG_MFD_STM32_LPTIMER)	+= stm32-lptimer.o
>  obj-$(CONFIG_MFD_STM32_TIMERS) 	+= stm32-timers.o
>  obj-$(CONFIG_MFD_MXS_LRADC)     += mxs-lradc.o
> +obj-$(CONFIG_MFD_SPACEMIT_PMIC)	+= spacemit-pmic.o
>  obj-$(CONFIG_MFD_SC27XX_PMIC)	+= sprd-sc27xx-spi.o
>  obj-$(CONFIG_RAVE_SP_CORE)	+= rave-sp.o
>  obj-$(CONFIG_MFD_ROHM_BD71828)	+= rohm-bd71828.o
> diff --git a/drivers/mfd/spacemit-pmic.c b/drivers/mfd/spacemit-pmic.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..d9f6785cecbd405821dead13cdf8d1f9fd64e508
> --- /dev/null
> +++ b/drivers/mfd/spacemit-pmic.c
> @@ -0,0 +1,159 @@
> +static const struct of_device_id spacemit_pmic_of_match[] = {
> +	{ .compatible = "spacemit,p1", .data = &pmic_p1_match_data },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, spacemit_pmic_of_match);
> +
> +static struct i2c_driver spacemit_pmic_i2c_driver = {
> +	.driver = {
> +		.name = "spacemit-pmic",
> +		.of_match_table = spacemit_pmic_of_match,
> +	},
> +	.probe    = spacemit_pmic_probe,
> +};
> +
> +static int __init spacemit_pmic_init(void)
> +{
> +	return platform_driver_register(&spacemit_pmic_i2c_driver);
> +}
> +
> +static void __exit spacemit_pmic_exit(void)
> +{
> +	platform_driver_unregister(&spacemit_pmic_i2c_driver);
> +}
I should use i2c_add_driver/i2c_del_driver here.
I forgot to add my modified c file via stg :(
> +
> +module_init(spacemit_pmic_init);
> +module_exit(spacemit_pmic_exit);
>

Krzysztof Kozlowski Dec. 30, 2024, 11:33 a.m. UTC | #2

On 30/12/2024 11:02, Troy Mitchell wrote:
> Add the core MFD driver for P1 PMIC. I define four sub-devices


I do not see any definition of MFD subdevices.

> for which the drivers will be added in subsequent patches.
> 
> For this patch, It supports `reboot` and `shutdown`.
> 
> Signed-off-by: Troy Mitchell <[e-mail]>
> ---
>  drivers/mfd/Kconfig                        |  14 +
>  drivers/mfd/Makefile                       |   1 +
>  drivers/mfd/spacemit-pmic.c                | 159 ++++++++++
>  include/linux/mfd/spacemit/spacemit-p1.h   | 491 +++++++++++++++++++++++++++++
>  include/linux/mfd/spacemit/spacemit-pmic.h |  39 +++
>  5 files changed, 704 insertions(+)
> 
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index ae23b317a64e49f0cb529ae6bd1becbb90b7c282..c062bf6b11fd23d420a6d5f6ee51b3ec97f9fcbb 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -1173,6 +1173,20 @@ config MFD_QCOM_RPM
>  	  Say M here if you want to include support for the Qualcomm RPM as a
>  	  module. This will build a module called "qcom_rpm".
>  
> +config MFD_SPACEMIT_PMIC
> +	tristate "SpacemiT PMIC"
> +	depends on ARCH_SPACEMIT || COMPILE_TEST
> +	depends on I2C && OF
> +	select MFD_CORE
> +	select REGMAP_I2C
> +	select REGMAP_IRQ
> +	help
> +	  If this option is turned on, the P1 chip produced by SpacemiT will
> +	  be supported.
> +
> +	  This driver can also be compiled as a module. If you choose to build
> +	  it as a module, the resulting kernel module will be named `spacemit-pmic`.
> +
>  config MFD_SPMI_PMIC
>  	tristate "Qualcomm SPMI PMICs"
>  	depends on ARCH_QCOM || COMPILE_TEST
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index e057d6d6faef5c1d639789e2560f336fa26cd872..284dbb8fe2ef83bdd994a598504fe315f2eabbdf 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -266,6 +266,7 @@ obj-$(CONFIG_MFD_SUN4I_GPADC)	+= sun4i-gpadc.o
>  obj-$(CONFIG_MFD_STM32_LPTIMER)	+= stm32-lptimer.o
>  obj-$(CONFIG_MFD_STM32_TIMERS) 	+= stm32-timers.o
>  obj-$(CONFIG_MFD_MXS_LRADC)     += mxs-lradc.o
> +obj-$(CONFIG_MFD_SPACEMIT_PMIC)	+= spacemit-pmic.o
>  obj-$(CONFIG_MFD_SC27XX_PMIC)	+= sprd-sc27xx-spi.o
>  obj-$(CONFIG_RAVE_SP_CORE)	+= rave-sp.o
>  obj-$(CONFIG_MFD_ROHM_BD71828)	+= rohm-bd71828.o
> diff --git a/drivers/mfd/spacemit-pmic.c b/drivers/mfd/spacemit-pmic.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..d9f6785cecbd405821dead13cdf8d1f9fd64e508
> --- /dev/null
> +++ b/drivers/mfd/spacemit-pmic.c
> @@ -0,0 +1,159 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) 2024 Troy Mitchell <[e-mail]>
> + */
> +
> +#include <linux/i2c.h>
> +#include <linux/mfd/core.h>
> +#include <linux/mfd/spacemit/spacemit-pmic.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>

I don't see usage of this header. Include what is used directly.

> +#include <linux/pm_wakeirq.h>
> +#include <linux/reboot.h>
> +
> +P1_REGMAP_CONFIG;
> +P1_IRQS_DESC;
> +P1_IRQ_CHIP_DESC;
> +P1_POWER_KEY_RESOURCES_DESC;
> +P1_RTC_RESOURCES_DESC;
> +P1_MFD_CELL;
> +P1_MFD_MATCH_DATA;

Hm? Declarations and definitions go here, not to somewhere else.



> +
> +static int spacemit_pmic_probe(struct i2c_client *client)
> +{
> +	const struct spacemit_pmic_match_data *match_data;
> +	const struct mfd_cell *cells;
> +	struct spacemit_pmic *pmic;
> +	int nr_cells, ret;
> +
> +	if (!client->irq)
> +		return dev_err_probe(&client->dev, -ENXIO, "no interrupt supported");

And why is this fatal error? If interrupt is not supported by hardware,
why would you add "unsupported" interrupt?

> +
> +	match_data = of_device_get_match_data(&client->dev);
> +	if (WARN_ON(!match_data))
> +		return -EINVAL;
> +
> +	pmic = devm_kzalloc(&client->dev, sizeof(*pmic), GFP_KERNEL);
> +	if (!pmic)
> +		return -ENOMEM;
> +
> +	cells = match_data->mfd_cells;
> +	nr_cells = match_data->nr_cells;
> +
> +	pmic->regmap_cfg = match_data->regmap_cfg;
> +	pmic->regmap_irq_chip = match_data->regmap_irq_chip;
> +	pmic->i2c = client;
> +	pmic->match_data = match_data;
> +	pmic->regmap = devm_regmap_init_i2c(client, pmic->regmap_cfg);
> +	if (IS_ERR(pmic->regmap))
> +		return dev_err_probe(&client->dev,
> +				     PTR_ERR(pmic->regmap),
> +				     "regmap initialization failed");
> +
> +	regcache_cache_bypass(pmic->regmap, true);
> +
> +	i2c_set_clientdata(client, pmic);
> +
> +	if (pmic->regmap_irq_chip) {


It's impossible to have it false. Test your driver.

> +		ret = regmap_add_irq_chip(pmic->regmap, client->irq, IRQF_ONESHOT, -1,
> +						pmic->regmap_irq_chip, &pmic->irq_data);
> +		if (ret)
> +			return dev_err_probe(&client->dev, ret, "failed to add irqchip");
> +	}
> +
> +	dev_pm_set_wake_irq(&client->dev, client->irq);
> +	device_init_wakeup(&client->dev, true);
> +
> +	ret = devm_mfd_add_devices(&client->dev, PLATFORM_DEVID_NONE,
> +				   cells, nr_cells, NULL, 0,
> +				   regmap_irq_get_domain(pmic->irq_data));
> +	if (ret)
> +		return dev_err_probe(&client->dev, ret, "failed to add MFD devices");
> +
> +	if (match_data->shutdown.reg) {

Also not possible, useless if.

> +		ret = devm_register_sys_off_handler(&client->dev,
> +						    SYS_OFF_MODE_POWER_OFF_PREPARE,
> +						    SYS_OFF_PRIO_HIGH,
> +						    &spacemit_pmic_shutdown,
> +						    pmic);
> +		if (ret)
> +			return dev_err_probe(&client->dev,
> +					     ret,
> +					     "failed to register restart handler");
> +
> +	}
> +
> +	if (match_data->reboot.reg) {

Also not possible.

> +		ret = devm_register_sys_off_handler(&client->dev,
> +						    SYS_OFF_MODE_RESTART,
> +						    SYS_OFF_PRIO_HIGH,
> +						    &spacemit_pmic_restart,
> +						    pmic);
> +		if (ret)
> +			return dev_err_probe(&client->dev,
> +					     ret,
> +					     "failed to register restart handler");
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id spacemit_pmic_of_match[] = {
> +	{ .compatible = "spacemit,p1", .data = &pmic_p1_match_data },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, spacemit_pmic_of_match);
> +
> +static struct i2c_driver spacemit_pmic_i2c_driver = {
> +	.driver = {
> +		.name = "spacemit-pmic",
> +		.of_match_table = spacemit_pmic_of_match,
> +	},
> +	.probe    = spacemit_pmic_probe,
> +};
> +
> +static int __init spacemit_pmic_init(void)
> +{
> +	return platform_driver_register(&spacemit_pmic_i2c_driver);
> +}
> +
> +static void __exit spacemit_pmic_exit(void)
> +{
> +	platform_driver_unregister(&spacemit_pmic_i2c_driver);
> +}
> +
> +module_init(spacemit_pmic_init);
> +module_exit(spacemit_pmic_exit);

Use proper wrapper for these above.

> +
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("mfd core driver for the SpacemiT PMIC");

...

> +
> +#define P1_MAX_REG			0xA8
> +
> +#define P1_BUCK_VSEL_MASK		0xff
> +#define P1_BUCK_EN_MASK			0x1
> +
> +#define P1_BUCK1_CTRL_REG		0x47
> +#define P1_BUCK2_CTRL_REG		0x4a


Either lowercase or uppercase hex, not both.

> +#define P1_BUCK3_CTRL_REG		0x4d
> +#define P1_BUCK4_CTRL_REG		0x50
> +#define P1_BUCK5_CTRL_REG		0x53
> +#define P1_BUCK6_CTRL_REG		0x56
> +
> +#define P1_BUCK1_VSEL_REG		0x48
> +#define P1_BUCK2_VSEL_REG		0x4b
> +#define P1_BUCK3_VSEL_REG		0x4e
> +#define P1_BUCK4_VSEL_REG		0x51
> +#define P1_BUCK5_VSEL_REG		0x54
> +#define P1_BUCK6_VSEL_REG		0x57
> +
> +#define P1_ALDO1_CTRL_REG		0x5b
> +#define P1_ALDO2_CTRL_REG		0x5e
> +#define P1_ALDO3_CTRL_REG		0x61
> +#define P1_ALDO4_CTRL_REG		0x64
> +
> +#define P1_ALDO1_VOLT_REG		0x5c
> +#define P1_ALDO2_VOLT_REG		0x5f
> +#define P1_ALDO3_VOLT_REG		0x62
> +#define P1_ALDO4_VOLT_REG		0x65
> +
> +#define P1_ALDO_EN_MASK			0x1
> +#define P1_ALDO_VSEL_MASK		0x7f
> +
> +#define P1_DLDO1_CTRL_REG		0x67
> +#define P1_DLDO2_CTRL_REG		0x6a
> +#define P1_DLDO3_CTRL_REG		0x6d
> +#define P1_DLDO4_CTRL_REG		0x70
> +#define P1_DLDO5_CTRL_REG		0x73
> +#define P1_DLDO6_CTRL_REG		0x76
> +#define P1_DLDO7_CTRL_REG		0x79
> +
> +#define P1_DLDO1_VOLT_REG		0x68
> +#define P1_DLDO2_VOLT_REG		0x6b
> +#define P1_DLDO3_VOLT_REG		0x6e
> +#define P1_DLDO4_VOLT_REG		0x71
> +#define P1_DLDO5_VOLT_REG		0x74
> +#define P1_DLDO6_VOLT_REG		0x77
> +#define P1_DLDO7_VOLT_REG		0x7a
> +
> +#define P1_DLDO_EN_MASK			0x1
> +#define P1_DLDO_VSEL_MASK		0x7f
> +
> +#define P1_SWITCH_CTRL_REG		0x59
> +#define P1_SWTICH_EN_MASK		0x1
> +
> +#define P1_PWR_CTRL2			0x7e
> +#define P1_SW_SHUTDOWN_BIT_MSK		0x4
> +#define P1_SW_RESET_BIT_MSK		0x2
> +
> +#define P1_E_GPI0_MSK			BIT(0)
> +#define P1_E_GPI1_MSK			BIT(1)
> +#define P1_E_GPI2_MSK			BIT(2)
> +#define P1_E_GPI3_MSK			BIT(3)
> +#define P1_E_GPI4_MSK			BIT(4)
> +#define P1_E_GPI5_MSK			BIT(5)
> +
> +#define P1_E_ADC_TEMP_MSK		BIT(0)
> +#define P1_E_ADC_EOC_MSK		BIT(1)
> +#define P1_E_ADC_EOS_MSK		BIT(2)
> +#define P1_E_WDT_TO_MSK			BIT(3)
> +#define P1_E_ALARM_MSK			BIT(4)
> +#define P1_E_TICK_MSK			BIT(5)
> +
> +#define P1_E_LDO_OV_MSK			BIT(0)
> +#define P1_E_LDO_UV_MSK			BIT(1)
> +#define P1_E_LDO_SC_MSK			BIT(2)
> +#define P1_E_SW_SC_MSK			BIT(3)
> +#define P1_E_TEMP_WARN_MSK		BIT(4)
> +#define P1_E_TEMP_SEVERE_MSK		BIT(5)
> +#define P1_E_TEMP_CRIT_MSK		BIT(6)
> +
> +#define P1_E_BUCK1_OV_MSK		BIT(0)
> +#define P1_E_BUCK2_OV_MSK		BIT(1)
> +#define P1_E_BUCK3_OV_MSK		BIT(2)
> +#define P1_E_BUCK4_OV_MSK		BIT(3)
> +#define P1_E_BUCK5_OV_MSK		BIT(4)
> +#define P1_E_BUCK6_OV_MSK		BIT(5)
> +
> +#define P1_E_BUCK1_UV_MSK		BIT(0)
> +#define P1_E_BUCK2_UV_MSK		BIT(1)
> +#define P1_E_BUCK3_UV_MSK		BIT(2)
> +#define P1_E_BUCK4_UV_MSK		BIT(3)
> +#define P1_E_BUCK5_UV_MSK		BIT(4)
> +#define P1_E_BUCK6_UV_MSK		BIT(5)
> +
> +#define P1_E_BUCK1_SC_MSK		BIT(0)
> +#define P1_E_BUCK2_SC_MSK		BIT(1)
> +#define P1_E_BUCK3_SC_MSK		BIT(2)
> +#define P1_E_BUCK4_SC_MSK		BIT(3)
> +#define P1_E_BUCK5_SC_MSK		BIT(4)
> +#define P1_E_BUCK6_SC_MSK		BIT(5)
> +
> +#define P1_E_PWRON_RINTR_MSK		BIT(0)
> +#define P1_E_PWRON_FINTR_MSK		BIT(1)
> +#define P1_E_PWRON_SINTR_MSK		BIT(2)
> +#define P1_E_PWRON_LINTR_MSK		BIT(3)
> +#define P1_E_PWRON_SDINTR_MSK		BIT(4)
> +#define P1_E_VSYS_OV_MSK		BIT(5)
> +
> +#define P1_E_STATUS_REG_BASE		0x91
> +#define P1_E_EN_REG_BASE		0x98
> +
> +#define P1_REGMAP_CONFIG	\
> +	static const struct regmap_config p1_regmap_config = {	\
> +		.reg_bits = 8,					\
> +		.val_bits = 8,					\
> +		.max_register = P1_MAX_REG,			\
> +		.cache_type = REGCACHE_RBTREE,			\
> +	}
> +
> +#define P1_IRQS_DESC					\
> +static const struct regmap_irq p1_irqs[] = {		\


No, all these defines are just not needed, not readable. Please follow
existing kernel style - just look at recent drivers in drivers/mfd/ to
see how they are designed and developed.

> +	[P1_E_GPI0] = {					\
> +		.mask = P1_E_GPI0_MSK,			\
> +		.reg_offset = 0,			\
> +	},						\
> +							\
> +	[P1_E_GPI1] = {					\
> +		.mask = P1_E_GPI1_MSK,			\

Best regards,
Krzysztof

Troy Mitchell Dec. 31, 2024, 7:05 a.m. UTC | #3

Hi, Krzysztof.
Thanks for ur review!

On 2024/12/30 19:33, Krzysztof Kozlowski wrote:
> On 30/12/2024 11:02, Troy Mitchell wrote:
>> Add the core MFD driver for P1 PMIC. I define four sub-devices
> 
> 
> I do not see any definition of MFD subdevices.
I define them in spacemit-p1.h.
the macro is named `P1_MFD_CELL`
> 
>> for which the drivers will be added in subsequent patches.
>>
>> For this patch, It supports `reboot` and `shutdown`.
>>
>> Signed-off-by: Troy Mitchell <[e-mail]>
>> ---
>>  drivers/mfd/Kconfig                        |  14 +
>>  drivers/mfd/Makefile                       |   1 +
>>  drivers/mfd/spacemit-pmic.c                | 159 ++++++++++
>>  include/linux/mfd/spacemit/spacemit-p1.h   | 491 +++++++++++++++++++++++++++++
>>  include/linux/mfd/spacemit/spacemit-pmic.h |  39 +++
>>  5 files changed, 704 insertions(+)
>>
>> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
>> index ae23b317a64e49f0cb529ae6bd1becbb90b7c282..c062bf6b11fd23d420a6d5f6ee51b3ec97f9fcbb 100644
>> --- a/drivers/mfd/Kconfig
>> +++ b/drivers/mfd/Kconfig
>> @@ -1173,6 +1173,20 @@ config MFD_QCOM_RPM
>>  	  Say M here if you want to include support for the Qualcomm RPM as a
>>  	  module. This will build a module called "qcom_rpm".
>>  
>> +config MFD_SPACEMIT_PMIC
>> +	tristate "SpacemiT PMIC"
>> +	depends on ARCH_SPACEMIT || COMPILE_TEST
>> +	depends on I2C && OF
>> +	select MFD_CORE
>> +	select REGMAP_I2C
>> +	select REGMAP_IRQ
>> +	help
>> +	  If this option is turned on, the P1 chip produced by SpacemiT will
>> +	  be supported.
>> +
>> +	  This driver can also be compiled as a module. If you choose to build
>> +	  it as a module, the resulting kernel module will be named `spacemit-pmic`.
>> +
>>  config MFD_SPMI_PMIC
>>  	tristate "Qualcomm SPMI PMICs"
>>  	depends on ARCH_QCOM || COMPILE_TEST
>> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
>> index e057d6d6faef5c1d639789e2560f336fa26cd872..284dbb8fe2ef83bdd994a598504fe315f2eabbdf 100644
>> --- a/drivers/mfd/Makefile
>> +++ b/drivers/mfd/Makefile
>> @@ -266,6 +266,7 @@ obj-$(CONFIG_MFD_SUN4I_GPADC)	+= sun4i-gpadc.o
>>  obj-$(CONFIG_MFD_STM32_LPTIMER)	+= stm32-lptimer.o
>>  obj-$(CONFIG_MFD_STM32_TIMERS) 	+= stm32-timers.o
>>  obj-$(CONFIG_MFD_MXS_LRADC)     += mxs-lradc.o
>> +obj-$(CONFIG_MFD_SPACEMIT_PMIC)	+= spacemit-pmic.o
>>  obj-$(CONFIG_MFD_SC27XX_PMIC)	+= sprd-sc27xx-spi.o
>>  obj-$(CONFIG_RAVE_SP_CORE)	+= rave-sp.o
>>  obj-$(CONFIG_MFD_ROHM_BD71828)	+= rohm-bd71828.o
>> diff --git a/drivers/mfd/spacemit-pmic.c b/drivers/mfd/spacemit-pmic.c
>> new file mode 100644
>> index 0000000000000000000000000000000000000000..d9f6785cecbd405821dead13cdf8d1f9fd64e508
>> --- /dev/null
>> +++ b/drivers/mfd/spacemit-pmic.c
>> @@ -0,0 +1,159 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright (C) 2024 Troy Mitchell <[e-mail]>
>> + */
>> +
>> +#include <linux/i2c.h>
>> +#include <linux/mfd/core.h>
>> +#include <linux/mfd/spacemit/spacemit-pmic.h>
>> +#include <linux/module.h>
>> +#include <linux/of_device.h>
> 
> I don't see usage of this header. Include what is used directly.
> 
>> +#include <linux/pm_wakeirq.h>
>> +#include <linux/reboot.h>
>> +
>> +P1_REGMAP_CONFIG;
>> +P1_IRQS_DESC;
>> +P1_IRQ_CHIP_DESC;
>> +P1_POWER_KEY_RESOURCES_DESC;
>> +P1_RTC_RESOURCES_DESC;
>> +P1_MFD_CELL;
>> +P1_MFD_MATCH_DATA;
> 
> Hm? Declarations and definitions go here, not to somewhere else.
I will write them here directly in next version
> 
> 
> 
>> +
>> +static int spacemit_pmic_probe(struct i2c_client *client)
>> +{
>> +	const struct spacemit_pmic_match_data *match_data;
>> +	const struct mfd_cell *cells;
>> +	struct spacemit_pmic *pmic;
>> +	int nr_cells, ret;
>> +
>> +	if (!client->irq)
>> +		return dev_err_probe(&client->dev, -ENXIO, "no interrupt supported");
> 
> And why is this fatal error? If interrupt is not supported by hardware,
> why would you add "unsupported" interrupt?
bcs the I2C driver is based on interrupt whatever I2C if use FIFO or dma.
So I judge here whether the client successfully obtains the irq. If not, the I2C
driver is unavailable.

I think I need to delete this `if`? Because if the I2C driver fails to load, the
I2C device driver will not be loaded
> 
>> +
>> +	match_data = of_device_get_match_data(&client->dev);
>> +	if (WARN_ON(!match_data))
>> +		return -EINVAL;
>> +
>> +	pmic = devm_kzalloc(&client->dev, sizeof(*pmic), GFP_KERNEL);
>> +	if (!pmic)
>> +		return -ENOMEM;
>> +
>> +	cells = match_data->mfd_cells;
>> +	nr_cells = match_data->nr_cells;
>> +
>> +	pmic->regmap_cfg = match_data->regmap_cfg;
>> +	pmic->regmap_irq_chip = match_data->regmap_irq_chip;
>> +	pmic->i2c = client;
>> +	pmic->match_data = match_data;
>> +	pmic->regmap = devm_regmap_init_i2c(client, pmic->regmap_cfg);
>> +	if (IS_ERR(pmic->regmap))
>> +		return dev_err_probe(&client->dev,
>> +				     PTR_ERR(pmic->regmap),
>> +				     "regmap initialization failed");
>> +
>> +	regcache_cache_bypass(pmic->regmap, true);
>> +
>> +	i2c_set_clientdata(client, pmic);
>> +
>> +	if (pmic->regmap_irq_chip) {
> 
> 
> It's impossible to have it false. Test your driver.
SpacemiT has another PMIC named P1S. I'm not sure if P1S has these features, and
I don't have a P1S chip to test and verify.
Therefore, I added a judgement here. But I will drop them in next version. I
should add the check only after confirming that P1S really doesn't have these
features
> 
>> +		ret = regmap_add_irq_chip(pmic->regmap, client->irq, IRQF_ONESHOT, -1,
>> +						pmic->regmap_irq_chip, &pmic->irq_data);
>> +		if (ret)
>> +			return dev_err_probe(&client->dev, ret, "failed to add irqchip");
>> +	}
>> +
>> +	dev_pm_set_wake_irq(&client->dev, client->irq);
>> +	device_init_wakeup(&client->dev, true);
>> +
>> +	ret = devm_mfd_add_devices(&client->dev, PLATFORM_DEVID_NONE,
>> +				   cells, nr_cells, NULL, 0,
>> +				   regmap_irq_get_domain(pmic->irq_data));
>> +	if (ret)
>> +		return dev_err_probe(&client->dev, ret, "failed to add MFD devices");
>> +
>> +	if (match_data->shutdown.reg) {
> 
> Also not possible, useless if.
same
> 
>> +		ret = devm_register_sys_off_handler(&client->dev,
>> +						    SYS_OFF_MODE_POWER_OFF_PREPARE,
>> +						    SYS_OFF_PRIO_HIGH,
>> +						    &spacemit_pmic_shutdown,
>> +						    pmic);
>> +		if (ret)
>> +			return dev_err_probe(&client->dev,
>> +					     ret,
>> +					     "failed to register restart handler");
>> +
>> +	}
>> +
>> +	if (match_data->reboot.reg) {
> 
> Also not possible.
same
> 
>> +		ret = devm_register_sys_off_handler(&client->dev,
>> +						    SYS_OFF_MODE_RESTART,
>> +						    SYS_OFF_PRIO_HIGH,
>> +						    &spacemit_pmic_restart,
>> +						    pmic);
>> +		if (ret)
>> +			return dev_err_probe(&client->dev,
>> +					     ret,
>> +					     "failed to register restart handler");
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct of_device_id spacemit_pmic_of_match[] = {
>> +	{ .compatible = "spacemit,p1", .data = &pmic_p1_match_data },
>> +	{ /* sentinel */ }
>> +};
>> +MODULE_DEVICE_TABLE(of, spacemit_pmic_of_match);
>> +
>> +static struct i2c_driver spacemit_pmic_i2c_driver = {
>> +	.driver = {
>> +		.name = "spacemit-pmic",
>> +		.of_match_table = spacemit_pmic_of_match,
>> +	},
>> +	.probe    = spacemit_pmic_probe,
>> +};
>> +
>> +static int __init spacemit_pmic_init(void)
>> +{
>> +	return platform_driver_register(&spacemit_pmic_i2c_driver);
>> +}
>> +
>> +static void __exit spacemit_pmic_exit(void)
>> +{
>> +	platform_driver_unregister(&spacemit_pmic_i2c_driver);
>> +}
>> +
>> +module_init(spacemit_pmic_init);
>> +module_exit(spacemit_pmic_exit);
> 
> Use proper wrapper for these above.
ok
> 
>> +
>> +MODULE_LICENSE("GPL");
>> +MODULE_DESCRIPTION("mfd core driver for the SpacemiT PMIC");
> 
> ...
> 
>> +
>> +#define P1_MAX_REG			0xA8
>> +
>> +#define P1_BUCK_VSEL_MASK		0xff
>> +#define P1_BUCK_EN_MASK			0x1
>> +
>> +#define P1_BUCK1_CTRL_REG		0x47
>> +#define P1_BUCK2_CTRL_REG		0x4a
> 
> 
> Either lowercase or uppercase hex, not both.
ok
> 
>> +#define P1_BUCK3_CTRL_REG		0x4d
>> +#define P1_BUCK4_CTRL_REG		0x50
>> +#define P1_BUCK5_CTRL_REG		0x53
>> +#define P1_BUCK6_CTRL_REG		0x56
>> +
>> +#define P1_BUCK1_VSEL_REG		0x48
>> +#define P1_BUCK2_VSEL_REG		0x4b
>> +#define P1_BUCK3_VSEL_REG		0x4e
>> +#define P1_BUCK4_VSEL_REG		0x51
>> +#define P1_BUCK5_VSEL_REG		0x54
>> +#define P1_BUCK6_VSEL_REG		0x57
>> +
>> +#define P1_ALDO1_CTRL_REG		0x5b
>> +#define P1_ALDO2_CTRL_REG		0x5e
>> +#define P1_ALDO3_CTRL_REG		0x61
>> +#define P1_ALDO4_CTRL_REG		0x64
>> +
>> +#define P1_ALDO1_VOLT_REG		0x5c
>> +#define P1_ALDO2_VOLT_REG		0x5f
>> +#define P1_ALDO3_VOLT_REG		0x62
>> +#define P1_ALDO4_VOLT_REG		0x65
>> +
>> +#define P1_ALDO_EN_MASK			0x1
>> +#define P1_ALDO_VSEL_MASK		0x7f
>> +
>> +#define P1_DLDO1_CTRL_REG		0x67
>> +#define P1_DLDO2_CTRL_REG		0x6a
>> +#define P1_DLDO3_CTRL_REG		0x6d
>> +#define P1_DLDO4_CTRL_REG		0x70
>> +#define P1_DLDO5_CTRL_REG		0x73
>> +#define P1_DLDO6_CTRL_REG		0x76
>> +#define P1_DLDO7_CTRL_REG		0x79
>> +
>> +#define P1_DLDO1_VOLT_REG		0x68
>> +#define P1_DLDO2_VOLT_REG		0x6b
>> +#define P1_DLDO3_VOLT_REG		0x6e
>> +#define P1_DLDO4_VOLT_REG		0x71
>> +#define P1_DLDO5_VOLT_REG		0x74
>> +#define P1_DLDO6_VOLT_REG		0x77
>> +#define P1_DLDO7_VOLT_REG		0x7a
>> +
>> +#define P1_DLDO_EN_MASK			0x1
>> +#define P1_DLDO_VSEL_MASK		0x7f
>> +
>> +#define P1_SWITCH_CTRL_REG		0x59
>> +#define P1_SWTICH_EN_MASK		0x1
>> +
>> +#define P1_PWR_CTRL2			0x7e
>> +#define P1_SW_SHUTDOWN_BIT_MSK		0x4
>> +#define P1_SW_RESET_BIT_MSK		0x2
>> +
>> +#define P1_E_GPI0_MSK			BIT(0)
>> +#define P1_E_GPI1_MSK			BIT(1)
>> +#define P1_E_GPI2_MSK			BIT(2)
>> +#define P1_E_GPI3_MSK			BIT(3)
>> +#define P1_E_GPI4_MSK			BIT(4)
>> +#define P1_E_GPI5_MSK			BIT(5)
>> +
>> +#define P1_E_ADC_TEMP_MSK		BIT(0)
>> +#define P1_E_ADC_EOC_MSK		BIT(1)
>> +#define P1_E_ADC_EOS_MSK		BIT(2)
>> +#define P1_E_WDT_TO_MSK			BIT(3)
>> +#define P1_E_ALARM_MSK			BIT(4)
>> +#define P1_E_TICK_MSK			BIT(5)
>> +
>> +#define P1_E_LDO_OV_MSK			BIT(0)
>> +#define P1_E_LDO_UV_MSK			BIT(1)
>> +#define P1_E_LDO_SC_MSK			BIT(2)
>> +#define P1_E_SW_SC_MSK			BIT(3)
>> +#define P1_E_TEMP_WARN_MSK		BIT(4)
>> +#define P1_E_TEMP_SEVERE_MSK		BIT(5)
>> +#define P1_E_TEMP_CRIT_MSK		BIT(6)
>> +
>> +#define P1_E_BUCK1_OV_MSK		BIT(0)
>> +#define P1_E_BUCK2_OV_MSK		BIT(1)
>> +#define P1_E_BUCK3_OV_MSK		BIT(2)
>> +#define P1_E_BUCK4_OV_MSK		BIT(3)
>> +#define P1_E_BUCK5_OV_MSK		BIT(4)
>> +#define P1_E_BUCK6_OV_MSK		BIT(5)
>> +
>> +#define P1_E_BUCK1_UV_MSK		BIT(0)
>> +#define P1_E_BUCK2_UV_MSK		BIT(1)
>> +#define P1_E_BUCK3_UV_MSK		BIT(2)
>> +#define P1_E_BUCK4_UV_MSK		BIT(3)
>> +#define P1_E_BUCK5_UV_MSK		BIT(4)
>> +#define P1_E_BUCK6_UV_MSK		BIT(5)
>> +
>> +#define P1_E_BUCK1_SC_MSK		BIT(0)
>> +#define P1_E_BUCK2_SC_MSK		BIT(1)
>> +#define P1_E_BUCK3_SC_MSK		BIT(2)
>> +#define P1_E_BUCK4_SC_MSK		BIT(3)
>> +#define P1_E_BUCK5_SC_MSK		BIT(4)
>> +#define P1_E_BUCK6_SC_MSK		BIT(5)
>> +
>> +#define P1_E_PWRON_RINTR_MSK		BIT(0)
>> +#define P1_E_PWRON_FINTR_MSK		BIT(1)
>> +#define P1_E_PWRON_SINTR_MSK		BIT(2)
>> +#define P1_E_PWRON_LINTR_MSK		BIT(3)
>> +#define P1_E_PWRON_SDINTR_MSK		BIT(4)
>> +#define P1_E_VSYS_OV_MSK		BIT(5)
>> +
>> +#define P1_E_STATUS_REG_BASE		0x91
>> +#define P1_E_EN_REG_BASE		0x98
>> +
>> +#define P1_REGMAP_CONFIG	\
>> +	static const struct regmap_config p1_regmap_config = {	\
>> +		.reg_bits = 8,					\
>> +		.val_bits = 8,					\
>> +		.max_register = P1_MAX_REG,			\
>> +		.cache_type = REGCACHE_RBTREE,			\
>> +	}
>> +
>> +#define P1_IRQS_DESC					\
>> +static const struct regmap_irq p1_irqs[] = {		\
> 
> 
> No, all these defines are just not needed, not readable. Please follow
> existing kernel style - just look at recent drivers in drivers/mfd/ to
> see how they are designed and developed.
ok thanks!
> 
>> +	[P1_E_GPI0] = {					\
>> +		.mask = P1_E_GPI0_MSK,			\
>> +		.reg_offset = 0,			\
>> +	},						\
>> +							\
>> +	[P1_E_GPI1] = {					\
>> +		.mask = P1_E_GPI1_MSK,			\
> 
> Best regards,
> Krzysztof

Jure Repinc Dec. 31, 2024, 7:42 p.m. UTC | #4

On 30/12/2024 11:02, Troy Mitchell wrote:

> +static int spacemit_pmic_shutdown(struct sys_off_data *data)
> +{
> +	int ret;
> +	struct spacemit_pmic *pmic = data->cb_data;
> +
> +	ret = regmap_update_bits(pmic->regmap,
> +				 pmic->match_data->shutdown.reg,
> +				 pmic->match_data->shutdown.bit,
> +				 pmic->match_data->shutdown.bit);
> +	if (ret)
> +		dev_err(data->dev, "failed to reboot device!");

reboot -> shutdown

> +	if (match_data->shutdown.reg) {
> +		ret = devm_register_sys_off_handler(&client->dev,
> +						    
SYS_OFF_MODE_POWER_OFF_PREPARE,
> +						    SYS_OFF_PRIO_HIGH,
> +						    
&spacemit_pmic_shutdown,
> +						    pmic);
> +		if (ret)
> +			return dev_err_probe(&client->dev,
> +					     ret,
> +					     "failed to register restart 
handler");

restart -> shutdown



Have a great time,
Jure Repinc

Patch

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index ae23b317a64e49f0cb529ae6bd1becbb90b7c282..c062bf6b11fd23d420a6d5f6ee51b3ec97f9fcbb 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -1173,6 +1173,20 @@  config MFD_QCOM_RPM
 	  Say M here if you want to include support for the Qualcomm RPM as a
 	  module. This will build a module called "qcom_rpm".
 
+config MFD_SPACEMIT_PMIC
+	tristate "SpacemiT PMIC"
+	depends on ARCH_SPACEMIT || COMPILE_TEST
+	depends on I2C && OF
+	select MFD_CORE
+	select REGMAP_I2C
+	select REGMAP_IRQ
+	help
+	  If this option is turned on, the P1 chip produced by SpacemiT will
+	  be supported.
+
+	  This driver can also be compiled as a module. If you choose to build
+	  it as a module, the resulting kernel module will be named `spacemit-pmic`.
+
 config MFD_SPMI_PMIC
 	tristate "Qualcomm SPMI PMICs"
 	depends on ARCH_QCOM || COMPILE_TEST
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index e057d6d6faef5c1d639789e2560f336fa26cd872..284dbb8fe2ef83bdd994a598504fe315f2eabbdf 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -266,6 +266,7 @@  obj-$(CONFIG_MFD_SUN4I_GPADC)	+= sun4i-gpadc.o
 obj-$(CONFIG_MFD_STM32_LPTIMER)	+= stm32-lptimer.o
 obj-$(CONFIG_MFD_STM32_TIMERS) 	+= stm32-timers.o
 obj-$(CONFIG_MFD_MXS_LRADC)     += mxs-lradc.o
+obj-$(CONFIG_MFD_SPACEMIT_PMIC)	+= spacemit-pmic.o
 obj-$(CONFIG_MFD_SC27XX_PMIC)	+= sprd-sc27xx-spi.o
 obj-$(CONFIG_RAVE_SP_CORE)	+= rave-sp.o
 obj-$(CONFIG_MFD_ROHM_BD71828)	+= rohm-bd71828.o
diff --git a/drivers/mfd/spacemit-pmic.c b/drivers/mfd/spacemit-pmic.c
new file mode 100644
index 0000000000000000000000000000000000000000..d9f6785cecbd405821dead13cdf8d1f9fd64e508
--- /dev/null
+++ b/drivers/mfd/spacemit-pmic.c
@@ -0,0 +1,159 @@ 
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2024 Troy Mitchell <[e-mail]>
+ */
+
+#include <linux/i2c.h>
+#include <linux/mfd/core.h>
+#include <linux/mfd/spacemit/spacemit-pmic.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/pm_wakeirq.h>
+#include <linux/reboot.h>
+
+P1_REGMAP_CONFIG;
+P1_IRQS_DESC;
+P1_IRQ_CHIP_DESC;
+P1_POWER_KEY_RESOURCES_DESC;
+P1_RTC_RESOURCES_DESC;
+P1_MFD_CELL;
+P1_MFD_MATCH_DATA;
+
+static int spacemit_pmic_shutdown(struct sys_off_data *data)
+{
+	int ret;
+	struct spacemit_pmic *pmic = data->cb_data;
+
+	ret = regmap_update_bits(pmic->regmap,
+				 pmic->match_data->shutdown.reg,
+				 pmic->match_data->shutdown.bit,
+				 pmic->match_data->shutdown.bit);
+	if (ret)
+		dev_err(data->dev, "failed to reboot device!");
+
+	return NOTIFY_DONE;
+}
+
+static int spacemit_pmic_restart(struct sys_off_data *data)
+{
+	int ret;
+	struct spacemit_pmic *pmic = data->cb_data;
+
+	ret = regmap_update_bits(pmic->regmap,
+				 pmic->match_data->reboot.reg,
+				 pmic->match_data->reboot.bit,
+				 pmic->match_data->reboot.bit);
+	if (ret)
+		dev_err(data->dev, "failed to reboot device!");
+
+	return NOTIFY_DONE;
+}
+
+static int spacemit_pmic_probe(struct i2c_client *client)
+{
+	const struct spacemit_pmic_match_data *match_data;
+	const struct mfd_cell *cells;
+	struct spacemit_pmic *pmic;
+	int nr_cells, ret;
+
+	if (!client->irq)
+		return dev_err_probe(&client->dev, -ENXIO, "no interrupt supported");
+
+	match_data = of_device_get_match_data(&client->dev);
+	if (WARN_ON(!match_data))
+		return -EINVAL;
+
+	pmic = devm_kzalloc(&client->dev, sizeof(*pmic), GFP_KERNEL);
+	if (!pmic)
+		return -ENOMEM;
+
+	cells = match_data->mfd_cells;
+	nr_cells = match_data->nr_cells;
+
+	pmic->regmap_cfg = match_data->regmap_cfg;
+	pmic->regmap_irq_chip = match_data->regmap_irq_chip;
+	pmic->i2c = client;
+	pmic->match_data = match_data;
+	pmic->regmap = devm_regmap_init_i2c(client, pmic->regmap_cfg);
+	if (IS_ERR(pmic->regmap))
+		return dev_err_probe(&client->dev,
+				     PTR_ERR(pmic->regmap),
+				     "regmap initialization failed");
+
+	regcache_cache_bypass(pmic->regmap, true);
+
+	i2c_set_clientdata(client, pmic);
+
+	if (pmic->regmap_irq_chip) {
+		ret = regmap_add_irq_chip(pmic->regmap, client->irq, IRQF_ONESHOT, -1,
+						pmic->regmap_irq_chip, &pmic->irq_data);
+		if (ret)
+			return dev_err_probe(&client->dev, ret, "failed to add irqchip");
+	}
+
+	dev_pm_set_wake_irq(&client->dev, client->irq);
+	device_init_wakeup(&client->dev, true);
+
+	ret = devm_mfd_add_devices(&client->dev, PLATFORM_DEVID_NONE,
+				   cells, nr_cells, NULL, 0,
+				   regmap_irq_get_domain(pmic->irq_data));
+	if (ret)
+		return dev_err_probe(&client->dev, ret, "failed to add MFD devices");
+
+	if (match_data->shutdown.reg) {
+		ret = devm_register_sys_off_handler(&client->dev,
+						    SYS_OFF_MODE_POWER_OFF_PREPARE,
+						    SYS_OFF_PRIO_HIGH,
+						    &spacemit_pmic_shutdown,
+						    pmic);
+		if (ret)
+			return dev_err_probe(&client->dev,
+					     ret,
+					     "failed to register restart handler");
+
+	}
+
+	if (match_data->reboot.reg) {
+		ret = devm_register_sys_off_handler(&client->dev,
+						    SYS_OFF_MODE_RESTART,
+						    SYS_OFF_PRIO_HIGH,
+						    &spacemit_pmic_restart,
+						    pmic);
+		if (ret)
+			return dev_err_probe(&client->dev,
+					     ret,
+					     "failed to register restart handler");
+	}
+
+	return 0;
+}
+
+static const struct of_device_id spacemit_pmic_of_match[] = {
+	{ .compatible = "spacemit,p1", .data = &pmic_p1_match_data },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, spacemit_pmic_of_match);
+
+static struct i2c_driver spacemit_pmic_i2c_driver = {
+	.driver = {
+		.name = "spacemit-pmic",
+		.of_match_table = spacemit_pmic_of_match,
+	},
+	.probe    = spacemit_pmic_probe,
+};
+
+static int __init spacemit_pmic_init(void)
+{
+	return platform_driver_register(&spacemit_pmic_i2c_driver);
+}
+
+static void __exit spacemit_pmic_exit(void)
+{
+	platform_driver_unregister(&spacemit_pmic_i2c_driver);
+}
+
+module_init(spacemit_pmic_init);
+module_exit(spacemit_pmic_exit);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("mfd core driver for the SpacemiT PMIC");
diff --git a/include/linux/mfd/spacemit/spacemit-p1.h b/include/linux/mfd/spacemit/spacemit-p1.h
new file mode 100644
index 0000000000000000000000000000000000000000..6d4d5126b79d6b887d23dea097ec585ecca5e758
--- /dev/null
+++ b/include/linux/mfd/spacemit/spacemit-p1.h
@@ -0,0 +1,491 @@ 
+/* SPDX-License-Identifier: GPL-2.0-only
+ *
+ * Copyright (C) 2024 Troy Mitchell <[e-mail]>
+ */
+#ifndef __SPACEMIT_P1_H__
+#define __SPACEMIT_P1_H__
+
+enum p1_reg {
+	P1_ID_DCDC1,
+	P1_ID_DCDC2,
+	P1_ID_DCDC3,
+	P1_ID_DCDC4,
+	P1_ID_DCDC5,
+	P1_ID_DCDC6,
+	P1_ID_LDO1,
+	P1_ID_LDO2,
+	P1_ID_LDO3,
+	P1_ID_LDO4,
+	P1_ID_LDO5,
+	P1_ID_LDO6,
+	P1_ID_LDO7,
+	P1_ID_LDO8,
+	P1_ID_LDO9,
+	P1_ID_LDO10,
+	P1_ID_LDO11,
+	P1_ID_SWITCH1,
+};
+
+enum p1_irq_line {
+	/* reg: 0x91 */
+	P1_E_GPI0,
+	P1_E_GPI1,
+	P1_E_GPI2,
+	P1_E_GPI3,
+	P1_E_GPI4,
+	P1_E_GPI5,
+
+	/* reg: 0x92 */
+	P1_E_ADC_TEMP,
+	P1_E_ADC_EOC,
+	P1_E_ADC_EOS,
+	P1_E_WDT_TO,
+	P1_E_ALARM,
+	P1_E_TICK,
+
+	/* reg: 0x93 */
+	P1_E_LDO_OV,
+	P1_E_LDO_UV,
+	P1_E_LDO_SC,
+	P1_E_SW_SC,
+	P1_E_TEMP_WARN,
+	P1_E_TEMP_SEVERE,
+	P1_E_TEMP_CRIT,
+
+	/* reg: 0x94 */
+	P1_E_BUCK1_OV,
+	P1_E_BUCK2_OV,
+	P1_E_BUCK3_OV,
+	P1_E_BUCK4_OV,
+	P1_E_BUCK5_OV,
+	P1_E_BUCK6_OV,
+
+	/* reg: 0x95 */
+	P1_E_BUCK1_UV,
+	P1_E_BUCK2_UV,
+	P1_E_BUCK3_UV,
+	P1_E_BUCK4_UV,
+	P1_E_BUCK5_UV,
+	P1_E_BUCK6_UV,
+
+	/* reg: 0x96 */
+	P1_E_BUCK1_SC,
+	P1_E_BUCK2_SC,
+	P1_E_BUCK3_SC,
+	P1_E_BUCK4_SC,
+	P1_E_BUCK5_SC,
+	P1_E_BUCK6_SC,
+
+	/* reg: 0x97 */
+	P1_E_PWRON_RINTR,
+	P1_E_PWRON_FINTR,
+	P1_E_PWRON_SINTR,
+	P1_E_PWRON_LINTR,
+	P1_E_PWRON_SDINTR,
+	P1_E_VSYS_OV,
+};
+
+#define P1_MAX_REG			0xA8
+
+#define P1_BUCK_VSEL_MASK		0xff
+#define P1_BUCK_EN_MASK			0x1
+
+#define P1_BUCK1_CTRL_REG		0x47
+#define P1_BUCK2_CTRL_REG		0x4a
+#define P1_BUCK3_CTRL_REG		0x4d
+#define P1_BUCK4_CTRL_REG		0x50
+#define P1_BUCK5_CTRL_REG		0x53
+#define P1_BUCK6_CTRL_REG		0x56
+
+#define P1_BUCK1_VSEL_REG		0x48
+#define P1_BUCK2_VSEL_REG		0x4b
+#define P1_BUCK3_VSEL_REG		0x4e
+#define P1_BUCK4_VSEL_REG		0x51
+#define P1_BUCK5_VSEL_REG		0x54
+#define P1_BUCK6_VSEL_REG		0x57
+
+#define P1_ALDO1_CTRL_REG		0x5b
+#define P1_ALDO2_CTRL_REG		0x5e
+#define P1_ALDO3_CTRL_REG		0x61
+#define P1_ALDO4_CTRL_REG		0x64
+
+#define P1_ALDO1_VOLT_REG		0x5c
+#define P1_ALDO2_VOLT_REG		0x5f
+#define P1_ALDO3_VOLT_REG		0x62
+#define P1_ALDO4_VOLT_REG		0x65
+
+#define P1_ALDO_EN_MASK			0x1
+#define P1_ALDO_VSEL_MASK		0x7f
+
+#define P1_DLDO1_CTRL_REG		0x67
+#define P1_DLDO2_CTRL_REG		0x6a
+#define P1_DLDO3_CTRL_REG		0x6d
+#define P1_DLDO4_CTRL_REG		0x70
+#define P1_DLDO5_CTRL_REG		0x73
+#define P1_DLDO6_CTRL_REG		0x76
+#define P1_DLDO7_CTRL_REG		0x79
+
+#define P1_DLDO1_VOLT_REG		0x68
+#define P1_DLDO2_VOLT_REG		0x6b
+#define P1_DLDO3_VOLT_REG		0x6e
+#define P1_DLDO4_VOLT_REG		0x71
+#define P1_DLDO5_VOLT_REG		0x74
+#define P1_DLDO6_VOLT_REG		0x77
+#define P1_DLDO7_VOLT_REG		0x7a
+
+#define P1_DLDO_EN_MASK			0x1
+#define P1_DLDO_VSEL_MASK		0x7f
+
+#define P1_SWITCH_CTRL_REG		0x59
+#define P1_SWTICH_EN_MASK		0x1
+
+#define P1_PWR_CTRL2			0x7e
+#define P1_SW_SHUTDOWN_BIT_MSK		0x4
+#define P1_SW_RESET_BIT_MSK		0x2
+
+#define P1_E_GPI0_MSK			BIT(0)
+#define P1_E_GPI1_MSK			BIT(1)
+#define P1_E_GPI2_MSK			BIT(2)
+#define P1_E_GPI3_MSK			BIT(3)
+#define P1_E_GPI4_MSK			BIT(4)
+#define P1_E_GPI5_MSK			BIT(5)
+
+#define P1_E_ADC_TEMP_MSK		BIT(0)
+#define P1_E_ADC_EOC_MSK		BIT(1)
+#define P1_E_ADC_EOS_MSK		BIT(2)
+#define P1_E_WDT_TO_MSK			BIT(3)
+#define P1_E_ALARM_MSK			BIT(4)
+#define P1_E_TICK_MSK			BIT(5)
+
+#define P1_E_LDO_OV_MSK			BIT(0)
+#define P1_E_LDO_UV_MSK			BIT(1)
+#define P1_E_LDO_SC_MSK			BIT(2)
+#define P1_E_SW_SC_MSK			BIT(3)
+#define P1_E_TEMP_WARN_MSK		BIT(4)
+#define P1_E_TEMP_SEVERE_MSK		BIT(5)
+#define P1_E_TEMP_CRIT_MSK		BIT(6)
+
+#define P1_E_BUCK1_OV_MSK		BIT(0)
+#define P1_E_BUCK2_OV_MSK		BIT(1)
+#define P1_E_BUCK3_OV_MSK		BIT(2)
+#define P1_E_BUCK4_OV_MSK		BIT(3)
+#define P1_E_BUCK5_OV_MSK		BIT(4)
+#define P1_E_BUCK6_OV_MSK		BIT(5)
+
+#define P1_E_BUCK1_UV_MSK		BIT(0)
+#define P1_E_BUCK2_UV_MSK		BIT(1)
+#define P1_E_BUCK3_UV_MSK		BIT(2)
+#define P1_E_BUCK4_UV_MSK		BIT(3)
+#define P1_E_BUCK5_UV_MSK		BIT(4)
+#define P1_E_BUCK6_UV_MSK		BIT(5)
+
+#define P1_E_BUCK1_SC_MSK		BIT(0)
+#define P1_E_BUCK2_SC_MSK		BIT(1)
+#define P1_E_BUCK3_SC_MSK		BIT(2)
+#define P1_E_BUCK4_SC_MSK		BIT(3)
+#define P1_E_BUCK5_SC_MSK		BIT(4)
+#define P1_E_BUCK6_SC_MSK		BIT(5)
+
+#define P1_E_PWRON_RINTR_MSK		BIT(0)
+#define P1_E_PWRON_FINTR_MSK		BIT(1)
+#define P1_E_PWRON_SINTR_MSK		BIT(2)
+#define P1_E_PWRON_LINTR_MSK		BIT(3)
+#define P1_E_PWRON_SDINTR_MSK		BIT(4)
+#define P1_E_VSYS_OV_MSK		BIT(5)
+
+#define P1_E_STATUS_REG_BASE		0x91
+#define P1_E_EN_REG_BASE		0x98
+
+#define P1_REGMAP_CONFIG	\
+	static const struct regmap_config p1_regmap_config = {	\
+		.reg_bits = 8,					\
+		.val_bits = 8,					\
+		.max_register = P1_MAX_REG,			\
+		.cache_type = REGCACHE_RBTREE,			\
+	}
+
+#define P1_IRQS_DESC					\
+static const struct regmap_irq p1_irqs[] = {		\
+	[P1_E_GPI0] = {					\
+		.mask = P1_E_GPI0_MSK,			\
+		.reg_offset = 0,			\
+	},						\
+							\
+	[P1_E_GPI1] = {					\
+		.mask = P1_E_GPI1_MSK,			\
+		.reg_offset = 0,			\
+	},						\
+							\
+	[P1_E_GPI2] = {					\
+		.mask = P1_E_GPI2_MSK,			\
+		.reg_offset = 0,			\
+	},						\
+							\
+	[P1_E_GPI3] = {					\
+		.mask = P1_E_GPI3_MSK,			\
+		.reg_offset = 0,			\
+	},						\
+							\
+	[P1_E_GPI4] = {					\
+		.mask = P1_E_GPI4_MSK,			\
+		.reg_offset = 0,			\
+	},						\
+							\
+	[P1_E_GPI5] = {					\
+		.mask = P1_E_GPI5_MSK,			\
+		.reg_offset = 0,			\
+	},						\
+							\
+	[P1_E_ADC_TEMP] = {				\
+		.mask = P1_E_ADC_TEMP_MSK,		\
+		.reg_offset = 1,			\
+	},						\
+							\
+	[P1_E_ADC_EOC] = {				\
+		.mask = P1_E_ADC_EOC_MSK,		\
+		.reg_offset = 1,			\
+	},						\
+							\
+	[P1_E_ADC_EOS] = {				\
+		.mask = P1_E_ADC_EOS_MSK,		\
+		.reg_offset = 1,			\
+	},						\
+							\
+	[P1_E_WDT_TO] = {				\
+		.mask = P1_E_WDT_TO_MSK,		\
+		.reg_offset = 1,			\
+	},						\
+							\
+	[P1_E_ALARM] = {				\
+		.mask = P1_E_ALARM_MSK,			\
+		.reg_offset = 1,			\
+	},						\
+							\
+	[P1_E_TICK] = {					\
+		.mask = P1_E_TICK_MSK,			\
+		.reg_offset = 1,			\
+	},						\
+							\
+	[P1_E_LDO_OV] = {				\
+		.mask = P1_E_LDO_OV_MSK,		\
+		.reg_offset = 2,			\
+	},						\
+							\
+	[P1_E_LDO_UV] = {				\
+		.mask = P1_E_LDO_UV_MSK,		\
+		.reg_offset = 2,			\
+	},						\
+							\
+	[P1_E_LDO_SC] = {				\
+		.mask = P1_E_LDO_SC_MSK,		\
+		.reg_offset = 2,			\
+	},						\
+							\
+	[P1_E_SW_SC] = {				\
+		.mask = P1_E_SW_SC_MSK,			\
+		.reg_offset = 2,			\
+	},						\
+							\
+	[P1_E_TEMP_WARN] = {				\
+		.mask = P1_E_TEMP_WARN_MSK,		\
+		.reg_offset = 2,			\
+	},						\
+							\
+	[P1_E_TEMP_SEVERE] = {				\
+		.mask = P1_E_TEMP_SEVERE_MSK,		\
+		.reg_offset = 2,			\
+	},						\
+							\
+	[P1_E_TEMP_CRIT] = {				\
+		.mask = P1_E_TEMP_CRIT_MSK,		\
+		.reg_offset = 2,			\
+	},						\
+							\
+	[P1_E_BUCK1_OV] = {				\
+		.mask = P1_E_BUCK1_OV_MSK,		\
+		.reg_offset = 3,			\
+	},						\
+							\
+	[P1_E_BUCK2_OV] = {				\
+		.mask = P1_E_BUCK2_OV_MSK,		\
+		.reg_offset = 3,			\
+	},						\
+							\
+	[P1_E_BUCK3_OV] = {				\
+		.mask = P1_E_BUCK3_OV_MSK,		\
+		.reg_offset = 3,			\
+	},						\
+							\
+	[P1_E_BUCK4_OV] = {				\
+		.mask = P1_E_BUCK4_OV_MSK,		\
+		.reg_offset = 3,			\
+	},						\
+							\
+	[P1_E_BUCK5_OV] = {				\
+		.mask = P1_E_BUCK5_OV_MSK,		\
+		.reg_offset = 3,			\
+	},						\
+							\
+	[P1_E_BUCK6_OV] = {				\
+		.mask = P1_E_BUCK6_OV_MSK,		\
+		.reg_offset = 3,			\
+	},						\
+							\
+	[P1_E_BUCK1_UV] = {				\
+		.mask = P1_E_BUCK1_UV_MSK,		\
+		.reg_offset = 4,			\
+	},						\
+							\
+	[P1_E_BUCK2_UV] = {				\
+		.mask = P1_E_BUCK2_UV_MSK,		\
+		.reg_offset = 4,			\
+	},						\
+							\
+	[P1_E_BUCK3_UV] = {				\
+		.mask = P1_E_BUCK3_UV_MSK,		\
+		.reg_offset = 4,			\
+	},						\
+							\
+	[P1_E_BUCK4_UV] = {				\
+		.mask = P1_E_BUCK4_UV_MSK,		\
+		.reg_offset = 4,			\
+	},						\
+							\
+	[P1_E_BUCK5_UV] = {				\
+		.mask = P1_E_BUCK5_UV_MSK,		\
+		.reg_offset = 4,			\
+	},						\
+							\
+	[P1_E_BUCK6_UV] = {				\
+		.mask = P1_E_BUCK6_UV_MSK,		\
+		.reg_offset = 4,			\
+	},						\
+							\
+	[P1_E_BUCK1_SC] = {				\
+		.mask = P1_E_BUCK1_SC_MSK,		\
+		.reg_offset = 5,			\
+	},						\
+							\
+	[P1_E_BUCK2_SC] = {				\
+		.mask = P1_E_BUCK2_SC_MSK,		\
+		.reg_offset = 5,			\
+	},						\
+							\
+	[P1_E_BUCK3_SC] = {				\
+		.mask = P1_E_BUCK3_SC_MSK,		\
+		.reg_offset = 5,			\
+	},						\
+							\
+	[P1_E_BUCK4_SC] = {				\
+		.mask = P1_E_BUCK4_SC_MSK,		\
+		.reg_offset = 5,			\
+	},						\
+							\
+	[P1_E_BUCK5_SC] = {				\
+		.mask = P1_E_BUCK5_SC_MSK,		\
+		.reg_offset = 5,			\
+	},						\
+							\
+	[P1_E_BUCK6_SC] = {				\
+		.mask = P1_E_BUCK6_SC_MSK,		\
+		.reg_offset = 5,			\
+	},						\
+							\
+	[P1_E_PWRON_RINTR] = {				\
+		.mask = P1_E_PWRON_RINTR_MSK,		\
+		.reg_offset = 6,			\
+	},						\
+							\
+	[P1_E_PWRON_FINTR] = {				\
+		.mask = P1_E_PWRON_FINTR_MSK,		\
+		.reg_offset = 6,			\
+	},						\
+							\
+	[P1_E_PWRON_SINTR] = {				\
+		.mask = P1_E_PWRON_SINTR_MSK,		\
+		.reg_offset = 6,			\
+	},						\
+							\
+	[P1_E_PWRON_LINTR] = {				\
+		.mask = P1_E_PWRON_LINTR_MSK,		\
+		.reg_offset = 6,			\
+	},						\
+							\
+	[P1_E_PWRON_SDINTR] = {				\
+		.mask = P1_E_PWRON_SDINTR_MSK,		\
+		.reg_offset = 6,			\
+	},						\
+							\
+	[P1_E_VSYS_OV] = {				\
+		.mask = P1_E_VSYS_OV_MSK,		\
+		.reg_offset = 6,			\
+	},						\
+}
+
+#define P1_IRQ_CHIP_DESC				\
+static const struct regmap_irq_chip p1_irq_chip = {	\
+	.name			= "p1",			\
+	.irqs			= p1_irqs,		\
+	.num_irqs		= ARRAY_SIZE(p1_irqs),	\
+	.num_regs		= 7,			\
+	.status_base		= P1_E_STATUS_REG_BASE,	\
+	.mask_base		= P1_E_EN_REG_BASE,	\
+	.ack_base		= P1_E_STATUS_REG_BASE,	\
+	.init_ack_masked	= true,			\
+}
+
+#define P1_POWER_KEY_RESOURCES_DESC			\
+static const struct resource p1_pwrkey_resources[] = {	\
+	DEFINE_RES_IRQ(P1_E_PWRON_RINTR),		\
+	DEFINE_RES_IRQ(P1_E_PWRON_FINTR),		\
+	DEFINE_RES_IRQ(P1_E_PWRON_SINTR),		\
+	DEFINE_RES_IRQ(P1_E_PWRON_LINTR),		\
+}
+
+#define P1_RTC_RESOURCES_DESC				\
+static const struct resource p1_rtc_resources[] = {	\
+	DEFINE_RES_IRQ(P1_E_ALARM),			\
+}
+
+#define P1_MFD_CELL	\
+	static const struct mfd_cell p1[] = {					\
+		{								\
+			.name = "spacemit-regulator@p1",			\
+			.of_compatible = "pmic,p1-regulator",			\
+		},								\
+		{								\
+			.name = "spacemit-pinctrl@p1",				\
+			.of_compatible = "pmic,p1-pinctrl",			\
+		},								\
+		{								\
+			.name = "spacemit-pwrkey@p1",				\
+			.of_compatible = "pmic,p1-pwrkey",			\
+			.num_resources = ARRAY_SIZE(p1_pwrkey_resources),	\
+			.resources = &p1_pwrkey_resources[0],			\
+		},								\
+		{								\
+			.name = "spacemit-rtc@p1",				\
+			.of_compatible = "pmic,p1-rtc",				\
+			.num_resources = ARRAY_SIZE(p1_rtc_resources),		\
+			.resources = &p1_rtc_resources[0],			\
+		},								\
+	}
+
+#define P1_MFD_MATCH_DATA							\
+static struct spacemit_pmic_match_data pmic_p1_match_data = {			\
+	.regmap_cfg = &p1_regmap_config,					\
+	.regmap_irq_chip = &p1_irq_chip,					\
+	.mfd_cells = p1,							\
+	.nr_cells = ARRAY_SIZE(p1),						\
+	.name = "p1",								\
+	.shutdown = {								\
+		.reg = P1_PWR_CTRL2,						\
+		.bit = P1_SW_SHUTDOWN_BIT_MSK,					\
+	},									\
+	.reboot = {								\
+		.reg = P1_PWR_CTRL2,						\
+		.bit = P1_SW_RESET_BIT_MSK,					\
+	},									\
+}
+
+#endif // __SPACEMIT_P1_H__
diff --git a/include/linux/mfd/spacemit/spacemit-pmic.h b/include/linux/mfd/spacemit/spacemit-pmic.h
new file mode 100644
index 0000000000000000000000000000000000000000..808bbf1883c4ba019e552ffca94a10e46036ad3a
--- /dev/null
+++ b/include/linux/mfd/spacemit/spacemit-pmic.h
@@ -0,0 +1,39 @@ 
+/* SPDX-License-Identifier: GPL-2.0-only
+ *
+ * Copyright (C) 2024 Troy Mitchell <[e-mail]>
+ */
+#ifndef __SPACEMIT_PMIC_H__
+#define __SPACEMIT_PMIC_H__
+
+#include <linux/regulator/machine.h>
+#include <linux/regmap.h>
+#include <linux/mfd/spacemit/spacemit-p1.h>
+
+struct spacemit_pmic_match_data {
+	const struct regmap_config *regmap_cfg;
+	const struct regmap_irq_chip *regmap_irq_chip;
+	const struct mfd_cell *mfd_cells;
+	int nr_cells;
+	const char *name;
+
+	struct {
+		unsigned char reg;
+		unsigned char bit;
+	} shutdown;
+
+	struct {
+		unsigned int reg;
+		unsigned char bit;
+	} reboot;
+};
+
+struct spacemit_pmic {
+	struct i2c_client		*i2c;
+	struct regmap_irq_chip_data	*irq_data;
+	struct regmap			*regmap;
+	const struct regmap_config	*regmap_cfg;
+	const struct regmap_irq_chip	*regmap_irq_chip;
+	const struct spacemit_pmic_match_data *match_data;
+};
+
+#endif // __SPACEMIT_PMIC_H__