Re: [PATCH] EG20T: Update PCH_I2C driver to 2.6.36
From: Tomoya MORINAGA
Date: Mon Dec 06 2010 - 20:39:08 EST
On Monday, December 06, 2010 1:27 PM, Ben Dooks wrote:
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index c3ef492..e55e051 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -60,6 +60,7 @@ obj-$(CONFIG_I2C_STU300) += i2c-stu300.o
obj-$(CONFIG_I2C_VERSATILE) += i2c-versatile.o
obj-$(CONFIG_I2C_OCTEON) += i2c-octeon.o
obj-$(CONFIG_I2C_XILINX) += i2c-xiic.o
+obj-$(CONFIG_I2C_EG20T) += i2c-eg20t.o
Jean, do you want these alphabetically sorted?
I will modify.
+ * struct adapter_info - This structure holds the adapter information
for the
+ PCH i2c controller
+ * @pch_data: stores a list of i2c_algo_pch_data
+ * @pch_i2c_suspended: specifies whether the system is suspended or not
+ * perhaps with more lines and words.
you don't need the 'or not' and the extra line does not make sense?
I will delete "or not" description.
+static int pch_i2c_speed = 100; /* I2C bus speed in Kbps */
+static int pch_clk = 50000; /* specifies I2C clock speed in KHz */
+static wait_queue_head_t pch_event;
+static DEFINE_MUTEX(pch_mutex);
+
+static struct pci_device_id __devinitdata pch_pcidev_id[] = {
+ {PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_PCH_I2C)},
+ {0,}
please put single space after the { and before the }.
I will put single space.
+static inline void pch_clrbit(void __iomem *addr, u32 offset, u32
bitmask)
+{
+ u32 val;
+ val = ioread32(addr + offset);
+ val &= (~bitmask);
the () around bitmask is useless.
I will delete.
+static inline bool ktime_lt(const ktime_t cmp1, const ktime_t cmp2)
+{
+ return cmp1.tv64 < cmp2.tv64;
+}
hmm, surely this sort of thing should be in a header somewhere?
I will move this function to head of file.
+static s32 pch_i2c_getack(struct i2c_algo_pch_data *adap)
+{
+ u32 reg_val;
+ void __iomem *p = adap->pch_base_address;
would prefer a blank line here
also prefernece for longer items first in the list
neither of these is really important though.
I will add blank line.
+ reg_val = ioread32(p + PCH_I2CSR) & PCH_GETACK;
+
+ if (reg_val != 0) {
would (reg_val & PCH_GETACK) do here?
I will do so.
+static s32 pch_i2c_writebytes(struct i2c_adapter *i2c_adap,
+ struct i2c_msg *msgs, u32 last, u32 first)
+{
+ struct i2c_algo_pch_data *adap = i2c_adap->algo_data;
+ u8 *buf;
+ u32 length;
+ u32 addr;
+ u32 addr_2_msb;
+ u32 addr_8_lsb;
+ s32 wrcount;
+ void __iomem *p = adap->pch_base_address;
+ length = msgs->len;
+ buf = msgs->buf;
+ addr = msgs->addr;
minor, these could have been merged with the declaration.
I will merge.
+ if (pch_i2c_wait_for_xfer_complete(adap) == 0 &&
+ pch_i2c_getack(adap) == 0) {
+ addr_8_lsb = (addr & I2C_ADDR_MSK);
+ iowrite32(addr_8_lsb, p + PCH_I2CDR);
+ } else {
+ pch_i2c_stop(adap);
+ return -ETIME;
I'll have to check the error codes here, thought ETIMEDOUT would be
better, or -EBUSY.
I will replace to ETIMEDOUT.
+ }
+
+ if ((pch_i2c_wait_for_xfer_complete(adap) == 0) &&
+ (pch_i2c_getack(adap) == 0)) {
+ for (wrcount = 0; wrcount < length; ++wrcount) {
+ /* write buffer value to I2C data register */
+ iowrite32(buf[wrcount], p + PCH_I2CDR);
+ pch_dbg(adap, "writing %x to Data register\n",
+ buf[wrcount]);
+
+ if (pch_i2c_wait_for_xfer_complete(adap) != 0)
+ return -ETIME;
see above.
I will replace to ETIMEDOUT.
+ /* Dummy read */
+ for (loop = 1, read_index = 0; loop < length; loop++) {
+ buf[read_index] = ioread32(p + PCH_I2CDR);
+
+ if (loop != 1)
+ read_index++;
+
+ if (pch_i2c_wait_for_xfer_complete(adap) != 0) {
+ pch_i2c_stop(adap);
+ return -ETIME;
don't think this is the correct return code here either..
I will replace to ETIMEDOUT.
+static void pch_i2c_disbl_int(struct i2c_algo_pch_data *adap)
+{
+ void __iomem *p = adap->pch_base_address;
+
+ pch_clrbit(adap->pch_base_address, PCH_I2CCTL, NORMAL_INTR_ENBL);
+
+ iowrite32(EEPROM_RST_INTR_DISBL, p + PCH_I2CESRMSK);
+
could avoid blank here.
I will delete the blank.
+ iowrite32(BUFFER_MODE_INTR_DISBL, p + PCH_I2CBUFMSK);
+}
+
+static int __devinit pch_i2c_probe(struct pci_dev *pdev,
+ const struct pci_device_id *id)
+{
+ void __iomem *base_addr;
+ s32 ret;
+ struct adapter_info *adap_info;
+
+ pch_pci_dbg(pdev, "Entered.\n");
+
+ adap_info = kzalloc((sizeof(struct adapter_info)), GFP_KERNEL);
extra () in here
I will delete.
+ if (adap_info == NULL) {
+ pch_pci_err(pdev, "Memory allocation FAILED\n");
+ return -ENOMEM;
+ }
do not need to capitalise 'FAILED'
I will replace "FAILED" to "failed"
+
+ ret = pci_enable_device(pdev);
+ if (ret) {
+ pch_pci_err(pdev, "pci_enable_device FAILED\n");
+ goto err_pci_enable;
+ }
() after pci_device_enable... also see previous.
Sorry, I couldn't understand your saying.
Let me know more in detail.
+
+ ret = pci_request_regions(pdev, KBUILD_MODNAME);
how about using dev_name() in case of multiple devices.
I will use dev_name instead of KBUILD_MODNAME.
BTW, this is my interested question,
why do you recommend use dev_name instead of KBUILD_MODNAME ?
+
+ adap_info->pch_i2c_suspended = false;
+
+ adap_info->pch_data.p_adapter_info = adap_info;
+
+ adap_info->pch_data.pch_adapter.owner = THIS_MODULE;
+ adap_info->pch_data.pch_adapter.class = I2C_CLASS_HWMON;
+ strcpy(adap_info->pch_data.pch_adapter.name, KBUILD_MODNAME);
please use a string copy that limits transfer size....
Do you mean we should use strncpy instead of strcpy ?
+ ret = i2c_add_adapter(&(adap_info->pch_data.pch_adapter));
+
+ if (ret) {
+ pch_pci_err(pdev, "i2c_add_adapter FAILED\n");
+ goto err_i2c_add_adapter;
+ }
+
+ pch_i2c_init(&adap_info->pch_data);
+ ret = request_irq(pdev->irq, pch_i2c_handler, IRQF_SHARED,
+ KBUILD_MODNAME, &adap_info->pch_data);
see note on dev_name()
I will replace to dev_name.
+static void __devexit pch_i2c_remove(struct pci_dev *pdev)
+{
+ struct adapter_info *adap_info = pci_get_drvdata(pdev);
+
+ pch_i2c_disbl_int(&adap_info->pch_data);
+ free_irq(pdev->irq, &adap_info->pch_data);
+ i2c_del_adapter(&(adap_info->pch_data.pch_adapter));
+
+ if (adap_info->pch_data.pch_base_address) {
+ pci_iounmap(pdev, adap_info->pch_data.pch_base_address);
+ adap_info->pch_data.pch_base_address = 0;
NULL, not 0
I will replace to "NULL".
+static struct pci_driver pch_pcidriver = {
+ .name = KBUILD_MODNAME,
+ .id_table = pch_pcidev_id,
+ .probe = pch_i2c_probe,
+ .remove = __devexit_p(pch_i2c_remove),
+ .suspend = pch_i2c_suspend,
+ .resume = pch_i2c_resume
runtime PM would be better.
Sorry, I couldn't understand your saying.
Let me know more in detail.
Thanks,
/_/_/_/_/_/_/_/_/_/_/_/_/_/_/_/_
Tomoya Morinaga
OKI SEMICONDUCTOR CO., LTD.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/