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

[Felipe Balbi <balbi@ti.com>]

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

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 <balbi@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 <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.
> 
> 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

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