Re: [PATCH 13/15] phy: qcom-qmp-pcie: add support for pipediv2 clock

[Johan Hovold]

On Tue, Oct 18, 2022 at 04:05:29PM +0300, Dmitry Baryshkov wrote:
> On 17/10/2022 17:53, Johan Hovold wrote:
> > Some QMP PHYs have a second fixed-divider pipe clock that needs to be
> > enabled along with the pipe clock.
> > 
> > Add support for an optional "pipediv2" clock.
> > 
> > Signed-off-by: Johan Hovold <johan+linaro@xxxxxxxxxx>
> > ---
> >   drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 42 ++++++++++++++++++++----
> >   1 file changed, 36 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
> > index 9c8e009033f1..c1d74c06fad1 100644
> > --- a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
> > +++ b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
> > @@ -1379,7 +1379,9 @@ struct qmp_pcie {
> >   	void __iomem *rx2;
> >   
> >   	struct clk *pipe_clk;
> > +	struct clk *pipediv2_clk;
> >   	struct clk_bulk_data *clks;
> > +
> >   	struct reset_control_bulk_data *resets;
> >   	struct regulator_bulk_data *vregs;
> >   
> > @@ -1902,6 +1904,36 @@ static int qmp_pcie_exit(struct phy *phy)
> >   	return 0;
> >   }
> >   
> > +static int pipe_clk_enable(struct qmp_pcie *qmp)
> > +{
> > +	int ret;
> > +
> > +	ret = clk_prepare_enable(qmp->pipe_clk);
> > +	if (ret) {
> > +		dev_err(qmp->dev, "failed to enable pipe clock: %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	ret = clk_prepare_enable(qmp->pipediv2_clk);
> > +	if (ret) {
> > +		dev_err(qmp->dev, "failed to enable pipediv2 clock: %d\n", ret);
> > +		goto err_disable_pipe_clk;
> > +	}
> 
> Do they have to be enabled in sequence? If not, I'd use a bulk clocks 
> here for the pipe clocks. While it can look like an overkill, it would 
> be a safe net for the possible future changes, which might include 
> additional clocks.

I don't believe the bulk API is a good fit here as we need to support
both the new and old bindings, and for the latter the pipe_clk is looked
up by index rather than name (and that's from the child node too which
limits which APIs you can use further).

The code is clear enough as it stands, and I don't think we need to take
height for a hypothetical third pipe clock just yet.

Johan