| Message ID | 1376718340-16885-1-git-send-email-shc_work@mail.ru (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Alexander, Please see my comments inline. On Saturday 17 of August 2013 09:45:40 Alexander Shiyan wrote: > Patch adds DT support for MC13783/MC13892 PMICs. > > Signed-off-by: Alexander Shiyan <shc_work@mail.ru> > --- > Documentation/devicetree/bindings/mfd/mc13xxx.txt | 13 +++++ > drivers/input/misc/mc13783-pwrbutton.c | 61 > +++++++++++++++++------ 2 files changed, 60 insertions(+), 14 > deletions(-) > > diff --git a/Documentation/devicetree/bindings/mfd/mc13xxx.txt > b/Documentation/devicetree/bindings/mfd/mc13xxx.txt index > abd9e3c..cf8b61c 100644 > --- a/Documentation/devicetree/bindings/mfd/mc13xxx.txt > +++ b/Documentation/devicetree/bindings/mfd/mc13xxx.txt > @@ -10,6 +10,12 @@ Optional properties: > - fsl,mc13xxx-uses-touch : Indicate the touchscreen controller is being > used > > Sub-nodes: > +- buttons : Contain power button nodes. Each button should be declared > as + "button@<num>" and contain code in "linux,code" property. You should not really be enforcing such strict node naming. If you have the "buttons" node, which aggregates all the buttons, all you need is just parsing all the child nodes of it, regardless of their names. As a side note, the @unit-address suffix is used only when you have the reg property present in the node and must be equal to the address value set in this property. If you need the button index (which is true, looking at the code), you should use the reg property instead, with appropriate #address-cells (probably 1 for your use case) and #size-cells (0, as size doesn't make sense in your use case) in "buttons" node. > + Optional properties: The properties below should be rather prefixed with "fsl," string to indicate that they are specific to this particular device. > + active-high : Change active button level from 0 to 1. It is a boolean property, isn't it? If yes, this should be noted. Also the "from 0 to 1" statement is a bit unfortunate. What about: active-high : A boolean property present if the button is active high. Otherwise the button is assumed to be active low. > + enable-reset: Performs hadware reset through PMIC. Could you elaborate on meaning of this property a bit more, please? > + debounce : Debounce value which will be taken from PMIC > datasheet. - regulators : Contain the regulator nodes. The regulators > are bound using their names as listed below with their registers and > bits for enabling. > > @@ -89,6 +95,13 @@ ecspi@70010000 { /* ECSPI1 */ > interrupt-parent = <&gpio0>; > interrupts = <8>; > > + buttons { > + button@1 { > + linux,code = <0x1f>; > + debounce = <1>; > + }; > + }; > + So basically this example after addressing my comments would look like: buttons { #address-cells = <1>; #size-cells = <0>; irrelevant-name@1 { reg = <1>; linux,code = <0x1f>; fsl,debounce = <1>; }; }; > regulators { > sw1_reg: mc13892__sw1 { > regulator-min-microvolt = <600000>; > diff --git a/drivers/input/misc/mc13783-pwrbutton.c > b/drivers/input/misc/mc13783-pwrbutton.c index 2e21f19..3f9cfd1 100644 > --- a/drivers/input/misc/mc13783-pwrbutton.c > +++ b/drivers/input/misc/mc13783-pwrbutton.c > @@ -24,6 +24,7 @@ > #include <linux/kernel.h> > #include <linux/errno.h> > #include <linux/input.h> > +#include <linux/of.h> > #include <linux/platform_device.h> > #include <linux/mfd/mc13783.h> > #include <linux/mfd/mc13892.h> > @@ -77,21 +78,28 @@ static int __init mc13xxx_pwrbutton_probe(struct > platform_device *pdev) struct mc13xxx *mc13xxx = > dev_get_drvdata(pdev->dev.parent); struct mc13xxx_pwrb_devtype *devtype > = > (struct mc13xxx_pwrb_devtype *)pdev->id_entry- >driver_data; > + struct device_node *parent, *child; > struct mc13xxx_pwrb *priv; > int i, reg = 0, ret = -EINVAL; > > - if (!pdata) { > + of_node_get(pdev->dev.parent->of_node); > + parent = of_find_node_by_name(pdev->dev.parent->of_node, "buttons"); The of_find_node_by_name() function does not search for node with given name inside the node you specify, but rather starting from this node and going over all the rest of device tree. of_get_child_by_name() seems to be what you need here. > + if (!pdata && !parent) { > dev_err(&pdev->dev, "Missing platform data\n"); > return -ENODEV; > } > > priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL); > - if (!priv) > - return -ENOMEM; > + if (!priv) { > + ret = -ENOMEM; > + goto out_node_put; > + } > > priv->input = devm_input_allocate_device(&pdev->dev); > - if (!priv->input) > - return -ENOMEM; > + if (!priv->input) { > + ret = -ENOMEM; > + goto out_node_put; > + } > > priv->mc13xxx = mc13xxx; > priv->devtype = devtype; > @@ -99,15 +107,36 @@ static int __init mc13xxx_pwrbutton_probe(struct > platform_device *pdev) > > for (i = 0; i < MAX13XXX_NUM_BUTTONS; i++) { > u16 code, invert, reset, debounce; > - > - if (!(pdata->buttons[i].flags & MC13XXX_BUTTON_ENABLE)) > - continue; > - code = pdata->buttons[i].keycode; > - invert = !!(pdata->buttons[i].flags & > - MC13XXX_BUTTON_POL_INVERT); > - reset = !!(pdata->buttons[i].flags & > - MC13XXX_BUTTON_RESET_EN); > - debounce = pdata->buttons[i].flags; > + const __be32 *prop; > + char childname[5]; > + > + if (parent) { > + sprintf(childname, "button@%i", i + 1); Hmm, this can lead to stack corruption. The line above will print 9 (including terminating zero) characters to the childname array, which can hold only 5 of them. > + child = of_get_child_by_name(parent, childname); > + if (!child) > + continue; > + prop = of_get_property(child, "linux,code", NULL); > + if (prop) > + code = be32_to_cpu(*prop) & 0xffff; What about using of_property_read_u32() here? See include/linux/of.h for a whole lot of useful DT parsing functions and their descriptions in drivers/of/base.c (kernel-doc comments). > + else { > + dev_err(&pdev->dev, > + "Button %i: Missing key code\n", i + 1); > + continue; > + } > + invert = of_property_read_bool(child, "active- high"); > + reset = of_property_read_bool(child, "enable- reset"); > + prop = of_get_property(child, "debounce", NULL); > + debounce = prop ? be32_to_cpu(*prop) : 0; Again, of_property_read_u32() would simplify the code. As a side note, there is also a be32_to_cpup() helper, which dereferences a __be32 pointer for you and returns a value in CPU endianess. > + } else { > + if (!(pdata->buttons[i].flags & MC13XXX_BUTTON_ENABLE)) > + continue; > + code = pdata->buttons[i].keycode; > + invert = !!(pdata->buttons[i].flags & > + MC13XXX_BUTTON_POL_INVERT); > + reset = !!(pdata->buttons[i].flags & > + MC13XXX_BUTTON_RESET_EN); > + debounce = pdata->buttons[i].flags; > + } Anyway, based on my comments wrt the bindings, I would rather separate the DT parsing code from this loop and, in DT case, parse the buttons in a for_each_child_of_node() loop, reading the reg property and using it as an index to the buttons array (after checking if the index isn't out of bounds, of course). Best regards, Tomasz -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/devicetree/bindings/mfd/mc13xxx.txt b/Documentation/devicetree/bindings/mfd/mc13xxx.txt index abd9e3c..cf8b61c 100644 --- a/Documentation/devicetree/bindings/mfd/mc13xxx.txt +++ b/Documentation/devicetree/bindings/mfd/mc13xxx.txt @@ -10,6 +10,12 @@ Optional properties: - fsl,mc13xxx-uses-touch : Indicate the touchscreen controller is being used Sub-nodes: +- buttons : Contain power button nodes. Each button should be declared as + "button@<num>" and contain code in "linux,code" property. + Optional properties: + active-high : Change active button level from 0 to 1. + enable-reset: Performs hadware reset through PMIC. + debounce : Debounce value which will be taken from PMIC datasheet. - regulators : Contain the regulator nodes. The regulators are bound using their names as listed below with their registers and bits for enabling. @@ -89,6 +95,13 @@ ecspi@70010000 { /* ECSPI1 */ interrupt-parent = <&gpio0>; interrupts = <8>; + buttons { + button@1 { + linux,code = <0x1f>; + debounce = <1>; + }; + }; + regulators { sw1_reg: mc13892__sw1 { regulator-min-microvolt = <600000>; diff --git a/drivers/input/misc/mc13783-pwrbutton.c b/drivers/input/misc/mc13783-pwrbutton.c index 2e21f19..3f9cfd1 100644 --- a/drivers/input/misc/mc13783-pwrbutton.c +++ b/drivers/input/misc/mc13783-pwrbutton.c @@ -24,6 +24,7 @@ #include <linux/kernel.h> #include <linux/errno.h> #include <linux/input.h> +#include <linux/of.h> #include <linux/platform_device.h> #include <linux/mfd/mc13783.h> #include <linux/mfd/mc13892.h> @@ -77,21 +78,28 @@ static int __init mc13xxx_pwrbutton_probe(struct platform_device *pdev) struct mc13xxx *mc13xxx = dev_get_drvdata(pdev->dev.parent); struct mc13xxx_pwrb_devtype *devtype = (struct mc13xxx_pwrb_devtype *)pdev->id_entry->driver_data; + struct device_node *parent, *child; struct mc13xxx_pwrb *priv; int i, reg = 0, ret = -EINVAL; - if (!pdata) { + of_node_get(pdev->dev.parent->of_node); + parent = of_find_node_by_name(pdev->dev.parent->of_node, "buttons"); + if (!pdata && !parent) { dev_err(&pdev->dev, "Missing platform data\n"); return -ENODEV; } priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL); - if (!priv) - return -ENOMEM; + if (!priv) { + ret = -ENOMEM; + goto out_node_put; + } priv->input = devm_input_allocate_device(&pdev->dev); - if (!priv->input) - return -ENOMEM; + if (!priv->input) { + ret = -ENOMEM; + goto out_node_put; + } priv->mc13xxx = mc13xxx; priv->devtype = devtype; @@ -99,15 +107,36 @@ static int __init mc13xxx_pwrbutton_probe(struct platform_device *pdev) for (i = 0; i < MAX13XXX_NUM_BUTTONS; i++) { u16 code, invert, reset, debounce; - - if (!(pdata->buttons[i].flags & MC13XXX_BUTTON_ENABLE)) - continue; - code = pdata->buttons[i].keycode; - invert = !!(pdata->buttons[i].flags & - MC13XXX_BUTTON_POL_INVERT); - reset = !!(pdata->buttons[i].flags & - MC13XXX_BUTTON_RESET_EN); - debounce = pdata->buttons[i].flags; + const __be32 *prop; + char childname[5]; + + if (parent) { + sprintf(childname, "button@%i", i + 1); + child = of_get_child_by_name(parent, childname); + if (!child) + continue; + prop = of_get_property(child, "linux,code", NULL); + if (prop) + code = be32_to_cpu(*prop) & 0xffff; + else { + dev_err(&pdev->dev, + "Button %i: Missing key code\n", i + 1); + continue; + } + invert = of_property_read_bool(child, "active-high"); + reset = of_property_read_bool(child, "enable-reset"); + prop = of_get_property(child, "debounce", NULL); + debounce = prop ? be32_to_cpu(*prop) : 0; + } else { + if (!(pdata->buttons[i].flags & MC13XXX_BUTTON_ENABLE)) + continue; + code = pdata->buttons[i].keycode; + invert = !!(pdata->buttons[i].flags & + MC13XXX_BUTTON_POL_INVERT); + reset = !!(pdata->buttons[i].flags & + MC13XXX_BUTTON_RESET_EN); + debounce = pdata->buttons[i].flags; + } priv->btn_code[i] = code; if (code != KEY_RESERVED) @@ -155,6 +184,10 @@ static int __init mc13xxx_pwrbutton_probe(struct platform_device *pdev) if (ret) dev_err(&pdev->dev, "Can't register input device: %i\n", ret); +out_node_put: + if (parent) + of_node_put(parent); + return ret; }
Patch adds DT support for MC13783/MC13892 PMICs. Signed-off-by: Alexander Shiyan <shc_work@mail.ru> --- Documentation/devicetree/bindings/mfd/mc13xxx.txt | 13 +++++ drivers/input/misc/mc13783-pwrbutton.c | 61 +++++++++++++++++------ 2 files changed, 60 insertions(+), 14 deletions(-)