Re: [RFC PATCH v4 0/4] Create common DPLL/clock configuration API

[Jiri Pirko <jiri@resnulli.us>]

Tue, Nov 29, 2022 at 10:37:20PM CET, vfedorenko@novek.ru wrote:
>Implement common API for clock/DPLL configuration and status reporting.
>The API utilises netlink interface as transport for commands and event
>notifications. This API aim to extend current pin configuration and
>make it flexible and easy to cover special configurations.

Overall, I see a lot of issues on multiple levels. I will go over them
in follow-up emails. So far, after couple of hours looking trought this,
I have following general feelings/notes:

1) Netlink interface looks much saner than in previous versions. I will
   send couple of notes, mainly around events and object mixtures and
   couple of bugs/redundancies. But overall, looks fineish.

2) I don't like that concept of a shared pin, at all. It makes things
   unnecessary complicated. Just have a pin created for dpll instance
   and that's it. If another instance has the same pin, it should create
   it as well. Keeps things separate and easy to model. Let the
   hw/fw/driver figure out the implementation oddities.
   Why exactly you keep pushing the shared pin idea? Perhaps I'm missing
   something crucial.

3) I don't like the concept of muxed pins and hierarchies of pins. Why
   does user care? If pin is muxed, the rest of the pins related to this
   one should be in state disabled/disconnected. The user only cares
   about to see which pins are related to each other. It can be easily
   exposed by "muxid" like this:
   pin 1
   pin 2
   pin 3 muxid 100
   pin 4 muxid 100
   pin 5 muxid 101
   pin 6 muxid 101
   In this example pins 3,4 and 5,6 are muxed, therefore the user knows
   if he connects one, the other one gets disconnected (or will have to
   disconnect the first one explicitly first).

4) I don't like the "attr" indirection. It makes things very tangled. It
   comes from the concepts of classes and objects and takes it to
   extreme. Not really something we are commonly used to in kernel.
   Also, it brings no value from what I can see, only makes things very
   hard to read and follow.

   Please keep things direct and simple:
   * If some option could be changed for a pin or dpll, just have an
     op that is directly called from netlink handler to change it.
     There should be clear set of ops for configuration of pin and
     dpll object. This "attr" indirection make this totally invisible.
   * If some attribute is const during dpll or pin lifetime, have it
     passed during dpll or pin creation.



>
>v3 -> v4:
> * redesign framework to make pins dynamically allocated (Arkadiusz)
> * implement shared pins (Arkadiusz)
>v2 -> v3:
> * implement source select mode (Arkadiusz)
> * add documentation
> * implementation improvements (Jakub)
>v1 -> v2:
> * implement returning supported input/output types
> * ptp_ocp: follow suggestions from Jonathan
> * add linux-clk mailing list
>v0 -> v1:
> * fix code style and errors
> * add linux-arm mailing list
>
>
>Arkadiusz Kubalewski (1):
>  dpll: add dpll_attr/dpll_pin_attr helper classes
>
>Vadim Fedorenko (3):
>  dpll: Add DPLL framework base functions
>  dpll: documentation on DPLL subsystem interface
>  ptp_ocp: implement DPLL ops
>
> Documentation/networking/dpll.rst  | 271 ++++++++
> Documentation/networking/index.rst |   1 +
> MAINTAINERS                        |   8 +
> drivers/Kconfig                    |   2 +
> drivers/Makefile                   |   1 +
> drivers/dpll/Kconfig               |   7 +
> drivers/dpll/Makefile              |  11 +
> drivers/dpll/dpll_attr.c           | 278 +++++++++
> drivers/dpll/dpll_core.c           | 760 +++++++++++++++++++++++
> drivers/dpll/dpll_core.h           | 176 ++++++
> drivers/dpll/dpll_netlink.c        | 963 +++++++++++++++++++++++++++++
> drivers/dpll/dpll_netlink.h        |  24 +
> drivers/dpll/dpll_pin_attr.c       | 456 ++++++++++++++
> drivers/ptp/Kconfig                |   1 +
> drivers/ptp/ptp_ocp.c              | 123 ++--
> include/linux/dpll.h               | 261 ++++++++
> include/linux/dpll_attr.h          | 433 +++++++++++++
> include/uapi/linux/dpll.h          | 263 ++++++++
> 18 files changed, 4002 insertions(+), 37 deletions(-)
> create mode 100644 Documentation/networking/dpll.rst
> create mode 100644 drivers/dpll/Kconfig
> create mode 100644 drivers/dpll/Makefile
> create mode 100644 drivers/dpll/dpll_attr.c
> create mode 100644 drivers/dpll/dpll_core.c
> create mode 100644 drivers/dpll/dpll_core.h
> create mode 100644 drivers/dpll/dpll_netlink.c
> create mode 100644 drivers/dpll/dpll_netlink.h
> create mode 100644 drivers/dpll/dpll_pin_attr.c
> create mode 100644 include/linux/dpll.h
> create mode 100644 include/linux/dpll_attr.h
> create mode 100644 include/uapi/linux/dpll.h
>
>-- 
>2.27.0
>