On Tue, Mar 17, 2009 at 09:15, Hennerich, Michael wrote: >From: [email protected] >>> +static int adp5520_bl_set(struct backlight_device *bl, int >>> +{ >>> + ... >>> + } else { >>> + ret |= adp5520_write(master, DAYLIGHT_MAX, brightness); >>> + } >> >>dont need these braces > > I thought - in case the if path required braces the else path should > have them too?
maybe, but istr people jumping on these in general when there's one statement. for the trailing else case for sure. >>> + if (ret) >>> + return ret; >>> + >>> + data->current_brightness = brightness; >>> + return 0; >> >>maybe better structured as >>if (!ret) >> data->current_brightness = brightness; >>return ret; > > Does this really matter? it seemed to flow more logically (at least to me). you're certainly free to say "no" :). >>> + if (data->pdata->en_ambl_sens) >>> + ret = sysfs_create_group(&bl->dev.kobj, >>> + &adp5520_bl_attr_group); >>> + >>> + platform_set_drvdata(pdev, bl); >>> + ret |= adp5520_bl_setup(bl); >>> + backlight_update_status(bl); >>> + >>> + return ret; >> >>can sysfs_create_group() fail ? if it does, do we care ? > > It can - but I don't care. Actually never seen this happen. > The driver is still functional. so we should not record the return value ... otherwise the probe step may return a failure even if we didnt care i.e. drop the "ret = " in the sysfs_create_group() statement -mike _______________________________________________ Linux-kernel-commits mailing list [email protected] http://blackfin.uclinux.org/mailman/listinfo/linux-kernel-commits
