Re: [PATCH v4 08/13] serial: sh-sci: Introduced function pointers

Re: [PATCH v4 08/13] serial: sh-sci: Introduced function pointers

[Geert Uytterhoeven]

Hi Thierry,

On Thu, 6 Mar 2025 at 16:26, Thierry Bultel
<thierry.bultel.yh@bp.renesas.com> wrote:
> The aim here is to prepare support for new sci controllers like
> the T2H/RSCI whose registers are too much different for being
> handled in common code.
>
> This named serial controller also has 32 bits register,
> so some return types had to be changed.
>
> The needed generic functions are no longer static, with prototypes
> defined in sh-sci-common.h so that they can be used from specific
> implementation in a separate file, to keep this driver as little
> changed as possible.
>
> For doing so, a set of 'ops' is added to struct sci_port.
>
> Signed-off-by: Thierry Bultel <thierry.bultel.yh@bp.renesas.com>
> ---
> Changes v3->v4:
>    - Add missing #include <bitfield.h>
>    - Remove sci_poll_get_char sci_poll_put_char from sh-sci-common.h (both
>      function are not used by rzsci yet).
>    - Add missing #ifdef around .poll_put_char pointer initialization.
>    - More registers to save & restore due to rebase on tty-next

Thanks for the update!

While most rough edges have been polished by now (thanks!), and the
driver seems to still work on a variety of platforms, I am still
worried about the impact of this change:
  - Maintainability and future bug fixing?
  - Performance of the additional indirections on slow platforms (SH)?

What do other people think?
Thanks!

Full series:
https://lore.kernel.org/all/20250306152451.2356762-1-thierry.bultel.yh@bp.renesas.com
My initial comments and RFC:
https://lore.kernel.org/all/CAMuHMdVD1dLP53V_zOhxpqazDdPDVafJ6iohY8u6WPQrmYH5Sw@mail.gmail.com

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v4 08/13] serial: sh-sci: Introduced function pointers

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1428 bytes --]

Hi all,

sorry for missing this series so far and thanks to Geert for pulling me
into the loop.

> While most rough edges have been polished by now (thanks!), and the
> driver seems to still work on a variety of platforms, I am still
> worried about the impact of this change:
>   - Maintainability and future bug fixing?

I hate to see development work going to waste, yet I have to say I am
also concerned about the maintainability of this driver after this very
intrusive changeset. The driver is already quite complex. Adding another
layer of complexity (function pointers) will make proper bugfixing for
all supported instances quite harder, I'd think.

Has it been discussed to have this as a separate driver? Were there
reasons against it? This is really an open question. Maybe it is
justified to do it like this if we have reasons for it.

Seeing that SCI core needs 800+ lines changed and we still have a
seperate driver with 460 lines driver, I do wonder if copying the logic
from SCI core to a seperate driver would make sense. I am aware that the
core has currently 3500+ lines currently. I'd estimate it would shrink
quite a bit when copying because you won't need to handle all the
differences to other SCI entries.

Again, this is not a request to follow my suggestion, it is an open
question to make sure all paths have been considered.

Thanks and happy hacking,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v4 08/13] serial: sh-sci: Introduced function pointers

[Paul Barker]

[-- Attachment #1.1.1: Type: text/plain, Size: 2490 bytes --]

On 17/03/2025 09:55, Wolfram Sang wrote:
> Hi all,
> 
> sorry for missing this series so far and thanks to Geert for pulling me
> into the loop.
> 
>> While most rough edges have been polished by now (thanks!), and the
>> driver seems to still work on a variety of platforms, I am still
>> worried about the impact of this change:
>>   - Maintainability and future bug fixing?
> 
> I hate to see development work going to waste, yet I have to say I am
> also concerned about the maintainability of this driver after this very
> intrusive changeset. The driver is already quite complex. Adding another
> layer of complexity (function pointers) will make proper bugfixing for
> all supported instances quite harder, I'd think.
> 
> Has it been discussed to have this as a separate driver? Were there
> reasons against it? This is really an open question. Maybe it is
> justified to do it like this if we have reasons for it.
> 
> Seeing that SCI core needs 800+ lines changed and we still have a
> seperate driver with 460 lines driver, I do wonder if copying the logic
> from SCI core to a seperate driver would make sense. I am aware that the
> core has currently 3500+ lines currently. I'd estimate it would shrink
> quite a bit when copying because you won't need to handle all the
> differences to other SCI entries.
> 
> Again, this is not a request to follow my suggestion, it is an open
> question to make sure all paths have been considered.

Hi Geert, Wolfram,

Thierry is out of the office this week so we can follow this up next
week, but I do want to give some input in the meantime.

We discussed both approaches internally and did an initial
proof-of-concept of a separate driver. The result was over 1,000 lines
of code copy-pasted from the existing sh-sci driver into the new driver,
which is generally something maintainers want us to avoid doing. The
trade off here is whether we want a single more complex driver, or two
copies of much of the code so that bugfixes/improvements to the common
sections in the future need to be duplicated.

The RZ/V2H and RZ/G3E have interfaces of both the existing sh-sci
register layout ("SCIF" ports in RZ/V2H & RZ/G3E manual) and the RZ/T2H
style register layout ("RSCI" ports in RZ/V2H manual, "SCI" ports in
RZ/G3E manual), so keeping things closely aligned as we move forward
will be beneficial. I expect that this will be easier with a combined
driver.

Thanks,

-- 
Paul Barker

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3577 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

Re: [PATCH v4 08/13] serial: sh-sci: Introduced function pointers

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 961 bytes --]

Hi Paul,

> We discussed both approaches internally and did an initial
> proof-of-concept of a separate driver. The result was over 1,000 lines
> of code copy-pasted from the existing sh-sci driver into the new driver,
> which is generally something maintainers want us to avoid doing.

Darn, 1000 lines of logic is a lot...

> trade off here is whether we want a single more complex driver, or two
> copies of much of the code so that bugfixes/improvements to the common
> sections in the future need to be duplicated.

Exactly.

> The RZ/V2H and RZ/G3E have interfaces of both the existing sh-sci
> register layout ("SCIF" ports in RZ/V2H & RZ/G3E manual) and the RZ/T2H
> style register layout ("RSCI" ports in RZ/V2H manual, "SCI" ports in
> RZ/G3E manual), so keeping things closely aligned as we move forward
> will be beneficial. I expect that this will be easier with a combined
> driver.

I will have a look at the series.

Happy hacking,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]