Re: [PATCH v4 3/6] mfd: Add ChromeOS EC I2C driver

[Felipe Balbi <balbi@ti.com>]

[-- Attachment #1: Type: text/plain, Size: 4266 bytes --]

On Fri, Feb 15, 2013 at 08:16:09PM -0800, Simon Glass wrote:
> This uses an I2C bus to talk to the ChromeOS EC. The protocol
> is defined by the EC and is fairly simple, with a length byte,
> checksum, command byte and version byte (to permit easy creation
> of new commands).
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> Signed-off-by: Che-Liang Chiou <clchiou@chromium.org>

the driver you're adding here is no where near being an MFD device. MFD
children shouldn't be under drivers/mfd/. Please find a proper location
for this driver.

> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 837a16b..e1cd15e 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -29,6 +29,16 @@ config MFD_CROS_EC
>  	  You also ned to enable the driver for the bus you are using. The
>  	  protocol for talking to the EC is defined by the bus driver.
>  
> +config MFD_CROS_EC_I2C
> +	tristate "ChromeOS Embedded Controller (I2C)"
> +	depends on MFD_CROS_EC && I2C
> +
> +	help
> +	  If you say here, you get support for talking to the ChromeOS EC

if you say what here ?

> diff --git a/drivers/mfd/cros_ec_i2c.c b/drivers/mfd/cros_ec_i2c.c
> new file mode 100644
> index 0000000..fe3f2bf
> --- /dev/null
> +++ b/drivers/mfd/cros_ec_i2c.c
> @@ -0,0 +1,262 @@
> +/*
> + * ChromeOS EC multi-function device (I2C)
> + *
> + * Copyright (C) 2012 Google, Inc
> + *
> + * This software is licensed under the terms of the GNU General Public
> + * License version 2, as published by the Free Software Foundation, and
> + * may be copied, distributed, and modified under those terms.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/i2c.h>
> +#include <linux/interrupt.h>
> +#include <linux/mfd/cros_ec.h>
> +#include <linux/mfd/cros_ec_commands.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +
> +

one blank line only.

> +/* Since I2C can be unreliable, we retry commands */
> +#define COMMAND_MAX_TRIES 3

unreliable in what way ? Are you sure you haven't found a bug on your
embedded controller or your i2c controller driver ?

> +static const char *cros_ec_get_name(struct cros_ec_device *ec_dev)
> +{
> +	struct i2c_client *client = ec_dev->priv;
> +
> +	return client->name;
> +}

why ? What do you need get_name() for ? Why don't you just pass a
pointer to client->name directly ?

> +static const char *cros_ec_get_phys_name(struct cros_ec_device *ec_dev)
> +{
> +	struct i2c_client *client = ec_dev->priv;
> +
> +	return client->adapter->name;
> +}
> +
> +static struct device *cros_ec_get_parent(struct cros_ec_device *ec_dev)
> +{
> +	struct i2c_client *client = ec_dev->priv;
> +
> +	return &client->dev;
> +}

not sure you should allow other layers to fiddle with these. Specially
the parent device ointer.

> +static int cros_ec_probe_i2c(struct i2c_client *client,
> +			     const struct i2c_device_id *dev_id)
> +{
> +	struct device *dev = &client->dev;
> +	struct cros_ec_device *ec_dev = NULL;
> +	int err;
> +
> +	if (dev->of_node && !of_device_is_available(dev->of_node)) {
> +		dev_warn(dev, "Device disabled by device tree\n");
> +		return -ENODEV;
> +	}
> +
> +	ec_dev = cros_ec_alloc("I2C");

please don't use any indirection to allocators

> +	if (!ec_dev) {
> +		err = -ENOMEM;
> +		dev_err(dev, "cannot create cros_ec\n");

OOM messages are printed for you, no need to add another one here.

> +	i2c_set_clientdata(client, ec_dev);
> +	ec_dev->dev = dev;
> +	ec_dev->priv = client;
> +	ec_dev->irq = client->irq;
> +	ec_dev->command_xfer = cros_ec_command_xfer;
> +	ec_dev->get_name = cros_ec_get_name;
> +	ec_dev->get_phys_name = cros_ec_get_phys_name;
> +	ec_dev->get_parent = cros_ec_get_parent;
> +
> +	err = cros_ec_register(ec_dev);
> +	if (err) {
> +		dev_err(dev, "cannot register EC\n");
> +		goto fail_register;
> +	}

What is this doing ?

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]