Hello,
On Tue, May 04, 2010 at 14:57:05, Sergei Shtylyov wrote:
> Hello.
>
> Sekhar Nori wrote:
> > In the edma driver some if statememnts are broken up such that
> > the condition code is at the same indentation level as the
> > statement block. This makes reading it particularly difficult.
> >
> > This patch fixes that issue.
> >
> > Signed-off-by: Sekhar Nori <[email protected]>
> > ---
> > arch/arm/mach-davinci/dma.c | 26 +++++++++++++-------------
> > 1 files changed, 13 insertions(+), 13 deletions(-)
> >
> > diff --git a/arch/arm/mach-davinci/dma.c b/arch/arm/mach-davinci/dma.c
> > index cc5fcda..2734de9 100644
> > --- a/arch/arm/mach-davinci/dma.c
> > +++ b/arch/arm/mach-davinci/dma.c
> > @@ -352,7 +352,7 @@ static irqreturn_t dma_irq_handler(int irq, void *data)
> > dev_dbg(data, "dma_irq_handler\n");
> >
> > if ((edma_shadow0_read_array(ctlr, SH_IPR, 0) == 0)
> > - && (edma_shadow0_read_array(ctlr, SH_IPR, 1) == 0))
> >
>
> IMHO this level of indentation doesn't affect the readability.
The original code was indented using spaces so the level of
indentation can change depending on your tabstop setting.
>
> > + && (edma_shadow0_read_array(ctlr, SH_IPR, 1) == 0))
> > return IRQ_NONE;
> >
> > while (1) {
> > @@ -406,9 +406,9 @@ static irqreturn_t dma_ccerr_handler(int irq, void
> > *data)
> > dev_dbg(data, "dma_ccerr_handler\n");
> >
> > if ((edma_read_array(ctlr, EDMA_EMR, 0) == 0) &&
> > - (edma_read_array(ctlr, EDMA_EMR, 1) == 0) &&
> > - (edma_read(ctlr, EDMA_QEMR) == 0) &&
> > - (edma_read(ctlr, EDMA_CCERR) == 0))
> >
>
> What's up with these too?
>
> > + (edma_read_array(ctlr, EDMA_EMR, 1) == 0) &&
> > + (edma_read(ctlr, EDMA_QEMR) == 0) &&
> > + (edma_read(ctlr, EDMA_CCERR) == 0))
> >
>
> Your increased indentation looks rather ugly to me... matter of
> taste, of course.
Same here, spaces were being used for indentation. I realize
now I should have rather stated this issue in patch description.
Also, broken condition expressions at the same indentation level
as the actual statement are tough to read. Hence the increased
indentation.
>
> > @@ -469,9 +469,9 @@ static irqreturn_t dma_ccerr_handler(int irq, void
> > *data)
> > }
> > }
> > if ((edma_read_array(ctlr, EDMA_EMR, 0) == 0)
> > - && (edma_read_array(ctlr, EDMA_EMR, 1) == 0)
> > - && (edma_read(ctlr, EDMA_QEMR) == 0)
> > - && (edma_read(ctlr, EDMA_CCERR) == 0))
> >
>
> Again, I don't find anything bad about this, except I'd put && at the
> end of lines, no at the start...
Spaces at play again. I agree on the && part. But that's
out of scope for $SUBJECT.
Thanks,
Sekhar
_______________________________________________
Davinci-linux-open-source mailing list
[email protected]
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
