Re: [PATCH] drivers: pmic: Add sysreset driver to da9063 pmic device

[Heinrich Schuchardt <heinrich.schuchardt@canonical.com>]

On 9/20/21 5:48 PM, Alexandre Ghiti wrote:
> This pmic device is present on the SiFive Unmatched board and this
> new driver adds the possibility to reset it.
> 
> Signed-off-by: Alexandre Ghiti <alexandre.ghiti@canonical.com>
> ---
>   configs/sifive_unmatched_defconfig |  2 ++
>   drivers/power/pmic/da9063.c        | 49 ++++++++++++++++++++++++++++++
>   2 files changed, 51 insertions(+)
> 
> diff --git a/configs/sifive_unmatched_defconfig b/configs/sifive_unmatched_defconfig
> index 978818b688..9ab058be39 100644
> --- a/configs/sifive_unmatched_defconfig
> +++ b/configs/sifive_unmatched_defconfig
> @@ -43,3 +43,5 @@ CONFIG_DM_USB=y
>   CONFIG_USB_XHCI_HCD=y
>   CONFIG_USB_XHCI_PCI=y
>   CONFIG_BOARD_EARLY_INIT_F=y
> +CONFIG_DM_PMIC=y
> +CONFIG_DM_PMIC_DA9063=y
> diff --git a/drivers/power/pmic/da9063.c b/drivers/power/pmic/da9063.c
> index 25101d18f7..b04879d9c5 100644
> --- a/drivers/power/pmic/da9063.c
> +++ b/drivers/power/pmic/da9063.c
> @@ -10,6 +10,7 @@
>   #include <dm.h>
>   #include <i2c.h>
>   #include <log.h>
> +#include <dm/lists.h>
>   #include <power/pmic.h>
>   #include <power/regulator.h>
>   #include <power/da9063_pmic.h>
> @@ -87,6 +88,7 @@ static int da9063_bind(struct udevice *dev)
>   {
>   	ofnode regulators_node;
>   	int children;
> +	int ret;
>   
>   	regulators_node = dev_read_subnode(dev, "regulators");
>   	if (!ofnode_valid(regulators_node)) {
> @@ -101,6 +103,14 @@ static int da9063_bind(struct udevice *dev)
>   	if (!children)
>   		debug("%s: %s - no child found\n", __func__, dev->name);
>   
> +	if (CONFIG_IS_ENABLED(SYSRESET)) {

Thank you for addressing the missing reset driver for the SiFive 
Unmatched board.

I imagine some existing or future boards using the DA9063 will have a 
GPIO for system reset. Should all boards having a DA9063 PMIC implement 
system reset via the power management IC?

We could instead use the devicetree to identify if a board shall use the 
DA9063 for system reset.

> +		ret = device_bind_driver(dev, "da9063-sysreset",
> +					 "da9063-sysreset", NULL);
> +		if (ret)
> +			debug("%s: %s - failed to bind sysreset driver\n",
> +			      __func__, dev->name);
> +	}
> +
>   	/* Always return success for this device */
>   	return 0;
>   }
> @@ -129,3 +139,42 @@ U_BOOT_DRIVER(pmic_da9063) = {
>   	.probe = da9063_probe,
>   	.ops = &da9063_ops,
>   };
> +
> +#ifdef CONFIG_SYSRESET

The linker will remove functions that are not used automatically. We 
have tended to not enclose functions in #ifdef but instead allow the 
compiler to check the code.

> +#include <sysreset.h>

Please, keep includes at the top of the code.

> +

Even though this is a static function I would add a Sphinx style comment 
here. Cf. 
https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html#function-documentation

Best regards

Heinrich

> +static int da9063_sysreset_request(struct udevice *dev, enum sysreset_t type)
> +{
> +	struct udevice *pmic_dev = dev->parent;
> +	uint ret;
> +
> +	if (type != SYSRESET_WARM && type != SYSRESET_COLD)
> +		return -EPROTONOSUPPORT;
> +
> +	ret = pmic_reg_write(pmic_dev, DA9063_REG_PAGE_CON, 0x00);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* Sets the WAKE_UP bit */
> +	ret = pmic_reg_write(pmic_dev, DA9063_REG_CONTROL_F, 0x04);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* Powerdown! */
> +	ret = pmic_reg_write(pmic_dev, DA9063_REG_CONTROL_A, 0x68);
> +	if (ret < 0)
> +		return ret;
> +
> +	return -EINPROGRESS;
> +}
> +
> +static struct sysreset_ops da9063_sysreset_ops = {
> +	.request = da9063_sysreset_request,
> +};
> +
> +U_BOOT_DRIVER(da9063_sysreset) = {
> +	.name = "da9063-sysreset",
> +	.id = UCLASS_SYSRESET,
> +	.ops = &da9063_sysreset_ops,
> +};
> +#endif
>