sys/arm/allwinner/a10_clk.c
274–275 ↗	(On Diff #12642)	What are these magic numbers? A comment explaining them would be useful.
279 ↗	(On Diff #12642)	More magic
sys/arm/allwinner/a10_codec.c
62 ↗	(On Diff #12642)	#define<tab>FOO
177–178 ↗	(On Diff #12642)	Why do you need these? You can use `bus_read_` and `bus_write_` with the res.
sys/arm/allwinner/a10_dmac.c
82–83	Are these needed when you could use `bus_read_` and `bus_write_`

br added a subscriber: br.Jan 24 2016, 4:42 PM

br added inline comments.

sys/arm/allwinner/a10_codec.c
260 ↗	(On Diff #12642)	is volume based on left channel only?
267–268 ↗	(On Diff #12642)	should not mixer_set just return (0) on success?
sys/arm/allwinner/a10_dmac.c
204	magic numbers ?

In D5050#107261, @andrew wrote:

Why are you adding both drivers in one review? I would expect the audio driver could depend on DMA, but not the reverse.

I can split the changes into separate reviews, not a problem.

sys/arm/allwinner/a10_clk.c
274–275 ↗	(On Diff #12642)	Sure. PLL2 output is ref * n / prediv / postdiv. With ref clock of 24MHz (always the case), 24MHZ * n / 21 / 4 gives us an output close to the desired frequency. For 24.576MHz target, n=86 gives us 24.571MHz and for 22.5792MHz target, n=79 gives us 22.571MHz. I'll add some comments to explain better.
sys/arm/allwinner/a10_codec.c
260 ↗	(On Diff #12642)	Correct, the hardware doesn't offer per-channel volume controls.
267–268 ↗	(On Diff #12642)	I couldn't find documentation on this interface but existing drivers in dev/sound do this as well.
sys/arm/allwinner/a10_dmac.c
204	This converts a bit in a status register into an index in either the ndma or ddma channels array. The register is encoded as two bits per channel (lowest bit is half transfer pending, highest bit is end transfer pending). The 8 normal DMA channel status are in bits 15-0 and the 8 dedicated DMA channel status are in bits 31-16. I'll add a comment to clarify what's going on here.

br added inline comments.Jan 24 2016, 5:43 PM

sys/arm/allwinner/a10_codec.c
267–268 ↗	(On Diff #12642)	you can grep MIXER_SET in kernel and see how it is used

jmcneill_invisible.ca mentioned this in D5052: Allwinner A10/A20 clock controller support for DMA and audio codec.Jan 24 2016, 6:38 PM

Reducing scope of this diff to the DMA controller driver only. Changes based on previous feedback.

jmcneill_invisible.ca added a parent revision: D5052: Allwinner A10/A20 clock controller support for DMA and audio codec.Jan 24 2016, 6:50 PM

andrew added inline comments.Jan 24 2016, 6:58 PM

sys/arm/allwinner/a10_dmac.c
123–138	You can use `bus_alloc_resources` to make this atomic & simplify the failure case.
sys/boot/fdt/dts/arm/sun7i-a20.dtsi
160	We need to also take into account the dts changes in D4792

Use bus_alloc_resources

jmcneill_invisible.ca updated this object.Jan 24 2016, 10:45 PM

andrew added inline comments.Jan 24 2016, 11:15 PM

sys/arm/allwinner/a10_dmac.c
36	I don't think you need this, it only defines `BUS_DEBUG` which is only used in `subr_bus.c`.
48	What do you need from here?
77	Do you need this?
434–439	Where do these get called from?

Remove unused headers and sc_dev softc member.

alvisen_gmail.com added a subscriber: alvisen_gmail.com.Jan 25 2016, 8:24 AM

jmcneill_invisible.ca added a child revision: D5202: Allwinner A10/A20 audio codec driver.Feb 5 2016, 1:16 PM

jmcneill_invisible.ca marked 5 inline comments as done.Feb 9 2016, 11:33 PM

jmcneill_invisible.ca added inline comments.

sys/arm/allwinner/a10_dmac.c
434–439	Called from the audio driver, and possibly any other device driver that needs to use the DMA controller.
sys/boot/fdt/dts/arm/sun7i-a20.dtsi
160	The compatible string "allwinner,sun4i-a10-dma" matches the one used in the vendor DTS.

manu_bidouilliste.com added a subscriber: manu_bidouilliste.com.Feb 12 2016, 5:45 PM

andrew added inline comments.Feb 13 2016, 1:26 PM

sys/arm/allwinner/a10_dmac.c
398–401	What are these magic numbers?

jmcneill_invisible.ca marked an inline comment as done.Feb 13 2016, 2:20 PM

jmcneill_invisible.ca added inline comments.

sys/arm/allwinner/a10_dmac.c
398–401	Oops, these should actually be set by a10dmac_set_config (this path is not exercised by the A10 codec driver as it uses an NDMA channel). Thanks, I'll fix that and update the patch.

Make DDMA-specific parameters configurable.

Just minor style issues.

sys/arm/allwinner/a10_dmac.c
27	We only need this once, remove this copy.
52–53	These should be `#include <arm/allwiner/...>` unless they are generated headers.

Closed by commit rS295635: Add support for the Allwinner DMA controller. This will be used by the at (authored by andrew). · Explain WhyFeb 15 2016, 7:56 PM

This revision was automatically updated to reflect the committed changes.

Allwinner A10/A20 DMA controller driver
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 12658

sys/arm/allwinner/a10_dmac.h

sys/arm/allwinner/a10_dmac.c

sys/arm/allwinner/files.allwinner

sys/arm/allwinner/sunxi_dma_if.m

sys/boot/fdt/dts/arm/sun7i-a20.dtsi

Allwinner A10/A20 DMA controller driverClosedPublicActions