| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20100824065843.GY27749@pengutronix.de>
Date: Tue, 24 Aug 2010 08:58:43 +0200
From: Sascha Hauer <s.hauer@...gutronix.de>
To: Linus Walleij <linus.ml.walleij@...il.com>
Cc: linux-kernel@...r.kernel.org,
Dan Williams <dan.j.williams@...el.com>,
linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH 3/3 v2] dmaengine: Add Freescale i.MX SDMA support
On Mon, Aug 23, 2010 at 07:30:34PM +0200, Linus Walleij wrote:
> 2010/8/23 Sascha Hauer <s.hauer@...gutronix.de>:
>
> > This patch adds support for the Freescale i.MX SDMA engine.
>
> Great progress!
>
> > (...)
> > diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
> > (...)
> > +/* SDMA registers */
> > +#define SDMA_H_C0PTR (sdma->regs + 0x000)
> > +#define SDMA_H_INTR (sdma->regs + 0x004)
> > +#define SDMA_H_STATSTOP (sdma->regs + 0x008)
> > +#define SDMA_H_START (sdma->regs + 0x00c)
> > +#define SDMA_H_EVTOVR (sdma->regs + 0x010)
> > +#define SDMA_H_DSPOVR (sdma->regs + 0x014)
> > +#define SDMA_H_HOSTOVR (sdma->regs + 0x018)
> > +#define SDMA_H_EVTPEND (sdma->regs + 0x01c)
> > +#define SDMA_H_DSPENBL (sdma->regs + 0x020)
> > +#define SDMA_H_RESET (sdma->regs + 0x024)
> > +#define SDMA_H_EVTERR (sdma->regs + 0x028)
> > +#define SDMA_H_INTRMSK (sdma->regs + 0x02c)
> > +#define SDMA_H_PSW (sdma->regs + 0x030)
> > +#define SDMA_H_EVTERRDBG (sdma->regs + 0x034)
> > +#define SDMA_H_CONFIG (sdma->regs + 0x038)
> > +#define SDMA_ONCE_ENB (sdma->regs + 0x040)
> > +#define SDMA_ONCE_DATA (sdma->regs + 0x044)
> > +#define SDMA_ONCE_INSTR (sdma->regs + 0x048)
> > +#define SDMA_ONCE_STAT (sdma->regs + 0x04c)
> > +#define SDMA_ONCE_CMD (sdma->regs + 0x050)
> > +#define SDMA_EVT_MIRROR (sdma->regs + 0x054)
> > +#define SDMA_ILLINSTADDR (sdma->regs + 0x058)
> > +#define SDMA_CHN0ADDR (sdma->regs + 0x05c)
> > +#define SDMA_ONCE_RTB (sdma->regs + 0x060)
> > +#define SDMA_XTRIG_CONF1 (sdma->regs + 0x070)
> > +#define SDMA_XTRIG_CONF2 (sdma->regs + 0x074)
> > +#define SDMA_CHNENBL_0 (sdma->regs + (sdma->version == 2 ? 0x200 : 0x80))
> > +#define SDMA_CHNPRI_0 (sdma->regs + 0x100)
>
> These macros expand to the local variable "sdma" which must
> be present in all functions using them. I don't know what is
> considered moste readable, but I would certainly just
>
> #define SDMA_FOO (0x0123)
> (...)
> u32 foo = readl(sdma->regs + SDMA_FOO);
>
> That is more common I think.
> > +
> > +/*
> > + * This enumerates transfer types
> > + */
> > +enum {
> > + emi_2_per = 0, /* EMI memory to peripheral */
> > + emi_2_int, /* EMI memory to internal RAM */
> > + emi_2_emi, /* EMI memory to EMI memory */
> > + emi_2_dsp, /* EMI memory to DSP memory */
> > + per_2_int, /* Peripheral to internal RAM */
> > + per_2_emi, /* Peripheral to internal EMI memory */
> > + per_2_dsp, /* Peripheral to DSP memory */
> > + per_2_per, /* Peripheral to Peripheral */
> > + int_2_per, /* Internal RAM to peripheral */
> > + int_2_int, /* Internal RAM to Internal RAM */
> > + int_2_emi, /* Internal RAM to EMI memory */
> > + int_2_dsp, /* Internal RAM to DSP memory */
> > + dsp_2_per, /* DSP memory to peripheral */
> > + dsp_2_int, /* DSP memory to internal RAM */
> > + dsp_2_emi, /* DSP memory to EMI memory */
> > + dsp_2_dsp, /* DSP memory to DSP memory */
> > + emi_2_dsp_loop, /* EMI memory to DSP memory loopback */
> > + dsp_2_emi_loop, /* DSP memory to EMI memory loopback */
> > + dvfs_pll, /* DVFS script with PLL change */
> > + dvfs_pdr /* DVFS script without PLL change */
> > +} sdma_transfer_type;
>
> Picky me, but it's no type, its an enum. I understand that it is
> a technical term...
>
> What about just calling is sdma_transfer? Short and nice.
> Or sdma_transfer_line?
This turned out to be unused anyway, so the simple fix was to remove it.
>
> > (...)
> > +/*
> > + * Stores the start address of the SDMA scripts
> > + */
> > +static struct sdma_script_start_addrs __sdma_script_addrs;
> > +static struct sdma_script_start_addrs *sdma_script_addrs = &__sdma_script_addrs;
>
> What's the rationale behind prefixing that variable with __?
>
> The same name for struct and variable is perfectly viable.
The rationale was to statically allocate a struct
sdma_script_start_addrs and create a pointer to it so that I don't have
to use &__sdma_script_addrs in the code.
I forgot this one while converting the driver to multi instance, so this
is now part of struct sdma_engine.
Fixed the other stuff aswell, I will send an update shortly.
Regards,
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists