* [PATCH v10 4/8] arm: omap3: twl: add phy consumer data in twl4030_usb_data
From: Kishon Vijay Abraham I @ 2013-07-26 12:54 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1374842582-13242-1-git-send-email-kishon@ti.com>
The PHY framework uses the phy consumer data populated in platform data in the
case of non-dt boot to return the reference to the PHY when the controller
(PHY consumer) requests for it. So populated the phy consumer data in the platform
data of twl usb.
Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
arch/arm/mach-omap2/twl-common.c | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/arch/arm/mach-omap2/twl-common.c b/arch/arm/mach-omap2/twl-common.c
index c05898f..b0d54da 100644
--- a/arch/arm/mach-omap2/twl-common.c
+++ b/arch/arm/mach-omap2/twl-common.c
@@ -24,6 +24,7 @@
#include <linux/i2c/twl.h>
#include <linux/gpio.h>
#include <linux/string.h>
+#include <linux/phy/phy.h>
#include <linux/regulator/machine.h>
#include <linux/regulator/fixed.h>
@@ -90,8 +91,18 @@ void __init omap_pmic_late_init(void)
}
#if defined(CONFIG_ARCH_OMAP3)
+struct phy_consumer consumers[] = {
+ PHY_CONSUMER("musb-hdrc.0", "usb"),
+};
+
+struct phy_init_data init_data = {
+ .consumers = consumers,
+ .num_consumers = ARRAY_SIZE(consumers),
+};
+
static struct twl4030_usb_data omap3_usb_pdata = {
.usb_mode = T2_USB_MODE_ULPI,
+ .init_data = &init_data,
};
static int omap3_batt_table[] = {
--
1.7.10.4
^ permalink raw reply related
* [PATCH v10 5/8] ARM: dts: omap: update usb_otg_hs data
From: Kishon Vijay Abraham I @ 2013-07-26 12:54 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1374842582-13242-1-git-send-email-kishon@ti.com>
Updated the usb_otg_hs dt data to include the *phy* and *phy-names*
binding in order for the driver to use the new generic PHY framework.
Also updated the Documentation to include the binding information.
The PHY binding information can be found at
Documentation/devicetree/bindings/phy/phy-bindings.txt
Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
Acked-by: Felipe Balbi <balbi@ti.com>
Reviewed-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
---
Documentation/devicetree/bindings/usb/omap-usb.txt | 5 +++++
Documentation/devicetree/bindings/usb/usb-phy.txt | 6 ++++++
arch/arm/boot/dts/omap3-beagle-xm.dts | 2 ++
arch/arm/boot/dts/omap3-evm.dts | 2 ++
arch/arm/boot/dts/omap3-overo.dtsi | 2 ++
arch/arm/boot/dts/omap4.dtsi | 3 +++
arch/arm/boot/dts/twl4030.dtsi | 1 +
7 files changed, 21 insertions(+)
diff --git a/Documentation/devicetree/bindings/usb/omap-usb.txt b/Documentation/devicetree/bindings/usb/omap-usb.txt
index 57e71f6..825790d 100644
--- a/Documentation/devicetree/bindings/usb/omap-usb.txt
+++ b/Documentation/devicetree/bindings/usb/omap-usb.txt
@@ -19,6 +19,9 @@ OMAP MUSB GLUE
- power : Should be "50". This signifies the controller can supply up to
100mA when operating in host mode.
- usb-phy : the phandle for the PHY device
+ - phys : the phandle for the PHY device (used by generic PHY framework)
+ - phy-names : the names of the PHY corresponding to the PHYs present in the
+ *phy* phandle.
Optional properties:
- ctrl-module : phandle of the control module this glue uses to write to
@@ -33,6 +36,8 @@ usb_otg_hs: usb_otg_hs@4a0ab000 {
num-eps = <16>;
ram-bits = <12>;
ctrl-module = <&omap_control_usb>;
+ phys = <&usb2_phy>;
+ phy-names = "usb2-phy";
};
Board specific device node entry
diff --git a/Documentation/devicetree/bindings/usb/usb-phy.txt b/Documentation/devicetree/bindings/usb/usb-phy.txt
index 61496f5..c0245c8 100644
--- a/Documentation/devicetree/bindings/usb/usb-phy.txt
+++ b/Documentation/devicetree/bindings/usb/usb-phy.txt
@@ -5,6 +5,8 @@ OMAP USB2 PHY
Required properties:
- compatible: Should be "ti,omap-usb2"
- reg : Address and length of the register set for the device.
+ - #phy-cells: determine the number of cells that should be given in the
+ phandle while referencing this phy.
Optional properties:
- ctrl-module : phandle of the control module used by PHY driver to power on
@@ -16,6 +18,7 @@ usb2phy@4a0ad080 {
compatible = "ti,omap-usb2";
reg = <0x4a0ad080 0x58>;
ctrl-module = <&omap_control_usb>;
+ #phy-cells = <0>;
};
OMAP USB3 PHY
@@ -25,6 +28,8 @@ Required properties:
- reg : Address and length of the register set for the device.
- reg-names: The names of the register addresses corresponding to the registers
filled in "reg".
+ - #phy-cells: determine the number of cells that should be given in the
+ phandle while referencing this phy.
Optional properties:
- ctrl-module : phandle of the control module used by PHY driver to power on
@@ -39,4 +44,5 @@ usb3phy@4a084400 {
<0x4a084c00 0x40>;
reg-names = "phy_rx", "phy_tx", "pll_ctrl";
ctrl-module = <&omap_control_usb>;
+ #phy-cells = <0>;
};
diff --git a/arch/arm/boot/dts/omap3-beagle-xm.dts b/arch/arm/boot/dts/omap3-beagle-xm.dts
index afdb164..533b2da 100644
--- a/arch/arm/boot/dts/omap3-beagle-xm.dts
+++ b/arch/arm/boot/dts/omap3-beagle-xm.dts
@@ -144,6 +144,8 @@
&usb_otg_hs {
interface-type = <0>;
usb-phy = <&usb2_phy>;
+ phys = <&usb2_phy>;
+ phy-names = "usb2-phy";
mode = <3>;
power = <50>;
};
diff --git a/arch/arm/boot/dts/omap3-evm.dts b/arch/arm/boot/dts/omap3-evm.dts
index 7d4329d..4134dd0 100644
--- a/arch/arm/boot/dts/omap3-evm.dts
+++ b/arch/arm/boot/dts/omap3-evm.dts
@@ -70,6 +70,8 @@
&usb_otg_hs {
interface-type = <0>;
usb-phy = <&usb2_phy>;
+ phys = <&usb2_phy>;
+ phy-names = "usb2-phy";
mode = <3>;
power = <50>;
};
diff --git a/arch/arm/boot/dts/omap3-overo.dtsi b/arch/arm/boot/dts/omap3-overo.dtsi
index 8f1abec..a461d2f 100644
--- a/arch/arm/boot/dts/omap3-overo.dtsi
+++ b/arch/arm/boot/dts/omap3-overo.dtsi
@@ -76,6 +76,8 @@
&usb_otg_hs {
interface-type = <0>;
usb-phy = <&usb2_phy>;
+ phys = <&usb2_phy>;
+ phy-names = "usb2-phy";
mode = <3>;
power = <50>;
};
diff --git a/arch/arm/boot/dts/omap4.dtsi b/arch/arm/boot/dts/omap4.dtsi
index 22d9f2b..1e8e2fe 100644
--- a/arch/arm/boot/dts/omap4.dtsi
+++ b/arch/arm/boot/dts/omap4.dtsi
@@ -520,6 +520,7 @@
compatible = "ti,omap-usb2";
reg = <0x4a0ad080 0x58>;
ctrl-module = <&omap_control_usb>;
+ #phy-cells = <0>;
};
};
@@ -658,6 +659,8 @@
interrupt-names = "mc", "dma";
ti,hwmods = "usb_otg_hs";
usb-phy = <&usb2_phy>;
+ phys = <&usb2_phy>;
+ phy-names = "usb2-phy";
multipoint = <1>;
num-eps = <16>;
ram-bits = <12>;
diff --git a/arch/arm/boot/dts/twl4030.dtsi b/arch/arm/boot/dts/twl4030.dtsi
index b3034da..ce4cd6f 100644
--- a/arch/arm/boot/dts/twl4030.dtsi
+++ b/arch/arm/boot/dts/twl4030.dtsi
@@ -80,6 +80,7 @@
usb1v8-supply = <&vusb1v8>;
usb3v1-supply = <&vusb3v1>;
usb_mode = <1>;
+ #phy-cells = <0>;
};
twl_pwm: pwm {
--
1.7.10.4
^ permalink raw reply related
* [PATCH v10 6/8] usb: musb: omap2430: use the new generic PHY framework
From: Kishon Vijay Abraham I @ 2013-07-26 12:55 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1374842582-13242-1-git-send-email-kishon@ti.com>
Use the generic PHY framework API to get the PHY. The usb_phy_set_resume
and usb_phy_set_suspend is replaced with power_on and
power_off to align with the new PHY framework.
musb->xceiv can't be removed as of now because musb core uses xceiv.state and
xceiv.otg. Once there is a separate state machine to handle otg, these can be
moved out of xceiv and then we can start using the generic PHY framework.
Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
Reviewed-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Acked-by: Felipe Balbi <balbi@ti.com>
---
drivers/usb/musb/Kconfig | 1 +
drivers/usb/musb/musb_core.h | 2 ++
drivers/usb/musb/omap2430.c | 26 ++++++++++++++++++++------
3 files changed, 23 insertions(+), 6 deletions(-)
diff --git a/drivers/usb/musb/Kconfig b/drivers/usb/musb/Kconfig
index 797e3fd..25e2e57 100644
--- a/drivers/usb/musb/Kconfig
+++ b/drivers/usb/musb/Kconfig
@@ -76,6 +76,7 @@ config USB_MUSB_TUSB6010
config USB_MUSB_OMAP2PLUS
tristate "OMAP2430 and onwards"
depends on ARCH_OMAP2PLUS
+ select GENERIC_PHY
config USB_MUSB_AM35X
tristate "AM35x"
diff --git a/drivers/usb/musb/musb_core.h b/drivers/usb/musb/musb_core.h
index 7d341c3..6e567bd 100644
--- a/drivers/usb/musb/musb_core.h
+++ b/drivers/usb/musb/musb_core.h
@@ -46,6 +46,7 @@
#include <linux/usb.h>
#include <linux/usb/otg.h>
#include <linux/usb/musb.h>
+#include <linux/phy/phy.h>
struct musb;
struct musb_hw_ep;
@@ -346,6 +347,7 @@ struct musb {
u16 int_tx;
struct usb_phy *xceiv;
+ struct phy *phy;
int nIrq;
unsigned irq_wake:1;
diff --git a/drivers/usb/musb/omap2430.c b/drivers/usb/musb/omap2430.c
index 6708a3b..f7b33f4 100644
--- a/drivers/usb/musb/omap2430.c
+++ b/drivers/usb/musb/omap2430.c
@@ -348,11 +348,21 @@ static int omap2430_musb_init(struct musb *musb)
* up through ULPI. TWL4030-family PMICs include one,
* which needs a driver, drivers aren't always needed.
*/
- if (dev->parent->of_node)
+ if (dev->parent->of_node) {
+ musb->phy = devm_phy_get(dev->parent, "usb2-phy");
+
+ /* We can't totally remove musb->xceiv as of now because
+ * musb core uses xceiv.state and xceiv.otg. Once we have
+ * a separate state machine to handle otg, these can be moved
+ * out of xceiv and then we can start using the generic PHY
+ * framework
+ */
musb->xceiv = devm_usb_get_phy_by_phandle(dev->parent,
"usb-phy", 0);
- else
+ } else {
musb->xceiv = devm_usb_get_phy_dev(dev, 0);
+ musb->phy = devm_phy_get(dev, "usb");
+ }
if (IS_ERR(musb->xceiv)) {
status = PTR_ERR(musb->xceiv);
@@ -364,6 +374,10 @@ static int omap2430_musb_init(struct musb *musb)
return -EPROBE_DEFER;
}
+ if (IS_ERR(musb->phy)) {
+ pr_err("HS USB OTG: no PHY configured\n");
+ return PTR_ERR(musb->phy);
+ }
musb->isr = omap2430_musb_interrupt;
status = pm_runtime_get_sync(dev);
@@ -397,7 +411,7 @@ static int omap2430_musb_init(struct musb *musb)
if (glue->status != OMAP_MUSB_UNKNOWN)
omap_musb_set_mailbox(glue);
- usb_phy_init(musb->xceiv);
+ phy_init(musb->phy);
pm_runtime_put_noidle(musb->controller);
return 0;
@@ -460,6 +474,7 @@ static int omap2430_musb_exit(struct musb *musb)
del_timer_sync(&musb_idle_timer);
omap2430_low_level_exit(musb);
+ phy_exit(musb->phy);
return 0;
}
@@ -633,7 +648,7 @@ static int omap2430_runtime_suspend(struct device *dev)
OTG_INTERFSEL);
omap2430_low_level_exit(musb);
- usb_phy_set_suspend(musb->xceiv, 1);
+ phy_power_off(musb->phy);
}
return 0;
@@ -648,8 +663,7 @@ static int omap2430_runtime_resume(struct device *dev)
omap2430_low_level_init(musb);
musb_writel(musb->mregs, OTG_INTERFSEL,
musb->context.otg_interfsel);
-
- usb_phy_set_suspend(musb->xceiv, 0);
+ phy_power_on(musb->phy);
}
return 0;
--
1.7.10.4
^ permalink raw reply related
* [PATCH v10 7/8] usb: phy: omap-usb2: remove *set_suspend* callback from omap-usb2
From: Kishon Vijay Abraham I @ 2013-07-26 12:55 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1374842582-13242-1-git-send-email-kishon@ti.com>
Now that omap-usb2 is adapted to the new generic PHY framework,
*set_suspend* ops can be removed from omap-usb2 driver.
Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
Acked-by: Felipe Balbi <balbi@ti.com>
Reviewed-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
---
drivers/phy/phy-omap-usb2.c | 25 -------------------------
1 file changed, 25 deletions(-)
diff --git a/drivers/phy/phy-omap-usb2.c b/drivers/phy/phy-omap-usb2.c
index 25e0f3c..dec3fab 100644
--- a/drivers/phy/phy-omap-usb2.c
+++ b/drivers/phy/phy-omap-usb2.c
@@ -97,29 +97,6 @@ static int omap_usb_set_peripheral(struct usb_otg *otg,
return 0;
}
-static int omap_usb2_suspend(struct usb_phy *x, int suspend)
-{
- u32 ret;
- struct omap_usb *phy = phy_to_omapusb(x);
-
- if (suspend && !phy->is_suspended) {
- omap_control_usb_phy_power(phy->control_dev, 0);
- pm_runtime_put_sync(phy->dev);
- phy->is_suspended = 1;
- } else if (!suspend && phy->is_suspended) {
- ret = pm_runtime_get_sync(phy->dev);
- if (ret < 0) {
- dev_err(phy->dev, "get_sync failed with err %d\n",
- ret);
- return ret;
- }
- omap_control_usb_phy_power(phy->control_dev, 1);
- phy->is_suspended = 0;
- }
-
- return 0;
-}
-
static int omap_usb_power_off(struct phy *x)
{
struct omap_usb *phy = phy_get_drvdata(x);
@@ -167,7 +144,6 @@ static int omap_usb2_probe(struct platform_device *pdev)
phy->phy.dev = phy->dev;
phy->phy.label = "omap-usb2";
- phy->phy.set_suspend = omap_usb2_suspend;
phy->phy.otg = otg;
phy->phy.type = USB_PHY_TYPE_USB2;
@@ -182,7 +158,6 @@ static int omap_usb2_probe(struct platform_device *pdev)
return -ENODEV;
}
- phy->is_suspended = 1;
omap_control_usb_phy_power(phy->control_dev, 0);
otg->set_host = omap_usb_set_host;
--
1.7.10.4
^ permalink raw reply related
* [PATCH v10 8/8] usb: phy: twl4030-usb: remove *set_suspend* and *phy_init* ops
From: Kishon Vijay Abraham I @ 2013-07-26 12:55 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1374842582-13242-1-git-send-email-kishon@ti.com>
Now that twl4030-usb is adapted to the new generic PHY framework,
*set_suspend* and *phy_init* ops can be removed from twl4030-usb driver.
Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
Acked-by: Felipe Balbi <balbi@ti.com>
Reviewed-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
---
drivers/phy/phy-twl4030-usb.c | 57 ++++++++++-------------------------------
1 file changed, 13 insertions(+), 44 deletions(-)
diff --git a/drivers/phy/phy-twl4030-usb.c b/drivers/phy/phy-twl4030-usb.c
index 494f107..c437211 100644
--- a/drivers/phy/phy-twl4030-usb.c
+++ b/drivers/phy/phy-twl4030-usb.c
@@ -422,25 +422,20 @@ static void twl4030_phy_power(struct twl4030_usb *twl, int on)
}
}
-static void twl4030_phy_suspend(struct twl4030_usb *twl, int controller_off)
+static int twl4030_phy_power_off(struct phy *phy)
{
+ struct twl4030_usb *twl = phy_get_drvdata(phy);
+
if (twl->asleep)
- return;
+ return 0;
twl4030_phy_power(twl, 0);
twl->asleep = 1;
dev_dbg(twl->dev, "%s\n", __func__);
-}
-
-static int twl4030_phy_power_off(struct phy *phy)
-{
- struct twl4030_usb *twl = phy_get_drvdata(phy);
-
- twl4030_phy_suspend(twl, 0);
return 0;
}
-static void __twl4030_phy_resume(struct twl4030_usb *twl)
+static void __twl4030_phy_power_on(struct twl4030_usb *twl)
{
twl4030_phy_power(twl, 1);
twl4030_i2c_access(twl, 1);
@@ -449,11 +444,13 @@ static void __twl4030_phy_resume(struct twl4030_usb *twl)
twl4030_i2c_access(twl, 0);
}
-static void twl4030_phy_resume(struct twl4030_usb *twl)
+static int twl4030_phy_power_on(struct phy *phy)
{
+ struct twl4030_usb *twl = phy_get_drvdata(phy);
+
if (!twl->asleep)
- return;
- __twl4030_phy_resume(twl);
+ return 0;
+ __twl4030_phy_power_on(twl);
twl->asleep = 0;
dev_dbg(twl->dev, "%s\n", __func__);
@@ -466,13 +463,6 @@ static void twl4030_phy_resume(struct twl4030_usb *twl)
cancel_delayed_work(&twl->id_workaround_work);
schedule_delayed_work(&twl->id_workaround_work, HZ);
}
-}
-
-static int twl4030_phy_power_on(struct phy *phy)
-{
- struct twl4030_usb *twl = phy_get_drvdata(phy);
-
- twl4030_phy_resume(twl);
return 0;
}
@@ -604,9 +594,9 @@ static void twl4030_id_workaround_work(struct work_struct *work)
}
}
-static int twl4030_usb_phy_init(struct usb_phy *phy)
+static int twl4030_phy_init(struct phy *phy)
{
- struct twl4030_usb *twl = phy_to_twl(phy);
+ struct twl4030_usb *twl = phy_get_drvdata(phy);
enum omap_musb_vbus_id_status status;
/*
@@ -621,32 +611,13 @@ static int twl4030_usb_phy_init(struct usb_phy *phy)
if (status = OMAP_MUSB_ID_GROUND || status = OMAP_MUSB_VBUS_VALID) {
omap_musb_mailbox(twl->linkstat);
- twl4030_phy_resume(twl);
+ twl4030_phy_power_on(phy);
}
sysfs_notify(&twl->dev->kobj, NULL, "vbus");
return 0;
}
-static int twl4030_phy_init(struct phy *phy)
-{
- struct twl4030_usb *twl = phy_get_drvdata(phy);
-
- return twl4030_usb_phy_init(&twl->phy);
-}
-
-static int twl4030_set_suspend(struct usb_phy *x, int suspend)
-{
- struct twl4030_usb *twl = phy_to_twl(x);
-
- if (suspend)
- twl4030_phy_suspend(twl, 1);
- else
- twl4030_phy_resume(twl);
-
- return 0;
-}
-
static int twl4030_set_peripheral(struct usb_otg *otg,
struct usb_gadget *gadget)
{
@@ -719,8 +690,6 @@ static int twl4030_usb_probe(struct platform_device *pdev)
twl->phy.label = "twl4030";
twl->phy.otg = otg;
twl->phy.type = USB_PHY_TYPE_USB2;
- twl->phy.set_suspend = twl4030_set_suspend;
- twl->phy.init = twl4030_usb_phy_init;
otg->phy = &twl->phy;
otg->set_host = twl4030_set_host;
--
1.7.10.4
^ permalink raw reply related
* Re: [RFC 0/1] drm/pl111: Initial drm/kms driver for pl111
From: Pawel Moll @ 2013-07-26 14:06 UTC (permalink / raw)
To: Rob Clark, Laurent Pinchart
Cc: linux-fbdev@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
dri-devel@lists.freedesktop.org, Tom Cooksey
In-Reply-To: <CAF6AEGtspnhSGNM4_QQubVfOkZ1Gh1-Z3iyHOLBPVWuqRy81ew@mail.gmail.com>
On Thu, 2013-07-25 at 19:21 +0100, Rob Clark wrote:
> > * Only supports 640x480 mode, which is hard-coded. We intend to
> > rebase on top of CDF once it is merged, which hopefully will
> > handle a lot of the EDID parsing & mode setting for us (once
> > Pawel's CDF patches for VExpress also land).
>
> note that drm core already handles EDID parsing quite nicely.. I'm not
> entirely sure why CDF would be needed for this?
>
> What is the dependency on CDF? Is there an external encoder/bridge
> that could be shared between platforms?
By all means. The platform we have here at ARM - Versatile Express - has
a pretty generic idea of "multimedia bus", carrying a set of RGB pixel
data and digital audio. There are three possible source of those: an
FPGA on the motherboard and two daughterboards which can take random
combination of FPGAs or test chips. In other words: the pixel data can
be generated by anything. And some of those things can be driven by
existing fbdev drivers. Now Tom is trying to make the most modern driver
for the oldest of the things ;-)
So that's about sources. All of them are connected to yet another FPGA
which acts as a muxer (switch if you wish). And from there things are
fed to SiI9022 HDMI/DVI formatter which is pretty common in the
"embedded world". In other words: loads of other fbdev-driven display
controllers are driving them as well (googling for sii9022 reveals at
least 3 different more or less custom drivers for it). And this chip is
also a reason Tom mentioned EDID. In order to get (just access, not even
parse) it we must jump through hoops - the chip acts as a I2C master
itself and must be kindly asked to switch into a sort of I2C bridge
mode.
So yes, the display pipeline is exactly straight-forward...
> btw, I think that having some share-able KMS objects is a good idea.
> I'm not entirely sure that the directions that the current CDF
> proposals are headed is necessarily the right way forward. I'd prefer
> to see small/incremental evolution of KMS (ie. add drm_bridge and
> drm_panel, and refactor the existing encoder-slave). Keeping it
> inside drm means that we can evolve it more easily, and avoid layers
> of glue code for no good reason.
To keep the story short - I'm a great enthusiast of CDF because it
caters for both DRM *and* fbdev. Today. I'm looking forward to the day
when the last fbdev driver is kicked off the kernel, but it doesn't look
like happening soon. At the same time I couldn't care less about naming,
so if you prefer to call it drm_panel (but keep the API abstract enough
so non-DRM drivers can use them as well) - it's fine for me :-) There
are already generic mode/timing structures and DT bidnings available
(with helpers for the interesting parties), so this doesn't seem like a
unreasonable request?
Cheers!
Pawel
^ permalink raw reply
* Re: [RFC 0/1] drm/pl111: Initial drm/kms driver for pl111
From: Russell King - ARM Linux @ 2013-07-26 14:14 UTC (permalink / raw)
To: Rob Clark
Cc: dri-devel, linux-fbdev, pawel.moll, linux-arm-kernel, tom.cooksey
In-Reply-To: <CAF6AEGtspnhSGNM4_QQubVfOkZ1Gh1-Z3iyHOLBPVWuqRy81ew@mail.gmail.com>
On Thu, Jul 25, 2013 at 02:21:59PM -0400, Rob Clark wrote:
> On Thu, Jul 25, 2013 at 1:17 PM, <tom.cooksey@arm.com> wrote:
> > Known issues:
> > * It uses KDS. We intend to switch to whatever implicit per-buffer
> > synchronisation mechanism gets merged, once something is merged.
> > * It abuses flags parameter of DRM_IOCTL_MODE_CREATE_DUMB to also
> > allocate buffers for the GPU. Still not sure how to resolve this
> > as we don't use DRM for our GPU driver.
>
> any thoughts/plans about a DRM GPU driver? Ideally long term (esp.
> once the dma-fence stuff is in place), we'd have gpu-specific drm
> (gpu-only, no kms) driver, and SoC/display specific drm/kms driver,
> using prime/dmabuf to share between the two.
The PL111 primecell is just a scanout device. It has no GPU embedded
in it. It's just like the Armada/Dove DRM stuff - if a GPU is provided
it's an entirely separate piece of IP, and will likely remain a separate
non-DRM driver.
In the case of PL111, PL111 is not manufacturer specific so the GPU can
be any standalone GPU.
^ permalink raw reply
* Re: [RFC 0/1] drm/pl111: Initial drm/kms driver for pl111
From: Rob Clark @ 2013-07-26 14:21 UTC (permalink / raw)
To: Pawel Moll
Cc: linux-arm-kernel@lists.infradead.org, linux-fbdev@vger.kernel.org,
Laurent Pinchart, dri-devel@lists.freedesktop.org
In-Reply-To: <1374847564.3213.70.camel@hornet>
On Fri, Jul 26, 2013 at 10:06 AM, Pawel Moll <pawel.moll@arm.com> wrote:
> On Thu, 2013-07-25 at 19:21 +0100, Rob Clark wrote:
>> > * Only supports 640x480 mode, which is hard-coded. We intend to
>> > rebase on top of CDF once it is merged, which hopefully will
>> > handle a lot of the EDID parsing & mode setting for us (once
>> > Pawel's CDF patches for VExpress also land).
>>
>> note that drm core already handles EDID parsing quite nicely.. I'm not
>> entirely sure why CDF would be needed for this?
>>
>> What is the dependency on CDF? Is there an external encoder/bridge
>> that could be shared between platforms?
>
> By all means. The platform we have here at ARM - Versatile Express - has
> a pretty generic idea of "multimedia bus", carrying a set of RGB pixel
> data and digital audio. There are three possible source of those: an
> FPGA on the motherboard and two daughterboards which can take random
> combination of FPGAs or test chips. In other words: the pixel data can
> be generated by anything. And some of those things can be driven by
> existing fbdev drivers. Now Tom is trying to make the most modern driver
> for the oldest of the things ;-)
>
> So that's about sources. All of them are connected to yet another FPGA
> which acts as a muxer (switch if you wish). And from there things are
> fed to SiI9022 HDMI/DVI formatter which is pretty common in the
> "embedded world". In other words: loads of other fbdev-driven display
> controllers are driving them as well (googling for sii9022 reveals at
> least 3 different more or less custom drivers for it). And this chip is
> also a reason Tom mentioned EDID. In order to get (just access, not even
> parse) it we must jump through hoops - the chip acts as a I2C master
> itself and must be kindly asked to switch into a sort of I2C bridge
> mode.
btw, you might want to have a look at some of the existing i2c slave
encoders, since that sounds a lot like what the si9022 is. There is
already drivers/gpu/drm/i2c/sil164_drv.c for some other SI part. And
tda998x is another more recently added hdmi encoder/formatter (the
edid probing code in there might serve as a reasonable source of
inspiration)
> So yes, the display pipeline is exactly straight-forward...
>
>> btw, I think that having some share-able KMS objects is a good idea.
>> I'm not entirely sure that the directions that the current CDF
>> proposals are headed is necessarily the right way forward. I'd prefer
>> to see small/incremental evolution of KMS (ie. add drm_bridge and
>> drm_panel, and refactor the existing encoder-slave). Keeping it
>> inside drm means that we can evolve it more easily, and avoid layers
>> of glue code for no good reason.
>
> To keep the story short - I'm a great enthusiast of CDF because it
> caters for both DRM *and* fbdev. Today. I'm looking forward to the day
> when the last fbdev driver is kicked off the kernel, but it doesn't look
> like happening soon. At the same time I couldn't care less about naming,
> so if you prefer to call it drm_panel (but keep the API abstract enough
> so non-DRM drivers can use them as well) - it's fine for me :-) There
> are already generic mode/timing structures and DT bidnings available
> (with helpers for the interesting parties), so this doesn't seem like a
> unreasonable request?
Well, if you have something complex enough to benefit from CDF, then
you probably ought to be looking at drm/kms. If someone wants to
somehow come up with some glue to re-use shareable drm/kms components
from fbdev, well.. I guess I don't really care about what goes in
fbdev. My main concern is that we have more than enough work to do,
and have more than enough complexity. If we end up w/ bunch of glue
to tie things in to drm properties, how drm helpers sequence things
during modeset, locking, etc.. I just don't see that turning out well.
And also, just on the logistical side, having something shared across
subsystems makes changes more of a pain during the merge window. Not
to mention backports to stable kernels, distro kernels, etc. It all
just seems like it will be a lot of unnecessary headache.
> Cheers!
>
> Pawel
>
>
^ permalink raw reply
* Re: [RFC 0/1] drm/pl111: Initial drm/kms driver for pl111
From: Rob Clark @ 2013-07-26 14:24 UTC (permalink / raw)
To: Russell King - ARM Linux
Cc: dri-devel, linux-fbdev, pawel.moll, linux-arm-kernel, tom.cooksey
In-Reply-To: <20130726141430.GQ24642@n2100.arm.linux.org.uk>
On Fri, Jul 26, 2013 at 10:14 AM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Thu, Jul 25, 2013 at 02:21:59PM -0400, Rob Clark wrote:
>> On Thu, Jul 25, 2013 at 1:17 PM, <tom.cooksey@arm.com> wrote:
>> > Known issues:
>> > * It uses KDS. We intend to switch to whatever implicit per-buffer
>> > synchronisation mechanism gets merged, once something is merged.
>> > * It abuses flags parameter of DRM_IOCTL_MODE_CREATE_DUMB to also
>> > allocate buffers for the GPU. Still not sure how to resolve this
>> > as we don't use DRM for our GPU driver.
>>
>> any thoughts/plans about a DRM GPU driver? Ideally long term (esp.
>> once the dma-fence stuff is in place), we'd have gpu-specific drm
>> (gpu-only, no kms) driver, and SoC/display specific drm/kms driver,
>> using prime/dmabuf to share between the two.
>
> The PL111 primecell is just a scanout device. It has no GPU embedded
> in it. It's just like the Armada/Dove DRM stuff - if a GPU is provided
> it's an entirely separate piece of IP, and will likely remain a separate
> non-DRM driver.
>
> In the case of PL111, PL111 is not manufacturer specific so the GPU can
> be any standalone GPU.
Oh, yeah, I know PL111 is different block independent from GPU. I
wasn't advocating adding mali support in this driver. But instead
creating a mali DRM driver which had GEM and gpu bits, but not KMS.
Then buffer sharing w/ prime/dmabuf just like you would, for example
on a laptop w/ both integrated graphics and discrete GPU.
BR,
-R
^ permalink raw reply
* Re: [RFC 0/1] drm/pl111: Initial drm/kms driver for pl111
From: Pawel Moll @ 2013-07-26 14:41 UTC (permalink / raw)
To: Rob Clark
Cc: linux-arm-kernel@lists.infradead.org, linux-fbdev@vger.kernel.org,
Laurent Pinchart, dri-devel@lists.freedesktop.org, Tom Cooksey
In-Reply-To: <CAF6AEGtNEOpuKqcr7vfieCx0mot44L2bZzOt4ipcmUVv4FwUVw@mail.gmail.com>
On Fri, 2013-07-26 at 15:21 +0100, Rob Clark wrote:
> Well, if you have something complex enough to benefit from CDF, then
> you probably ought to be looking at drm/kms.
That's what we're doing - hope you appreciate Tom's effort of re-writing
a driver for something that pre-dates DRM/KMS by generations ;-)
> I guess I don't really care about what goes in fbdev.
Well, I've guessed that ;-)
> My main concern is that we have more than enough work to do,
> and have more than enough complexity. If we end up w/ bunch of glue
> to tie things in to drm properties, how drm helpers sequence things
> during modeset, locking, etc.. I just don't see that turning out well.
> And also, just on the logistical side, having something shared across
> subsystems makes changes more of a pain during the merge window. Not
> to mention backports to stable kernels, distro kernels, etc. It all
> just seems like it will be a lot of unnecessary headache.
I won't argue that, however it seems to me that CDF is such a small
variable in the equation that it would disappear in the noise. Surely
you have (or will have) standard DRM interface to drive the display
pipeline where CDF would be one of many back-ends? In the end you'll
have to solve the same problem - how to represent a random pipeline in a
generic way so different components can be plugged one into another.
Anyway, I'm not planning to stick my nose between DRM and CDF, really.
I'm just hoping for a solution that will fit my needs :-)
Pawel
^ permalink raw reply
* Re: [RFC 0/1] drm/pl111: Initial drm/kms driver for pl111
From: Daniel Vetter @ 2013-07-26 18:56 UTC (permalink / raw)
To: Tom Cooksey
Cc: dri-devel, 'Rob Clark', Pawel Moll, linux-fbdev,
linux-arm-kernel
In-Reply-To: <51f29cd1.e686440a.66b2.fffff9d5SMTPIN_ADDED_BROKEN@mx.google.com>
On Fri, Jul 26, 2013 at 04:58:55PM +0100, Tom Cooksey wrote:
> Hi Rob,
>
> > > * It abuses flags parameter of DRM_IOCTL_MODE_CREATE_DUMB to also
> > > allocate buffers for the GPU. Still not sure how to resolve this
> > > as we don't use DRM for our GPU driver.
> >
> > any thoughts/plans about a DRM GPU driver? Ideally long term (esp.
> > once the dma-fence stuff is in place), we'd have gpu-specific drm
> > (gpu-only, no kms) driver, and SoC/display specific drm/kms driver,
> > using prime/dmabuf to share between the two.
>
> The "extra" buffers we were allocating from armsoc DDX were really
> being allocated through DRM/GEM so we could get an flink name
> for them and pass a reference to them back to our GPU driver on
> the client side. If it weren't for our need to access those
> extra off-screen buffers with the GPU we wouldn't need to
> allocate them with DRM at all. So, given they are really "GPU"
> buffers, it does absolutely make sense to allocate them in a
> different driver to the display driver.
>
> However, to avoid unnecessary memcpys & related cache
> maintenance ops, we'd also like the GPU to render into buffers
> which are scanned out by the display controller. So let's say
> we continue using DRM_IOCTL_MODE_CREATE_DUMB to allocate scan
> out buffers with the display's DRM driver but a custom ioctl
> on the GPU's DRM driver to allocate non scanout, off-screen
> buffers. Sounds great, but I don't think that really works
> with DRI2. If we used two drivers to allocate buffers, which
> of those drivers do we return in DRI2ConnectReply? Even if we
> solve that somehow, GEM flink names are name-spaced to a
> single device node (AFAIK). So when we do a DRI2GetBuffers,
> how does the EGL in the client know which DRM device owns GEM
> flink name "1234"? We'd need some pretty dirty hacks.
I don't know the details, but having different gem driver nodes is exactly
what prime support in X allows. The X server then passes the buffer object
between the different ddx drivers using dma-buf sharing.
So if we have two drm drivers, one a kms-only scanout driver and one a
gem-only mali driver, then the userspace gl mali driver would only ever
talk to the mali drm node. So flink for DRI2 would use that node
exclusively. The scanout drm node would then be treated like e.g. an usb
display-link device which is also display-only.
> So then we looked at allocating _all_ buffers with the GPU's
> DRM driver. That solves the DRI2 single-device-name and single
> name-space issue. It also means the GPU would _never_ render
> into buffers allocated through DRM_IOCTL_MODE_CREATE_DUMB.
> One thing I wasn't sure about is if there was an objection
> to using PRIME to export scanout buffers allocated with
> DRM_IOCTL_MODE_CREATE_DUMB and then importing them into a GPU
> driver to be rendered into? Is that a concern?
Imo sharing dumb buffers should be ok. The "dumb" concept is only really
relevant when the display and render part are integrated into one IP block
and driver.
> Anyway, that latter case also gets quite difficult. The "GPU"
> DRM driver would need to know the constraints of the display
> controller when allocating buffers intended to be scanned out.
> For example, pl111 typically isn't behind an IOMMU and so
> requires physically contiguous memory. We'd have to teach the
> GPU's DRM driver about the constraints of the display HW. Not
> exactly a clean driver model. :-(
Well the current dma-buf sharing code essentially only really works on x86
where everyone has a decent graphics tt and no cache flushing is required.
For ARM I expect that we need to have a common dma-buf backing storage
layer which walks all attached devices on the dma-buf, checks allocation
constraints and then picks a suitable pool to allocate the buffer.
Since dma-bufs should only be allocated once they're getting used (and not
when establishing the sharing) that should work out. But with the current
dma apis exposed to drivers that's not possible really. Essentially we
need ion but please not expose the heaps explicitly to userspace. The
kernel already knows all this, so could take care of this for userspace
without breaking the current dma api abstractions we have.
Of course if you start to share buffers between different userspace drives
they need to talk to each another about what stride/pixel layout/tiling is
suitable. Since I'm a kernel guy I'll punt on this problem ;-)
> I'm still a little stuck on how to proceed, so any ideas
> would greatly appreciated! My current train of thought is
> having a kind of SoC-specific DRM driver which allocates
> buffers for both display and GPU within a single GEM
> namespace. That SoC-specific DRM driver could then know the
> constraints of both the GPU and the display HW. We could then
> use PRIME to export buffers allocated with the SoC DRM driver
> and import them into the GPU and/or display DRM driver.
That's pretty much how ION works and I'm not in favour of leaking
allocation constraints to userspace like that ...
> Note: While it doesn't use the DRM framework, the Mali T6xx
> kernel driver has supported importing buffers through dma_buf
> for some time. I've even written an EGL extension :-):
>
> <http://www.khronos.org/registry/egl/extensions/EXT/EGL_EXT_image_dma_buf_im
> port.txt>
>
>
> > I'm not entirely sure that the directions that the current CDF
> > proposals are headed is necessarily the right way forward. I'd prefer
> > to see small/incremental evolution of KMS (ie. add drm_bridge and
> > drm_panel, and refactor the existing encoder-slave). Keeping it
> > inside drm means that we can evolve it more easily, and avoid layers
> > of glue code for no good reason.
>
> I think CDF could allow vendors to re-use code they've written
> for their Android driver stack in DRM drivers more easily. Though
> I guess ideally KMS would evolve to a point where it could be used
> by an Android driver stack. I.e. Support explicit fences.
Just fyi with intel we have some DSI/MIPI support patches floating around.
Our plan is to just merge them and then once we have a 2nd drm driver with
DSI support grow some common infrastructure out of this.
The same approach seems to work neatly for hdmi (infoframes) and dp
(although the dp helpers can be seriously extended).
Cheers, Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply
* [PATCH] video: remove unused variable dev
From: Kefeng Wang @ 2013-07-27 10:05 UTC (permalink / raw)
To: Jean-Christophe Plagniol-Villard, Tomi Valkeinen, linux-fbdev,
linux-kernel
Cc: Jingoo Han, guohanjun
Due to commit: e21d2170f [video: remove unnecessary platform_set_drvdata()],
variable dev is unused, so remove it.
Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
---
drivers/video/vga16fb.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/video/vga16fb.c b/drivers/video/vga16fb.c
index 830ded4..2827333 100644
--- a/drivers/video/vga16fb.c
+++ b/drivers/video/vga16fb.c
@@ -1265,7 +1265,6 @@ static void vga16fb_imageblit(struct fb_info *info, const struct fb_image *image
static void vga16fb_destroy(struct fb_info *info)
{
- struct platform_device *dev = container_of(info->device, struct platform_device, dev);
iounmap(info->screen_base);
fb_dealloc_cmap(&info->cmap);
/* XXX unshare VGA regions */
--
1.8.2.2
^ permalink raw reply related
* Re: [PATCH] video: remove unused variable dev
From: Jingoo Han @ 2013-07-29 0:52 UTC (permalink / raw)
To: 'Kefeng Wang', 'Tomi Valkeinen'
Cc: 'Jean-Christophe Plagniol-Villard', linux-fbdev,
linux-kernel, guohanjun, 'Luis Henriques', Jingoo Han
In-Reply-To: <1374919513-15356-1-git-send-email-wangkefeng.wang@huawei.com>
On Saturday, July 27, 2013 7:05 PM, Kefeng Wang wrote:
>
> Due to commit: e21d2170f [video: remove unnecessary platform_set_drvdata()],
> variable dev is unused, so remove it.
>
> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
> ---
> drivers/video/vga16fb.c | 1 -
> 1 file changed, 1 deletion(-)
CC'ed Luis Henriques.
Hi Kefeng Wang,
The same patch was submitted by Luis Henriques 2 weeks ago.
Also, the patch was already applied by Tomi Valkeinen.
Best regards,
Jingoo Han
>
> diff --git a/drivers/video/vga16fb.c b/drivers/video/vga16fb.c
> index 830ded4..2827333 100644
> --- a/drivers/video/vga16fb.c
> +++ b/drivers/video/vga16fb.c
> @@ -1265,7 +1265,6 @@ static void vga16fb_imageblit(struct fb_info *info, const struct fb_image *image
>
> static void vga16fb_destroy(struct fb_info *info)
> {
> - struct platform_device *dev = container_of(info->device, struct platform_device, dev);
> iounmap(info->screen_base);
> fb_dealloc_cmap(&info->cmap);
> /* XXX unshare VGA regions */
> --
> 1.8.2.2
^ permalink raw reply
* Re: [PATCH] video: remove unused variable dev
From: Kefeng Wang @ 2013-07-29 1:27 UTC (permalink / raw)
To: Jingoo Han
Cc: 'Tomi Valkeinen',
'Jean-Christophe Plagniol-Villard', linux-fbdev,
linux-kernel, guohanjun, 'Luis Henriques'
In-Reply-To: <009601ce8bf5$e88bfda0$b9a3f8e0$@samsung.com>
On 07/29 8:52, Jingoo Han wrote:
> On Saturday, July 27, 2013 7:05 PM, Kefeng Wang wrote:
>>
>> Due to commit: e21d2170f [video: remove unnecessary platform_set_drvdata()],
>> variable dev is unused, so remove it.
>>
>> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
>> ---
>> drivers/video/vga16fb.c | 1 -
>> 1 file changed, 1 deletion(-)
>
> CC'ed Luis Henriques.
>
>
> Hi Kefeng Wang,
>
> The same patch was submitted by Luis Henriques 2 weeks ago.
> Also, the patch was already applied by Tomi Valkeinen.
>
> Best regards,
> Jingoo Han
>
Thanks for your reply, I got it, so ignore it.
>>
>> diff --git a/drivers/video/vga16fb.c b/drivers/video/vga16fb.c
>> index 830ded4..2827333 100644
>> --- a/drivers/video/vga16fb.c
>> +++ b/drivers/video/vga16fb.c
>> @@ -1265,7 +1265,6 @@ static void vga16fb_imageblit(struct fb_info *info, const struct fb_image *image
>>
>> static void vga16fb_destroy(struct fb_info *info)
>> {
>> - struct platform_device *dev = container_of(info->device, struct platform_device, dev);
>> iounmap(info->screen_base);
>> fb_dealloc_cmap(&info->cmap);
>> /* XXX unshare VGA regions */
>> --
>> 1.8.2.2
>
>
>
>
^ permalink raw reply
* RE: [PATCH v2 0/5] ARM: vf610: Add DCU framebuffer driver for Vybrid VF610 platform
From: Wang Huan-B18965 @ 2013-07-29 6:13 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <81BA6E5E0BC2344391CABCEE22D1B6D83AA3AE@039-SN1MPN1-004.039d.mgd.msft.net>
Hi, Jean-Christophe, Tomi,
Do you have any comments for these patches?
Thanks!
Best Regards,
Alison Wang
> -----Original Message-----
> From: linux-arm-kernel [mailto:linux-arm-kernel-
> bounces@lists.infradead.org] On Behalf Of Wang Huan-B18965
> Sent: Friday, July 19, 2013 11:49 AM
> To: plagnioj@jcrosoft.com; tomi.valkeinen@ti.com; shawn.guo@linaro.org;
> Estevam Fabio-R49496; linux-fbdev@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org
> Cc: Jin Zhengxiong-R64188
> Subject: RE: [PATCH v2 0/5] ARM: vf610: Add DCU framebuffer driver for
> Vybrid VF610 platform
>
> Hi, Jean-Christophe,
>
> Could you please help to review these patches?
>
> Thanks a lot!
>
>
> Best Regards,
> Alison Wang
>
> > -----Original Message-----
> > From: linux-arm-kernel [mailto:linux-arm-kernel-
> > bounces@lists.infradead.org] On Behalf Of Alison Wang
> > Sent: Friday, July 12, 2013 2:08 PM
> > To: plagnioj@jcrosoft.com; tomi.valkeinen@ti.com;
> > shawn.guo@linaro.org; Estevam Fabio-R49496;
> > linux-fbdev@vger.kernel.org; linux-arm- kernel@lists.infradead.org
> > Cc: Jin Zhengxiong-R64188
> > Subject: [PATCH v2 0/5] ARM: vf610: Add DCU framebuffer driver for
> > Vybrid VF610 platform
> >
> > This series contain DCU framebuffer driver for Freescale Vybrid VF610
> > platform.
> >
> > The Display Controller Unit (DCU) module is a system master that
> > fetches graphics stored in internal or external memory and displays
> > them on a TFT LCD panel. A wide range of panel sizes is supported and
> > the timing of the interface signals is highly configurable.
> > Graphics are read directly from memory and then blended in real-time,
> > which allows for dynamic content creation with minimal CPU
> intervention.
> >
> > The features:
> >
> > (1) Full RGB888 output to TFT LCD panel.
> > (2) For the current LCD panel, WQVGA "480x272" is tested.
> > (3) Blending of each pixel using up to 4 source layers dependent on
> > size of panel.
> > (4) Each graphic layer can be placed with one pixel resolution in
> > either axis.
> > (5) Each graphic layer support RGB565 and RGB888 direct colors
> without
> > alpha channel and BGRA8888 direct colors with an alpha channel.
> > (6) Each graphic layer support alpha blending with 8-bit resolution.
> >
> > Changes in v2:
> > - Add a document for DCU framebuffer driver under
> > Documentation/devicetree/bindings/fb/.
> >
> > ----------------------------------------------------------------
> > Alison Wang (5):
> > ARM: dts: vf610: Add DCU and TCON nodes
> > ARM: dts: vf610-twr: Enable DCU and TCON devices
> > ARM: clk: vf610: Add DCU and TCON clock support
> > fb: Add DCU framebuffer driver for Vybrid VF610 platform
> > Documentation: DT: Add DCU framebuffer driver
> >
> > Documentation/devicetree/bindings/fb/fsl-dcu-fb.txt | 36 ++++
> > arch/arm/boot/dts/vf610-twr.dts | 10 +
> > arch/arm/boot/dts/vf610.dtsi | 19 +-
> > arch/arm/mach-imx/clk-vf610.c | 5 +
> > drivers/video/Kconfig | 9 +
> > drivers/video/Makefile | 1 +
> > drivers/video/fsl-dcu-fb.c | 1091
> >
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > +++++++++++++++++++++++++
> > include/dt-bindings/clock/vf610-clock.h | 3 +-
> > 8 files changed, 1172 insertions(+), 2 deletions(-) create mode
> > 100644 Documentation/devicetree/bindings/fb/fsl-dcu-fb.txt
> > create mode 100644 drivers/video/fsl-dcu-fb.c
> >
> >
> >
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 4/5] fb: Add DCU framebuffer driver for Vybrid VF610 platform
From: Sascha Hauer @ 2013-07-29 11:14 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1373609276-14566-5-git-send-email-b18965@freescale.com>
On Fri, Jul 12, 2013 at 02:07:55PM +0800, Alison Wang wrote:
> The Display Controller Unit (DCU) module is a system master that
> fetches graphics stored in internal or external memory and displays
> them on a TFT LCD panel. A wide range of panel sizes is supported
> and the timing of the interface signals is highly configurable.
> Graphics are read directly from memory and then blended in real-time,
> which allows for dynamic content creation with minimal CPU intervention.
Only a review of the code inline.
Maybe the real question is whether we want to introduce another
framebuffer driver at all instead of making it a DRM driver.
> +
> +#define DRIVER_NAME "fsl-dcu-fb"
> +
> +#define DCU_DCU_MODE 0x0010
> +#define DCU_MODE_BLEND_ITER(x) (x << 20)
What's the result of DCU_MODE_BLEND_ITER(1 + 1)?
> +static struct fb_videomode dcu_mode_db[] = {
> + {
> + .name = "480x272",
> + .refresh = 75,
> + .xres = 480,
> + .yres = 272,
> + .pixclock = 91996,
> + .left_margin = 2,
> + .right_margin = 2,
> + .upper_margin = 1,
> + .lower_margin = 1,
> + .hsync_len = 41,
> + .vsync_len = 2,
> + .sync = FB_SYNC_COMP_HIGH_ACT | FB_SYNC_VERT_HIGH_ACT,
> + .vmode = FB_VMODE_NONINTERLACED,
> + },
> +};
We have ways to describe a panel in dt. Use them.
> +
> +static struct mfb_info mfb_template[] = {
> + {
> + .index = LAYER0,
> + .id = "Layer0",
> + .alpha = 0xff,
> + .blend = 0,
> + .count = 0,
> + .x_layer_d = 0,
> + .y_layer_d = 0,
> + },
Wrong indentation.
> + default:
> + printk(KERN_ERR "unsupported color depth: %u\n",
> + var->bits_per_pixel);
Use dev_* for printing messages in drivers.
> +static int fsl_dcu_check_var(struct fb_var_screeninfo *var,
> + struct fb_info *info)
> +{
> + if (var->xres_virtual < var->xres)
> + var->xres_virtual = var->xres;
> + if (var->yres_virtual < var->yres)
> + var->yres_virtual = var->yres;
> +
> + if (var->xoffset < 0)
> + var->xoffset = 0;
> +
> + if (var->yoffset < 0)
> + var->yoffset = 0;
Ever seen an unsigned value going below zero?
> + default:
> + printk(KERN_ERR "unsupported color depth: %u\n",
> + var->bits_per_pixel);
BUG(). This can't happen since you make that sure above.
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +static int calc_div_ratio(struct fb_info *info)
> +{
Use a consistent namespace for function names (fsl_dcu_)
> + writel(DCU_THRESHOLD_LS_BF_VS(0x3) | DCU_THRESHOLD_OUT_BUF_HIGH(0x78) |
> + DCU_THRESHOLD_OUT_BUF_LOW(0), dcufb->reg_base + DCU_THRESHOLD);
> +
> + enable_controller(info);
> +}
Make your functions symmetric. If there's update_controller(), the
function should do exactly that, it should *not* enable the controller.
Call this from the users of this function if necessary.
> + if (copy_to_user(buf, &alpha, sizeof(alpha)))
> + return -EFAULT;
> + break;
> + case MFB_SET_ALPHA:
> + if (copy_from_user(&alpha, buf, sizeof(alpha)))
> + return -EFAULT;
> + mfbi->blend = 1;
> + mfbi->alpha = alpha;
> + fsl_dcu_set_par(info);
> + break;
Is it still state of the art to introduce ioctls in the fb framework
without any kind of documentation?
> + default:
> + printk(KERN_ERR "unknown ioctl command (0x%08X)\n", cmd);
What shall a reader of the kernel log do with a message like this? It's
utterly useless when he doesn't even now which device failed here. Just
drop this.
> +static void enable_interrupts(struct dcu_fb_data *dcufb)
> +{
> + u32 int_mask = readl(dcufb->reg_base + DCU_INT_MASK);
> +
> + writel(int_mask & ~DCU_INT_MASK_UNDRUN, dcufb->reg_base + DCU_INT_MASK);
> +}
Inline this code where you need it. Introducing a function for a single
register write seems quite useless.
> +static int install_framebuffer(struct fb_info *info)
> +{
> + struct mfb_info *mfbi = info->par;
> + struct fb_videomode *db = dcu_mode_db;
> + unsigned int dbsize = ARRAY_SIZE(dcu_mode_db);
> + int ret;
> +
> + info->var.activate = FB_ACTIVATE_NOW;
> + info->fbops = &fsl_dcu_ops;
> + info->flags = FBINFO_FLAG_DEFAULT;
> + info->pseudo_palette = &mfbi->pseudo_palette;
> +
> + fb_alloc_cmap(&info->cmap, 16, 0);
> +
> + ret = fb_find_mode(&info->var, info, fb_mode, db, dbsize,
> + NULL, default_bpp);
> +
> + if (fsl_dcu_check_var(&info->var, info)) {
> + ret = -EINVAL;
Propagate the error.
> + goto failed_checkvar;
> + }
> +
> + if (register_framebuffer(info) < 0) {
> + ret = -EINVAL;
ditto
> +static irqreturn_t fsl_dcu_irq(int irq, void *dev_id)
> +{
> + struct dcu_fb_data *dcufb = dev_id;
> + unsigned int status = readl(dcufb->reg_base + DCU_INT_STATUS);
> + u32 dcu_mode;
> +
> + if (status) {
Save indendation level by bailing out early.
> + if (status & DCU_INT_STATUS_UNDRUN) {
> + dcu_mode = readl(dcufb->reg_base + DCU_DCU_MODE);
> + dcu_mode &= ~DCU_MODE_DCU_MODE_MASK;
> + writel(dcu_mode | DCU_MODE_DCU_MODE(DCU_MODE_OFF),
> + dcufb->reg_base + DCU_DCU_MODE);
> + udelay(1);
> + writel(dcu_mode | DCU_MODE_DCU_MODE(DCU_MODE_NORMAL),
> + dcufb->reg_base + DCU_DCU_MODE);
> + }
> + writel(status, dcufb->reg_base + DCU_INT_STATUS);
> + return IRQ_HANDLED;
> + }
> + return IRQ_NONE;
> +}
> +
> + if (IS_ERR(tcon_clk)) {
> + ret = PTR_ERR(tcon_clk);
> + goto failed_getclock;
> + }
> + clk_prepare_enable(tcon_clk);
> +
> + tcon_reg = of_iomap(tcon_np, 0);
Use devm_*
> + dcufb->irq = platform_get_irq(pdev, 0);
> + if (!dcufb->irq) {
> + ret = -EINVAL;
> + goto failed_getirq;
> + }
> +
> + ret = request_irq(dcufb->irq, fsl_dcu_irq, 0, DRIVER_NAME, dcufb);
Use devm_request_irq
> + if (ret) {
> + dev_err(&pdev->dev, "could not request irq\n");
> + goto failed_requestirq;
> + }
> +
> + /* Put TCON in bypass mode, so the input signals from DCU are passed
> + * through TCON unchanged */
> + ret = bypass_tcon(pdev->dev.of_node);
> + if (ret) {
> + dev_err(&pdev->dev, "could not bypass TCON\n");
> + goto failed_bypasstcon;
> + }
> +
> + dcufb->clk = devm_clk_get(&pdev->dev, "dcu");
> + if (IS_ERR(dcufb->clk)) {
> + dev_err(&pdev->dev, "could not get clock\n");
> + goto failed_getclock;
You will return 0 here.
> + }
> + clk_prepare_enable(dcufb->clk);
> +
> + for (i = 0; i < ARRAY_SIZE(dcufb->fsl_dcu_info); i++) {
> + dcufb->fsl_dcu_info[i] > + framebuffer_alloc(sizeof(struct mfb_info), &pdev->dev);
> + if (!dcufb->fsl_dcu_info[i]) {
> + ret = ENOMEM;
-ENOMEM
> +failed_alloc_framebuffer:
> +failed_getclock:
> +failed_bypasstcon:
> + free_irq(dcufb->irq, dcufb);
> +failed_requestirq:
> +failed_getirq:
> + iounmap(dcufb->reg_base);
You used devm_ioremap, so drop this.
> +failed_ioremap:
> + kfree(dcufb);
This is allocated with devm_*. Drop this.
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply
* Re: [RFC 0/1] drm/pl111: Initial drm/kms driver for pl111
From: Rob Clark @ 2013-07-29 14:58 UTC (permalink / raw)
To: Tom Cooksey; +Cc: linux-arm-kernel, linux-fbdev, Pawel Moll, dri-devel
In-Reply-To: <51f29ccd.f014b40a.34cc.ffffca2aSMTPIN_ADDED_BROKEN@mx.google.com>
On Fri, Jul 26, 2013 at 11:58 AM, Tom Cooksey <tom.cooksey@arm.com> wrote:
> Hi Rob,
>
>> > * It abuses flags parameter of DRM_IOCTL_MODE_CREATE_DUMB to also
>> > allocate buffers for the GPU. Still not sure how to resolve this
>> > as we don't use DRM for our GPU driver.
>>
>> any thoughts/plans about a DRM GPU driver? Ideally long term (esp.
>> once the dma-fence stuff is in place), we'd have gpu-specific drm
>> (gpu-only, no kms) driver, and SoC/display specific drm/kms driver,
>> using prime/dmabuf to share between the two.
>
> The "extra" buffers we were allocating from armsoc DDX were really
> being allocated through DRM/GEM so we could get an flink name
> for them and pass a reference to them back to our GPU driver on
> the client side. If it weren't for our need to access those
> extra off-screen buffers with the GPU we wouldn't need to
> allocate them with DRM at all. So, given they are really "GPU"
> buffers, it does absolutely make sense to allocate them in a
> different driver to the display driver.
>
> However, to avoid unnecessary memcpys & related cache
> maintenance ops, we'd also like the GPU to render into buffers
> which are scanned out by the display controller. So let's say
> we continue using DRM_IOCTL_MODE_CREATE_DUMB to allocate scan
> out buffers with the display's DRM driver but a custom ioctl
> on the GPU's DRM driver to allocate non scanout, off-screen
> buffers. Sounds great, but I don't think that really works
> with DRI2. If we used two drivers to allocate buffers, which
> of those drivers do we return in DRI2ConnectReply? Even if we
> solve that somehow, GEM flink names are name-spaced to a
> single device node (AFAIK). So when we do a DRI2GetBuffers,
> how does the EGL in the client know which DRM device owns GEM
> flink name "1234"? We'd need some pretty dirty hacks.
You would return the name of the display driver allocating the
buffers. On the client side you can use generic ioctls to go from
flink -> handle -> dmabuf. So the client side would end up opening
both the display drm device and the gpu, but without needing to know
too much about the display.
(Probably in (for example) mesa there needs to be a bit of work to
handle this better, but I think that would be needed as well for
sharing between, say, nouveau and udl displaylink driver.. which is
really the same scenario.)
> So then we looked at allocating _all_ buffers with the GPU's
> DRM driver. That solves the DRI2 single-device-name and single
> name-space issue. It also means the GPU would _never_ render
> into buffers allocated through DRM_IOCTL_MODE_CREATE_DUMB.
Well, I think we can differentiate between shared buffers and internal
buffers (textures, vertex upload, etc, etc).
For example, in mesa/gallium driver there are two paths to getting a
buffer... ->resource_create() and ->resource_from_handle(), the
latter is the path you go for dri2 buffers shared w/ x11. The former
is buffers that are just internal to gpu (if !(bind &
PIPE_BIND_SHARED)). I guess you must have something similar in mali
driver.
> One thing I wasn't sure about is if there was an objection
> to using PRIME to export scanout buffers allocated with
> DRM_IOCTL_MODE_CREATE_DUMB and then importing them into a GPU
> driver to be rendered into? Is that a concern?
well.. it wasn't quite the original intention of the 'dumb' ioctls.
But I guess in the end if you hand the gpu a buffer with a
layout/format that it can't grok, then gpu does a staging texture plus
copy. If you had, for example, some setup w/ gpu that could only
render to tiled format, plus a display that could scanout that format,
then your DDX driver would need to allocate the dri2 buffers with
something other than dumb ioctl. (But at that point, you probably
need to implement your own EXA so you could hook in your own custom
upload-to/download-from screen fxns for sw fallbacks, so you're not
talking about using generic DDX anyways.)
> Anyway, that latter case also gets quite difficult. The "GPU"
> DRM driver would need to know the constraints of the display
> controller when allocating buffers intended to be scanned out.
> For example, pl111 typically isn't behind an IOMMU and so
> requires physically contiguous memory. We'd have to teach the
> GPU's DRM driver about the constraints of the display HW. Not
> exactly a clean driver model. :-(
>
> I'm still a little stuck on how to proceed, so any ideas
> would greatly appreciated! My current train of thought is
> having a kind of SoC-specific DRM driver which allocates
> buffers for both display and GPU within a single GEM
> namespace. That SoC-specific DRM driver could then know the
> constraints of both the GPU and the display HW. We could then
> use PRIME to export buffers allocated with the SoC DRM driver
> and import them into the GPU and/or display DRM driver.
Usually if the display drm driver is allocating the buffers that might
be scanned out, it just needs to have minimal knowledge of the GPU
(pitch alignment constraints). I don't think we need a 3rd device
just to allocate buffers.
Really I don't think the separate display drm and gpu drm device is
much different from desktop udl/displaylink case. Or desktop
integrated-gpu + external gpu.
> Note: While it doesn't use the DRM framework, the Mali T6xx
> kernel driver has supported importing buffers through dma_buf
> for some time. I've even written an EGL extension :-):
>
> <http://www.khronos.org/registry/egl/extensions/EXT/EGL_EXT_image_dma_buf_im
> port.txt>
>
>
>> I'm not entirely sure that the directions that the current CDF
>> proposals are headed is necessarily the right way forward. I'd prefer
>> to see small/incremental evolution of KMS (ie. add drm_bridge and
>> drm_panel, and refactor the existing encoder-slave). Keeping it
>> inside drm means that we can evolve it more easily, and avoid layers
>> of glue code for no good reason.
>
> I think CDF could allow vendors to re-use code they've written
> for their Android driver stack in DRM drivers more easily. Though
> I guess ideally KMS would evolve to a point where it could be used
> by an Android driver stack. I.e. Support explicit fences.
>
yeah, the best would be evolving DRM/KMS to the point that it meets
android requirements. Because I really don't think android has any
special requirements compared to, say, a wayland compositor. (Other
than the way they do synchronization. But that doesn't really *need*
to be different, as far as I can tell.) It would certainly be easier
if android dev's actually participated in upstream linux graphics, and
talked about what they needed, and maybe even contributed a patch or
two. But that is a whole different topic.
BR,
-R
>
> Cheers,
>
> Tom
>
>
>
>
>
^ permalink raw reply
* RE: [RESEND PATCH v10 1/8] drivers: phy: add generic PHY framework
From: Kamil Debski @ 2013-07-29 15:28 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1374842963-13545-2-git-send-email-kishon@ti.com>
Hi Kishon,
A small fix follows inline.
> From: linux-media-owner@vger.kernel.org [mailto:linux-media-
> owner@vger.kernel.org] On Behalf Of Kishon Vijay Abraham I
> Sent: Friday, July 26, 2013 2:49 PM
>
> The PHY framework provides a set of APIs for the PHY drivers to
> create/destroy a PHY and APIs for the PHY users to obtain a reference
> to the PHY with or without using phandle. For dt-boot, the PHY drivers
> should also register *PHY provider* with the framework.
>
> PHY drivers should create the PHY by passing id and ops like init, exit,
> power_on and power_off. This framework is also pm runtime enabled.
>
> The documentation for the generic PHY framework is added in
> Documentation/phy.txt and the documentation for dt binding can be found
> at Documentation/devicetree/bindings/phy/phy-bindings.txt
>
> Cc: Tomasz Figa <t.figa@samsung.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> Acked-by: Felipe Balbi <balbi@ti.com>
> Tested-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> ---
> .../devicetree/bindings/phy/phy-bindings.txt | 66 ++
> Documentation/phy.txt | 166 +++++
> MAINTAINERS | 8 +
> drivers/Kconfig | 2 +
> drivers/Makefile | 2 +
> drivers/phy/Kconfig | 18 +
> drivers/phy/Makefile | 5 +
> drivers/phy/phy-core.c | 714
> ++++++++++++++++++++
> include/linux/phy/phy.h | 270 ++++++++
> 9 files changed, 1251 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/phy/phy-
> bindings.txt
> create mode 100644 Documentation/phy.txt create mode 100644
> drivers/phy/Kconfig create mode 100644 drivers/phy/Makefile create
> mode 100644 drivers/phy/phy-core.c create mode 100644
> include/linux/phy/phy.h
>
[snip]
> diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h new file
> mode 100644 index 0000000..e444b23
> --- /dev/null
> +++ b/include/linux/phy/phy.h
> @@ -0,0 +1,270 @@
[snip]
> +struct phy_init_data {
> + unsigned int num_consumers;
> + struct phy_consumer *consumers;
> +};
> +
> +#define PHY_CONSUMER(_dev_name, _port) \
> +{ \
> + .dev_name = _dev_name, \
> + .port = _port, \
> +}
> +
> +#define to_phy(dev) (container_of((dev), struct phy, dev))
> +
> +#define of_phy_provider_register(dev, xlate) \
> + __of_phy_provider_register((dev), THIS_MODULE, (xlate))
> +
> +#define devm_of_phy_provider_register(dev, xlate) \
> + __of_phy_provider_register((dev), THIS_MODULE, (xlate))
I think this should be:
+ __devm_of_phy_provider_register((dev), THIS_MODULE, (xlate))
Right?
> +
> +static inline void phy_set_drvdata(struct phy *phy, void *data) {
> + dev_set_drvdata(&phy->dev, data);
> +}
> +
> +static inline void *phy_get_drvdata(struct phy *phy) {
> + return dev_get_drvdata(&phy->dev);
> +}
> +
[snip]
Best wishes,
--
Kamil Debski
Linux Kernel Developer
Samsung R&D Institute Poland
^ permalink raw reply
* Re: [RESEND PATCH v10 1/8] drivers: phy: add generic PHY framework
From: Kishon Vijay Abraham I @ 2013-07-29 15:42 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <023801ce8c70$427cbd20$c7763760$%debski@samsung.com>
On Monday 29 July 2013 08:58 PM, Kamil Debski wrote:
> Hi Kishon,
>
> A small fix follows inline.
>
>> From: linux-media-owner@vger.kernel.org [mailto:linux-media-
>> owner@vger.kernel.org] On Behalf Of Kishon Vijay Abraham I
>> Sent: Friday, July 26, 2013 2:49 PM
>>
>> The PHY framework provides a set of APIs for the PHY drivers to
>> create/destroy a PHY and APIs for the PHY users to obtain a reference
>> to the PHY with or without using phandle. For dt-boot, the PHY drivers
>> should also register *PHY provider* with the framework.
>>
>> PHY drivers should create the PHY by passing id and ops like init, exit,
>> power_on and power_off. This framework is also pm runtime enabled.
>>
>> The documentation for the generic PHY framework is added in
>> Documentation/phy.txt and the documentation for dt binding can be found
>> at Documentation/devicetree/bindings/phy/phy-bindings.txt
>>
>> Cc: Tomasz Figa <t.figa@samsung.com>
>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>> Acked-by: Felipe Balbi <balbi@ti.com>
>> Tested-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
>> ---
>> .../devicetree/bindings/phy/phy-bindings.txt | 66 ++
>> Documentation/phy.txt | 166 +++++
>> MAINTAINERS | 8 +
>> drivers/Kconfig | 2 +
>> drivers/Makefile | 2 +
>> drivers/phy/Kconfig | 18 +
>> drivers/phy/Makefile | 5 +
>> drivers/phy/phy-core.c | 714
>> ++++++++++++++++++++
>> include/linux/phy/phy.h | 270 ++++++++
>> 9 files changed, 1251 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/phy/phy-
>> bindings.txt
>> create mode 100644 Documentation/phy.txt create mode 100644
>> drivers/phy/Kconfig create mode 100644 drivers/phy/Makefile create
>> mode 100644 drivers/phy/phy-core.c create mode 100644
>> include/linux/phy/phy.h
>>
>
> [snip]
>
>> diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h new file
>> mode 100644 index 0000000..e444b23
>> --- /dev/null
>> +++ b/include/linux/phy/phy.h
>> @@ -0,0 +1,270 @@
>
> [snip]
>
>> +struct phy_init_data {
>> + unsigned int num_consumers;
>> + struct phy_consumer *consumers;
>> +};
>> +
>> +#define PHY_CONSUMER(_dev_name, _port) \
>> +{ \
>> + .dev_name = _dev_name, \
>> + .port = _port, \
>> +}
>> +
>> +#define to_phy(dev) (container_of((dev), struct phy, dev))
>> +
>> +#define of_phy_provider_register(dev, xlate) \
>> + __of_phy_provider_register((dev), THIS_MODULE, (xlate))
>> +
>> +#define devm_of_phy_provider_register(dev, xlate) \
>> + __of_phy_provider_register((dev), THIS_MODULE, (xlate))
>
> I think this should be:
> + __devm_of_phy_provider_register((dev), THIS_MODULE, (xlate))
> Right?
right.. thanks for spotting this.
Regards
Kishon
^ permalink raw reply
* Re: [RESEND PATCH v10 1/8] drivers: phy: add generic PHY framework
From: Sylwester Nawrocki @ 2013-07-29 15:51 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1374842963-13545-2-git-send-email-kishon@ti.com>
On 07/26/2013 02:49 PM, Kishon Vijay Abraham I wrote:
> The PHY framework provides a set of APIs for the PHY drivers to
> create/destroy a PHY and APIs for the PHY users to obtain a reference to the
> PHY with or without using phandle. For dt-boot, the PHY drivers should
> also register *PHY provider* with the framework.
>
> PHY drivers should create the PHY by passing id and ops like init, exit,
> power_on and power_off. This framework is also pm runtime enabled.
>
> The documentation for the generic PHY framework is added in
> Documentation/phy.txt and the documentation for dt binding can be found at
> Documentation/devicetree/bindings/phy/phy-bindings.txt
>
> Cc: Tomasz Figa <t.figa@samsung.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> Acked-by: Felipe Balbi <balbi@ti.com>
> Tested-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> ---
> .../devicetree/bindings/phy/phy-bindings.txt | 66 ++
> Documentation/phy.txt | 166 +++++
> MAINTAINERS | 8 +
> drivers/Kconfig | 2 +
> drivers/Makefile | 2 +
> drivers/phy/Kconfig | 18 +
> drivers/phy/Makefile | 5 +
> drivers/phy/phy-core.c | 714 ++++++++++++++++++++
> include/linux/phy/phy.h | 270 ++++++++
> 9 files changed, 1251 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/phy/phy-bindings.txt
> create mode 100644 Documentation/phy.txt
> create mode 100644 drivers/phy/Kconfig
> create mode 100644 drivers/phy/Makefile
> create mode 100644 drivers/phy/phy-core.c
> create mode 100644 include/linux/phy/phy.h
>
> diff --git a/Documentation/devicetree/bindings/phy/phy-bindings.txt b/Documentation/devicetree/bindings/phy/phy-bindings.txt
> new file mode 100644
> index 0000000..8ae844f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/phy/phy-bindings.txt
> @@ -0,0 +1,66 @@
> +This document explains only the device tree data binding. For general
> +information about PHY subsystem refer to Documentation/phy.txt
[...]
> @@ -0,0 +1,166 @@
> + PHY SUBSYSTEM
> + Kishon Vijay Abraham I <kishon@ti.com>
> +
> +This document explains the Generic PHY Framework along with the APIs provided,
> +and how-to-use.
> +
> +1. Introduction
> +
> +*PHY* is the abbreviation for physical layer. It is used to connect a device
> +to the physical medium e.g., the USB controller has a PHY to provide functions
> +such as serialization, de-serialization, encoding, decoding and is responsible
> +for obtaining the required data transmission rate. Note that some USB
> +controllers have PHY functionality embedded into it and others use an external
> +PHY. Other peripherals that use PHY include Wireless LAN, Ethernet,
> +SATA etc.
> +
> +The intention of creating this framework is to bring the PHY drivers spread
> +all over the Linux kernel to drivers/phy to increase code re-use and for
> +better code maintainability.
> +
> +This framework will be of use only to devices that use external PHY (PHY
> +functionality is not embedded within the controller).
> +
> +2. Registering/Unregistering the PHY provider
> +
> +PHY provider refers to an entity that implements one or more PHY instances.
> +For the simple case where the PHY provider implements only a single instance of
> +the PHY, the framework provides its own implementation of of_xlate in
> +of_phy_simple_xlate. If the PHY provider implements multiple instances, it
> +should provide its own implementation of of_xlate. of_xlate is used only for
> +dt boot case.
> +
> +#define of_phy_provider_register(dev, xlate) \
> + __of_phy_provider_register((dev), THIS_MODULE, (xlate))
> +
> +#define devm_of_phy_provider_register(dev, xlate) \
> + __of_phy_provider_register((dev), THIS_MODULE, (xlate))
This needs to be:
__devm_of_phy_provider_register((dev), THIS_MODULE, (xlate))
as Kamil pointed out. We've tested it here with v9 and it makes
Bad Things happen. ;)
> +of_phy_provider_register and devm_of_phy_provider_register macros can be used to
> +register the phy_provider and it takes device and of_xlate as
> +arguments. For the dt boot case, all PHY providers should use one of the above
> +2 macros to register the PHY provider.
> +
> +void devm_of_phy_provider_unregister(struct device *dev,
> + struct phy_provider *phy_provider);
> +void of_phy_provider_unregister(struct phy_provider *phy_provider);
> +
> +devm_of_phy_provider_unregister and of_phy_provider_unregister can be used to
> +unregister the PHY.
> +
[...]
> diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
> new file mode 100644
> index 0000000..f1d15e5
> --- /dev/null
> +++ b/drivers/phy/phy-core.c
> @@ -0,0 +1,714 @@
[...]
> +static struct phy *phy_lookup(struct device *device, const char *port)
> +{
> + unsigned int count;
> + struct phy *phy;
> + struct device *dev;
> + struct phy_consumer *consumers;
> + struct class_dev_iter iter;
Don't you need something like
if (phy->init_data = NULL)
return ERR_PTR(-EINVAL);
to ensure there is no attempt to dereference NULL platform data ?
> + class_dev_iter_init(&iter, phy_class, NULL, NULL);
> + while ((dev = class_dev_iter_next(&iter))) {
> + phy = to_phy(dev);
> + count = phy->init_data->num_consumers;
> + consumers = phy->init_data->consumers;
> + while (count--) {
> + if (!strcmp(consumers->dev_name, dev_name(device)) &&
> + !strcmp(consumers->port, port)) {
> + class_dev_iter_exit(&iter);
> + return phy;
> + }
> + consumers++;
> + }
> + }
> +
> + class_dev_iter_exit(&iter);
> + return ERR_PTR(-ENODEV);
> +}
> +
[...]
> +int phy_init(struct phy *phy)
> +{
> + int ret;
> +
> + ret = phy_pm_runtime_get_sync(phy);
> + if (ret < 0 && ret != -ENOTSUPP)
> + return ret;
> +
> + mutex_lock(&phy->mutex);
> + if (phy->init_count++ = 0 && phy->ops->init) {
> + ret = phy->ops->init(phy);
> + if (ret < 0) {
> + dev_err(&phy->dev, "phy init failed --> %d\n", ret);
> + goto out;
Is this 'goto' and similar ones below really needed ?
> + }
> + }
> +
> +out:
> + mutex_unlock(&phy->mutex);
> + phy_pm_runtime_put(phy);
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(phy_init);
> +
> +int phy_exit(struct phy *phy)
> +{
> + int ret;
> +
> + ret = phy_pm_runtime_get_sync(phy);
> + if (ret < 0 && ret != -ENOTSUPP)
> + return ret;
> +
> + mutex_lock(&phy->mutex);
> + if (--phy->init_count = 0 && phy->ops->exit) {
> + ret = phy->ops->exit(phy);
> + if (ret < 0) {
> + dev_err(&phy->dev, "phy exit failed --> %d\n", ret);
> + goto out;
> + }
> + }
> +
> +out:
> + mutex_unlock(&phy->mutex);
> + phy_pm_runtime_put(phy);
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(phy_exit);
> +
> +int phy_power_on(struct phy *phy)
> +{
> + int ret = -ENOTSUPP;
> +
> + ret = phy_pm_runtime_get_sync(phy);
> + if (ret < 0 && ret != -ENOTSUPP)
> + return ret;
> +
> + mutex_lock(&phy->mutex);
> + if (phy->power_count++ = 0 && phy->ops->power_on) {
> + ret = phy->ops->power_on(phy);
> + if (ret < 0) {
> + dev_err(&phy->dev, "phy poweron failed --> %d\n", ret);
> + goto out;
> + }
> + }
> +
> +out:
> + mutex_unlock(&phy->mutex);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(phy_power_on);
> +
> +int phy_power_off(struct phy *phy)
> +{
> + int ret = -ENOTSUPP;
> +
> + mutex_lock(&phy->mutex);
> + if (--phy->power_count = 0 && phy->ops->power_off) {
> + ret = phy->ops->power_off(phy);
> + if (ret < 0) {
> + dev_err(&phy->dev, "phy poweroff failed --> %d\n", ret);
> + goto out;
> + }
> + }
> +
> +out:
> + mutex_unlock(&phy->mutex);
> + phy_pm_runtime_put(phy);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(phy_power_off);
--
Thanks,
Sylwester
^ permalink raw reply
* Re: [PATCH v2 6/9] ARM: msm: Move mach/board.h contents to common.h
From: David Brown @ 2013-07-29 20:20 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1374699274-18388-7-git-send-email-sboyd@codeaurora.org>
On Wed, Jul 24, 2013 at 01:54:31PM -0700, Stephen Boyd wrote:
>The contents of mach/board.h are only used by files within
>mach-msm so there is no need to export this file outside of the
>mach-msm directory. Move the contents of the file to common.h to
>allow us to compile MSM in the multi-platform kernel.
>
>Cc: Florian Tobias Schandinat <FlorianSchandinat@gmx.de>
>Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
>---
> drivers/video/msm/msm_fb.c | 1 -
It'd be nice to get a framebuffer Ack from this part.
original patch:
https://patchwork.kernel.org/patch/2833029/
David
--
sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation
^ permalink raw reply
* Re: [RESEND PATCH v10 1/8] drivers: phy: add generic PHY framework
From: Kishon Vijay Abraham I @ 2013-07-30 5:24 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <51F68F73.9060603@samsung.com>
Hi,
On Monday 29 July 2013 09:21 PM, Sylwester Nawrocki wrote:
> On 07/26/2013 02:49 PM, Kishon Vijay Abraham I wrote:
>> The PHY framework provides a set of APIs for the PHY drivers to
>> create/destroy a PHY and APIs for the PHY users to obtain a reference to the
>> PHY with or without using phandle. For dt-boot, the PHY drivers should
>> also register *PHY provider* with the framework.
>>
>> PHY drivers should create the PHY by passing id and ops like init, exit,
>> power_on and power_off. This framework is also pm runtime enabled.
>>
>> The documentation for the generic PHY framework is added in
>> Documentation/phy.txt and the documentation for dt binding can be found at
>> Documentation/devicetree/bindings/phy/phy-bindings.txt
>>
>> Cc: Tomasz Figa <t.figa@samsung.com>
>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>> Acked-by: Felipe Balbi <balbi@ti.com>
>> Tested-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
>> ---
>> .../devicetree/bindings/phy/phy-bindings.txt | 66 ++
>> Documentation/phy.txt | 166 +++++
>> MAINTAINERS | 8 +
>> drivers/Kconfig | 2 +
>> drivers/Makefile | 2 +
>> drivers/phy/Kconfig | 18 +
>> drivers/phy/Makefile | 5 +
>> drivers/phy/phy-core.c | 714 ++++++++++++++++++++
>> include/linux/phy/phy.h | 270 ++++++++
>> 9 files changed, 1251 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/phy/phy-bindings.txt
>> create mode 100644 Documentation/phy.txt
>> create mode 100644 drivers/phy/Kconfig
>> create mode 100644 drivers/phy/Makefile
>> create mode 100644 drivers/phy/phy-core.c
>> create mode 100644 include/linux/phy/phy.h
>>
>> diff --git a/Documentation/devicetree/bindings/phy/phy-bindings.txt b/Documentation/devicetree/bindings/phy/phy-bindings.txt
>> new file mode 100644
>> index 0000000..8ae844f
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/phy/phy-bindings.txt
>> @@ -0,0 +1,66 @@
>> +This document explains only the device tree data binding. For general
>> +information about PHY subsystem refer to Documentation/phy.txt
> [...]
>> @@ -0,0 +1,166 @@
>> + PHY SUBSYSTEM
>> + Kishon Vijay Abraham I <kishon@ti.com>
>> +
>> +This document explains the Generic PHY Framework along with the APIs provided,
>> +and how-to-use.
>> +
>> +1. Introduction
>> +
>> +*PHY* is the abbreviation for physical layer. It is used to connect a device
>> +to the physical medium e.g., the USB controller has a PHY to provide functions
>> +such as serialization, de-serialization, encoding, decoding and is responsible
>> +for obtaining the required data transmission rate. Note that some USB
>> +controllers have PHY functionality embedded into it and others use an external
>> +PHY. Other peripherals that use PHY include Wireless LAN, Ethernet,
>> +SATA etc.
>> +
>> +The intention of creating this framework is to bring the PHY drivers spread
>> +all over the Linux kernel to drivers/phy to increase code re-use and for
>> +better code maintainability.
>> +
>> +This framework will be of use only to devices that use external PHY (PHY
>> +functionality is not embedded within the controller).
>> +
>> +2. Registering/Unregistering the PHY provider
>> +
>> +PHY provider refers to an entity that implements one or more PHY instances.
>> +For the simple case where the PHY provider implements only a single instance of
>> +the PHY, the framework provides its own implementation of of_xlate in
>> +of_phy_simple_xlate. If the PHY provider implements multiple instances, it
>> +should provide its own implementation of of_xlate. of_xlate is used only for
>> +dt boot case.
>> +
>> +#define of_phy_provider_register(dev, xlate) \
>> + __of_phy_provider_register((dev), THIS_MODULE, (xlate))
>> +
>> +#define devm_of_phy_provider_register(dev, xlate) \
>> + __of_phy_provider_register((dev), THIS_MODULE, (xlate))
>
> This needs to be:
>
> __devm_of_phy_provider_register((dev), THIS_MODULE, (xlate))
>
> as Kamil pointed out. We've tested it here with v9 and it makes
> Bad Things happen. ;)
>
>> +of_phy_provider_register and devm_of_phy_provider_register macros can be used to
>> +register the phy_provider and it takes device and of_xlate as
>> +arguments. For the dt boot case, all PHY providers should use one of the above
>> +2 macros to register the PHY provider.
>> +
>> +void devm_of_phy_provider_unregister(struct device *dev,
>> + struct phy_provider *phy_provider);
>> +void of_phy_provider_unregister(struct phy_provider *phy_provider);
>> +
>> +devm_of_phy_provider_unregister and of_phy_provider_unregister can be used to
>> +unregister the PHY.
>> +
> [...]
>> diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
>> new file mode 100644
>> index 0000000..f1d15e5
>> --- /dev/null
>> +++ b/drivers/phy/phy-core.c
>> @@ -0,0 +1,714 @@
> [...]
>> +static struct phy *phy_lookup(struct device *device, const char *port)
>> +{
>> + unsigned int count;
>> + struct phy *phy;
>> + struct device *dev;
>> + struct phy_consumer *consumers;
>> + struct class_dev_iter iter;
>
> Don't you need something like
>
> if (phy->init_data = NULL)
> return ERR_PTR(-EINVAL);
>
> to ensure there is no attempt to dereference NULL platform data ?
hmm.. perhaps a dev_WARN too..
>
>> + class_dev_iter_init(&iter, phy_class, NULL, NULL);
>> + while ((dev = class_dev_iter_next(&iter))) {
>> + phy = to_phy(dev);
>> + count = phy->init_data->num_consumers;
>> + consumers = phy->init_data->consumers;
>> + while (count--) {
>> + if (!strcmp(consumers->dev_name, dev_name(device)) &&
>> + !strcmp(consumers->port, port)) {
>> + class_dev_iter_exit(&iter);
>> + return phy;
>> + }
>> + consumers++;
>> + }
>> + }
>> +
>> + class_dev_iter_exit(&iter);
>> + return ERR_PTR(-ENODEV);
>> +}
>> +
> [...]
>> +int phy_init(struct phy *phy)
>> +{
>> + int ret;
>> +
>> + ret = phy_pm_runtime_get_sync(phy);
>> + if (ret < 0 && ret != -ENOTSUPP)
>> + return ret;
>> +
>> + mutex_lock(&phy->mutex);
>> + if (phy->init_count++ = 0 && phy->ops->init) {
>> + ret = phy->ops->init(phy);
>> + if (ret < 0) {
>> + dev_err(&phy->dev, "phy init failed --> %d\n", ret);
>> + goto out;
>
> Is this 'goto' and similar ones below really needed ?
That's just to signify an error path.. it doesn't affect anyways ;-)
Thanks
Kishon
^ permalink raw reply
* Re: [PATCH v2 6/9] ARM: msm: Move mach/board.h contents to common.h
From: Tomi Valkeinen @ 2013-07-30 5:45 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20130729202031.GA20694@codeaurora.org>
[-- Attachment #1: Type: text/plain, Size: 736 bytes --]
On 29/07/13 23:20, David Brown wrote:
> On Wed, Jul 24, 2013 at 01:54:31PM -0700, Stephen Boyd wrote:
>> The contents of mach/board.h are only used by files within
>> mach-msm so there is no need to export this file outside of the
>> mach-msm directory. Move the contents of the file to common.h to
>> allow us to compile MSM in the multi-platform kernel.
>>
>> Cc: Florian Tobias Schandinat <FlorianSchandinat@gmx.de>
>> Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
>> ---
>> drivers/video/msm/msm_fb.c | 1 -
>
> It'd be nice to get a framebuffer Ack from this part.
>
> original patch:
> https://patchwork.kernel.org/patch/2833029/
Acked-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
Tom
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]
^ permalink raw reply
* [GIT PULL] Small fbdev fixes for 3.11-rc
From: Tomi Valkeinen @ 2013-07-30 6:31 UTC (permalink / raw)
To: Linus Torvalds
Cc: Jean-Christophe PLAGNIOL-VILLARD, linux-kernel, linux-fbdev
[-- Attachment #1: Type: text/plain, Size: 1469 bytes --]
Hi Linus,
The following changes since commit 3b2f64d00c46e1e4e9bd0bb9bb12619adac27a4b:
Linux 3.11-rc2 (2013-07-21 12:05:29 -0700)
are available in the git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/tomba/linux.git tags/fbdev-fixes-3.11-rc2
for you to fetch changes up to 7808e3291e1e101e9ad6e8263119c4a2abae05ef:
video: sh7760fb: fix to pass correct device identity to free_irq() (2013-07-29 11:25:03 +0300)
----------------------------------------------------------------
Small fbdev fixes
- Compile fixes
- atyfb initialization fix
- Fix freeing of the irq in sh7760fb & nuc900fb
----------------------------------------------------------------
Dan Carpenter (1):
fbdev/atyfb: fix recent breakage in correct_chipset()
Luis Henriques (1):
vga16fb: Remove unused variable
Michal Simek (1):
video: xilinxfb: Fix compilation warning
Tomi Valkeinen (1):
fbdev/sgivwfb: fix compilation error in sgivwfb_mmap()
Wei Yongjun (2):
video: nuc900fb: fix to pass correct device identity to request_irq()
video: sh7760fb: fix to pass correct device identity to free_irq()
drivers/video/aty/atyfb_base.c | 4 ++--
drivers/video/nuc900fb.c | 3 +--
drivers/video/sgivwfb.c | 2 +-
drivers/video/sh7760fb.c | 2 +-
drivers/video/vga16fb.c | 1 -
drivers/video/xilinxfb.c | 4 ++--
6 files changed, 7 insertions(+), 9 deletions(-)
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]
^ permalink raw reply
* Re: [PATCH 01/15] drivers: phy: add generic PHY framework
From: Felipe Balbi @ 2013-07-30 7:11 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20130721154653.GG16598@kroah.com>
[-- Attachment #1: Type: text/plain, Size: 6276 bytes --]
On Sun, Jul 21, 2013 at 08:46:53AM -0700, Greg KH wrote:
> On Sun, Jul 21, 2013 at 01:12:07PM +0200, Tomasz Figa wrote:
> > On Sunday 21 of July 2013 16:37:33 Kishon Vijay Abraham I wrote:
> > > Hi,
> > >
> > > On Sunday 21 July 2013 04:01 PM, Tomasz Figa wrote:
> > > > Hi,
> > > >
> > > > On Saturday 20 of July 2013 19:59:10 Greg KH wrote:
> > > >> On Sat, Jul 20, 2013 at 10:32:26PM -0400, Alan Stern wrote:
> > > >>> On Sat, 20 Jul 2013, Greg KH wrote:
> > > >>>>>>> That should be passed using platform data.
> > > >>>>>>
> > > >>>>>> Ick, don't pass strings around, pass pointers. If you have
> > > >>>>>> platform
> > > >>>>>> data you can get to, then put the pointer there, don't use a
> > > >>>>>> "name".
> > > >>>>>
> > > >>>>> I don't think I understood you here :-s We wont have phy pointer
> > > >>>>> when we create the device for the controller no?(it'll be done in
> > > >>>>> board file). Probably I'm missing something.
> > > >>>>
> > > >>>> Why will you not have that pointer? You can't rely on the "name"
> > > >>>> as
> > > >>>> the device id will not match up, so you should be able to rely on
> > > >>>> the pointer being in the structure that the board sets up, right?
> > > >>>>
> > > >>>> Don't use names, especially as ids can, and will, change, that is
> > > >>>> going
> > > >>>> to cause big problems. Use pointers, this is C, we are supposed to
> > > >>>> be
> > > >>>> doing that :)
> > > >>>
> > > >>> Kishon, I think what Greg means is this: The name you are using
> > > >>> must
> > > >>> be stored somewhere in a data structure constructed by the board
> > > >>> file,
> > > >>> right? Or at least, associated with some data structure somehow.
> > > >>> Otherwise the platform code wouldn't know which PHY hardware
> > > >>> corresponded to a particular name.
> > > >>>
> > > >>> Greg's suggestion is that you store the address of that data
> > > >>> structure
> > > >>> in the platform data instead of storing the name string. Have the
> > > >>> consumer pass the data structure's address when it calls phy_create,
> > > >>> instead of passing the name. Then you don't have to worry about two
> > > >>> PHYs accidentally ending up with the same name or any other similar
> > > >>> problems.
> > > >>
> > > >> Close, but the issue is that whatever returns from phy_create()
> > > >> should
> > > >> then be used, no need to call any "find" functions, as you can just
> > > >> use
> > > >> the pointer that phy_create() returns. Much like all other class api
> > > >> functions in the kernel work.
> > > >
> > > > I think there is a confusion here about who registers the PHYs.
> > > >
> > > > All platform code does is registering a platform/i2c/whatever device,
> > > > which causes a driver (located in drivers/phy/) to be instantiated.
> > > > Such drivers call phy_create(), usually in their probe() callbacks,
> > > > so platform_code has no way (and should have no way, for the sake of
> > > > layering) to get what phy_create() returns.
>
> Why not put pointers in the platform data structure that can hold these
> pointers? I thought that is why we created those structures in the
> first place. If not, what are they there for?
heh, IMO we shouldn't pass pointers of any kind through platform_data,
we want to pass data :-)
Allowing to pass pointers through that, is one of the reasons which got
us in such a big mess in ARM land, well it was much easier for a
board-file/driver writer to pass a function pointer then to create a
generic framework :-)
> > > > IMHO we need a lookup method for PHYs, just like for clocks,
> > > > regulators, PWMs or even i2c busses because there are complex cases
> > > > when passing just a name using platform data will not work. I would
> > > > second what Stephen said [1] and define a structure doing things in a
> > > > DT-like way.
> > > >
> > > > Example;
> > > >
> > > > [platform code]
> > > >
> > > > static const struct phy_lookup my_phy_lookup[] = {
> > > >
> > > > PHY_LOOKUP("s3c-hsotg.0", "otg", "samsung-usbphy.1", "phy.2"),
> > >
> > > The only problem here is that if *PLATFORM_DEVID_AUTO* is used while
> > > creating the device, the ids in the device name would change and
> > > PHY_LOOKUP wont be useful.
> >
> > I don't think this is a problem. All the existing lookup methods already
> > use ID to identify devices (see regulators, clkdev, PWMs, i2c, ...). You
> > can simply add a requirement that the ID must be assigned manually,
> > without using PLATFORM_DEVID_AUTO to use PHY lookup.
>
> And I'm saying that this idea, of using a specific name and id, is
> frought with fragility and will break in the future in various ways when
> devices get added to systems, making these strings constantly have to be
> kept up to date with different board configurations.
>
> People, NEVER, hardcode something like an id. The fact that this
> happens today with the clock code, doesn't make it right, it makes the
> clock code wrong. Others have already said that this is wrong there as
> well, as systems change and dynamic ids get used more and more.
>
> Let's not repeat the same mistakes of the past just because we refuse to
> learn from them...
>
> So again, the "find a phy by a string" functions should be removed, the
> device id should be automatically created by the phy core just to make
> things unique in sysfs, and no driver code should _ever_ be reliant on
> the number that is being created, and the pointer to the phy structure
> should be used everywhere instead.
>
> With those types of changes, I will consider merging this subsystem, but
> without them, sorry, I will not.
I'll agree with Greg here, the very fact that we see people trying to
add a requirement of *NOT* using PLATFORM_DEVID_AUTO already points to a
big problem in the framework.
The fact is that if we don't allow PLATFORM_DEVID_AUTO we will end up
adding similar infrastructure to the driver themselves to make sure we
don't end up with duplicate names in sysfs in case we have multiple
instances of the same IP in the SoC (or several of the same PCIe card).
I really don't want to go back to that.
--
balbi
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox