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

[Emil Velikov]

Hi all,

On 24 March 2016 at 11:28, Dan Carpenter <dan.carpenter@xxxxxxxxxx> 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