Re: [PATCH 2/5] HDQ: BQ27000 HDQ Slave Interface Driver

Andrew Morton
2008-10-10T20:45:00Z

On Wed, 8 Oct 2008 12:49:19 +0530
"Gadiyar, Anand" <gadiyar@ti.com> wrote:

> From: Madhusudhan Chikkature <madhu.cr@ti.com>
>
> This patch provides the BQ27000 slave interface driver.
>
> ...
>
> --- /dev/null 1970-01-01 00:00:00.000000000 +0000
> +++ linux-2.6/drivers/w1/slaves/w1_bq27000.c 2008-09-26 14:43:42.000000000 +0530
> @@ -0,0 +1,120 @@
> +/*
> + * drivers/w1/slaves/w1_bq27000.c
> + *
> + * Copyright (C) 2007 Texas Instruments, Inc.
> + *
> + * This file is licensed under the terms of the GNU General Public License
> + * version 2. This program is licensed "as is" without any warranty of any
> + * kind, whether express or implied.
> + *
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/device.h>
> +#include <linux/types.h>
> +#include <linux/platform_device.h>
> +#include <linux/mutex.h>
> +
> +#include "../w1.h"
> +#include "../w1_int.h"
> +#include "../w1_family.h"
> +
> +#define HDQ_CMD_READ (0)
> +#define HDQ_CMD_WRITE (1<<7)
> +
> +int F_ID;
> +int family_id;

No, we don't want to add kernel-wide symbols with names like this.
Please either rename them to something which clearly identifies the
subsystem to which they belong, or make them static.

> +void w1_bq27000_write(struct device *dev, u8 buf, u8 reg)
> +{
> + struct w1_slave *sl = container_of(dev, struct w1_slave, dev);
> + if (!dev) {
> + pr_info("Could not obtain slave dev ptr\n");
> + return;
> + }
> +
> + w1_write_8(sl->master, HDQ_CMD_WRITE | reg);
> + w1_write_8(sl->master, buf);
> +}
> +EXPORT_SYMBOL(w1_bq27000_write);
> +
> +int w1_bq27000_read(struct device *dev, u8 reg)
> +{
> + u8 val;
> + struct w1_slave *sl = container_of(dev, struct w1_slave, dev);
> + if (!dev)
> + return 0;
> +
> + w1_write_8(sl->master, HDQ_CMD_READ | reg);
> + val = w1_read_8(sl->master);
> +
> + return val;
> +}
> +EXPORT_SYMBOL(w1_bq27000_read);

It's conventional to put a blank line between end-of-locals and
start-of-code.

> +static int w1_bq27000_add_slave(struct w1_slave *sl)
> +{
> + int ret;
> + int id = 1;
> + struct platform_device *pdev;
> +
> + pdev = platform_device_alloc("bq27000-battery", id);

Like that.

> + if (!pdev) {
> + ret = -ENOMEM;
> + return ret;
> + }
> + pdev->dev.parent = &sl->dev;
> +
> + ret = platform_device_add(pdev);
> + if (ret)
> + goto pdev_add_failed;
> +
> + dev_set_drvdata(&sl->dev, pdev);
> +
> + goto success;
> +
> +pdev_add_failed:
> + platform_device_unregister(pdev);
> +success:
> + return ret;
> +}
> +
> +static void w1_bq27000_remove_slave(struct w1_slave *sl)
> +{
> + struct platform_device *pdev = dev_get_drvdata(&sl->dev);
> + platform_device_unregister(pdev);
> +}
> +
> +static struct w1_family_ops w1_bq27000_fops = {
> + .add_slave = w1_bq27000_add_slave,
> + .remove_slave = w1_bq27000_remove_slave,
> +};
> +
> +static struct w1_family w1_bq27000_family = {
> + .fid = 1,
> + .fops = &w1_bq27000_fops,
> +};
> +
> +static int __init w1_bq27000_init(void)
> +{
> + if (F_ID)
> + w1_bq27000_family.fid = F_ID;
> +
> + return w1_register_family(&w1_bq27000_family);
> +}
> +
> +static void __exit w1_bq27000_exit(void)
> +{
> + w1_unregister_family(&w1_bq27000_family);
> +}
> +
> +
> +module_init(w1_bq27000_init);
> +module_exit(w1_bq27000_exit);
> +
> +module_param(F_ID, int, S_IRUSR);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Texas Instruments Ltd");
> +MODULE_DESCRIPTION("HDQ/1-wire slave driver bq27000 battery monitor chip");--