Fabio Estevam <festevam@xxxxxxxxx> writes: > On Sat, Dec 6, 2014 at 7:05 PM, Robert Jarzmik <robert.jarzmik@xxxxxxx> wrote: >> Change internal gpio handling from integer gpios into gpio >> descriptors. This change only addresses the internal API and >> device-tree/ACPI, while the legacy platform data remains integer space >> based. >> >> This change is only build compile tested, and very prone to error. I >> leave this comment for now in the commit message so that this patch gets >> some testing as I'm pretty sure it's buggy. > > I can confirm it is buggy. It makes the property 'reset-gpios' to be > mandatory now and causes regression. >> - gpio_set_value_cansleep(nop->gpio_reset, value); >> + if (nop->gpiod_reset) >> + gpiod_set_value(nop->gpiod_reset, asserted); > > Previously we had the active_low or active_high flag taken into > account. Now we don't. Is it something you're seing by testing of by code review ? Because the active high/active low is taken into account by gpiod_set_value(). >> - nop->gpio_reset = of_get_named_gpio_flags(node, "reset-gpios", >> - 0, &flags); >> - if (nop->gpio_reset == -EPROBE_DEFER) >> - return -EPROBE_DEFER; >> - >> - nop->reset_active_low = flags & OF_GPIO_ACTIVE_LOW; >> - >> + nop->gpiod_reset = devm_gpiod_get(dev, "reset-gpios"); > > We should do 'devm_gpiod_get(dev, "reset");' instead. Why ? The previous call was of_get_named_gpio_flags(node, "reset-gpios", ...). Why this name change into "reset" ? Now if you say we should do a "gpiod_get_optional()" instead of "devm_gpiod_get()", and if you're not willing to make that patch, I can make it. > This commit is now in linux-next as e9f2cefb0cdc2aea. > Should we revert it as it has so many issues? I'm not convinced of the "so many issues". So far I see the "gpiod_get_optional()" requirement, which is a one liner patch. Cheers. -- Robert -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
