Re: [PATCH v3 1/5] sc_phy:SmartCard(SC) PHY interface to SC controller

[Greg KH <gregkh@linuxfoundation.org>]

On Thu, May 29, 2014 at 02:26:55PM +0530, Satish Patel wrote:
> 
> 
> On 5/29/2014 12:14 AM, Greg KH wrote:
> >On Wed, May 28, 2014 at 02:27:13PM +0530, Satish Patel wrote:
> >>+/**
> >>+ * struct sc_phy - The basic smart card phy structure
> >>+ *
> >>+ * @dev: phy device
> >>+ * @pdata: pointer to phy's private data structure
> >>+ * @set_config: called to set phy's configuration
> >>+ * @get_config: called to get phy's configuration
> >>+ * @activate_card: perform smart card activation
> >>+ * @deactivate_card: perform smart card de-activation
> >>+ * @warm_reset: execute smart card warm reset sequence
> >>+ * @register_card_activity_cb: register call back to phy device.
> >>+ * This call back will be called on card insert or remove event
> >>+ *
> >>+ * smart card controller uses this interface to communicate with
> >>+ * smart card via phy.Some smart card phy has multiple slots for
> >>+ * cards. This inerface also enables controller to communicate with
> >>+ * one or more smart card connected over phy.
> >>+ */
> >>+struct sc_phy {
> >>+	/* phy's device pointer */
> >>+	struct device *dev;
> >
> >So this is the "parent", right?  Why not embed a struct device into this
> >structure as well, further streaching out the device tree.
> >
> Do you mean to use "dev->p" for pdata ?

No, use the device itself.

> I have kept it outside, to give easeness/pragmatic focal for new phy driver
> development. Driver can make this pointer null and use above pointer.

Ick, no, that's not how the driver model works.  Each device in the
system needs a struct device, don't try to "chain" off of an existing
device like this.  Make it a "real" one.

> >>+
> >>+	/* phy's private data */
> >>+	void *pdata;
> >
> >If you do the above, then this pointer is not needed.
> >
> >
> >>+
> >>+	/* notify data, passed by interface user as a part of
> >>+	 * register_notify API. Data should be passed back when
> >>+	 * notification raised to the interface user
> >>+	 */
> >>+	void *notify_data;
> >
> >What makes this different from the pdata?
> pdata is phy's private data, while notify_data is something phy will send to
> smart card controller on some event, like card remove/insert/timeout etc..

That doesn't make much sense to me, why not just put that in the notify
function callback itself?

Please either use the existing phy layer of the kernel, or make a "real"
subsystem here, don't try to put a tiny shim on top of an existing
struct device for this driver, that's not how subsystems in Linux are
done.

thanks,

greg k-h