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