[PATCH v9 1/8] drivers: phy: add generic PHY framework

On Wed, Jul 17, 2013 at 03:02:59PM +0530, Kishon Vijay Abraham I wrote:

Hi,

On Wednesday 17 July 2013 11:59 AM, Greg KH wrote:

quoted

On Wed, Jun 26, 2013 at 05:17:29PM +0530, Kishon Vijay Abraham I wrote:

quoted

+menuconfig GENERIC_PHY
+	tristate "PHY Subsystem"
+	help
+	  Generic PHY support.
+
+	  This framework is designed to provide a generic interface for PHY
+	  devices present in the kernel. This layer will have the generic
+	  API by which phy drivers can create PHY using the phy framework and
+	  phy users can obtain reference to the PHY.

Shouldn't this be something that other drivers select?  How will anyone
know if they need this or not?

All the PHY drivers should go here. So only if *GENERIC_PHY* is enabled those
PHY drivers can be selected like in [1].
The PHY consumer driver should ideally use *depends on* in their Kconfig.

[1] ->
http://archive.arm.linux.org.uk/lurker/message/20130628.134308.4a8f7668.ca.html

No, switch it around the other way.  How would I even _know_ that I need
to enable the generic PHY subsystem in the first place?  How can I
determine that I need this for my hardware?  You need to give hints to
someone who doesn't even know what a PHY is, otherwise they will always
disable it and move on.

quoted

quoted

--- /dev/null
+++ b/drivers/phy/phy-core.c

@@ -0,0 +1,544 @@
+/*
+ * phy-core.c  --  Generic Phy framework.
+ *
+ * Copyright (C) 2013 Texas Instruments
+ *
+ * Author: Kishon Vijay Abraham I <kishon@ti.com>
+ *
+ * This program is free software; you can redistribute  it and/or modify it
+ * under  the terms of  the GNU General  Public License as published by the
+ * Free Software Foundation;  either version 2 of the  License, or (at your
+ * option) any later version.

You really mean "any later version" (I have to ask)?

That was copied from somewhere :-s

So, is that what you really mean to have for this code?

quoted

quoted

+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.

Are these two paragraphs needed?  This isn't a "program", and they got a
copy of the GPL already with the kernel.

hmm.. I can remove that.

quoted

quoted

+static struct class *phy_class;

Why do you need a class?

Wanted to group all the PHY drivers to be used by different subsystems
(SATA/USB/PCIE/HDMI/VIDEO) into a single entity. There were some comments in my
initial version [3] on using a bus_type instead of class but then it was
decided to go with class itself.

[3] -> http://lkml.indiana.edu/hypermail/linux/kernel/1302.2/01389.html

Ok, but what does the class usage get you?

quoted

When modifying/adding new sysfs stuff, you need a Documentation/ABI/
entry as well.

I'm not actually adding any new sysfs entry other than what a *class_create*
must have created. Do I need to add one for that?

If you are not creating anything in sysfs at all, why use the driver
model?  (hint, I think you need to do something here to justify it...)

thanks,

greg k-h