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
