Re: [char-misc-next 01/11 V4] mei: bus: Initial MEI Client bus type implementation

[Samuel Ortiz <sameo@linux.intel.com>]

Hi Greg,

On Mon, Mar 25, 2013 at 01:28:55PM -0700, Greg KH wrote:
> On Thu, Mar 21, 2013 at 12:44:19AM +0200, Tomas Winkler wrote:
> >  create mode 100644 Documentation/misc-devices/mei/mei-client-bus.txt
> 
> Shouldn't you also create Documentation/ABI/ entries as well?
You mean for the bus specific stuff or for the /dev/mei entry ?
For the bus parts, we're not adding any MEI specific sysfs entries. What
should we document ?

 
> > +#define NFC_UUID UUID_LE(0x0bb17a78, 0x2a8e, 0x4c50, 0x94, \
> > +			       0xd4, 0x50, 0x26, 0x67, 0x23, 0x77, 0x5c)
> > +
> > +static struct mei_cl_id contact_mei_cl_tbl[] = {
> > +	{ CONTACT_DRIVER_NAME, NFC_UUID },
> > +
> > +	/* required last entry */
> > +	{ }
> > +};
> 
> What about MODULE_DEVICE() functionality for this structure?  Don't you
> need/want that as well?
I was planning to do that as a separate mod_devicetable.h patch once this
code gets merged, but I guess it makes sense to include it with this patchset.
It reminds me that I should remove the uuid_le field from the mei_cl_id
structure, it's useless.


> > +/**
> > + * struct mei_cl_device - MEI device handle
> > + * An mei_cl_device pointer is returned from mei_add_device()
> > + * and links MEI bus clients to their actual ME host client pointer.
> > + * Drivers for MEI devices will get an mei_cl_device pointer
> > + * when being probed and shall use it for doing ME bus I/O.
> > + *
> > + * @dev: linux driver model device pointer
> > + * @uuid: me client uuid
> > + * @cl: mei client
> > + * @priv_data: client private data
> > + */
> > +struct mei_cl_device {
> > +	struct device dev;
> > +
> > +	uuid_le uuid;
> > +	struct mei_cl *cl;
> > +
> > +	void *priv_data;
> > +};
> 
> Why is priv_data needed?  What's wrong with the pointer provided to you
> in struct device?
mei_cl_device->dev->p->driver_data is used by mei drivers to carry their
private data around, we define mei_cl_get_clientdata/mei_cl_set_clientdata for
that and keep the mei_cl_device structure opaque to them.
mei_cl_device->priv_data is used internally by the drivers/misc/mei/ bus
related code to carry technology specific pointers. drivers/misc/mei/nfc.c
uses it to fetch its private data when it gets a mei_cl_device pointer.



> > + *
> > + * Contact Information:
> > + *	Intel Corporation.
> > + *	linux-mei@linux.intel.com
> > + *	http://www.intel.com
> > + *
> > + * BSD LICENSE
> 
> Wait, you are putting code that has EXPORT_SYMBOL_GPL() usage under a
> GPL/BSD license?  I need an Intel lawyer signed-off-by: on the patch
> before I can accept that.
Sorry about that. We changed our EXPORT_SYMBOL to EXPORT_SYMBOL_GPL when you
asked for it, and I missed changing this header file at the same time. I will
actually remove the licensing terms from this file.


> > +struct mei_cl_driver {
> > +	struct device_driver driver;
> > +	const char *name;
> 
> What's wrong with the driver.name field?
>From your initial comments on patchset v1:

"
	> +static struct mei_bus_driver contact_driver = {
	> +     .driver = {
	> +                .name = CONTAC_DRIVER_NAME,
	> +               },

	Can't you put a name field in your mei_bus_driver structure and then
	copy it to the version in the driver model?  That's what other bus
	drivers do and it makes more sense.
"

And you were right, that is indeed what other bus drivers do. Did I
misunderstand you here ?

Cheers,
Samuel.

-- 
Intel Open Source Technology Centre
http://oss.intel.com/