Re: [PATCH] shdma: add R-Car Audio DMAC peri peri driver
Hi Geert > > This driver is called from shdma-base.c. > > And shdmac.c/sudmac.c/rcar-hpbdmac.c are same style. > > > > 1) this "slave_id" came from shdma_ops::set_slave > > and, it is using "int" > > 2) above drivers have same xxx_find_slave(), > > they are using "int". > > (this driver is based on that) > > > > What should I do ? > > OK, then you should keep using int, and change the test to: > > if (slave_id < 0 || slave_id >= AUDMAPP_SLAVE_NUMBER) I see Thank you Best regards --- Kuninori Morimoto -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] shdma: add R-Car Audio DMAC peri peri driver
Hi Morimoto-san,
On Fri, Jan 24, 2014 at 9:18 AM, Kuninori Morimoto
wrote:
>> > + if (slave_id >= AUDMAPP_SLAVE_NUMBER)
>>
>> So slave_id should be unsigned int, too, and AUDMAPP_SLAVE_NUMBER
>> too ("29U").
>
> Hmm...
> This driver is called from shdma-base.c.
> And shdmac.c/sudmac.c/rcar-hpbdmac.c are same style.
>
> 1) this "slave_id" came from shdma_ops::set_slave
> and, it is using "int"
> 2) above drivers have same xxx_find_slave(),
> they are using "int".
> (this driver is based on that)
>
> What should I do ?
OK, then you should keep using int, and change the test to:
if (slave_id < 0 || slave_id >= AUDMAPP_SLAVE_NUMBER)
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] shdma: add R-Car Audio DMAC peri peri driver
Hi Geert
Thank you for your review
> > --- a/drivers/dma/sh/Kconfig
> > +++ b/drivers/dma/sh/Kconfig
> > @@ -29,6 +29,12 @@ config RCAR_HPB_DMAE
> > help
> > Enable support for the Renesas R-Car series DMA controllers.
> >
> > +config RCAR_AUDMAC_PP
> > + tristate "Renesas R-Car Audio DMAC Peripheral Peripheral support"
>
> double "Peripheral"
Unfortunately (?) "Audio DMAC Peripheral Peripheral"
(= double Peripheral) is the formal name
> > + for(i = 0; i < 1024; i++) {
>
> Missing space between "for" and "(" (have you run checkpatch.pl?)
>
> What's a typical value of i when leaving the loop?
Oops, this is my fault.
I will fix in v2
> > +static struct audmapp_slave_config *
> > +audmapp_find_slave(struct audmapp_chan *auchan, int slave_id)
> > +{
> > + struct audmapp_device *audev = to_dev(auchan);
> > + struct audmapp_pdata *pdata = audev->pdata;
> > + struct audmapp_slave_config *cfg;
> > + int i;
>
> unsigned int
>
> > +
> > + if (slave_id >= AUDMAPP_SLAVE_NUMBER)
>
> So slave_id should be unsigned int, too, and AUDMAPP_SLAVE_NUMBER
> too ("29U").
Hmm...
This driver is called from shdma-base.c.
And shdmac.c/sudmac.c/rcar-hpbdmac.c are same style.
1) this "slave_id" came from shdma_ops::set_slave
and, it is using "int"
2) above drivers have same xxx_find_slave(),
they are using "int".
(this driver is based on that)
What should I do ?
> > + if (*len > (size_t)AUDMAPP_LEN_MAX)
>
> I think you can get rid of the cast by adding a "U" suffix to one of
> the constants in the definition of AUDMAPP_LEN_MAX.
>
> > + *len = (size_t)AUDMAPP_LEN_MAX;
>
> The cast is not needed (I think even without the change above).
I will fixup these in v2
Thank you
Best regards
---
Kuninori Morimoto
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] shdma: add R-Car Audio DMAC peri peri driver
Hi Geert
Thank you for your review
--- a/drivers/dma/sh/Kconfig
+++ b/drivers/dma/sh/Kconfig
@@ -29,6 +29,12 @@ config RCAR_HPB_DMAE
help
Enable support for the Renesas R-Car series DMA controllers.
+config RCAR_AUDMAC_PP
+ tristate Renesas R-Car Audio DMAC Peripheral Peripheral support
double Peripheral
Unfortunately (?) Audio DMAC Peripheral Peripheral
(= double Peripheral) is the formal name
+ for(i = 0; i 1024; i++) {
Missing space between for and ( (have you run checkpatch.pl?)
What's a typical value of i when leaving the loop?
Oops, this is my fault.
I will fix in v2
+static struct audmapp_slave_config *
+audmapp_find_slave(struct audmapp_chan *auchan, int slave_id)
+{
+ struct audmapp_device *audev = to_dev(auchan);
+ struct audmapp_pdata *pdata = audev-pdata;
+ struct audmapp_slave_config *cfg;
+ int i;
unsigned int
+
+ if (slave_id = AUDMAPP_SLAVE_NUMBER)
So slave_id should be unsigned int, too, and AUDMAPP_SLAVE_NUMBER
too (29U).
Hmm...
This driver is called from shdma-base.c.
And shdmac.c/sudmac.c/rcar-hpbdmac.c are same style.
1) this slave_id came from shdma_ops::set_slave
and, it is using int
2) above drivers have same xxx_find_slave(),
they are using int.
(this driver is based on that)
What should I do ?
+ if (*len (size_t)AUDMAPP_LEN_MAX)
I think you can get rid of the cast by adding a U suffix to one of
the constants in the definition of AUDMAPP_LEN_MAX.
+ *len = (size_t)AUDMAPP_LEN_MAX;
The cast is not needed (I think even without the change above).
I will fixup these in v2
Thank you
Best regards
---
Kuninori Morimoto
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] shdma: add R-Car Audio DMAC peri peri driver
Hi Morimoto-san,
On Fri, Jan 24, 2014 at 9:18 AM, Kuninori Morimoto
kuninori.morimoto...@gmail.com wrote:
+ if (slave_id = AUDMAPP_SLAVE_NUMBER)
So slave_id should be unsigned int, too, and AUDMAPP_SLAVE_NUMBER
too (29U).
Hmm...
This driver is called from shdma-base.c.
And shdmac.c/sudmac.c/rcar-hpbdmac.c are same style.
1) this slave_id came from shdma_ops::set_slave
and, it is using int
2) above drivers have same xxx_find_slave(),
they are using int.
(this driver is based on that)
What should I do ?
OK, then you should keep using int, and change the test to:
if (slave_id 0 || slave_id = AUDMAPP_SLAVE_NUMBER)
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say programmer or something like that.
-- Linus Torvalds
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] shdma: add R-Car Audio DMAC peri peri driver
Hi Geert This driver is called from shdma-base.c. And shdmac.c/sudmac.c/rcar-hpbdmac.c are same style. 1) this slave_id came from shdma_ops::set_slave and, it is using int 2) above drivers have same xxx_find_slave(), they are using int. (this driver is based on that) What should I do ? OK, then you should keep using int, and change the test to: if (slave_id 0 || slave_id = AUDMAPP_SLAVE_NUMBER) I see Thank you Best regards --- Kuninori Morimoto -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] shdma: add R-Car Audio DMAC peri peri driver
Hi Morimoto-san,
On Fri, Jan 24, 2014 at 3:32 AM, Kuninori Morimoto
wrote:
> --- a/drivers/dma/sh/Kconfig
> +++ b/drivers/dma/sh/Kconfig
> @@ -29,6 +29,12 @@ config RCAR_HPB_DMAE
> help
> Enable support for the Renesas R-Car series DMA controllers.
>
> +config RCAR_AUDMAC_PP
> + tristate "Renesas R-Car Audio DMAC Peripheral Peripheral support"
double "Peripheral"
> + depends on SH_DMAE_BASE
> + help
> + Enable support for the Renesas R-Car Audio DMAC Peripheral
> Peripheral controllers.
idem.
> --- /dev/null
> +++ b/drivers/dma/sh/rcar-audmapp.c
> @@ -0,0 +1,325 @@
> +static void audmapp_halt(struct shdma_chan *schan)
> +{
> + struct audmapp_chan *auchan = to_chan(schan);
> + int i;
unsigned int
> +
> + audmapp_write(auchan, 0, PDMACHCR);
> +
> + for(i = 0; i < 1024; i++) {
Missing space between "for" and "(" (have you run checkpatch.pl?)
What's a typical value of i when leaving the loop?
> + if (0 == audmapp_read(auchan, PDMACHCR))
> + return;
> + udelay(1);
> + }
> +}
> +
> +
> +static struct audmapp_slave_config *
> +audmapp_find_slave(struct audmapp_chan *auchan, int slave_id)
> +{
> + struct audmapp_device *audev = to_dev(auchan);
> + struct audmapp_pdata *pdata = audev->pdata;
> + struct audmapp_slave_config *cfg;
> + int i;
unsigned int
> +
> + if (slave_id >= AUDMAPP_SLAVE_NUMBER)
So slave_id should be unsigned int, too, and AUDMAPP_SLAVE_NUMBER
too ("29U").
> + return NULL;
> +
> + for (i = 0, cfg = pdata->slave; i < pdata->slave_num; i++, cfg++)
> + if (cfg->slave_id == slave_id)
> + return cfg;
> +
> + return NULL;
> +}
> +static int audmapp_desc_setup(struct shdma_chan *schan,
> + struct shdma_desc *sdecs,
> + dma_addr_t src, dma_addr_t dst, size_t *len)
> +{
> + struct audmapp_chan *auchan = to_chan(schan);
> + struct audmapp_slave_config *cfg = auchan->config;
> +
> + if (!cfg)
> + return -ENODEV;
> +
> + if (*len > (size_t)AUDMAPP_LEN_MAX)
I think you can get rid of the cast by adding a "U" suffix to one of
the constants in the definition of AUDMAPP_LEN_MAX.
> + *len = (size_t)AUDMAPP_LEN_MAX;
The cast is not needed (I think even without the change above).
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] shdma: add R-Car Audio DMAC peri peri driver
Hi Morimoto-san,
On Fri, Jan 24, 2014 at 3:32 AM, Kuninori Morimoto
kuninori.morimoto...@gmail.com wrote:
--- a/drivers/dma/sh/Kconfig
+++ b/drivers/dma/sh/Kconfig
@@ -29,6 +29,12 @@ config RCAR_HPB_DMAE
help
Enable support for the Renesas R-Car series DMA controllers.
+config RCAR_AUDMAC_PP
+ tristate Renesas R-Car Audio DMAC Peripheral Peripheral support
double Peripheral
+ depends on SH_DMAE_BASE
+ help
+ Enable support for the Renesas R-Car Audio DMAC Peripheral
Peripheral controllers.
idem.
--- /dev/null
+++ b/drivers/dma/sh/rcar-audmapp.c
@@ -0,0 +1,325 @@
+static void audmapp_halt(struct shdma_chan *schan)
+{
+ struct audmapp_chan *auchan = to_chan(schan);
+ int i;
unsigned int
+
+ audmapp_write(auchan, 0, PDMACHCR);
+
+ for(i = 0; i 1024; i++) {
Missing space between for and ( (have you run checkpatch.pl?)
What's a typical value of i when leaving the loop?
+ if (0 == audmapp_read(auchan, PDMACHCR))
+ return;
+ udelay(1);
+ }
+}
+
+
+static struct audmapp_slave_config *
+audmapp_find_slave(struct audmapp_chan *auchan, int slave_id)
+{
+ struct audmapp_device *audev = to_dev(auchan);
+ struct audmapp_pdata *pdata = audev-pdata;
+ struct audmapp_slave_config *cfg;
+ int i;
unsigned int
+
+ if (slave_id = AUDMAPP_SLAVE_NUMBER)
So slave_id should be unsigned int, too, and AUDMAPP_SLAVE_NUMBER
too (29U).
+ return NULL;
+
+ for (i = 0, cfg = pdata-slave; i pdata-slave_num; i++, cfg++)
+ if (cfg-slave_id == slave_id)
+ return cfg;
+
+ return NULL;
+}
+static int audmapp_desc_setup(struct shdma_chan *schan,
+ struct shdma_desc *sdecs,
+ dma_addr_t src, dma_addr_t dst, size_t *len)
+{
+ struct audmapp_chan *auchan = to_chan(schan);
+ struct audmapp_slave_config *cfg = auchan-config;
+
+ if (!cfg)
+ return -ENODEV;
+
+ if (*len (size_t)AUDMAPP_LEN_MAX)
I think you can get rid of the cast by adding a U suffix to one of
the constants in the definition of AUDMAPP_LEN_MAX.
+ *len = (size_t)AUDMAPP_LEN_MAX;
The cast is not needed (I think even without the change above).
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say programmer or something like that.
-- Linus Torvalds
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
