| Message ID | 32490844.DoY2McpBxU@dabox |
|---|---|
| State | Changes Requested |
| Headers | show |
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.
G'day Andy, Thanks for the review. On 10/05/2017 21:13, Andy Shevchenko wrote: > 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(). My logic here was that if the gpio is optional return null we return 0. which is an okay status. But this breaks if !CONFIG_GPIOLIB, which I keep forgetting. I've never quite wrapped my head around why that's the case. But the probe function only bails out if this returns EPROBE_DEFER. Not sure that's the best approach > >> + 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?! From my first attempt, didn't remove it from the example I sent. We could change i2c_init_recovery to something like the following then the gpio set / getter could use the default functions. Not sure the code is completely correct but hopefully you get the concept. static void i2c_init_recovery(struct i2c_adapter *adap) { struct i2c_bus_recovery_info *bri = adap->bus_recovery_info; char *err_str; if (!bri) return; if (!bri->recover_bus) { err_str = "no recover_bus() found"; goto err; } /* bail out if either no gpio or no set/get callback. */ if (!gpio_is_valid(bri->scl_gpio) && (!bri->set_scl || !bri->get_scl)) { if (!gpio_is_valid(bri->scl_gpio)) err_str = "invalid SCL gpio"; else err_str = "no {get|set}_scl() found"; goto err; } if (gpio_is_valid(bri->sda_gpio)) bri->get_sda = get_sda_gpio_value; if (gpio_is_valid(bri->scl_gpio)) { bri->get_scl = get_scl_gpio_value; bri->set_scl = set_scl_gpio_value; } return; err: dev_err(&adap->dev, "Not using recovery: %s\n", err_str); adap->bus_recovery_info = NULL; } > >> +}; > >> @@ -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. >
On Thu, 2017-05-11 at 09:24 +0800, Phil Reid wrote: > G'day Andy, > > Thanks for the review. You're welcome, just don't forget to remove the parts that are out of scope and/or you agree with. > On 10/05/2017 21:13, Andy Shevchenko wrote: > > On Wed, 2017-05-10 at 13:57 +0200, Tim Sander wrote: > > > +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(). > > My logic here was that if the gpio is optional return null we return > 0. Why?! _optional() *implies* that all rest calls will go fine and do nothing. > which is an okay status. > But this breaks if !CONFIG_GPIOLIB, which I keep forgetting. I've > never > quite wrapped my head around why that's the case. > > But the probe function only bails out if this returns EPROBE_DEFER. > Not sure that's the best approach You need something like desc = devm_gpiod_get_optional(...); if (IS_ERR(desc)) return PTR_ERR(desc); > > > + 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?! > > From my first attempt, didn't remove it from the example I sent. > > We could change i2c_init_recovery to something like the following > then the gpio set / getter could use the default functions. > Not sure the code is completely correct but hopefully you get the > concept. > > static void i2c_init_recovery(struct i2c_adapter *adap) > { > struct i2c_bus_recovery_info *bri = adap->bus_recovery_info; > char *err_str; > > if (!bri) > return; > > if (!bri->recover_bus) { > err_str = "no recover_bus() found"; > goto err; > } > > /* bail out if either no gpio or no set/get callback. */ > if (!gpio_is_valid(bri->scl_gpio) && (!bri->set_scl || !bri- > >get_scl)) { > if (!gpio_is_valid(bri->scl_gpio)) > err_str = "invalid SCL gpio"; > else > err_str = "no {get|set}_scl() found"; > goto err; > } > > if (gpio_is_valid(bri->sda_gpio)) > bri->get_sda = get_sda_gpio_value; > > if (gpio_is_valid(bri->scl_gpio)) { > bri->get_scl = get_scl_gpio_value; > bri->set_scl = set_scl_gpio_value; > } > > return; > err: > dev_err(&adap->dev, "Not using recovery: %s\n", err_str); > adap->bus_recovery_info = NULL; > } I have briefly looked at the current code. So, my suggestion is to switch to gpio descriptors in current code and then rebase your stuff on top. I wouldn't encourage people to continue using legacy GPIO API.
On Thu, 2017-05-11 at 16:53 +0300, Andy Shevchenko wrote: > On Thu, 2017-05-11 at 09:24 +0800, Phil Reid wrote: > I have briefly looked at the current code. > So, my suggestion is to switch to gpio descriptors in current code and > then rebase your stuff on top. For better transition it might be worth to create gpiod_ support in parallel with existing and convert existing clients case-by-case if needed, while discourage people to use gpio_ API. > I wouldn't encourage people to continue using legacy GPIO API.
On 11/05/2017 21:53, Andy Shevchenko wrote: >>>> +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(). >> My logic here was that if the gpio is optional return null we return >> 0. > Why?! > > _optional()*implies* that all rest calls will go fine and do nothing. > >> which is an okay status. >> But this breaks if !CONFIG_GPIOLIB, which I keep forgetting. I've >> never >> quite wrapped my head around why that's the case. >> >> But the probe function only bails out if this returns EPROBE_DEFER. >> Not sure that's the best approach > You need something like > > desc = devm_gpiod_get_optional(...); > if (IS_ERR(desc)) > return PTR_ERR(desc); > I found that continuing without the check on null results in a kernel panic for a dereferenced null pointer. So something in the gpiolib doesn't treat a null desc as optional. It was probably this code: int desc_to_gpio(const struct gpio_desc *desc) { return desc->gdev->base + (desc - &desc->gdev->descs[0]); } So perhaps this should return an invalid gpio number when desc == null I don't know what the intents are, so don't know if its a "bug" or by design.
On Fri, 2017-05-12 at 09:49 +0800, Phil Reid wrote: > On 11/05/2017 21:53, Andy Shevchenko wrote: > > > > > +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(). > > > > > > My logic here was that if the gpio is optional return null we > > > return > > > 0. > > > > Why?! > > > > _optional()*implies* that all rest calls will go fine and do > > nothing. > > > > > which is an okay status. > > > But this breaks if !CONFIG_GPIOLIB, which I keep forgetting. I've > > > never > > > quite wrapped my head around why that's the case. > > > > > > But the probe function only bails out if this returns > > > EPROBE_DEFER. > > > Not sure that's the best approach > > > > You need something like > > > > desc = devm_gpiod_get_optional(...); > > if (IS_ERR(desc)) > > return PTR_ERR(desc); > > > > I found that continuing without the check on null results in a kernel > panic for a dereferenced null pointer. > So something in the gpiolib doesn't treat a null desc as optional. > > It was probably this code: > int desc_to_gpio(const struct gpio_desc *desc) > { > return desc->gdev->base + (desc - &desc->gdev->descs[0]); > } > > So perhaps this should return an invalid gpio number when desc == null > > I don't know what the intents are, so don't know if its a "bug" or by > design. No, it doesn't seem like a bug to me. If you don't use legacy API it would be fine. Summarize this discussion, I would rather go this way: 1) introduce gpiod_ based API in I2C core for recovering; 2) rebase your patch on top of that change. It would be beneficial in a long term for everyone.
diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c index 7a3faa551cf8..f955f29ff8e7 100644 --- a/drivers/i2c/busses/i2c-designware-core.c +++ b/drivers/i2c/busses/i2c-designware-core.c @@ -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 @@ -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 + return 0; } 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 return -EIO; @@ -766,6 +773,7 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num) if (!wait_for_completion_timeout(&dev->cmd_complete, adap->timeout)) { dev_err(dev->dev, "controller timed out\n"); /* i2c_dw_init implicitly disables the adapter */ + i2c_recover_bus(&dev->adapter); i2c_dw_init(dev); ret = -ETIMEDOUT; goto done; diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h index d9aaf1790e0e..cedc895a795d 100644 --- a/drivers/i2c/busses/i2c-designware-core.h +++ b/drivers/i2c/busses/i2c-designware-core.h @@ -23,6 +23,7 @@ */ #include <linux/i2c.h> +#include <linux/gpio.h> #define DW_IC_DEFAULT_FUNCTIONALITY (I2C_FUNC_I2C | \ I2C_FUNC_SMBUS_BYTE | \ @@ -126,6 +127,9 @@ struct dw_i2c_dev { int (*acquire_lock)(struct dw_i2c_dev *dev); void (*release_lock)(struct dw_i2c_dev *dev); bool pm_runtime_disabled; + struct i2c_bus_recovery_info rinfo; + struct gpio_desc *gpio_sda; + struct gpio_desc *gpio_scl; }; #define ACCESS_SWAP 0x00000001 diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c index 79c4b4ea0539..b2d5adc8df2b 100644 --- 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> #include <linux/platform_data/i2c-designware.h> #include "i2c-designware-core.h" @@ -174,6 +175,88 @@ static void dw_i2c_set_fifo_size(struct dw_i2c_dev *dev, int id) } } +/* + * This routine does i2c bus recovery by using i2c_generic_gpio_recovery + * which is provided by I2C Bus recovery infrastructure. + */ +static void i2c_dw_prepare_recovery(struct i2c_adapter *adap) +{ + struct platform_device *pdev = to_platform_device(&adap->dev); + struct dw_i2c_dev *i_dev = platform_get_drvdata(pdev); + + i2c_dw_disable(i_dev); + reset_control_assert(i_dev->rst); + i2c_dw_plat_prepare_clk(i_dev, false); +} + +void i2c_dw_unprepare_recovery(struct i2c_adapter *adap) +{ + struct platform_device *pdev = to_platform_device(&adap->dev); + struct dw_i2c_dev *i_dev = platform_get_drvdata(pdev); + + i2c_dw_plat_prepare_clk(i_dev, true); + reset_control_deassert(i_dev->rst); + i2c_dw_init(i_dev); +} + + +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); + + return gpiod_get_value_cansleep(dev->gpio_scl); +} + +static void i2c_dw_set_scl(struct i2c_adapter *adap, int val) +{ + struct platform_device *pdev = to_platform_device(&adap->dev); + struct dw_i2c_dev *dev = platform_get_drvdata(pdev); + + gpiod_set_value_cansleep(dev->gpio_scl, val); +} + +static int i2c_dw_get_sda(struct i2c_adapter *adap) +{ + struct platform_device *pdev = to_platform_device(&adap->dev); + struct dw_i2c_dev *dev = platform_get_drvdata(pdev); + + return gpiod_get_value_cansleep(dev->gpio_sda); +} + +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)) + 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)) + return PTR_ERR(dev->gpio_sda); + + rinfo->scl_gpio = desc_to_gpio(dev->gpio_scl); + rinfo->sda_gpio = desc_to_gpio(dev->gpio_sda); + rinfo->set_scl = i2c_dw_set_scl; + rinfo->get_scl = i2c_dw_get_scl; + rinfo->get_sda = i2c_dw_get_sda; + + rinfo->recover_bus = i2c_generic_scl_recovery; + rinfo->prepare_recovery = i2c_dw_prepare_recovery; + rinfo->unprepare_recovery = i2c_dw_unprepare_recovery; + adap->bus_recovery_info = rinfo; + + dev_info(dev->dev, + "adapter: %s running with gpio recovery mode! scl:%i sda:%i \n", + adap->name, rinfo->scl_gpio, rinfo->sda_gpio); + + return 0; +}; + static int dw_i2c_plat_probe(struct platform_device *pdev) { struct dw_i2c_platform_data *pdata = dev_get_platdata(&pdev->dev); @@ -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"); if (dev->pm_runtime_disabled) { pm_runtime_forbid(&pdev->dev); @@ -295,11 +379,15 @@ static int dw_i2c_plat_probe(struct platform_device *pdev) pm_runtime_enable(&pdev->dev); } + r = i2c_dw_init_recovery_info(dev, adap); + if (r == -EPROBE_DEFER) + goto exit_probe; + r = i2c_dw_probe(dev); if (r) goto exit_probe; - return r; + return 0; exit_probe: if (!dev->pm_runtime_disabled)
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. Signed-off-by: Tim Sander <tim@krieglstein.org> --- drivers/i2c/busses/i2c-designware-core.c | 14 ++++- drivers/i2c/busses/i2c-designware-core.h | 4 ++ drivers/i2c/busses/i2c-designware-platdrv.c | 90 ++++++++++++++++++++++++++++- 3 files changed, 104 insertions(+), 4 deletions(-)