From: Jiri Pirko <jiri@resnulli.us>
Sent: Friday, August 18, 2023 9:24 AM

Fri, Aug 18, 2023 at 01:36:40AM CEST, kuba@kernel.org wrote:

quoted

On Thu, 17 Aug 2023 18:40:00 +0000 Kubalewski, Arkadiusz wrote:

quoted

quoted

Why are all attributes in a single attr space? :(
More than half of them are prefixed with a pin- does it really
not scream to you that they belong to a different space?

I agree, but there is an issue with this, currently:

name: pin-parent-device
subset-of: dpll
attributes:
  -
    name: id
    type: u32
  -
    name: pin-direction
    type: u32
  -
    name: pin-prio
    type: u32
  -
    name: pin-state
    type: u32

Where "id" is a part of device space, rest attrs would be a pin space..
Shall we have another argument for device id in a pin space?

Why would pin and device not have separate spaces?

When referring to a pin from a "device mostly" command you can
usually wrap the pin attributes in a nest, and vice versa.
But it may not be needed at all here? Let's look at the commands:

+    -
+      name: device-id-get
+        request:
+          attributes:
+            - module-name
+            - clock-id
+            - type
+        reply:
+          attributes:
+            - id

All attributes are in "device" space, no mixing.

+      name: device-get
+        request:
+          attributes:
+            - id
+        reply: &dev-attrs
+          attributes:
+            - id
+            - module-name
+            - mode
+            - mode-supported
+            - lock-status
+            - temp
+            - clock-id
+            - type

Again, no pin attributes, so pin can be separate?

+    -
+      name: device-set
+        request:
+          attributes:
+            - id

Herm, this one looks like it's missing attrs :S

+    -
+      name: pin-id-get
+        request:
+          attributes:
+            - module-name
+            - clock-id
+            - pin-board-label
+            - pin-panel-label
+            - pin-package-label
+            - pin-type
+        reply:
+          attributes:
+            - pin-id

Mostly pin stuff. I guess the module-name and clock-id attrs can be
copy/pasted between device and pin, or put them in a separate set
and add that set as an attr here. Copy paste is likely much simpler.

Agreed for the copy.

Honestly, I wound thing that shared ATTR space is fine for DPLL,
the split is an overkill here. But up to you Jakub :)

I prepared some POC's and it seems most convenient way to do the
split was to add new argument as proposed on the previous mail.
After all the spec generated diff for uAPI header like this:

--- a/include/uapi/linux/dpll.h
+++ b/include/uapi/linux/dpll.h

@@ -148,7 +148,17 @@ enum dpll_a {
        DPLL_A_LOCK_STATUS,
        DPLL_A_TEMP,
        DPLL_A_TYPE,
-       DPLL_A_PIN_ID,
+
+       __DPLL_A_MAX,
+       DPLL_A_MAX = (__DPLL_A_MAX - 1)
+};
+
+enum dpll_a_pin {
+       DPLL_A_PIN_ID = 1,
+       DPLL_A_PIN_PARENT_ID,
+       DPLL_A_PIN_MODULE_NAME,
+       DPLL_A_PIN_PAD,
+       DPLL_A_PIN_CLOCK_ID,
        DPLL_A_PIN_BOARD_LABEL,
        DPLL_A_PIN_PANEL_LABEL,
        DPLL_A_PIN_PACKAGE_LABEL,

@@ -164,8 +174,8 @@ enum dpll_a {
        DPLL_A_PIN_PARENT_DEVICE,
        DPLL_A_PIN_PARENT_PIN,

-       __DPLL_A_MAX,
-       DPLL_A_MAX = (__DPLL_A_MAX - 1)
+       __DPLL_A_PIN_MAX,
+       DPLL_A_PIN_MAX = (__DPLL_A_PIN_MAX - 1)
 };

So we have additional attribute for targeting either a pin or device
DPLL_A_PIN_PARENT_ID (u32) - which would be enclosed in the nests as
previously:
- DPLL_A_PIN_PARENT_DEVICE (if parent is a device)
- DPLL_A_PIN_PARENT_PIN (if parent is a pin)


I will adapt the docs and send this to Vadim's repo for review today,
if that is ok for us.

Thank you!
Arkadiusz

quoted

+    -
+      name: pin-get
+        request:
+          attributes:
+            - pin-id
+        reply: &pin-attrs
+          attributes:
+            - pin-id
+            - pin-board-label
+            - pin-panel-label
+            - pin-package-label
+            - pin-type
+            - pin-frequency
+            - pin-frequency-supported
+            - pin-dpll-caps
+            - pin-parent-device

The ID of device is inside this nest.

quoted

+            - pin-parent-pin

All pin.

+    -
+      name: pin-set
+        request:
+          attributes:
+            - pin-id
+            - pin-frequency
+            - pin-direction
+            - pin-prio
+            - pin-state
+            - pin-parent-device

Same here.

quoted

+            - pin-parent-pin

And all pin.