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+rene...@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
