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.
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.
- 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);
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 -- 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