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

From: Jiri Pirko <jiri@resnulli.us>
Sent: Friday, May 26, 2023 12:39 PM

Fri, May 26, 2023 at 12:14:00PM CEST, arkadiusz.kubalewski@intel.com wrote:

quoted

quoted

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

quoted

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)

It is a parent of pin, not device. The name is confusing. But see below.

quoted

	DPLL_A_ID	(id of parent dpll)
	DPLL_A_PIN_STATE(expected state)
	...		(other pin-dpll attributes to be set)

Does it make sense?

Yeah, good idea. I like this. We will have consistent approach for
parent pin and device. To take it even further, we can have one nested
attr for parent and decide the parent type according to the id attr
given:

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)

DPLL_CMD_PIN_SET
DPLL_A_PIN_ID		(id of pin being configured)
DPLL_A_PIN_PARENT	(nested)
	DPLL_A_ID	(id of parent dpll)
	DPLL_A_PIN_STATE(expected state)
	...		(other pin-dpll attributes to be set)


Same for PIN_GET

Makes sense?

Sure, fixed.

quoted

quoted

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.

quoted

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.

quoted

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?

The point is to make the driver developer to think before passing
randomly constructed label strings. For example, "board_label" would lead
the developer to check how the pin is labeled on the board. The
"panel_label" indicates this is label on a panel. Also, developer can
fill multiple labels for the same pin.

Ok, makes sense, added as suggested.

Thank you,
Arkadiusz

quoted

quoted

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.

quoted

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.

quoted

--------------------------------------------------------------
** 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.

quoted

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.

Cool, thx!

quoted

Thank you,
Arkadiusz