[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20130218074056.GB29848@arwen.pp.htv.fi>
Date: Mon, 18 Feb 2013 09:40:56 +0200
From: Felipe Balbi <balbi@...com>
To: Simon Glass <sjg@...omium.org>
CC: <balbi@...com>, LKML <linux-kernel@...r.kernel.org>,
Samuel Ortiz <sameo@...ux.intel.com>,
Che-Liang Chiou <clchiou@...omium.org>
Subject: Re: [PATCH v4 3/6] mfd: Add ChromeOS EC I2C driver
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@...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@...omium.org>
> >> Signed-off-by: Che-Liang Chiou <clchiou@...omium.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
Download attachment "signature.asc" of type "application/pgp-signature" (837 bytes)
Powered by blists - more mailing lists