* Re: [PATCH 01/05] dmaengine: shdma: Remove sh_dmae_slave_chan_id
2010-03-19 4:46 [PATCH 01/05] dmaengine: shdma: Remove sh_dmae_slave_chan_id enum Magnus Damm
@ 2010-03-19 7:30 ` Guennadi Liakhovetski
2010-03-19 8:02 ` [PATCH 01/05] dmaengine: shdma: Remove sh_dmae_slave_chan_id enum Magnus Damm
` (2 subsequent siblings)
3 siblings, 0 replies; 5+ messages in thread
From: Guennadi Liakhovetski @ 2010-03-19 7:30 UTC (permalink / raw)
To: linux-sh
On Fri, 19 Mar 2010, Magnus Damm wrote:
> From: Magnus Damm <damm@opensource.se>
>
> This patch replaces the sh_dmae_slave_chan_id enum
> with an unsigned int. The purpose of this chainge is
> to make it possible to separate the slave id enums
> from the dmaengine header.
>
> The slave id enums varies with processor model, so in
> the future it makes sense to put these in the processor
> specific headers together with the pinmux enums.
>
> Signed-off-by: Magnus Damm <damm@opensource.se>
> ---
>
> arch/sh/include/asm/dmaengine.h | 6 +++---
> arch/sh/include/asm/siu.h | 8 ++++----
> drivers/dma/shdma.c | 8 ++++----
> drivers/serial/sh-sci.c | 4 ++--
> include/linux/serial_sci.h | 4 ++--
> 5 files changed, 15 insertions(+), 15 deletions(-)
>
> --- 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.
[snip]
> --- 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.
Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH 01/05] dmaengine: shdma: Remove sh_dmae_slave_chan_id enum
2010-03-19 4:46 [PATCH 01/05] dmaengine: shdma: Remove sh_dmae_slave_chan_id enum Magnus Damm
2010-03-19 7:30 ` [PATCH 01/05] dmaengine: shdma: Remove sh_dmae_slave_chan_id Guennadi Liakhovetski
@ 2010-03-19 8:02 ` Magnus Damm
2010-03-19 8:16 ` [PATCH 01/05] dmaengine: shdma: Remove sh_dmae_slave_chan_id Guennadi Liakhovetski
2010-03-19 8:23 ` [PATCH 01/05] dmaengine: shdma: Remove sh_dmae_slave_chan_id enum Paul Mundt
3 siblings, 0 replies; 5+ messages in thread
From: Magnus Damm @ 2010-03-19 8:02 UTC (permalink / raw)
To: linux-sh
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.
>> --- 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? =)
Cheers,
/ magnus
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH 01/05] dmaengine: shdma: Remove sh_dmae_slave_chan_id
2010-03-19 4:46 [PATCH 01/05] dmaengine: shdma: Remove sh_dmae_slave_chan_id enum Magnus Damm
2010-03-19 7:30 ` [PATCH 01/05] dmaengine: shdma: Remove sh_dmae_slave_chan_id Guennadi Liakhovetski
2010-03-19 8:02 ` [PATCH 01/05] dmaengine: shdma: Remove sh_dmae_slave_chan_id enum Magnus Damm
@ 2010-03-19 8:16 ` Guennadi Liakhovetski
2010-03-19 8:23 ` [PATCH 01/05] dmaengine: shdma: Remove sh_dmae_slave_chan_id enum Paul Mundt
3 siblings, 0 replies; 5+ messages in thread
From: Guennadi Liakhovetski @ 2010-03-19 8:16 UTC (permalink / raw)
To: linux-sh
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/
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH 01/05] dmaengine: shdma: Remove sh_dmae_slave_chan_id enum
2010-03-19 4:46 [PATCH 01/05] dmaengine: shdma: Remove sh_dmae_slave_chan_id enum Magnus Damm
` (2 preceding siblings ...)
2010-03-19 8:16 ` [PATCH 01/05] dmaengine: shdma: Remove sh_dmae_slave_chan_id Guennadi Liakhovetski
@ 2010-03-19 8:23 ` Paul Mundt
3 siblings, 0 replies; 5+ messages in thread
From: Paul Mundt @ 2010-03-19 8:23 UTC (permalink / raw)
To: linux-sh
On Fri, Mar 19, 2010 at 09:16:46AM +0100, Guennadi Liakhovetski wrote:
> On Fri, 19 Mar 2010, Magnus Damm wrote:
> > 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:)
>
I don't have any strong opinions on this one way or the other. If 0 =
disabled then this is the default behaviour people will end up with when
just sticking with default initialization, explicit slave ID setting will
trump this regardless.
I do generally prefer a -1 for explicitly disabling something since it
means someone has thought about it and wants that behaviour as opposed to
simply defaulting to it after clearing memory. However, there are also
many places in the kernel where 0 = invalid assumptions are hardcoded,
as with the IRQ case.
The difference between INT_MAX vs UINT_MAX is a pretty academic one in
terms of suitability for slave IDs, no CPU is going to approach either
one of those limits regardless.
^ permalink raw reply [flat|nested] 5+ messages in thread