Re: [PATCH net-next] dpll: zl3073x: add chip-specific minimum input reference frequency
From: Vincent Jardin <vjardin@free.fr>
Date: 2026-08-16 11:04:18
Also in:
lkml
Hi Ivan, Thanks for leading it. Below some few minor comments.
quoted hunk ↗ jump to hunk
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.
quoted hunk ↗ jump to hunk
--- 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)
quoted hunk ↗ jump to hunk
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