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
| ||
|
Date: Tue, 31 Oct 2017 19:14:37 -0700 From: Guenter Roeck <linux@...ck-us.net> To: Stafford Horne <shorne@...il.com> Cc: Joel Stanley <joel@....id.au>, Rob Herring <robh+dt@...nel.org>, Philipp Zabel <philipp.zabel@...il.com>, Mykola Kostenok <c_mykolak@...lanox.com>, Jaghathiswari Rankappagounder Natarajan <jaghu@...gle.com>, devicetree@...r.kernel.org, linux-hwmon@...r.kernel.org, linux-kernel@...r.kernel.org Subject: Re: [PATCH] hwmon: (aspeed-pwm-tacho) Deassert reset in probe On 10/31/2017 07:04 PM, Stafford Horne wrote: > On Tue, Oct 31, 2017 at 06:53:15PM -0700, Guenter Roeck wrote: >> On 10/31/2017 06:34 PM, Joel Stanley wrote: >>> The ASPEED SoC must deassert a reset in order to use the PWM/tach >>> peripheral. >>> >>> The device tree bindings are updated to document the resets phandle, and >>> the example is updated to match what is expected for both the reset and >>> clock phandle. Note that the bindings should have always had the reset >>> controller, as the hardware is unusable without it. >>> >>> Signed-off-by: Joel Stanley <joel@....id.au> >> >> Presumably the driver is being used. This change makes it incompatible with >> existing users. This is unacceptable; after all, it is possible that the >> device is taken out of reset by ROMMON or BIOS. >> >> On top of that, the reset controller code is quite strict and issues a >> backtrace if CONFIG_RESET_CONTROLLER is not enabled. Yet, there is no >> dependency added on RESET_CONTROLLER. You might want to consider making >> the new control optional and using devm_reset_control_get_optional_exclusive(). >> >> The DT change should be a separate patch. >> >> More comments below. > > [..] > >>> return PTR_ERR_OR_ZERO(hwmon); >>> } >>> +static int aspeed_pwm_tacho_remove(struct platform_device *pdev) >>> +{ >>> + struct aspeed_pwm_tacho_data *priv = platform_get_drvdata(pdev); >>> + >>> + reset_control_deassert(priv->rst); >> >> This seems to be quite pointless. Also, did you test this code ? >> >>> + >>> + return 0; >>> +} >>> + >>> static const struct of_device_id of_pwm_tacho_match_table[] = { >>> { .compatible = "aspeed,ast2400-pwm-tacho", }, >>> { .compatible = "aspeed,ast2500-pwm-tacho", }, >>> @@ -969,6 +989,7 @@ MODULE_DEVICE_TABLE(of, of_pwm_tacho_match_table); >>> static struct platform_driver aspeed_pwm_tacho_driver = { >>> .probe = aspeed_pwm_tacho_probe, >>> + .probe = aspeed_pwm_tacho_remove, > > Also, this cant be right (should be .remove)? > Nice. Makes me really wonder what this code would do. Does this even compile ? Guenter
Powered by blists - more mailing lists