| Message ID | 20250411154419.1379529-1-elder@riscstar.com |
|---|---|
| Headers | show |
| Series | serial: 8250_of: support an optional bus clock | expand |
On Fri, Apr 11, 2025 at 10:36 PM Alex Elder <elder@riscstar.com> wrote: > On 4/11/25 2:30 PM, Andy Shevchenko wrote: > > Fri, Apr 11, 2025 at 10:44:18AM -0500, Alex Elder kirjoitti: > >> Save the bus clock pointer in the of_serial_info structure, and use > >> that to disable the bus clock on suspend and re-enable it on resume. ... > >> if (!port->uartclk) { > >> - struct clk *bus_clk; > >> - > >> - bus_clk = devm_clk_get_optional_enabled(dev, "bus"); > >> - if (IS_ERR(bus_clk)) { > >> - ret = dev_err_probe(dev, PTR_ERR(bus_clk), "failed to get bus clock\n"); > >> + info->bus_clk = devm_clk_get_optional_enabled(dev, "bus"); > >> + if (IS_ERR(info->bus_clk)) { > >> + ret = dev_err_probe(dev, PTR_ERR(info->bus_clk), > >> + "failed to get bus clock\n"); > >> goto err_pmruntime; > >> } > >> > >> /* If the bus clock is required, core clock must be named */ > >> - info->clk = devm_clk_get_enabled(dev, bus_clk ? "core" : NULL); > >> + info->clk = devm_clk_get_enabled(dev, info->bus_clk ? "core" : NULL); > >> if (IS_ERR(info->clk)) { > >> ret = dev_err_probe(dev, PTR_ERR(info->clk), "failed to get clock\n"); > > > > While the first patch against this file looks okay now, this one inherits the > > same problem (seems like not enought thinking about the code representation). > > > > Instead of rewritting half of the lines you just introduced (which is also a > > bad practice), add a one-liner that assigns a field to the local variable. > > So you want me to re-spin this again so that I use the local variable? Yes. > I understand what you're saying based on ease of review, No, not only review. Here the main issue is ping-pong between patches. If you know ahead that these lines should be changed, do it from the start. But I understand the need to have separate changes for logically different pieces. That's why > but this > is a simple patch It doesn't matter how simple it is. > and the change is very understandable, and the > code is no more or less clear when using the local variable. See above. > >> goto err_pmruntime;
On 4/11/25 2:44 PM, Andy Shevchenko wrote: > On Fri, Apr 11, 2025 at 10:36 PM Alex Elder <elder@riscstar.com> wrote: >> On 4/11/25 2:30 PM, Andy Shevchenko wrote: >>> Fri, Apr 11, 2025 at 10:44:18AM -0500, Alex Elder kirjoitti: >>>> Save the bus clock pointer in the of_serial_info structure, and use >>>> that to disable the bus clock on suspend and re-enable it on resume. > > ... > >>>> if (!port->uartclk) { >>>> - struct clk *bus_clk; >>>> - >>>> - bus_clk = devm_clk_get_optional_enabled(dev, "bus"); >>>> - if (IS_ERR(bus_clk)) { >>>> - ret = dev_err_probe(dev, PTR_ERR(bus_clk), "failed to get bus clock\n"); >>>> + info->bus_clk = devm_clk_get_optional_enabled(dev, "bus"); >>>> + if (IS_ERR(info->bus_clk)) { >>>> + ret = dev_err_probe(dev, PTR_ERR(info->bus_clk), >>>> + "failed to get bus clock\n"); >>>> goto err_pmruntime; >>>> } >>>> >>>> /* If the bus clock is required, core clock must be named */ >>>> - info->clk = devm_clk_get_enabled(dev, bus_clk ? "core" : NULL); >>>> + info->clk = devm_clk_get_enabled(dev, info->bus_clk ? "core" : NULL); >>>> if (IS_ERR(info->clk)) { >>>> ret = dev_err_probe(dev, PTR_ERR(info->clk), "failed to get clock\n"); >>> >>> While the first patch against this file looks okay now, this one inherits the >>> same problem (seems like not enought thinking about the code representation). >>> >>> Instead of rewritting half of the lines you just introduced (which is also a >>> bad practice), add a one-liner that assigns a field to the local variable. >> >> So you want me to re-spin this again so that I use the local variable? > > Yes. > >> I understand what you're saying based on ease of review, > > No, not only review. Here the main issue is ping-pong between patches. > If you know ahead that these lines should be changed, do it from the > start. But I understand the need to have separate changes for > logically different pieces. That's why I did do it from the start (v1 of the series). Then I reworked it based on your feedback and concluded there was no need for keeping a copy of the bus clock in the of_serial_info structure. In the interest of keeping the code simple I dropped it. Yixun asked me to disable/enable the bus clock in suspend/resume, so I was preparing the patches for that, when Greg informed me he had already accepted the first two patches. So I added this new feature on the end. This is the reason for this "ping pong." I would have done it from the start in this series otherwise in v3. Anyway, I'm sending v4 shortly. I just have to run it through another test. It is indeed a much simpler change. -Alex >> but this >> is a simple patch > > It doesn't matter how simple it is. > >> and the change is very understandable, and the >> code is no more or less clear when using the local variable. > > See above. > >>>> goto err_pmruntime; > >
The SpacemiT UART hardware requires a bus clock to be enabled in addition to the primary function clock. This series makes it possible to specify both clocks via DTS. If a bus clock is required, it and the primary clock are fetched by name. Changes between v2 and v3: - A third patch was added to disable the bus clock on suspend and enable it again on resume, as requested by Yixun Lan - Rob's Reviewed-by tag has been added to patch 1 - The first two patches are otherwise identical to what was in v2 Here is version 2 of this series: https://lore.kernel.org/lkml/20250409192213.1130181-1-elder@riscstar.com/ Changes between v1 and v2: - The DT binding was fixed to properly define the number of clocks and clock names based on the compatible string, as suggested by Rob Herring - Logic to look up the optional bus clock was changed as requested by Andy Shevchenko - The bus clock pointer (which was never used after it was enabled) was renmoved from the of_serial_info structure Here is the first version of this series: https://lore.kernel.org/lkml/20250408175146.979557-1-elder@riscstar.com/ -Alex Alex Elder (3): dt-bindings: serial: 8250: support an optional second clock serial: 8250_of: add support for an optional bus clock serial: 8250_of: manage bus clock in suspend/resume .../devicetree/bindings/serial/8250.yaml | 30 ++++++++++++++++++- drivers/tty/serial/8250/8250_of.c | 13 +++++++- 2 files changed, 41 insertions(+), 2 deletions(-) base-commit: 0af2f6be1b4281385b618cb86ad946eded089ac8