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-...@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/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
