| Message ID | 1396289664-31893-1-git-send-email-sboyd@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Mar 31, 2014 at 11:14:24AM -0700, Stephen Boyd wrote: > If the debounce time is 0 our usage of ilog2() later on in this > driver will cause undefined behavior. If CONFIG_OF=n this fact is > evident to the compiler, and it emits a call to ____ilog2_NaN() > which doesn't exist. Fix this by setting a sane default for > debounce. > > Reported-by: Arnd Bergmann <arnd@arndb.de> > Signed-off-by: Stephen Boyd <sboyd@codeaurora.org> > --- > drivers/input/misc/pmic8xxx-pwrkey.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/input/misc/pmic8xxx-pwrkey.c b/drivers/input/misc/pmic8xxx-pwrkey.c > index 1cb8fda7a166..27add04676e1 100644 > --- a/drivers/input/misc/pmic8xxx-pwrkey.c > +++ b/drivers/input/misc/pmic8xxx-pwrkey.c > @@ -92,7 +92,7 @@ static int pmic8xxx_pwrkey_probe(struct platform_device *pdev) > bool pull_up; > > if (of_property_read_u32(pdev->dev.of_node, "debounce", &kpd_delay)) > - kpd_delay = 0; > + kpd_delay = 15625; What if somebody supplied 0 via DT? Can we check and return -EINVAL? > > pull_up = of_property_read_bool(pdev->dev.of_node, "pull-up"); > > -- > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, > hosted by The Linux Foundation >
On Mon, Mar 31, 2014 at 11:14:24AM -0700, Stephen Boyd wrote: > If the debounce time is 0 our usage of ilog2() later on in this > driver will cause undefined behavior. If CONFIG_OF=n this fact is > evident to the compiler, and it emits a call to ____ilog2_NaN() > which doesn't exist. Fix this by setting a sane default for > debounce. > > Reported-by: Arnd Bergmann <arnd@arndb.de> > Signed-off-by: Stephen Boyd <sboyd@codeaurora.org> > --- > drivers/input/misc/pmic8xxx-pwrkey.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/input/misc/pmic8xxx-pwrkey.c b/drivers/input/misc/pmic8xxx-pwrkey.c > index 1cb8fda7a166..27add04676e1 100644 > --- a/drivers/input/misc/pmic8xxx-pwrkey.c > +++ b/drivers/input/misc/pmic8xxx-pwrkey.c > @@ -92,7 +92,7 @@ static int pmic8xxx_pwrkey_probe(struct platform_device *pdev) > bool pull_up; > > if (of_property_read_u32(pdev->dev.of_node, "debounce", &kpd_delay)) > - kpd_delay = 0; > + kpd_delay = 15625; Should "debounce" even be optional? I'm wondering if we should just make it required... At the very least this default value should be documented in the DT binding.
On 03/31/14 11:23, Josh Cartwright wrote: > On Mon, Mar 31, 2014 at 11:14:24AM -0700, Stephen Boyd wrote: >> If the debounce time is 0 our usage of ilog2() later on in this >> driver will cause undefined behavior. If CONFIG_OF=n this fact is >> evident to the compiler, and it emits a call to ____ilog2_NaN() >> which doesn't exist. Fix this by setting a sane default for >> debounce. >> >> Reported-by: Arnd Bergmann <arnd@arndb.de> >> Signed-off-by: Stephen Boyd <sboyd@codeaurora.org> >> --- >> drivers/input/misc/pmic8xxx-pwrkey.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/input/misc/pmic8xxx-pwrkey.c b/drivers/input/misc/pmic8xxx-pwrkey.c >> index 1cb8fda7a166..27add04676e1 100644 >> --- a/drivers/input/misc/pmic8xxx-pwrkey.c >> +++ b/drivers/input/misc/pmic8xxx-pwrkey.c >> @@ -92,7 +92,7 @@ static int pmic8xxx_pwrkey_probe(struct platform_device *pdev) >> bool pull_up; >> >> if (of_property_read_u32(pdev->dev.of_node, "debounce", &kpd_delay)) >> - kpd_delay = 0; >> + kpd_delay = 15625; > Should "debounce" even be optional? I'm wondering if we should just > make it required... Hmm maybe. It's not like things won't work without it set though. > > At the very least this default value should be documented in the DT > binding. > I never understood this. The default value is something that is entirely software driven. It could change at any time. If DT implementers aren't specifying it they're saying "I don't care what the value is, let the driver decide if they care". They're not saying, "Oh, that default value in the binding is good enough for me so I don't need to specify this".
diff --git a/drivers/input/misc/pmic8xxx-pwrkey.c b/drivers/input/misc/pmic8xxx-pwrkey.c index 1cb8fda7a166..27add04676e1 100644 --- a/drivers/input/misc/pmic8xxx-pwrkey.c +++ b/drivers/input/misc/pmic8xxx-pwrkey.c @@ -92,7 +92,7 @@ static int pmic8xxx_pwrkey_probe(struct platform_device *pdev) bool pull_up; if (of_property_read_u32(pdev->dev.of_node, "debounce", &kpd_delay)) - kpd_delay = 0; + kpd_delay = 15625; pull_up = of_property_read_bool(pdev->dev.of_node, "pull-up");
If the debounce time is 0 our usage of ilog2() later on in this driver will cause undefined behavior. If CONFIG_OF=n this fact is evident to the compiler, and it emits a call to ____ilog2_NaN() which doesn't exist. Fix this by setting a sane default for debounce. Reported-by: Arnd Bergmann <arnd@arndb.de> Signed-off-by: Stephen Boyd <sboyd@codeaurora.org> --- drivers/input/misc/pmic8xxx-pwrkey.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)