Details
- Reviewers
rwatson - Group Reviewers
ARM - Commits
- rS332469: Add driver for ARM PrimeCell PL330 DMA engine.
Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Looks good, however I think a few comments would be useful, e.g. in pl330_channel_submit_sg, and it seems to be missing the build glue.
| sys/dev/xdma/controller/pl330.c | ||
|---|---|---|
| 91–92 ↗ | (On Diff #26824) | You could make these a single struct resource *, then use bus_read_4 and bus_write_4. |
| 130 ↗ | (On Diff #26824) | I think we might need to think about a new bus_alloc_resources like function to handle this sort of thing. |
| 181 ↗ | (On Diff #26824) | Why 0x10000? |
| 197–199 ↗ | (On Diff #26824) | I wonder if there should be macros for these, e.g. #define MOV_3(val) (((val) >> 8) & 0xff) ... buf[3] = MOV_3(val) |
| 496 ↗ | (On Diff #26824) | What will happen when len is not a multiple of 4? |
| 498 ↗ | (On Diff #26824) | What is the meaning of 0 here, and 1 below? |
| 512–514 ↗ | (On Diff #26824) | Could these be merged into a single call? |
| 548–550 ↗ | (On Diff #26824) | Is this needed? |
| 553 ↗ | (On Diff #26824) | Magic 0x10000 again |
| 636 ↗ | (On Diff #26824) | We should probably add a DMA bus pass. |
| sys/dev/xdma/controller/pl330.h | ||
| 32 ↗ | (On Diff #26824) | It might pay to add a guard against including this header twice. |
| 43 ↗ | (On Diff #26824) | You should put n in parentheses, i.e. #define FTR(n) (0x040 + 0x04 * (n)) |
| 76 ↗ | (On Diff #26824) | Why #if 0 and not /* ... */? |
| 108–111 ↗ | (On Diff #26824) | I would put these in the .c file, they don't seem to be needed anywhere else. |
| sys/dev/xdma/controller/pl330.c | ||
|---|---|---|
| 496 ↗ | (On Diff #26824) | ok I dehardcoded port width. |
| 498 ↗ | (On Diff #26824) | offs0 is the jump address for loop0, while offs1 is the jump addr for loop1 |
| 512–514 ↗ | (On Diff #26824) | Yes. I merged this to single call |
| 548–550 ↗ | (On Diff #26824) | No. Thanks |
| 636 ↗ | (On Diff #26824) | agree |