On Thu, Nov 14, 2013 at 03:04:19PM +0000, Bert Kenward wrote:
On 11/14/2013 14:16, Andrzej Hajda wrote:
On 11/13/2013 10:38 PM, Thierry Reding wrote:
Furthermore I think if we kept the transfer function proposed in my patch should make it easier to address Bert's comments from your posting.
I am not sure which part of Barts comment you are addressing. Anyway I also prefer passing struct and returning ssize_t. I am not sure about splitting type and channel but this seems to be a minor detail.
Most of the comments I made about Andrzej's patch from last month apply here, and would result in extensions to struct dsi_msg. Some means of choosing whether the transfer should be in HS or LP mode is necessary. For video mode panels some means of specifying a period (VFP, VBP, etc) for sending the transfer is needed. Perhaps struct dsi_msg should include:
#define DSI_WINDOW_VFP (1 << 0) #define DSI_WINDOW_ACT (1 << 1) #define DSI_WINDOW_VBP (1 << 2) #define DSI_WINDOW_VSY (1 << 3)
/**
- struct dsi_msg - DSI command message
- @channel: virtual channel to send the message to
- @type: data ID of the message
- @lp_mode: send in LP mode if non-zero
Perhaps a flags field would be more flexible here. I can easily imagine other I can imagine that other flags may be needed eventually.
- @window: video period when transfer is allowed - bitmask of DSI_WINDOW_*
I'm not sure if this is the right interface. What will happen for instance if the hardware doesn't support any of the bits in that mask? Perhaps a slightly better approach might be to expose the capabilities of the DSI host, so that the DSI core knows up front which windows can be used.
- @tx_len: length of transmission buffer
- @tx: transmission buffer
- @rx_len: length of reception buffer
- @rx: reception buffer
*/ struct dsi_msg { u8 channel; u8 type; u8 lp_mode; u8 window;
size_t tx_len; void *tx;
size_t rx_len; void *rx; };
The ability to specify synchronisation with a tearing effect control signal for a command mode panel is obviously important. It's somewhat analogous to the "window" option for video mode.
They're little used, but a mechanism for sending triggers should be included. They're probably sufficiently different that it should be a different op.
I agree. While I currently have no use-case where this is required, I think having a struct dsi_msg makes it easier to extend the featureset on an as-needed basis.
Thierry