Re: [PATCH] EG20T: Update PCH_I2C driver to 2.6.36

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

 



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-i2c" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [Linux GPIO]     [Linux SPI]     [Linux Hardward Monitoring]     [LM Sensors]     [Linux USB Devel]     [Linux Media]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux