From: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
To: jacopo mondi <jacopo@jmondi.org>
Cc: Jacopo Mondi <jacopo+renesas@jmondi.org>,
architt@codeaurora.org, a.hajda@samsung.com,
Laurent.pinchart@ideasonboard.com, airlied@linux.ie,
horms@verge.net.au, magnus.damm@gmail.com, geert@linux-m68k.org,
niklas.soderlund@ragnatech.se, robh+dt@kernel.org,
mark.rutland@arm.com, dri-devel@lists.freedesktop.org,
linux-renesas-soc@vger.kernel.org, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3 2/3] drm: bridge: Add thc63lvd1024 LVDS decoder driver
Date: Wed, 14 Mar 2018 21:17:10 +0300 [thread overview]
Message-ID: <ee1d849a-2996-8813-384b-cc2c22dd78a8@cogentembedded.com> (raw)
In-Reply-To: <20180314180438.GD16424@w540>
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
next prev parent reply other threads:[~2018-03-14 18:17 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-03-13 14:30 [PATCH v3 0/3] drm: Add Thine THC63LVD1024 LVDS decoder bridge Jacopo Mondi
2018-03-13 14:30 ` [PATCH v3 1/3] dt-bindings: display: bridge: Document THC63LVD1024 LVDS decoder Jacopo Mondi
2018-03-14 8:15 ` Andrzej Hajda
2018-03-14 9:06 ` jacopo mondi
2018-03-20 12:30 ` Laurent Pinchart
2018-03-13 14:30 ` [PATCH v3 2/3] drm: bridge: Add thc63lvd1024 LVDS decoder driver Jacopo Mondi
2018-03-14 8:42 ` Andrzej Hajda
2018-03-14 10:09 ` jacopo mondi
2018-03-15 9:44 ` Andrzej Hajda
2018-03-15 10:30 ` jacopo mondi
2018-03-14 17:09 ` Sergei Shtylyov
2018-03-14 18:04 ` jacopo mondi
2018-03-14 18:17 ` Sergei Shtylyov [this message]
2018-03-13 14:30 ` [PATCH v3 3/3] arm64: dts: renesas: Add LVDS decoder to R-Car V3M Eagle Jacopo Mondi
2018-03-13 18:47 ` Simon Horman
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ee1d849a-2996-8813-384b-cc2c22dd78a8@cogentembedded.com \
--to=sergei.shtylyov@cogentembedded.com \
--cc=Laurent.pinchart@ideasonboard.com \
--cc=a.hajda@samsung.com \
--cc=airlied@linux.ie \
--cc=architt@codeaurora.org \
--cc=devicetree@vger.kernel.org \
--cc=dri-devel@lists.freedesktop.org \
--cc=geert@linux-m68k.org \
--cc=horms@verge.net.au \
--cc=jacopo+renesas@jmondi.org \
--cc=jacopo@jmondi.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-renesas-soc@vger.kernel.org \
--cc=magnus.damm@gmail.com \
--cc=mark.rutland@arm.com \
--cc=niklas.soderlund@ragnatech.se \
--cc=robh+dt@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).