[please always add the maintainer to Cc or To, in this case sre@kernel.org]

Hi,

On Sat, Apr 25, 2020 at 03:00:43AM +0200, Daniel González Cabanelas wrote:
> Some Buffalo LinkStations perform the power off operation, at restart
> time, depending on the state of an output pin (LED) at the ethernet PHY.
> 
> The driver is required by the Buffalo LinkStation LS421DE (ARM MVEBU),
> and other models. Without it, the board remains forever halted if a 
> power off command is executed, unless the PSU is disconnected and
> connected again.
> 
> Signed-off-by: Daniel González Cabanelas <dgcbueu@gmail.com>
> Reviewed-by: Álvaro Fernández Rojas <noltari@gmail.com>
> ---

Thanks for the patch. Looks like that hardware was designed by
creative hardware engineers trying to impress software engineers
with weird designs :)

>  drivers/power/reset/Kconfig                |  10 ++
>  drivers/power/reset/Makefile               |   1 +
>  drivers/power/reset/linkstation-poweroff.c | 117 +++++++++++++++++++++
>  3 files changed, 128 insertions(+)
>  create mode 100644 drivers/power/reset/linkstation-poweroff.c
> 
> diff --git a/drivers/power/reset/Kconfig b/drivers/power/reset/Kconfig
> index 8903803020..de948835f8 100644
> --- a/drivers/power/reset/Kconfig
> +++ b/drivers/power/reset/Kconfig
> @@ -203,6 +203,16 @@ config POWER_RESET_KEYSTONE
>  	help
>  	  Reboot support for the KEYSTONE SoCs.
>  
> +config POWER_RESET_LINKSTATION
> +	tristate "Buffalo LinkStation power-off driver"
> +	depends on ARCH_MVEBU || COMPILE_TEST
> +	depends on OF_MDIO && PHYLIB
> +	help
> +	  Some Buffalo LinkStations use a LED output at the ethernet PHY
> +	  to inform the board that a power off operation must be performed 
> +	  after restarting. This driver sets the LED to the proper state
> +	  for restarting or powering off.
> +
>  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 da37f8b851..a8c83d2470 100644
> --- a/drivers/power/reset/Makefile
> +++ b/drivers/power/reset/Makefile
> @@ -10,6 +10,7 @@ obj-$(CONFIG_POWER_RESET_GEMINI_POWEROFF) += gemini-poweroff.o
>  obj-$(CONFIG_POWER_RESET_GPIO) += gpio-poweroff.o
>  obj-$(CONFIG_POWER_RESET_GPIO_RESTART) += gpio-restart.o
>  obj-$(CONFIG_POWER_RESET_HISI) += hisi-reboot.o
> +obj-${CONFIG_POWER_RESET_LINKSTATION} += linkstation-poweroff.o
>  obj-$(CONFIG_POWER_RESET_MSM) += msm-poweroff.o
>  obj-$(CONFIG_POWER_RESET_MT6323) += mt6323-poweroff.o
>  obj-$(CONFIG_POWER_RESET_QCOM_PON) += qcom-pon.o
> diff --git a/drivers/power/reset/linkstation-poweroff.c b/drivers/power/reset/linkstation-poweroff.c
> new file mode 100644
> index 0000000000..b5a4cc2c2b
> --- /dev/null
> +++ b/drivers/power/reset/linkstation-poweroff.c
> @@ -0,0 +1,117 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * LinkStation power off restart driver
> + * Copyright (C) 2020 Daniel González Cabanelas <dgcbueu@gmail.com>
> + */
> +
> +#include <linux/module.h>
> +#include <linux/notifier.h>
> +#include <linux/of.h>
> +#include <linux/of_mdio.h>
> +#include <linux/of_platform.h>
> +#include <linux/reboot.h>
> +#include <linux/phy.h>
> +
> +#define MII_MARVELL_LED_PAGE	3
> +#define MII_PHY_LED_CTRL	16
> +
> +#define LED2_OFF		(0x8 << 8)
> +#define LED2_ON			(0x9 << 8)
> +#define LEDMASK			GENMASK(11,8)
> +
> +static struct phy_device *phydev;
> +
> +static void mvphy_reg_led(u16 data)
> +{
> +	int rc;
> +	rc = phy_modify_paged(phydev, MII_MARVELL_LED_PAGE,
> +			      MII_PHY_LED_CTRL, LEDMASK, data);
> +	if (rc < 0)
> +		dev_err(&phydev->mdio.dev,
> +			"LED2 write register failed, %d\n", rc);
> +}
> +
> +static int linkstation_reboot_notifier(struct notifier_block *nb,
> +				       unsigned long action, void *unused)
> +{
> +	if (action == SYS_RESTART)
> +		mvphy_reg_led(LED2_ON);
> +
> +	return NOTIFY_DONE;
> +}
> +
> +static struct notifier_block linkstation_reboot_nb = {
> +	.notifier_call = linkstation_reboot_notifier,
> +};
> +
> +static void linkstation_poweroff(void)
> +{
> +	unregister_reboot_notifier(&linkstation_reboot_nb);
> +	mvphy_reg_led(LED2_OFF);
> +
> +	kernel_restart("Power off");
> +}
> +
> +static int linkstation_poweroff_probe(struct platform_device *pdev)
> +{
> +	struct mii_bus *bus;
> +	struct device_node *dn;
> +
> +	dn = of_parse_phandle(pdev->dev.of_node, "phy-handle,led", 0);
> +
> +	if (dn) {
> +		phydev = of_phy_find_device(dn);
> +		of_node_put(dn);
> +	} else {
> +		dn = of_find_node_by_name(NULL, "mdio");
> +		if (!dn)
> +			return -ENODEV;
> +
> +		bus = of_mdio_find_bus(dn);
> +		of_node_put(dn);
> +		if (!bus)
> +			return -EPROBE_DEFER;
> +
> +		phydev = phy_find_first(bus);

do we need this code, or can we just make phy-handle,led mandatory
and error out otherwise?

> +	}
> +
> +	if (!phydev)
> +		return -ENODEV;

Should this return -EPROBE_DEFER?

> +	register_reboot_notifier(&linkstation_reboot_nb);
> +	pm_power_off = linkstation_poweroff;
> +
> +	dev_info(&pdev->dev, "PHY [%s]\n", phydev_name(phydev));

I don't think this deserves more than dev_dbg()

> +	return 0;
> +}
> +
> +static int linkstation_poweroff_remove(struct platform_device *pdev)
> +{
> +	pm_power_off = NULL;
> +	unregister_reboot_notifier(&linkstation_reboot_nb);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id ls_poweroff_of_match[] = {
> +	{ .compatible = "linkstation,power-off", },
> +	{ },
> +};
> +
> +MODULE_DEVICE_TABLE(of, ls_poweroff_of_match);
> +
> +static struct platform_driver linkstation_poweroff_driver = {
> +	.probe = linkstation_poweroff_probe,
> +	.remove = linkstation_poweroff_remove,
> +	.driver = {
> +		.name = "linkstation_power_off",
> +		.of_match_table = ls_poweroff_of_match,
> +	},
> +};
> +
> +module_platform_driver(linkstation_poweroff_driver);
> +
> +MODULE_AUTHOR("Daniel González Cabanelas <dgcbueu@gmail.com>");
> +MODULE_DESCRIPTION("LinkStation power off driver");
> +MODULE_LICENSE("GPL v2");

Otherwise looks good to me,

-- Sebastian