Re: [PATCH v5 07/10] power: reset: macsmc-reboot: Add driver for rebooting via Apple SMC

["Sven Peter" <sven@svenpeter.dev>]

Hi Sebastian,

thanks for the review!

On Mon, May 12, 2025, at 00:16, Sebastian Reichel wrote:
> Hi,
>
> On Sun, May 11, 2025 at 08:18:42AM +0000, Sven Peter via B4 Relay wrote:
>> From: Hector Martin <marcan@marcan.st>
>> 
>> This driver implements the reboot/shutdown support exposed by the SMC
>> on Apple Silicon machines, such as Apple M1 Macs.
>> 
>> Signed-off-by: Hector Martin <marcan@marcan.st>
>> Signed-off-by: Sven Peter <sven@svenpeter.dev>
>> ---
>>  MAINTAINERS                         |   1 +
>>  drivers/power/reset/Kconfig         |  11 ++
>>  drivers/power/reset/Makefile        |   1 +
>>  drivers/power/reset/macsmc-reboot.c | 363 ++++++++++++++++++++++++++++++++++++
>>  4 files changed, 376 insertions(+)
>> 
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index fa3a5f9ee40446bcc725c9eac2a36651e6bc7553..84f7a730eb2260b7c1e0487d18c8eb3de82f5206 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -2303,6 +2303,7 @@ F:	drivers/mfd/macsmc.c
>>  F:	drivers/nvme/host/apple.c
>>  F:	drivers/nvmem/apple-efuses.c
>>  F:	drivers/pinctrl/pinctrl-apple-gpio.c
>> +F:	drivers/power/reset/macsmc-reboot.c
>>  F:	drivers/pwm/pwm-apple.c
>>  F:	drivers/soc/apple/*
>>  F:	drivers/spi/spi-apple.c
>> diff --git a/drivers/power/reset/Kconfig b/drivers/power/reset/Kconfig
>> index 60bf0ca64cf395cd18238fc626611c74d29844ee..6e8dfff64fdc001d09b6c00630cd8b7e2fafdd8e 100644
>> --- a/drivers/power/reset/Kconfig
>> +++ b/drivers/power/reset/Kconfig
>> @@ -128,6 +128,17 @@ config POWER_RESET_LINKSTATION
>>  
>>  	  Say Y here if you have a Buffalo LinkStation LS421D/E.
>>  
>> +config POWER_RESET_MACSMC
>> +	tristate "Apple SMC reset/power-off driver"
>> +	depends on ARCH_APPLE || COMPILE_TEST
>> +	depends on MFD_MACSMC
>> +	depends on OF
>
> This can also be 'OF || COMPILE_TEST'. But I would expect this
> driver to just have 'depends on MFD_MACSMC' and then manage the
> checks for ARCH_APPLE and OF in the MFD Kconfig.

Makes sense, it'll just depend on MFD_MACSMC then and I'll move the ARCH_APPLE,
OF, etc. depends to the MFD Kconfig.

>
[...]
>> +#include <linux/delay.h>
>> +#include <linux/mfd/core.h>
>> +#include <linux/mfd/macsmc.h>
>> +#include <linux/module.h>
>> +#include <linux/nvmem-consumer.h>
>> +#include <linux/of.h>
>
> Once of_get_child_by_name() is no lnger used the correct include for
> the remaining 'struct of_device_id' is <linux/mod_devicetable.h>
> instead of <linux/of.h>.

Fixed.

>
[...]
>> +
>> +	pdev->dev.of_node = of_get_child_by_name(pdev->dev.parent->of_node, "reboot");
>
> Why is this needed? The of_node should already be set correctly when
> probed via the of_match_table.

Leftovers from a previous version that didn't use of_match_table.
I'll remove it.

>
[...]
>> +
>> +	if (device_create_file(&pdev->dev, &dev_attr_ac_power_mode))
>> +		dev_warn(&pdev->dev, "could not create sysfs file\n");
>
> custom sysfs files must be documented in Documentation/ABI.

This sysfs file allows to configure if the system reboots automatically after
power loss. But now that I'm looking at this again I'm not sure this driver
is even the proper place for this (the nvmem cell is kinda unrelated to SMC)
or if we need this at all in the kernel since the nvmem cell is already
exposed to sysfs just with a less convenient interface at 
/sys/bus/nvmem/devices/spmi_nvmem0/cells/pm-setting@d001,0.

I'm going to drop it for now and revisit this later.


>
>> +
[...]
>> +MODULE_LICENSE("Dual MIT/GPL");
>> +MODULE_DESCRIPTION("Apple SMC reboot/poweroff driver");
>> +MODULE_AUTHOR("Hector Martin <marcan@marcan.st>");
>> +MODULE_ALIAS("platform:macsmc-reboot");
>
> Why is the MODULE_ALIAS needed?

No idea, my best guess is it was copy/pasted without a good reason.
I'll drop it.


Thanks,


Sven