460EX on-chip SATA driver<resubmisison>
PATCH v2 1/1 460EX on-chip SATA driver<resubmisison>
This patch enables the on-chip DWC SATA controller of the AppliedMicro processor 460EX.
---
This patch incorporates the changes advised in the mailing list. The device tree changes were submitted as a seperate patch. Kconfig file is fixed in this version.
drivers/ata/Kconfig | 9 + drivers/ata/Makefile | 1 + drivers/ata/sata_dwc_460ex.c | 1756 ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 1766 insertions(+), 0 deletions(-) create mode 100644 drivers/ata/sata_dwc_460ex.c
drivers/ata/Kconfig +9 -0
@@ -187,6 +187,15 @@ config ATA_PIIX If unsure, say N. +config SATA_DWC + tristate "DesignWare Cores SATA support" + depends on 460EX + help + This option enables support for the on-chip SATA controller of the + AppliedMicro processor 460EX. + + If unsure, say N. + config SATA_MV tristate "Marvell SATA support" help
drivers/ata/Makefile +1 -0
@@ -7,6 +7,7 @@ obj-$(CONFIG_SATA_AHCI_PLATFORM) += ahci_platform.o libahci.o obj-$(CONFIG_SATA_FSL) += sata_fsl.o obj-$(CONFIG_SATA_INIC162X) += sata_inic162x.o obj-$(CONFIG_SATA_SIL24) += sata_sil24.o +obj-$(CONFIG_SATA_DWC) += sata_dwc_460ex.o # SFF w/ custom DMA obj-$(CONFIG_PDC_ADMA) += pdc_adma.o
drivers/ata/sata_dwc_460ex.c new file +1756 -0
1,756 lines not on this page · raw message
Signature · 1 line
1.5.6.3
Jeff Garzik
AppliedQuoted 10 lines · Rupjyoti Sarmah
This patch enables the on-chip DWC SATA controller of the AppliedMicro processor 460EX.
Signed-off-by Rupjyoti Sarmah<[email protected]>Signed-off-by Mark Miesfeld<[email protected]>Signed-off-by Prodyut Hazarika<[email protected]>---
This patch incorporates the changes advised in the mailing list. The device tree changes were submitted as a seperate patch.Kconfigfile is fixed in this version.
Quoted 5 lines
drivers/ata/Kconfig | 9 + drivers/ata/Makefile | 1 + drivers/ata/sata_dwc_460ex.c | 1756 ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 1766 insertions(+), 0 deletions(-)create mode 100644
drivers/ata/sata_dwc_460ex.c
applied
Hello.
Quoted 1 line · Rupjyoti Sarmah
This patch enables the on-chip DWC SATA controller of the AppliedMicro processor 460EX.
Too bad thius has already been applied but here's my (mostly stylistic)
comments anyway:
Quoted 2 lines
[...]
drivers/ata/sata_dwc_460ex.c line 2
Quoted 8 lines
drivers/ata/sata_dwc_460ex.c new file +2
-0@@ -0,0 +1,1756 @@ +/* +* drivers/ata/sata_dwc_460ex.c
Filenames in the heading comments have long been frowned upon.
drivers/ata/sata_dwc_460ex.c line 21
Quoted 1 line
drivers/ata/sata_dwc_460ex.c new file +1
-0+#ifdef CONFIG_SATA_DWC_DEBUG
I don't see this option defined anywahere.
drivers/ata/sata_dwc_460ex.c line 25
Quoted 4 lines
drivers/ata/sata_dwc_460ex.c new file +4
-0+#define DEBUG +#endif + +#ifdef CONFIG_SATA_DWC_VDEBUG
The same about this one.
Quoted 3 lines
drivers/ata/sata_dwc_460ex.c new file +3
-0+#define VERBOSE_DEBUG +#define DEBUG_NCQ +#endif
[...]
drivers/ata/sata_dwc_460ex.c line 51
Quoted 6 lines
drivers/ata/sata_dwc_460ex.c new file +6
-0+/* SATA DMA driver Globals */ +#define DMA_NUM_CHANS 1 +#define DMA_NUM_CHAN_REGS 8 + +/* SATA DMA Register definitions */ +#define AHB_DMA_BRST_DFLT 64/* 16 data items burst length*/
Please put a space before */.
drivers/ata/sata_dwc_460ex.c line 88
Quoted 7 lines
drivers/ata/sata_dwc_460ex.c new file +7
-0+struct ahb_dma_regs { +struct dma_chan_regs chan_regs[DMA_NUM_CHAN_REGS ]; +struct dma_interrupt_regs interrupt_raw;/* Raw Interrupt */ +struct dma_interrupt_regs interrupt_status;/* Interrupt Status */ +struct dma_interrupt_regs interrupt_mask;/* Interrupt Mask */ +struct dma_interrupt_regs interrupt_clear;/* Interrupt Clear */ +struct dmareg statusInt;/* Interrupt combined*/
No camelCase please, rename it to status_int.
drivers/ata/sata_dwc_460ex.c line 141
Quoted 7 lines
drivers/ata/sata_dwc_460ex.c new file +7
-0+#define DMA_CTL_BLK_TS (size) ((size) & 0x000000FFF)/* Blk Transfer size */ +#define DMA_CHANNEL (ch) (0x00000001 << (ch))/* Select channel */ +/* Enable channel */ +#define DMA_ENABLE_CHAN (ch) ((0x00000001 << (ch)) | \ + ((0x000000001 << (ch)) << 8)) +/* Disable channel */ +#define DMA_DISABLE_CHAN (ch) (0x00000000 | ((0x000000001 << (ch)) << 8))
What's the point of OR'ing with zero?
drivers/ata/sata_dwc_460ex.c line 298
Quoted 13 lines
drivers/ata/sata_dwc_460ex.c new file +13
-0+/* +* Commonly used DWC SATA driver Macros +*/ +#define HSDEV_FROM_HOST (host) ((struct sata_dwc_device *)\ + (host)->private_data) +#define HSDEV_FROM_AP (ap) ((struct sata_dwc_device *)\ + (ap)->host->private_data) +#define HSDEVP_FROM_AP (ap) ((struct sata_dwc_device_port *)\ + (ap)->private_data) +#define HSDEV_FROM_QC (qc) ((struct sata_dwc_device *)\ + (qc)->ap->host->private_data) +#define HSDEV_FROM_HSDEVP (p) ((struct sata_dwc_device *)\ + (hsdevp)->hsdev)
Are you sure it's '(hsdevp)', not '(p)'?
drivers/ata/sata_dwc_460ex.c line 314
Quoted 4 lines
drivers/ata/sata_dwc_460ex.c new file +4
-0+struct sata_dwc_host_priv { +void __iomem *scr_addr_sstatus; +u32 sata_dwc_sactive_issued ; +u32 sata_dwc_sactive_queued ;
Remove spaces befoer semicolons, please.
drivers/ata/sata_dwc_460ex.c line 339
Quoted 4 lines
drivers/ata/sata_dwc_460ex.c new file +4
-0+static void sata_dwc_tf_dump (struct ata_taskfile *tf) +{ +dev_vdbg (host_pvt.dwc_dev,"taskfile cmd: 0x%02x protocol: %s flags:" + "0x%lx device: %x\n", tf->command, ata_get_cmd_descript\
There's no need to use \ outside macro defintions.
Quoted 17 lines
drivers/ata/sata_dwc_460ex.c new file +17
-0+/* +* Function: get_burst_length_encode +* arguments: datalength: length in bytes of data +* returns value to be programmed in register corrresponding to data length +* This value is effectively the log(base 2) of the length +*/ +static int get_burst_length_encode (int datalength) +{ +int items = datalength >>2 ;/* div by 4 to get lword count */ + +if (items >=64 ) +return 5 ; + +if (items >=32 ) +return 4 ; + +if (items >=16 )
drivers/ata/sata_dwc_460ex.c line 376
Quoted 10 lines
drivers/ata/sata_dwc_460ex.c new file +10
-0+return 3 ; + +if (items >=8 ) +return 2 ; + +if (items >=4 ) +return 1 ; + +return 0 ; +}
Hmm, there should be a function in the kernel to calculate 2^n order from
size, something like get_count_order()...
Quoted 15 lines
drivers/ata/sata_dwc_460ex.c new file +15
-0+/* +* Function: dma_dwc_interrupt +* arguments: irq, dev_id, pt_regs +* returns channel number if available else -1 +* Interrupt Handler for DW AHB SATA DMA +*/ +static irqreturn_t dma_dwc_interrupt (int irq,void *hsdev_instance) +{ +int chan; +u32 tfr_reg, err_reg; +unsigned long flags; +struct sata_dwc_device *hsdev = + (struct sata_dwc_device *)hsdev_instance; +struct ata_host *host = (struct ata_host *)hsdev->host; +struct ata_port *ap;
drivers/ata/sata_dwc_460ex.c line 437
Quoted 10 lines
drivers/ata/sata_dwc_460ex.c new file +10
-0+struct sata_dwc_device_port *hsdevp; +u8 tag =0 ; +unsigned int port =0 ; + +spin_lock_irqsave (&host->lock, flags); + ap = host->ports[port]; + hsdevp =HSDEVP_FROM_AP (ap); + tag = ap->link.active_tag; + + tfr_reg = in_le32(&(host_pvt.sata_dma_regs->interrupt_status.tfr\
There's no need to use \ outside macro defintions.
drivers/ata/sata_dwc_460ex.c line 439
Quoted 2 lines
drivers/ata/sata_dwc_460ex.c new file +2
-0+ .low)); + err_reg = in_le32(&(host_pvt.sata_dma_regs->interrupt_status.error\
Same comment here...
Quoted 10 lines
drivers/ata/sata_dwc_460ex.c new file +10
-0+for (chan =0 ; chan <DMA_NUM_CHANS ; chan++) { +/* Check for end-of-transfer interrupt. */ +if (tfr_reg &DMA_CHANNEL (chan)) { +/* +* Each DMA command produces 2 interrupts. Only +* complete the command after both interrupts have been +* seen. (See sata_dwc_isr()) +*/ + host_pvt.dma_interrupt_count++; +sata_dwc_clear_dmacr (hsdevp, tag);
drivers/ata/sata_dwc_460ex.c line 464
Quoted 10 lines
drivers/ata/sata_dwc_460ex.c new file +10
-0+ +if (hsdevp->dma_pending[tag] == +SATA_DWC_DMA_PENDING_NONE ) { +dev_err (ap->dev,"DMA not pending eot=0x%08x " +"err=0x%08x tag=0x%02x pending=%d\n" , + tfr_reg, err_reg, tag, + hsdevp->dma_pending[tag]); + } + +if ((host_pvt.dma_interrupt_count %2 ) ==0 )
Prens around % operation not necessary.
drivers/ata/sata_dwc_460ex.c line 468
Quoted 4 lines
drivers/ata/sata_dwc_460ex.c new file +4
-0+sata_dwc_dma_xfer_complete (ap,1 ); + +/* Clear the interrupt */ + out_le32(&(host_pvt.sata_dma_regs->interrupt_clear\
\ not needed.
drivers/ata/sata_dwc_460ex.c line 480
Quoted 2 lines
drivers/ata/sata_dwc_460ex.c new file +2
-0+/* Clear the interrupt. */ + out_le32(&(host_pvt.sata_dma_regs->interrupt_clear\
\ not needed.
Quoted 13 lines
drivers/ata/sata_dwc_460ex.c new file +13
-0+/* +* Function: map_sg_to_lli +* The Synopsis driver has a comment proposing that better performance +* is possible by only enabling interrupts on the last item in the linked list. +* However, it seems that could be a problem if an error happened on one of the +* first items. The transfer would halt, but no error interrupt would occur. +* Currently this function sets interrupts enabled for each linked list item: +* DMA_CTL_INT_EN. +*/ +static int map_sg_to_lli (struct scatterlist *sg,int num_elems, +struct lli *lli,dma_addr_t dma_lli, +void __iomem *dmadr_addr,int dir) +{
[...]
drivers/ata/sata_dwc_460ex.c line 632
Quoted 2 lines
drivers/ata/sata_dwc_460ex.c new file +2
-0+/* Program the LLI CTL high register */ + lli[idx].ctl.high = cpu_to_le32(DMA_CTL_BLK_TS\
\ not needed.
drivers/ata/sata_dwc_460ex.c line 638
Quoted 6 lines
drivers/ata/sata_dwc_460ex.c new file +6
-0+ (len / 4)); + +/* Program the next pointer. The next pointer must be +* the physical address, not the virtual address. +*/ + next_llp = (dma_lli + ((idx + 1) * sizeof(struct \
Same here...
drivers/ata/sata_dwc_460ex.c line 688
Quoted 6 lines
drivers/ata/sata_dwc_460ex.c new file +6
-0+static int dma_dwc_xfer_setup (struct scatterlist *sg,int num_elems, +struct lli *lli,dma_addr_t dma_lli, +void __iomem *addr,int dir) +{ +int dma_ch; +int num_lli;
There should be an empty line here.
drivers/ata/sata_dwc_460ex.c line 747
Quoted 11 lines
drivers/ata/sata_dwc_460ex.c new file +11
-0+/* +* Function: dma_dwc_init +* arguments: hsdev +* returns status +* This function initializes the SATA DMA driver +*/ +static int dma_dwc_init (struct sata_dwc_device *hsdev,int irq) +{ +int err; + + err =dma_request_interrupts (hsdev, irq);
Could be initilaizer...
drivers/ata/sata_dwc_460ex.c line 758
Quoted 2 lines
drivers/ata/sata_dwc_460ex.c new file +2
-0+dev_notice (host_pvt.dwc_dev,"DMA initialized\n" ); + dev_dbg(host_pvt.dwc_dev, "SATA DMA registers=0x%p\n", host_pvt.\
\ not needed.
drivers/ata/sata_dwc_460ex.c line 800
Quoted 3 lines
drivers/ata/sata_dwc_460ex.c new file +3
-0+static u32 core_scr_read (unsigned int scr) +{ + return in_le32((void __iomem *)(host_pvt.scr_addr_sstatus) +\
Not needed.
drivers/ata/sata_dwc_460ex.c line 812
Quoted 3 lines
drivers/ata/sata_dwc_460ex.c new file +3
-0+static void clear_serror (void ) +{ +u32 val;
Empty line needed here.
drivers/ata/sata_dwc_460ex.c line 813
Quoted 1 line
drivers/ata/sata_dwc_460ex.c new file +1
-0+ val =core_scr_read (SCR_ERROR );
This could be an initilaizer.
Quoted 6 lines
drivers/ata/sata_dwc_460ex.c new file +6
-0+/* See ahci.c */ +static void sata_dwc_error_intr (struct ata_port *ap, +struct sata_dwc_device *hsdev,uint intpr) +{ +struct sata_dwc_device_port *hsdevp =HSDEVP_FROM_AP (ap); +struct ata_eh_info *ehi = &ap->link.eh_info;
drivers/ata/sata_dwc_460ex.c line 844
Quoted 10 lines
drivers/ata/sata_dwc_460ex.c new file +10
-0+unsigned int err_mask =0 , action =0 ; +struct ata_queued_cmd *qc; +u32 serror; +u8 status, tag; +u32 err_reg; + +ata_ehi_clear_desc (ehi); + + serror =core_scr_read (SCR_ERROR ); + status = ap->ops->sff_check_status (ap);
Why not call ata_sff_check_status() directly?
drivers/ata/sata_dwc_460ex.c line 846
Quoted 1 line
drivers/ata/sata_dwc_460ex.c new file +1
-0+ err_reg = in_le32(&(host_pvt.sata_dma_regs->interrupt_status.error.\
\ not neeeded.
Quoted 19 lines
drivers/ata/sata_dwc_460ex.c new file +19
-0+/* +* Function : sata_dwc_isr +* arguments : irq, void *dev_instance, struct pt_regs *regs +* Return value : irqreturn_t - status of IRQ +* This Interrupt handler called via port ops registered function. +* .irq_handler = sata_dwc_isr +*/ +static irqreturn_t sata_dwc_isr (int irq,void *dev_instance) +{ +struct ata_host *host = (struct ata_host *)dev_instance; +struct sata_dwc_device *hsdev =HSDEV_FROM_HOST (host); +struct ata_port *ap; +struct ata_queued_cmd *qc; +unsigned long flags; +u8 status, tag; +int handled, num_processed, port =0 ; +uint intpr, sactive, sactive2, tag_mask; +struct sata_dwc_device_port *hsdevp; + host_pvt.sata_dwc_sactive_issued =0 ;
[...]
drivers/ata/sata_dwc_460ex.c
Quoted 2 lines
drivers/ata/sata_dwc_460ex.c new file +2
-0+ sactive =core_scr_read (SCR_ACTIVE ); + tag_mask = (host_pvt.sata_dwc_sactive_issued | sactive) ^ sactive;
Isn't it the same as 'host_pvt.sata_dwc_sactive_issued & ~sactive'?
drivers/ata/sata_dwc_460ex.c line 953
Quoted 13 lines
drivers/ata/sata_dwc_460ex.c new file +13
-0+/* If no sactive issued and tag_mask is zero then this is not NCQ */ +if (host_pvt.sata_dwc_sactive_issued ==0 && tag_mask ==0 ) { +if (ap->link.active_tag ==ATA_TAG_POISON ) + tag =0 ; +else + tag = ap->link.active_tag; + qc =ata_qc_from_tag (ap, tag); + +/* DEV interrupt w/ no active qc? */ +if (unlikely (!qc || (qc->tf.flags &ATA_TFLAG_POLLING ))) { +dev_err (ap->dev,"%s interrupt with no active qc " +"qc=%p\n" , __func__, qc); + ap->ops->sff_check_status (ap);
Call ata_sff_check_status() directly...
drivers/ata/sata_dwc_460ex.c line 957
Quoted 4 lines
drivers/ata/sata_dwc_460ex.c new file +4
-0+ handled =1 ; + goto DONE; + } + status = ap->ops->sff_check_status (ap);
Here too...
Quoted 6 lines
drivers/ata/sata_dwc_460ex.c new file +6
-0+DRVSTILLBUSY: +if (ata_is_dma (qc->tf.protocol)) { +/* +* Each DMA transaction produces 2 interrupts. The DMAC +* transfer complete interrupt and the SATA controller +* operation done interrupt. The command should be
drivers/ata/sata_dwc_460ex.c line 986
Quoted 10 lines
drivers/ata/sata_dwc_460ex.c new file +10
-0+* completed only after both interrupts are seen. +*/ + host_pvt.dma_interrupt_count++; +if (hsdevp->dma_pending[tag] == \ +SATA_DWC_DMA_PENDING_NONE ) { +dev_err (ap->dev,"%s: DMA not pending " +"intpr=0x%08x status=0x%08x pending" +"=%d\n" , __func__, intpr, status, + hsdevp->dma_pending[tag]); + }
Curly brace not needed here. I assume you haven't run the patch thruscripts/checkpatch.pl?
drivers/ata/sata_dwc_460ex.c line 1012
Quoted 10 lines
drivers/ata/sata_dwc_460ex.c new file +10
-0+/* +* This is a NCQ command. At this point we need to figure out for which +* tags we have gotten a completion interrupt. One interrupt may serve +* as completion for more than one operation when commands are queued +* (NCQ). We need to process each completed command. +*/ + +/* process completed commands */ + sactive =core_scr_read (SCR_ACTIVE ); + tag_mask = (host_pvt.sata_dwc_sactive_issued | sactive) ^ sactive;
Isn't it the same as 'host_pvt.sata_dwc_sactive_issued & ~sactive'?
drivers/ata/sata_dwc_460ex.c line 1014
Quoted 1 line
drivers/ata/sata_dwc_460ex.c new file +1
-0+ if (sactive != 0 || (host_pvt.sata_dwc_sactive_issued) > 1 || \
Useless parens around 'host_pvt.sata_dwc_sactive_issued' + \ not needed.
drivers/ata/sata_dwc_460ex.c line 1019
Quoted 5 lines
drivers/ata/sata_dwc_460ex.c new file +5
-0+ tag_mask > 1) { +dev_dbg (ap->dev,"%s NCQ:sactive=0x%08x sactive_issued=0x%08x" +"tag_mask=0x%08x\n" , __func__, sactive, + host_pvt.sata_dwc_sactive_issued, tag_mask); + }
Curly braces not needed here.
drivers/ata/sata_dwc_460ex.c line 1022
Quoted 2 lines
drivers/ata/sata_dwc_460ex.c new file +2
-0+if ((tag_mask | (host_pvt.sata_dwc_sactive_issued)) != \ + (host_pvt.sata_dwc_sactive_issued)) {
Useless parens around 'host_pvt.sata_dwc_sactive_issued'.
drivers/ata/sata_dwc_460ex.c line 1027
Quoted 5 lines
drivers/ata/sata_dwc_460ex.c new file +5
-0+dev_warn (ap->dev,"Bad tag mask? sactive=0x%08x " +"(host_pvt.sata_dwc_sactive_issued)=0x%08x tag_mask" +"=0x%08x\n" , sactive, host_pvt.sata_dwc_sactive_issued, + tag_mask); + }
Curly braces not needed around single statement.
drivers/ata/sata_dwc_460ex.c line 1030
Quoted 3 lines
drivers/ata/sata_dwc_460ex.c new file +3
-0+ +/* read just to clear ... not bad if currently still busy */ + status = ap->ops->sff_check_status (ap);
Call ata_sff_check_status() directly.
drivers/ata/sata_dwc_460ex.c line 1042
Quoted 12 lines
drivers/ata/sata_dwc_460ex.c new file +12
-0+dev_dbg (ap->dev,"%s ATA status register=0x%x\n" , __func__, status); + + tag =0 ; + num_processed =0 ; +while (tag_mask) { + num_processed++; +while (!(tag_mask &0x00000001 )) { + tag++; + tag_mask <<=1 ; + } + + tag_mask &= (~0x00000001 );
Useless parens.
drivers/ata/sata_dwc_460ex.c line 1063
Quoted 6 lines
drivers/ata/sata_dwc_460ex.c new file +6
-0+/* Process completed command */ +dev_dbg (ap->dev,"%s NCQ command, protocol: %s\n" , __func__, +ata_get_cmd_descript (qc->tf.protocol)); +if (ata_is_dma (qc->tf.protocol)) { + host_pvt.dma_interrupt_count++; + if (hsdevp->dma_pending[tag] == \
\ not needed.
drivers/ata/sata_dwc_460ex.c line 1067
Quoted 4 lines
drivers/ata/sata_dwc_460ex.c new file +4
-0+ SATA_DWC_DMA_PENDING_NONE) + dev_warn(ap->dev, "%s: DMA not pending?\n", + __func__); +if ((host_pvt.dma_interrupt_count %2 ) ==0 )
Parens not needed around % operation.
drivers/ata/sata_dwc_460ex.c line 1072
Quoted 5 lines
drivers/ata/sata_dwc_460ex.c new file +5
-0+sata_dwc_dma_xfer_complete (ap,1 ); + }else { +if (unlikely (sata_dwc_qc_complete (ap, qc,1 ))) + goto STILLBUSY; + }
You should write:
} else if (unlikely(sata_dwc_qc_complete(ap, qc, 1))) {
goto STILLBUSY;
}drivers/ata/sata_dwc_460ex.c line 1089
Quoted 9 lines
drivers/ata/sata_dwc_460ex.c new file +9
-0+/* +* Check to see if any commands completed while we were processing our +* initial set of completed commands (read status clears interrupts, +* so we might miss a completed command interrupt if one came in while +* we were processing --we read status as part of processing a completed +* command). +*/ + sactive2 =core_scr_read (SCR_ACTIVE ); +if (sactive2 != sactive) {
{} not needed here.
Quoted 6 lines
drivers/ata/sata_dwc_460ex.c new file +6
-0+static void sata_dwc_dma_xfer_complete (struct ata_port *ap,u32 check_status) +{ +struct ata_queued_cmd *qc; +struct sata_dwc_device_port *hsdevp =HSDEVP_FROM_AP (ap); +struct sata_dwc_device *hsdev =HSDEV_FROM_AP (ap); +u8 tag =0 ;
drivers/ata/sata_dwc_460ex.c line 1142
Quoted 10 lines
drivers/ata/sata_dwc_460ex.c new file +10
-0+ + tag = ap->link.active_tag; + qc =ata_qc_from_tag (ap, tag); +if (!qc) { +dev_err (ap->dev,"failed to get qc" ); +return ; + } + +#ifdef DEBUG_NCQ +if (tag >0 ) {
Curly braces not needed around single statement.
drivers/ata/sata_dwc_460ex.c line 1152
Quoted 10 lines
drivers/ata/sata_dwc_460ex.c new file +10
-0+dev_info (ap->dev,"%s tag=%u cmd=0x%02x dma dir=%s proto=%s " +"dmacr=0x%08x\n" , __func__, qc->tag, qc->tf.command, +ata_get_cmd_descript (qc->dma_dir), +ata_get_cmd_descript (qc->tf.protocol), + in_le32(&(hsdev->sata_dwc_regs->dmacr))); + } +#endif + +if (ata_is_dma (qc->tf.protocol)) { +if (hsdevp->dma_pending[tag] ==SATA_DWC_DMA_PENDING_NONE ) {
Curly braces not needed around single statement.
drivers/ata/sata_dwc_460ex.c line 1172
Quoted 7 lines
drivers/ata/sata_dwc_460ex.c new file +7
-0+static int sata_dwc_qc_complete (struct ata_port *ap,struct ata_queued_cmd *qc, +u32 check_status) +{ +u8 status =0 ; +u32 mask =0x0 ; +u8 tag = qc->tag; +struct sata_dwc_device_port *hsdevp =HSDEVP_FROM_AP (ap);
There should be an empty line here.
drivers/ata/sata_dwc_460ex.c line 1185
Quoted 13 lines
drivers/ata/sata_dwc_460ex.c new file +13
-0+ host_pvt.sata_dwc_sactive_queued =0 ; +dev_dbg (ap->dev,"%s checkstatus? %x\n" , __func__, check_status); + +if (hsdevp->dma_pending[tag] ==SATA_DWC_DMA_PENDING_TX ) +dev_err (ap->dev,"TX DMA PENDING\n" ); +else if (hsdevp->dma_pending[tag] ==SATA_DWC_DMA_PENDING_RX ) +dev_err (ap->dev,"RX DMA PENDING\n" ); +dev_dbg (ap->dev,"QC complete cmd=0x%02x status=0x%02x ata%u:" +" protocol=%d\n" , qc->tf.command, status, ap->print_id, + qc->tf.protocol); + +/* clear active bit */ + mask = (~(qcmd_tag_to_mask (tag)));
Useless parens.
drivers/ata/sata_dwc_460ex.c line 1188
Quoted 3 lines
drivers/ata/sata_dwc_460ex.c new file +3
-0+ host_pvt.sata_dwc_sactive_queued = (host_pvt.sata_dwc_sactive_queued) \ + & mask; + host_pvt.sata_dwc_sactive_issued = (host_pvt.sata_dwc_sactive_issued) \
\ not needed.
drivers/ata/sata_dwc_460ex.c line 1331
Quoted 11 lines
drivers/ata/sata_dwc_460ex.c new file +11
-0+static void sata_dwc_port_stop (struct ata_port *ap) +{ +int i; +struct sata_dwc_device *hsdev =HSDEV_FROM_AP (ap); +struct sata_dwc_device_port *hsdevp =HSDEVP_FROM_AP (ap); + +dev_dbg (ap->dev,"%s: ap->id = %d\n" , __func__, ap->print_id); + +if (hsdevp && hsdev) { +/* deallocate LLI table */ +for (i =0 ; i <SATA_DWC_QCMD_MAX ; i++) {
Curly braces not needed.
drivers/ata/sata_dwc_460ex.c line 1334
Quoted 3 lines
drivers/ata/sata_dwc_460ex.c new file +3
-0+dma_free_coherent (ap->host->dev, +SATA_DWC_DMAC_LLI_TBL_SZ , + hsdevp->llit[i], hsdevp->llit_dma[i]);
Should be indented on the same level as above line
drivers/ata/sata_dwc_460ex.c line 1387
Quoted 10 lines
drivers/ata/sata_dwc_460ex.c new file +10
-0+static void sata_dwc_bmdma_setup (struct ata_queued_cmd *qc) +{ +u8 tag = qc->tag; + +if (ata_is_ncq (qc->tf.protocol)) { +dev_dbg (qc->ap->dev,"%s: ap->link.sactive=0x%08x tag=%d\n" , + __func__, qc->ap->link.sactive, tag); + }else { + tag =0 ; + }
Curly braces not needed around single statements.
drivers/ata/sata_dwc_460ex.c line 1398
Quoted 8 lines
drivers/ata/sata_dwc_460ex.c new file +8
-0+static void sata_dwc_bmdma_start_by_tag (struct ata_queued_cmd *qc,u8 tag) +{ +int start_dma; +u32 reg, dma_chan; +struct sata_dwc_device *hsdev =HSDEV_FROM_QC (qc); +struct ata_port *ap = qc->ap; +struct sata_dwc_device_port *hsdevp =HSDEVP_FROM_AP (ap); +int dir = qc->dma_dir;
There should be an empty line here.
drivers/ata/sata_dwc_460ex.c line 1421
Quoted 3 lines
drivers/ata/sata_dwc_460ex.c new file +3
-0+if (start_dma) { + reg =core_scr_read (SCR_ERROR ); +if (reg &SATA_DWC_SERROR_ERR_BITS ) {
Curly braces not needed here.
drivers/ata/sata_dwc_460ex.c line 1447
Quoted 10 lines
drivers/ata/sata_dwc_460ex.c new file +10
-0+static void sata_dwc_bmdma_start (struct ata_queued_cmd *qc) +{ +u8 tag = qc->tag; + +if (ata_is_ncq (qc->tf.protocol)) { +dev_dbg (qc->ap->dev,"%s: ap->link.sactive=0x%08x tag=%d\n" , + __func__, qc->ap->link.sactive, tag); + }else { + tag =0 ; + }
Curly braces not needed around single statements.
Quoted 8 lines
drivers/ata/sata_dwc_460ex.c new file +8
-0+/* +* Function : sata_dwc_qc_prep_by_tag +* arguments : ata_queued_cmd *qc, u8 tag +* Return value : None +* qc_prep for a particular queued command based on tag +*/ +static void sata_dwc_qc_prep_by_tag (struct ata_queued_cmd *qc,u8 tag) +{
[...]
drivers/ata/sata_dwc_460ex.c line 1473
Quoted 3 lines
drivers/ata/sata_dwc_460ex.c new file +3
-0+ dma_chan = dma_dwc_xfer_setup(sg, qc->n_elem, hsdevp->llit[tag], + hsdevp->llit_dma[tag], + (void *__iomem)(&hsdev->sata_dwc_regs->\
\ not needed.
Quoted 5 lines
drivers/ata/sata_dwc_460ex.c new file +5
-0+static unsigned int sata_dwc_qc_issue (struct ata_queued_cmd *qc) +{ +u32 sactive; +u8 tag = qc->tag; +struct ata_port *ap = qc->ap;
[...]
drivers/ata/sata_dwc_460ex.c line 1505
Quoted 4 lines
drivers/ata/sata_dwc_460ex.c new file +4
-0+ +if (ata_is_ncq (qc->tf.protocol)) { + sactive =core_scr_read (SCR_ACTIVE ); + sactive |= (0x00000001 << tag);
Parens not needed.
drivers/ata/sata_dwc_460ex.c line 1512
Quoted 7 lines
drivers/ata/sata_dwc_460ex.c new file +7
-0+core_scr_write (SCR_ACTIVE , sactive); + +dev_dbg (qc->ap->dev,"%s: tag=%d ap->link.sactive = 0x%08x " +"sactive=0x%08x\n" , __func__, tag, qc->ap->link.sactive, + sactive); + + ap->ops->sff_tf_load (ap, &qc->tf);
Why not call ata_sff_tf_load() directly?
drivers/ata/sata_dwc_460ex.c line 1530
Quoted 10 lines
drivers/ata/sata_dwc_460ex.c new file +10
-0+/* +* Function : sata_dwc_qc_prep +* arguments : ata_queued_cmd *qc +* Return value : None +* qc_prep for a particular queued command +*/ + +static void sata_dwc_qc_prep (struct ata_queued_cmd *qc) +{ +if ((qc->dma_dir ==DMA_NONE ) || (qc->tf.protocol ==ATA_PROT_PIO ))
Parens not needed arounf == operation.
drivers/ata/sata_dwc_460ex.c line 1538
Quoted 8 lines
drivers/ata/sata_dwc_460ex.c new file +8
-0+return ; + +#ifdef DEBUG_NCQ +if (qc->tag >0 ) +dev_info (qc->ap->dev,"%s: qc->tag=%d ap->active_tag=0x%08x\n" , + __func__, tag, qc->ap->link.active_tag); + +return ;
Remove spade before semicolon please.
drivers/ata/sata_dwc_460ex.c line 1585
Quoted 5 lines
drivers/ata/sata_dwc_460ex.c new file +5
-0+static const struct ata_port_info sata_dwc_port_info[] = { + { + .flags =ATA_FLAG_SATA |ATA_FLAG_NO_LEGACY | +ATA_FLAG_MMIO |ATA_FLAG_NCQ , + .pio_mask =0x1f ,/* pio 0-4 */
Replace 0x1f with ATA_PIO4, please.
drivers/ata/sata_dwc_460ex.c line 1592
Quoted 2 lines
drivers/ata/sata_dwc_460ex.c new file +2
-0+static int sata_dwc_probe (struct of_device *ofdev, +const struct of_device_id *match)
Shouldn't this function be '__init' or '__devinit'?
Quoted 9 lines
drivers/ata/sata_dwc_460ex.c new file +9
-0+{ +struct sata_dwc_device *hsdev; +u32 idr, versionr; +char *ver = (char *)&versionr; +u8 *base =NULL ; +int err =0 ; +int irq, rc; +struct ata_host *host; +struct ata_port_info pi = sata_dwc_port_info[0 ];
drivers/ata/sata_dwc_460ex.c line 1611
Quoted 10 lines
drivers/ata/sata_dwc_460ex.c new file +10
-0+const struct ata_port_info *ppi[] = { &pi,NULL }; + +/* Allocate DWC SATA device */ + hsdev =kmalloc (sizeof (*hsdev),GFP_KERNEL ); +if (hsdev ==NULL ) { +dev_err (&ofdev->dev,"kmalloc failed for hsdev\n" ); + err = -ENOMEM ; + goto error_out; + } +memset (hsdev,0 ,sizeof (*hsdev));
Use kzalloc() instead iof kmalloc()/memset().
drivers/ata/sata_dwc_460ex.c line 1659
Quoted 3 lines
drivers/ata/sata_dwc_460ex.c new file +3
-0+/* Get physical SATA DMA register base address */ + host_pvt.sata_dma_regs =of_iomap (ofdev->dev.of_node,1 ); +if (!(host_pvt.sata_dma_regs)) {
Parens not needed around 'host_pvt.sata_dma_regs'.
drivers/ata/sata_dwc_460ex.c line 1689
Quoted 7 lines
drivers/ata/sata_dwc_460ex.c new file +7
-0+/* +* Now, register with libATA core, this will also initiate the +* device discovery process, invoking our port_start() handler & +* error_handler() to execute a dummy Softreset EH session +*/ + rc =ata_host_activate (host, irq, sata_dwc_isr,0 , &sata_dwc_sht); +
I think that empty line here is not needed.
drivers/ata/sata_dwc_460ex.c line 1701
Quoted 12 lines
drivers/ata/sata_dwc_460ex.c new file +12
-0+if (rc !=0 ) +dev_err (&ofdev->dev,"failed to activate host" ); + +dev_set_drvdata (&ofdev->dev, host); +return 0 ; + +error_out: +/* Free SATA DMA resources */ +dma_dwc_exit (hsdev); + +if (base) +iounmap (base);
What about freeing 'hsdev' and 'host' and unmapping
'host_pvt.sata_dma_regs'? And does iounmap() really pair with of_iomap()?
Quoted 1 line
+static int
sata_dwc_remove(struct of_device *ofdev)
Shouldn't this be '__exit' or '__devexit'?
drivers/ata/sata_dwc_460ex.c line 1717
Quoted 12 lines
drivers/ata/sata_dwc_460ex.c new file +12
-0+{ +struct device *dev = &ofdev->dev; +struct ata_host *host =dev_get_drvdata (dev); +struct sata_dwc_device *hsdev = host->private_data; + +ata_host_detach (host); +dev_set_drvdata (dev,NULL ); + +/* Free SATA DMA resources */ +dma_dwc_exit (hsdev); + +iounmap (hsdev->reg_base);
What about unmapping 'host_pvt.sata_dma_regs'?
drivers/ata/sata_dwc_460ex.c line 1719
Quoted 2 lines
drivers/ata/sata_dwc_460ex.c new file +2
-0+kfree (hsdev); +kfree (host);
Does kfree() really pair with ata_host_alloc_pinfo()?
MBR, Sergei
Hi Sergei,
Thanks for your suggestions.
I would look into them and submit a patch for style improvement some time later.
Regards,
Rup
-----Original Message-----
From: Sergei Shtylyov [mailto:[email protected]]
Sent: Saturday, July 17, 2010 9:21 PM
To: Rupjyoti Sarmah
Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH v2]460EX on-chip SATA driver<resubmisison>
Hello.
Quoted 1 line · Rupjyoti Sarmah
This patch enables the on-chip DWC SATA controller of the AppliedMicro
processor 460EX.
Too bad thius has already been applied but here's my (mostly
stylistic)
comments anyway:
Quoted 3 lines
Signed-off-by Rupjyoti Sarmah <[email protected]>Signed-off-by Mark Miesfeld <[email protected]>Signed-off-by Prodyut Hazarika <[email protected]>
[...]
drivers/ata/sata_dwc_460ex.c line 2
Quoted 8 lines
drivers/ata/sata_dwc_460ex.c new file +2
-0@@ -0,0 +1,1756 @@ +/* +* drivers/ata/sata_dwc_460ex.c
Filenames in the heading comments have long been frowned upon.
drivers/ata/sata_dwc_460ex.c line 21
Quoted 1 line
drivers/ata/sata_dwc_460ex.c new file +1
-0+#ifdef CONFIG_SATA_DWC_DEBUG
I don't see this option defined anywahere.
drivers/ata/sata_dwc_460ex.c line 25
Quoted 4 lines
drivers/ata/sata_dwc_460ex.c new file +4
-0+#define DEBUG +#endif + +#ifdef CONFIG_SATA_DWC_VDEBUG
The same about this one.
Quoted 3 lines
drivers/ata/sata_dwc_460ex.c new file +3
-0+#define VERBOSE_DEBUG +#define DEBUG_NCQ +#endif
[...]
drivers/ata/sata_dwc_460ex.c line 51
Quoted 6 lines
drivers/ata/sata_dwc_460ex.c new file +6
-0+/* SATA DMA driver Globals */ +#define DMA_NUM_CHANS 1 +#define DMA_NUM_CHAN_REGS 8 + +/* SATA DMA Register definitions */ +#define AHB_DMA_BRST_DFLT 64/* 16 data items burst length*/
Please put a space before */.
drivers/ata/sata_dwc_460ex.c
Quoted 3 lines
drivers/ata/sata_dwc_460ex.c new file +3
-0+struct ahb_dma_regs { +struct dma_chan_regs chan_regs[DMA_NUM_CHAN_REGS ]; +struct dma_interrupt_regs interrupt_raw;/* Raw Interrupt
*/
drivers/ata/sata_dwc_460ex.c
Quoted 1 line
drivers/ata/sata_dwc_460ex.c new file +1
-0+struct dma_interrupt_regs interrupt_status;/* Interrupt
Status */
drivers/ata/sata_dwc_460ex.c
Quoted 1 line
drivers/ata/sata_dwc_460ex.c new file +1
-0+struct dma_interrupt_regs interrupt_mask;/* Interrupt Mask
*/
drivers/ata/sata_dwc_460ex.c
Quoted 1 line
drivers/ata/sata_dwc_460ex.c new file +1
-0+struct dma_interrupt_regs interrupt_clear;/* Interrupt Clear
*/
drivers/ata/sata_dwc_460ex.c line 88
Quoted 1 line
drivers/ata/sata_dwc_460ex.c new file +1
-0+struct dmareg statusInt;/* Interrupt combined*/
No camelCase please, rename it to status_int.
Quoted 1 line
+#define
DMA_CTL_BLK_TS(size)((size) &0x000000FFF) /* Blk
Transfer size */
Quoted 1 line
+#define
DMA_CHANNEL(ch)(0x00000001<< (ch)) /* Select
channel */
drivers/ata/sata_dwc_460ex.c
Quoted 2 lines
drivers/ata/sata_dwc_460ex.c new file +2
-0+/* Enable channel */ +#define DMA_ENABLE_CHAN (ch) ((0x00000001 << (ch)) |
\
drivers/ata/sata_dwc_460ex.c
Quoted 3 lines
drivers/ata/sata_dwc_460ex.c new file +3
-0+ ((0x000000001 << (ch)) << 8)) +/* Disable channel */ +#define DMA_DISABLE_CHAN (ch) (0x00000000 | ((0x000000001 <<
(ch)) << 8))
What's the point of OR'ing with zero?
drivers/ata/sata_dwc_460ex.c line 298
Quoted 13 lines
drivers/ata/sata_dwc_460ex.c new file +13
-0+/* +* Commonly used DWC SATA driver Macros +*/ +#define HSDEV_FROM_HOST (host) ((struct sata_dwc_device *)\ + (host)->private_data) +#define HSDEV_FROM_AP (ap) ((struct sata_dwc_device *)\ + (ap)->host->private_data) +#define HSDEVP_FROM_AP (ap) ((struct sata_dwc_device_port *)\ + (ap)->private_data) +#define HSDEV_FROM_QC (qc) ((struct sata_dwc_device *)\ + (qc)->ap->host->private_data) +#define HSDEV_FROM_HSDEVP (p) ((struct sata_dwc_device *)\ + (hsdevp)->hsdev)
Are you sure it's '(hsdevp)', not '(p)'?
drivers/ata/sata_dwc_460ex.c line 314
Quoted 4 lines
drivers/ata/sata_dwc_460ex.c new file +4
-0+struct sata_dwc_host_priv { +void __iomem *scr_addr_sstatus; +u32 sata_dwc_sactive_issued ; +u32 sata_dwc_sactive_queued ;
Remove spaces befoer semicolons, please.
drivers/ata/sata_dwc_460ex.c
Quoted 3 lines
drivers/ata/sata_dwc_460ex.c new file +3
-0+static void sata_dwc_tf_dump (struct ata_taskfile *tf) +{ + dev_vdbg(host_pvt.dwc_dev, "taskfile cmd: 0x%02x protocol: %s
flags:"
drivers/ata/sata_dwc_460ex.c line 339
Quoted 1 line
drivers/ata/sata_dwc_460ex.c new file +1
-0+"0x%lx device: %x\n" , tf->command, ata_get_cmd_descript\
There's no need to use \ outside macro defintions.
Quoted 4 lines
+/*
+ * Function:get_burst_length_encode
+ * arguments: datalength: length in bytes of data
+ * returns value to be programmed in register corrresponding to data
length
Quoted 13 lines
+ * This value is effectively the log(base 2) of the length
+ */drivers/ata/sata_dwc_460ex.c new file +11
-0+static int get_burst_length_encode (int datalength) +{ +int items = datalength >>2 ;/* div by 4 to get lword count */ + +if (items >=64 ) +return 5 ; + +if (items >=32 ) +return 4 ; + +if (items >=16 )
drivers/ata/sata_dwc_460ex.c line 376
Quoted 10 lines
drivers/ata/sata_dwc_460ex.c new file +10
-0+return 3 ; + +if (items >=8 ) +return 2 ; + +if (items >=4 ) +return 1 ; + +return 0 ; +}
Hmm, there should be a function in the kernel to calculate 2^n order
from
size, something like get_count_order()...
Quoted 15 lines
drivers/ata/sata_dwc_460ex.c new file +15
-0+/* +* Function: dma_dwc_interrupt +* arguments: irq, dev_id, pt_regs +* returns channel number if available else -1 +* Interrupt Handler for DW AHB SATA DMA +*/ +static irqreturn_t dma_dwc_interrupt (int irq,void *hsdev_instance) +{ +int chan; +u32 tfr_reg, err_reg; +unsigned long flags; +struct sata_dwc_device *hsdev = + (struct sata_dwc_device *)hsdev_instance; +struct ata_host *host = (struct ata_host *)hsdev->host; +struct ata_port *ap;
drivers/ata/sata_dwc_460ex.c line 437
Quoted 10 lines
drivers/ata/sata_dwc_460ex.c new file +10
-0+struct sata_dwc_device_port *hsdevp; +u8 tag =0 ; +unsigned int port =0 ; + +spin_lock_irqsave (&host->lock, flags); + ap = host->ports[port]; + hsdevp =HSDEVP_FROM_AP (ap); + tag = ap->link.active_tag; + + tfr_reg = in_le32(&(host_pvt.sata_dma_regs->interrupt_status.tfr\
There's no need to use \ outside macro defintions.
drivers/ata/sata_dwc_460ex.c
Quoted 2 lines
drivers/ata/sata_dwc_460ex.c new file +2
-0+ .low)); + err_reg =
in_le32(&(host_pvt.sata_dma_regs->interrupt_status.error\
Same comment here...
drivers/ata/sata_dwc_460ex.c
Quoted 6 lines
drivers/ata/sata_dwc_460ex.c new file +6
-0+for (chan =0 ; chan <DMA_NUM_CHANS ; chan++) { +/* Check for end-of-transfer interrupt. */ +if (tfr_reg &DMA_CHANNEL (chan)) { +/* +* Each DMA command produces 2 interrupts. Only +* complete the command after both interrupts have
been
drivers/ata/sata_dwc_460ex.c
Quoted 8 lines
drivers/ata/sata_dwc_460ex.c new file +8
-0+* seen. (See sata_dwc_isr()) +*/ + host_pvt.dma_interrupt_count++; +sata_dwc_clear_dmacr (hsdevp, tag); + +if (hsdevp->dma_pending[tag] == +SATA_DWC_DMA_PENDING_NONE ) { + dev_err(ap->dev, "DMA not pending
eot=0x%08x "
drivers/ata/sata_dwc_460ex.c
Quoted 1 line
drivers/ata/sata_dwc_460ex.c new file +1
-0+ "err=0x%08x tag=0x%02x
pending=%d\n",
drivers/ata/sata_dwc_460ex.c line 464
Quoted 5 lines
drivers/ata/sata_dwc_460ex.c new file +5
-0+ tfr_reg, err_reg, tag, + hsdevp->dma_pending[tag]); + } + +if ((host_pvt.dma_interrupt_count %2 ) ==0 )
Prens around % operation not necessary.
drivers/ata/sata_dwc_460ex.c line 472
Quoted 4 lines
drivers/ata/sata_dwc_460ex.c new file +4
-0+sata_dwc_dma_xfer_complete (ap,1 ); + +/* Clear the interrupt */ +
out_le32(&(host_pvt.sata_dma_regs->interrupt_clear\
\ not needed.
drivers/ata/sata_dwc_460ex.c line 488
Quoted 2 lines
drivers/ata/sata_dwc_460ex.c new file +2
-0+/* Clear the interrupt. */ +
out_le32(&(host_pvt.sata_dma_regs->interrupt_clear\
\ not needed.
Quoted 4 lines
+/*
+ * Function:map_sg_to_lli
+ * The Synopsis driver has a comment proposing that better performance
+ * is possible by only enabling interrupts on the last item in the
linked list.
Quoted 1 line
+ * However, it seems that could be a problem if an error happened on
one of the
Quoted 1 line
+ * first items. The transfer would halt, but no error interrupt would
occur.
Quoted 1 line
+ * Currently this function sets interrupts enabled for each linked list
item:
Quoted 6 lines
+ *
DMA_CTL_INT_EN.
+ */drivers/ata/sata_dwc_460ex.c new file +4
-0+static int map_sg_to_lli (struct scatterlist *sg,int num_elems, +struct lli *lli,dma_addr_t dma_lli, +void __iomem *dmadr_addr,int dir) +{
[...]
drivers/ata/sata_dwc_460ex.c line 632
Quoted 2 lines
drivers/ata/sata_dwc_460ex.c new file +2
-0+/* Program the LLI CTL high register */ + lli[idx].ctl.high = cpu_to_le32(DMA_CTL_BLK_TS\
\ not needed.
drivers/ata/sata_dwc_460ex.c
Quoted 3 lines
drivers/ata/sata_dwc_460ex.c new file +3
-0+ (len / 4)); + +/* Program the next pointer. The next pointer
must be
drivers/ata/sata_dwc_460ex.c line 638
Quoted 3 lines
drivers/ata/sata_dwc_460ex.c new file +3
-0+* the physical address, not the virtual address. +*/ + next_llp = (dma_lli + ((idx + 1) * sizeof(struct \
Same here...
drivers/ata/sata_dwc_460ex.c line 688
Quoted 6 lines
drivers/ata/sata_dwc_460ex.c new file +6
-0+static int dma_dwc_xfer_setup (struct scatterlist *sg,int num_elems, +struct lli *lli,dma_addr_t dma_lli, +void __iomem *addr,int dir) +{ +int dma_ch; +int num_lli;
There should be an empty line here.
drivers/ata/sata_dwc_460ex.c line 747
Quoted 11 lines
drivers/ata/sata_dwc_460ex.c new file +11
-0+/* +* Function: dma_dwc_init +* arguments: hsdev +* returns status +* This function initializes the SATA DMA driver +*/ +static int dma_dwc_init (struct sata_dwc_device *hsdev,int irq) +{ +int err; + + err =dma_request_interrupts (hsdev, irq);
Could be initilaizer...
drivers/ata/sata_dwc_460ex.c line 758
Quoted 2 lines
drivers/ata/sata_dwc_460ex.c new file +2
-0+dev_notice (host_pvt.dwc_dev,"DMA initialized\n" ); + dev_dbg(host_pvt.dwc_dev, "SATA DMA registers=0x%p\n", host_pvt.\
\ not needed.
drivers/ata/sata_dwc_460ex.c line 800
Quoted 3 lines
drivers/ata/sata_dwc_460ex.c new file +3
-0+static u32 core_scr_read (unsigned int scr) +{ + return in_le32((void __iomem *)(host_pvt.scr_addr_sstatus) +\
Not needed.
drivers/ata/sata_dwc_460ex.c line 812
Quoted 3 lines
drivers/ata/sata_dwc_460ex.c new file +3
-0+static void clear_serror (void ) +{ +u32 val;
Empty line needed here.
drivers/ata/sata_dwc_460ex.c line 813
Quoted 1 line
drivers/ata/sata_dwc_460ex.c new file +1
-0+ val =core_scr_read (SCR_ERROR );
This could be an initilaizer.
Quoted 6 lines
drivers/ata/sata_dwc_460ex.c new file +6
-0+/* See ahci.c */ +static void sata_dwc_error_intr (struct ata_port *ap, +struct sata_dwc_device *hsdev,uint intpr) +{ +struct sata_dwc_device_port *hsdevp =HSDEVP_FROM_AP (ap); +struct ata_eh_info *ehi = &ap->link.eh_info;
drivers/ata/sata_dwc_460ex.c line 844
Quoted 10 lines
drivers/ata/sata_dwc_460ex.c new file +10
-0+unsigned int err_mask =0 , action =0 ; +struct ata_queued_cmd *qc; +u32 serror; +u8 status, tag; +u32 err_reg; + +ata_ehi_clear_desc (ehi); + + serror =core_scr_read (SCR_ERROR ); + status = ap->ops->sff_check_status (ap);
Why not call ata_sff_check_status() directly?
drivers/ata/sata_dwc_460ex.c
Quoted 1 line
drivers/ata/sata_dwc_460ex.c new file +1
-0+ err_reg =
in_le32(&(host_pvt.sata_dma_regs->interrupt_status.error.\
\ not neeeded.
Quoted 19 lines
drivers/ata/sata_dwc_460ex.c new file +19
-0+/* +* Function : sata_dwc_isr +* arguments : irq, void *dev_instance, struct pt_regs *regs +* Return value : irqreturn_t - status of IRQ +* This Interrupt handler called via port ops registered function. +* .irq_handler = sata_dwc_isr +*/ +static irqreturn_t sata_dwc_isr (int irq,void *dev_instance) +{ +struct ata_host *host = (struct ata_host *)dev_instance; +struct sata_dwc_device *hsdev =HSDEV_FROM_HOST (host); +struct ata_port *ap; +struct ata_queued_cmd *qc; +unsigned long flags; +u8 status, tag; +int handled, num_processed, port =0 ; +uint intpr, sactive, sactive2, tag_mask; +struct sata_dwc_device_port *hsdevp; + host_pvt.sata_dwc_sactive_issued =0 ;
[...]
drivers/ata/sata_dwc_460ex.c
Quoted 2 lines
drivers/ata/sata_dwc_460ex.c new file +2
-0+ sactive =core_scr_read (SCR_ACTIVE ); + tag_mask = (host_pvt.sata_dwc_sactive_issued | sactive) ^ sactive;
Isn't it the same as 'host_pvt.sata_dwc_sactive_issued & ~sactive'?
drivers/ata/sata_dwc_460ex.c
Quoted 1 line
drivers/ata/sata_dwc_460ex.c new file +1
-0+/* If no sactive issued and tag_mask is zero then this is not NCQ
*/
drivers/ata/sata_dwc_460ex.c line 953
Quoted 12 lines
drivers/ata/sata_dwc_460ex.c new file +12
-0+if (host_pvt.sata_dwc_sactive_issued ==0 && tag_mask ==0 ) { +if (ap->link.active_tag ==ATA_TAG_POISON ) + tag =0 ; +else + tag = ap->link.active_tag; + qc =ata_qc_from_tag (ap, tag); + +/* DEV interrupt w/ no active qc? */ +if (unlikely (!qc || (qc->tf.flags &ATA_TFLAG_POLLING ))) { +dev_err (ap->dev,"%s interrupt with no active qc " +"qc=%p\n" , __func__, qc); + ap->ops->sff_check_status (ap);
Call ata_sff_check_status() directly...
drivers/ata/sata_dwc_460ex.c line 957
Quoted 4 lines
drivers/ata/sata_dwc_460ex.c new file +4
-0+ handled =1 ; + goto DONE; + } + status = ap->ops->sff_check_status (ap);
Here too...
drivers/ata/sata_dwc_460ex.c
Quoted 4 lines
drivers/ata/sata_dwc_460ex.c new file +4
-0+DRVSTILLBUSY: +if (ata_is_dma (qc->tf.protocol)) { +/* +* Each DMA transaction produces 2 interrupts. The
DMAC
drivers/ata/sata_dwc_460ex.c
Quoted 1 line
drivers/ata/sata_dwc_460ex.c new file +1
-0+* transfer complete interrupt and the SATA
controller
drivers/ata/sata_dwc_460ex.c
Quoted 8 lines
drivers/ata/sata_dwc_460ex.c new file +8
-0+* operation done interrupt. The command should be +* completed only after both interrupts are seen. +*/ + host_pvt.dma_interrupt_count++; + if (hsdevp->dma_pending[tag] == \ + SATA_DWC_DMA_PENDING_NONE) { + dev_err(ap->dev, "%s: DMA not pending " + "intpr=0x%08x status=0x%08x
pending"
drivers/ata/sata_dwc_460ex.c line 986
Quoted 3 lines
drivers/ata/sata_dwc_460ex.c new file +3
-0+"=%d\n" , __func__, intpr, status, + hsdevp->dma_pending[tag]); + }
Curly brace not needed here. I assume you haven't run the patch thruscripts/checkpatch.pl?
drivers/ata/sata_dwc_460ex.c
Quoted 2 lines
drivers/ata/sata_dwc_460ex.c new file +2
-0+/* +* This is a NCQ command. At this point we need to figure out for
which
drivers/ata/sata_dwc_460ex.c
Quoted 1 line
drivers/ata/sata_dwc_460ex.c new file +1
-0+* tags we have gotten a completion interrupt. One interrupt may
serve
drivers/ata/sata_dwc_460ex.c
Quoted 1 line
drivers/ata/sata_dwc_460ex.c new file +1
-0+* as completion for more than one operation when commands are
queued
drivers/ata/sata_dwc_460ex.c line 1012
Quoted 6 lines
drivers/ata/sata_dwc_460ex.c new file +6
-0+* (NCQ). We need to process each completed command. +*/ + +/* process completed commands */ + sactive =core_scr_read (SCR_ACTIVE ); + tag_mask = (host_pvt.sata_dwc_sactive_issued | sactive) ^ sactive;
Isn't it the same as 'host_pvt.sata_dwc_sactive_issued & ~sactive'?
drivers/ata/sata_dwc_460ex.c line 1014
Quoted 1 line
drivers/ata/sata_dwc_460ex.c new file +1
-0+ if (sactive != 0 || (host_pvt.sata_dwc_sactive_issued) > 1 || \
Useless parens around 'host_pvt.sata_dwc_sactive_issued' + \ not
needed.
drivers/ata/sata_dwc_460ex.c
Quoted 2 lines
drivers/ata/sata_dwc_460ex.c new file +2
-0+ tag_mask > 1) { + dev_dbg(ap->dev, "%s NCQ:sactive=0x%08x
sactive_issued=0x%08x"
drivers/ata/sata_dwc_460ex.c line 1019
Quoted 3 lines
drivers/ata/sata_dwc_460ex.c new file +3
-0+"tag_mask=0x%08x\n" , __func__, sactive, + host_pvt.sata_dwc_sactive_issued, tag_mask); + }
Curly braces not needed here.
drivers/ata/sata_dwc_460ex.c line 1028
Quoted 2 lines
drivers/ata/sata_dwc_460ex.c new file +2
-0+ if ((tag_mask | (host_pvt.sata_dwc_sactive_issued)) != \ +
(host_pvt.sata_dwc_sactive_issued)) {
Useless parens around 'host_pvt.sata_dwc_sactive_issued'.
drivers/ata/sata_dwc_460ex.c
Quoted 2 lines
drivers/ata/sata_dwc_460ex.c new file +2
-0+dev_warn (ap->dev,"Bad tag mask? sactive=0x%08x " + "(host_pvt.sata_dwc_sactive_issued)=0x%08x
tag_mask"
drivers/ata/sata_dwc_460ex.c
Quoted 1 line
drivers/ata/sata_dwc_460ex.c new file +1
-0+ "=0x%08x\n", sactive,
host_pvt.sata_dwc_sactive_issued,
drivers/ata/sata_dwc_460ex.c line 1027
Quoted 2 lines
drivers/ata/sata_dwc_460ex.c new file +2
-0+ tag_mask); + }
Curly braces not needed around single statement.
drivers/ata/sata_dwc_460ex.c line 1030
Quoted 3 lines
drivers/ata/sata_dwc_460ex.c new file +3
-0+ +/* read just to clear ... not bad if currently still busy */ + status = ap->ops->sff_check_status (ap);
Call ata_sff_check_status() directly.
drivers/ata/sata_dwc_460ex.c
Quoted 1 line
drivers/ata/sata_dwc_460ex.c new file +1
-0+ dev_dbg(ap->dev, "%s ATA status register=0x%x\n", __func__,
status);
drivers/ata/sata_dwc_460ex.c line 1042
Quoted 11 lines
drivers/ata/sata_dwc_460ex.c new file +11
-0+ + tag =0 ; + num_processed =0 ; +while (tag_mask) { + num_processed++; +while (!(tag_mask &0x00000001 )) { + tag++; + tag_mask <<=1 ; + } + + tag_mask &= (~0x00000001 );
Useless parens.
drivers/ata/sata_dwc_460ex.c
Quoted 2 lines
drivers/ata/sata_dwc_460ex.c new file +2
-0+/* Process completed command */ + dev_dbg(ap->dev, "%s NCQ command, protocol: %s\n",
__func__,
drivers/ata/sata_dwc_460ex.c line 1063
Quoted 4 lines
drivers/ata/sata_dwc_460ex.c new file +4
-0+ ata_get_cmd_descript(qc->tf.protocol)); +if (ata_is_dma (qc->tf.protocol)) { + host_pvt.dma_interrupt_count++; + if (hsdevp->dma_pending[tag] == \
\ not needed.
drivers/ata/sata_dwc_460ex.c
Quoted 2 lines
drivers/ata/sata_dwc_460ex.c new file +2
-0+ SATA_DWC_DMA_PENDING_NONE) + dev_warn(ap->dev, "%s: DMA not
pending?\n",
drivers/ata/sata_dwc_460ex.c line 1067
Quoted 2 lines
drivers/ata/sata_dwc_460ex.c new file +2
-0+ __func__); +if ((host_pvt.dma_interrupt_count %2 ) ==0 )
Parens not needed around % operation.
drivers/ata/sata_dwc_460ex.c line 1072
Quoted 5 lines
drivers/ata/sata_dwc_460ex.c new file +5
-0+sata_dwc_dma_xfer_complete (ap,1 ); + }else { +if (unlikely (sata_dwc_qc_complete (ap, qc,1 ))) + goto STILLBUSY; + }
You should write:
} else if (unlikely(sata_dwc_qc_complete(ap, qc, 1))) {
goto STILLBUSY;
}drivers/ata/sata_dwc_460ex.c
Quoted 2 lines
drivers/ata/sata_dwc_460ex.c new file +2
-0+/* +* Check to see if any commands completed while we were processing
our
drivers/ata/sata_dwc_460ex.c
Quoted 1 line
drivers/ata/sata_dwc_460ex.c new file +1
-0+* initial set of completed commands (read status clears
interrupts,
drivers/ata/sata_dwc_460ex.c
Quoted 1 line
drivers/ata/sata_dwc_460ex.c new file +1
-0+* so we might miss a completed command interrupt if one came in
while
drivers/ata/sata_dwc_460ex.c
Quoted 1 line
drivers/ata/sata_dwc_460ex.c new file +1
-0+* we were processing --we read status as part of processing a
completed
drivers/ata/sata_dwc_460ex.c line 1089
Quoted 4 lines
drivers/ata/sata_dwc_460ex.c new file +4
-0+* command). +*/ + sactive2 =core_scr_read (SCR_ACTIVE ); +if (sactive2 != sactive) {
{} not needed here.
Quoted 1 line
+static void sata_dwc_dma_xfer_complete(struct
ata_port*ap, u32
check_status)
Quoted 5 lines
drivers/ata/sata_dwc_460ex.c new file +5
-0+{ +struct ata_queued_cmd *qc; +struct sata_dwc_device_port *hsdevp =HSDEVP_FROM_AP (ap); +struct sata_dwc_device *hsdev =HSDEV_FROM_AP (ap); +u8 tag =0 ;
drivers/ata/sata_dwc_460ex.c line 1142
Quoted 10 lines
drivers/ata/sata_dwc_460ex.c new file +10
-0+ + tag = ap->link.active_tag; + qc =ata_qc_from_tag (ap, tag); +if (!qc) { +dev_err (ap->dev,"failed to get qc" ); +return ; + } + +#ifdef DEBUG_NCQ +if (tag >0 ) {
Curly braces not needed around single statement.
drivers/ata/sata_dwc_460ex.c
Quoted 1 line
drivers/ata/sata_dwc_460ex.c new file +1
-0+ dev_info(ap->dev, "%s tag=%u cmd=0x%02x dma dir=%s
proto=%s "
drivers/ata/sata_dwc_460ex.c
Quoted 1 line
drivers/ata/sata_dwc_460ex.c new file +1
-0+ "dmacr=0x%08x\n", __func__, qc->tag,
qc->tf.command,
drivers/ata/sata_dwc_460ex.c
Quoted 8 lines
drivers/ata/sata_dwc_460ex.c new file +8
-0+ata_get_cmd_descript (qc->dma_dir), +ata_get_cmd_descript (qc->tf.protocol), + in_le32(&(hsdev->sata_dwc_regs->dmacr))); + } +#endif + +if (ata_is_dma (qc->tf.protocol)) { +if (hsdevp->dma_pending[tag] ==SATA_DWC_DMA_PENDING_NONE )
{
Curly braces not needed around single statement.
Quoted 1 line
+static int sata_dwc_qc_complete(struct
ata_port*ap, struct
ata_queued_cmd *qc,
drivers/ata/sata_dwc_460ex.c line 1172
Quoted 6 lines
drivers/ata/sata_dwc_460ex.c new file +6
-0+ u32 check_status) +{ +u8 status =0 ; +u32 mask =0x0 ; +u8 tag = qc->tag; +struct sata_dwc_device_port *hsdevp =HSDEVP_FROM_AP (ap);
There should be an empty line here.
drivers/ata/sata_dwc_460ex.c line 1185
Quoted 13 lines
drivers/ata/sata_dwc_460ex.c new file +13
-0+ host_pvt.sata_dwc_sactive_queued =0 ; +dev_dbg (ap->dev,"%s checkstatus? %x\n" , __func__, check_status); + +if (hsdevp->dma_pending[tag] ==SATA_DWC_DMA_PENDING_TX ) +dev_err (ap->dev,"TX DMA PENDING\n" ); +else if (hsdevp->dma_pending[tag] ==SATA_DWC_DMA_PENDING_RX ) +dev_err (ap->dev,"RX DMA PENDING\n" ); +dev_dbg (ap->dev,"QC complete cmd=0x%02x status=0x%02x ata%u:" +" protocol=%d\n" , qc->tf.command, status, ap->print_id, + qc->tf.protocol); + +/* clear active bit */ + mask = (~(qcmd_tag_to_mask (tag)));
Useless parens.
drivers/ata/sata_dwc_460ex.c
Quoted 1 line
drivers/ata/sata_dwc_460ex.c new file +1
-0+ host_pvt.sata_dwc_sactive_queued =
(host_pvt.sata_dwc_sactive_queued) \
drivers/ata/sata_dwc_460ex.c
Quoted 2 lines
drivers/ata/sata_dwc_460ex.c new file +2
-0+ & mask; + host_pvt.sata_dwc_sactive_issued =
(host_pvt.sata_dwc_sactive_issued) \
\ not needed.
drivers/ata/sata_dwc_460ex.c line 1331
Quoted 11 lines
drivers/ata/sata_dwc_460ex.c new file +11
-0+static void sata_dwc_port_stop (struct ata_port *ap) +{ +int i; +struct sata_dwc_device *hsdev =HSDEV_FROM_AP (ap); +struct sata_dwc_device_port *hsdevp =HSDEVP_FROM_AP (ap); + +dev_dbg (ap->dev,"%s: ap->id = %d\n" , __func__, ap->print_id); + +if (hsdevp && hsdev) { +/* deallocate LLI table */ +for (i =0 ; i <SATA_DWC_QCMD_MAX ; i++) {
Curly braces not needed.
drivers/ata/sata_dwc_460ex.c
Quoted 3 lines
drivers/ata/sata_dwc_460ex.c new file +3
-0+ dma_free_coherent(ap->host->dev, + SATA_DWC_DMAC_LLI_TBL_SZ, + hsdevp->llit[i],
hsdevp->llit_dma[i]);
Should be indented on the same level as above line
drivers/ata/sata_dwc_460ex.c
Quoted 6 lines
drivers/ata/sata_dwc_460ex.c new file +6
-0+static void sata_dwc_bmdma_setup (struct ata_queued_cmd *qc) +{ +u8 tag = qc->tag; + +if (ata_is_ncq (qc->tf.protocol)) { + dev_dbg(qc->ap->dev, "%s: ap->link.sactive=0x%08x
tag=%d\n",
drivers/ata/sata_dwc_460ex.c line 1387
Quoted 4 lines
drivers/ata/sata_dwc_460ex.c new file +4
-0+ __func__, qc->ap->link.sactive, tag); + }else { + tag =0 ; + }
Curly braces not needed around single statements.
Quoted 1 line
+static void sata_dwc_bmdma_start_by_tag(struct
ata_queued_cmd*qc, u8
tag)
drivers/ata/sata_dwc_460ex.c line 1398
Quoted 7 lines
drivers/ata/sata_dwc_460ex.c new file +7
-0+{ +int start_dma; +u32 reg, dma_chan; +struct sata_dwc_device *hsdev =HSDEV_FROM_QC (qc); +struct ata_port *ap = qc->ap; +struct sata_dwc_device_port *hsdevp =HSDEVP_FROM_AP (ap); +int dir = qc->dma_dir;
There should be an empty line here.
drivers/ata/sata_dwc_460ex.c line 1421
Quoted 3 lines
drivers/ata/sata_dwc_460ex.c new file +3
-0+if (start_dma) { + reg =core_scr_read (SCR_ERROR ); +if (reg &SATA_DWC_SERROR_ERR_BITS ) {
Curly braces not needed here.
drivers/ata/sata_dwc_460ex.c
Quoted 6 lines
drivers/ata/sata_dwc_460ex.c new file +6
-0+static void sata_dwc_bmdma_start (struct ata_queued_cmd *qc) +{ +u8 tag = qc->tag; + +if (ata_is_ncq (qc->tf.protocol)) { + dev_dbg(qc->ap->dev, "%s: ap->link.sactive=0x%08x
tag=%d\n",
drivers/ata/sata_dwc_460ex.c line 1387
Quoted 4 lines
drivers/ata/sata_dwc_460ex.c new file +4
-0+ __func__, qc->ap->link.sactive, tag); + }else { + tag =0 ; + }
Curly braces not needed around single statements.
Quoted 8 lines
drivers/ata/sata_dwc_460ex.c new file +8
-0+/* +* Function : sata_dwc_qc_prep_by_tag +* arguments : ata_queued_cmd *qc, u8 tag +* Return value : None +* qc_prep for a particular queued command based on tag +*/ +static void sata_dwc_qc_prep_by_tag (struct ata_queued_cmd *qc,u8 tag) +{
[...]
drivers/ata/sata_dwc_460ex.c
Quoted 3 lines
drivers/ata/sata_dwc_460ex.c new file +3
-0+ dma_chan = dma_dwc_xfer_setup(sg, qc->n_elem, hsdevp->llit[tag], + hsdevp->llit_dma[tag], + (void
*__iomem)(&hsdev->sata_dwc_regs->\
\ not needed.
Quoted 5 lines
drivers/ata/sata_dwc_460ex.c new file +5
-0+static unsigned int sata_dwc_qc_issue (struct ata_queued_cmd *qc) +{ +u32 sactive; +u8 tag = qc->tag; +struct ata_port *ap = qc->ap;
[...]
drivers/ata/sata_dwc_460ex.c line 1505
Quoted 4 lines
drivers/ata/sata_dwc_460ex.c new file +4
-0+ +if (ata_is_ncq (qc->tf.protocol)) { + sactive =core_scr_read (SCR_ACTIVE ); + sactive |= (0x00000001 << tag);
Parens not needed.
drivers/ata/sata_dwc_460ex.c
Quoted 3 lines
drivers/ata/sata_dwc_460ex.c new file +3
-0+core_scr_write (SCR_ACTIVE , sactive); + + dev_dbg(qc->ap->dev, "%s: tag=%d ap->link.sactive = 0x%08x
"
drivers/ata/sata_dwc_460ex.c
Quoted 1 line
drivers/ata/sata_dwc_460ex.c new file +1
-0+ "sactive=0x%08x\n", __func__, tag,
qc->ap->link.sactive,
drivers/ata/sata_dwc_460ex.c line 1512
Quoted 3 lines
drivers/ata/sata_dwc_460ex.c new file +3
-0+ sactive); + + ap->ops->sff_tf_load (ap, &qc->tf);
Why not call ata_sff_tf_load() directly?
drivers/ata/sata_dwc_460ex.c
Quoted 10 lines
drivers/ata/sata_dwc_460ex.c new file +10
-0+/* +* Function : sata_dwc_qc_prep +* arguments : ata_queued_cmd *qc +* Return value : None +* qc_prep for a particular queued command +*/ + +static void sata_dwc_qc_prep (struct ata_queued_cmd *qc) +{ + if ((qc->dma_dir == DMA_NONE) || (qc->tf.protocol ==
ATA_PROT_PIO))
Parens not needed arounf == operation.
drivers/ata/sata_dwc_460ex.c
Quoted 5 lines
drivers/ata/sata_dwc_460ex.c new file +5
-0+return ; + +#ifdef DEBUG_NCQ +if (qc->tag >0 ) + dev_info(qc->ap->dev, "%s: qc->tag=%d
ap->active_tag=0x%08x\n",
drivers/ata/sata_dwc_460ex.c line 1538
Quoted 3 lines
drivers/ata/sata_dwc_460ex.c new file +3
-0+ __func__, tag, qc->ap->link.active_tag); + +return ;
Remove spade before semicolon please.
drivers/ata/sata_dwc_460ex.c line 1585
Quoted 5 lines
drivers/ata/sata_dwc_460ex.c new file +5
-0+static const struct ata_port_info sata_dwc_port_info[] = { + { + .flags =ATA_FLAG_SATA |ATA_FLAG_NO_LEGACY | +ATA_FLAG_MMIO |ATA_FLAG_NCQ , + .pio_mask =0x1f ,/* pio 0-4 */
Replace 0x1f with ATA_PIO4, please.
drivers/ata/sata_dwc_460ex.c line 1592
Quoted 2 lines
drivers/ata/sata_dwc_460ex.c new file +2
-0+static int sata_dwc_probe (struct of_device *ofdev, +const struct of_device_id *match)
Shouldn't this function be '__init' or '__devinit'?
Quoted 9 lines
drivers/ata/sata_dwc_460ex.c new file +9
-0+{ +struct sata_dwc_device *hsdev; +u32 idr, versionr; +char *ver = (char *)&versionr; +u8 *base =NULL ; +int err =0 ; +int irq, rc; +struct ata_host *host; +struct ata_port_info pi = sata_dwc_port_info[0 ];
drivers/ata/sata_dwc_460ex.c line 1611
Quoted 10 lines
drivers/ata/sata_dwc_460ex.c new file +10
-0+const struct ata_port_info *ppi[] = { &pi,NULL }; + +/* Allocate DWC SATA device */ + hsdev =kmalloc (sizeof (*hsdev),GFP_KERNEL ); +if (hsdev ==NULL ) { +dev_err (&ofdev->dev,"kmalloc failed for hsdev\n" ); + err = -ENOMEM ; + goto error_out; + } +memset (hsdev,0 ,sizeof (*hsdev));
Use kzalloc() instead iof kmalloc()/memset().
drivers/ata/sata_dwc_460ex.c line 1659
Quoted 3 lines
drivers/ata/sata_dwc_460ex.c new file +3
-0+/* Get physical SATA DMA register base address */ + host_pvt.sata_dma_regs =of_iomap (ofdev->dev.of_node,1 ); +if (!(host_pvt.sata_dma_regs)) {
Parens not needed around 'host_pvt.sata_dma_regs'.
drivers/ata/sata_dwc_460ex.c line 1689
Quoted 7 lines
drivers/ata/sata_dwc_460ex.c new file +7
-0+/* +* Now, register with libATA core, this will also initiate the +* device discovery process, invoking our port_start() handler & +* error_handler() to execute a dummy Softreset EH session +*/ + rc =ata_host_activate (host, irq, sata_dwc_isr,0 , &sata_dwc_sht); +
I think that empty line here is not needed.
drivers/ata/sata_dwc_460ex.c line 1701
Quoted 12 lines
drivers/ata/sata_dwc_460ex.c new file +12
-0+if (rc !=0 ) +dev_err (&ofdev->dev,"failed to activate host" ); + +dev_set_drvdata (&ofdev->dev, host); +return 0 ; + +error_out: +/* Free SATA DMA resources */ +dma_dwc_exit (hsdev); + +if (base) +iounmap (base);
What about freeing 'hsdev' and 'host' and unmapping
'host_pvt.sata_dma_regs'? And does iounmap() really pair with of_iomap()?
Quoted 1 line
+static int
sata_dwc_remove(struct of_device *ofdev)
Shouldn't this be '__exit' or '__devexit'?
drivers/ata/sata_dwc_460ex.c line 1717
Quoted 12 lines
drivers/ata/sata_dwc_460ex.c new file +12
-0+{ +struct device *dev = &ofdev->dev; +struct ata_host *host =dev_get_drvdata (dev); +struct sata_dwc_device *hsdev = host->private_data; + +ata_host_detach (host); +dev_set_drvdata (dev,NULL ); + +/* Free SATA DMA resources */ +dma_dwc_exit (hsdev); + +iounmap (hsdev->reg_base);
What about unmapping 'host_pvt.sata_dma_regs'?
drivers/ata/sata_dwc_460ex.c line 1719
Quoted 2 lines
drivers/ata/sata_dwc_460ex.c new file +2
-0+kfree (hsdev); +kfree (host);
Does kfree() really pair with ata_host_alloc_pinfo()?
MBR, Sergei