Re: [PATCH 1/4] drm/i915: Add more dev ops for MIPI sub encoder

[Shobhit Kumar <shobhit.kumar@intel.com>]

On 10/21/2013 6:57 PM, Jani Nikula wrote:
>
> Hi Shobhit -
>
> On Mon, 21 Oct 2013, Shobhit Kumar <shobhit.kumar@intel.com> wrote:
>> Also add new fields in intel_dsi to have all dphy related parameters.
>> These will be useful even when we go for pure generic MIPI design
>
> I feel like we have a different idea of what the ideal generic design
> is. For me, the goal is that we change the struct intel_dsi_device to
> struct drm_bridge, and those drm_bridge drivers are all about the panel,
> and have as few details about i915 or our hardware as possible. Having
> the bridge drivers fill in register values to be written by the core DSI
> code does not fit that. Yes, I had some of those in my bridge conversion
> patches too, but I did not intend we'd keep adding more.
>
> I'd rather we provide generic helpers the bridge driver can call.

Yeah, look like our ideas are different. In your goal with drm_bridge 
architecture, we will still end up having multiple bridge drivers for 
each different panel. But my goal is to have a single driver which can 
work for multiple panels. Since we already have enabled some panels with 
sub-encoder architecture for completeness I was planning to maintain 
generic driver as one sub-encoder. But actually we can do away with all 
sub-encoder and do not need even drm_bridge and all implementation will 
be in intel_dsi.c. Panel specific configurations or sequences will come 
from VBT which I have tried to convert as parameters.

All the parameters DSI/DPHY spec specific and none of them particularly 
relate to our hardware.

>
>> Yogesh Mohan Marimuthu <yogesh.mohan.marimuthu@intel.com>
>> Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_dsi.c |    9 ++++++++-
>>   drivers/gpu/drm/i915/intel_dsi.h |   29 +++++++++++++++++++++++++++++
>>   2 files changed, 37 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
>> index 9a2fdd2..34e19b7 100644
>> --- a/drivers/gpu/drm/i915/intel_dsi.c
>> +++ b/drivers/gpu/drm/i915/intel_dsi.c
>> @@ -147,6 +147,9 @@ static void intel_dsi_enable(struct intel_encoder *encoder)
>>
>>   	DRM_DEBUG_KMS("\n");
>>
>> +	if (intel_dsi->dev.dev_ops->panel_reset)
>> +		intel_dsi->dev.dev_ops->panel_reset(&intel_dsi->dev);
>
> Would this map to ->pre_enable in drm_bridge?

I have not yet migrated to drm_bridge and need to check thes call flows 
for drm_bridge

>
>> +
>>   	temp = I915_READ(MIPI_DEVICE_READY(pipe));
>>   	if ((temp & DEVICE_READY) == 0) {
>>   		temp &= ~ULPS_STATE_MASK;
>> @@ -162,6 +165,9 @@ static void intel_dsi_enable(struct intel_encoder *encoder)
>>   		I915_WRITE(MIPI_DEVICE_READY(pipe), temp);
>>   	}
>>
>> +	if (intel_dsi->dev.dev_ops->send_otp_cmds)
>> +		intel_dsi->dev.dev_ops->send_otp_cmds(&intel_dsi->dev);
>
> What is otp? one time programming? Why not in ->enable?

Yes. OTP is done before sending pixel stream and enable is done after we 
start pixel stream

>
>> +
>>   	if (is_cmd_mode(intel_dsi))
>>   		I915_WRITE(MIPI_MAX_RETURN_PKT_SIZE(pipe), 8 * 4);
>>
>> @@ -176,7 +182,8 @@ static void intel_dsi_enable(struct intel_encoder *encoder)
>>   		POSTING_READ(MIPI_PORT_CTRL(pipe));
>>   	}
>>
>> -	intel_dsi->dev.dev_ops->enable(&intel_dsi->dev);
>> +	if (intel_dsi->dev.dev_ops->enable)
>> +		intel_dsi->dev.dev_ops->enable(&intel_dsi->dev);
>>   }
>>
>>   static void intel_dsi_disable(struct intel_encoder *encoder)
>> diff --git a/drivers/gpu/drm/i915/intel_dsi.h b/drivers/gpu/drm/i915/intel_dsi.h
>> index c7765f3..b71c9b3 100644
>> --- a/drivers/gpu/drm/i915/intel_dsi.h
>> +++ b/drivers/gpu/drm/i915/intel_dsi.h
>> @@ -39,6 +39,13 @@ struct intel_dsi_device {
>>   struct intel_dsi_dev_ops {
>>   	bool (*init)(struct intel_dsi_device *dsi);
>>
>> +	void (*panel_reset)(struct intel_dsi_device *dsi);
>> +
>> +	void (*disable_panel_power)(struct intel_dsi_device *dsi);
>
> What is the enabling counterpart to disable_panel_power? panel_reset?

Yes.

>
>> +
>> +	/* send one time programmable commands */
>> +	void (*send_otp_cmds)(struct intel_dsi_device *dsi);
>> +
>>   	/* This callback must be able to assume DSI commands can be sent */
>>   	void (*enable)(struct intel_dsi_device *dsi);
>>
>> @@ -89,6 +96,28 @@ struct intel_dsi {
>>
>>   	/* eot for MIPI_EOT_DISABLE register */
>>   	u32 eot_disable;
>> +
>> +	u16 dsi_clock_freq;
>> +	u8 video_mode_type;
>> +	u32 data_width;
>> +	u8 dither;
>> +	u32 port_bits;
>> +	u8 escape_clk_div;
>> +	u32 lp_rx_timeout;
>> +	u8 turn_arnd_val;
>> +	u16 init_count;
>> +	u16 rst_timer_val;
>> +	u16 hs_to_lp_count;
>> +	u16 lp_byte_clk;
>> +	u32 bw_timer;
>> +	u16 clk_lp_to_hs_count;
>> +	u16 clk_hs_to_lp_count;
>> +	u32 video_frmt_cfg_bits;
>> +	u32 dphy_reg;
>> +
>> +	u8 backlight_off_delay; /*in ms*/
>> +	bool send_shutdown;
>> +	u8 shutdown_pkt_delay; /*in ms*/
>>   };
>>
>>   static inline struct intel_dsi *enc_to_intel_dsi(struct drm_encoder *encoder)
>> --
>> 1.7.9.5
>>
>

Regards
Shobhit