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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20140529155122.GA32214@kroah.com>
Date:	Thu, 29 May 2014 08:51:22 -0700
From:	Greg KH <gregkh@...uxfoundation.org>
To:	Satish Patel <satish.patel@...com>
Cc:	linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	linux-omap@...r.kernel.org, devicetree@...r.kernel.org,
	rob@...dley.net, tony@...mide.com, grant.likely@...aro.org,
	robh+dt@...nel.org
Subject: Re: [PATCH v3 1/5] sc_phy:SmartCard(SC) PHY interface to SC
 controller

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