linux-kernel - Re: [PATCH v4 1/3] regulator: bd9571mwv: Use "backup_mode" sysfs file instead of "wake

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20180717113630.GF10087@bigcity.dyn.berto.se>
Date:   Tue, 17 Jul 2018 13:36:30 +0200
From:   Niklas Söderlund 
        <niklas.soderlund@...natech.se>
To:     Geert Uytterhoeven <geert+renesas@...der.be>
Cc:     Marek Vasut <marek.vasut+renesas@...il.com>,
        Liam Girdwood <lgirdwood@...il.com>,
        Mark Brown <broonie@...nel.org>,
        "Rafael J . Wysocki" <rjw@...ysocki.net>,
        Pavel Machek <pavel@....cz>, Len Brown <len.brown@...el.com>,
        linux-pm@...r.kernel.org, linux-renesas-soc@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 1/3] regulator: bd9571mwv: Use "backup_mode" sysfs
 file instead of "wake_up"

Hi Geert,

Thanks for your patch.

On 2018-07-16 17:30:50 +0200, Geert Uytterhoeven wrote:
> Currently the BD9571MWV PMIC driver uses the standard "wake_up" sysfs
> file to control enablement of DDR Backup Mode.
> 
> However, configuring DDR Backup Mode is not really equivalent to
> configuring the PMIC as a wake-up source.  To avoid confusion, use a
> custom "backup_mode" attribute file in sysfs instead.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@...der.be>

Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@...natech.se>

> ---
> v4:
>   - Fix build error and warning if !CONFIG_PM_SLEEP,
> 
> v3:
>   - New.
> ---
>  drivers/regulator/bd9571mwv-regulator.c | 52 ++++++++++++++++++++++---
>  1 file changed, 47 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/regulator/bd9571mwv-regulator.c b/drivers/regulator/bd9571mwv-regulator.c
> index be574eb444ebda97..1da36a6590c84ba4 100644
> --- a/drivers/regulator/bd9571mwv-regulator.c
> +++ b/drivers/regulator/bd9571mwv-regulator.c
> @@ -30,6 +30,7 @@ struct bd9571mwv_reg {
>  	/* DDR Backup Power */
>  	u8 bkup_mode_cnt_keepon;	/* from "rohm,ddr-backup-power" */
>  	u8 bkup_mode_cnt_saved;
> +	bool bkup_mode_enabled;
>  
>  	/* Power switch type */
>  	bool rstbmode_level;
> @@ -171,13 +172,40 @@ static int bd9571mwv_bkup_mode_write(struct bd9571mwv *bd, unsigned int mode)
>  	return 0;
>  }
>  
> +static ssize_t backup_mode_show(struct device *dev,
> +				struct device_attribute *attr, char *buf)
> +{
> +	struct bd9571mwv_reg *bdreg = dev_get_drvdata(dev);
> +
> +	return sprintf(buf, "%s\n", bdreg->bkup_mode_enabled ? "on" : "off");
> +}
> +
> +static ssize_t backup_mode_store(struct device *dev,
> +				 struct device_attribute *attr,
> +				 const char *buf, size_t count)
> +{
> +	struct bd9571mwv_reg *bdreg = dev_get_drvdata(dev);
> +	int ret;
> +
> +	if (!count)
> +		return 0;
> +
> +	ret = kstrtobool(buf, &bdreg->bkup_mode_enabled);
> +	if (ret)
> +		return ret;
> +
> +	return count;
> +}
> +
> +DEVICE_ATTR_RW(backup_mode);
> +
>  static int bd9571mwv_suspend(struct device *dev)
>  {
>  	struct bd9571mwv_reg *bdreg = dev_get_drvdata(dev);
>  	unsigned int mode;
>  	int ret;
>  
> -	if (!device_may_wakeup(dev))
> +	if (!bdreg->bkup_mode_enabled)
>  		return 0;
>  
>  	/* Save DDR Backup Mode */
> @@ -204,7 +232,7 @@ static int bd9571mwv_resume(struct device *dev)
>  {
>  	struct bd9571mwv_reg *bdreg = dev_get_drvdata(dev);
>  
> -	if (!device_may_wakeup(dev))
> +	if (!bdreg->bkup_mode_enabled)
>  		return 0;
>  
>  	/* Restore DDR Backup Mode */
> @@ -215,9 +243,15 @@ static const struct dev_pm_ops bd9571mwv_pm  = {
>  	SET_SYSTEM_SLEEP_PM_OPS(bd9571mwv_suspend, bd9571mwv_resume)
>  };
>  
> +static int bd9571mwv_regulator_remove(struct platform_device *pdev)
> +{
> +	device_remove_file(&pdev->dev, &dev_attr_backup_mode);
> +	return 0;
> +}
>  #define DEV_PM_OPS	&bd9571mwv_pm
>  #else
>  #define DEV_PM_OPS	NULL
> +#define bd9571mwv_regulator_remove	NULL
>  #endif /* CONFIG_PM_SLEEP */
>  
>  static int bd9571mwv_regulator_probe(struct platform_device *pdev)
> @@ -270,14 +304,21 @@ static int bd9571mwv_regulator_probe(struct platform_device *pdev)
>  		return -EINVAL;
>  	}
>  
> +#ifdef CONFIG_PM_SLEEP
>  	if (bdreg->bkup_mode_cnt_keepon) {
> -		device_set_wakeup_capable(&pdev->dev, true);
> +		int ret;
> +
>  		/*
> -		 * Wakeup is enabled by default in pulse mode, but needs
> +		 * Backup mode is enabled by default in pulse mode, but needs
>  		 * explicit user setup in level mode.
>  		 */
> -		device_set_wakeup_enable(&pdev->dev, bdreg->rstbmode_pulse);
> +		bdreg->bkup_mode_enabled = bdreg->rstbmode_pulse;
> +
> +		ret = device_create_file(&pdev->dev, &dev_attr_backup_mode);
> +		if (ret)
> +			return ret;
>  	}
> +#endif /* CONFIG_PM_SLEEP */
>  
>  	return 0;
>  }
> @@ -294,6 +335,7 @@ static struct platform_driver bd9571mwv_regulator_driver = {
>  		.pm = DEV_PM_OPS,
>  	},
>  	.probe = bd9571mwv_regulator_probe,
> +	.remove = bd9571mwv_regulator_remove,
>  	.id_table = bd9571mwv_regulator_id_table,
>  };
>  module_platform_driver(bd9571mwv_regulator_driver);
> -- 
> 2.17.1
> 

-- 
Regards,
Niklas Söderlund