linux-kernel - Re: [PATCH 3/3] drm: bridge: anx78xx: Add anx78xx bridge driver support.

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] [day] [month] [year] [list]

Date:	Mon, 28 Mar 2016 13:03:48 +0100
From:	Emil Velikov <emil.l.velikov@...il.com>
To:	Dan Carpenter <dan.carpenter@...cle.com>
Cc:	Enric Balletbo i Serra <enric.balletbo@...labora.com>,
	devicetree <devicetree@...r.kernel.org>,
	"Linux-Kernel@...r. Kernel. Org" <linux-kernel@...r.kernel.org>,
	ML dri-devel <dri-devel@...ts.freedesktop.org>,
	devel@...verdev.osuosl.org, Thierry Reding <treding@...dia.com>,
	Rob Herring <robh+dt@...nel.org>,
	Pawel Moll <pawel.moll@....com>,
	Mark Rutland <mark.rutland@....com>,
	Ian Campbell <ijc+devicetree@...lion.org.uk>,
	Kumar Gala <galak@...eaurora.org>,
	David Airlie <airlied@...ux.ie>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Sjoerd Simons <sjoerd.simons@...labora.co.uk>,
	javier@...hile0.org, span@...logixsemi.com,
	nathan.chung@...iatek.com, Daniel Kurtz <djkurtz@...omium.org>,
	drinkcat@...omium.org,
	Laurent Pinchart <laurent.pinchart@...asonboard.com>,
	jb.tsai@...iatek.com, cawa cheng <cawa.cheng@...iatek.com>,
	eddie.huang@...iatek.com, cjiao@...logixsemi.com
Subject: Re: [PATCH 3/3] drm: bridge: anx78xx: Add anx78xx bridge driver support.

Hi all,

On 24 March 2016 at 11:28, Dan Carpenter <dan.carpenter@...cle.com> wrote:
> On Thu, Mar 24, 2016 at 11:41:46AM +0100, Enric Balletbo i Serra wrote:
>> +     /* Map slave addresses of ANX7814 */
>> +     for (i = 0; i < I2C_NUM_ADDRESSES; i++) {
>> +             anx78xx->i2c_dummy[i] = i2c_new_dummy(client->adapter,
>> +                                             anx78xx_i2c_addresses[i] >> 1);
>> +             if (!anx78xx->i2c_dummy[i]) {
>> +                     DRM_ERROR("Failed to reserve i2c bus %02x.\n",
>> +                               anx78xx_i2c_addresses[i]);
>
> Missing error code here.
>
>> +                     goto err_i2c;
>> +             }
>
> I'm, of course, not a fan of the naming.  The name should be based on
> what the goto location does...  In this case it turns it off.  Which is
> slightly weird because we have not turned it on yet...  I always say
> that you should have multiple error labels and you only undo things
> which have been done.
>
> Having a common exit path for the other functions where it was "goto out"
> makes sense.  But again in those cases I would prefer a meaningful label
> name like "goto unlock;".  In the kernel "goto out;" is meaningless, it
> could do anything or everything or nothing.  A lot of people like it
> of course, but out: label code tends to be buggier than using a
> meaningful name.
>
Dan, I'm so glad to see another like minded person on the topic. It
seems that we're a minority though :-(

Enric, if you want to increase the chances of this getting reviewed I
would humbly suggest adding a per-patch changelog (must), explicitly
Cc (in the commit message) the people who commented on your patch
(highly recommended), and perhaps cutting down the 20+ people from the
To/Cc list (nitpicking).

Another option would be to assist/review similar (drm bridge) patches
for other contributors, who should return with the same :-)

Just some suggestions (my 2c as they say), seeing that this has been
around for a while.

Regards,
Emil