Dear Tor Krill,
In message <1277387833-6925-1-git-send-email-...@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 <t...@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
--
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de
HEALTH WARNING: Care Should Be Taken When Lifting This Product, Since
Its Mass, and Thus Its Weight, Is Dependent on Its Velocity Relative
to the User.
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
