Hi, 

From: Kishon Vijay Abraham I [mailto:kishon@ti.com]
Sent: Monday, December 09, 2013 8:56 AM

Hi,

On Friday 06 December 2013 09:58 PM, Kamil Debski wrote:

quoted

Hi Kishon,

Thank you for the review.

quoted

From: Kishon Vijay Abraham I [mailto:kishon@ti.com]
Sent: Friday, December 06, 2013 11:59 AM

Hi,

On Thursday 05 December 2013 05:59 PM, Kamil Debski wrote:

quoted

Add a new driver for the Exynos USB PHY. The new driver uses the
generic PHY framework. The driver includes support for the Exynos

4x10

quoted

and 4x12 SoC families.

Signed-off-by: Kamil Debski <redacted>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---

<snip>
.
.

quoted

quoted

quoted

diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile index

d0caae9..9f4befd 100644

--- a/drivers/phy/Makefile
+++ b/drivers/phy/Makefile

@@ -7,3 +7,6 @@ obj-$(CONFIG_PHY_EXYNOS_DP_VIDEO)	+= phy-

exynos-dp-

quoted

quoted

video.o

quoted

  obj-$(CONFIG_PHY_EXYNOS_MIPI_VIDEO)	+= phy-exynos-mipi-

video.o

quoted

quoted

quoted

  obj-$(CONFIG_OMAP_USB2)			+= phy-omap-usb2.o
  obj-$(CONFIG_TWL4030_USB)		+= phy-twl4030-usb.o
+obj-$(CONFIG_PHY_SAMSUNG_USB2)		+= phy-samsung-usb2.o
+obj-$(CONFIG_PHY_EXYNOS4210_USB2)	+= phy-exynos4210-usb2.o
+obj-$(CONFIG_PHY_EXYNOS4212_USB2)	+= phy-exynos4212-usb2.o

diff --git a/drivers/phy/phy-exynos4210-usb2.c

b/drivers/phy/phy-exynos4210-usb2.c
new file mode 100644
index 0000000..a02e5c2

--- /dev/null
+++ b/drivers/phy/phy-exynos4210-usb2.c

@@ -0,0 +1,264 @@
+/*
+ * Samsung SoC USB 1.1/2.0 PHY driver - Exynos 4210 support
+ *
+ * Copyright (C) 2013 Samsung Electronics Co., Ltd.
+ * Author: Kamil Debski <k.debski@samsung.com>
+ *
+ * This program is free software; you can redistribute it and/or
+modify
+ * it under the terms of the GNU General Public License version 2
+as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/phy/phy.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/spinlock.h>

You've included most of the above header files in phy-samsung-usb2.h
which you are including below.

I agree that includes in phy-samsung-usb2.h could use a cleanup. On
the other hand my opinion is that a .c file should include all .h
files that are used in this .c file. Relaying on .h file to include
another .h doesn't seem good to me.

then remove it in .h file.

quoted

quoted

quoted

+#include "phy-samsung-usb2.h"
+
+/* Exynos USB PHY registers */
+
+/* PHY power control */
+#define EXYNOS_4210_UPHYPWR			0x0
+
+#define EXYNOS_4210_UPHYPWR_PHY0_SUSPEND	(1 << 0)

use BIT() here and everywhere below.

<snip>
.
.

quoted

quoted

quoted

+#ifdef CONFIG_PHY_EXYNOS4212_USB2
+	{
+		.compatible = "samsung,exynos4212-usb2-phy",
+		.data = &exynos4212_usb2_phy_config,
+	},
+#endif
+	{ },
+};

I think we've had enough discussion about this approach. Let's get
the opinion of others too. Felipe? Greg?

Good idea.

quoted

Summary:
We have two drivers PHY_EXYNOS4210_USB2 and PHY_EXYNOS4212_USB2 with
almost similar register map [1] and a samsung helper driver for

these

quoted

quoted

two drivers.

I would not call them separate drivers. It's a single USB 2.0 driver
with the option to include support for various SoCs. This patchset

adds:

quoted

Exynos 4210, Exynos 4212, Exynos 5250 and S5PCV210. I know that
another person is working on supporting S3C6410.

quoted

These two PHY drivers populate the function pointers in the helper
driver. So any phy_ops will first invoke the helper driver which

will

quoted

quoted

