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
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
