On Thu, Oct 04, 2012 at 04:34:53PM +0530, Viresh Kumar wrote: > Add i2c bus recovery infrastructure to i2c adapters as specified in the i2c > protocol Rev. 03 section 3.1.16 titled "Bus clear". > > http://www.nxp.com/documents/user_manual/UM10204.pdf > > Sometimes during operation i2c bus hangs and we need to give dummy clocks to > slave device to start the transfer again. Now we may have capability in the > bus > controller to generate these clocks or platform may have gpio pins which can > be > toggled to generate dummy clocks. This patch supports both. > > This patch also adds in generic bus recovery routines gpio or scl line based > which can be used by bus controller. In addition controller driver may provide > its own version of the bus recovery routine. > > This doesn't support multi-master recovery for now. > > Signed-off-by: Viresh Kumar <[email protected]>
As mentioned before, I still have issues with the API and have most
comments to that for now.
> ---
> V5->V6:
> - Removed sda_gpio_flags
> - Make scl_gpio_flags as GPIOF_OPEN_DRAIN | GPIOF_OUT_INIT_HIGH by default
> - update bri->set_scl and bri->get_sda for gpio recovery case in i2c core
> - Guaranteed to generate 9 falling-rising edges for bus recovery
>
> V4->V5:
> - section name corrected to 3.1.16
> - merged gpio and non-gpio recovery routines to remove code redundancy
> - Changed types of gpio and gpio-flags to unsigned and unsigned long
> - Checking return value of get_gpio() now
> - using DIV_ROUND_UP for calculating delay, to get more correct value
>
> drivers/i2c/i2c-core.c | 156
> +++++++++++++++++++++++++++++++++++++++++++++++++
> include/linux/i2c.h | 55 +++++++++++++++++
> 2 files changed, 211 insertions(+)
>
> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> index a7edf98..e78033b 100644
> --- a/drivers/i2c/i2c-core.c
> +++ b/drivers/i2c/i2c-core.c
> @@ -27,7 +27,9 @@
>
> #include <linux/module.h>
> #include <linux/kernel.h>
> +#include <linux/delay.h>
> #include <linux/errno.h>
> +#include <linux/gpio.h>
> #include <linux/slab.h>
> #include <linux/i2c.h>
> #include <linux/init.h>
> @@ -104,6 +106,111 @@ static int i2c_device_uevent(struct device *dev, struct
> kobj_uevent_env *env)
> #define i2c_device_uevent NULL
> #endif /* CONFIG_HOTPLUG */
>
> +/* i2c bus recovery routines */
> +static void set_scl_gpio_value(struct i2c_adapter *adap, int val)
> +{
> + gpio_set_value(adap->bus_recovery_info->scl_gpio, val);
> +}
> +
> +static int get_sda_gpio_value(struct i2c_adapter *adap)
> +{
> + return gpio_get_value(adap->bus_recovery_info->sda_gpio);
> +}
> +
> +static int i2c_get_gpios_for_recovery(struct i2c_adapter *adap)
> +{
> + struct i2c_bus_recovery_info *bri = adap->bus_recovery_info;
> + struct device *dev = &adap->dev;
> + int ret = 0;
> +
> + if (bri->get_gpio) {
> + ret = bri->get_gpio(bri->scl_gpio);
> + if (ret) {
> + dev_warn(dev, "scl get_gpio: %d\n", bri->scl_gpio);
This warning is probably not very helpful to a user.
> + return ret;
> + }
> + }
> +
> + ret = gpio_request_one(bri->scl_gpio, bri->scl_gpio_flags, "i2c-scl");
> + if (ret) {
> + dev_warn(dev, "gpio request fail: %d\n", bri->scl_gpio);
> + goto scl_put_gpio;
> + }
> +
> + if (!bri->skip_sda_polling) {
> + if (bri->get_gpio)
> + ret = bri->get_gpio(bri->sda_gpio);
> +
> + if (unlikely(ret ||
Since the unlikely() are not in hot-paths, you probably better skip
them.
> + gpio_request_one(bri->sda_gpio, GPIOF_IN, "i2c-sda"))) {
> + /* work without sda polling */
> + dev_warn(dev, "can't get sda: %d. Skip sda polling\n",
> + bri->sda_gpio);
> + bri->skip_sda_polling = true;
> + if (!ret && bri->put_gpio)
> + bri->put_gpio(bri->sda_gpio);
> +
> + ret = 0;
> + }
> + }
> +
> +scl_put_gpio:
> + if (bri->put_gpio)
> + bri->put_gpio(bri->scl_gpio);
> +
> + return ret;
> +}
> +
> +static void i2c_put_gpios_for_recovery(struct i2c_adapter *adap)
> +{
> + struct i2c_bus_recovery_info *bri = adap->bus_recovery_info;
> +
> + gpio_free(bri->scl_gpio);
> +
> + if (!bri->skip_sda_polling) {
> + gpio_free(bri->sda_gpio);
> +
> + if (bri->put_gpio)
> + bri->put_gpio(bri->sda_gpio);
> + }
> +}
> +
> +static int i2c_recover_bus(struct i2c_adapter *adap)
> +{
> + struct i2c_bus_recovery_info *bri = adap->bus_recovery_info;
> + unsigned long delay = 1000000;
What is this magic value?
> + int i, ret, val = 0;
> +
> + if (bri->is_gpio_recovery) {
> + ret = i2c_get_gpios_for_recovery(adap);
> + if (ret)
> + return ret;
> + } else {
> + bri->set_scl(adap, 1);
> + }
> +
> + /*
> + * By this time SCL is high, as we need to give 9 falling-rising edges
> + */
> +
> + delay = DIV_ROUND_UP(delay, bri->clock_rate_khz * 2);
> +
> + for (i = 0; i < bri->clock_cnt * 2; i++, val = !val) {
> + bri->set_scl(adap, val);
> + ndelay(delay);
> +
> + /* break if sda got high, check only when scl line is high */
> + if (!bri->skip_sda_polling && val)
> + if (unlikely(bri->get_sda(adap)))
> + break;
> + }
> +
> + if (bri->is_gpio_recovery)
> + i2c_put_gpios_for_recovery(adap);
> +
> + return 0;
> +}
> +
> static int i2c_device_probe(struct device *dev)
> {
> struct i2c_client *client = i2c_verify_client(dev);
> @@ -896,6 +1003,55 @@ static int i2c_register_adapter(struct i2c_adapter
> *adap)
> "Failed to create compatibility class link\n");
> #endif
>
> + /* bus recovery specific initialization */
> + if (adap->bus_recovery_info) {
> + struct i2c_bus_recovery_info *bri = adap->bus_recovery_info;
> +
> + if (bri->recover_bus) {
> + dev_info(&adap->dev,
> + "registered for non-generic bus recovery\n");
> + } else {
> + /* Use generic recovery routines */
> + if (!bri->clock_rate_khz) {
> + dev_warn(&adap->dev,
> + "doesn't have valid recovery clock
> rate\n");
> + goto exit_recovery;
> + }
> +
> + /* Most controller need 9 clocks at max */
> + if (!bri->clock_cnt)
> + bri->clock_cnt = 9;
> +
> + bri->recover_bus = i2c_recover_bus;
> +
> + if (bri->is_gpio_recovery) {
> + if (!bri->scl_gpio_flags)
> + bri->scl_gpio_flags = GPIOF_OPEN_DRAIN;
> +
> + /* We always start by making GPIO HIGH */
> + bri->scl_gpio_flags |= GPIOF_OUT_INIT_HIGH;
> +
> + bri->set_scl = set_scl_gpio_value;
> + bri->get_sda = get_sda_gpio_value;
> + dev_info(&adap->dev,
> + "registered for gpio bus recovery\n");
> + } else if (bri->set_scl) {
> + if (!bri->skip_sda_polling && !bri->get_sda) {
> + dev_warn(&adap->dev,
> + "!get_sda. skip sda polling\n");
> + bri->skip_sda_polling = true;
> + }
> +
> + dev_info(&adap->dev,
> + "registered for scl bus recovery\n");
> + } else {
> + dev_warn(&adap->dev,
> + "doesn't have valid recovery type\n");
> + }
> + }
> + }
> +
> +exit_recovery:
> /* create pre-declared device nodes */
> if (adap->nr < __i2c_first_dynamic_bus_num)
> i2c_scan_static_board_info(adap);
> diff --git a/include/linux/i2c.h b/include/linux/i2c.h
> index 5970266..13eeb2e 100644
> --- a/include/linux/i2c.h
> +++ b/include/linux/i2c.h
> @@ -370,6 +370,58 @@ struct i2c_algorithm {
> u32 (*functionality) (struct i2c_adapter *);
> };
>
> +/**
> + * struct i2c_bus_recovery_info - I2c bus recovery information
> + * @recover_bus: Recover routine. Either pass driver's recover_bus()
> routine, or
> + * pass it NULL to use generic ones, i.e. gpio or scl based.
What about having those options?
NULL or custom_pointer or i2c_generic_scl_recovery or i2c_generic_gpio_recovery
where i2c_generic_gpio_recovery is probably:
get_gpios_for_recovery
i2c_generic_scl_recovery
put_gpios_for_recovery
and i2c_generic_scl_recovery is basically your current
i2c_generic_recovery. That makes it easier to add other generic routines
if that should ever become necessary.
> + * @skip_sda_polling: if true, bus recovery will not poll sda line to check
> if
> + * it became high or not. Only required if recover_bus == NULL.
Does a user really need to set this?
> + * @is_gpio_recovery: true, select gpio type else scl type. Only required if
> + * recover_bus == NULL.
This could be dropped in favor of i2c_generic_*_recovery in recover_bus
> + * @clock_rate_khz: clock rate of dummy clock in khz. Required for both gpio
> and
> + * scl type recovery.
Does a user really need this? We could probably use something close to
100kHz always?
> + * @clock_cnt: count of max clocks to be generated. Required for both gpio
> and
> + * scl type recovery.
Don't think this should be something else than 9. If so, it should be
increased generally in the core and not inside some platform data.
> + * @set_scl: controller specific routine, if is_gpio_recovery == false.
> + * set_scl_gpio_value otherwise
> + * @get_sda: controller specific routine, if is_gpio_recovery == false.
> + * get_sda_gpio_value otherwise
Basically OK, documentation should be more user-centric not
implementation centric :)
> + * @get_gpio: called before recover_bus() to get padmux configured for scl
> line.
> + * as gpio. Only required if is_gpio_recovery == true. Return 0 on success.
> + * @put_gpio: called after recover_bus() to get padmux configured for scl
> line
> + * as scl. Only required if is_gpio_recovery == true.
I wonder if it makes sense to have those more generic like
prepare_recovery and unprepare_recovery?
> + * @scl_gpio: gpio number of the scl line. Only required if is_gpio_recovery
> ==
> + * true.
> + * @sda_gpio: gpio number of the sda line. Only required if is_gpio_recovery
> ==
> + * true and skip_sda_polling == false.
OK.
> + * @scl_gpio_flags: flag for gpio_request_one of scl_gpio. If passed as 0,
> + * (GPIOF_OPEN_DRAIN | GPIOF_OUT_INIT_HIGH) is used instead. Otherwise,
> it
> + * is ORRED with GPIOF_OUT_INIT_HIGH.
Is this needed? I'd say we drop it until somebody with a need can add
something like this.
> + * These is no need of sda_gpio_flags, as we always read it in input
> mode.
> + */
> +struct i2c_bus_recovery_info {
> + int (*recover_bus)(struct i2c_adapter *);
> + bool skip_sda_polling;
> + bool is_gpio_recovery;
> + u32 clock_rate_khz;
> + u8 clock_cnt;
> +
> + /*
> + * Fn pointers for recovery, will point either to:
> + * - set_scl_gpio_value and get_sda_gpio_value for gpio recovery
> + * - Controller specific routines, otherwise
> + */
> + void (*set_scl)(struct i2c_adapter *, int val);
> + int (*get_sda)(struct i2c_adapter *);
> +
> + /* gpio recovery */
> + int (*get_gpio)(unsigned gpio);
> + void (*put_gpio)(unsigned gpio);
> + unsigned scl_gpio;
> + unsigned sda_gpio;
> + unsigned long scl_gpio_flags;
> +};
> +
> /*
> * i2c_adapter is the structure used to identify a physical i2c bus along
> * with the access algorithms necessary to access it.
> @@ -393,6 +445,9 @@ struct i2c_adapter {
>
> struct mutex userspace_clients_lock;
> struct list_head userspace_clients;
> +
> + /* Pass valid pointer if recovery infrastructure is required */
This comment can be left out.
> + struct i2c_bus_recovery_info *bus_recovery_info;
> };
> #define to_i2c_adapter(d) container_of(d, struct i2c_adapter, dev)
>
> --
> 1.7.12.rc2.18.g61b472e
>
>
See also the next mail...
--
Pengutronix e.K. | Wolfram Sang |
Industrial Linux Solutions | http://www.pengutronix.de/ |
signature.asc
Description: Digital signature
