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
