Re: [PATCH 2/2] phy: qcom-qmp: new qmp phy driver for qcom-chipsets

On 02/11/16 07:26, Vivek Gautam wrote:

quoted

quoted

+               phy_desc->tx = base + qphy->cfg->tx_offsets[id];

quoted

quoted

+               phy_desc->rx = base + qphy->cfg->rx_offsets[id];
+               phy_desc->pcs = base + qphy->cfg->pcs_offsets[id];
+
+               /*
+                * Get PHY's Pipe clock, if any; USB3 and PCIe are PIPE
+                * based phys, so they essentially have pipe clock
+                */
+               memset(&prop_name, 0, sizeof(prop_name));
+               snprintf(prop_name, MAX_PROP_NAME, "pipe%d", id);
+               phy_desc->pipe_clk = devm_clk_get(dev, prop_name);
+               if (IS_ERR(phy_desc->pipe_clk)) {
+                       if (qphy->cfg->type == PHY_TYPE_PCIE ||
+                           qphy->cfg->type == PHY_TYPE_USB3) {
+                               ret = PTR_ERR(phy_desc->pipe_clk);
+                               if (ret != -EPROBE_DEFER)
+                                       dev_err(dev,
+                                       "failed to get lane%d pipe_clk\n",
id);
+                               return ret;
+                       } else {
+                               phy_desc->pipe_clk = NULL;
+                       }
+               }

Do we need to really need phy type for this?
You should proably make a flag in cfg to mark this rather then using phy
type. Hopefully it will also simplify the logic.

The pipe clock is something that's specific to PCIe and USB3 type
phys (both are PIPE3 specs based phys). So i think it is okay
to use the PHY_TYPE_{*} macros here. Looks a bit cleaner rather
than having a cfg flag. No ?

Yes, it makes sense, I was not sure of full details of the pipe3 specs, 
now that you made it more clear.
Remember to add this in comments.

Thanks,
srini

Using PHY_TYPE_{*} macros will also keep the doors open to
other PHY types that are added at a later point in time.

quoted

quoted