[PATCH] gpio/omap: ensure gpio context is initialised

[khilman@linaro.org (Kevin Hilman)]

Santosh Shilimkar <santosh.shilimkar@ti.com> writes:

> On Friday 19 April 2013 06:19 AM, Jon Hunter wrote:
>> 
>> On 04/18/2013 07:34 PM, Jon Hunter wrote:
>>>
>>> On 04/18/2013 06:10 PM, Jon Hunter wrote:
>>>> On 04/18/2013 04:34 PM, Kevin Hilman wrote:
>>>
>>> ...
>>>
>>>>> Why not just init context right here if bank->loses_context &&
>>>>> !bank->context_valid?
>>>
>>> I really like this idea a lot. It can really clean-up the code
>>> and really make it much more readable. Before we were playing 
>>> some tricks with when we init'ed the get_context_loss_count()
>>> function pointer. How about the below?
>>>
>>> Tony, care to re-test?
>>>
>>> Cheers
>>> Jon
>>>
>>> From d7a940531d354e6be5e16ee50fa8344041df963a Mon Sep 17 00:00:00 2001
>>> From: Jon Hunter <jon-hunter@ti.com>
>>> Date: Mon, 15 Apr 2013 13:06:54 -0500
>>> Subject: [PATCH] gpio/omap: ensure gpio context is initialised
>>>
>>> Commit a2797be (gpio/omap: force restore if context loss is not
>>> detectable) broke gpio support for OMAP when booting with device-tree
>>> because a restore of the gpio context being performed without ever
>>> initialising the gpio context. In other words, the context restored was
>>> bad.
>>>
>>> This problem could also occur in the non device-tree case, however, it
>>> is much less likely because when booting without device-tree we can
>>> detect context loss via a platform specific API and so context restore
>>> is performed less often.
>>>
>>> Nevertheless we should ensure that the gpio context is initialised
>>> on the first pm-runtime resume for gpio banks that could lose their
>>> state regardless of whether we are booting with device-tree or not.
>>>
>>> The context loss count was being initialised on the first pm-runtime
>>> suspend following a resume, by populating the get_count_loss_count()
>>> function pointer after the first pm-runtime resume. To make the code
>>> more readable and logical, initialise the context loss count on the
>>> first pm-runtime resume if the context is not yet valid.
>>>
>>> Reported-by: Tony Lindgren <tony@atomide.com>
>>> Signed-off-by: Jon Hunter <jon-hunter@ti.com>
>>> ---
>>>  drivers/gpio/gpio-omap.c |   45 ++++++++++++++++++++++++++++++++++++++++++---
>>>  1 file changed, 42 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
>>> index 0557529..db3c732 100644
>>> --- a/drivers/gpio/gpio-omap.c
>>> +++ b/drivers/gpio/gpio-omap.c
>>> @@ -70,6 +70,7 @@ struct gpio_bank {
>>>  	bool is_mpuio;
>>>  	bool dbck_flag;
>>>  	bool loses_context;
>>> +	bool context_valid;
>>>  	int stride;
>>>  	u32 width;
>>>  	int context_loss_count;
>>> @@ -1129,6 +1130,7 @@ static int omap_gpio_probe(struct platform_device *pdev)
>>>  			bank->loses_context = true;
>>>  	} else {
>>>  		bank->loses_context = pdata->loses_context;
>>> +		bank->get_context_loss_count = pdata->get_context_loss_count;
>> 
>> Still need to check loses_context for populating
>> get_context_loss_count here. Updated patch below.
>> 
>> Jon
>> 
>> From d02ef7b7dfcf8e13bf019aedfdecb38ca3c6749f Mon Sep 17 00:00:00 2001
>> From: Jon Hunter <jon-hunter@ti.com>
>> Date: Mon, 15 Apr 2013 13:06:54 -0500
>> Subject: [PATCH] gpio/omap: ensure gpio context is initialised
>> 
>> Commit a2797be (gpio/omap: force restore if context loss is not
>> detectable) broke gpio support for OMAP when booting with device-tree
>> because a restore of the gpio context being performed without ever
>> initialising the gpio context. In other words, the context restored was
>> bad.
>> 
>> This problem could also occur in the non device-tree case, however, it
>> is much less likely because when booting without device-tree we can
>> detect context loss via a platform specific API and so context restore
>> is performed less often.
>> 
>> Nevertheless we should ensure that the gpio context is initialised
>> on the first pm-runtime resume for gpio banks that could lose their
>> state regardless of whether we are booting with device-tree or not.
>> 
>> The context loss count was being initialised on the first pm-runtime
>> suspend following a resume, by populating the get_count_loss_count()
>> function pointer after the first pm-runtime resume. To make the code
>> more readable and logical, initialise the context loss count on the
>> first pm-runtime resume if the context is not yet valid.
>> 
>> Reported-by: Tony Lindgren <tony@atomide.com>
>> Signed-off-by: Jon Hunter <jon-hunter@ti.com>
>> ---
> This version looks better than the first one for sure. I am still not
> happy with per bank "context_valid" flag whose job just ends after
> the probe. 

Assuming this driver could become a module someday (not terribly likely,
I know), but context_valid would have meaning for each module reload.

> But then I do agree with you about the global flag might
> be fragile and less maintainable.
>
> So, FWIW,
> Acked-by: Santosh Shilimkar<santosh.shilimkar@ti.com>

Yes, this version is more readable.  Thanks for the update.

Reviewed-by: Kevin Hilman <khilman@linaro.org>