[v2,4/6] mfd: atmel-hlcdc: return in case of error
diff mbox series

Message ID 1576249496-4849-5-git-send-email-claudiu.beznea@microchip.com
State New
Headers show
Series
  • fixes for atmel-hlcdc
Related show

Commit Message

Claudiu Beznea Dec. 13, 2019, 3:04 p.m. UTC
For HLCDC timing engine configurations bit ATMEL_HLCDC_SIP of
ATMEL_HLCDC_SR needs to be polled before applying new config. In case of
timeout there is no indicator about this, so, return in case of timeout
and also print a message about this.

Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
---
 drivers/mfd/atmel-hlcdc.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

Comments

Lee Jones Dec. 16, 2019, 4:22 p.m. UTC | #1
On Fri, 13 Dec 2019, Claudiu Beznea wrote:

> For HLCDC timing engine configurations bit ATMEL_HLCDC_SIP of
> ATMEL_HLCDC_SR needs to be polled before applying new config. In case of
> timeout there is no indicator about this, so, return in case of timeout
> and also print a message about this.
> 
> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
> ---
>  drivers/mfd/atmel-hlcdc.c | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)

For my own reference:
  Acked-for-MFD-by: Lee Jones <lee.jones@linaro.org>
Lee Jones Dec. 16, 2019, 4:24 p.m. UTC | #2
On Fri, 13 Dec 2019, Claudiu Beznea wrote:

> For HLCDC timing engine configurations bit ATMEL_HLCDC_SIP of
> ATMEL_HLCDC_SR needs to be polled before applying new config. In case of
> timeout there is no indicator about this, so, return in case of timeout
> and also print a message about this.
> 
> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
> ---
>  drivers/mfd/atmel-hlcdc.c | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/mfd/atmel-hlcdc.c b/drivers/mfd/atmel-hlcdc.c
> index 92bfcaa62ace..a1e46c87b956 100644
> --- a/drivers/mfd/atmel-hlcdc.c
> +++ b/drivers/mfd/atmel-hlcdc.c
> @@ -40,10 +40,17 @@ static int regmap_atmel_hlcdc_reg_write(void *context, unsigned int reg,
>  
>  	if (reg <= ATMEL_HLCDC_DIS) {
>  		u32 status;
> -
> -		readl_poll_timeout_atomic(hregmap->regs + ATMEL_HLCDC_SR,
> -					  status, !(status & ATMEL_HLCDC_SIP),
> -					  1, 100);
> +		int ret;
> +
> +		ret = readl_poll_timeout_atomic(hregmap->regs + ATMEL_HLCDC_SR,
> +						status,
> +						!(status & ATMEL_HLCDC_SIP),
> +						1, 100);
> +		if (ret) {
> +			dev_err(hregmap->dev,
> +				"Timeout waiting for ATMEL_HLCDC_SIP\n");

Nit: Just in case you have to rework this, placing register names in
the kernel log isn't usually helpful.  Can you swap it out for a more
user friendly message?

"Timed out waiting for ..."

... X status
... Y to update
... setting Z configuration

Etc.

> +			return ret;
> +		}
>  	}
>  
>  	writel(val, hregmap->regs + reg);
Claudiu Beznea Dec. 18, 2019, 10:02 a.m. UTC | #3
On 16.12.2019 18:24, Lee Jones wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Fri, 13 Dec 2019, Claudiu Beznea wrote:
> 
>> For HLCDC timing engine configurations bit ATMEL_HLCDC_SIP of
>> ATMEL_HLCDC_SR needs to be polled before applying new config. In case of
>> timeout there is no indicator about this, so, return in case of timeout
>> and also print a message about this.
>>
>> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
>> ---
>>  drivers/mfd/atmel-hlcdc.c | 15 +++++++++++----
>>  1 file changed, 11 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/mfd/atmel-hlcdc.c b/drivers/mfd/atmel-hlcdc.c
>> index 92bfcaa62ace..a1e46c87b956 100644
>> --- a/drivers/mfd/atmel-hlcdc.c
>> +++ b/drivers/mfd/atmel-hlcdc.c
>> @@ -40,10 +40,17 @@ static int regmap_atmel_hlcdc_reg_write(void *context, unsigned int reg,
>>
>>       if (reg <= ATMEL_HLCDC_DIS) {
>>               u32 status;
>> -
>> -             readl_poll_timeout_atomic(hregmap->regs + ATMEL_HLCDC_SR,
>> -                                       status, !(status & ATMEL_HLCDC_SIP),
>> -                                       1, 100);
>> +             int ret;
>> +
>> +             ret = readl_poll_timeout_atomic(hregmap->regs + ATMEL_HLCDC_SR,
>> +                                             status,
>> +                                             !(status & ATMEL_HLCDC_SIP),
>> +                                             1, 100);
>> +             if (ret) {
>> +                     dev_err(hregmap->dev,
>> +                             "Timeout waiting for ATMEL_HLCDC_SIP\n");
> 
> Nit: Just in case you have to rework this, placing register names in
> the kernel log isn't usually helpful.  Can you swap it out for a more
> user friendly message?

Sure! I'll do a v3 for it.

Thank you,
Claudiu Beznea

> 
> "Timed out waiting for ..."
> 
> ... X status
> ... Y to update
> ... setting Z configuration
> 
> Etc.
> 
>> +                     return ret;
>> +             }
>>       }
>>
>>       writel(val, hregmap->regs + reg);
> 
> --
> Lee Jones [李琼斯]
> Linaro Services Technical Lead
> Linaro.org │ Open source software for ARM SoCs
> Follow Linaro: Facebook | Twitter | Blog
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>

Patch
diff mbox series

diff --git a/drivers/mfd/atmel-hlcdc.c b/drivers/mfd/atmel-hlcdc.c
index 92bfcaa62ace..a1e46c87b956 100644
--- a/drivers/mfd/atmel-hlcdc.c
+++ b/drivers/mfd/atmel-hlcdc.c
@@ -40,10 +40,17 @@  static int regmap_atmel_hlcdc_reg_write(void *context, unsigned int reg,
 
 	if (reg <= ATMEL_HLCDC_DIS) {
 		u32 status;
-
-		readl_poll_timeout_atomic(hregmap->regs + ATMEL_HLCDC_SR,
-					  status, !(status & ATMEL_HLCDC_SIP),
-					  1, 100);
+		int ret;
+
+		ret = readl_poll_timeout_atomic(hregmap->regs + ATMEL_HLCDC_SR,
+						status,
+						!(status & ATMEL_HLCDC_SIP),
+						1, 100);
+		if (ret) {
+			dev_err(hregmap->dev,
+				"Timeout waiting for ATMEL_HLCDC_SIP\n");
+			return ret;
+		}
 	}
 
 	writel(val, hregmap->regs + reg);