Hi Benoit, On 08/15/2013 04:58 PM, Benoit Cousson wrote: > Hi Roger, > > On 15/08/2013 15:15, Roger Quadros wrote: >> Add support for new device types and in the process rid of "ti,type" >> device tree property. The correct type of device will be determined >> from the compatible string instead. >> >> Introduce a compatible string for each device type. At the moment >> we support 4 types Mailbox, USB2, USB3 and DRA7. >> >> Update DT binding information to reflect these changes. >> >> Also get rid of omap_control_usb3_phy_power(). Just one function >> i.e. omap_control_usb_phy_power() will now take care of all PHY types. >> >> Signed-off-by: Roger Quadros <rogerq@xxxxxx> >> --- >> Documentation/devicetree/bindings/usb/omap-usb.txt | 29 ++-- >> drivers/usb/phy/phy-omap-control.c | 148 ++++++++++++-------- >> drivers/usb/phy/phy-omap-usb2.c | 4 + >> drivers/usb/phy/phy-omap-usb3.c | 6 +- >> include/linux/usb/omap_control_usb.h | 24 ++-- >> 5 files changed, 122 insertions(+), 89 deletions(-) >> >> diff --git a/Documentation/devicetree/bindings/usb/omap-usb.txt b/Documentation/devicetree/bindings/usb/omap-usb.txt >> index 57e71f6..1c6b54a 100644 >> --- a/Documentation/devicetree/bindings/usb/omap-usb.txt >> +++ b/Documentation/devicetree/bindings/usb/omap-usb.txt >> @@ -73,22 +73,23 @@ omap_dwc3 { >> OMAP CONTROL USB >> >> Required properties: >> - - compatible: Should be "ti,omap-control-usb" >> + - compatible: Should be one of >> + "ti,omap-control-usb" - if it has otghs_control mailbox register >> + e.g. on OMAP4. > > How generic is that one? I mean if this is applicable for OMAP4 only, you'd better name it omap4-control-usb. > AFAIK it is OMAP4 only so i'll change it to omap4-control-usb. >> + "ti,usb2-control-usb" - if it has Power down bit in control_dev_conf register >> + e.g. USB2_PHY on OMAP5. >> + "ti,usb3-control-usb" - if it has DPLL and individual Rx & Tx power control >> + e.g. USB3 PHY and SATA PHY on OMAP5. >> + "ti,dra7-control-usb" - if it has both power down and power aux registers >> + e.g. USB2 PHY on DRA7 platform. >> + >> - reg : Address and length of the register set for the device. It contains >> - the address of "control_dev_conf" and "otghs_control" or "phy_power_usb" >> - depending upon omap4 or omap5. >> - - reg-names: The names of the register addresses corresponding to the registers >> - filled in "reg". >> - - ti,type: This is used to differentiate whether the control module has >> - usb mailbox or usb3 phy power. omap4 has usb mailbox in control module to >> - notify events to the musb core and omap5 has usb3 phy power register to >> - power on usb3 phy. Should be "1" if it has mailbox and "2" if it has usb3 >> - phy power. >> + the address of "otghs_control" for type 1 or "power" register for other types. >> + For type 4, it must also contain "power_aux" register. >> + - reg-names: should be otghs_control for type 1 and "power" for other types. > > type1 and 4 are less obvious now that you have name, you should be more explicit and name them directly. OK, I'll fix it. > >> >> omap_control_usb: omap-control-usb@4a002300 { >> compatible = "ti,omap-control-usb"; >> - reg = <0x4a002300 0x4>, >> - <0x4a00233c 0x4>; >> - reg-names = "control_dev_conf", "otghs_control"; >> - ti,type = <1>; >> + reg = <0x4a00233c 0x4>; >> + reg-names = "otghs_control"; >> }; >> diff --git a/drivers/usb/phy/phy-omap-control.c b/drivers/usb/phy/phy-omap-control.c >> index 3b9ee83..078c46f 100644 >> --- a/drivers/usb/phy/phy-omap-control.c >> +++ b/drivers/usb/phy/phy-omap-control.c >> @@ -46,61 +46,76 @@ struct device *omap_get_control_dev(void) >> EXPORT_SYMBOL_GPL(omap_get_control_dev); >> ... >> >> - control_usb->dev = &pdev->dev; >> + control_usb->dev = &pdev->dev; >> + control_usb->type = OMAP_CTRL_TYPE_OMAP; >> >> - res = platform_get_resource_byname(pdev, IORESOURCE_MEM, >> - "control_dev_conf"); >> - control_usb->dev_conf = devm_ioremap_resource(&pdev->dev, res); >> - if (IS_ERR(control_usb->dev_conf)) >> - return PTR_ERR(control_usb->dev_conf); >> + if (of_device_is_compatible(np, "ti,usb2-control-usb")) >> + control_usb->type = OMAP_CTRL_TYPE_USB2; >> + else if (of_device_is_compatible(np, "ti,usb3-control-usb")) >> + control_usb->type = OMAP_CTRL_TYPE_USB3; >> + else if (of_device_is_compatible(np, "ti,dra7-control-usb")) >> + control_usb->type = OMAP_CTRL_TYPE_DRA7; > > You can avoid that by adding the type directly in the data field of the omap_control_usb_id_table. > It will be more readable and scalable. > > This how it was done on gpio-omap for example. OK. Thanks for the hint. > >> >> - if (control_usb->type == OMAP_CTRL_DEV_TYPE1) { >> + if (control_usb->type == OMAP_CTRL_TYPE_OMAP) { >> res = platform_get_resource_byname(pdev, IORESOURCE_MEM, >> "otghs_control"); >> control_usb->otghs_control = devm_ioremap_resource( >> &pdev->dev, res); >> if (IS_ERR(control_usb->otghs_control)) >> return PTR_ERR(control_usb->otghs_control); >> - } >> - >> - if (control_usb->type == OMAP_CTRL_DEV_TYPE2) { >> + } else { >> res = platform_get_resource_byname(pdev, IORESOURCE_MEM, >> - "phy_power_usb"); >> - control_usb->phy_power = devm_ioremap_resource( >> - &pdev->dev, res); >> - if (IS_ERR(control_usb->phy_power)) >> - return PTR_ERR(control_usb->phy_power); >> + "power"); >> + control_usb->power = devm_ioremap_resource(&pdev->dev, res); >> + if (IS_ERR(control_usb->power)) { >> + dev_err(&pdev->dev, "Couldn't get power register\n"); >> + return PTR_ERR(control_usb->power); >> + } >> + } >> >> + if (control_usb->type == OMAP_CTRL_TYPE_USB3) { >> control_usb->sys_clk = devm_clk_get(control_usb->dev, >> "sys_clkin"); >> if (IS_ERR(control_usb->sys_clk)) { >> @@ -245,6 +261,15 @@ static int omap_control_usb_probe(struct platform_device *pdev) >> } >> } >> >> + if (control_usb->type == OMAP_CTRL_TYPE_DRA7) { >> + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, >> + "power_aux"); >> + control_usb->power_aux = devm_ioremap_resource(&pdev->dev, res); >> + if (IS_ERR(control_usb->power_aux)) { >> + dev_err(&pdev->dev, "Couldn't get power_aux register\n"); >> + return PTR_ERR(control_usb->power_aux); >> + } >> + } >> >> dev_set_drvdata(control_usb->dev, control_usb); >> >> @@ -254,6 +279,9 @@ static int omap_control_usb_probe(struct platform_device *pdev) >> #ifdef CONFIG_OF >> static const struct of_device_id omap_control_usb_id_table[] = { >> { .compatible = "ti,omap-control-usb" }, >> + { .compatible = "ti,usb2-control-usb" }, >> + { .compatible = "ti,usb3-control-usb" }, >> + { .compatible = "ti,dra7-control-usb" }, >> {} > > In that table you can add a data field with the proper type enum. got it. cheers, -roger -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html