linux-kernel - Re: [PATCH v3 2/3] drm: bridge: Add thc63lvd1024 LVDS decoder driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ee1d849a-2996-8813-384b-cc2c22dd78a8@cogentembedded.com>
Date:   Wed, 14 Mar 2018 21:17:10 +0300
From:   Sergei Shtylyov <sergei.shtylyov@...entembedded.com>
To:     jacopo mondi <jacopo@...ndi.org>
Cc:     Jacopo Mondi <jacopo+renesas@...ndi.org>, architt@...eaurora.org,
        a.hajda@...sung.com, Laurent.pinchart@...asonboard.com,
        airlied@...ux.ie, horms@...ge.net.au, magnus.damm@...il.com,
        geert@...ux-m68k.org, niklas.soderlund@...natech.se,
        robh+dt@...nel.org, mark.rutland@....com,
        dri-devel@...ts.freedesktop.org, linux-renesas-soc@...r.kernel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 2/3] drm: bridge: Add thc63lvd1024 LVDS decoder driver

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@...ndi.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