Re: [patch net-next v2 1/3] dpll: extend uapi by lock status error attribute

[Simon Horman <horms@kernel.org>]

On Tue, Jan 30, 2024 at 01:08:29PM +0100, Jiri Pirko wrote:
> From: Jiri Pirko <jiri@nvidia.com>
> 
> If the dpll devices goes to state "unlocked" or "holdover", it may be
> caused by an error. In that case, allow user to see what the error was.
> Introduce a new attribute and values it can carry.
> 
> Signed-off-by: Jiri Pirko <jiri@nvidia.com>
> Acked-by: Vadim Fedorenko <vadim.fedorenko@linux.dev>

The nit below notwithstanding, this looks good to me.

Reviewed-by: Simon Horman <horms@kernel.org>

...

> diff --git a/include/uapi/linux/dpll.h b/include/uapi/linux/dpll.h
> index b4e947f9bfbc..0c13d7f1a1bc 100644
> --- a/include/uapi/linux/dpll.h
> +++ b/include/uapi/linux/dpll.h
> @@ -50,6 +50,35 @@ enum dpll_lock_status {
>  	DPLL_LOCK_STATUS_MAX = (__DPLL_LOCK_STATUS_MAX - 1)
>  };
>  
> +/**
> + * enum dpll_lock_status_error - if previous status change was done due to a
> + *   failure, this provides information of dpll device lock status error. Valid
> + *   values for DPLL_A_LOCK_STATUS_ERROR attribute
> + * @DPLL_LOCK_STATUS_ERROR_NONE: dpll device lock status was changed without
> + *   any error
> + * @DPLL_LOCK_STATUS_ERROR_UNDEFINED: dpll device lock status was changed due
> + *   to undefined error. Driver fills this value up in case it is not able to
> + *   obtain suitable exact error type.
> + * @DPLL_LOCK_STATUS_ERROR_MEDIA_DOWN: dpll device lock status was changed
> + *   because of associated media got down. This may happen for example if dpll
> + *   device was previously locked on an input pin of type
> + *   PIN_TYPE_SYNCE_ETH_PORT.
> + * @DPLL_LOCK_STATUS_ERROR_FRACTIONAL_FREQUENCY_OFFSET_TOO_HIGH: the FFO
> + *   (Fractional Frequency Offset) between the RX and TX symbol rate on the
> + *   media got too high. This may happen for example if dpll device was
> + *   previously locked on an input pin of type PIN_TYPE_SYNCE_ETH_PORT.
> + */
> +enum dpll_lock_status_error {
> +	DPLL_LOCK_STATUS_ERROR_NONE = 1,
> +	DPLL_LOCK_STATUS_ERROR_UNDEFINED,
> +	DPLL_LOCK_STATUS_ERROR_MEDIA_DOWN,
> +	DPLL_LOCK_STATUS_ERROR_FRACTIONAL_FREQUENCY_OFFSET_TOO_HIGH,

nit: I'm all for descriptive names,
     but this one is rather long to say the least.

> +
> +	/* private: */
> +	__DPLL_LOCK_STATUS_ERROR_MAX,
> +	DPLL_LOCK_STATUS_ERROR_MAX = (__DPLL_LOCK_STATUS_ERROR_MAX - 1)
> +};
> +
>  #define DPLL_TEMP_DIVIDER	1000
>  
>  /**
> @@ -150,6 +179,7 @@ enum dpll_a {
>  	DPLL_A_LOCK_STATUS,
>  	DPLL_A_TEMP,
>  	DPLL_A_TYPE,
> +	DPLL_A_LOCK_STATUS_ERROR,
>  
>  	__DPLL_A_MAX,
>  	DPLL_A_MAX = (__DPLL_A_MAX - 1)
> -- 
> 2.43.0
> 
>