Re: [PATCH v1 6/9] serial: qcom-geni: move resource control logic to separate functions

[Praveen Talari <quic_ptalari@quicinc.com>]

Hi

On 4/14/2025 1:29 PM, Jiri Slaby wrote:
> On 10. 04. 25, 19:40, Praveen Talari wrote:
>> Supports use in PM system/runtime frameworks, helping to
>> distinguish new resource control mechanisms and facilitate
>> future modifications within the new API.
>>
>> The code that handles the actual enable or disable of resources
>> like clock and ICC paths to a separate function
>> (geni_serial_resources_on() and geni_serial_resources_off()) which
>> enhances code readability.
>>
>> Signed-off-by: Praveen Talari <quic_ptalari@quicinc.com>
>> ---
>>   drivers/tty/serial/qcom_geni_serial.c | 53 +++++++++++++++++++++------
>>   1 file changed, 42 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/tty/serial/qcom_geni_serial.c 
>> b/drivers/tty/serial/qcom_geni_serial.c
>> index 889ce8961e0a..e341f5090ecc 100644
>> --- a/drivers/tty/serial/qcom_geni_serial.c
>> +++ b/drivers/tty/serial/qcom_geni_serial.c
>> @@ -1572,6 +1572,42 @@ static struct uart_driver 
>> qcom_geni_uart_driver = {
>>       .nr =  GENI_UART_PORTS,
>>   };
>>   +static int geni_serial_resources_off(struct uart_port *uport)
>> +{
>> +    struct qcom_geni_serial_port *port = to_dev_port(uport);
>> +    int ret;
>> +
>> +    dev_pm_opp_set_rate(uport->dev, 0);
>> +    ret = geni_se_resources_off(&port->se);
>> +    if (ret)
>> +        return ret;
>> +
>> +    geni_icc_disable(&port->se);
>> +
>> +    return ret;
>
> This is a bit confusing (needs context). return 0 directly.
here "ret" is also pointing to 0. Why can't we use "ret" directly 
instead of 0.
>
>> +}
>> +
>> +static int geni_serial_resources_on(struct uart_port *uport)
>> +{
>> +    struct qcom_geni_serial_port *port = to_dev_port(uport);
>> +    int ret;
>> +
>> +    ret = geni_icc_enable(&port->se);
>> +    if (ret)
>> +        return ret;
>> +
>> +    ret = geni_se_resources_on(&port->se);
>> +    if (ret) {
>> +        geni_icc_disable(&port->se);
>> +        return ret;
>> +    }
>> +
>> +    if (port->clk_rate)
>> +        dev_pm_opp_set_rate(uport->dev, port->clk_rate);
>> +
>> +    return ret;
>
> Same here.
>
> thanks,