On 2023-06-20 14:38:34, Jessica Zhang wrote: <snip> > >>>>> + 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. > > Hi Dmitry, > > After discussing this approach with Abhinav, we still have a few > questions about it: > > Currently, only 3 of the 32 bits for INTF_CONFIG2 are being used (the > rest are reserved with no plans of being programmed in the future). Does > this still justify the use of a struct to pass in the necessary > configuration? No. The point Dmitry is making is **not** about this concidentally using the same register, but about adding a common codepath to enable compression on this hw_intf (regardless of the registers it needs to touch). Similar to how dpu_hw_intf_setup_timing_engine() programs the hw_intf - including widebus! - for video-mode. Or even more generically, have a struct similar to intf_timing_params that says how the intf needs to be configured - without the caller knowing about INTF_CONFIG2. struct dpu_hw_intf_cfg is a very good example of how we can use a single struct and a single callback to configure multiple registers at once based on some input parameters. > In addition, it seems that video mode does all its INTF_CONFIG2 > configuration separately in dpu_hw_intf_setup_timing_engine(). If we > have a generic set_intf_config2() op, it might be good to have it as > part of a larger cleanup where we have both video and command mode use > the generic op. What are your thoughts on this? Not in that way, but if there is a generic enable_compression() or configure_compression() callback (or even more generic, similar to setup_intf_cfg in dpu_hw_ctl) that would work for both video-mode and command-mode, maybe that is beneficial. - Marijn
