[PATCH v3 0/2] driver: reset: spacemit-p1: add driver for poweroff/reboot

[PATCH v3 0/2] driver: reset: spacemit-p1: add driver for poweroff/reboot

[Aurelien Jarno]

This adds poweroff/reboot support for the SpacemiT P1 PMIC chip, which is
commonly paired with the SpacemiT K1 SoC.

Note: For reliable operation, this driver depends on a this patch that adds
atomic transfer support to the SpacemiT I2C controller driver:
https://lore.kernel.org/spacemit/20251009-k1-i2c-atomic-v4-1-a89367870286@linux.spacemit.com/

Changes between version 2 and version 3:
- Allow building as a module
- Remove outdated Acked-by and Tested-by
- Collect Reviewed-by

Here is version 2 of this series:
  https://lore.kernel.org/spacemit/20251019191519.3898095-1-aurelien@aurel32.net/    

Changes between version 1 and version 2:
- Rebase onto v6.18-rc1
- Use dev_err_probe() to simplify the code
- Fix indentation of patch 1
- Collect Acked-by and Tested-by

Here is version 1 of this series:
  https://lore.kernel.org/spacemit/20250927220824.1267318-1-aurelien@aurel32.net/
  

Aurelien Jarno (2):
  driver: reset: spacemit-p1: add driver for poweroff/reboot
  mfd: simple-mfd-i2c: add a reboot cell for the SpacemiT P1 chip

 drivers/mfd/simple-mfd-i2c.c             |  1 +
 drivers/power/reset/Kconfig              |  9 +++
 drivers/power/reset/Makefile             |  1 +
 drivers/power/reset/spacemit-p1-reboot.c | 88 ++++++++++++++++++++++++
 4 files changed, 99 insertions(+)
 create mode 100644 drivers/power/reset/spacemit-p1-reboot.c

-- 
2.47.2

[PATCH v3 1/2] driver: reset: spacemit-p1: add driver for poweroff/reboot

[Aurelien Jarno]

This driver implements poweroff/reboot support for the SpacemiT P1 PMIC
chip, which is commonly paired with the SpacemiT K1 SoC.

The SpacemiT P1 support is implemented as a MFD driver, so the access is
done directly through the regmap interface. Reboot or poweroff is
triggered by setting a specific bit in a control register, which is
automatically cleared by the hardware afterwards.

Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
Reviewed-by: Yixun Lan <dlan@gentoo.org>
---
v3:
 - Allow building as a module
 - Remove outdated Acked-by and Tested-by
 - Collect Reviewed-by

 drivers/power/reset/Kconfig              |  9 +++
 drivers/power/reset/Makefile             |  1 +
 drivers/power/reset/spacemit-p1-reboot.c | 88 ++++++++++++++++++++++++
 3 files changed, 98 insertions(+)
 create mode 100644 drivers/power/reset/spacemit-p1-reboot.c

diff --git a/drivers/power/reset/Kconfig b/drivers/power/reset/Kconfig
index 8248895ca9038..6577d73edbda4 100644
--- a/drivers/power/reset/Kconfig
+++ b/drivers/power/reset/Kconfig
@@ -283,6 +283,15 @@ config POWER_RESET_KEYSTONE
 	help
 	  Reboot support for the KEYSTONE SoCs.
 
+config POWER_RESET_SPACEMIT_P1
+	tristate "SpacemiT P1 poweroff and reset driver"
+	depends on ARCH_SPACEMIT || COMPILE_TEST
+	select MFD_SPACEMIT_P1
+	default m
+	help
+	  This driver supports power-off and reset operations for the SpacemiT
+	  P1 PMIC.
+
 config POWER_RESET_SYSCON
 	bool "Generic SYSCON regmap reset driver"
 	depends on OF
