Re: [PATCH v10 3/8] i2c: thunderx: Add i2c driver for ThunderX SOC

From: Wolfram Sang
Date: Tue Aug 23 2016 - 16:36:47 EST



> 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@xxxxxxxxxxxxxxxxxx>");
> +MODULE_DESCRIPTION("I2C-Bus adapter for Cavium ThunderX SOC");
> --
> 1.9.1
>

Attachment: signature.asc
Description: PGP signature