Messages in this thread | | | Date | Tue, 23 Aug 2016 22:36:29 +0200 | From | Wolfram Sang <> | Subject | Re: [PATCH v10 3/8] i2c: thunderx: Add i2c driver for ThunderX SOC |
| |
> i2c-octeon-objs := i2c-cavium.o i2c-octeon-core.o > obj-$(CONFIG_I2C_OCTEON) += i2c-octeon.o > +i2c-thunderx-objs := i2c-cavium.o i2c-thunderx-core.o > +obj-$(CONFIG_I2C_THUNDERX) += i2c-thunderx.o
Shouldn't that rather be "i2c-cavium-core.o", "i2c-octeon-platdrv.o", and "i2c-thunderx-pcidrv.o" for the -objs? I mean, the cavium part is the core-part...
> +skip: > + if (!i2c->sys_freq) > + i2c->sys_freq = SYS_FREQ_DEFAULT; > + > + dev_info(dev, "Set system clock to %u\n", i2c->sys_freq);
Too many dev_info IMO, see later.
> + i2c->adap.class = I2C_CLASS_HWMON | I2C_CLASS_SPD;
Do you really need that?
> + ret = i2c_add_adapter(&i2c->adap); > + if (ret < 0) { > + dev_err(dev, "Failed to add i2c adapter\n");
I2C core does error reporting for you meanwhile.
> + goto out_irq; > + } > + > + dev_info(i2c->dev, "probed\n");
I'd think all nice to know dev_info should go here in condensed form.
> +out_irq: > + devm_free_irq(dev, i2c->i2c_msix.vector, i2c);
If you need to free irq manually here anyhow, then you don't need the devm variant.
> +out_msix: > + pci_disable_msix(pdev); > +out_unmap: > + iounmap(i2c->twsi_base); > + thunder_i2c_clock_disable(dev, i2c->clk); > +out_release_regions: > + pci_release_regions(pdev); > +out_disable_device: > + pci_disable_device(pdev); > +out_free_i2c: > + pci_set_drvdata(pdev, NULL);
Similar to devm_*, there is also pcim_* for PCI devices. You might want to have a look for those.
> + devm_kfree(dev, i2c);
Not needed.
> + return ret; > +} > + > +static void thunder_i2c_remove_pci(struct pci_dev *pdev) > +{ > + struct octeon_i2c *i2c = pci_get_drvdata(pdev); > + struct device *dev; > + > + if (!i2c) > + return;
How should that happen?
> + > +module_pci_driver(thunder_i2c_pci_driver); > + > +MODULE_LICENSE("GPL");
Please add a minimal GPL header at the top of the file, so it is clear if you mean "v2" or "v2 or later".
> +MODULE_AUTHOR("Fred Martin <fmartin@caviumnetworks.com>"); > +MODULE_DESCRIPTION("I2C-Bus adapter for Cavium ThunderX SOC"); > -- > 1.9.1 > [unhandled content-type:application/pgp-signature]
| |