Re: [PATCH 4/8] tty: atmel_serial: prepare clk before calling enable

[Nicolas Ferre <nicolas.ferre@atmel.com>]

On 20/06/2013 10:06, boris brezillon :
> On 20/06/2013 09:48, Nicolas Ferre wrote:
>> On 19/06/2013 13:17, Boris BREZILLON :
>>> Replace clk_enable/disable with clk_prepare_enable/disable_unprepare to
>>> avoid common clk framework warnings.
>>>
>>> Signed-off-by: Boris BREZILLON <b.brezillon@overkiz.com>
>>
>> Acked-by: Nicolas Ferre <nicolas.ferre@atmel.com>
>>
>> (one comment though)
>>
>>> ---
>>>   drivers/tty/serial/atmel_serial.c |   41
>>> ++++++++++++++++++++++++++++---------
>>>   1 file changed, 31 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/drivers/tty/serial/atmel_serial.c
>>> b/drivers/tty/serial/atmel_serial.c
>>> index 3467462..691265f 100644
>>> --- a/drivers/tty/serial/atmel_serial.c
>>> +++ b/drivers/tty/serial/atmel_serial.c
>>> @@ -1100,7 +1100,7 @@ static void atmel_serial_pm(struct uart_port
>>> *port, unsigned int state,
>>>            * Enable the peripheral clock for this serial port.
>>>            * This is called on uart_open() or a resume event.
>>>            */
>>> -        clk_enable(atmel_port->clk);
>>> +        clk_prepare_enable(atmel_port->clk);
>>
>> Do we need to check return code here?
> We could, but the pm function will not reflect the failure (void return).
> In addition, the clk_prepare_enable should not fail because it already
> succeed in the probe
> function (no memory allocation, simple bit masking in clk-peripheral
> implem).
>
> I can add it add this check (and avoid the interrrupt activation if it
> fails) if you want.

No, it seems okay like that. Thanks for the precision.

Bye,

>
>>
>>>
>>>           /* re-enable interrupts if we disabled some on suspend */
>>>           UART_PUT_IER(port, atmel_port->backup_imr);
>>> @@ -1114,7 +1114,7 @@ static void atmel_serial_pm(struct uart_port
>>> *port, unsigned int state,
>>>            * Disable the peripheral clock for this serial port.
>>>            * This is called on uart_close() or a suspend event.
>>>            */
>>> -        clk_disable(atmel_port->clk);
>>> +        clk_disable_unprepare(atmel_port->clk);
>>>           break;
>>>       default:
>>>           printk(KERN_ERR "atmel_serial: unknown pm %d\n", state);
>>> @@ -1458,9 +1458,10 @@ static void atmel_of_init_port(struct
>>> atmel_uart_port *atmel_port,
>>>   /*
>>>    * Configure the port from the platform device resource info.
>>>    */
>>> -static void atmel_init_port(struct atmel_uart_port *atmel_port,
>>> +static int atmel_init_port(struct atmel_uart_port *atmel_port,
>>>                         struct platform_device *pdev)
>>>   {
>>> +    int ret;
>>>       struct uart_port *port = &atmel_port->uart;
>>>       struct atmel_uart_data *pdata = pdev->dev.platform_data;
>>>
>>> @@ -1496,9 +1497,19 @@ static void atmel_init_port(struct
>>> atmel_uart_port *atmel_port,
>>>       /* for console, the clock could already be configured */
>>>       if (!atmel_port->clk) {
>>>           atmel_port->clk = clk_get(&pdev->dev, "usart");
>>> -        clk_enable(atmel_port->clk);
>>> +        if (IS_ERR(atmel_port->clk)) {
>>> +            ret = PTR_ERR(atmel_port->clk);
>>> +            atmel_port->clk = NULL;
>>> +            return ret;
>>> +        }
>>> +        ret = clk_prepare_enable(atmel_port->clk);
>>> +        if (ret) {
>>> +            clk_put(atmel_port->clk);
>>> +            atmel_port->clk = NULL;
>>> +            return ret;
>>> +        }
>>>           port->uartclk = clk_get_rate(atmel_port->clk);
>>> -        clk_disable(atmel_port->clk);
>>> +        clk_disable_unprepare(atmel_port->clk);
>>>           /* only enable clock when USART is in use */
>>>       }
>>>
>>> @@ -1511,6 +1522,8 @@ static void atmel_init_port(struct
>>> atmel_uart_port *atmel_port,
>>>       } else {
>>>           atmel_port->tx_done_mask = ATMEL_US_TXRDY;
>>>       }
>>> +
>>> +    return 0;
>>>   }
>>>
>>>   struct platform_device *atmel_default_console_device;    /* the
>>> serial console device */
>>> @@ -1601,6 +1614,7 @@ static void __init
>>> atmel_console_get_options(struct uart_port *port, int *baud,
>>>
>>>   static int __init atmel_console_setup(struct console *co, char
>>> *options)
>>>   {
>>> +    int ret;
>>>       struct uart_port *port = &atmel_ports[co->index].uart;
>>>       int baud = 115200;
>>>       int bits = 8;
>>> @@ -1612,7 +1626,9 @@ static int __init atmel_console_setup(struct
>>> console *co, char *options)
>>>           return -ENODEV;
>>>       }
>>>
>>> -    clk_enable(atmel_ports[co->index].clk);
>>> +    ret = clk_prepare_enable(atmel_ports[co->index].clk);
>>> +    if (ret)
>>> +        return ret;
>>>
>>>       UART_PUT_IDR(port, -1);
>>>       UART_PUT_CR(port, ATMEL_US_RSTSTA | ATMEL_US_RSTRX);
>>> @@ -1645,6 +1661,7 @@ static struct console atmel_console = {
>>>    */
>>>   static int __init atmel_console_init(void)
>>>   {
>>> +    int ret;
>>>       if (atmel_default_console_device) {
>>>           struct atmel_uart_data *pdata =
>>> atmel_default_console_device->dev.platform_data;
>>> @@ -1655,7 +1672,9 @@ static int __init atmel_console_init(void)
>>>           port->uart.line = id;
>>>
>>>           add_preferred_console(ATMEL_DEVICENAME, id, NULL);
>>> -        atmel_init_port(port, atmel_default_console_device);
>>> +        ret = atmel_init_port(port, atmel_default_console_device);
>>> +        if (ret)
>>> +            return ret;
>>>           register_console(&atmel_console);
>>>       }
>>>
>>> @@ -1786,7 +1805,9 @@ static int atmel_serial_probe(struct
>>> platform_device *pdev)
>>>       port->backup_imr = 0;
>>>       port->uart.line = ret;
>>>
>>> -    atmel_init_port(port, pdev);
>>> +    ret = atmel_init_port(port, pdev);
>>> +    if (ret)
>>> +        goto err;
>>>
>>>       pinctrl = devm_pinctrl_get_select_default(&pdev->dev);
>>>       if (IS_ERR(pinctrl)) {
>>> @@ -1812,9 +1833,9 @@ static int atmel_serial_probe(struct
>>> platform_device *pdev)
>>>               && ATMEL_CONSOLE_DEVICE->flags & CON_ENABLED) {
>>>           /*
>>>            * The serial core enabled the clock for us, so undo
>>> -         * the clk_enable() in atmel_console_setup()
>>> +         * the clk_prepare_enable() in atmel_console_setup()
>>>            */
>>> -        clk_disable(port->clk);
>>> +        clk_disable_unprepare(port->clk);
>>>       }
>>>   #endif
>>>
>>>
>>
>>
>
>
>


-- 
Nicolas Ferre