On Sat, 30 Dec 2023 at 02:12, Konrad Dybcio <konrad.dybcio@xxxxxxxxxx> wrote:
>
> On 29.12.2023 23:56, Dmitry Baryshkov wrote:
> > The dp_power submodule is limited to handling the clocks only following
> > previous cleanups. Fold it into the dp_ctrl submodule, removing one
> > unnecessary level of indirection.
> >
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
> > ---
> [...]
>
> >
> > - /* set dongle to D3 (power off) mode */
> > + /* set dongle to D3 (ctrl off) mode */
> :thinking emoji:
>
> [...]
>
> > +int dp_ctrl_clk_enable(struct dp_ctrl *dp_ctrl,
> > + enum dp_pm_type pm_type, bool enable)
> > +{
> > + int rc = 0;
> > + struct dp_ctrl_private *ctrl;
> > + struct dss_module_power *mp;
> I haven't checked out the rest of the series yet, but cleaning
> this up with rc->ret and reverse-Christmas-tree (merry late
> Christmas!) would be nice
>
> > +
> > + ctrl = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl);
> > +
> > + if (pm_type != DP_CORE_PM && pm_type != DP_CTRL_PM &&
> > + pm_type != DP_STREAM_PM) {
> > + DRM_ERROR("unsupported ctrl module: %s\n",
> > + dp_parser_pm_name(pm_type));
> > + return -EINVAL;
> > + }
> > +
> > + if (enable) {
> > + if (pm_type == DP_CORE_PM && ctrl->core_clks_on) {
> again, haven't yet looked through all the patches but this screams
> "if (enable && pm_type == DP_CORE_PM)"
I'd prefer to keep it as is, it follows the existing code and it is
also more idiomatic: we perform several checks if we were told to
enable clocks.
--
With best wishes
Dmitry
