> 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
��.n��������+%������w��{.n�����{����n�r������&��z�ޗ�zf���h���~����������_��+v���)ߣ�
