Re: [PATCH v2] i2c: cadence: Move to sensible power management
Hi Shubhrajyoti,
On Wed, 2015-10-28 at 12:56PM +0530, Shubhrajyoti Datta wrote:
> Currently the clocks are enabled at probe and disabled at remove.
> Which keeps the clocks enabled even if no transaction is going on.
> This patch enables the clocks at the start of transfer and disables
> after it.
>
> Also adapts to runtime pm.
> Remove xi2c->suspended and use pm runtime status instead.
>
> converts dev pm to const to silence a checkpatch warning.
>
> Signed-off-by: Shubhrajyoti Datta
To me, this looks all good. Just one small concern below.
> ---
> changes since v2
> update the cc list
>
> drivers/i2c/busses/i2c-cadence.c | 73
> --
> 1 files changed, 46 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-cadence.c
> b/drivers/i2c/busses/i2c-cadence.c
> index 84deed6..6b08d16 100644
> --- a/drivers/i2c/busses/i2c-cadence.c
> +++ b/drivers/i2c/busses/i2c-cadence.c
> @@ -18,6 +18,7 @@
> #include
> #include
> #include
> +#include
>
> /* Register offsets for the I2C device. */
> #define CDNS_I2C_CR_OFFSET 0x00 /* Control Register, RW */
> @@ -96,6 +97,8 @@
>CDNS_I2C_IXR_COMP)
>
> #define CDNS_I2C_TIMEOUT msecs_to_jiffies(1000)
> +/* timeout for pm runtime autosuspend */
> +#define CNDS_I2C_PM_TIMEOUT 1000/* ms */
>
> #define CDNS_I2C_FIFO_DEPTH 16
> /* FIFO depth at which the DATA interrupt occurs */
> @@ -128,7 +131,6 @@
> * @xfer_done: Transfer complete status
> * @p_send_buf: Pointer to transmit buffer
> * @p_recv_buf: Pointer to receive buffer
> - * @suspended: Flag holding the device's PM status
> * @send_count: Number of bytes still expected to send
> * @recv_count: Number of bytes still expected to receive
> * @curr_recv_count: Number of bytes to be received in current transfer
> @@ -141,6 +143,7 @@
> * @quirks: flag for broken hold bit usage in r1p10
> */
> struct cdns_i2c {
> + struct device *dev;
> void __iomem *membase;
> struct i2c_adapter adap;
> struct i2c_msg *p_msg;
> @@ -148,7 +151,6 @@ struct cdns_i2c {
> struct completion xfer_done;
> unsigned char *p_send_buf;
> unsigned char *p_recv_buf;
> - u8 suspended;
There might have been a reason to store this flag here. Did you test
this with lockdep and CONFIG_DEBUG_ATOMIC_SLEEP? Just to make sure that
nothing that can sleep is called from atomic context.
Sören
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] i2c: cadence: Move to sensible power management
On Wed, Oct 28, 2015 at 9:48 PM, Sören Brinkmann
wrote:
> Hi Shubhrajyoti,
>
>
> On Wed, 2015-10-28 at 12:56PM +0530, Shubhrajyoti Datta wrote:
>> Currently the clocks are enabled at probe and disabled at remove.
>> Which keeps the clocks enabled even if no transaction is going on.
>> This patch enables the clocks at the start of transfer and disables
>> after it.
>>
>> Also adapts to runtime pm.
>> Remove xi2c->suspended and use pm runtime status instead.
>>
>> converts dev pm to const to silence a checkpatch warning.
>>
>> Signed-off-by: Shubhrajyoti Datta
>
> To me, this looks all good. Just one small concern below.
Thanks for the review.
>
>> ---
>> changes since v2
>> update the cc list
>>
>> drivers/i2c/busses/i2c-cadence.c | 73
>> --
>> 1 files changed, 46 insertions(+), 27 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-cadence.c
>> b/drivers/i2c/busses/i2c-cadence.c
>> index 84deed6..6b08d16 100644
>> --- a/drivers/i2c/busses/i2c-cadence.c
>> +++ b/drivers/i2c/busses/i2c-cadence.c
>> @@ -18,6 +18,7 @@
>> #include
>> #include
>> #include
>> +#include
>>
>> /* Register offsets for the I2C device. */
>> #define CDNS_I2C_CR_OFFSET 0x00 /* Control Register, RW */
>> @@ -96,6 +97,8 @@
>>CDNS_I2C_IXR_COMP)
>>
>> #define CDNS_I2C_TIMEOUT msecs_to_jiffies(1000)
>> +/* timeout for pm runtime autosuspend */
>> +#define CNDS_I2C_PM_TIMEOUT 1000/* ms */
>>
>> #define CDNS_I2C_FIFO_DEPTH 16
>> /* FIFO depth at which the DATA interrupt occurs */
>> @@ -128,7 +131,6 @@
>> * @xfer_done: Transfer complete status
>> * @p_send_buf: Pointer to transmit buffer
>> * @p_recv_buf: Pointer to receive buffer
>> - * @suspended: Flag holding the device's PM status
>> * @send_count: Number of bytes still expected to send
>> * @recv_count: Number of bytes still expected to receive
>> * @curr_recv_count: Number of bytes to be received in current transfer
>> @@ -141,6 +143,7 @@
>> * @quirks: flag for broken hold bit usage in r1p10
>> */
>> struct cdns_i2c {
>> + struct device *dev;
>> void __iomem *membase;
>> struct i2c_adapter adap;
>> struct i2c_msg *p_msg;
>> @@ -148,7 +151,6 @@ struct cdns_i2c {
>> struct completion xfer_done;
>> unsigned char *p_send_buf;
>> unsigned char *p_recv_buf;
>> - u8 suspended;
>
> There might have been a reason to store this flag here. Did you test
> this with lockdep and CONFIG_DEBUG_ATOMIC_SLEEP? Just to make sure that
> nothing that can sleep is called from atomic context.
Done now.
Essentially this is a flag is set in suspend routine. and checked in
the isr I use
pm_runtime_suspended(id->dev) instead.
>
> Sören
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] i2c: cadence: Move to sensible power management
On Thu, Oct 29, 2015 at 8:27 PM, Shubhrajyoti Datta wrote: > On Wed, Oct 28, 2015 at 9:48 PM, Sören Brinkmann > wrote: >> Hi Shubhrajyoti, >> >> >> On Wed, 2015-10-28 at 12:56PM +0530, Shubhrajyoti Datta wrote: >>> Currently the clocks are enabled at probe and disabled at remove. >>> Which keeps the clocks enabled even if no transaction is going on. >>> This patch enables the clocks at the start of transfer and disables >>> after it. >>> >>> Also adapts to runtime pm. >>> Remove xi2c->suspended and use pm runtime status instead. >>> >>> converts dev pm to const to silence a checkpatch warning. >>> >>> Signed-off-by: Shubhrajyoti Datta >> >> To me, this looks all good. Just one small concern below. > > Thanks for the review. Soren , Do are you ok with the change or do you want me to resend without the suspended flag change. <> >> >> There might have been a reason to store this flag here. Did you test >> this with lockdep and CONFIG_DEBUG_ATOMIC_SLEEP? Just to make sure that >> nothing that can sleep is called from atomic context. > Done now. > > > Essentially this is a flag is set in suspend routine. and checked in > the isr I use > pm_runtime_suspended(id->dev) instead. > >> >> Sören >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in >> the body of a message to majord...@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html >> Please read the FAQ at http://www.tux.org/lkml/ -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] i2c: cadence: Move to sensible power management
On Sat, 2015-11-21 at 07:00PM +0530, Shubhrajyoti Datta wrote: > On Thu, Oct 29, 2015 at 8:27 PM, Shubhrajyoti Datta > wrote: > > On Wed, Oct 28, 2015 at 9:48 PM, Sören Brinkmann > > wrote: > >> Hi Shubhrajyoti, > >> > >> > >> On Wed, 2015-10-28 at 12:56PM +0530, Shubhrajyoti Datta wrote: > >>> Currently the clocks are enabled at probe and disabled at remove. > >>> Which keeps the clocks enabled even if no transaction is going on. > >>> This patch enables the clocks at the start of transfer and disables > >>> after it. > >>> > >>> Also adapts to runtime pm. > >>> Remove xi2c->suspended and use pm runtime status instead. > >>> > >>> converts dev pm to const to silence a checkpatch warning. > >>> > >>> Signed-off-by: Shubhrajyoti Datta > >> > >> To me, this looks all good. Just one small concern below. > > > > Thanks for the review. > Soren , > Do are you ok with the change or do you want me to resend without the > suspended flag change. I'm always for removing code that is not needed. If things are tested and well and work without throwing any warnings I'm OK with it. Sören -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] i2c: cadence: Move to sensible power management
On Tue, Nov 24, 2015 at 12:17 AM, Sören Brinkmann wrote: > On Sat, 2015-11-21 at 07:00PM +0530, Shubhrajyoti Datta wrote: >> On Thu, Oct 29, 2015 at 8:27 PM, Shubhrajyoti Datta >> wrote: >> > On Wed, Oct 28, 2015 at 9:48 PM, Sören Brinkmann >> > wrote: >> >> Hi Shubhrajyoti, >> >> >> >> >> >> On Wed, 2015-10-28 at 12:56PM +0530, Shubhrajyoti Datta wrote: >> >>> Currently the clocks are enabled at probe and disabled at remove. >> >>> Which keeps the clocks enabled even if no transaction is going on. >> >>> This patch enables the clocks at the start of transfer and disables >> >>> after it. >> >>> >> >>> Also adapts to runtime pm. >> >>> Remove xi2c->suspended and use pm runtime status instead. >> >>> >> >>> converts dev pm to const to silence a checkpatch warning. >> >>> >> >>> Signed-off-by: Shubhrajyoti Datta >> >> >> >> To me, this looks all good. Just one small concern below. >> > >> > Thanks for the review. >> Soren , >> Do are you ok with the change or do you want me to resend without the >> suspended flag change. > > I'm always for removing code that is not needed. If things are tested > and well and work without throwing any warnings I'm OK with it. It should be also having a suspended book-keeping in the driver is not needed the pm does that for us. I will spilt the patch and resend. Thanks, > > Sören -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
