Messages in this thread | | | Date | Sat, 16 Feb 2013 11:06:37 +0200 | From | Felipe Balbi <> | Subject | Re: [PATCH v4 3/6] mfd: Add ChromeOS EC I2C driver |
| |
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 [unhandled content-type:application/pgp-signature] | |