Re: [PATCH 1/2] i2c: mux: pca9541: add delayed-release support

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, 24 Jan 2022 at 21:39, Zev Weiss <zev@xxxxxxxxxxxxxxxxx> wrote:
>
> For heavily-utilized i2c busses, the overhead of reacquiring bus
> ownership on every transaction can be quite substantial.  By delaying
> the release of the bus (in anticipation of another transaction needing
> to re-acquire ownership in the near future), we can reduce the
> overhead significantly -- a subsequent transaction that arrives within
> the delay window can merely verify that we still own it.
>
> The new "release-delay-us" DT property specifies a number of
> microseconds to wait after a transaction before releasing the bus
> (zero by default so as to preserve the existing behavior of releasing
> it immediately).

Sounds like a good idea to me!

>
> Signed-off-by: Zev Weiss <zev@xxxxxxxxxxxxxxxxx>

Reviewed-by: Joel Stanley <joel@xxxxxxxxx>

> ---
>  drivers/i2c/muxes/i2c-mux-pca9541.c | 56 ++++++++++++++++++++++++-----
>  1 file changed, 47 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/i2c/muxes/i2c-mux-pca9541.c b/drivers/i2c/muxes/i2c-mux-pca9541.c
> index 6daec8d3d331..76269bf9f9ca 100644
> --- a/drivers/i2c/muxes/i2c-mux-pca9541.c
> +++ b/drivers/i2c/muxes/i2c-mux-pca9541.c
> @@ -19,6 +19,7 @@
>  #include <linux/bitops.h>
>  #include <linux/delay.h>
>  #include <linux/device.h>
> +#include <linux/devm-helpers.h>
>  #include <linux/i2c.h>
>  #include <linux/i2c-mux.h>
>  #include <linux/jiffies.h>
> @@ -75,6 +76,8 @@ struct pca9541 {
>         struct i2c_client *client;
>         unsigned long select_timeout;
>         unsigned long arb_timeout;
> +       unsigned long release_delay;
> +       struct delayed_work release_work;
>  };
>
>  static const struct i2c_device_id pca9541_id[] = {
> @@ -127,8 +130,11 @@ static int pca9541_reg_read(struct i2c_client *client, u8 command)
>   * Arbitration management functions
>   */
>
> -/* Release bus. Also reset NTESTON and BUSINIT if it was set. */
> -static void pca9541_release_bus(struct i2c_client *client)
> +/*
> + * Release bus. Also reset NTESTON and BUSINIT if it was set.
> + * client->adapter must already be locked.
> + */
> +static void __pca9541_release_bus(struct i2c_client *client)
>  {
>         int reg;
>
> @@ -138,6 +144,13 @@ static void pca9541_release_bus(struct i2c_client *client)
>                                   (reg & PCA9541_CTL_NBUSON) >> 1);
>  }
>
> +static void pca9541_release_bus(struct i2c_client *client)
> +{
> +       i2c_lock_bus(client->adapter, I2C_LOCK_SEGMENT);
> +       __pca9541_release_bus(client);
> +       i2c_unlock_bus(client->adapter, I2C_LOCK_SEGMENT);
> +}
> +
>  /*
>   * Arbitration is defined as a two-step process. A bus master can only activate
>   * the slave bus if it owns it; otherwise it has to request ownership first.
> @@ -254,6 +267,9 @@ static int pca9541_select_chan(struct i2c_mux_core *muxc, u32 chan)
>         unsigned long timeout = jiffies + ARB2_TIMEOUT;
>                 /* give up after this time */
>
> +       if (data->release_delay)
> +               cancel_delayed_work_sync(&data->release_work);
> +
>         data->arb_timeout = jiffies + ARB_TIMEOUT;
>                 /* force bus ownership after this time */
>
> @@ -276,10 +292,21 @@ static int pca9541_release_chan(struct i2c_mux_core *muxc, u32 chan)
>         struct pca9541 *data = i2c_mux_priv(muxc);
>         struct i2c_client *client = data->client;
>
> -       pca9541_release_bus(client);
> +       if (data->release_delay)
> +               schedule_delayed_work(&data->release_work, data->release_delay);
> +       else
> +               __pca9541_release_bus(client);
> +
>         return 0;
>  }
>
> +static void pca9541_release_workfn(struct work_struct *work)
> +{
> +       struct pca9541 *data = container_of(work, struct pca9541, release_work.work);
> +
> +       pca9541_release_bus(data->client);
> +}
> +
>  /*
>   * I2C init/probing/exit functions
>   */
> @@ -289,18 +316,13 @@ static int pca9541_probe(struct i2c_client *client,
>         struct i2c_adapter *adap = client->adapter;
>         struct i2c_mux_core *muxc;
>         struct pca9541 *data;
> +       u32 release_delay_us;
>         int ret;
>
>         if (!i2c_check_functionality(adap, I2C_FUNC_SMBUS_BYTE_DATA))
>                 return -ENODEV;
>
> -       /*
> -        * I2C accesses are unprotected here.
> -        * We have to lock the I2C segment before releasing the bus.
> -        */
> -       i2c_lock_bus(adap, I2C_LOCK_SEGMENT);
>         pca9541_release_bus(client);
> -       i2c_unlock_bus(adap, I2C_LOCK_SEGMENT);
>
>         /* Create mux adapter */
>
> @@ -313,6 +335,14 @@ static int pca9541_probe(struct i2c_client *client,
>         data = i2c_mux_priv(muxc);
>         data->client = client;
>
> +       if (!device_property_read_u32(&client->dev, "release-delay-us", &release_delay_us)) {
> +               data->release_delay = usecs_to_jiffies(release_delay_us);
> +               ret = devm_delayed_work_autocancel(&client->dev, &data->release_work,
> +                                                  pca9541_release_workfn);
> +               if (ret)
> +                       return ret;
> +       }
> +
>         i2c_set_clientdata(client, muxc);
>
>         ret = i2c_mux_add_adapter(muxc, 0, 0, 0);
> @@ -328,6 +358,14 @@ static int pca9541_probe(struct i2c_client *client,
>  static int pca9541_remove(struct i2c_client *client)
>  {
>         struct i2c_mux_core *muxc = i2c_get_clientdata(client);
> +       struct pca9541 *data = i2c_mux_priv(muxc);
> +
> +       /*
> +        * Ensure the bus is released (in case the device gets destroyed
> +        * before release_work runs).
> +        */
> +       if (data->release_delay)
> +               pca9541_release_bus(client);
>
>         i2c_mux_del_adapters(muxc);
>         return 0;
> --
> 2.34.1
>

On Mon, 24 Jan 2022 at 21:39, Zev Weiss <zev@xxxxxxxxxxxxxxxxx> wrote:
>
> For heavily-utilized i2c busses, the overhead of reacquiring bus
> ownership on every transaction can be quite substantial.  By delaying
> the release of the bus (in anticipation of another transaction needing
> to re-acquire ownership in the near future), we can reduce the
> overhead significantly -- a subsequent transaction that arrives within
> the delay window can merely verify that we still own it.
>
> The new "release-delay-us" DT property specifies a number of
> microseconds to wait after a transaction before releasing the bus
> (zero by default so as to preserve the existing behavior of releasing
> it immediately).
>
> Signed-off-by: Zev Weiss <zev@xxxxxxxxxxxxxxxxx>
> ---
>  drivers/i2c/muxes/i2c-mux-pca9541.c | 56 ++++++++++++++++++++++++-----
>  1 file changed, 47 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/i2c/muxes/i2c-mux-pca9541.c b/drivers/i2c/muxes/i2c-mux-pca9541.c
> index 6daec8d3d331..76269bf9f9ca 100644
> --- a/drivers/i2c/muxes/i2c-mux-pca9541.c
> +++ b/drivers/i2c/muxes/i2c-mux-pca9541.c
> @@ -19,6 +19,7 @@
>  #include <linux/bitops.h>
>  #include <linux/delay.h>
>  #include <linux/device.h>
> +#include <linux/devm-helpers.h>
>  #include <linux/i2c.h>
>  #include <linux/i2c-mux.h>
>  #include <linux/jiffies.h>
> @@ -75,6 +76,8 @@ struct pca9541 {
>         struct i2c_client *client;
>         unsigned long select_timeout;
>         unsigned long arb_timeout;
> +       unsigned long release_delay;
> +       struct delayed_work release_work;
>  };
>
>  static const struct i2c_device_id pca9541_id[] = {
> @@ -127,8 +130,11 @@ static int pca9541_reg_read(struct i2c_client *client, u8 command)
>   * Arbitration management functions
>   */
>
> -/* Release bus. Also reset NTESTON and BUSINIT if it was set. */
> -static void pca9541_release_bus(struct i2c_client *client)
> +/*
> + * Release bus. Also reset NTESTON and BUSINIT if it was set.
> + * client->adapter must already be locked.
> + */
> +static void __pca9541_release_bus(struct i2c_client *client)
>  {
>         int reg;
>
> @@ -138,6 +144,13 @@ static void pca9541_release_bus(struct i2c_client *client)
>                                   (reg & PCA9541_CTL_NBUSON) >> 1);
>  }
>
> +static void pca9541_release_bus(struct i2c_client *client)
> +{
> +       i2c_lock_bus(client->adapter, I2C_LOCK_SEGMENT);
> +       __pca9541_release_bus(client);
> +       i2c_unlock_bus(client->adapter, I2C_LOCK_SEGMENT);
> +}
> +
>  /*
>   * Arbitration is defined as a two-step process. A bus master can only activate
>   * the slave bus if it owns it; otherwise it has to request ownership first.
> @@ -254,6 +267,9 @@ static int pca9541_select_chan(struct i2c_mux_core *muxc, u32 chan)
>         unsigned long timeout = jiffies + ARB2_TIMEOUT;
>                 /* give up after this time */
>
> +       if (data->release_delay)
> +               cancel_delayed_work_sync(&data->release_work);
> +
>         data->arb_timeout = jiffies + ARB_TIMEOUT;
>                 /* force bus ownership after this time */
>
> @@ -276,10 +292,21 @@ static int pca9541_release_chan(struct i2c_mux_core *muxc, u32 chan)
>         struct pca9541 *data = i2c_mux_priv(muxc);
>         struct i2c_client *client = data->client;
>
> -       pca9541_release_bus(client);
> +       if (data->release_delay)
> +               schedule_delayed_work(&data->release_work, data->release_delay);
> +       else
> +               __pca9541_release_bus(client);
> +
>         return 0;
>  }
>
> +static void pca9541_release_workfn(struct work_struct *work)
> +{
> +       struct pca9541 *data = container_of(work, struct pca9541, release_work.work);
> +
> +       pca9541_release_bus(data->client);
> +}
> +
>  /*
>   * I2C init/probing/exit functions
>   */
> @@ -289,18 +316,13 @@ static int pca9541_probe(struct i2c_client *client,
>         struct i2c_adapter *adap = client->adapter;
>         struct i2c_mux_core *muxc;
>         struct pca9541 *data;
> +       u32 release_delay_us;
>         int ret;
>
>         if (!i2c_check_functionality(adap, I2C_FUNC_SMBUS_BYTE_DATA))
>                 return -ENODEV;
>
> -       /*
> -        * I2C accesses are unprotected here.
> -        * We have to lock the I2C segment before releasing the bus.
> -        */
> -       i2c_lock_bus(adap, I2C_LOCK_SEGMENT);
>         pca9541_release_bus(client);
> -       i2c_unlock_bus(adap, I2C_LOCK_SEGMENT);
>
>         /* Create mux adapter */
>
> @@ -313,6 +335,14 @@ static int pca9541_probe(struct i2c_client *client,
>         data = i2c_mux_priv(muxc);
>         data->client = client;
>
> +       if (!device_property_read_u32(&client->dev, "release-delay-us", &release_delay_us)) {
> +               data->release_delay = usecs_to_jiffies(release_delay_us);
> +               ret = devm_delayed_work_autocancel(&client->dev, &data->release_work,
> +                                                  pca9541_release_workfn);
> +               if (ret)
> +                       return ret;
> +       }
> +
>         i2c_set_clientdata(client, muxc);
>
>         ret = i2c_mux_add_adapter(muxc, 0, 0, 0);
> @@ -328,6 +358,14 @@ static int pca9541_probe(struct i2c_client *client,
>  static int pca9541_remove(struct i2c_client *client)
>  {
>         struct i2c_mux_core *muxc = i2c_get_clientdata(client);
> +       struct pca9541 *data = i2c_mux_priv(muxc);
> +
> +       /*
> +        * Ensure the bus is released (in case the device gets destroyed
> +        * before release_work runs).
> +        */
> +       if (data->release_delay)
> +               pca9541_release_bus(client);
>
>         i2c_mux_del_adapters(muxc);
>         return 0;
> --
> 2.34.1
>



[Index of Archives]     [Linux GPIO]     [Linux SPI]     [Linux Hardward Monitoring]     [LM Sensors]     [Linux USB Devel]     [Linux Media]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux