Mike, Thanks for reviewing this stuff ... Should be all fixed now.
Regards, Michael >-----Original Message----- >From: Mike Frysinger [mailto:[email protected]] >Sent: Tuesday, March 17, 2009 2:34 PM >To: [email protected] >Cc: [email protected] >Subject: Re: [Linux-kernel-commits] [6184] trunk: [#4375] ADP5520 Multifunction LCD Backlight and >Keypad Input Device > >On Tue, Mar 17, 2009 at 08:24, <[email protected]> wrote: >> --- trunk/drivers/mfd/adp5520.c >> +++ trunk/drivers/mfd/adp5520.c >> @@ -0,0 +1,370 @@ >> +int __adp5520_ack_bits(struct i2c_client *client, int reg, uint8_t >> bit_mask) >> +{ >> + struct adp5520_chip *chip = i2c_get_clientdata(client); >> + uint8_t reg_val; >> + int ret; >> + >> + mutex_lock(&chip->lock); >> + >> + ret = __adp5520_read(client, reg, ®_val); >> + if (ret) >> + goto out; >> + >> + reg_val |= bit_mask; >> + ret = __adp5520_write(client, reg, reg_val); >> +out: >> + mutex_unlock(&chip->lock); > >if (!ret) { > reg_val |= bit_mask; > ret = __adp5520_write(client, reg, reg_val); >} > >this would avoid the goto usage and make understanding code flow easier i think > >> +int adp5520_set_bits(struct device *dev, int reg, uint8_t bit_mask) >> +{ >> + struct adp5520_chip *chip = dev_get_drvdata(dev); >> + uint8_t reg_val; >> + int ret; >> + >> + mutex_lock(&chip->lock); >> + >> + ret = __adp5520_read(chip->client, reg, ®_val); >> + if (ret) >> + goto out; >> + >> + if ((reg_val & bit_mask) == 0) { >> + reg_val |= bit_mask; >> + ret = __adp5520_write(chip->client, reg, reg_val); >> + } >> +out: > >the ret check could be moved into the if statement to avoid the goto >if (!ret && .... > >> +int adp5520_clr_bits(struct device *dev, int reg, uint8_t bit_mask) >> +{ >> + struct adp5520_chip *chip = dev_get_drvdata(dev); >> + uint8_t reg_val; >> + int ret; >> + >> + mutex_lock(&chip->lock); >> + >> + ret = __adp5520_read(chip->client, reg, ®_val); >> + if (ret) >> + goto out; >> + >> + if (reg_val & bit_mask) { >> + reg_val &= ~bit_mask; >> + ret = __adp5520_write(chip->client, reg, reg_val); >> + } >> +out: > >same here ... > >> +int adp5520_register_notifier(struct device *dev, struct notifier_block >> *nb, >> + unsigned int events) >> +{ >> + struct adp5520_chip *chip = dev_get_drvdata(dev); >> + >> + if (chip->irq) { >> + adp5520_set_bits(chip->dev, INTERRUPT_ENABLE, >> + events & (KP_IEN | KR_IEN | OVP_IEN | CMPR_IEN)); >> + >> + return blocking_notifier_chain_register(&chip->notifier_list, nb); > >missing indentation > >> +static int __devinit adp5520_add_subdevs(struct adp5520_chip *chip, >> + struct adp5520_platform_data *pdata) >> +{ >> + struct adp5520_subdev_info *subdev; >> + struct platform_device *pdev; >> + int i, ret = 0; >> + >> + for (i = 0; i < pdata->num_subdevs; i++) { >> + subdev = &pdata->subdevs[i]; >> + >> + pdev = platform_device_alloc(subdev->name, subdev->id); >> + >> + pdev->dev.parent = chip->dev; >> + pdev->dev.platform_data = subdev->platform_data; >> + >> + ret = platform_device_add(pdev); >> + if (ret) >> + goto failed; >> + } > >is it possible for the platform device data to be NULL here ? > >> +static int __devinit adp5520_probe(struct i2c_client *client, >> + const struct i2c_device_id *id) >> +... >> + chip = kzalloc(sizeof(struct adp5520_chip), GFP_KERNEL); > >sizeof(*chip) >-mike _______________________________________________ Linux-kernel-commits mailing list [email protected] http://blackfin.uclinux.org/mailman/listinfo/linux-kernel-commits
