PHY: sunxi: Add driver for sunxi usb phy

The Allwinner A1x / A2x SoCs have 2 or 3 usb phys which are all accessed through a single set of registers. Besides this there are also some other phy related bits which need poking, which are per phy, but shared between the ohci and ehci controllers, so these are also controlled from this new phy driver.

Signed-off-by Hans de Goede <[email protected]>

Acked-by Maxime Ripard <[email protected]>

---
  .../devicetree/bindings/phy/sun4i-usb-phy.txt      |  26 ++
  drivers/phy/Kconfig                                |  11 +
  drivers/phy/Makefile                               |   1 +
  drivers/phy/phy-sun4i-usb.c                        | 331 +++++++++++++++++++++
  4 files changed, 369 insertions(+)

create mode 100644 Documentation/devicetree/bindings/phy/sun4i-usb-phy.txt create mode 100644 drivers/phy/phy-sun4i-usb.c

drivers/phy/phy-sun4i-usb.c new file +24 -0

+		clk_disable_unprepare(data->clk);
+		return ret;
+	}
+
+	/* Adjust PHY's magnitude and rate */
+	sun4i_usb_phy_write(phy, PHY_TX_AMPLITUDE_TUNE, 0x14, 5);
+
+	/* Disconnect threshold adjustment */
+	sun4i_usb_phy_write(phy, PHY_DISCON_TH_SEL, data->disc_thresh, 2);
+
+	sun4i_usb_phy_passby(phy, 1);
+
+	return 0;
+}
+
+static int sun4i_usb_phy_exit(struct phy *_phy)
+{
+	struct sun4i_usb_phy *phy = phy_get_drvdata(_phy);
+	struct sun4i_usb_phy_data *data = to_sun4i_usb_phy_data(phy);
+
+	sun4i_usb_phy_passby(phy, 0);
+	reset_control_assert(phy->reset);
+	clk_disable_unprepare(data->clk);
+

drivers/phy/phy-sun4i-usb.c new file +52 -0

+
+	return ret;
+}
+
+static int sun4i_usb_phy_power_off(struct phy *_phy)
+{
+	struct sun4i_usb_phy *phy = phy_get_drvdata(_phy);
+
+	if (phy->vbus)
+		regulator_disable(phy->vbus);
+
+	return 0;
+}
+
+static struct phy_ops sun4i_usb_phy_ops = {
+	.init		= sun4i_usb_phy_init,
+	.exit		= sun4i_usb_phy_exit,
+	.power_on	= sun4i_usb_phy_power_on,
+	.power_off	= sun4i_usb_phy_power_off,
+	.owner		= THIS_MODULE,
+};
+
+static struct phy *sun4i_usb_phy_xlate(struct device *dev,
+					struct of_phandle_args *args)
+{
+	struct sun4i_usb_phy_data *data = dev_get_drvdata(dev);
+
+	if (WARN_ON(args->args[0] == 0 || args->args[0] >= data->num_phys))
+		return ERR_PTR(-ENODEV);
+
+	return data->phys[args->args[0]].phy;
+}
+
+static int sun4i_usb_phy_probe(struct platform_device *pdev)
+{
+	struct sun4i_usb_phy_data *data;
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	void __iomem *pmu = NULL;
+	struct phy_provider *phy_provider;
+	struct reset_control *reset;
+	struct regulator *vbus;
+	struct resource *res;
+	struct phy *phy;
+	char name[16];
+	int i;
+
+	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	mutex_init(&data->mutex);

Hi,

Quoted 8 lines · Hans de Goede

The Allwinner A1x / A2x SoCs have 2 or 3 usb phys which are all accessed through a single set of registers. Besides this there are also some other phy related bits which need poking, which are per phy, but shared between the ohci and ehci controllers, so these are also controlled from this new phy driver.

Signed-off-by Hans de Goede <[email protected]>

Acked-by Maxime Ripard <[email protected]>

Quoted 9 lines

---
  .../devicetree/bindings/phy/sun4i-usb-phy.txt      |  26 ++
  drivers/phy/Kconfig                                |  11 +
  drivers/phy/Makefile                               |   1 +
  drivers/phy/phy-sun4i-usb.c                        | 331 +++++++++++++++++++++
  4 files changed, 369 insertions(+)

create mode 100644 Documentation/devicetree/bindings/phy/sun4i-usb-phy.txt create mode 100644 drivers/phy/phy-sun4i-usb.c

..
<snip>
.
.

drivers/phy/phy-sun4i-usb.c line 150

Quoted 8 lines

drivers/phy/phy-sun4i-usb.c new file +8 -0

+static int sun4i_usb_phy_init(struct phy *_phy)
+{
+    struct sun4i_usb_phy *phy = phy_get_drvdata(_phy);
+    struct sun4i_usb_phy_data *data = to_sun4i_usb_phy_data(phy);
+    int ret;
+
+    ret = clk_prepare_enable(data->clk);
+    if (ret)

dev_err here?

drivers/phy/phy-sun4i-usb.c line 154

Quoted 4 lines

drivers/phy/phy-sun4i-usb.c new file +4 -0

+        return ret;
+
+    ret = reset_control_deassert(phy->reset);
+    if (ret) {

here too..

Quoted 27 lines

drivers/phy/phy-sun4i-usb.c new file +27 -0

+        clk_disable_unprepare(data->clk);
+        return ret;
+    }
+
+    /* Adjust PHY's magnitude and rate */
+    sun4i_usb_phy_write(phy, PHY_TX_AMPLITUDE_TUNE, 0x14, 5);
+
+    /* Disconnect threshold adjustment */
+    sun4i_usb_phy_write(phy, PHY_DISCON_TH_SEL, data->disc_thresh, 2);
+
+    sun4i_usb_phy_passby(phy, 1);
+
+    return 0;
+}
+
+static int sun4i_usb_phy_exit(struct phy *_phy)
+{
+    struct sun4i_usb_phy *phy = phy_get_drvdata(_phy);
+    struct sun4i_usb_phy_data *data = to_sun4i_usb_phy_data(phy);
+
+    sun4i_usb_phy_passby(phy, 0);
+    reset_control_assert(phy->reset);
+    clk_disable_unprepare(data->clk);
+
+    return 0;
+}
+

drivers/phy/phy-sun4i-usb.c line 188

Quoted 7 lines

drivers/phy/phy-sun4i-usb.c new file +7 -0

+static int sun4i_usb_phy_power_on(struct phy *_phy)
+{
+    struct sun4i_usb_phy *phy = phy_get_drvdata(_phy);
+    int ret = 0;
+
+    if (phy->vbus)
+        ret = regulator_enable(phy->vbus);

dev_err here too..

Quoted 56 lines

drivers/phy/phy-sun4i-usb.c new file +56 -0

+
+    return ret;
+}
+
+static int sun4i_usb_phy_power_off(struct phy *_phy)
+{
+    struct sun4i_usb_phy *phy = phy_get_drvdata(_phy);
+
+    if (phy->vbus)
+        regulator_disable(phy->vbus);
+
+    return 0;
+}
+
+static struct phy_ops sun4i_usb_phy_ops = {
+    .init        = sun4i_usb_phy_init,
+    .exit        = sun4i_usb_phy_exit,
+    .power_on    = sun4i_usb_phy_power_on,
+    .power_off    = sun4i_usb_phy_power_off,
+    .owner        = THIS_MODULE,
+};
+
+static struct phy *sun4i_usb_phy_xlate(struct device *dev,
+                    struct of_phandle_args *args)
+{
+    struct sun4i_usb_phy_data *data = dev_get_drvdata(dev);
+
+    if (WARN_ON(args->args[0] == 0 || args->args[0] >= data->num_phys))
+        return ERR_PTR(-ENODEV);
+
+    return data->phys[args->args[0]].phy;
+}
+
+static int sun4i_usb_phy_probe(struct platform_device *pdev)
+{
+    struct sun4i_usb_phy_data *data;
+    struct device *dev = &pdev->dev;
+    struct device_node *np = dev->of_node;
+    void __iomem *pmu = NULL;
+    struct phy_provider *phy_provider;
+    struct reset_control *reset;
+    struct regulator *vbus;
+    struct resource *res;
+    struct phy *phy;
+    char name[16];
+    int i;
+
+    data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
+    if (!data)
+        return -ENOMEM;
+
+    mutex_init(&data->mutex);
+
+    if (of_device_is_compatible(np, "allwinner,sun5i-a13-usb-phy"))
+        data->num_phys = 2;
+    else

drivers/phy/phy-sun4i-usb.c line 250

Quoted 6 lines

drivers/phy/phy-sun4i-usb.c new file +6 -0

+        data->num_phys = 3;
+
+    if (of_device_is_compatible(np, "allwinner,sun4i-a10-usb-phy"))
+        data->disc_thresh = 3;
+    else
+        data->disc_thresh = 2;

These 'data' can actually be part of 'of_device_id' table and can be obtained by using 'of_match_device'.

Hi,

Quoted 25 lines · Kishon Vijay Abraham I

Hi,

Quoted 8 lines · Hans de Goede

The Allwinner A1x / A2x SoCs have 2 or 3 usb phys which are all accessed through a single set of registers. Besides this there are also some other phy related bits which need poking, which are per phy, but shared between the ohci and ehci controllers, so these are also controlled from this new phy driver.

Signed-off-by Hans de Goede <[email protected]>

Acked-by Maxime Ripard <[email protected]>

Quoted 9 lines

---
   .../devicetree/bindings/phy/sun4i-usb-phy.txt      |  26 ++
   drivers/phy/Kconfig                                |  11 +
   drivers/phy/Makefile                               |   1 +
   drivers/phy/phy-sun4i-usb.c                        | 331 +++++++++++++++++++++
   4 files changed, 369 insertions(+)

create mode 100644 Documentation/devicetree/bindings/phy/sun4i-usb-phy.txt create mode 100644 drivers/phy/phy-sun4i-usb.c

..
<snip>
.
.

Quoted 9 lines

drivers/phy/phy-sun4i-usb.c line 150

Quoted 8 lines

drivers/phy/phy-sun4i-usb.c new file +8 -0

+static int sun4i_usb_phy_init(struct phy *_phy)
+{
+    struct sun4i_usb_phy *phy = phy_get_drvdata(_phy);
+    struct sun4i_usb_phy_data *data = to_sun4i_usb_phy_data(phy);
+    int ret;
+
+    ret = clk_prepare_enable(data->clk);
+    if (ret)

dev_err here?

I would expect the clk subsys to print an error if anything goes wrong here, repeating that error in the device driver seems not useful to me.

Note that this practice of simply propagating the error is a common pattern in many many users of the clk / reset / regulator framework.

Also can I please get one final review of this patch rather then a round of comments ending with:

"If you can fix these minor comments while changing the $subject (PHY: sunxi) it should be good to get merged."

Only to get more review comments when I've already fixed the minor comments ?

Quoted 6 lines

drivers/phy/phy-sun4i-usb.c line 154

Quoted 4 lines

drivers/phy/phy-sun4i-usb.c new file +4 -0

+        return ret;
+
+    ret = reset_control_deassert(phy->reset);
+    if (ret) {

here too..

idem.

Quoted 27 lines

Quoted 27 lines

drivers/phy/phy-sun4i-usb.c new file +27 -0

+        clk_disable_unprepare(data->clk);
+        return ret;
+    }
+
+    /* Adjust PHY's magnitude and rate */
+    sun4i_usb_phy_write(phy, PHY_TX_AMPLITUDE_TUNE, 0x14, 5);
+
+    /* Disconnect threshold adjustment */
+    sun4i_usb_phy_write(phy, PHY_DISCON_TH_SEL, data->disc_thresh, 2);
+
+    sun4i_usb_phy_passby(phy, 1);
+
+    return 0;
+}
+
+static int sun4i_usb_phy_exit(struct phy *_phy)
+{
+    struct sun4i_usb_phy *phy = phy_get_drvdata(_phy);
+    struct sun4i_usb_phy_data *data = to_sun4i_usb_phy_data(phy);
+
+    sun4i_usb_phy_passby(phy, 0);
+    reset_control_assert(phy->reset);
+    clk_disable_unprepare(data->clk);
+
+    return 0;
+}
+

Quoted 8 lines

drivers/phy/phy-sun4i-usb.c line 188

Quoted 7 lines

drivers/phy/phy-sun4i-usb.c new file +7 -0

+static int sun4i_usb_phy_power_on(struct phy *_phy)
+{
+    struct sun4i_usb_phy *phy = phy_get_drvdata(_phy);
+    int ret = 0;
+
+    if (phy->vbus)
+        ret = regulator_enable(phy->vbus);

dev_err here too..

idem.

Quoted 56 lines

Quoted 56 lines

drivers/phy/phy-sun4i-usb.c new file +56 -0

+
+    return ret;
+}
+
+static int sun4i_usb_phy_power_off(struct phy *_phy)
+{
+    struct sun4i_usb_phy *phy = phy_get_drvdata(_phy);
+
+    if (phy->vbus)
+        regulator_disable(phy->vbus);
+
+    return 0;
+}
+
+static struct phy_ops sun4i_usb_phy_ops = {
+    .init        = sun4i_usb_phy_init,
+    .exit        = sun4i_usb_phy_exit,
+    .power_on    = sun4i_usb_phy_power_on,
+    .power_off    = sun4i_usb_phy_power_off,
+    .owner        = THIS_MODULE,
+};
+
+static struct phy *sun4i_usb_phy_xlate(struct device *dev,
+                    struct of_phandle_args *args)
+{
+    struct sun4i_usb_phy_data *data = dev_get_drvdata(dev);
+
+    if (WARN_ON(args->args[0] == 0 || args->args[0] >= data->num_phys))
+        return ERR_PTR(-ENODEV);
+
+    return data->phys[args->args[0]].phy;
+}
+
+static int sun4i_usb_phy_probe(struct platform_device *pdev)
+{
+    struct sun4i_usb_phy_data *data;
+    struct device *dev = &pdev->dev;
+    struct device_node *np = dev->of_node;
+    void __iomem *pmu = NULL;
+    struct phy_provider *phy_provider;
+    struct reset_control *reset;
+    struct regulator *vbus;
+    struct resource *res;
+    struct phy *phy;
+    char name[16];
+    int i;
+
+    data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
+    if (!data)
+        return -ENOMEM;
+
+    mutex_init(&data->mutex);
+
+    if (of_device_is_compatible(np, "allwinner,sun5i-a13-usb-phy"))
+        data->num_phys = 2;
+    else

Quoted 8 lines

drivers/phy/phy-sun4i-usb.c line 250

Quoted 6 lines

drivers/phy/phy-sun4i-usb.c new file +6 -0

+        data->num_phys = 3;
+
+    if (of_device_is_compatible(np, "allwinner,sun4i-a10-usb-phy"))
+        data->disc_thresh = 3;
+    else
+        data->disc_thresh = 2;

These 'data' can actually be part of 'of_device_id' table and can be obtained by using 'of_match_device'.

This was already suggested by Maxime around the time of v2, and I responded with this:

"The problem with using the of_device_id .data field is that I can only store a single integer there. To store 2 I need to: define a struct, create an array of these structs with initialization. Create an enum for indexing the array which must be kept in sync with the initializers manually, store either the index, or a direct pointer to the correct array entry into the .data field, add code to get the of_device_id from the compatible string, and then finally extract the settings from the struct again.

See IE how this is done in drivers/ata/ahci_platform.c, I've tried to come up with a simpler way and failed, for ahci_platform.c the struct with per compatible-string data is quite big so it makes some sense to use this construction. Here however not so much, this adds a whole lot of unnecessary extra code + indirection. I esp. object against the indirection as that unnecessarily makes it harder to follow whats going on."

Hi,

Quoted 178 lines · Hans de Goede

Hi,

Quoted 25 lines · Kishon Vijay Abraham I

Hi,

Quoted 8 lines · Hans de Goede

The Allwinner A1x / A2x SoCs have 2 or 3 usb phys which are all accessed through a single set of registers. Besides this there are also some other phy related bits which need poking, which are per phy, but shared between the ohci and ehci controllers, so these are also controlled from this new phy driver.

Signed-off-by Hans de Goede <[email protected]>

Acked-by Maxime Ripard <[email protected]>

Quoted 9 lines

---
   .../devicetree/bindings/phy/sun4i-usb-phy.txt      |  26 ++
   drivers/phy/Kconfig                                |  11 +
   drivers/phy/Makefile                               |   1 +
   drivers/phy/phy-sun4i-usb.c                        | 331 +++++++++++++++++++++
   4 files changed, 369 insertions(+)

create mode 100644 Documentation/devicetree/bindings/phy/sun4i-usb-phy.txt create mode 100644 drivers/phy/phy-sun4i-usb.c

..
<snip>
.
.

Quoted 9 lines

drivers/phy/phy-sun4i-usb.c line 150

Quoted 8 lines

drivers/phy/phy-sun4i-usb.c new file +8 -0

+static int sun4i_usb_phy_init(struct phy *_phy)
+{
+    struct sun4i_usb_phy *phy = phy_get_drvdata(_phy);
+    struct sun4i_usb_phy_data *data = to_sun4i_usb_phy_data(phy);
+    int ret;
+
+    ret = clk_prepare_enable(data->clk);
+    if (ret)

dev_err here?

I would expect the clk subsys to print an error if anything goes wrong here, repeating that error in the device driver seems not useful to me.

Note that this practice of simply propagating the error is a common pattern in many many users of the clk / reset / regulator framework.

Also can I please get one final review of this patch rather then a round of comments ending with:

"If you can fix these minor comments while changing the $subject (PHY: sunxi) it should be good to get merged."

Only to get more review comments when I've already fixed the minor comments ?

Quoted 6 lines

drivers/phy/phy-sun4i-usb.c line 154

Quoted 4 lines

drivers/phy/phy-sun4i-usb.c new file +4 -0

+        return ret;
+
+    ret = reset_control_deassert(phy->reset);
+    if (ret) {

here too..

idem.

Quoted 27 lines

Quoted 27 lines

drivers/phy/phy-sun4i-usb.c new file +27 -0

+        clk_disable_unprepare(data->clk);
+        return ret;
+    }
+
+    /* Adjust PHY's magnitude and rate */
+    sun4i_usb_phy_write(phy, PHY_TX_AMPLITUDE_TUNE, 0x14, 5);
+
+    /* Disconnect threshold adjustment */
+    sun4i_usb_phy_write(phy, PHY_DISCON_TH_SEL, data->disc_thresh, 2);
+
+    sun4i_usb_phy_passby(phy, 1);
+
+    return 0;
+}
+
+static int sun4i_usb_phy_exit(struct phy *_phy)
+{
+    struct sun4i_usb_phy *phy = phy_get_drvdata(_phy);
+    struct sun4i_usb_phy_data *data = to_sun4i_usb_phy_data(phy);
+
+    sun4i_usb_phy_passby(phy, 0);
+    reset_control_assert(phy->reset);
+    clk_disable_unprepare(data->clk);
+
+    return 0;
+}
+

Quoted 8 lines

drivers/phy/phy-sun4i-usb.c line 188

Quoted 7 lines

drivers/phy/phy-sun4i-usb.c new file +7 -0

+static int sun4i_usb_phy_power_on(struct phy *_phy)
+{
+    struct sun4i_usb_phy *phy = phy_get_drvdata(_phy);
+    int ret = 0;
+
+    if (phy->vbus)
+        ret = regulator_enable(phy->vbus);

dev_err here too..

idem.

Quoted 56 lines

Quoted 56 lines

drivers/phy/phy-sun4i-usb.c new file +56 -0

+
+    return ret;
+}
+
+static int sun4i_usb_phy_power_off(struct phy *_phy)
+{
+    struct sun4i_usb_phy *phy = phy_get_drvdata(_phy);
+
+    if (phy->vbus)
+        regulator_disable(phy->vbus);
+
+    return 0;
+}
+
+static struct phy_ops sun4i_usb_phy_ops = {
+    .init        = sun4i_usb_phy_init,
+    .exit        = sun4i_usb_phy_exit,
+    .power_on    = sun4i_usb_phy_power_on,
+    .power_off    = sun4i_usb_phy_power_off,
+    .owner        = THIS_MODULE,
+};
+
+static struct phy *sun4i_usb_phy_xlate(struct device *dev,
+                    struct of_phandle_args *args)
+{
+    struct sun4i_usb_phy_data *data = dev_get_drvdata(dev);
+
+    if (WARN_ON(args->args[0] == 0 || args->args[0] >= data->num_phys))
+        return ERR_PTR(-ENODEV);
+
+    return data->phys[args->args[0]].phy;
+}
+
+static int sun4i_usb_phy_probe(struct platform_device *pdev)
+{
+    struct sun4i_usb_phy_data *data;
+    struct device *dev = &pdev->dev;
+    struct device_node *np = dev->of_node;
+    void __iomem *pmu = NULL;
+    struct phy_provider *phy_provider;
+    struct reset_control *reset;
+    struct regulator *vbus;
+    struct resource *res;
+    struct phy *phy;
+    char name[16];
+    int i;
+
+    data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
+    if (!data)
+        return -ENOMEM;
+
+    mutex_init(&data->mutex);
+
+    if (of_device_is_compatible(np, "allwinner,sun5i-a13-usb-phy"))
+        data->num_phys = 2;
+    else

Quoted 8 lines

drivers/phy/phy-sun4i-usb.c line 250

Quoted 6 lines

drivers/phy/phy-sun4i-usb.c new file +6 -0

+        data->num_phys = 3;
+
+    if (of_device_is_compatible(np, "allwinner,sun4i-a10-usb-phy"))
+        data->disc_thresh = 3;
+    else
+        data->disc_thresh = 2;

These 'data' can actually be part of 'of_device_id' table and can be obtained by using 'of_match_device'.

This was already suggested by Maxime around the time of v2, and I responded with this:

"The problem with using the of_device_id .data field is that I can only store a single integer there. To store 2 I need to: define a struct, create an array of these structs with initialization. Create an enum for indexing the array which must be kept in sync with the initializers manually, store either the index, or a direct pointer to the correct array entry into the .data field, add code to get the of_device_id from the compatible string, and then finally extract the settings from the struct again.

Quoted 7 lines

See IE how this is done in drivers/ata/ahci_platform.c, I've tried to come up with a simpler way and failed, for ahci_platform.c the struct with per compatible-string data is quite big so it makes some sense to use this construction. Here however not so much, this adds a whole lot of unnecessary extra code + indirection. I esp. object against the indirection as that unnecessarily makes it harder to follow whats going on."

alright.. missed that earlier. Sorry.

Hi,

Quoted 201 lines · Kishon Vijay Abraham I

Hi,

Quoted 196 lines · Hans de Goede

Hi,

Quoted 31 lines · Kishon Vijay Abraham I

Hi,

Quoted 19 lines · Hans de Goede

The Allwinner A1x / A2x SoCs have 2 or 3 usb phys which are all accessed
through a single set of registers. Besides this there are also some other
phy related bits which need poking, which are per phy, but shared between the
ohci and ehci controllers, so these are also controlled from this new phy
driver.

Signed-off-by Hans de Goede <[email protected]>

Acked-by Maxime Ripard <[email protected]>

---
   .../devicetree/bindings/phy/sun4i-usb-phy.txt      |  26 ++
   drivers/phy/Kconfig                                |  11 +
   drivers/phy/Makefile                               |   1 +
   drivers/phy/phy-sun4i-usb.c                        | 331
+++++++++++++++++++++
   4 files changed, 369 insertions(+)

Quoted 4 lines

create mode 100644
Documentation/devicetree/bindings/phy/sun4i-usb-phy.txt
create mode 100644 drivers/phy/phy-sun4i-usb.c

..
<snip>
.
.

Quoted 9 lines

drivers/phy/phy-sun4i-usb.c line 150

Quoted 8 lines

drivers/phy/phy-sun4i-usb.c new file +8 -0

+static int sun4i_usb_phy_init(struct phy *_phy)
+{
+    struct sun4i_usb_phy *phy = phy_get_drvdata(_phy);
+    struct sun4i_usb_phy_data *data = to_sun4i_usb_phy_data(phy);
+    int ret;
+
+    ret = clk_prepare_enable(data->clk);
+    if (ret)

dev_err here?

I would expect the clk subsys to print an error if anything goes wrong here, repeating that error in the device driver seems not useful to me.

Note that this practice of simply propagating the error is a common pattern in many many users of the clk / reset / regulator framework.

Also can I please get one final review of this patch rather then a round of comments ending with:

"If you can fix these minor comments while changing the $subject (PHY: sunxi) it
should be good to get merged."

Only to get more review comments when I've already fixed the minor comments ?

Quoted 6 lines

drivers/phy/phy-sun4i-usb.c line 154

Quoted 4 lines

drivers/phy/phy-sun4i-usb.c new file +4 -0

+        return ret;
+
+    ret = reset_control_deassert(phy->reset);
+    if (ret) {

here too..

idem.

Quoted 28 lines

Quoted 28 lines

drivers/phy/phy-sun4i-usb.c new file +9 -0

+        clk_disable_unprepare(data->clk);
+        return ret;
+    }
+
+    /* Adjust PHY's magnitude and rate */
+    sun4i_usb_phy_write(phy, PHY_TX_AMPLITUDE_TUNE, 0x14, 5);
+
+    /* Disconnect threshold adjustment */
+    sun4i_usb_phy_write(phy, PHY_DISCON_TH_SEL, data->disc_thresh,

2);

drivers/phy/phy-sun4i-usb.c new file +18 -0

+
+    sun4i_usb_phy_passby(phy, 1);
+
+    return 0;
+}
+
+static int sun4i_usb_phy_exit(struct phy *_phy)
+{
+    struct sun4i_usb_phy *phy = phy_get_drvdata(_phy);
+    struct sun4i_usb_phy_data *data = to_sun4i_usb_phy_data(phy);
+
+    sun4i_usb_phy_passby(phy, 0);
+    reset_control_assert(phy->reset);
+    clk_disable_unprepare(data->clk);
+
+    return 0;
+}
+

Quoted 8 lines

drivers/phy/phy-sun4i-usb.c line 188

Quoted 7 lines

drivers/phy/phy-sun4i-usb.c new file +7 -0

+static int sun4i_usb_phy_power_on(struct phy *_phy)
+{
+    struct sun4i_usb_phy *phy = phy_get_drvdata(_phy);
+    int ret = 0;
+
+    if (phy->vbus)
+        ret = regulator_enable(phy->vbus);

dev_err here too..

idem.

Quoted 58 lines

Quoted 58 lines

drivers/phy/phy-sun4i-usb.c new file +28 -0

+
+    return ret;
+}
+
+static int sun4i_usb_phy_power_off(struct phy *_phy)
+{
+    struct sun4i_usb_phy *phy = phy_get_drvdata(_phy);
+
+    if (phy->vbus)
+        regulator_disable(phy->vbus);
+
+    return 0;
+}
+
+static struct phy_ops sun4i_usb_phy_ops = {
+    .init        = sun4i_usb_phy_init,
+    .exit        = sun4i_usb_phy_exit,
+    .power_on    = sun4i_usb_phy_power_on,
+    .power_off    = sun4i_usb_phy_power_off,
+    .owner        = THIS_MODULE,
+};
+
+static struct phy *sun4i_usb_phy_xlate(struct device *dev,
+                    struct of_phandle_args *args)
+{
+    struct sun4i_usb_phy_data *data = dev_get_drvdata(dev);
+
+    if (WARN_ON(args->args[0] == 0 || args->args[0] >=

data->num_phys))

drivers/phy/phy-sun4i-usb.c new file +29 -0

+        return ERR_PTR(-ENODEV);
+
+    return data->phys[args->args[0]].phy;
+}
+
+static int sun4i_usb_phy_probe(struct platform_device *pdev)
+{
+    struct sun4i_usb_phy_data *data;
+    struct device *dev = &pdev->dev;
+    struct device_node *np = dev->of_node;
+    void __iomem *pmu = NULL;
+    struct phy_provider *phy_provider;
+    struct reset_control *reset;
+    struct regulator *vbus;
+    struct resource *res;
+    struct phy *phy;
+    char name[16];
+    int i;
+
+    data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
+    if (!data)
+        return -ENOMEM;
+
+    mutex_init(&data->mutex);
+
+    if (of_device_is_compatible(np, "allwinner,sun5i-a13-usb-phy"))
+        data->num_phys = 2;
+    else
+        data->num_phys = 3;

Quoted 8 lines

drivers/phy/phy-sun4i-usb.c line 250

Quoted 5 lines

drivers/phy/phy-sun4i-usb.c new file +5 -0

+
+    if (of_device_is_compatible(np, "allwinner,sun4i-a10-usb-phy"))
+        data->disc_thresh = 3;
+    else
+        data->disc_thresh = 2;

These 'data' can actually be part of 'of_device_id' table and can be obtained by using 'of_match_device'.

This was already suggested by Maxime around the time of v2, and I responded with this:

"The problem with using the of_device_id .data field is that I can only store a single integer there. To store 2 I need to: define a struct, create an array of these structs with initialization. Create an enum for indexing the array which must be kept in sync with the initializers manually, store either the index, or a direct pointer to the correct array
entry into the .data field, add code to get the of_device_id from the compatible string, and then finally extract the settings from the struct again.

See IE how this is done in drivers/ata/ahci_platform.c, I've tried to come up with a simpler way and failed, for ahci_platform.c the struct with per compatible-string data is quite big so it makes some sense to use this construction. Here however not so much, this adds a whole lot of unnecessary extra code + indirection. I esp. object against the indirection as that unnecessarily makes it harder to follow whats going on."

Quoted 1 line

alright.. missed that earlier. Sorry.

So does this mean you're going to take v4 as is, or do you want me to add the dev_err calls you've pointed out before (note I still prefer to not add these, but if you insist...) ?

On Tuesday 04 March 2014 10:33 PM, Hans de Goede wrote:

Quoted 5 lines

Quoted 1 line

alright.. missed that earlier. Sorry.

So does this mean you're going to take v4 as is, or do you want me to add the dev_err calls you've pointed out before (note I still prefer to not add these, but if you insist...) ?

You should have received a notification on it being merged to 'next'. didn't you?