diff --git a/drivers/power/reset/Makefile b/drivers/power/reset/Makefile
index 51da87e05ce76..0e4ae6f6b5c55 100644
--- a/drivers/power/reset/Makefile
+++ b/drivers/power/reset/Makefile
@@ -24,6 +24,7 @@ obj-$(CONFIG_POWER_RESET_LTC2952) += ltc2952-poweroff.o
 obj-$(CONFIG_POWER_RESET_QNAP) += qnap-poweroff.o
 obj-$(CONFIG_POWER_RESET_REGULATOR) += regulator-poweroff.o
 obj-$(CONFIG_POWER_RESET_RESTART) += restart-poweroff.o
+obj-$(CONFIG_POWER_RESET_SPACEMIT_P1) += spacemit-p1-reboot.o
 obj-$(CONFIG_POWER_RESET_ST) += st-poweroff.o
 obj-$(CONFIG_POWER_RESET_TH1520_AON) += th1520-aon-reboot.o
 obj-$(CONFIG_POWER_RESET_TORADEX_EC) += tdx-ec-poweroff.o
diff --git a/drivers/power/reset/spacemit-p1-reboot.c b/drivers/power/reset/spacemit-p1-reboot.c
new file mode 100644
index 0000000000000..9ec3d1fff8f3d
--- /dev/null
+++ b/drivers/power/reset/spacemit-p1-reboot.c
@@ -0,0 +1,88 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2025 by Aurelien Jarno
+ */
+
+#include <linux/bits.h>
+#include <linux/mod_devicetable.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/reboot.h>
+
+/* Power Control Register 2 */
+#define PWR_CTRL2		0x7e
+#define PWR_CTRL2_SHUTDOWN	BIT(2)	/* Shutdown request */
+#define PWR_CTRL2_RST		BIT(1)	/* Reset request */
+
+static int spacemit_p1_pwroff_handler(struct sys_off_data *data)
+{
+	struct regmap *regmap = data->cb_data;
+	int ret;
+
+	/* Put the PMIC into shutdown state */
+	ret = regmap_set_bits(regmap, PWR_CTRL2, PWR_CTRL2_SHUTDOWN);
+	if (ret) {
+		dev_err(data->dev, "shutdown failed: %d\n", ret);
+		return notifier_from_errno(ret);
+	}
+
+	return NOTIFY_DONE;
+}
+
+static int spacemit_p1_restart_handler(struct sys_off_data *data)
+{
+	struct regmap *regmap = data->cb_data;
+	int ret;
+
+	/* Put the PMIC into reset state */
+	ret = regmap_set_bits(regmap, PWR_CTRL2, PWR_CTRL2_RST);
+	if (ret) {
+		dev_err(data->dev, "restart failed: %d\n", ret);
+		return notifier_from_errno(ret);
+	}
+
+	return NOTIFY_DONE;
+}
+
+static int spacemit_p1_reboot_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct regmap *regmap;
+	int ret;
+
+	regmap = dev_get_regmap(dev->parent, NULL);
+	if (!regmap)
+		return -ENODEV;
+
+	ret = devm_register_power_off_handler(dev, &spacemit_p1_pwroff_handler,
+					      regmap);
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "Failed to register power off handler\n");
+
+	ret = devm_register_restart_handler(dev, spacemit_p1_restart_handler,
+					    regmap);
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "Failed to register restart handler\n");
+
+	return 0;
+}
+
+static const struct platform_device_id spacemit_p1_reboot_id_table[] = {
+	{ "spacemit-p1-reboot", },
+	{ /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(platform, spacemit_p1_reboot_id_table);
+
+static struct platform_driver spacemit_p1_reboot_driver = {
+	.driver = {
+		.name = "spacemit-p1-reboot",
+	},
+	.probe = spacemit_p1_reboot_probe,
+	.id_table = spacemit_p1_reboot_id_table,
+};
+module_platform_driver(spacemit_p1_reboot_driver);
+
+MODULE_DESCRIPTION("SpacemiT P1 reboot/poweroff driver");
+MODULE_LICENSE("GPL");
-- 
2.47.2

[PATCH v3 2/2] mfd: simple-mfd-i2c: add a reboot cell for the SpacemiT P1 chip

[Aurelien Jarno]

Add a "spacemit-p1-reboot" cell for the SpacemiT P1 chip.

Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
---
v3: no changes

 drivers/mfd/simple-mfd-i2c.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/mfd/simple-mfd-i2c.c b/drivers/mfd/simple-mfd-i2c.c
index 0a607a1e3ca1d..542d378cdcd1f 100644
--- a/drivers/mfd/simple-mfd-i2c.c
+++ b/drivers/mfd/simple-mfd-i2c.c
@@ -99,6 +99,7 @@ static const struct regmap_config spacemit_p1_regmap_config = {
 };
 
 static const struct mfd_cell spacemit_p1_cells[] = {
+	{ .name = "spacemit-p1-reboot", },
 	{ .name = "spacemit-p1-regulator", },
 	{ .name = "spacemit-p1-rtc", },
 };
-- 
2.47.2

Re: [PATCH v3 1/2] driver: reset: spacemit-p1: add driver for poweroff/reboot

[Yixun Lan]

Hi Aurelien,

On 22:11 Tue 21 Oct     , Aurelien Jarno wrote:
> This driver implements poweroff/reboot support for the SpacemiT P1 PMIC
> chip, which is commonly paired with the SpacemiT K1 SoC.
> 
> The SpacemiT P1 support is implemented as a MFD driver, so the access is
> done directly through the regmap interface. Reboot or poweroff is
> triggered by setting a specific bit in a control register, which is
> automatically cleared by the hardware afterwards.
> 
> Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
> Reviewed-by: Yixun Lan <dlan@gentoo.org>
> ---
> v3:
>  - Allow building as a module
>  - Remove outdated Acked-by and Tested-by
>  - Collect Reviewed-by
> 
>  drivers/power/reset/Kconfig              |  9 +++
>  drivers/power/reset/Makefile             |  1 +
>  drivers/power/reset/spacemit-p1-reboot.c | 88 ++++++++++++++++++++++++
>  3 files changed, 98 insertions(+)
>  create mode 100644 drivers/power/reset/spacemit-p1-reboot.c
> 
> diff --git a/drivers/power/reset/Kconfig b/drivers/power/reset/Kconfig
> index 8248895ca9038..6577d73edbda4 100644
> --- a/drivers/power/reset/Kconfig
> +++ b/drivers/power/reset/Kconfig
> @@ -283,6 +283,15 @@ config POWER_RESET_KEYSTONE
>  	help
>  	  Reboot support for the KEYSTONE SoCs.
>  
> +config POWER_RESET_SPACEMIT_P1
> +	tristate "SpacemiT P1 poweroff and reset driver"
> +	depends on ARCH_SPACEMIT || COMPILE_TEST
..
> +	select MFD_SPACEMIT_P1
I'd suggest to use "depends on" instead of "select", the reason is that
using "select" will sometimes ignore the dependency, considering
the reset driver here is tightly coupled with P1, so I think it's 
reasonable to switch to use "depends on", also refer below link

https://lxr.linux.no/#linux+v6.7.1/Documentation/kbuild/kconfig-language.rst#L144

        select should be used with care. select will force
        a symbol to a value without visiting the dependencies.
        By abusing select you are able to select a symbol FOO even
        if FOO depends on BAR that is not set.
        In general use select only for non-visible symbols
        (no prompts anywhere) and for symbols with no dependencies.
        That will limit the usefulness but on the other hand avoid
        the illegal configurations all over.

> +	default m
> +	help
> +	  This driver supports power-off and reset operations for the SpacemiT
> +	  P1 PMIC.
> +
>  config POWER_RESET_SYSCON
>  	bool "Generic SYSCON regmap reset driver"
>  	depends on OF
> diff --git a/drivers/power/reset/Makefile b/drivers/power/reset/Makefile
> index 51da87e05ce76..0e4ae6f6b5c55 100644
> --- a/drivers/power/reset/Makefile
> +++ b/drivers/power/reset/Makefile
> @@ -24,6 +24,7 @@ obj-$(CONFIG_POWER_RESET_LTC2952) += ltc2952-poweroff.o
>  obj-$(CONFIG_POWER_RESET_QNAP) += qnap-poweroff.o
>  obj-$(CONFIG_POWER_RESET_REGULATOR) += regulator-poweroff.o
>  obj-$(CONFIG_POWER_RESET_RESTART) += restart-poweroff.o
> +obj-$(CONFIG_POWER_RESET_SPACEMIT_P1) += spacemit-p1-reboot.o
>  obj-$(CONFIG_POWER_RESET_ST) += st-poweroff.o
>  obj-$(CONFIG_POWER_RESET_TH1520_AON) += th1520-aon-reboot.o
>  obj-$(CONFIG_POWER_RESET_TORADEX_EC) += tdx-ec-poweroff.o
> diff --git a/drivers/power/reset/spacemit-p1-reboot.c b/drivers/power/reset/spacemit-p1-reboot.c
> new file mode 100644
> index 0000000000000..9ec3d1fff8f3d
> --- /dev/null
> +++ b/drivers/power/reset/spacemit-p1-reboot.c
> @@ -0,0 +1,88 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2025 by Aurelien Jarno
> + */
> +
> +#include <linux/bits.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/reboot.h>
> +
> +/* Power Control Register 2 */
> +#define PWR_CTRL2		0x7e
> +#define PWR_CTRL2_SHUTDOWN	BIT(2)	/* Shutdown request */
> +#define PWR_CTRL2_RST		BIT(1)	/* Reset request */
> +
> +static int spacemit_p1_pwroff_handler(struct sys_off_data *data)
> +{
> +	struct regmap *regmap = data->cb_data;
> +	int ret;
> +
> +	/* Put the PMIC into shutdown state */
> +	ret = regmap_set_bits(regmap, PWR_CTRL2, PWR_CTRL2_SHUTDOWN);
> +	if (ret) {
> +		dev_err(data->dev, "shutdown failed: %d\n", ret);
> +		return notifier_from_errno(ret);
> +	}
> +
> +	return NOTIFY_DONE;
> +}
> +
> +static int spacemit_p1_restart_handler(struct sys_off_data *data)
> +{
> +	struct regmap *regmap = data->cb_data;
> +	int ret;
> +
> +	/* Put the PMIC into reset state */
> +	ret = regmap_set_bits(regmap, PWR_CTRL2, PWR_CTRL2_RST);
> +	if (ret) {
> +		dev_err(data->dev, "restart failed: %d\n", ret);
> +		return notifier_from_errno(ret);
> +	}
> +
> +	return NOTIFY_DONE;
> +}
> +
> +static int spacemit_p1_reboot_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct regmap *regmap;
> +	int ret;
> +
> +	regmap = dev_get_regmap(dev->parent, NULL);
> +	if (!regmap)
> +		return -ENODEV;
> +
> +	ret = devm_register_power_off_handler(dev, &spacemit_p1_pwroff_handler,
> +					      regmap);
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "Failed to register power off handler\n");
> +
> +	ret = devm_register_restart_handler(dev, spacemit_p1_restart_handler,
> +					    regmap);
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "Failed to register restart handler\n");
> +
> +	return 0;
> +}
> +
> +static const struct platform_device_id spacemit_p1_reboot_id_table[] = {
> +	{ "spacemit-p1-reboot", },
> +	{ /* sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(platform, spacemit_p1_reboot_id_table);
> +
> +static struct platform_driver spacemit_p1_reboot_driver = {
> +	.driver = {
> +		.name = "spacemit-p1-reboot",
> +	},
> +	.probe = spacemit_p1_reboot_probe,
> +	.id_table = spacemit_p1_reboot_id_table,
> +};
> +module_platform_driver(spacemit_p1_reboot_driver);
> +
> +MODULE_DESCRIPTION("SpacemiT P1 reboot/poweroff driver");
> +MODULE_LICENSE("GPL");
> -- 
> 2.47.2
> 

-- 
Yixun Lan (dlan)

Re: [PATCH v3 1/2] driver: reset: spacemit-p1: add driver for poweroff/reboot

[Aurelien Jarno]

On 2025-10-22 08:48, Yixun Lan wrote:
> Hi Aurelien,
> 
> On 22:11 Tue 21 Oct     , Aurelien Jarno wrote:
> > This driver implements poweroff/reboot support for the SpacemiT P1 PMIC
> > chip, which is commonly paired with the SpacemiT K1 SoC.
> > 
> > The SpacemiT P1 support is implemented as a MFD driver, so the access is
> > done directly through the regmap interface. Reboot or poweroff is
> > triggered by setting a specific bit in a control register, which is
> > automatically cleared by the hardware afterwards.
> > 
> > Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
> > Reviewed-by: Yixun Lan <dlan@gentoo.org>
> > ---
> > v3:
> >  - Allow building as a module
> >  - Remove outdated Acked-by and Tested-by
> >  - Collect Reviewed-by
> > 
> >  drivers/power/reset/Kconfig              |  9 +++
> >  drivers/power/reset/Makefile             |  1 +
> >  drivers/power/reset/spacemit-p1-reboot.c | 88 ++++++++++++++++++++++++
> >  3 files changed, 98 insertions(+)
> >  create mode 100644 drivers/power/reset/spacemit-p1-reboot.c
> > 
> > diff --git a/drivers/power/reset/Kconfig b/drivers/power/reset/Kconfig
> > index 8248895ca9038..6577d73edbda4 100644
> > --- a/drivers/power/reset/Kconfig
> > +++ b/drivers/power/reset/Kconfig
> > @@ -283,6 +283,15 @@ config POWER_RESET_KEYSTONE
> >  	help
> >  	  Reboot support for the KEYSTONE SoCs.
> >  
> > +config POWER_RESET_SPACEMIT_P1
> > +	tristate "SpacemiT P1 poweroff and reset driver"
> > +	depends on ARCH_SPACEMIT || COMPILE_TEST
> ..
> > +	select MFD_SPACEMIT_P1
> I'd suggest to use "depends on" instead of "select", the reason is that
> using "select" will sometimes ignore the dependency, considering
> the reset driver here is tightly coupled with P1, so I think it's 
> reasonable to switch to use "depends on", also refer below link
> 
> https://lxr.linux.no/#linux+v6.7.1/Documentation/kbuild/kconfig-language.rst#L144
> 
>         select should be used with care. select will force
>         a symbol to a value without visiting the dependencies.
>         By abusing select you are able to select a symbol FOO even
>         if FOO depends on BAR that is not set.
>         In general use select only for non-visible symbols
>         (no prompts anywhere) and for symbols with no dependencies.
>         That will limit the usefulness but on the other hand avoid
>         the illegal configurations all over.

Thanks for the pointer, I'll fix that in the next version. I used 
REGULATOR_SPACEMIT_P1 and RTC_DRV_SPACEMIT_P1 as examples, they'll also 
need to be fixed.

Note also that without the select, a default value has to be added to 
MFD_SPACEMIT_P1, otherwise this makes the default values on the 
regulator, rtc and reboot drivers useless.

-- 
Aurelien Jarno                          GPG: 4096R/1DDD8C9B
aurelien@aurel32.net                     http://aurel32.net

Re: [PATCH v3 1/2] driver: reset: spacemit-p1: add driver for poweroff/reboot

[Troy Mitchell]

> > > diff --git a/drivers/power/reset/Kconfig b/drivers/power/reset/Kconfig
> > > index 8248895ca9038..6577d73edbda4 100644
> > > --- a/drivers/power/reset/Kconfig
> > > +++ b/drivers/power/reset/Kconfig
> > > @@ -283,6 +283,15 @@ config POWER_RESET_KEYSTONE
> > >  	help
> > >  	  Reboot support for the KEYSTONE SoCs.
> > >  
> > > +config POWER_RESET_SPACEMIT_P1
> > > +	tristate "SpacemiT P1 poweroff and reset driver"
> > > +	depends on ARCH_SPACEMIT || COMPILE_TEST
> > ..
> > > +	select MFD_SPACEMIT_P1
> > I'd suggest to use "depends on" instead of "select", the reason is that
> > using "select" will sometimes ignore the dependency, considering
> > the reset driver here is tightly coupled with P1, so I think it's 
> > reasonable to switch to use "depends on", also refer below link
> > 
> > https://lxr.linux.no/#linux+v6.7.1/Documentation/kbuild/kconfig-language.rst#L144
> > 
> >         select should be used with care. select will force
> >         a symbol to a value without visiting the dependencies.
> >         By abusing select you are able to select a symbol FOO even
> >         if FOO depends on BAR that is not set.
> >         In general use select only for non-visible symbols
> >         (no prompts anywhere) and for symbols with no dependencies.
> >         That will limit the usefulness but on the other hand avoid
> >         the illegal configurations all over.
> 
> Thanks for the pointer, I'll fix that in the next version. I used 
> REGULATOR_SPACEMIT_P1 and RTC_DRV_SPACEMIT_P1 as examples, they'll also 
> need to be fixed.
Yes, I have said here[1].
Do you want to fix that? If you don't have time, I can do it.

> 
> Note also that without the select, a default value has to be added to 
> MFD_SPACEMIT_P1. 
Yes, I will add it in my patch. Thanks.

                  - Troy

Link: https://lore.kernel.org/all/6DB8C5F20B3FAC2E+aPhfoRXlJtJymlB5@kernel.org/ [1]

> otherwise this makes the default values on the 
> regulator, rtc and reboot drivers useless.
> 
> -- 
> Aurelien Jarno                          GPG: 4096R/1DDD8C9B
> aurelien@aurel32.net                     http://aurel32.net
> 

Re: [PATCH v3 1/2] driver: reset: spacemit-p1: add driver for poweroff/reboot

[Aurelien Jarno]

On 2025-10-22 13:42, Troy Mitchell wrote:
> > > > diff --git a/drivers/power/reset/Kconfig b/drivers/power/reset/Kconfig
> > > > index 8248895ca9038..6577d73edbda4 100644
> > > > --- a/drivers/power/reset/Kconfig
> > > > +++ b/drivers/power/reset/Kconfig
> > > > @@ -283,6 +283,15 @@ config POWER_RESET_KEYSTONE
> > > >  	help
> > > >  	  Reboot support for the KEYSTONE SoCs.
> > > >  
> > > > +config POWER_RESET_SPACEMIT_P1
> > > > +	tristate "SpacemiT P1 poweroff and reset driver"
> > > > +	depends on ARCH_SPACEMIT || COMPILE_TEST
> > > ..
> > > > +	select MFD_SPACEMIT_P1
> > > I'd suggest to use "depends on" instead of "select", the reason is that
> > > using "select" will sometimes ignore the dependency, considering
> > > the reset driver here is tightly coupled with P1, so I think it's 
> > > reasonable to switch to use "depends on", also refer below link
> > > 
> > > https://lxr.linux.no/#linux+v6.7.1/Documentation/kbuild/kconfig-language.rst#L144
> > > 
> > >         select should be used with care. select will force
> > >         a symbol to a value without visiting the dependencies.
> > >         By abusing select you are able to select a symbol FOO even
> > >         if FOO depends on BAR that is not set.
> > >         In general use select only for non-visible symbols
> > >         (no prompts anywhere) and for symbols with no dependencies.
> > >         That will limit the usefulness but on the other hand avoid
> > >         the illegal configurations all over.
> > 
> > Thanks for the pointer, I'll fix that in the next version. I used 
> > REGULATOR_SPACEMIT_P1 and RTC_DRV_SPACEMIT_P1 as examples, they'll also 
> > need to be fixed.
> Yes, I have said here[1].
> Do you want to fix that? If you don't have time, I can do it.

For the reboot patch series, I'll do that in the v4, but I would 
appreciate if you can do it for the PMIC and RTC drivers. I guess it can 
be a patch series with the first patch being the one you already posted.

> > Note also that without the select, a default value has to be added to 
> > MFD_SPACEMIT_P1. 
> Yes, I will add it in my patch. Thanks.

Thanks.

Regards
Aurelien

-- 
Aurelien Jarno                          GPG: 4096R/1DDD8C9B
aurelien@aurel32.net                     http://aurel32.net

Re: [PATCH v3 1/2] driver: reset: spacemit-p1: add driver for poweroff/reboot

[Troy Mitchell]

On Wed, Oct 22, 2025 at 06:48:45PM +0200, Aurelien Jarno wrote:
> On 2025-10-22 13:42, Troy Mitchell wrote:
> > > > > diff --git a/drivers/power/reset/Kconfig b/drivers/power/reset/Kconfig
> > > > > index 8248895ca9038..6577d73edbda4 100644
> > > > > --- a/drivers/power/reset/Kconfig
> > > > > +++ b/drivers/power/reset/Kconfig
> > > > > @@ -283,6 +283,15 @@ config POWER_RESET_KEYSTONE
> > > > >  	help
> > > > >  	  Reboot support for the KEYSTONE SoCs.
> > > > >  
> > > > > +config POWER_RESET_SPACEMIT_P1
> > > > > +	tristate "SpacemiT P1 poweroff and reset driver"
> > > > > +	depends on ARCH_SPACEMIT || COMPILE_TEST
> > > > ..
> > > > > +	select MFD_SPACEMIT_P1
> > > > I'd suggest to use "depends on" instead of "select", the reason is that
> > > > using "select" will sometimes ignore the dependency, considering
> > > > the reset driver here is tightly coupled with P1, so I think it's 
> > > > reasonable to switch to use "depends on", also refer below link
> > > > 
> > > > https://lxr.linux.no/#linux+v6.7.1/Documentation/kbuild/kconfig-language.rst#L144
> > > > 
> > > >         select should be used with care. select will force
> > > >         a symbol to a value without visiting the dependencies.
> > > >         By abusing select you are able to select a symbol FOO even
> > > >         if FOO depends on BAR that is not set.
> > > >         In general use select only for non-visible symbols
> > > >         (no prompts anywhere) and for symbols with no dependencies.
> > > >         That will limit the usefulness but on the other hand avoid
> > > >         the illegal configurations all over.
> > > 
> > > Thanks for the pointer, I'll fix that in the next version. I used 
> > > REGULATOR_SPACEMIT_P1 and RTC_DRV_SPACEMIT_P1 as examples, they'll also 
> > > need to be fixed.
> > Yes, I have said here[1].
> > Do you want to fix that? If you don't have time, I can do it.
> 
> For the reboot patch series, I'll do that in the v4, 
makes sense.

> but I would 
> appreciate if you can do it for the PMIC and RTC drivers. I guess it can 
> be a patch series with the first patch being the one you already posted.
Thanks for your suggestion! I will.

                    - Troy
> 
> > > Note also that without the select, a default value has to be added to 
> > > MFD_SPACEMIT_P1. 
> > Yes, I will add it in my patch. Thanks.
> 
> Thanks.
> 
> Regards
> Aurelien
> 
> -- 
> Aurelien Jarno                          GPG: 4096R/1DDD8C9B
> aurelien@aurel32.net                     http://aurel32.net
>