Hi, On Sat, Feb 16, 2013 at 06:46:53AM -0800, Simon Glass wrote: > Hi, > > On Sat, Feb 16, 2013 at 1:06 AM, Felipe Balbi <ba...@ti.com> wrote: > > 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 <s...@chromium.org> > >> Signed-off-by: Che-Liang Chiou <clch...@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. > > I think you might be misunderstanding the intent here. This driver is > actually not an MFD child, but a real MFD device. The children are > things like the cros_ec_keyb which use this device to access to the EC > and provide a function to the kernel (such as keyboard, EC flash > access and so on). They are indeed somewhere else in the tree. > > This driver sits in MFD for that reason, and provides a way to talk to > the EC over I2C. Rather than duplicate the code in each bus driver > (I2C, SPI, LPC) we have chosen to put that core code in a common file, > cros_ec, So one way to think of it is that we have several transports > which each provides an abstracted interface to an EC. > > There are several examples in drivers/mfd where this is done. For example: > > da9052_i2c.c and da9052_spi.c each provide an MFD driver, which calls > da9052_device_init() in da50542-core.c. > > and there are many others in MFD which use this approach, for example: > > drivers/mfd/wm831x-core.c > drivers/mfd/wm831x-i2c.c > drivers/mfd/wm831x-spi.c > > and > > drivers/mfd/tps65912-core.c > drivers/mfd/tps65912-i2c.c > drivers/mfd/tps65912-spi.c > > The intent is to keep the communications separate from the function > provided by the device.
fair enough. > >> +/* 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 ? > > Well those bugs were fixed. I think this is here for safety just in > case is this bad? I guess it looks a bit weird, specially since that retry mechanism should be on the i2c bus driver. > >> +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. > > This is the parent device for any children. Some MFD children will > want to know their parent. I can just add it to the structure I think. they can know their parent by using "dev->parent". Also you should be creating your children with proper mfd_add_devices() call. -- balbi
signature.asc
Description: Digital signature