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:sshtylyov@xxxxxxxxxx] Sent: Saturday, July 17, 2010 9:21 PM To: Rupjyoti Sarmah Cc: linux-ide@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; rsarmah@xxxxxxx; jgarzik@xxxxxxxxx; sr@xxxxxxx; linuxppc-dev@xxxxxxxxxx Subject: Re: [PATCH v2]460EX on-chip SATA driver<resubmisison> Hello. Rupjyoti Sarmah wrote: > 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: > Signed-off-by: Rupjyoti Sarmah <rsarmah@xxxxxxxxxxxxxxxx> > Signed-off-by: Mark Miesfeld <mmiesfeld@xxxxxxxxxxxxxxxx> > Signed-off-by: Prodyut Hazarika <phazarika@xxxxxxxxxxxxxxxx> [...] > diff --git a/drivers/ata/sata_dwc_460ex.c b/drivers/ata/sata_dwc_460ex.c > new file mode 100644 > index 0000000..ea24c1e > --- /dev/null > +++ b/drivers/ata/sata_dwc_460ex.c > @@ -0,0 +1,1756 @@ > +/* > + * drivers/ata/sata_dwc_460ex.c Filenames in the heading comments have long been frowned upon. > +#ifdef CONFIG_SATA_DWC_DEBUG I don't see this option defined anywahere. > +#define DEBUG > +#endif > + > +#ifdef CONFIG_SATA_DWC_VDEBUG The same about this one. > +#define VERBOSE_DEBUG > +#define DEBUG_NCQ > +#endif [...] > +/* 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 */. > +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. > +#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? > +/* > + * 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)'? > +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. > +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. > +/* > + * 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) > + 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()... > +/* > + * 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; > + 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. > + .low)); > + err_reg = in_le32(&(host_pvt.sata_dma_regs->interrupt_status.error\ Same comment here... > + 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); > + > + 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. > + sata_dwc_dma_xfer_complete(ap, 1); > + > + /* Clear the interrupt */ > + out_le32(&(host_pvt.sata_dma_regs->interrupt_clear\ \ not needed. > + /* Clear the interrupt. */ > + out_le32(&(host_pvt.sata_dma_regs->interrupt_clear\ \ not needed. > +/* > + * 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) > +{ [...] > + /* Program the LLI CTL high register */ > + lli[idx].ctl.high = cpu_to_le32(DMA_CTL_BLK_TS\ \ not needed. > + (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... > +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. > +/* > + * 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... > + 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. > +static u32 core_scr_read(unsigned int scr) > +{ > + return in_le32((void __iomem *)(host_pvt.scr_addr_sstatus) +\ Not needed. > +static void clear_serror(void) > +{ > + u32 val; Empty line needed here. > + val = core_scr_read(SCR_ERROR); This could be an initilaizer. > +/* 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; > + 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? > + err_reg = in_le32(&(host_pvt.sata_dma_regs->interrupt_status.error.\ \ not neeeded. > +/* > + * 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; [...] > + 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'? > + /* 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... > + handled = 1; > + goto DONE; > + } > + status = ap->ops->sff_check_status(ap); Here too... > +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 > + * 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 thru scripts/checkpatch.pl? > + /* > + * 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'? > + if (sactive != 0 || (host_pvt.sata_dwc_sactive_issued) > 1 || \ Useless parens around 'host_pvt.sata_dwc_sactive_issued' + \ not needed. > + 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. > + if ((tag_mask | (host_pvt.sata_dwc_sactive_issued)) != \ > + (host_pvt.sata_dwc_sactive_issued)) { Useless parens around 'host_pvt.sata_dwc_sactive_issued'. > + 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. > + > + /* read just to clear ... not bad if currently still busy */ > + status = ap->ops->sff_check_status(ap); Call ata_sff_check_status() directly. > + 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. > + /* 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. > + 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. > + 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; } > + /* > + * 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. > +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; > + > + 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. > + 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. > +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. > + 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. > + 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. > +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. > + 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 > +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. > +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. > + if (start_dma) { > + reg = core_scr_read(SCR_ERROR); > + if (reg & SATA_DWC_SERROR_ERR_BITS) { Curly braces not needed here. > +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. > +/* > + * 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) > +{ [...] > + 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. > +static unsigned int sata_dwc_qc_issue(struct ata_queued_cmd *qc) > +{ > + u32 sactive; > + u8 tag = qc->tag; > + struct ata_port *ap = qc->ap; [...] > + > + if (ata_is_ncq(qc->tf.protocol)) { > + sactive = core_scr_read(SCR_ACTIVE); > + sactive |= (0x00000001 << tag); Parens not needed. > + 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? > +/* > + * 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. > + 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. > +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. > +static int sata_dwc_probe(struct of_device *ofdev, > + const struct of_device_id *match) Shouldn't this function be '__init' or '__devinit'? > +{ > + 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]; > + 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(). > + /* 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'. > + /* > + * 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. > + 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()? > +static int sata_dwc_remove(struct of_device *ofdev) Shouldn't this be '__exit' or '__devexit'? > +{ > + 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'? > + kfree(hsdev); > + kfree(host); Does kfree() really pair with ata_host_alloc_pinfo()? MBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
