Re: [PATCH 1/4] fbdev: add a MIPI DSI header

["Ville Syrjälä" <ville.syrjala@nokia.com>]

On Wed, May 19, 2010 at 10:21:48AM +0200, Valkeinen Tomi (Nokia-D/Helsinki) wrote:
> Hi,
> 
> On Wed, 2010-05-19 at 10:08 +0200, ext Guennadi Liakhovetski wrote:
> > Hi Tomi
> > 
> > On Wed, 19 May 2010, Tomi Valkeinen wrote:
> > > > +	MIPI_DSI_DCS_SHORT_WRITE			= 5,
> > > 
> > > Please use the same number format for all enums. So this should be 0x05.
> > 
> > Where does this requirement come from? Is it in CodingStyle?;)
> 
> I don't know, but mixing different formats looks ugly to me =).

One case where it would perhaps make sense if you define some fields
with shifts and masks. Specifying shifts as decimal and masks as hex
is clearer IMO. But in this case I agree it just looks ugly and only
serves to confuse the reader as he tries to figure out if there's
some magic meaning to the different formats.

> 
> > > > +	MIPI_DSI_DCS_SHORT_WRITE_PARAM			= 0x15,
> > > > +	MIPI_DSI_COLOR_MODE_OFF				= 2,
> > > > +	MIPI_DSI_COLOR_MODE_ON				= 0x12,
> > > > +	MIPI_DSI_SHUTDOWN_PERIPHERAL			= 0x22,
> > > > +	MIPI_DSI_TURN_ON_PERIPHERAL			= 0x32,
> > > > +	MIPI_DSI_GENERIC_SHORT_WRITE_0_PARAM		= 3,
> > > > +	MIPI_DSI_GENERIC_SHORT_WRITE_1_PARAM		= 0x13,
> > > > +	MIPI_DSI_GENERIC_SHORT_WRITE_2_PARAM		= 0x23,
> > > > +	MIPI_DSI_GENERIC_READ_REQUEST_0_PARAM		= 4,
> > > > +	MIPI_DSI_GENERIC_READ_REQUEST_1_PARAM		= 0x14,
> > > > +	MIPI_DSI_GENERIC_READ_REQUEST_2_PARAM		= 0x24,
> > > > +	MIPI_DSI_DCS_LONG_WRITE				= 0x39,
> > > > +	MIPI_DSI_SET_MAXIMUM_RETURN_PACKET_SIZE		= 0x37,
> > > > +	MIPI_DSI_NULL_PACKET				= 9,
> > > > +	MIPI_DSI_BLANKING_PACKET			= 0x19,
> > > > +	MIPI_DSI_GENERIC_LONG_WRITE			= 0x29,
> > > > +	MIPI_DSI_LOOSELY_PACKED_PIXEL_STREAM_YCBCR20	= 0xc,
> > > > +	MIPI_DSI_PACKED_PIXEL_STREAM_YCBCR24		= 0x1c,
> > > > +	MIPI_DSI_PACKED_PIXEL_STREAM_YCBCR16		= 0x2c,
> > > > +	MIPI_DSI_PACKED_PIXEL_STREAM_30			= 0xd,
> > > > +	MIPI_DSI_PACKED_PIXEL_STREAM_36			= 0x1d,
> > > > +	MIPI_DSI_PACKED_PIXEL_STREAM_YCBCR12		= 0x3d,
> > > > +	MIPI_DSI_PACKED_PIXEL_STREAM_16			= 0xe,
> > > > +	MIPI_DSI_PACKED_PIXEL_STREAM_18			= 0x1e,
> > > > +	MIPI_DSI_PIXEL_STREAM_3BYTE_18			= 0x2e,
> > > > +	MIPI_DSI_PACKED_PIXEL_STREAM_24			= 0x3e,
> > > > +};
> > > > +
> > > > +enum mipi_dcs_cmd {
> > > 
> > > While the MIPI specs define a certain set of commands, the real panels
> > > usually implement also their own custom commands. That's why I don't
> > > think enum is a good choice here.
> > 
> > Yes, that's a valid point, I'll have to think about it more.
> 
> I think a simple solution would be to just use defines, and have
> functions that take the command as u8. That's what the OMAP DSI driver
> does. If you have better ideas, please share =).

I find enums easier on the eye than defines. Less irrelevant junk on
each line. There's no reason you can't pass enum values as u8. But in
that case giving the enum a name doesn't really make sense.

-- 
Ville Syrjälä