Re: [PATCH] ata_piix: minor typo fixes and threading fix

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



2013-09-22 16:44 keltezéssel, Sergei Shtylyov írta:
Hello.

On 22.09.2013 17:53, Levente Kurusa wrote:

Hi,

The following patch fixes a printk() call, which was originally used with
pr_cont, which will however fail. Fixed that with setting up a buffer
to save
data to that first, and then printk() it. The patch also
fixes some minor typos and a comment, so that it better reflects the
documentation of ICH*.

    Wrap your changelog lines at 80 columns (preferably less).

Regards,
Levente Kurusa

    The patch changelog is not a place for greetings and regards.
Sorry for that.

Signed-off-by: Levente Kurusa <levex@xxxxxxxxx>
---
diff --git a/drivers/ata/ata_piix.c b/drivers/ata/ata_piix.c
index 93cb092..b7bf3df 100644
--- a/drivers/ata/ata_piix.c
+++ b/drivers/ata/ata_piix.c
@@ -100,7 +100,7 @@

  enum {

    Your patch is whitespace damaged, there was no space on previous
line, and there are two spaces starting this line (instead of one).

@@ -1368,34 +1368,53 @@ static const int *piix_init_sata_map(struct
pci_dev
*pdev,
      pci_read_config_byte(pdev, ICH5_PMR, &map_value);

      map = map_db->map[map_value & map_db->mask];
-
-    dev_info(&pdev->dev, "MAP [");
+    char* mapdata[4];

    Don't mix declarations and data.

      for (i = 0; i < 4; i++) {
          switch (map[i]) {
          case RV:
              invalid_map = 1;
-            pr_cont(" XX");
+            mapdata[i] =" XX";
              break;

          case NA:
-            pr_cont(" --");
+            mapdata[i] = " --";
              break;

          case IDE:
              WARN_ON((i & 1) || map[i + 1] != IDE);
              pinfo[i / 2] = piix_port_info[ich_pata_100];
              i++;
-            pr_cont(" IDE IDE");
+            mapdata[i] = " IDE IDE";
+            break;
+        case P0:
+            mapdata[i] = " P0";
+            if (i & 1)
+                pinfo[i / 2].flags |= ATA_FLAG_SLAVE_POSS;
+            break;
+        case P1:
+            mapdata[i] = " P1";
+            if (i & 1)
+                pinfo[i / 2].flags |= ATA_FLAG_SLAVE_POSS;
+            break;
+        case P2:
+            mapdata[i] = " P2";
+            if (i & 1)
+                pinfo[i / 2].flags |= ATA_FLAG_SLAVE_POSS;
+            break;
+        case P3:
+            mapdata[i] = " P3";
+            pinfo[i / 2].flags |= ATA_FLAG_SLAVE_POSS;
+            if (i & 1)
+                pinfo[i / 2].flags |= ATA_FLAG_SLAVE_POSS;
              break;

    I don't think the code repeating 4 times is a good idea.
I did this to prevent a goto, but changing that back then.

-
          default:
-            pr_cont(" P%d", map[i]);
+            mapdata[i] = " INV";
              if (i & 1)
                  pinfo[i / 2].flags |= ATA_FLAG_SLAVE_POSS;
              break;
          }
      }
-    pr_cont(" ]\n");
+    dev_info(&pdev->dev, "MAP [%s%s%s%s ]\n", mapdata[0], mapdata[1],
mapdata[2], mapdata[3], map_value, map_db->mask);
That was from my former debug mechanism, overlooked that. Sorry.

    You didn't specify the formats for the last 2 arguments and your
line is longer than 80 columns, please wrap it.

@@ -1718,18 +1741,17 @@ static int piix_init_one(struct pci_dev *pdev,
const
struct pci_device_id *ent)
          if (host->ports[0]->ops == &piix_sidpr_sata_ops)
              sht = &piix_sidpr_sht;
      }
-

    Why?

      /* apply IOCFG bit18 quirk */
      piix_iocfg_bit18_quirk(host);

-    /* On ICH5, some BIOSen disable the interrupt using the
+    /* On ICH5, some BIOSes disable the interrupt using the

    Why? "BIOSen" is not a typo, it's a jargon.

WBR, Sergei


Thank you for all your comments, I am not
a native writer of english, and hence I didn't have knowledge
of that jargon. I am fixing the issues, as per checkpatch.pl and
your guidelines. Once finished, I will repost it.

--
Regards,
Levente Kurusa
--
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




[Index of Archives]     [Linux Filesystems]     [Linux SCSI]     [Linux RAID]     [Git]     [Kernel Newbies]     [Linux Newbie]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Samba]     [Device Mapper]

  Powered by Linux