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 devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
