Messages in this thread | | | Subject | Re: [PATCH v3 2/3] drm: bridge: Add thc63lvd1024 LVDS decoder driver | From | Sergei Shtylyov <> | Date | Wed, 14 Mar 2018 21:17:10 +0300 |
| |
On 03/14/2018 09:04 PM, jacopo mondi wrote:
>>> Add DRM bridge driver for Thine THC63LVD1024 LVDS to digital parallel >>> output decoder. >>> >>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> >> [...] >>> diff --git a/drivers/gpu/drm/bridge/thc63lvd1024.c b/drivers/gpu/drm/bridge/thc63lvd1024.c >>> new file mode 100644 >>> index 0000000..4b059c0 >>> --- /dev/null >>> +++ b/drivers/gpu/drm/bridge/thc63lvd1024.c >>> @@ -0,0 +1,241 @@ [...] >>> +static void thc63_enable(struct drm_bridge *bridge) >>> +{ >>> + struct thc63_dev *thc63 = to_thc63(bridge); >>> + struct regulator *vcc; >>> + unsigned int i; >>> + int ret; >>> + >>> + for (i = 0; i < ARRAY_SIZE(thc63->vccs); i++) { >>> + vcc = thc63->vccs[i]; >>> + if (vcc) { >>> + ret = regulator_enable(vcc); >>> + if (ret) >> >> You hardly need this variable, could do a call right in this *if*. >> >> [...] >>> +error_vcc_enable: >>> + dev_err(thc63->dev, "Failed to enable regulator %u\n", i); >>> +} >>> + >> >> Why not do this instead of *goto* before? > > Well, goto breaks the loop, if I only print out the error message, the > enable sequence will go on and enable the other regulators.
> I can print out and break, but I don't see that much benefit
Sorry, I meant that you should *return* there instead of *goto*.
> One thing I could do instead, is not only print out the error message, > but disable the already enabled regulators if one fails to start.
Yeah, probably...
[...] >>> +static int thc63_gpio_init(struct thc63_dev *thc63) >>> +{ >>> + thc63->pwdn = devm_gpiod_get_optional(thc63->dev, "pwdn", >>> + GPIOD_OUT_LOW); >>> + if (IS_ERR(thc63->pwdn)) { >>> + dev_err(thc63->dev, "Unable to get GPIO \"pwdn\"\n"); >> >> "pwdn-gpios" maybe? >> >>> + return PTR_ERR(thc63->pwdn); >>> + } >>> + >>> + thc63->oe = devm_gpiod_get_optional(thc63->dev, "oe", GPIOD_OUT_LOW); >>> + if (IS_ERR(thc63->oe)) { >>> + dev_err(thc63->dev, "Unable to get GPIO \"oe\"\n"); >> >> "oe-gpios" maybe? > > Are you referring to the error message?
Yes, seems more clear what to look for this way, IMHO...
[...]
MBR, Sergei
| |