Re: [PATCH v4 3/9] phy: Add new Exynos USB PHY driver
From: Kishon Vijay Abraham I
Date: Mon Dec 09 2013 - 02:56:47 EST
Hi,
On Friday 06 December 2013 09:58 PM, Kamil Debski wrote:
Hi Kishon,
Thank you for the review.
From: Kishon Vijay Abraham I [mailto:kishon@xxxxxx]
Sent: Friday, December 06, 2013 11:59 AM
Hi,
On Thursday 05 December 2013 05:59 PM, Kamil Debski wrote:
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
and 4x12 SoC families.
Signed-off-by: Kamil Debski <k.debski@xxxxxxxxxxx>
Signed-off-by: Kyungmin Park <kyungmin.park@xxxxxxxxxxx>
---
<snip>
.
.
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-
video.o
obj-$(CONFIG_PHY_EXYNOS_MIPI_VIDEO) += phy-exynos-mipi-video.o
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@xxxxxxxxxxx>
+ *
+ * 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.
+#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>
.
.
+#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.
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
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:
Exynos 4210, Exynos 4212, Exynos 5250 and S5PCV210. I know that another
person is working on supporting S3C6410.
These two PHY drivers populate the function pointers in the helper
driver. So any phy_ops will first invoke the helper driver which will
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.
It seems that the file is too large for the tool. But still this makes a
false impression that only registers are different.
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
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've given the exact 'same'
comment at-least twice in this patch). There are quite a few examples in
the kernel where the same driver is used for multiple versions of the IP.
to be repeated. That is 240 times 4 (and surely more in the future, as
this patchset adds support for 4 SoCs). Which is almost 1000 lines more.
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.
+
+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");
+ return -EINVAL;
+ }
+
<snip>
.
.
+ 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 is wrong?
Do you really assign a char? Having a char data type and assigning an
integer is misleading.
Please read this paragraph from LDD3:
"Sometimes kernel code requires data items of a specific size,
perhaps 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.
Thanks
Kishon
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/