[PATCH RFC v6 2/6] dpll: Add DPLL framework base functions

[Jiri Pirko]

Tue, May 09, 2023 at 07:53:02PM CEST, kuba at kernel.org wrote:
>On Tue, 9 May 2023 17:21:42 +0200 Jiri Pirko wrote:
>> Tue, May 09, 2023 at 04:52:47PM CEST, kuba at kernel.org wrote:
>> >On Tue, 9 May 2023 09:53:07 +0200 Jiri Pirko wrote:  
>> >> >Yup. Even renaming EXT to something that's less.. relative :(    
>> >> 
>> >> Suggestion?  
>> >
>> >Well, is an SMT socket on the board an EXT pin?
>> >Which is why I prefer PANEL.  
>> 
>> Makes sense.
>> To speak code, we'll have:
>> 
>> /**
>>  * enum dpll_pin_type - defines possible types of a pin, valid values for
>>  *   DPLL_A_PIN_TYPE attribute
>>  * @DPLL_PIN_TYPE_UNSPEC: unspecified value
>>  * @DPLL_PIN_TYPE_MUX: aggregates another layer of selectable pins
>>  * @DPLL_PIN_TYPE_PANEL: physically facing user, for example on a front panel
>>  * @DPLL_PIN_TYPE_SYNCE_ETH_PORT: ethernet port PHY's recovered clock
>>  * @DPLL_PIN_TYPE_INT_OSCILLATOR: device internal oscillator
>>  * @DPLL_PIN_TYPE_GNSS: GNSS recovered clock
>>  */
>> enum dpll_pin_type {
>>         DPLL_PIN_TYPE_UNSPEC,
>>         DPLL_PIN_TYPE_MUX,
>>         DPLL_PIN_TYPE_PANEL,
>>         DPLL_PIN_TYPE_SYNCE_ETH_PORT,
>>         DPLL_PIN_TYPE_INT_OSCILLATOR,
>>         DPLL_PIN_TYPE_GNSS,
>> 
>>         __DPLL_PIN_TYPE_MAX,
>>         DPLL_PIN_TYPE_MAX = (__DPLL_PIN_TYPE_MAX - 1)
>> };
>
>Maybe we can keep the EXT here, just not in the label itself.
>Don't think we care to add pin type for PANEL vs SMT vs jumper?

Okay:
 enum dpll_pin_type {
         DPLL_PIN_TYPE_UNSPEC,
         DPLL_PIN_TYPE_MUX,
	 DPLL_PIN_TYPE_EXT,
         DPLL_PIN_TYPE_SYNCE_ETH_PORT,
         DPLL_PIN_TYPE_INT_OSCILLATOR,
         DPLL_PIN_TYPE_GNSS,

         __DPLL_PIN_TYPE_MAX,
         DPLL_PIN_TYPE_MAX = (__DPLL_PIN_TYPE_MAX - 1)
};



>
>> >> Sure, I get what you say and agree. I'm just trying to find out the
>> >> actual attributes :)  
>> >
>> >PANEL label must match the name on the panel. User can take the card
>> >into their hand, look at the front, and there should be a label/sticker/
>> >/engraving which matches exactly what the kernel reports.
>> >
>> >If the label is printed on the board it's a BOARD_LABEL, if it's the
>> >name of a trace in board docs it's a BOARD_TRACE, if it's a pin of 
>> >the ASIC it's a PACKAGE_PIN.
>> >
>> >If it's none of those, or user does not have access to the detailed
>> >board / pinout - don't use the label.  
>> 
>> To speak code, we'll have:
>> DPLL_A_PIN_PANEL_LABEL (string)
>>    available always when attr[DPLL_A_PIN_TYPE] == DPLL_PIN_TYPE_PANEL
>
>Not sure about always, if there's only one maybe there's no need 
>to provide the label?

Well, I would like to have check in pin_register() for this. That would
be non-trivial if this would be required only in case pin_count>1.
However I can imagine a card with one connector without any label on it.
IDK.


>
>> DPLL_A_PIN_BOARD_LABEL (string)
>>    may be available for any type, optional
>> DPLL_A_PIN_BOARD_TRACE (string)
>>    may be available for any type, optional
>> DPLL_A_PIN_PACKAGE_PIN (string)
>>    may be available for any type, optional
>> 
>> Makes sense?
>
>yup (obviously we need to document the semantics)

Agreed.


>
>> But this does not prevent driver developer to pack random crap in the
>> string anyway :/
>
>It doesn't but it hopefully makes it much more likely that (1) reviewer
>will notice that something is off if the driver printfs random crap;
>and (2) that the user reading the documentation will complain that 
>e.g.BOARD_LABEL is used but does not match the label on they see...

Fair.