Re: [PATCH v2] pch ieee1588 driver for Intel EG20T PCH
flat view
From: Joe Perches <joe@perches.com>
Date: 2011-08-10 13:40:46
Also in:
lkml
On Wed, 2011-08-10 at 15:13 +0900, Toshiharu Okada wrote:
This patch is for IEEE1588 driver of Intel EG20T PCH. EG20T PCH is the platform controller hub that is used in Intel's general embedded platform. This driver adds support for using the EG20T PCH as a PTP clock.
Would you review this patch, although this driver has not been tested yet?
Just trivia...
quoted hunk ↗ jump to hunk
diff --git a/drivers/ptp/ptp_pch.c b/drivers/ptp/ptp_pch.c
[]
+/** + * struct ch_1588_params_ - 1588 module paramter
parameter
quoted hunk ↗ jump to hunk
+ */ +struct pch_params_ { + u8 station[STATION_ADDR_LEN]; +};
names for struct types ending in _ are pretty unusual. []
quoted hunk ↗ jump to hunk
+/** + * get_decimal() - Returns the decimal value of the passed hexadecimal value + * @ch: The hexadecimal value that has to be converted. + */ +static s32 get_decimal(u8 ch) +{ + s32 ret; + + if ((ch >= '0') && (ch <= '9')) + ret = ch - '0'; + else if ((ch >= 'A') && (ch <= 'F')) + ret = 10 + ch - 'A'; + else if ((ch >= 'a') && (ch <= 'f')) + ret = 10 + ch - 'a'; + else + return -1; + + return ret; +}
hex_to_bin
+pch_probe(struct pci_dev *pdev, const struct pci_device_id *id)
[]
quoted hunk ↗ jump to hunk
+ if (ret != 0) { + dev_err(&pdev->dev, + "%s:could not enable the pci device\n", __func__); + goto err_pci_en; + }
It seems all of the dev_<level> logging messages include __func__. I think these __func__ uses are not particularly valuable and could be removed.
+ /* retreive the available length of the IO memory space */
retrieve
quoted hunk ↗ jump to hunk
+ if (!request_mem_region(chip->mem_base, chip->mem_size, "1588_regs")) { + dev_err(&pdev->dev, "%s: could not allocate register memory " + "space\n", __func__);
Please don't split format strings. dev_err(&pdev->dev, "could not allocate register memory space\n"); or if you must dev_err(&pdev->dev, "%s: could not allocate register memory space\n", __func__); []
quoted hunk ↗ jump to hunk
+ if (strcmp(pch_param.station, "00:00:00:00:00:00") != 0) { + if (pch_set_station_address(pch_param.station, pdev) != 0) { + dev_err(&pdev->dev, + "%s: Invalid station address parameter\n" + "Module loaded; But, station address not set " + "correctly\n", __func__);
dev_err(&pdev->dev, "%s: Invalid station address parameter\n" "Module loaded but station address not set correctly\n", __func__);