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

[Srinivas Kandagatla]

On 02/11/16 07:26, Vivek Gautam wrote:
> > > +               phy_desc->tx = base + qphy->cfg->tx_offsets[id];
> > > >> +               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.
>
> > >