On 17/06/2023 00:54, Marijn Suijten wrote:
On 2023-06-16 14:18:39, Abhinav Kumar wrote:
On 6/14/2023 12:56 AM, Dmitry Baryshkov wrote:
On 14/06/2023 04:57, Jessica Zhang wrote:
Add a DPU INTF op to set the DATABUS_WIDEN register to enable the
databus-widen mode datapath.
Signed-off-by: Jessica Zhang <quic_jesszhan@xxxxxxxxxxx>
---
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 3 +++
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 12 ++++++++++++
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h | 3 +++
3 files changed, 18 insertions(+)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
index b856c6286c85..124ba96bebda 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
@@ -70,6 +70,9 @@ static void _dpu_encoder_phys_cmd_update_intf_cfg(
if (intf_cfg.dsc != 0 && phys_enc->hw_intf->ops.enable_compression)
phys_enc->hw_intf->ops.enable_compression(phys_enc->hw_intf);
+
+ if (phys_enc->hw_intf->ops.enable_widebus)
+ phys_enc->hw_intf->ops.enable_widebus(phys_enc->hw_intf);
No. Please provide a single function which takes necessary
configuration, including compression and wide_bus_enable.
There are two ways to look at this. Your point is coming from the
perspective that its programming the same register but just a different
bit. But that will also make it a bit confusing.
My point is to have a high-level function that configures the INTF for
the CMD mode. This way it can take a structure with necessary
configuration bits.
So lets say the function is called intf_cfg2_xxx(..., bool widebus, bool
data_compress)
Now for the caller who wants to just enable widebus this will be
intf_cfg2_xxx(....., true, false)
if we want to do both
intf_cfg2_xxx(...., true, true)
the last one where we want to do just data_compress(highly unlikely and
not recommended)
intf_cfg2_xxx(...., false, true)
Now someone looking at the code will have to go and find out what each
bool is.
Whereas with separate ops, its kind of implicit.
That's why you never pass bools as function argument (edge-case if it is
the only argument, and its meaning becomes clear from the function
name). Use enumerations anywhere else.
- Marijn
For that reason, I dont think this patch is bad at all.
--
With best wishes
Dmitry
