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

Jiri Pirko
2022-11-30T12:32:29Z

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
>