Re: [PATCH net-next] dpll: zl3073x: add chip-specific minimum input reference frequency

[Vincent Jardin <vjardin@free.fr>]

Hi Ivan,

Thanks for leading it. Below some few minor comments.

> diff --git a/drivers/dpll/zl3073x/core.c b/drivers/dpll/zl3073x/core.c
> index 5b2d77f2c2288e..c0b3b0d579d524 100644
> --- a/drivers/dpll/zl3073x/core.c
> +++ b/drivers/dpll/zl3073x/core.c
> @@ -20,29 +20,30 @@
>  #include "dpll.h"
>  #include "regs.h"
>  
> -#define ZL_CHIP_INFO(_id, _nchannels, _flags)				\
> -	{ .id = (_id), .num_channels = (_nchannels), .flags = (_flags) }
> +#define ZL_CHIP_INFO(_id, _nchannels, _flags, _min_freq)		\
> +	{ .id = (_id), .num_channels = (_nchannels), .flags = (_flags),	\
> +	  .min_ref_freq = (_min_freq) }

Do we really need this C macro ? Everytime we'll add something, it leads to a
wide numbder of line changes for zl3073x_chip_ids[]. What's about using directly the named
fields below and leaving 0/nothing for default cases such as the min_ref_freq ( something like
min_ref_freq ?: 1; ) ? 
 ... return zldev->info->min_ref_freq ?: 1; ...

If you prefer to keep the C macro, then let's use a variadic, something like:
#define ZL_CHIP_INFO(_id, _nchannels, _flags, ...)			\
	{ .id = (_id), .num_channels = (_nchannels), .flags = (_flags),\
	  ##__VA_ARGS__ }

then only the ZL30643 entry needs to be touched:
	ZL_CHIP_INFO(0x0E3B, 3, ZL3073X_FLAG_REF_PHASE_COMP_32,
		     .min_ref_freq = 1000),

>  
>  static const struct zl3073x_chip_info zl3073x_chip_ids[] = {
> -	ZL_CHIP_INFO(0x0E30, 2, ZL3073X_FLAG_REF_PHASE_COMP_32),
> -	ZL_CHIP_INFO(0x0E3B, 3, ZL3073X_FLAG_REF_PHASE_COMP_32),
 ...
> -	ZL_CHIP_INFO(0x2E97, 5, ZL3073X_FLAG_DIE_TEMP),
> -	ZL_CHIP_INFO(0x3FC4, 2, ZL3073X_FLAG_DIE_TEMP),
> +	ZL_CHIP_INFO(0x0E30, 2, ZL3073X_FLAG_REF_PHASE_COMP_32, 1),
> +	ZL_CHIP_INFO(0x0E3B, 3, ZL3073X_FLAG_REF_PHASE_COMP_32, 1000),
> +	ZL_CHIP_INFO(0x0E93, 1, ZL3073X_FLAG_REF_PHASE_COMP_32, 1),
 ...
> +	ZL_CHIP_INFO(0x2E97, 5, ZL3073X_FLAG_DIE_TEMP, 1),
> +	ZL_CHIP_INFO(0x3FC4, 2, ZL3073X_FLAG_DIE_TEMP, 1),

That's a wide set of changes just to add/update 1 line. Since an update is needed, I feel the macro
should be avoided so only
	ZL_CHIP_INFO(0x0E3B, 3, ZL3073X_FLAG_REF_PHASE_COMP_32, 1000),
is needed.

Then, the init code could set to 1 if the value of .min_ref_freq is 0.

> --- a/drivers/dpll/zl3073x/dpll.c
> +++ b/drivers/dpll/zl3073x/dpll.c
> @@ -281,7 +281,8 @@ zl3073x_dpll_input_pin_ref_sync_set(const struct dpll_pin *dpll_pin,
>  		sync_freq = zl3073x_ref_freq_get(sync_ref);
>  
>  		/* Sync signal must be 8 kHz or less and clock reference
> -		 * must be 1 kHz or more and higher than the sync signal.
> +		 * must meet the chip's minimum frequency requirement and be
> +		 * higher than the sync signal.
>  		 */
>  		if (sync_freq > 8000) {
>  			NL_SET_ERR_MSG(extack,
> @@ -289,9 +290,10 @@ zl3073x_dpll_input_pin_ref_sync_set(const struct dpll_pin *dpll_pin,
>  			rc = -EINVAL;
>  			goto unlock;
>  		}
> -		if (ref_freq < 1000) {
> -			NL_SET_ERR_MSG(extack,
> -				       "clock frequency must be 1 kHz or more");
> +		if (ref_freq < zldev->info->min_ref_freq) {
> +			NL_SET_ERR_MSG_FMT(extack,
> +					   "clock frequency must be %u Hz or more",
> +					   zldev->info->min_ref_freq);
>  			rc = -EINVAL;
>  			goto unlock;
>  		}

TBC: to be considered if 0 means 1 (à la zldev->info->min_ref_freq ?: 1)

> diff --git a/drivers/dpll/zl3073x/prop.c b/drivers/dpll/zl3073x/prop.c
> index ac9d41d0f978ef..cdceddcf353e46 100644
> --- a/drivers/dpll/zl3073x/prop.c
> +++ b/drivers/dpll/zl3073x/prop.c
> @@ -20,9 +20,9 @@
>   * @freq: frequency to check
>   *
>   * The function checks the given frequency is valid for the device. For input
> - * pins it checks that the frequency can be factorized using supported base
> - * frequencies. For output pins it checks that the frequency divides connected
> - * synth frequency without remainder.
> + * pins it checks that the frequency is above the chip's minimum and can be
> + * factorized using supported base frequencies. For output pins it checks that
> + * the frequency divides connected synth frequency without remainder.
>   *
>   * Return: true if the frequency is valid, false if not.
>   */
> @@ -36,6 +36,10 @@ zl3073x_pin_check_freq(struct zl3073x_dev *zldev, enum dpll_pin_direction dir,
>  	if (dir == DPLL_PIN_DIRECTION_INPUT) {
>  		int rc;
>  
> +		/* Check minimum frequency */

This comment is not needed, the code tells us.

> +		if (freq < zldev->info->min_ref_freq)
> +			goto err_inv_freq;
> +
>  		/* Check if the frequency can be factorized */
>  		rc = zl3073x_ref_freq_factorize(freq, NULL, NULL);
>  		if (rc)
> 

Best regards,
  Vincent