Re: [PATCH v6 2/2] soc: loongson2_pm: add power management support

["Arnd Bergmann" <arnd@arndb.de>]

On Thu, Aug 3, 2023, at 08:37, Yinbo Zhu wrote:
> The Loongson-2's power management controller was ACPI, supports ACPI
> S2Idle (Suspend To Idle), ACPI S3 (Suspend To RAM), ACPI S4 (Suspend To
> Disk), ACPI S5 (Soft Shutdown) and supports multiple wake-up methods
> (USB, GMAC, PWRBTN, etc.). This driver was to add power management
> controller support that base on dts for Loongson-2 series SoCs.
>
> Co-developed-by: Liu Yun <liuyun@loongson.cn>
> Signed-off-by: Liu Yun <liuyun@loongson.cn>
> Co-developed-by: Liu Peibao <liupeibao@loongson.cn>
> Signed-off-by: Liu Peibao <liupeibao@loongson.cn>
> Cc: soc@kernel.org
> Cc: Ulf Hansson <ulf.hansson@linaro.org>
> Signed-off-by: Yinbo Zhu <zhuyinbo@loongson.cn>

I'm still waiting for Ulf to take a look here to see whether
this should be in drivers/genpd instead, but he might still
be on vacation.

A few minor comments from me in the meantime:

> +#define loongson2_pm_readw(reg)		readw(loongson2_pm.base + reg)
> +#define loongson2_pm_readl(reg)		readl(loongson2_pm.base + reg)
> +#define loongson2_pm_writew(val, reg)	writew(val, loongson2_pm.base + 
> reg)
> +#define loongson2_pm_writel(val, reg)	writel(val, loongson2_pm.base + 
> reg)

I would prefer these to be 'static inline' functions rather than
macros, or you can just open-code them, as each macro is only
used once at the moment.

> +static irqreturn_t loongson2_pm_irq_handler(int irq, void *dev_id)
> +{
> +	u16 status = loongson2_pm_readw(LOONGSON2_PM1_STS_REG);
> +
> +	if (!loongson2_pm.suspended && (status & LOONGSON2_PM1_PWRBTN_STS)) {
> +		pr_info("Power Button pressed...\n");

The message is probably more appropriate as a pr_debug() than
pr_info().

> +static int __maybe_unused loongson2_pm_suspend(struct device *dev)
> +{
> +	loongson2_pm.suspended = true;
> +
> +	return 0;
> +}
> +
> +static int __maybe_unused loongson2_pm_resume(struct device *dev)
> +{
> +	loongson2_pm.suspended = false;
> +
> +	return 0;
> +}
> +static SIMPLE_DEV_PM_OPS(loongson2_pm_ops, loongson2_pm_suspend, 
> loongson2_pm_resume);

Please change this to DEFINE_SIMPLE_DEV_PM_OPS() and remove the
__maybe_unused, this is what all drivers should have these days.

> +
> +static int loongson2_pm_probe(struct platform_device *pdev)
> +{
> +	int irq, retval;
> +	u64 suspend_addr;
> +	struct device *dev = &pdev->dev;
> +
> +	loongson2_pm.base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(loongson2_pm.base))
> +		return PTR_ERR(loongson2_pm.base);
> +
> +	irq = platform_get_irq(pdev, 0);
> +	if (irq < 0)
> +		return irq;
> +
> +	if (!device_property_read_u64(dev, "loongson,suspend-address", 
> &suspend_addr))
> +		loongson_sysconf.suspend_addr = (u64)phys_to_virt(suspend_addr);
> +	else

Having a custom "loongson,suspend-address" property here feels wrong
to me. Can't this be moved into the "regs" property that holds
the other mmio registers?

    Arnd