[PATCH 4/5] fb: Add DCU framebuffer driver for Vybrid VF610 platform

[Wang Huan-B18965]

> On Mon, Aug 05, 2013 at 09:51:40AM +0000, Wang Huan-B18965 wrote:
> > Hi, Sascha,
> >
> > > On Fri, Jul 12, 2013 at 02:07:55PM +0800, Alison Wang wrote:
> > > > The Display Controller Unit (DCU) module is a system master that
> > > > fetches graphics stored in internal or external memory and
> > > > displays them on a TFT LCD panel. A wide range of panel sizes is
> > > > supported and the timing of the interface signals is highly
> configurable.
> > > > Graphics are read directly from memory and then blended in
> > > > real-time, which allows for dynamic content creation with minimal
> > > > CPU
> > > intervention.
> > >
> > > Only a review of the code inline.
> > >
> > > Maybe the real question is whether we want to introduce another
> > > framebuffer driver at all instead of making it a DRM driver.
> > [Alison Wang] I think DCU module is more suitable to be designed as a
> framebuffer driver than a DRM driver. Just like DIU framebuffer driver
> for PowerPC.
> > >
> > > > +
> > > > +#define DRIVER_NAME			"fsl-dcu-fb"
> > > > +
> > > > +#define DCU_DCU_MODE			0x0010
> > > > +#define DCU_MODE_BLEND_ITER(x)		(x << 20)
> > >
> > > What's the result of DCU_MODE_BLEND_ITER(1 + 1)?
> > [Alison Wang] Is it really necessary? I don't use this macro like
> DCU_MODE_BLEND_ITER(1 + 1), just use DCU_MODE_BLEND_ITER(2).
> 
> <irony>
> No it's not necessary. We can just add incorrect macros everywhere and
> fix them as necessary after a long bug squashing session </irony>
> 
> YES! This is necessary.
[Alison Wang] Ok, I will change it as follows.
#define DCU_MODE_BLEND_ITER(x)	((x) << 20)
> 
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static int calc_div_ratio(struct fb_info *info) {
> > >
> > > Use a consistent namespace for function names (fsl_dcu_)
> > [Alison Wang] Is it necessary to use a consistent namespace for this
> generic function? I think it is necessary to use a consistent namespace
> (fsl_dcu_) for the function names in structure fsl_dcu_ops.
> 
> This function calculates some divider out of a struct fb_info. This is
> by no means generic.
[Alison Wang] Ok, I will use a consistent namespace for this function.
> 
> > > > +		if (copy_to_user(buf, &alpha, sizeof(alpha)))
> > > > +			return -EFAULT;
> > > > +		break;
> > > > +	case MFB_SET_ALPHA:
> > > > +		if (copy_from_user(&alpha, buf, sizeof(alpha)))
> > > > +			return -EFAULT;
> > > > +		mfbi->blend = 1;
> > > > +		mfbi->alpha = alpha;
> > > > +		fsl_dcu_set_par(info);
> > > > +		break;
> > >
> > > Is it still state of the art to introduce ioctls in the fb
> framework
> > > without any kind of documentation?
> > [Alison Wang] Do you mean I need to add a documentation in
> Documentation/fb/?
> 
> This was more a question to the fb maintainers.
> 
> > > > +static irqreturn_t fsl_dcu_irq(int irq, void *dev_id) {
> > > > +	struct dcu_fb_data *dcufb = dev_id;
> > > > +	unsigned int status = readl(dcufb->reg_base +
> DCU_INT_STATUS);
> > > > +	u32 dcu_mode;
> > > > +
> > > > +	if (status) {
> > >
> > > Save indendation level by bailing out early.
> > [Alison Wang] What do you mean?
> 
> 
> Instead of doing:
> 
> 	if (bla) {
> 		dothis();
> 		dothat();
> 		return;
> 	}
> 
> 	return;
> 
> it's easier to read:
> 
> 	if (!bla)
> 		return;
> 
> 	dothis();
> 	dothat();
> 	return;
> 
> Note that dothis() and dothat() are one indentation level to the left
> and thus have more space per line.
[Alison Wang] I see. Thanks.


Best Regards,
Alison Wang