RE: [RFC PATCH v7 0/8] Create common DPLL configuration API

From: Jiri Pirko <jiri@resnulli.us>
Sent: Wednesday, May 17, 2023 12:19 PM

Let me summarize the outcome of the discussion between me and Jakub
regarding attributes, handles and ID lookups in the RFCv7 thread:

--------------------------------------------------------------
** Needed changes for RFCv8 **

1) No scoped indexes.
  The indexes passed from driver to dpll core during call of:
       dpll_device_get() - device_idx
       dpll_pin_get() - pin_idx
  should be for INTERNAL kernel use only and NOT EXPOSED over uapi.
  Therefore following attributes need to be removed:
  DPLL_A_PIN_IDX
  DPLL_A_PIN_PARENT_IDX

Seems doable.
So just to be clear, configuring a pin-pair (MUXed pins) will now be done
with DPLL_A_PIN_PARENT nested attribute.
I.e. configuring state of pin on parent:
DPLL_CMD_PIN_SET
	DPLL_A_PIN_ID		(id of pin being configured)
	DPLL_A_PIN_PARENT	(nested)
		DPLL_A_PIN_ID	(id of parent pin)
		DPLL_A_PIN_STATE(expected state)
		...		(other pin-pair attributes to be set)

Is that ok, or we need separated attribute like DPLL_A_PIN_PARENT_ID??
I think there is no need for separated one, documentation shall just reflect
that.
Also we have nested attribute DPLL_A_DEVICE which is used to show connections
between PIN and multiple dpll devices, to make it consistent I will rename
it to `DPLL_A_DEVICE_PARENT` and make configuration set cmd for the pin-dpll
pair similar to the above:
DPLL_CMD_PIN_SET
	DPLL_A_PIN_ID		(id of pin being configured)
	DPLL_A_DEVICE_PARENT	(nested)
		DPLL_A_ID	(id of parent dpll)
		DPLL_A_PIN_STATE(expected state)
		...		(other pin-dpll attributes to be set)

Does it make sense?

2) For device, the handle will be DPLL_A_ID == dpll->id.
  This will be the only handle for device for every
  device related GET, SET command and every device related notification.
  Note: this ID is not deterministing and may be different depending on
  order of device probes etc.

Seems doable.

3) For pin, the handle will be DPLL_A_PIN_ID == pin->id
  This will be the only handle for pin for every
  pin related GET, SET command and every pin related notification.
  Note: this ID is not deterministing and may be different depending on
  order of device probes etc.

Seems doable.

4) Remove attribute:
  DPLL_A_PIN_LABEL
  and replace it with:
  DPLL_A_PIN_PANEL_LABEL (string)
  DPLL_A_PIN_XXX (string)
  where XXX is a label type, like for example:
    DPLL_A_PIN_BOARD_LABEL
    DPLL_A_PIN_BOARD_TRACE
    DPLL_A_PIN_PACKAGE_PIN

Sorry, I don't get this idea, what are those types?
What are they for?

5) Make sure you expose following attributes for every device and
  pin GET/DUMP command reply message:
  DPLL_A_MODULE_NAME
  DPLL_A_CLOCK_ID

Seems doable.

6) Remove attributes:
  DPLL_A_DEV_NAME
  DPLL_A_BUS_NAME
  as they no longer have any value and do no make sense (even in RFCv7)

Seems doable.

--------------------------------------------------------------
** Lookup commands **

Basically these would allow user to query DEVICE_ID and PIN_ID
according to provided atributes (see examples below).

These would be from my perspective optional for this patchsets.
I believe we can do it as follow-up if needed. For example for mlx5
I don't have usecase for it, since I can consistently get PIN_ID
using RT netlink for given netdev. But I can imagine that for non-SyncE
dpll driver this would make sense to have.

1) Introduce CMD_GET_ID - query the kernel for a dpll device
                         specified by given attrs
  Example:
  -> DPLL_A_MODULE_NAME
     DPLL_A_CLOCK_ID
     DPLL_A_TYPE
  <- DPLL_A_ID
  Example:
  -> DPLL_A_MODULE_NAME
     DPLL_A_CLOCK_ID
  <- DPLL_A_ID
  Example:
  -> DPLL_A_MODULE_NAME
  <- -EINVAL (Extack: "multiple devices matched")

  If user passes a subset of attrs which would not result in
  a single match, kernel returns -EINVAL and proper extack message.

Seems ok.

2) Introduce CMD_GET_PIN_ID - query the kernel for a dpll pin
                             specified by given attrs
  Example:
  -> DPLL_A_MODULE_NAME
     DPLL_A_CLOCK_ID
     DPLL_A_PIN_TYPE
     DPLL_A_PIN_PANEL_LABEL
  <- DPLL_A_PIN_ID
  Example:
  -> DPLL_A_MODULE_NAME
     DPLL_A_CLOCK_ID
  <- DPLL_A_PIN_ID    (There was only one pin for given module/clock_id)
  Example:
  -> DPLL_A_MODULE_NAME
     DPLL_A_CLOCK_ID
  <- -EINVAL (Extack: "multiple pins matched")

  If user passes a subset of attrs which would not result in
  a single match, kernel returns -EINVAL and proper extack message.

Seems ok.

Will try to implement those now.

Thank you,
Arkadiusz