Re: [PATCH 01/05] dmaengine: shdma: Remove sh_dmae_slave_chan_id

[Guennadi Liakhovetski <g.liakhovetski@gmx.de>]

On Fri, 19 Mar 2010, Magnus Damm wrote:

> Hey Guennadi,
> 
> On Fri, Mar 19, 2010 at 4:30 PM, Guennadi Liakhovetski
> <g.liakhovetski@gmx.de> wrote:
> > On Fri, 19 Mar 2010, Magnus Damm wrote:
> >> --- 0001/arch/sh/include/asm/dmaengine.h
> >> +++ work/arch/sh/include/asm/dmaengine.h      2010-03-18 23:30:46.000000000 +0900
> >> @@ -17,7 +17,7 @@
> >>
> >>  #define SH_DMAC_MAX_CHANNELS 6
> >>
> >> -enum sh_dmae_slave_chan_id {
> >> +enum {
> >>       SHDMA_SLAVE_SCIF0_TX,
> >>       SHDMA_SLAVE_SCIF0_RX,
> >>       SHDMA_SLAVE_SCIF1_TX,
> >> @@ -38,7 +38,7 @@ enum sh_dmae_slave_chan_id {
> >>  };
> >>
> >>  struct sh_dmae_slave_config {
> >> -     enum sh_dmae_slave_chan_id      slave_id;
> >> +     unsigned int                    slave_id;
> >
> > I think, INT_MAX DMA slave IDs will be enough;) So, you could just use an
> > int to save typing and to reserve -1 (or -EINVAL) as an invalid DMA ID. As
> > I am currencly realising, it can be useful, e.g., if you want to enable
> > DMA only on Rx and not on Tx of some device. This, of course, throughout
> > this patch.
> 
> Reserving some number sounds like a good plan, but exactly what is the
> best is a different question. If I'm allowed to nitpick then I think
> its wasteful to use one bit for the sign when only needing one value
> to mark unused. So I'd prefer to go with unsigned and 0 as unused mark
> and the rest as valid ids.

Well, personally I somewhat dislike using 0 as an invalid ID (or invalid 
IRQ...). Then you have to remember to use an offset:

	if ((unsigned)slave_id >= SHDMA_SLAVE_NUMBER)

gets an oddition of "|| !slave_id", and you're wasting one bit in 
sh_dmae_slave_used[];) And, as I said - extra typing:)

> >> --- 0001/drivers/dma/shdma.c
> >> +++ work/drivers/dma/shdma.c  2010-03-18 23:30:46.000000000 +0900
> >> @@ -266,7 +266,7 @@ static struct sh_desc *sh_dmae_get_desc(
> >>  }
> >>
> >>  static struct sh_dmae_slave_config *sh_dmae_find_slave(
> >> -     struct sh_dmae_chan *sh_chan, enum sh_dmae_slave_chan_id slave_id)
> >> +     struct sh_dmae_chan *sh_chan, struct sh_dmae_slave *param)
> >
> > This doesn't seem to be needed, at least, not in this patch.
> 
> Well, I have to change the function to something, no? =)

Yes, orry, I mean make it

	struct sh_dmae_chan *sh_chan, int slave_id)

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/