On Tue, Sep 24, 2013 at 7:47 PM, Laxman Dewangan <ldewangan@xxxxxxxxxx> wrote:
function which just return not supported.This is getting a lot better quickly.
- Nit cleanups.
I'd like someone from devicetree@xxxxxxxxxxxxxxx to say
something about the DT bindings, can you convince one of
the DT custodians to ACK this?
Remaining issues though:
+This binding uses the following generic properties as defined inYou're not explaining what this property does.
+pinctrl-bindings.txt:
+
+Required: pins
Normally the set of pins are defined by the driver, not by the
device tree, since it's a property of the hardware, i.e. the
driver defines what hardware we have, and the DT defines
only how to set it up to do what we want to do.
+Example:
+ as3722: as3722 {
+ ....
+ gpio-controller;
+ #gpio-cells = <2>;
+
+ pinctrl-names = "default";
+ pinctrl-0 = <&as3722_default>;
+
+ as3722_default: pinmux {
+ gpio0 {
+ pins = "gpio0";
+ function = "gpio";
+ bias-pull-down;
+ };
+
+ gpio1_2_4_7 {
+ pins = "gpio1", "gpio2", "gpio4", "gpio7";
+ function = "gpio";
+ bias-pull-up;
+ };
+
+ gpio5 {
+ pins = "gpio5";
+ function = "clk32k_out";
+ };
+ };
As you see the name "gpio0" thru "gpio5" is quite confusing
here, as it is referring to pins, not GPIO lines (which is something
else in Linux).
I guess you know for sure whatever it is, and if the hardware
manual names the pins like this then there is not much we can
do. On most chips the actual pins have better names, like
chessboard coordinates, "D1" etc on BGAs or other good names.
Can you just inspect your HW docs and verify that your pins
really have these confusing names?
So it's this:
+static const char * const gpio_groups[] = {
+ "gpio0",
+ "gpio1",
+ "gpio2",
+ "gpio3",
+ "gpio4",
+ "gpio5",
+ "gpio6",
+ "gpio7",
+};
That is really, really confusing.
And this one-pin-per-group:
+static int as3722_gpio_direction_output(struct gpio_chip *chip,Son't you want to set the direction first, then output the value?
+ unsigned offset, int value)
+{
+ as3722_gpio_set(chip, offset, value);
+ return pinctrl_gpio_direction_output(chip->base + offset);
+}
This order is OK with some hardware, I'm just asking...