Hi Naveen,
> +Optional properties:
> + - samsung,hs-mode: Mode of operation, High speed or Fast speed mode. If not
> + specified, default value is 0.
This was probably overlooked from my last review: Why can't you simply
enable hs-mode when clock-frequency is > 1MBit?
> +Example:
> +
> +hsi2c@12ca0000 {
> + compatible = "samsung,exynos5-hsi2c";
> + reg = <0x12ca0000 0x100>;
> + interrupts = <56>;
> + clock-frequency = <100000>;
> +
> + /* Pinctrl variant begins here */
> + pinctrl-0 = <&i2c4_bus>;
> + pinctrl-names = "default";
> + /* Pinctrl variant ends here */
I'd think the two comments above are not needed.
> +/*
> + * exynos5_i2c_wait_bus_idle
> + *
> + * Wait for the transaction to complete (indicated by the TRANS_DONE bit
> + * being set), and, if this is the last message in a transfer, wait for the
> + * MASTER_BUSY bit to be cleared.
> + *
> + * Returns -EBUSY if the bus cannot be bought to idle
s/bought/brought/
> +static int exynos5_i2c_xfer_msg(struct exynos5_i2c *i2c,
> + struct i2c_msg *msgs, int stop)
> +{
> + unsigned long timeout;
> + int ret;
> +
> + i2c->msg = msgs;
> + i2c->msg_ptr = 0;
> + i2c->msg_len = 0;
> + i2c->trans_done = 0;
> +
> + INIT_COMPLETION(i2c->msg_complete);
> +
> + exynos5_i2c_message_start(i2c, stop);
> +
> + ret = wait_for_completion_interruptible_timeout
> + (&i2c->msg_complete, EXYNOS5_I2C_TIMEOUT);
Have you tested with SIGINT? Most drivers removed the _interruptible_
version of waiting since they couldn't get handling the signals proper
and the bus locked up.
> + if (ret >= 0)
> + timeout = ret;
> + else
> + return ret;
> +
> + ret = i2c->state;
> +
> + if ((timeout == 0) || (ret < 0)) {
> + exynos5_i2c_reset(i2c);
> + if (timeout == 0) {
> + dev_warn(i2c->dev, "%s timeout\n",
> + (msgs->flags & I2C_M_RD) ? "rx" : "tx");
> + return ret;
> + } else if (ret == -EAGAIN) {
> + return ret;
> + }
> + }
> +
> + /*
> + * If this is the last message to be transfered (stop == 1)
> + * Then check if the bus can be brought back to idle.
> + *
> + * Return -EBUSY if the bus still busy.
> + */
> + if (exynos5_i2c_wait_bus_idle(i2c, stop))
> + return -EBUSY;
> +
> + /* Return the state as in interrupt routine */
> + return ret;
> +}
> +
> +static int exynos5_i2c_xfer(struct i2c_adapter *adap,
> + struct i2c_msg *msgs, int num)
> +{
> + struct exynos5_i2c *i2c = (struct exynos5_i2c *)adap->algo_data;
> + struct i2c_msg *msgs_ptr = msgs;
> + int retry, i = 0;
> + int ret = 0, ret_pm;
> + int stop = 0;
> +
> + if (i2c->suspended) {
> + dev_err(i2c->dev, "HS-I2C is not initialzed.\n");
> + return -EIO;
> + }
> +
> + ret_pm = pm_runtime_get_sync(i2c->dev);
> + if (IS_ERR_VALUE(ret_pm)) {
> + ret = -EIO;
> + goto out;
> + }
> +
> + clk_prepare_enable(i2c->clk);
> +
> + for (retry = 0; retry < adap->retries; retry++) {
You don't need to retry. The core does it if you return -EAGAIN.
> + for (i = 0; i < num; i++) {
> + stop = (i == num - 1);
> +
> + ret = exynos5_i2c_xfer_msg(i2c, msgs_ptr, stop);
> + msgs_ptr++;
> +
> + if (ret == -EAGAIN) {
> + msgs_ptr = msgs;
> + break;
> + } else if (ret < 0) {
> + goto out;
> + }
> + }
> +
> + if ((i == num) && (ret != -EAGAIN))
> + break;
> +
> + dev_dbg(i2c->dev, "retrying transfer (%d)\n", retry);
> +
> + udelay(100);
> + }
> +
> + if (i == num) {
> + ret = num;
> + } else {
> + /* Only one message, cannot access the device */
> + if (i == 1)
> + ret = -EREMOTEIO;
> + else
> + ret = i;
> +
> + dev_warn(i2c->dev, "xfer message failed\n");
> + }
> +
> + out:
> + clk_disable_unprepare(i2c->clk);
> + pm_runtime_mark_last_busy(i2c->dev);
> + pm_runtime_put_autosuspend(i2c->dev);
> + return ret;
> +}
> +
> +static u32 exynos5_i2c_func(struct i2c_adapter *adap)
> +{
> + return I2C_FUNC_I2C | (I2C_FUNC_SMBUS_EMUL & ~I2C_FUNC_SMBUS_QUICK);
> +}
> +
> +static const struct i2c_algorithm exynos5_i2c_algorithm = {
> + .master_xfer = exynos5_i2c_xfer,
> + .functionality = exynos5_i2c_func,
> +};
> +
> +static int exynos5_i2c_probe(struct platform_device *pdev)
> +{
> + struct device_node *np = pdev->dev.of_node;
> + struct exynos5_i2c *i2c;
> + struct resource *mem;
> + int ret;
> +
> + if (!np) {
> + dev_err(&pdev->dev, "no device node\n");
> + return -ENOENT;
> + }
> +
> + i2c = devm_kzalloc(&pdev->dev, sizeof(struct exynos5_i2c), GFP_KERNEL);
> + if (!i2c) {
> + dev_err(&pdev->dev, "no memory for state\n");
> + return -ENOMEM;
> + }
> +
> + /* Mode of operation High/Fast Speed mode */
> + if (of_get_property(np, "samsung,hs-mode", NULL)) {
> + i2c->speed_mode = HSI2C_HIGH_SPD;
> + i2c->fs_clock = HSI2C_FS_TX_CLOCK;
> + if (of_property_read_u32(np, "clock-frequency", &i2c->hs_clock))
> + i2c->hs_clock = HSI2C_HS_TX_CLOCK;
> + } else {
> + i2c->speed_mode = HSI2C_FAST_SPD;
> + if (of_property_read_u32(np, "clock-frequency", &i2c->fs_clock))
> + i2c->fs_clock = HSI2C_FS_TX_CLOCK;
> + }
> +
> + strlcpy(i2c->adap.name, "exynos5-i2c", sizeof(i2c->adap.name));
> + i2c->adap.owner = THIS_MODULE;
> + i2c->adap.algo = &exynos5_i2c_algorithm;
> + i2c->adap.retries = 2;
> + i2c->adap.class = I2C_CLASS_HWMON | I2C_CLASS_SPD;
Don't use .class unless you have an explicit reason to do so. It may
cost boot-time.
> +
> + i2c->dev = &pdev->dev;
> + i2c->clk = devm_clk_get(&pdev->dev, "hsi2c");
> + if (IS_ERR(i2c->clk)) {
> + dev_err(&pdev->dev, "cannot get clock\n");
> + return -ENOENT;
> + }
> +
> + clk_prepare_enable(i2c->clk);
> +
> + mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + i2c->regs = devm_ioremap_resource(&pdev->dev, mem);
> + if (IS_ERR(i2c->regs)) {
> + dev_err(&pdev->dev, "cannot map HS-I2C IO\n");
devm_ioremap_resource will print the error message for you.
> + ret = PTR_ERR(i2c->regs);
> + goto err_clk;
> + }
> +
> + i2c->adap.dev.of_node = np;
> + i2c->adap.algo_data = i2c;
> + i2c->adap.dev.parent = &pdev->dev;
> +
> + /* Clear pending interrupts from u-boot or misc causes */
> + exynos5_i2c_clr_pend_irq(i2c);
> +
> + init_completion(&i2c->msg_complete);
> +
> + i2c->irq = ret = irq_of_parse_and_map(np, 0);
> + if (ret <= 0) {
> + dev_err(&pdev->dev, "cannot find HS-I2C IRQ\n");
> + ret = -EINVAL;
> + goto err_clk;
> + }
> +
> + ret = devm_request_irq(&pdev->dev, i2c->irq, exynos5_i2c_irq,
> + 0, dev_name(&pdev->dev), i2c);
> +
> + if (ret != 0) {
> + dev_err(&pdev->dev, "cannot request HS-I2C IRQ %d\n", i2c->irq);
> + goto err_clk;
> + }
> +
> + /*
> + * TODO: Use private lock to avoid race conditions as
> + * mentioned in pm_runtime.txt
> + */
Is this planned somewhen?
> + pm_runtime_enable(i2c->dev);
> + pm_runtime_set_autosuspend_delay(i2c->dev, EXYNOS5_I2C_PM_TIMEOUT);
> + pm_runtime_use_autosuspend(i2c->dev);
> +
> + ret = pm_runtime_get_sync(i2c->dev);
> + if (IS_ERR_VALUE(ret))
> + goto err_clk;
> +
> + ret = exynos5_hsi2c_clock_setup(i2c);
> + if (ret)
> + goto err_pm;
> +
> + i2c->bus_id = of_alias_get_id(i2c->adap.dev.of_node, "hsi2c");
> +
> + exynos5_i2c_init(i2c);
> +
> + i2c->adap.nr = -1;
> + ret = i2c_add_numbered_adapter(&i2c->adap);
Skip setting -1 and use i2c_add_adapter.
> + if (ret < 0) {
> + dev_err(&pdev->dev, "failed to add bus to i2c core\n");
> + goto err_pm;
> + }
> +
> + of_i2c_register_devices(&i2c->adap);
> + platform_set_drvdata(pdev, i2c);
> +
> + clk_disable_unprepare(i2c->clk);
> + pm_runtime_mark_last_busy(i2c->dev);
> + pm_runtime_put_autosuspend(i2c->dev);
> +
> + return 0;
> +
> + err_pm:
> + pm_runtime_put(i2c->dev);
> + pm_runtime_disable(&pdev->dev);
> + err_clk:
> + clk_disable_unprepare(i2c->clk);
> + return ret;
> +}
...
> +
> +static struct platform_driver exynos5_i2c_driver = {
> + .probe = exynos5_i2c_probe,
> + .remove = exynos5_i2c_remove,
> + .driver = {
> + .owner = THIS_MODULE,
> + .name = "exynos5-hsi2c",
> + .pm = EXYNOS5_DEV_PM_OPS,
> + .of_match_table = exynos5_i2c_match,
> + },
> +};
> +
> +static int __init i2c_adap_exynos5_init(void)
> +{
> + return platform_driver_register(&exynos5_i2c_driver);
> +}
> +subsys_initcall(i2c_adap_exynos5_init);
> +
> +static void __exit i2c_adap_exynos5_exit(void)
> +{
> + platform_driver_unregister(&exynos5_i2c_driver);
> +}
> +module_exit(i2c_adap_exynos5_exit);
Use the module_platform_driver macro, please.
Thanks for keeping at it!
Wolfram
Attachment:
signature.asc
Description: Digital signature
