Quoting Sean Paul (2018-11-01 14:03:15)
> On Wed, Oct 10, 2018 at 10:15:59AM -0700, Chandan Uddaraju wrote:
>
> > +}
> > +
> > static int dp_power_set_pixel_clk_parent(struct dp_power *dp_power)
> > {
> > int rc = 0;
> > @@ -472,8 +520,12 @@ static int dp_power_set_pixel_clk_parent(struct dp_power *dp_power)
> >
> > power = container_of(dp_power, struct dp_power_private, dp_power);
> >
> > - if (power->pixel_clk_rcg && power->pixel_parent)
> > - clk_set_parent(power->pixel_clk_rcg, power->pixel_parent);
> > + if (power->pixel_clk_rcg && power->pixel_provider) {
> > + clk_set_parent(power->pixel_clk_rcg, power->pixel_provider);
>
> s/pixel_parent/pixel_provider/ isn't related to this change, can you please use
> the final name in the main dp patch so it's correct in this one?
>
> > + pr_debug("%s: is the parent of clk=%s\n",
> > + __clk_get_name(power->pixel_provider),
> > + __clk_get_name(power->pixel_clk_rcg));
>
> Same comment here, this debug can go in the main patch.
Is there any reason why assigned-clock-parents wouldn't work here?
> > diff --git a/drivers/gpu/drm/msm/dp/dp_power.h b/drivers/gpu/drm/msm/dp/dp_power.h
> > index d1adaaf..5effd32 100644
> > --- a/drivers/gpu/drm/msm/dp/dp_power.h
> > +++ b/drivers/gpu/drm/msm/dp/dp_power.h
> > @@ -24,6 +24,7 @@
> > * @init: initializes the regulators/core clocks/GPIOs/pinctrl
> > * @deinit: turns off the regulators/core clocks/GPIOs/pinctrl
> > * @clk_enable: enable/disable the DP clocks
> > + * @set_link_clk_parent: set the parent of DP link clock
> > * @set_pixel_clk_parent: set the parent of DP pixel clock
> > */
> > struct dp_power {
[...]
> > +
> > +static int clk_mux_determine_rate(struct clk_hw *hw,
> > + struct clk_rate_request *req)
> > +{
> > + int ret = 0;
>
> no need to init this to 0
>
> > +
> > + ret = __clk_mux_determine_rate_closest(hw, req);
> > + if (ret)
> > + return ret;
> > +
> > + /* Set the new parent of mux if there is a new valid parent */
> > + if (hw->clk && req->best_parent_hw->clk)
> > + clk_set_parent(hw->clk, req->best_parent_hw->clk);
>
> What about the return value? All other clk_set_parent calls in this patch are
> also unchecked, so please add those as well.
The determine_rate clk_op should _never_ change clk hardware. It's a
function that's about determining what rate a clk can support based on
the rate that's requested and what the hardware can do.
Ok, let me just take a pass at the overall clk parts of this patch
instead of doing an "and also" review on Sean's review.
