| Message ID | 20241212-fd-dp-audio-fixup-v3-13-0b1c65e7dba3@linaro.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | drm/msm/dp: perform misc cleanups | expand |
On 12/11/2024 3:41 PM, Dmitry Baryshkov wrote: > All other submodules pass arguments directly. Drop struct > msm_dp_panel_in that is used to wrap dp_panel's submodule args and pass > all data to msm_dp_panel_get() directly. > > Reviewed-by: Stephen Boyd <swboyd@chromium.org> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > --- > drivers/gpu/drm/msm/dp/dp_display.c | 9 +-------- > drivers/gpu/drm/msm/dp/dp_panel.c | 15 ++++++++------- > drivers/gpu/drm/msm/dp/dp_panel.h | 10 ++-------- > 3 files changed, 11 insertions(+), 23 deletions(-) > Change not necessarily tied to catalog cleanup, and can be sent independently IMO. > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c > index cb02d5d5b404925707c737ed75e9e83fbec34f83..a2cdcdac042d63a59ff71aefcecb7f8b22f01167 100644 > --- a/drivers/gpu/drm/msm/dp/dp_display.c > +++ b/drivers/gpu/drm/msm/dp/dp_display.c > @@ -722,9 +722,6 @@ static int msm_dp_init_sub_modules(struct msm_dp_display_private *dp) > { > int rc = 0; > struct device *dev = &dp->msm_dp_display.pdev->dev; > - struct msm_dp_panel_in panel_in = { > - .dev = dev, > - }; > struct phy *phy; > > phy = devm_phy_get(dev, "dp"); > @@ -765,11 +762,7 @@ static int msm_dp_init_sub_modules(struct msm_dp_display_private *dp) > goto error_link; > } > > - panel_in.aux = dp->aux; > - panel_in.catalog = dp->catalog; > - panel_in.link = dp->link; > - > - dp->panel = msm_dp_panel_get(&panel_in); > + dp->panel = msm_dp_panel_get(dev, dp->aux, dp->link, dp->catalog); > if (IS_ERR(dp->panel)) { > rc = PTR_ERR(dp->panel); > DRM_ERROR("failed to initialize panel, rc = %d\n", rc); > diff --git a/drivers/gpu/drm/msm/dp/dp_panel.c b/drivers/gpu/drm/msm/dp/dp_panel.c > index 25869e2ac93aba0bffeddae9f95917d81870d8cb..49bbcde8cf60ac1b297310a50191135d79b092fb 100644 > --- a/drivers/gpu/drm/msm/dp/dp_panel.c > +++ b/drivers/gpu/drm/msm/dp/dp_panel.c > @@ -659,25 +659,26 @@ static int msm_dp_panel_parse_dt(struct msm_dp_panel *msm_dp_panel) > return 0; > } > > -struct msm_dp_panel *msm_dp_panel_get(struct msm_dp_panel_in *in) > +struct msm_dp_panel *msm_dp_panel_get(struct device *dev, struct drm_dp_aux *aux, > + struct msm_dp_link *link, struct msm_dp_catalog *catalog) > { so this API, takes a filled input panel, makes a msm_dp_panel out of it by filling out more information on top of what was already passed in and returns a msm_dp_panel. So IOW, converts a msm_dp_panel_in to msm_dp_panel. What is the gain by passing individual params rather than passing them as a struct instead? Isnt it better to have it within that struct to show the conversion and moreover we dont have to pass in 4 arguments instead of 1. > struct msm_dp_panel_private *panel; > struct msm_dp_panel *msm_dp_panel; > int ret; > > - if (!in->dev || !in->catalog || !in->aux || !in->link) { > + if (!dev || !catalog || !aux || !link) { > DRM_ERROR("invalid input\n"); > return ERR_PTR(-EINVAL); > } > > - panel = devm_kzalloc(in->dev, sizeof(*panel), GFP_KERNEL); > + panel = devm_kzalloc(dev, sizeof(*panel), GFP_KERNEL); > if (!panel) > return ERR_PTR(-ENOMEM); > > - panel->dev = in->dev; > - panel->aux = in->aux; > - panel->catalog = in->catalog; > - panel->link = in->link; > + panel->dev = dev; > + panel->aux = aux; > + panel->catalog = catalog; > + panel->link = link; > > msm_dp_panel = &panel->msm_dp_panel; > msm_dp_panel->max_bw_code = DP_LINK_BW_8_1; > diff --git a/drivers/gpu/drm/msm/dp/dp_panel.h b/drivers/gpu/drm/msm/dp/dp_panel.h > index f305b1151118b53762368905b70d951a366ba1a8..a4719a3bbbddd18304227a006e82a5ce9ad7bbf3 100644 > --- a/drivers/gpu/drm/msm/dp/dp_panel.h > +++ b/drivers/gpu/drm/msm/dp/dp_panel.h > @@ -21,13 +21,6 @@ struct msm_dp_display_mode { > bool out_fmt_is_yuv_420; > }; > > -struct msm_dp_panel_in { > - struct device *dev; > - struct drm_dp_aux *aux; > - struct msm_dp_link *link; > - struct msm_dp_catalog *catalog; > -}; > - > struct msm_dp_panel_psr { > u8 version; > u8 capabilities; > @@ -94,6 +87,7 @@ static inline bool is_lane_count_valid(u32 lane_count) > lane_count == 4); > } > > -struct msm_dp_panel *msm_dp_panel_get(struct msm_dp_panel_in *in); > +struct msm_dp_panel *msm_dp_panel_get(struct device *dev, struct drm_dp_aux *aux, > + struct msm_dp_link *link, struct msm_dp_catalog *catalog); > void msm_dp_panel_put(struct msm_dp_panel *msm_dp_panel); > #endif /* _DP_PANEL_H_ */ >
On Thu, 12 Dec 2024 at 05:26, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: > > > > On 12/11/2024 3:41 PM, Dmitry Baryshkov wrote: > > All other submodules pass arguments directly. Drop struct > > msm_dp_panel_in that is used to wrap dp_panel's submodule args and pass > > all data to msm_dp_panel_get() directly. > > > > Reviewed-by: Stephen Boyd <swboyd@chromium.org> > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > --- > > drivers/gpu/drm/msm/dp/dp_display.c | 9 +-------- > > drivers/gpu/drm/msm/dp/dp_panel.c | 15 ++++++++------- > > drivers/gpu/drm/msm/dp/dp_panel.h | 10 ++-------- > > 3 files changed, 11 insertions(+), 23 deletions(-) > > > > Change not necessarily tied to catalog cleanup, and can be sent > independently IMO. > > > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c > > index cb02d5d5b404925707c737ed75e9e83fbec34f83..a2cdcdac042d63a59ff71aefcecb7f8b22f01167 100644 > > --- a/drivers/gpu/drm/msm/dp/dp_display.c > > +++ b/drivers/gpu/drm/msm/dp/dp_display.c > > @@ -722,9 +722,6 @@ static int msm_dp_init_sub_modules(struct msm_dp_display_private *dp) > > { > > int rc = 0; > > struct device *dev = &dp->msm_dp_display.pdev->dev; > > - struct msm_dp_panel_in panel_in = { > > - .dev = dev, > > - }; > > struct phy *phy; > > > > phy = devm_phy_get(dev, "dp"); > > @@ -765,11 +762,7 @@ static int msm_dp_init_sub_modules(struct msm_dp_display_private *dp) > > goto error_link; > > } > > > > - panel_in.aux = dp->aux; > > - panel_in.catalog = dp->catalog; > > - panel_in.link = dp->link; > > - > > - dp->panel = msm_dp_panel_get(&panel_in); > > + dp->panel = msm_dp_panel_get(dev, dp->aux, dp->link, dp->catalog); > > if (IS_ERR(dp->panel)) { > > rc = PTR_ERR(dp->panel); > > DRM_ERROR("failed to initialize panel, rc = %d\n", rc); > > diff --git a/drivers/gpu/drm/msm/dp/dp_panel.c b/drivers/gpu/drm/msm/dp/dp_panel.c > > index 25869e2ac93aba0bffeddae9f95917d81870d8cb..49bbcde8cf60ac1b297310a50191135d79b092fb 100644 > > --- a/drivers/gpu/drm/msm/dp/dp_panel.c > > +++ b/drivers/gpu/drm/msm/dp/dp_panel.c > > @@ -659,25 +659,26 @@ static int msm_dp_panel_parse_dt(struct msm_dp_panel *msm_dp_panel) > > return 0; > > } > > > > -struct msm_dp_panel *msm_dp_panel_get(struct msm_dp_panel_in *in) > > +struct msm_dp_panel *msm_dp_panel_get(struct device *dev, struct drm_dp_aux *aux, > > + struct msm_dp_link *link, struct msm_dp_catalog *catalog) > > { > > so this API, takes a filled input panel, makes a msm_dp_panel out of it > by filling out more information on top of what was already passed in and > returns a msm_dp_panel. > > So IOW, converts a msm_dp_panel_in to msm_dp_panel. > > What is the gain by passing individual params rather than passing them > as a struct instead? Isnt it better to have it within that struct to > show the conversion and moreover we dont have to pass in 4 arguments > instead of 1. We gain uniformity. All other modules use params. And, as pointed out by Maxime during HDMI Codec reviews, it's easier to handle function params - it makes it more obvious that one of the params got missing.
On 12/12/2024 12:53 AM, Dmitry Baryshkov wrote: > On Thu, 12 Dec 2024 at 05:26, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: >> >> >> >> On 12/11/2024 3:41 PM, Dmitry Baryshkov wrote: >>> All other submodules pass arguments directly. Drop struct >>> msm_dp_panel_in that is used to wrap dp_panel's submodule args and pass >>> all data to msm_dp_panel_get() directly. >>> >>> Reviewed-by: Stephen Boyd <swboyd@chromium.org> >>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >>> --- >>> drivers/gpu/drm/msm/dp/dp_display.c | 9 +-------- >>> drivers/gpu/drm/msm/dp/dp_panel.c | 15 ++++++++------- >>> drivers/gpu/drm/msm/dp/dp_panel.h | 10 ++-------- >>> 3 files changed, 11 insertions(+), 23 deletions(-) >>> >> >> Change not necessarily tied to catalog cleanup, and can be sent >> independently IMO. >> >>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c >>> index cb02d5d5b404925707c737ed75e9e83fbec34f83..a2cdcdac042d63a59ff71aefcecb7f8b22f01167 100644 >>> --- a/drivers/gpu/drm/msm/dp/dp_display.c >>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c >>> @@ -722,9 +722,6 @@ static int msm_dp_init_sub_modules(struct msm_dp_display_private *dp) >>> { >>> int rc = 0; >>> struct device *dev = &dp->msm_dp_display.pdev->dev; >>> - struct msm_dp_panel_in panel_in = { >>> - .dev = dev, >>> - }; >>> struct phy *phy; >>> >>> phy = devm_phy_get(dev, "dp"); >>> @@ -765,11 +762,7 @@ static int msm_dp_init_sub_modules(struct msm_dp_display_private *dp) >>> goto error_link; >>> } >>> >>> - panel_in.aux = dp->aux; >>> - panel_in.catalog = dp->catalog; >>> - panel_in.link = dp->link; >>> - >>> - dp->panel = msm_dp_panel_get(&panel_in); >>> + dp->panel = msm_dp_panel_get(dev, dp->aux, dp->link, dp->catalog); >>> if (IS_ERR(dp->panel)) { >>> rc = PTR_ERR(dp->panel); >>> DRM_ERROR("failed to initialize panel, rc = %d\n", rc); >>> diff --git a/drivers/gpu/drm/msm/dp/dp_panel.c b/drivers/gpu/drm/msm/dp/dp_panel.c >>> index 25869e2ac93aba0bffeddae9f95917d81870d8cb..49bbcde8cf60ac1b297310a50191135d79b092fb 100644 >>> --- a/drivers/gpu/drm/msm/dp/dp_panel.c >>> +++ b/drivers/gpu/drm/msm/dp/dp_panel.c >>> @@ -659,25 +659,26 @@ static int msm_dp_panel_parse_dt(struct msm_dp_panel *msm_dp_panel) >>> return 0; >>> } >>> >>> -struct msm_dp_panel *msm_dp_panel_get(struct msm_dp_panel_in *in) >>> +struct msm_dp_panel *msm_dp_panel_get(struct device *dev, struct drm_dp_aux *aux, >>> + struct msm_dp_link *link, struct msm_dp_catalog *catalog) >>> { >> >> so this API, takes a filled input panel, makes a msm_dp_panel out of it >> by filling out more information on top of what was already passed in and >> returns a msm_dp_panel. >> >> So IOW, converts a msm_dp_panel_in to msm_dp_panel. >> >> What is the gain by passing individual params rather than passing them >> as a struct instead? Isnt it better to have it within that struct to >> show the conversion and moreover we dont have to pass in 4 arguments >> instead of 1. > > We gain uniformity. All other modules use params. And, as pointed out > by Maxime during HDMI Codec reviews, it's easier to handle function > params - it makes it more obvious that one of the params got missing. > Point noted but a very long param list also makes it harder to manage. So we should really evaluate on a case-by-case basis and not generalize here. Here its only 4, so i would say its kindof okay. If it goes beyond it, then msm_dp_panel_in is probably going to come back. Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c index cb02d5d5b404925707c737ed75e9e83fbec34f83..a2cdcdac042d63a59ff71aefcecb7f8b22f01167 100644 --- a/drivers/gpu/drm/msm/dp/dp_display.c +++ b/drivers/gpu/drm/msm/dp/dp_display.c @@ -722,9 +722,6 @@ static int msm_dp_init_sub_modules(struct msm_dp_display_private *dp) { int rc = 0; struct device *dev = &dp->msm_dp_display.pdev->dev; - struct msm_dp_panel_in panel_in = { - .dev = dev, - }; struct phy *phy; phy = devm_phy_get(dev, "dp"); @@ -765,11 +762,7 @@ static int msm_dp_init_sub_modules(struct msm_dp_display_private *dp) goto error_link; } - panel_in.aux = dp->aux; - panel_in.catalog = dp->catalog; - panel_in.link = dp->link; - - dp->panel = msm_dp_panel_get(&panel_in); + dp->panel = msm_dp_panel_get(dev, dp->aux, dp->link, dp->catalog); if (IS_ERR(dp->panel)) { rc = PTR_ERR(dp->panel); DRM_ERROR("failed to initialize panel, rc = %d\n", rc); diff --git a/drivers/gpu/drm/msm/dp/dp_panel.c b/drivers/gpu/drm/msm/dp/dp_panel.c index 25869e2ac93aba0bffeddae9f95917d81870d8cb..49bbcde8cf60ac1b297310a50191135d79b092fb 100644 --- a/drivers/gpu/drm/msm/dp/dp_panel.c +++ b/drivers/gpu/drm/msm/dp/dp_panel.c @@ -659,25 +659,26 @@ static int msm_dp_panel_parse_dt(struct msm_dp_panel *msm_dp_panel) return 0; } -struct msm_dp_panel *msm_dp_panel_get(struct msm_dp_panel_in *in) +struct msm_dp_panel *msm_dp_panel_get(struct device *dev, struct drm_dp_aux *aux, + struct msm_dp_link *link, struct msm_dp_catalog *catalog) { struct msm_dp_panel_private *panel; struct msm_dp_panel *msm_dp_panel; int ret; - if (!in->dev || !in->catalog || !in->aux || !in->link) { + if (!dev || !catalog || !aux || !link) { DRM_ERROR("invalid input\n"); return ERR_PTR(-EINVAL); } - panel = devm_kzalloc(in->dev, sizeof(*panel), GFP_KERNEL); + panel = devm_kzalloc(dev, sizeof(*panel), GFP_KERNEL); if (!panel) return ERR_PTR(-ENOMEM); - panel->dev = in->dev; - panel->aux = in->aux; - panel->catalog = in->catalog; - panel->link = in->link; + panel->dev = dev; + panel->aux = aux; + panel->catalog = catalog; + panel->link = link; msm_dp_panel = &panel->msm_dp_panel; msm_dp_panel->max_bw_code = DP_LINK_BW_8_1; diff --git a/drivers/gpu/drm/msm/dp/dp_panel.h b/drivers/gpu/drm/msm/dp/dp_panel.h index f305b1151118b53762368905b70d951a366ba1a8..a4719a3bbbddd18304227a006e82a5ce9ad7bbf3 100644 --- a/drivers/gpu/drm/msm/dp/dp_panel.h +++ b/drivers/gpu/drm/msm/dp/dp_panel.h @@ -21,13 +21,6 @@ struct msm_dp_display_mode { bool out_fmt_is_yuv_420; }; -struct msm_dp_panel_in { - struct device *dev; - struct drm_dp_aux *aux; - struct msm_dp_link *link; - struct msm_dp_catalog *catalog; -}; - struct msm_dp_panel_psr { u8 version; u8 capabilities; @@ -94,6 +87,7 @@ static inline bool is_lane_count_valid(u32 lane_count) lane_count == 4); } -struct msm_dp_panel *msm_dp_panel_get(struct msm_dp_panel_in *in); +struct msm_dp_panel *msm_dp_panel_get(struct device *dev, struct drm_dp_aux *aux, + struct msm_dp_link *link, struct msm_dp_catalog *catalog); void msm_dp_panel_put(struct msm_dp_panel *msm_dp_panel); #endif /* _DP_PANEL_H_ */