then invoke the corresponding phy driver.

[1] -> http://www.diffchecker.com/7yno1uvk

Come on, this diff only includes the registers part of the file.
The following functions are also different:
- exynos421*_rate_to_clk
- exynos421*_isol
- exynos421*_phy_pwr
- exynos421*_power_on
- exynos421*_power_on

But most of the differences is because your 4212 has additional
features in HSIC and supports more clock rates.

quoted

It seems that the file is too large for the tool. But still this

makes

quoted

a false impression that only registers are different.

quoted

Advantages:
* (more) clean and readable
* helper driver can be used with other PHY drivers which will be
added soon

Disadvantages:
* code duplication

I would say that actually in this case less code is duplicated.

Having

quoted

Separate drivers would mean that most of the phy-samsung-usb2.c file
has

I actually meant a single driver for 4210 and 4212.

your current code has separate drivers for different versions of the
same IP. If you have a single driver for the different versions, it
will lead to a lot less code duplication (hint: I'v given the exact
'same'
comment at-least twice in this patch).

You wrote more than twice, I know. I also replied quite a few times. 
I want to explain that this is not a driver for Exynos 4210 and 4212 only.
It is a driver for more SoCs with different IPs.
Like Exynos 5250, S5PV210 and I am sure many more are to follow.
The other two patches in this series add new functionality to this driver
and are not separate drivers.

It is a single driver with custom functions for separate SoC and separate
versions of the same IP. In my opinion it will lead to less code in the
kernel. But we have different opinion on that :) We agree that the code
is more clear and it is easier to manage and add support for new SoC.
Maybe we should focus more on having a clear and readable code?

Kishon, if we were talking about Exynos 4210 and 4212 only, then I could
agree with you. But this driver covers more SoCs.

There are quite a few examples
in the kernel where the same driver is used for multiple versions of
the IP.

quoted

to be repeated. That is 240 times 4 (and surely more in the future,

as

quoted

this patchset adds support for 4 SoCs). Which is almost 1000 lines

more.

quoted

quoted

Maybe having a helper driver makes sense when we have other samsung
PHY drivers added but not sure if it's needed here for
EXYNOS4210_USB2 and
EXYNOS4212_USB2

Need your inputs on what you think about this.

Yes, I would also welcome other people's opinions.

quoted

quoted

+
+static int samsung_usb2_phy_probe(struct platform_device *pdev) {
+	const struct of_device_id *match;
+	const struct samsung_usb2_phy_config *cfg;
+	struct clk *clk;
+	struct device *dev = &pdev->dev;
+	struct phy_provider *phy_provider;
+	struct resource *mem;
+	struct samsung_usb2_phy_driver *drv;
+	int i;
+
+	if (!pdev->dev.of_node) {
+		dev_err(dev, "This driver is required to be instantiated

from device tree\n");

quoted

+		return -EINVAL;
+	}
+

<snip>
.
.

quoted

quoted

quoted

+	int (*power_on)(struct samsung_usb2_phy_instance *);
+	int (*power_off)(struct samsung_usb2_phy_instance *); };
+
+
+struct samsung_usb2_phy_config {
+	int num_phys;
+	const struct samsung_usb2_common_phy *phys;
+	char has_mode_switch;

u8 instead?

Do we really need to specify that we need 8bits? Why do you think

char

quoted

is wrong?

Do you really assign a char? Having a char data type and assigning an
integer is misleading.

quoted

Please read this paragraph from LDD3:
"Sometimes kernel code requires data items of a specific size,

perhaps

quoted

to match predefined binary structures,* to communicate with user
space, or to align data within structures by inserting "padding"
fields (but refer to the section "Data Alignment" for information
about alignment issues)."
Chapter 11, page 290 http://lwn.net/images/pdf/LDD3/ch11.pdf

has_mode_switch is only a flag 0 or 1. Never written anywhere in
hardware registers. Used in an if somewhere in code. Give me a good
reason why do you think it should be explicitly 8 bit long.

I just thought you created a char type to assign an integer value is
you wanted to some data type which is 8 bits long. If it is for any
other reason you used a char data type, pls let us know.

You must have missed the message I sent soon after the one you replied
to - bool is the best candidate for has_mode_switch.

Best wishes,
-- 
Kamil Debski
Samsung R&D Institute Poland