Re: [PATCH net-next v12 09/12] dpll: sit9531x: implement output pin state on a DPLL

[Ivan Vecera <ivecera@redhat.com>]

On 10/9/26 20:31, Ali Rouhi wrote:
> From: Oleg Zadorozhnyi <Oleg.Zadorozhnyi@devoxsoftware.com>
> 
> An output is driven by its PLL rather than selected by it, so its state
> says whether it carries a signal: connected while it drives, disconnected
> while it is muted.  Setting the state mutes or un-mutes it by forcing the
> pad to Hi-Z, the only per-output control the device offers that leaves the
> divider alone.
> 
> The force bit and the state bit are separate, and a pad follows the loaded
> configuration while the force bit is clear, so both are read to decide
> whether an output is muted and both are written to change it.  Which of
> the four register banks applies depends on the slot and on whether the pad
> is single-ended or differential.
> 
> A mute or un-mute is a per-output control, but each one that changes
> something runs the device-wide programming sequence of the previous
> patch, so a request for the state the output already has is answered
> from the registers without entering it.  A request that fails after some
> of its writes landed still announces the change, so a subscriber is not
> left with the old state; the pin handle is read once, so a teardown
> racing the request cannot hand the notification a cleared one.  A failed
> entry into the programming state is left the way a commit leaves it:
> loops re-locked and the settling time waited for.
> 
> This is also where output pins start being registered at all: until now
> the registration filter took inputs only, so the output operations the
> earlier patches added -- direction and frequency -- had nothing to
> dispatch on.
> 
> Signed-off-by: Oleg Zadorozhnyi <Oleg.Zadorozhnyi@devoxsoftware.com>
> Assisted-by: LLM
> Signed-off-by: Ali Rouhi <arouhi@sitime.com>
> ---
>   drivers/dpll/sit9531x/core.c | 344 +++++++++++++++++++++++++++++------
>   drivers/dpll/sit9531x/core.h |   7 +
>   drivers/dpll/sit9531x/dpll.c | 130 +++++++++++++
>   3 files changed, 427 insertions(+), 54 deletions(-)
> 
> diff --git a/drivers/dpll/sit9531x/core.c b/drivers/dpll/sit9531x/core.c
> index ea15830b0efa..19aeabd3cd4f 100644
> --- a/drivers/dpll/sit9531x/core.c
> +++ b/drivers/dpll/sit9531x/core.c
> @@ -488,71 +488,50 @@ static int sit9531x_output_forced_hiz(struct sit9531x_dev *sitdev,
>                                          muted);
>   }
> 
> -/* Attempts to re-lock the output loops before reporting them open. */
> -#define SIT9531X_LOOP_LOCK_TRIES       3
> -
>   /*
> - * sit9531x_prg_abort - leave the programming state without committing
> + * sit9531x_output_state_refresh - read an output's mute state back
>    *
> - * Entering the state is two writes, and the second can fail with the debug
> - * block already unlocked and the part possibly already in PRG_CMD.  There
> - * is nothing to commit in that case, but the loops still have to be closed
> - * and the debug key put back, which is otherwise only done by
> - * sit9531x_prg_commit().
> + * Used when a mute could not be confirmed at the time it was written.  The
> + * driver does not poll output state, so without this the cached value would
> + * stand until something else happened to write it.
> + *
> + * Caller must hold sitdev->multiop_lock.
>    */
> -static void sit9531x_prg_abort(struct sit9531x_dev *sitdev)
> +int sit9531x_output_state_refresh(struct sit9531x_dev *sitdev, u8 out_idx)
>   {
> -       u8 attempt;
> -       int rc = -EIO;
> +       bool muted;
> +       int rc;
> 
> -       for (attempt = 0; attempt < SIT9531X_LOOP_LOCK_TRIES; attempt++) {
> -               rc = sit9531x_write_u8(sitdev, SIT9531X_REG_PRG_DIR_GEN,
> -                                      SIT9531X_LOOP_LOCK);
> -               if (!rc)
> -                       break;
> -               usleep_range(1000, 2000);
> -       }
> +       lockdep_assert_held(&sitdev->multiop_lock);
> +
> +       rc = sit9531x_output_forced_hiz(sitdev, out_idx, &muted);
>          if (rc)
> -               dev_err(sitdev->dev,
> -                       "output loops left unlocked after a failed entry: %d\n",
> -                       rc);
> +               return rc;
> +
> +       sitdev->out[out_idx].enabled = !muted;
> +       sitdev->out[out_idx].state_stale = false;
> 
> -       sit9531x_write_u8(sitdev, SIT9531X_REG_OUTSYS_DEBUG,
> -                         SIT9531X_DEBUG_LOCK_VAL);
> +       return 0;
>   }
> 
> -/*
> - * Enter the output-system programming state: unlock the debug
> - * registers on Page 3 and issue the PRG_CMD state command.  Register
> - * writes that reconfigure the output system only take effect when
> - * they are made inside this state.
> - */
> -static int sit9531x_prg_enter(struct sit9531x_dev *sitdev)
> +static int sit9531x_hiz_set_bit(struct sit9531x_dev *sitdev,
> +                               unsigned int reg, u8 bit, bool set)
>   {
> +       u8 cur, new_val;
>          int rc;
> 
> -       rc = sit9531x_write_u8(sitdev, SIT9531X_REG_OUTSYS_DEBUG,
> -                              SIT9531X_DEBUG_UNLOCK_VAL);
> +       rc = sit9531x_read_u8(sitdev, reg, &cur);
>          if (rc)
>                  return rc;
> 
> -       rc = sit9531x_write_u8(sitdev, SIT9531X_REG_PRG_DIR_GEN,
> -                              SIT9531X_PRG_CMD_STATE);
> -       if (rc) {
> -               /*
> -                * The debug block is unlocked at this point, and a transfer
> -                * that reported an error may still have reached the part --
> -                * which would leave the device in PRG_CMD with its output
> -                * loops open.  Callers skip the commit when the entry
> -                * fails, so close both here.
> -                */
> -               sit9531x_prg_abort(sitdev);
> -               return rc;
> -       }
> +       new_val = set ? (cur | BIT(bit)) : (cur & ~BIT(bit));
> 
> -       return 0;
> +       return sit9531x_write_u8(sitdev, reg, new_val);
>   }
> 
> +/* Attempts to re-lock the output loops before reporting them open. */
> +#define SIT9531X_LOOP_LOCK_TRIES       3
> +
>   /*
>    * Commit a programming sequence started by sit9531x_prg_enter():
>    * update the NVM shadow and re-lock the loops.  The sleep gives the
> @@ -622,6 +601,251 @@ static int sit9531x_prg_commit(struct sit9531x_dev *sitdev)
>          return 0;
>   }
> 
> +/*
> + * Enter the output-system programming state: unlock the debug
> + * registers on Page 3 and issue the PRG_CMD state command.  Register
> + * writes that reconfigure the output system only take effect when
> + * they are made inside this state.
> + */
> +static int sit9531x_prg_enter(struct sit9531x_dev *sitdev)
> +{
> +       int rc;
> +
> +       rc = sit9531x_write_u8(sitdev, SIT9531X_REG_OUTSYS_DEBUG,
> +                              SIT9531X_DEBUG_UNLOCK_VAL);
> +       if (rc)
> +               return rc;
> +
> +       rc = sit9531x_write_u8(sitdev, SIT9531X_REG_PRG_DIR_GEN,
> +                              SIT9531X_PRG_CMD_STATE);
> +       if (rc) {
> +               /*
> +                * The debug block is unlocked at this point, and a transfer
> +                * that reported an error may still have reached the part --
> +                * which would leave the device in PRG_CMD with its output
> +                * loops open.  Callers skip the commit when the entry fails,
> +                * so leave the state here the way every completed sequence
> +                * does: nothing was changed, so the update commits what the
> +                * part already holds, the loops are re-locked, and the
> +                * settling time is waited out before the caller drops the
> +                * lock and the next request programs the part.  The entry
> +                * failure is the error to report.
> +                */
> +               sit9531x_prg_commit(sitdev);
> +               return rc;
> +       }
> +
> +       return 0;
> +}
> +
> +/*
> + * sit9531x_output_hiz_write - mute or unmute an output
> + *
> + * Muting takes control of the pin (MASK=1) and forces it to Hi-Z
> + * (STATE=0) on every Hi-Z pair of the slot -- differential, CMOS OutP
> + * and CMOS OutN -- because the output must go quiet whichever way it is
> + * wired; unmuting releases the override on all of them and hands the pin
> + * back to the device.  The caller must already be in the programming
> + * state.
> + */
> +static int sit9531x_output_hiz_write(struct sit9531x_dev *sitdev, u8 slot,
> +                                    bool mute)
> +{
> +       struct sit9531x_hiz_pair p[SIT9531X_HIZ_PAIRS];
> +       u8 mask[SIT9531X_HIZ_PAIRS], state[SIT9531X_HIZ_PAIRS];
> +       int i, rc, undo_rc;

Reverse X-mas here, please.

With both of them...

Reviewed-by: Ivan Vecera <ivecera@redhat.com>