Am Dienstag, den 19.02.2013, 14:57 -0700 schrieb Stephen Warren: > On 02/19/2013 04:35 AM, Philipp Zabel wrote: > > This driver implements a reset controller device that toggles gpios > > connected to reset pins of peripheral ICs. The delay between assertion > > and de-assertion of the reset signal can be configured. > > > diff --git a/Documentation/devicetree/bindings/reset/gpio-reset.txt > > b/Documentation/devicetree/bindings/reset/gpio-reset.txt > > > +Required properties: > > > +- reset-delays: List of delays in ms. The corresponding gpio reset line is > > + asserted for this duration to reset. > > mS are quite long. Would it make sense for this property to be uS instead?
All GPIO resets that I've seen so far wait for several milliseconds. But I see no downside here to using microseconds instead. > > diff --git a/drivers/reset/gpio-reset.c b/drivers/reset/gpio-reset.c > > > +static int gpio_reset_probe(struct platform_device *pdev) > > > + if (of_find_property(np, "reset-delays", NULL)) { > > + delays = devm_kzalloc(&pdev->dev, sizeof(u32) * > > + drvdata->nr_gpios, GFP_KERNEL); > > + if (delays == NULL) > > + return -ENOMEM; > > It'd be nice if there were something like of_property_read_u32_index() > so you could read each value one-by-one in the loop later on, rather > than dynamically allocating this temporarily. Yes. Let's defer that to a separate patch. > > + ret = of_property_read_u32_array(np, "reset-delays", delays, > > + drvdata->nr_gpios); > > + if (ret < 0) > > + return ret; > > + } > > + > > + for (i = 0; i < drvdata->nr_gpios; i++) { > > + drvdata->gpios[i].gpio = of_get_named_gpio_flags(np, > > + "reset-gpios", i, &flags); > > + if (drvdata->gpios[i].gpio < 0) { > > + dev_err(&pdev->dev, "invalid gpio for reset %d\n", i); > > It's not an error if the value is -EPROBE_DEFERRED; you might want to > explicitly check for that case and not print anything? Right, I'll change that. > > + return drvdata->gpios[i].gpio; > > + } > > + > > + /* > > + * The flags are also used to remember whether a given GPIO > > + * reset is active-low. > > + */ > > + if (flags & OF_GPIO_ACTIVE_LOW) > > + drvdata->gpios[i].flags = GPIOF_OUT_INIT_HIGH; > > + else > > + drvdata->gpios[i].flags = GPIOF_OUT_INIT_LOW; > > That raises the question: What is the initial reset state expected to > be? Some devices might want to stay in reset until their driver > explicitly removes the reset signal. > > We could handle that by adding another (optional) property indicating > the initial reset state of each GPIO; default to initially not-in-reset > unless that property exists and specifies initially-in-reset. As with the time parameter, I wonder if this configuration is something we want to have in the consumer device tree node, or in the gpio-reset device node: gpio_reset: gpio-reset { compatible = "gpio-reset"; reset-gpios = <&gpio2 15 0>; reset-delays = <1000>; /* 1 ms */ initially-in-reset = <1>; } some-device { resets = <&gpio_reset 0>; } vs. gpio_reset: gpio-reset { compatible = "gpio-reset"; reset-gpios = <&gpio2 15 0>; } some-device { resets = <&gpio_reset 0 1000>; /* 1 ms */ initially-in-reset; } > > + ret = devm_gpio_request_one(&pdev->dev, drvdata->gpios[i].gpio, > > + drvdata->gpios[i].flags, NULL); > > + if (ret < 0) { > > + dev_err(&pdev->dev, "failed to request gpio %d for > > reset %d\n", > > + drvdata->gpios[i].gpio, i); > > + return ret; > > + } > > Perhaps first loop to look up all the GPIOs and initialize data > structures, then loop to request the GPIOs? That'd prevent lots of HW > programming and de-programming for the GPIOs near the start of the list, > in the case where some later GPIO causes -EPROBE_DEFERRED, and so this > probe() function keeps getting executed over and over. > > > + if (delays != NULL) > > + drvdata->gpios[i].delay_ms = delays[i]; > > + else > > + drvdata->gpios[i].delay_ms = -1; /* .reset returns > > -ENOSYS */ > > (here is where of_property_read_u32_index() might be handy) > > > +static struct platform_driver gpio_reset_driver = { > > + .probe = gpio_reset_probe, > > + .remove = gpio_reset_remove, > > + .driver = { > > + .name = "gpio-reset", > > + .owner = THIS_MODULE, > > + .of_match_table = of_match_ptr(gpio_reset_dt_ids), > > + }, > > +}; > > + > > +module_platform_driver(gpio_reset_driver); > > You might want to add a few things like MODULE_AUTHOR, > MODULE_DESCRIPTION, MODULE_LICENSE, > MODULE_DEVICE_TABLE(of, gpio_reset_dt_ids), perhaps MODULE_ALIAS for the > platform device too. Ok. thanks Philipp _______________________________________________ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss