Re: [PATCH] i2c-designware: add i2c gpio recovery option

[Andy Shevchenko]

On Wed, 2017-05-10 at 13:57 +0200, Tim Sander wrote:
> This patch contains much input from Phil Reid and has been tested
> on Intel/Altera Cyclone V SOC Hardware with Altera GPIO's for theÂ
> SCL and SDA GPIO's. I am still a little unsure about the recover
> in the timeout case (i2c-designware-core.c:770) as i could not
> test this codepath.

Since it's not an RFC anymore let me do some comments on the below.

> @@ -317,6 +317,7 @@ static void i2c_dw_release_lock(struct dw_i2c_dev
> *dev)
> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂdev->release_lock(dev);
> Â}
> Â
> +
> Â/**
> Â * i2c_dw_init() - initialize the designware i2c master hardware
> Â * @dev: device private data

This doesn't belong to the change.

> @@ -463,7 +464,12 @@ static int i2c_dw_wait_bus_not_busy(struct
> dw_i2c_dev *dev)
> ÂÂÂÂÂÂÂÂwhile (dw_readl(dev, DW_IC_STATUS) & DW_IC_STATUS_ACTIVITY) {

> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂif (timeout <= 0) {
> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂdev_warn(dev->dev, "timeout waiting for bus
> ready\n");
> -ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturn -ETIMEDOUT;
> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂi2c_recover_bus(&dev->adapter);
> +
> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂif (dw_readl(dev, DW_IC_STATUS) &
> DW_IC_STATUS_ACTIVITY)
> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturn -ETIMEDOUT;

> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂelse

Redundant.

> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturn 0;
> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ}

Actually I would rather refactor first above function: 
1) to be do {} while();
2) to have invariant condition out of the loop.

> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂtimeout--;
> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂusleep_range(1000, 1100);

> @@ -719,9 +725,10 @@ static int i2c_dw_handle_tx_abort(struct
> dw_i2c_dev *dev)
> ÂÂÂÂÂÂÂÂfor_each_set_bit(i, &abort_source, ARRAY_SIZE(abort_sources))
> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂdev_err(dev->dev, "%s: %s\n", __func__,
> abort_sources[i]);
> Â
> -ÂÂÂÂÂÂÂif (abort_source & DW_IC_TX_ARB_LOST)
> +ÂÂÂÂÂÂÂif (abort_source & DW_IC_TX_ARB_LOST) {
> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂi2c_recover_bus(&dev->adapter);
> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturn -EAGAIN;

> -ÂÂÂÂÂÂÂelse if (abort_source & DW_IC_TX_ABRT_GCALL_READ)
> +ÂÂÂÂÂÂÂ} else if (abort_source & DW_IC_TX_ABRT_GCALL_READ)
> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturn -EINVAL; /* wrong msgs[] data */
> ÂÂÂÂÂÂÂÂelse

Both else:s are redundant.

	if (abort_source & DW_IC_TX_ARB_LOST) {
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂi2c_recover_bus(&dev->adapter);
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturn -EAGAIN;
	}

ÂÂÂÂÂÂÂ if (abort_source & DW_IC_TX_ABRT_GCALL_READ)
...

Though I may agree on leaving them here for sake of keeping less lines
of code. 

> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturn -EIO;

> +#include <linux/gpio.h>

I think it should be

#include <linux/gpio/consumer.h>

> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> @@ -41,6 +41,7 @@
> Â#include <linux/reset.h>
> Â#include <linux/slab.h>
> Â#include <linux/acpi.h>

> +#include <linux/of_gpio.h>

No, please don't.

In recent code we try to avoid OF/ACPI/platform specific bits if there
is a common resource provider (and API) for that. GPIO is the case.Â

> +void i2c_dw_unprepare_recovery(struct i2c_adapter *adap)
> +{

> +}
> +
> +

Remove extra line.

> +static int i2c_dw_get_scl(struct i2c_adapter *adap)
> +{
> +ÂÂÂÂÂÂÂstruct platform_device *pdev = to_platform_device(&adap->dev);
> +ÂÂÂÂÂÂÂstruct dw_i2c_dev *dev = platform_get_drvdata(pdev);

struct dw_i2c_dev *dev = i2c_get_adapdata(adap); ?

Ditto for all occurrences in the code.

> +
> +ÂÂÂÂÂÂÂreturn gpiod_get_value_cansleep(dev->gpio_scl);
> +}

> +static int i2c_dw_init_recovery_info(struct dw_i2c_dev *dev,
> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂstruct i2c_adapter *adap)
> +{
> +ÂÂÂÂÂÂÂstruct i2c_bus_recovery_info *rinfo = &dev->rinfo;
> +
> +ÂÂÂÂÂÂÂdev->gpio_scl = devm_gpiod_get_optional(dev->dev,
> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ"scl",
> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂGPIOD_OUT_HIGH);

> +ÂÂÂÂÂÂÂif (IS_ERR_OR_NULL(dev->gpio_scl))

This is wrong. You should not use this macro in most cases. And
especially it breaks the logic behind _optional().

> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturn PTR_ERR(dev->gpio_scl);
> +
> +ÂÂÂÂÂÂÂdev->gpio_sda = devm_gpiod_get_optional(dev->dev, "sda",
> GPIOD_IN);

> +ÂÂÂÂÂÂÂif (IS_ERR_OR_NULL(dev->gpio_sda))

Ditto.

> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturn PTR_ERR(dev->gpio_sda);

> +ÂÂÂÂÂÂÂrinfo->scl_gpio = desc_to_gpio(dev->gpio_scl);
> +ÂÂÂÂÂÂÂrinfo->sda_gpio = desc_to_gpio(dev->gpio_sda);

Why?!

> +};

> @@ -285,6 +368,7 @@ static int dw_i2c_plat_probe(struct
> platform_device *pdev)
> ÂÂÂÂÂÂÂÂadap->class = I2C_CLASS_DEPRECATED;
> ÂÂÂÂÂÂÂÂACPI_COMPANION_SET(&adap->dev, ACPI_COMPANION(&pdev->dev));
> ÂÂÂÂÂÂÂÂadap->dev.of_node = pdev->dev.of_node;
> +ÂÂÂÂÂÂÂsnprintf(adap->name, sizeof(adap->name), "Designware i2c
> adapter");

It looks like a separate change.

> Â
> +ÂÂÂÂÂÂÂr = i2c_dw_init_recovery_info(dev, adap);
> +ÂÂÂÂÂÂÂif (rÂÂ== -EPROBE_DEFER)

Remove extra space.

> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂgoto exit_probe;
> +
> ÂÂÂÂÂÂÂÂr = i2c_dw_probe(dev);
> ÂÂÂÂÂÂÂÂif (r)
> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂgoto exit_probe;
> Â

> -ÂÂÂÂÂÂÂreturn r;
> +ÂÂÂÂÂÂÂreturn 0;

Doesn't belong to the change.

Don't change arbitrary typos or do small "improvements" in the change
which is not about them.

-- 
Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
Intel Finland Oy