Dear Tor Krill,
In message 1277387833-6925-1-git-send-email-tor@excito.com you wrote:
A rudimentary sata driver for the Marvell Kirkwood CPU based on the architecture of the fsl_sata driver.
Signed-off-by: Tor Krill tor@excito.com
drivers/block/Makefile | 1 + drivers/block/sata_mv.c | 800 +++++++++++++++++++++++++++++++++++++++++++++++ drivers/block/sata_mv.h | 229 ++++++++++++++ 3 files changed, 1030 insertions(+), 0 deletions(-) create mode 100644 drivers/block/sata_mv.c create mode 100644 drivers/block/sata_mv.h
...
+#ifdef MALLOC_QUEUE
- void *crqb_alloc;
- struct crqb *request;
- void *crpb_alloc;
- struct crpb *response;
+#else
- struct crqb request[REQUEST_QUEUE_SIZE] __attribute__ ((aligned(CRQB_ALIGN)));
- struct crpb response[REQUEST_QUEUE_SIZE] __attribute__ ((aligned(CRPB_ALIGN)));
+#endif
What about always using pointers for request and response, and initializing these statically (taking the address of a diffrently named static array) in one case and dynamically (from malloc() in the other case? Then we would need the #ifdef's only in one or two places, and avoid constructs like these:
+#ifdef MALLOC_QUEUE
- out_le32(priv->regbase+EDMA_RQIPR, priv->request);
+#else
- out_le32(priv->regbase+EDMA_RQIPR, &priv->request);
+#endif
- out_le32(priv->regbase+EDMA_RQOPR, 0x0);
- /* Setup response queue */
- out_le32(priv->regbase+EDMA_RSBA_HI, 0x0);
+#ifdef MALLOC_QUEUE
- out_le32(priv->regbase+EDMA_RSOPR, priv->response);
+#else
- out_le32(priv->regbase+EDMA_RSOPR, &priv->response);
+#endif
- out_le32(priv->regbase+EDMA_RSIPR, 0x0);
...
- debug("Probe port: %d\n\r",port);
- for ( tries=0; tries<2; tries++){
No space after the '('.
tmp = in_le32(priv->regbase + SIR_SCONTROL);tries2 = 5;do{
Space: "do {"
if ( !set15 ){
Make this:
if (!set15) {
- tmp = in_le32(priv->regbase+EDMA_RQIPR) & EDMA_RQIPR_IPMASK;
- tmp = tmp >> EDMA_RQIPR_IPSHIFT;
- return tmp;
combine the last 2 lines into
return tmp >> EDMA_RQIPR_IPSHIFT;
- tmp = in_le32(priv->regbase+EDMA_RQOPR) & EDMA_RQOPR_OPMASK;
- tmp = tmp >> EDMA_RQOPR_OPSHIFT;
- return tmp;
Ditto, and so on.
+/* Get response queue in pointer */ +static int get_rspip(int port){
- u32 tmp;
- struct mv_priv *priv = (struct mv_priv *) sata_dev_desc[port].priv;
- tmp = in_le32(priv->regbase+EDMA_RSIPR) & EDMA_RSIPR_IPMASK;
- tmp = tmp >> EDMA_RSIPR_IPSHIFT;
- return tmp;
+}
+/* Get response queue out pointer */ +static int get_rspop(int port){
- u32 tmp;
- struct mv_priv *priv = (struct mv_priv *) sata_dev_desc[port].priv;
- if ( (res = ata_wait_register(
(u32*)(KW_SATAHC_BASE+SATAHC_ICR), tmp ,tmp, timeout_msec))){
Line too long, spaces after ',' (not before), space before '{'.
printf("Failed to wait for completion on port %d\n",port);
Space after comma.
- if ( port == 0 ){
tmp &= ~( 1<<0 | 1<<8);- }else{
tmp &= ~(1<<1 | 1<<9);- }
No braces needed for one line statements. If you use braches, they are always separated by a space.
- while(get_rspip(port)!=outind){
debug("Response index %d flags %08x on port %d\n",outind,priv->response[outind].flags,port);
Line too long. Please fix globally.
+static int mv_ata_exec_ata_cmd(int port, struct sata_fis_h2d *cfis,
u8 *buffer, u32 len, u32 iswrite){
Brace goes on next line.
- priv->request[slot].ata_sect_count |=
((cfis->sector_count_exp<<CRQB_SECTCOUNT_COUNT_EXP_SHIFT) &CRQB_SECTCOUNT_COUNT_EXP_MASK);
Line too long.
- /* Wait for completion */
- if( wait_dma_completion(port,slot,10000) ){
if (wait_dma_completion(port,slot,10000)) {
Please fix brace style globally.
- debug("pio %04x, mwdma %04x, udma %04x\n\r"
,priv->pio, priv->mwdma, priv->udma);
Comma goes at end of first line.
- /* First check the device capablity */
- udma_cap = (u8)(priv->udma & 0xff);
- if (udma_cap == ATA_UDMA6)
cfis.sector_count = XFER_UDMA_6;- if (udma_cap == ATA_UDMA5)
cfis.sector_count = XFER_UDMA_5;- if (udma_cap == ATA_UDMA4)
cfis.sector_count = XFER_UDMA_4;- if (udma_cap == ATA_UDMA3)
cfis.sector_count = XFER_UDMA_3;
Use a switch() ?
Handle "default:" case?
- if ( dev < 0 || dev >= CONFIG_SYS_SATA_MAX_DEVICE) {
... ----^
- if ( !priv ){
... ----^-----^-^
- memset((void*) priv,0 , sizeof(struct mv_priv));
... --------------------^^
There are tons of such coding style issues. Please fix globally.
Best regards,
Wolfgang Denk