| Message ID | 20250506180232.1299-6-quic_ptalari@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | Enable QUPs and Serial on SA8255p Qualcomm platforms | expand |
On 06/05/2025 19:02, 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> > --- > v3 -> v4 > - added version log after --- > > v1 -> v2 > - returned 0 instead of ret variable > --- > drivers/tty/serial/qcom_geni_serial.c | 54 +++++++++++++++++++++------ > 1 file changed, 42 insertions(+), 12 deletions(-) > > diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c > index 6ad759146f71..2cd2085473f3 100644 > --- a/drivers/tty/serial/qcom_geni_serial.c > +++ b/drivers/tty/serial/qcom_geni_serial.c > @@ -1588,6 +1588,42 @@ static struct uart_driver qcom_geni_uart_driver = { > .nr = GENI_UART_PORTS, > }; > > +static int geni_serial_resources_off(struct uart_port *uport) It seems like an extremely nit-picky thing to say BUT geni_serial_resources_on(); geni_serial_resources_off(); since that is the order you use in the code below. > +{ > + 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 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; You're adding additional logical checks here, which seem reasonable but are not stated in your commit log. Please make clear in the commit log that additional, minor function return checks are added as you do your functional decomposition. > + > + 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 0; > +} > + > static int geni_serial_resource_init(struct qcom_geni_serial_port *port) > { > int ret; > @@ -1628,23 +1664,17 @@ static int geni_serial_resource_init(struct qcom_geni_serial_port *port) > static void qcom_geni_serial_pm(struct uart_port *uport, > unsigned int new_state, unsigned int old_state) > { > - struct qcom_geni_serial_port *port = to_dev_port(uport); > > /* If we've never been called, treat it as off */ > if (old_state == UART_PM_STATE_UNDEFINED) > old_state = UART_PM_STATE_OFF; > > - if (new_state == UART_PM_STATE_ON && old_state == UART_PM_STATE_OFF) { > - geni_icc_enable(&port->se); > - if (port->clk_rate) > - dev_pm_opp_set_rate(uport->dev, port->clk_rate); > - geni_se_resources_on(&port->se); > - } else if (new_state == UART_PM_STATE_OFF && > - old_state == UART_PM_STATE_ON) { > - geni_se_resources_off(&port->se); > - dev_pm_opp_set_rate(uport->dev, 0); > - geni_icc_disable(&port->se); > - } > + if (new_state == UART_PM_STATE_ON && old_state == UART_PM_STATE_OFF) > + geni_serial_resources_on(uport); > + else if (new_state == UART_PM_STATE_OFF && > + old_state == UART_PM_STATE_ON) > + geni_serial_resources_off(uport); > + > } > > static const struct uart_ops qcom_geni_console_pops = { > -- > 2.17.1 > > Assuming you address my points.
On 03/06/2025 15:28, Bryan O'Donoghue wrote: >> 2.17.1 >> >> > Assuming you address my points. [sic] Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
On 03/06/2025 15:29, Bryan O'Donoghue wrote: > On 03/06/2025 15:28, Bryan O'Donoghue wrote: >>> 2.17.1 >>> >>> >> Assuming you address my points. > > [sic] > > Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> > Oh please fix this in the next version checkpatch.pl --strict mypatch.patch CHECK: Alignment should match open parenthesis #92: FILE: drivers/tty/serial/qcom_geni_serial.c:1675: + else if (new_state == UART_PM_STATE_OFF && + old_state == UART_PM_STATE_ON) total: 0 errors, 0 warnings, 1 checks, 71 lines checked NOTE: For some of the reported defects, checkpatch may be able to mechanically convert to the typical style using --fix or --fix-inplace. 0005-serial-qcom-geni-move-resource-control-logic-to-sepa.patch has style problems, please review. --- bod
Hi Bryan On 6/3/2025 8:16 PM, Bryan O'Donoghue wrote: > On 03/06/2025 15:29, Bryan O'Donoghue wrote: >> On 03/06/2025 15:28, Bryan O'Donoghue wrote: >>>> 2.17.1 >>>> >>>> >>> Assuming you address my points. >> >> [sic] >> >> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> >> > > Oh please fix this in the next version can i fix this in separate patch since i haven't touch these two lines or fix within same patch? Thanks, Praveen Talari > > checkpatch.pl --strict mypatch.patch > > CHECK: Alignment should match open parenthesis > #92: FILE: drivers/tty/serial/qcom_geni_serial.c:1675: > + else if (new_state == UART_PM_STATE_OFF && > + old_state == UART_PM_STATE_ON) > > total: 0 errors, 0 warnings, 1 checks, 71 lines checked > > NOTE: For some of the reported defects, checkpatch may be able to > mechanically convert to the typical style using --fix or > --fix-inplace. > > 0005-serial-qcom-geni-move-resource-control-logic-to-sepa.patch has > style problems, please review. > > --- > bod
Hi Bryan On 6/4/2025 7:39 PM, Praveen Talari wrote: > Hi Bryan > > On 6/3/2025 8:16 PM, Bryan O'Donoghue wrote: >> On 03/06/2025 15:29, Bryan O'Donoghue wrote: >>> On 03/06/2025 15:28, Bryan O'Donoghue wrote: >>>>> 2.17.1 >>>>> >>>>> >>>> Assuming you address my points. >>> >>> [sic] >>> >>> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> >>> >> >> Oh please fix this in the next version > can i fix this in separate patch since i haven't touch these two lines Sorry bit confused. Will fix this in the same patch. Thanks, Praveen Talari > or > fix within same patch? > > Thanks, > Praveen Talari >> >> checkpatch.pl --strict mypatch.patch >> >> CHECK: Alignment should match open parenthesis >> #92: FILE: drivers/tty/serial/qcom_geni_serial.c:1675: >> + else if (new_state == UART_PM_STATE_OFF && >> + old_state == UART_PM_STATE_ON) >> >> total: 0 errors, 0 warnings, 1 checks, 71 lines checked >> >> NOTE: For some of the reported defects, checkpatch may be able to >> mechanically convert to the typical style using --fix or >> --fix-inplace. >> >> 0005-serial-qcom-geni-move-resource-control-logic-to-sepa.patch has >> style problems, please review. >> >> --- >> bod
diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c index 6ad759146f71..2cd2085473f3 100644 --- a/drivers/tty/serial/qcom_geni_serial.c +++ b/drivers/tty/serial/qcom_geni_serial.c @@ -1588,6 +1588,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 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 0; +} + static int geni_serial_resource_init(struct qcom_geni_serial_port *port) { int ret; @@ -1628,23 +1664,17 @@ static int geni_serial_resource_init(struct qcom_geni_serial_port *port) static void qcom_geni_serial_pm(struct uart_port *uport, unsigned int new_state, unsigned int old_state) { - struct qcom_geni_serial_port *port = to_dev_port(uport); /* If we've never been called, treat it as off */ if (old_state == UART_PM_STATE_UNDEFINED) old_state = UART_PM_STATE_OFF; - if (new_state == UART_PM_STATE_ON && old_state == UART_PM_STATE_OFF) { - geni_icc_enable(&port->se); - if (port->clk_rate) - dev_pm_opp_set_rate(uport->dev, port->clk_rate); - geni_se_resources_on(&port->se); - } else if (new_state == UART_PM_STATE_OFF && - old_state == UART_PM_STATE_ON) { - geni_se_resources_off(&port->se); - dev_pm_opp_set_rate(uport->dev, 0); - geni_icc_disable(&port->se); - } + if (new_state == UART_PM_STATE_ON && old_state == UART_PM_STATE_OFF) + geni_serial_resources_on(uport); + else if (new_state == UART_PM_STATE_OFF && + old_state == UART_PM_STATE_ON) + geni_serial_resources_off(uport); + } static const struct uart_ops qcom_geni_console_pops = {
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> --- v3 -> v4 - added version log after --- v1 -> v2 - returned 0 instead of ret variable --- drivers/tty/serial/qcom_geni_serial.c | 54 +++++++++++++++++++++------ 1 file changed, 42 insertions(+), 12 deletions(-)