Registers present in page 10 and higher are called mailbox type
registers. Each page represents a mailbox and is used to read and write
configuration of particular object (dpll, output, reference & synth).

The mailbox page contains mask register that is used to select an index of
requested object to work with and semaphore register to indicate what
operation is requested.

The rest of registers in the particular register page are latch
registers that are filled by the firmware during read operation or by
the driver prior write operation.

For read operation the driver...
1) ... updates the mailbox mask register with index of particular object
2) ... sets the mailbox semaphore register read bit
3) ... waits for the semaphore register read bit to be cleared by FW
4) ... reads the configuration from latch registers

For write operation the driver...
1) ... writes the requested configuration to latch registers
2) ... sets the mailbox mask register for the DPLL to be updated
3) ... sets the mailbox semaphore register bit for the write operation
4) ... waits for the semaphore register bit to be cleared by FW

Add functions to read and write mailboxes for all supported object types.

All these functions as well as functions accessing mailbox latch registers
(zl3073x_mb_* functions) have to be called with zl3073x_dev->mailbox_lock
held and a caller is responsible to take this lock.

Signed-off-by: Ivan Vecera <ivecera@redhat.com>
v1->v3:
* dropped ZL3073X_MB_OP macro usage
---
 drivers/mfd/zl3073x-core.c       | 232 +++++++++++++++++++++++
 include/linux/mfd/zl3073x.h      |  12 ++
 include/linux/mfd/zl3073x_regs.h | 304 +++++++++++++++++++++++++++++++
 3 files changed, 548 insertions(+)

diff --git a/drivers/mfd/zl3073x-core.c b/drivers/mfd/zl3073x-core.c
index 8a15237e0f731..3d18786c769d2 100644
--- a/drivers/mfd/zl3073x-core.c
+++ b/drivers/mfd/zl3073x-core.c
@@ -115,6 +115,238 @@ static const struct regmap_config zl3073x_regmap_config = {
 	.volatile_reg	= zl3073x_is_volatile_reg,
 };
 
+/**
+ * zl3073x_mb_dpll_read - read given DPLL configuration to mailbox
+ * @zldev: pointer to device structure
+ * @index: DPLL index
+ *
+ * Reads configuration of given DPLL into DPLL mailbox.
+ *
+ * Context: Process context. Expects zldev->regmap_lock to be held by caller.
+ * Return: 0 on success, <0 on error
+ */
+int zl3073x_mb_dpll_read(struct zl3073x_dev *zldev, u8 index)
+{
+	int rc;
+
+	/* Select requested index in mask register */
+	rc = zl3073x_mb_write_dpll_mb_mask(zldev, BIT(index));
+	if (rc)
+		return rc;
+
+	/* Perform read operation */
+	rc = zl3073x_mb_write_dpll_mb_sem(zldev, ZL_DPLL_MB_SEM_RD);
+	if (rc)
+		return rc;
+
+	/* Wait for the command to actually finish */
+	return zl3073x_mb_poll_dpll_mb_sem(zldev, ZL_DPLL_MB_SEM_RD);
+}
+EXPORT_SYMBOL_GPL(zl3073x_mb_dpll_read);
+
+/**
+ * zl3073x_mb_dpll_write - write given DPLL configuration from mailbox
+ * @zldev: pointer to device structure
+ * @index: DPLL index
+ *
+ * Writes (commits) configuration of given DPLL from DPLL mailbox.
+ *
+ * Context: Process context. Expects zldev->regmap_lock to be held by caller.
+ * Return: 0 on success, <0 on error
+ */
+int zl3073x_mb_dpll_write(struct zl3073x_dev *zldev, u8 index)
+{
+	int rc;
+
+	/* Select requested index in mask register */
+	rc = zl3073x_mb_write_dpll_mb_mask(zldev, BIT(index));
+	if (rc)
+		return rc;
+
+	/* Perform read operation */
+	rc = zl3073x_mb_write_dpll_mb_sem(zldev, ZL_DPLL_MB_SEM_WR);
+	if (rc)
+		return rc;
+
+	/* Wait for the command to actually finish */
+	return zl3073x_mb_poll_dpll_mb_sem(zldev, ZL_DPLL_MB_SEM_WR);
+}
+EXPORT_SYMBOL_GPL(zl3073x_mb_dpll_write);
+
+/**
+ * zl3073x_mb_output_read - read given output configuration to mailbox
+ * @zldev: pointer to device structure
+ * @index: output index
+ *
+ * Reads configuration of given output into output mailbox.
+ *
+ * Context: Process context. Expects zldev->regmap_lock to be held by caller.
+ * Return: 0 on success, <0 on error
+ */
+int zl3073x_mb_output_read(struct zl3073x_dev *zldev, u8 index)
+{
+	int rc;
+
+	/* Select requested index in mask register */
+	rc = zl3073x_mb_write_output_mb_mask(zldev, BIT(index));
+	if (rc)
+		return rc;
+
+	/* Perform read operation */
+	rc = zl3073x_mb_write_output_mb_sem(zldev, ZL_OUTPUT_MB_SEM_RD);
+	if (rc)
+		return rc;
+
+	/* Wait for the command to actually finish */
+	return zl3073x_mb_poll_output_mb_sem(zldev, ZL_OUTPUT_MB_SEM_RD);
+}
+EXPORT_SYMBOL_GPL(zl3073x_mb_output_read);
+
+/**
+ * zl3073x_mb_output_write - write given output configuration from mailbox
+ * @zldev: pointer to device structure
+ * @index: output index
+ *
+ * Writes (commits) configuration of given output from output mailbox.
+ *
+ * Context: Process context. Expects zldev->regmap_lock to be held by caller.
+ * Return: 0 on success, <0 on error
+ */
+int zl3073x_mb_output_write(struct zl3073x_dev *zldev, u8 index)
+{
+	int rc;
+
+	/* Select requested index in mask register */
+	rc = zl3073x_mb_write_output_mb_mask(zldev, BIT(index));
+	if (rc)
+		return rc;
+
+	/* Perform read operation */
+	rc = zl3073x_mb_write_output_mb_sem(zldev, ZL_OUTPUT_MB_SEM_WR);
+	if (rc)
+		return rc;
+
+	/* Wait for the command to actually finish */
+	return zl3073x_mb_poll_output_mb_sem(zldev, ZL_OUTPUT_MB_SEM_WR);
+}
+EXPORT_SYMBOL_GPL(zl3073x_mb_output_write);
+
+/**
+ * zl3073x_mb_ref_read - read given reference configuration to mailbox
+ * @zldev: pointer to device structure
+ * @index: reference index
+ *
+ * Reads configuration of given reference into ref mailbox.
+ *
+ * Context: Process context. Expects zldev->regmap_lock to be held by caller.
+ * Return: 0 on success, <0 on error
+ */
+int zl3073x_mb_ref_read(struct zl3073x_dev *zldev, u8 index)
+{
+	int rc;
+
+	/* Select requested index in mask register */
+	rc = zl3073x_mb_write_ref_mb_mask(zldev, BIT(index));
+	if (rc)
+		return rc;
+
+	/* Perform read operation */
+	rc = zl3073x_mb_write_ref_mb_sem(zldev, ZL_REF_MB_SEM_RD);
+	if (rc)
+		return rc;
+
+	/* Wait for the command to actually finish */
+	return zl3073x_mb_poll_ref_mb_sem(zldev, ZL_REF_MB_SEM_RD);
+}
+EXPORT_SYMBOL_GPL(zl3073x_mb_ref_read);
+
+/**
+ * zl3073x_mb_ref_write - write given reference configuration from mailbox
+ * @zldev: pointer to device structure
+ * @index: reference index
+ *
+ * Writes (commits) configuration of given reference from ref mailbox.
+ *
+ * Context: Process context. Expects zldev->regmap_lock to be held by caller.
+ * Return: 0 on success, <0 on error
+ */
+int zl3073x_mb_ref_write(struct zl3073x_dev *zldev, u8 index)
+{
+	int rc;
+
+	/* Select requested index in mask register */
+	rc = zl3073x_mb_write_ref_mb_mask(zldev, BIT(index));
+	if (rc)
+		return rc;
+
+	/* Perform read operation */
+	rc = zl3073x_mb_write_ref_mb_sem(zldev, ZL_REF_MB_SEM_WR);
+	if (rc)
+		return rc;
+
+	/* Wait for the command to actually finish */
+	return zl3073x_mb_poll_ref_mb_sem(zldev, ZL_REF_MB_SEM_WR);
+}
+EXPORT_SYMBOL_GPL(zl3073x_mb_ref_write);
+
+/**
+ * zl3073x_mb_synth_read - read given synth configuration to mailbox
+ * @zldev: pointer to device structure
+ * @index: synth index
+ *
+ * Reads configuration of given synth into synth mailbox.
+ *
+ * Context: Process context. Expects zldev->regmap_lock to be held by caller.
+ * Return: 0 on success, <0 on error
+ */
+int zl3073x_mb_synth_read(struct zl3073x_dev *zldev, u8 index)
+{
+	int rc;
+
+	/* Select requested index in mask register */
+	rc = zl3073x_mb_write_synth_mb_mask(zldev, BIT(index));
+	if (rc)
+		return rc;
+
+	/* Perform read operation */
+	rc = zl3073x_mb_write_synth_mb_sem(zldev, ZL_SYNTH_MB_SEM_RD);
+	if (rc)
+		return rc;
+
+	/* Wait for the command to actually finish */
+	return zl3073x_mb_poll_synth_mb_sem(zldev, ZL_SYNTH_MB_SEM_RD);
+}
+EXPORT_SYMBOL_GPL(zl3073x_mb_synth_read);
+
+/**
+ * zl3073x_mb_synth_write - write given synth configuration from mailbox
+ * @zldev: pointer to device structure
+ * @index: synth index
+ *
+ * Writes (commits) configuration of given synth from synth mailbox.
+ *
+ * Context: Process context. Expects zldev->regmap_lock to be held by caller.
+ * Return: 0 on success, <0 on error
+ */
+int zl3073x_mb_synth_write(struct zl3073x_dev *zldev, u8 index)
+{
+	int rc;
+
+	/* Select requested index in mask register */
+	rc = zl3073x_mb_write_synth_mb_mask(zldev, BIT(index));
+	if (rc)
+		return rc;
+
+	/* Perform read operation */
+	rc = zl3073x_mb_write_synth_mb_sem(zldev, ZL_SYNTH_MB_SEM_WR);
+	if (rc)
+		return rc;
+
+	/* Wait for the command to actually finish */
+	return zl3073x_mb_poll_synth_mb_sem(zldev, ZL_SYNTH_MB_SEM_WR);
+}
+EXPORT_SYMBOL_GPL(zl3073x_mb_synth_write);
+
 /**
  * zl3073x_devlink_info_get - Devlink device info callback
  * @devlink: devlink structure pointer
diff --git a/include/linux/mfd/zl3073x.h b/include/linux/mfd/zl3073x.h
index b68481dcf77a5..53813a8c39660 100644
--- a/include/linux/mfd/zl3073x.h
+++ b/include/linux/mfd/zl3073x.h
@@ -47,4 +47,16 @@ static inline void zl3073x_mailbox_unlock(struct zl3073x_dev *zldev)
 DEFINE_GUARD(zl3073x_mailbox, struct zl3073x_dev *, zl3073x_mailbox_lock(_T),
 	     zl3073x_mailbox_unlock(_T));
 
+/*
+ * Mailbox operations
+ */
+int zl3073x_mb_dpll_read(struct zl3073x_dev *zldev, u8 index);
+int zl3073x_mb_dpll_write(struct zl3073x_dev *zldev, u8 index);
+int zl3073x_mb_output_read(struct zl3073x_dev *zldev, u8 index);
+int zl3073x_mb_output_write(struct zl3073x_dev *zldev, u8 index);
+int zl3073x_mb_ref_read(struct zl3073x_dev *zldev, u8 index);
+int zl3073x_mb_ref_write(struct zl3073x_dev *zldev, u8 index);
+int zl3073x_mb_synth_read(struct zl3073x_dev *zldev, u8 index);
+int zl3073x_mb_synth_write(struct zl3073x_dev *zldev, u8 index);
+
 #endif /* __LINUX_MFD_ZL3073X_H */
diff --git a/include/linux/mfd/zl3073x_regs.h b/include/linux/mfd/zl3073x_regs.h
index 453a5da8ac63f..d155650349c97 100644
--- a/include/linux/mfd/zl3073x_regs.h
+++ b/include/linux/mfd/zl3073x_regs.h
@@ -15,6 +15,10 @@
 #define ZL_PAGE_SIZE	       0x80
 #define ZL_REG_ADDR(_pg, _off) (ZL_RANGE_OFF + (_pg) * ZL_PAGE_SIZE + (_off))
 
+/* Register polling sleep & timeout */
+#define ZL_POLL_SLEEP_US   10
+#define ZL_POLL_TIMEOUT_US 2000000
+
 /**************************
  * Register Page 0, General
  **************************/
@@ -102,4 +106,304 @@ zl3073x_read_custom_config_ver(struct zl3073x_dev *zldev, u32 *value)
 	return rc;
 }
 
+/*******************************
+ * Register Page 10, Ref Mailbox
+ *******************************/
+
+/*
+ * Register 'ref_mb_mask'
+ * Page: 10, Offset: 0x02, Size: 16 bits
+ */
+#define ZL_REG_REF_MB_MASK ZL_REG_ADDR(10, 0x02)
+
+static inline __maybe_unused int
+zl3073x_mb_read_ref_mb_mask(struct zl3073x_dev *zldev, u16 *value)
+{
+	__be16 temp;
+	int rc;
+
+	lockdep_assert_held(&zldev->mailbox_lock);
+	rc = regmap_bulk_read(zldev->regmap, ZL_REG_REF_MB_MASK, &temp,
+			      sizeof(temp));
+	if (rc)
+		return rc;
+
+	*value = be16_to_cpu(temp);
+	return rc;
+}
+
+static inline __maybe_unused int
+zl3073x_mb_write_ref_mb_mask(struct zl3073x_dev *zldev, u16 value)
+{
+	__be16 temp;
+
+	lockdep_assert_held(&zldev->mailbox_lock);
+	temp = cpu_to_be16(value);
+	return regmap_bulk_write(zldev->regmap, ZL_REG_REF_MB_MASK, &temp,
+				 sizeof(temp));
+}
+
+/*
+ * Register 'ref_mb_sem'
+ * Page: 10, Offset: 0x04, Size: 8 bits
+ */
+#define ZL_REG_REF_MB_SEM ZL_REG_ADDR(10, 0x04)
+#define ZL_REF_MB_SEM_WR  BIT(0)
+#define ZL_REF_MB_SEM_RD  BIT(1)
+
+static inline __maybe_unused int
+zl3073x_mb_read_ref_mb_sem(struct zl3073x_dev *zldev, u8 *value)
+{
+	unsigned int v;
+	int rc;
+
+	lockdep_assert_held(&zldev->mailbox_lock);
+	rc = regmap_read(zldev->regmap, ZL_REG_REF_MB_SEM, &v);
+	*value = v;
+	return rc;
+}
+
+static inline __maybe_unused int
+zl3073x_mb_write_ref_mb_sem(struct zl3073x_dev *zldev, u8 value)
+{
+	lockdep_assert_held(&zldev->mailbox_lock);
+	return regmap_write(zldev->regmap, ZL_REG_REF_MB_SEM, value);
+}
+
+static inline __maybe_unused int
+zl3073x_mb_poll_ref_mb_sem(struct zl3073x_dev *zldev, u8 bitmask)
+{
+	unsigned int v;
+
+	lockdep_assert_held(&zldev->mailbox_lock);
+	return regmap_read_poll_timeout(zldev->regmap, ZL_REG_REF_MB_SEM, v,
+					!(v & bitmask), ZL_POLL_SLEEP_US,
+					ZL_POLL_TIMEOUT_US);
+}
+
+/********************************
+ * Register Page 12, DPLL Mailbox
+ ********************************/
+
+/*
+ * Register 'dpll_mb_mask'
+ * Page: 12, Offset: 0x02, Size: 16 bits
+ */
+#define ZL_REG_DPLL_MB_MASK ZL_REG_ADDR(12, 0x02)
+
+static inline __maybe_unused int
+zl3073x_mb_read_dpll_mb_mask(struct zl3073x_dev *zldev, u16 *value)
+{
+	__be16 temp;
+	int rc;
+
+	lockdep_assert_held(&zldev->mailbox_lock);
+	rc = regmap_bulk_read(zldev->regmap, ZL_REG_DPLL_MB_MASK, &temp,
+			      sizeof(temp));
+	if (rc)
+		return rc;
+
+	*value = be16_to_cpu(temp);
+	return rc;
+}
+
+static inline __maybe_unused int
+zl3073x_mb_write_dpll_mb_mask(struct zl3073x_dev *zldev, u16 value)
+{
+	__be16 temp;
+
+	lockdep_assert_held(&zldev->mailbox_lock);
+	temp = cpu_to_be16(value);
+	return regmap_bulk_write(zldev->regmap, ZL_REG_DPLL_MB_MASK, &temp,
+				 sizeof(temp));
+}
+
+/*
+ * Register 'dpll_mb_sem'
+ * Page: 12, Offset: 0x04, Size: 8 bits
+ */
+#define ZL_REG_DPLL_MB_SEM ZL_REG_ADDR(12, 0x04)
+#define ZL_DPLL_MB_SEM_WR  BIT(0)
+#define ZL_DPLL_MB_SEM_RD  BIT(1)
+
+static inline __maybe_unused int
+zl3073x_mb_read_dpll_mb_sem(struct zl3073x_dev *zldev, u8 *value)
+{
+	unsigned int v;
+	int rc;
+
+	lockdep_assert_held(&zldev->mailbox_lock);
+	rc = regmap_read(zldev->regmap, ZL_REG_DPLL_MB_SEM, &v);
+	*value = v;
+	return rc;
+}
+
+static inline __maybe_unused int
+zl3073x_mb_write_dpll_mb_sem(struct zl3073x_dev *zldev, u8 value)
+{
+	lockdep_assert_held(&zldev->mailbox_lock);
+	return regmap_write(zldev->regmap, ZL_REG_DPLL_MB_SEM, value);
+}
+
+static inline __maybe_unused int
+zl3073x_mb_poll_dpll_mb_sem(struct zl3073x_dev *zldev, u8 bitmask)
+{
+	unsigned int v;
+
+	lockdep_assert_held(&zldev->mailbox_lock);
+	return regmap_read_poll_timeout(zldev->regmap, ZL_REG_DPLL_MB_SEM, v,
+					!(v & bitmask), ZL_POLL_SLEEP_US,
+					ZL_POLL_TIMEOUT_US);
+}
+
+/*********************************
+ * Register Page 13, Synth Mailbox
+ *********************************/
+
+/*
+ * Register 'synth_mb_mask'
+ * Page: 13, Offset: 0x02, Size: 16 bits
+ */
+#define ZL_REG_SYNTH_MB_MASK ZL_REG_ADDR(13, 0x02)
+
+static inline __maybe_unused int
+zl3073x_mb_read_synth_mb_mask(struct zl3073x_dev *zldev, u16 *value)
+{
+	__be16 temp;
+	int rc;
+
+	lockdep_assert_held(&zldev->mailbox_lock);
+	rc = regmap_bulk_read(zldev->regmap, ZL_REG_SYNTH_MB_MASK, &temp,
+			      sizeof(temp));
+	if (rc)
+		return rc;
+
+	*value = be16_to_cpu(temp);
+	return rc;
+}
+
+static inline __maybe_unused int
+zl3073x_mb_write_synth_mb_mask(struct zl3073x_dev *zldev, u16 value)
+{
+	__be16 temp;
+
+	lockdep_assert_held(&zldev->mailbox_lock);
+	temp = cpu_to_be16(value);
+	return regmap_bulk_write(zldev->regmap, ZL_REG_SYNTH_MB_MASK, &temp,
+				 sizeof(temp));
+}
+
+/*
+ * Register 'synth_mb_sem'
+ * Page: 13, Offset: 0x04, Size: 8 bits
+ */
+#define ZL_REG_SYNTH_MB_SEM ZL_REG_ADDR(13, 0x04)
+#define ZL_SYNTH_MB_SEM_WR  BIT(0)
+#define ZL_SYNTH_MB_SEM_RD  BIT(1)
+
+static inline __maybe_unused int
+zl3073x_mb_read_synth_mb_sem(struct zl3073x_dev *zldev, u8 *value)
+{
+	unsigned int v;
+	int rc;
+
+	lockdep_assert_held(&zldev->mailbox_lock);
+	rc = regmap_read(zldev->regmap, ZL_REG_SYNTH_MB_SEM, &v);
+	*value = v;
+	return rc;
+}
+
+static inline __maybe_unused int
+zl3073x_mb_write_synth_mb_sem(struct zl3073x_dev *zldev, u8 value)
+{
+	lockdep_assert_held(&zldev->mailbox_lock);
+	return regmap_write(zldev->regmap, ZL_REG_SYNTH_MB_SEM, value);
+}
+
+static inline __maybe_unused int
+zl3073x_mb_poll_synth_mb_sem(struct zl3073x_dev *zldev, u8 bitmask)
+{
+	unsigned int v;
+
+	lockdep_assert_held(&zldev->mailbox_lock);
+	return regmap_read_poll_timeout(zldev->regmap, ZL_REG_SYNTH_MB_SEM, v,
+					!(v & bitmask), ZL_POLL_SLEEP_US,
+					ZL_POLL_TIMEOUT_US);
+}
+
+/**********************************
+ * Register Page 14, Output Mailbox
+ **********************************/
+
+/*
+ * Register 'output_mb_mask'
+ * Page: 14, Offset: 0x02, Size: 16 bits
+ */
+#define ZL_REG_OUTPUT_MB_MASK ZL_REG_ADDR(14, 0x02)
+
+static inline __maybe_unused int
+zl3073x_mb_read_output_mb_mask(struct zl3073x_dev *zldev, u16 *value)
+{
+	__be16 temp;
+	int rc;
+
+	lockdep_assert_held(&zldev->mailbox_lock);
+	rc = regmap_bulk_read(zldev->regmap, ZL_REG_OUTPUT_MB_MASK, &temp,
+			      sizeof(temp));
+	if (rc)
+		return rc;
+
+	*value = be16_to_cpu(temp);
+	return rc;
+}
+
+static inline __maybe_unused int
+zl3073x_mb_write_output_mb_mask(struct zl3073x_dev *zldev, u16 value)
+{
+	__be16 temp;
+
+	lockdep_assert_held(&zldev->mailbox_lock);
+	temp = cpu_to_be16(value);
+	return regmap_bulk_write(zldev->regmap, ZL_REG_OUTPUT_MB_MASK, &temp,
+				 sizeof(temp));
+}
+
+/*
+ * Register 'output_mb_sem'
+ * Page: 14, Offset: 0x04, Size: 8 bits
+ */
+#define ZL_REG_OUTPUT_MB_SEM ZL_REG_ADDR(14, 0x04)
+#define ZL_OUTPUT_MB_SEM_WR  BIT(0)
+#define ZL_OUTPUT_MB_SEM_RD  BIT(1)
+
+static inline __maybe_unused int
+zl3073x_mb_read_output_mb_sem(struct zl3073x_dev *zldev, u8 *value)
+{
+	unsigned int v;
+	int rc;
+
+	lockdep_assert_held(&zldev->mailbox_lock);
+	rc = regmap_read(zldev->regmap, ZL_REG_OUTPUT_MB_SEM, &v);
+	*value = v;
+	return rc;
+}
+
+static inline __maybe_unused int
+zl3073x_mb_write_output_mb_sem(struct zl3073x_dev *zldev, u8 value)
+{
+	lockdep_assert_held(&zldev->mailbox_lock);
+	return regmap_write(zldev->regmap, ZL_REG_OUTPUT_MB_SEM, value);
+}
+
+static inline __maybe_unused int
+zl3073x_mb_poll_output_mb_sem(struct zl3073x_dev *zldev, u8 bitmask)
+{
+	unsigned int v;
+
+	lockdep_assert_held(&zldev->mailbox_lock);
+	return regmap_read_poll_timeout(zldev->regmap, ZL_REG_OUTPUT_MB_SEM, v,
+					!(v & bitmask), ZL_POLL_SLEEP_US,
+					ZL_POLL_TIMEOUT_US);
+}
+
 #endif /* __LINUX_MFD_ZL3073X_REGS_H */
-- 
2.48.1

On Wed, 16 Apr 2025, Ivan Vecera wrote:

> Registers present in page 10 and higher are called mailbox type
> registers. Each page represents a mailbox and is used to read and write
> configuration of particular object (dpll, output, reference & synth).
> 
> The mailbox page contains mask register that is used to select an index of
> requested object to work with and semaphore register to indicate what
> operation is requested.
> 
> The rest of registers in the particular register page are latch
> registers that are filled by the firmware during read operation or by
> the driver prior write operation.
> 
> For read operation the driver...
> 1) ... updates the mailbox mask register with index of particular object
> 2) ... sets the mailbox semaphore register read bit
> 3) ... waits for the semaphore register read bit to be cleared by FW
> 4) ... reads the configuration from latch registers
> 
> For write operation the driver...
> 1) ... writes the requested configuration to latch registers
> 2) ... sets the mailbox mask register for the DPLL to be updated
> 3) ... sets the mailbox semaphore register bit for the write operation
> 4) ... waits for the semaphore register bit to be cleared by FW
> 
> Add functions to read and write mailboxes for all supported object types.
> 
> All these functions as well as functions accessing mailbox latch registers
> (zl3073x_mb_* functions) have to be called with zl3073x_dev->mailbox_lock
> held and a caller is responsible to take this lock.
> 
> Signed-off-by: Ivan Vecera <ivecera@redhat.com>
> v1->v3:
> * dropped ZL3073X_MB_OP macro usage
> ---
>  drivers/mfd/zl3073x-core.c       | 232 +++++++++++++++++++++++
>  include/linux/mfd/zl3073x.h      |  12 ++
>  include/linux/mfd/zl3073x_regs.h | 304 +++++++++++++++++++++++++++++++
>  3 files changed, 548 insertions(+)

> +/*
> + * Mailbox operations
> + */
> +int zl3073x_mb_dpll_read(struct zl3073x_dev *zldev, u8 index);
> +int zl3073x_mb_dpll_write(struct zl3073x_dev *zldev, u8 index);
> +int zl3073x_mb_output_read(struct zl3073x_dev *zldev, u8 index);
> +int zl3073x_mb_output_write(struct zl3073x_dev *zldev, u8 index);
> +int zl3073x_mb_ref_read(struct zl3073x_dev *zldev, u8 index);
> +int zl3073x_mb_ref_write(struct zl3073x_dev *zldev, u8 index);
> +int zl3073x_mb_synth_read(struct zl3073x_dev *zldev, u8 index);
> +int zl3073x_mb_synth_write(struct zl3073x_dev *zldev, u8 index);

Why aren't these being placed into drivers/mailbox?

-- 
Lee Jones [李琼斯]

On 17. 04. 25 6:13 odp., Lee Jones wrote:
> On Wed, 16 Apr 2025, Ivan Vecera wrote:
> 
>> Registers present in page 10 and higher are called mailbox type
>> registers. Each page represents a mailbox and is used to read and write
>> configuration of particular object (dpll, output, reference & synth).
>>
>> The mailbox page contains mask register that is used to select an index of
>> requested object to work with and semaphore register to indicate what
>> operation is requested.
>>
>> The rest of registers in the particular register page are latch
>> registers that are filled by the firmware during read operation or by
>> the driver prior write operation.
>>
>> For read operation the driver...
>> 1) ... updates the mailbox mask register with index of particular object
>> 2) ... sets the mailbox semaphore register read bit
>> 3) ... waits for the semaphore register read bit to be cleared by FW
>> 4) ... reads the configuration from latch registers
>>
>> For write operation the driver...
>> 1) ... writes the requested configuration to latch registers
>> 2) ... sets the mailbox mask register for the DPLL to be updated
>> 3) ... sets the mailbox semaphore register bit for the write operation
>> 4) ... waits for the semaphore register bit to be cleared by FW
>>
>> Add functions to read and write mailboxes for all supported object types.
>>
>> All these functions as well as functions accessing mailbox latch registers
>> (zl3073x_mb_* functions) have to be called with zl3073x_dev->mailbox_lock
>> held and a caller is responsible to take this lock.
>>
>> Signed-off-by: Ivan Vecera <ivecera@redhat.com>
>> v1->v3:
>> * dropped ZL3073X_MB_OP macro usage
>> ---
>>   drivers/mfd/zl3073x-core.c       | 232 +++++++++++++++++++++++
>>   include/linux/mfd/zl3073x.h      |  12 ++
>>   include/linux/mfd/zl3073x_regs.h | 304 +++++++++++++++++++++++++++++++
>>   3 files changed, 548 insertions(+)
> 
>> +/*
>> + * Mailbox operations
>> + */
>> +int zl3073x_mb_dpll_read(struct zl3073x_dev *zldev, u8 index);
>> +int zl3073x_mb_dpll_write(struct zl3073x_dev *zldev, u8 index);
>> +int zl3073x_mb_output_read(struct zl3073x_dev *zldev, u8 index);
>> +int zl3073x_mb_output_write(struct zl3073x_dev *zldev, u8 index);
>> +int zl3073x_mb_ref_read(struct zl3073x_dev *zldev, u8 index);
>> +int zl3073x_mb_ref_write(struct zl3073x_dev *zldev, u8 index);
>> +int zl3073x_mb_synth_read(struct zl3073x_dev *zldev, u8 index);
>> +int zl3073x_mb_synth_write(struct zl3073x_dev *zldev, u8 index);
> 
> Why aren't these being placed into drivers/mailbox?

I think the only common thing of this with drivers/mailbox is only the
name. Mailbox (this comes from datasheet) here is just an atomic way to
read or write some range of registers.

How can be that used here?

Ivan

> +/**
> + * zl3073x_mb_dpll_read - read given DPLL configuration to mailbox
> + * @zldev: pointer to device structure
> + * @index: DPLL index
> + *
> + * Reads configuration of given DPLL into DPLL mailbox.
> + *
> + * Context: Process context. Expects zldev->regmap_lock to be held by caller.
> + * Return: 0 on success, <0 on error
> + */
> +int zl3073x_mb_dpll_read(struct zl3073x_dev *zldev, u8 index)
> +{
> +	int rc;

lockdep_assert_held(zldev->regmap_lock) is stronger than having a
comment. When talking about i2c and spi devices, it costs nothing, and
catches bugs early.

> +/*
> + * Mailbox operations
> + */
> +int zl3073x_mb_dpll_read(struct zl3073x_dev *zldev, u8 index);
> +int zl3073x_mb_dpll_write(struct zl3073x_dev *zldev, u8 index);
> +int zl3073x_mb_output_read(struct zl3073x_dev *zldev, u8 index);
> +int zl3073x_mb_output_write(struct zl3073x_dev *zldev, u8 index);
> +int zl3073x_mb_ref_read(struct zl3073x_dev *zldev, u8 index);
> +int zl3073x_mb_ref_write(struct zl3073x_dev *zldev, u8 index);
> +int zl3073x_mb_synth_read(struct zl3073x_dev *zldev, u8 index);
> +int zl3073x_mb_synth_write(struct zl3073x_dev *zldev, u8 index);

I assume these are the only valid ways to access a mailbox?

If so:

> +static inline __maybe_unused int
> +zl3073x_mb_read_ref_mb_mask(struct zl3073x_dev *zldev, u16 *value)
> +{
> +	__be16 temp;
> +	int rc;
> +
> +	lockdep_assert_held(&zldev->mailbox_lock);
> +	rc = regmap_bulk_read(zldev->regmap, ZL_REG_REF_MB_MASK, &temp,
> +			      sizeof(temp));
> +	if (rc)
> +		return rc;
> +
> +	*value = be16_to_cpu(temp);
> +	return rc;
> +}

These helpers can be made local to the core. You can then drop the
lockdep_assert_held() from here, since the only way to access them is
via the API you defined above, and add the checks in those API
functions.

	Andrew

On Wed, Apr 16, 2025 at 7:32 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> > +/**
> > + * zl3073x_mb_dpll_read - read given DPLL configuration to mailbox
> > + * @zldev: pointer to device structure
> > + * @index: DPLL index
> > + *
> > + * Reads configuration of given DPLL into DPLL mailbox.
> > + *
> > + * Context: Process context. Expects zldev->regmap_lock to be held by caller.
> > + * Return: 0 on success, <0 on error
> > + */
> > +int zl3073x_mb_dpll_read(struct zl3073x_dev *zldev, u8 index)
> > +{
> > +     int rc;
>
> lockdep_assert_held(zldev->regmap_lock) is stronger than having a
> comment. When talking about i2c and spi devices, it costs nothing, and
> catches bugs early.

Makes sense to put the assert here...

Will add.

>
> > +/*
> > + * Mailbox operations
> > + */
> > +int zl3073x_mb_dpll_read(struct zl3073x_dev *zldev, u8 index);
> > +int zl3073x_mb_dpll_write(struct zl3073x_dev *zldev, u8 index);
> > +int zl3073x_mb_output_read(struct zl3073x_dev *zldev, u8 index);
> > +int zl3073x_mb_output_write(struct zl3073x_dev *zldev, u8 index);
> > +int zl3073x_mb_ref_read(struct zl3073x_dev *zldev, u8 index);
> > +int zl3073x_mb_ref_write(struct zl3073x_dev *zldev, u8 index);
> > +int zl3073x_mb_synth_read(struct zl3073x_dev *zldev, u8 index);
> > +int zl3073x_mb_synth_write(struct zl3073x_dev *zldev, u8 index);
>
> I assume these are the only valid ways to access a mailbox?
>
> If so:
>
> > +static inline __maybe_unused int
> > +zl3073x_mb_read_ref_mb_mask(struct zl3073x_dev *zldev, u16 *value)
> > +{
> > +     __be16 temp;
> > +     int rc;
> > +
> > +     lockdep_assert_held(&zldev->mailbox_lock);
> > +     rc = regmap_bulk_read(zldev->regmap, ZL_REG_REF_MB_MASK, &temp,
> > +                           sizeof(temp));
> > +     if (rc)
> > +             return rc;
> > +
> > +     *value = be16_to_cpu(temp);
> > +     return rc;
> > +}
>
> These helpers can be made local to the core. You can then drop the
> lockdep_assert_held() from here, since the only way to access them is
> via the API you defined above, and add the checks in those API
> functions.

This cannot be done this way... the above API just simplifies the
operation of read and write latch registers from/to mailbox.

Whole operation is described in the commit description.

E.g. read something about DPLL1
1. Call zl3073x_mb_dpll_read(..., 1)
   This selects DPLL1 in the DPLL mailbox and performs read operation
and waits for finish
2. Call zl3073x_mb_read_dpll_mode()
   This reads dpll_mode latch register

write:
1. Call zl3073x_mb_write_dpll_mode(...)
   This writes mode to dpll_mode latch register
2. Call zl3073x_mb_dpll_read(..., 1)
   This writes all info from latch registers to DPLL1

The point is that between step 1 and 2 nobody else cannot touch
latch_registers or mailbox select register and op semaphore.

Thanks,
Ivan

> > > +/*
> > > + * Mailbox operations
> > > + */
> > > +int zl3073x_mb_dpll_read(struct zl3073x_dev *zldev, u8 index);
> > > +int zl3073x_mb_dpll_write(struct zl3073x_dev *zldev, u8 index);
> > > +int zl3073x_mb_output_read(struct zl3073x_dev *zldev, u8 index);
> > > +int zl3073x_mb_output_write(struct zl3073x_dev *zldev, u8 index);
> > > +int zl3073x_mb_ref_read(struct zl3073x_dev *zldev, u8 index);
> > > +int zl3073x_mb_ref_write(struct zl3073x_dev *zldev, u8 index);
> > > +int zl3073x_mb_synth_read(struct zl3073x_dev *zldev, u8 index);
> > > +int zl3073x_mb_synth_write(struct zl3073x_dev *zldev, u8 index);
> >
> > I assume these are the only valid ways to access a mailbox?
> >
> > If so:
> >
> > > +static inline __maybe_unused int
> > > +zl3073x_mb_read_ref_mb_mask(struct zl3073x_dev *zldev, u16 *value)
> > > +{
> > > +     __be16 temp;
> > > +     int rc;
> > > +
> > > +     lockdep_assert_held(&zldev->mailbox_lock);
> > > +     rc = regmap_bulk_read(zldev->regmap, ZL_REG_REF_MB_MASK, &temp,
> > > +                           sizeof(temp));
> > > +     if (rc)
> > > +             return rc;
> > > +
> > > +     *value = be16_to_cpu(temp);
> > > +     return rc;
> > > +}
> >
> > These helpers can be made local to the core. You can then drop the
> > lockdep_assert_held() from here, since the only way to access them is
> > via the API you defined above, and add the checks in those API
> > functions.
> 
> This cannot be done this way... the above API just simplifies the
> operation of read and write latch registers from/to mailbox.
> 
> Whole operation is described in the commit description.
> 
> E.g. read something about DPLL1
> 1. Call zl3073x_mb_dpll_read(..., 1)
>    This selects DPLL1 in the DPLL mailbox and performs read operation
> and waits for finish
> 2. Call zl3073x_mb_read_dpll_mode()
>    This reads dpll_mode latch register
> 
> write:
> 1. Call zl3073x_mb_write_dpll_mode(...)
>    This writes mode to dpll_mode latch register
> 2. Call zl3073x_mb_dpll_read(..., 1)
>    This writes all info from latch registers to DPLL1
> 
> The point is that between step 1 and 2 nobody else cannot touch
> latch_registers or mailbox select register and op semaphore.

Again, think about your layering. What does your lower level mailbox
API look like? What does the MFD need to export for a safe API?

So maybe you need zl3073x_mb_read_u8(), zl3073x_mb_read_u16(),
zl3073x_mb_read_u32(), as part of your mailbox API. These assert the
lock is held.

You could even make zl3073x_mb_read_u8() validate the register is in
the upper range, and that zl3073x_read_u8() the register is in the
lower range.

	Andrew

On 17. 04. 25 3:22 odp., Andrew Lunn wrote:
>>>> +/*
>>>> + * Mailbox operations
>>>> + */
>>>> +int zl3073x_mb_dpll_read(struct zl3073x_dev *zldev, u8 index);
>>>> +int zl3073x_mb_dpll_write(struct zl3073x_dev *zldev, u8 index);
>>>> +int zl3073x_mb_output_read(struct zl3073x_dev *zldev, u8 index);
>>>> +int zl3073x_mb_output_write(struct zl3073x_dev *zldev, u8 index);
>>>> +int zl3073x_mb_ref_read(struct zl3073x_dev *zldev, u8 index);
>>>> +int zl3073x_mb_ref_write(struct zl3073x_dev *zldev, u8 index);
>>>> +int zl3073x_mb_synth_read(struct zl3073x_dev *zldev, u8 index);
>>>> +int zl3073x_mb_synth_write(struct zl3073x_dev *zldev, u8 index);
>>>
>>> I assume these are the only valid ways to access a mailbox?
>>>
>>> If so:
>>>
>>>> +static inline __maybe_unused int
>>>> +zl3073x_mb_read_ref_mb_mask(struct zl3073x_dev *zldev, u16 *value)
>>>> +{
>>>> +     __be16 temp;
>>>> +     int rc;
>>>> +
>>>> +     lockdep_assert_held(&zldev->mailbox_lock);
>>>> +     rc = regmap_bulk_read(zldev->regmap, ZL_REG_REF_MB_MASK, &temp,
>>>> +                           sizeof(temp));
>>>> +     if (rc)
>>>> +             return rc;
>>>> +
>>>> +     *value = be16_to_cpu(temp);
>>>> +     return rc;
>>>> +}
>>>
>>> These helpers can be made local to the core. You can then drop the
>>> lockdep_assert_held() from here, since the only way to access them is
>>> via the API you defined above, and add the checks in those API
>>> functions.
>>
>> This cannot be done this way... the above API just simplifies the
>> operation of read and write latch registers from/to mailbox.
>>
>> Whole operation is described in the commit description.
>>
>> E.g. read something about DPLL1
>> 1. Call zl3073x_mb_dpll_read(..., 1)
>>     This selects DPLL1 in the DPLL mailbox and performs read operation
>> and waits for finish
>> 2. Call zl3073x_mb_read_dpll_mode()
>>     This reads dpll_mode latch register
>>
>> write:
>> 1. Call zl3073x_mb_write_dpll_mode(...)
>>     This writes mode to dpll_mode latch register
>> 2. Call zl3073x_mb_dpll_read(..., 1)
>>     This writes all info from latch registers to DPLL1
>>
>> The point is that between step 1 and 2 nobody else cannot touch
>> latch_registers or mailbox select register and op semaphore.
> 
> Again, think about your layering. What does your lower level mailbox
> API look like? What does the MFD need to export for a safe API?
> 
> So maybe you need zl3073x_mb_read_u8(), zl3073x_mb_read_u16(),
> zl3073x_mb_read_u32(), as part of your mailbox API. These assert the
> lock is held.
> 
> You could even make zl3073x_mb_read_u8() validate the register is in
> the upper range, and that zl3073x_read_u8() the register is in the
> lower range.

Yes, this LGTM... Anyway if the MB API would provide reading and writing 
MBs at once then zl3073x_mb_{read,write}_u* are not necessary as the 
only caller of these functions is MFD itself and they would be called 
under MB API that holds the lock.

Ivan

On 16. 04. 25 8:27 odp., Ivan Vecera wrote:
> On Wed, Apr 16, 2025 at 7:32 PM Andrew Lunn <andrew@lunn.ch> wrote:
>>
>>> +/**
>>> + * zl3073x_mb_dpll_read - read given DPLL configuration to mailbox
>>> + * @zldev: pointer to device structure
>>> + * @index: DPLL index
>>> + *
>>> + * Reads configuration of given DPLL into DPLL mailbox.
>>> + *
>>> + * Context: Process context. Expects zldev->regmap_lock to be held by caller.
>>> + * Return: 0 on success, <0 on error
>>> + */
>>> +int zl3073x_mb_dpll_read(struct zl3073x_dev *zldev, u8 index)
>>> +{
>>> +     int rc;
>>
>> lockdep_assert_held(zldev->regmap_lock) is stronger than having a
>> comment. When talking about i2c and spi devices, it costs nothing, and
>> catches bugs early.
> 
> Makes sense to put the assert here...
> 
> Will add.
> 
>>
>>> +/*
>>> + * Mailbox operations
>>> + */
>>> +int zl3073x_mb_dpll_read(struct zl3073x_dev *zldev, u8 index);
>>> +int zl3073x_mb_dpll_write(struct zl3073x_dev *zldev, u8 index);
>>> +int zl3073x_mb_output_read(struct zl3073x_dev *zldev, u8 index);
>>> +int zl3073x_mb_output_write(struct zl3073x_dev *zldev, u8 index);
>>> +int zl3073x_mb_ref_read(struct zl3073x_dev *zldev, u8 index);
>>> +int zl3073x_mb_ref_write(struct zl3073x_dev *zldev, u8 index);
>>> +int zl3073x_mb_synth_read(struct zl3073x_dev *zldev, u8 index);
>>> +int zl3073x_mb_synth_write(struct zl3073x_dev *zldev, u8 index);
>>
>> I assume these are the only valid ways to access a mailbox?
>>
>> If so:
>>
>>> +static inline __maybe_unused int
>>> +zl3073x_mb_read_ref_mb_mask(struct zl3073x_dev *zldev, u16 *value)
>>> +{
>>> +     __be16 temp;
>>> +     int rc;
>>> +
>>> +     lockdep_assert_held(&zldev->mailbox_lock);
>>> +     rc = regmap_bulk_read(zldev->regmap, ZL_REG_REF_MB_MASK, &temp,
>>> +                           sizeof(temp));
>>> +     if (rc)
>>> +             return rc;
>>> +
>>> +     *value = be16_to_cpu(temp);
>>> +     return rc;
>>> +}
>>
>> These helpers can be made local to the core. You can then drop the
>> lockdep_assert_held() from here, since the only way to access them is
>> via the API you defined above, and add the checks in those API
>> functions.
> 
> This cannot be done this way... the above API just simplifies the
> operation of read and write latch registers from/to mailbox.
> 
> Whole operation is described in the commit description.
> 
> E.g. read something about DPLL1
> 1. Call zl3073x_mb_dpll_read(..., 1)
>     This selects DPLL1 in the DPLL mailbox and performs read operation
> and waits for finish
> 2. Call zl3073x_mb_read_dpll_mode()
>     This reads dpll_mode latch register
> 
> write:
> 1. Call zl3073x_mb_write_dpll_mode(...)
>     This writes mode to dpll_mode latch register
> 2. Call zl3073x_mb_dpll_read(..., 1)
>     This writes all info from latch registers to DPLL1
> 
> The point is that between step 1 and 2 nobody else cannot touch
> latch_registers or mailbox select register and op semaphore.
> 

Anyway, I have a different idea... completely abstract mailboxes from 
the caller. The mailbox content can be large and the caller is barely 
interested in all registers from the mailbox but this could be resolved 
this way:

The proposed API e.g for Ref mailbox:

int zl3073x_mb_ref_read(struct zl3073x_dev *zldev, u8 index,
                         struct zl3073x_mb_ref *mb);
int zl3073x_mb_ref_write(struct zl3073x_dev *zldev, u8 index,
                          struct zl3073x_mb_ref *mb);

struct zl3073x_mb_ref {
	u32	flags;
	u16	freq_base;
	u16	freq_mult;
	u16	ratio_m;
	u16	ratio_n;
	u8	config;
	u64	phase_offset_compensation;
	u8	sync_ctrl;
	u32	esync_div;
}

#define ZL3073X_MB_REF_FREQ_BASE			BIT(0)
#define ZL3073X_MB_REF_FREQ_MULT			BIT(1)
#define ZL3073X_MB_REF_RATIO_M				BIT(2)
#define ZL3073X_MB_REF_RATIO_N			 	BIT(3)
#define ZL3073X_MB_REF_CONFIG			 	BIT(4)
#define ZL3073X_MB_REF_PHASE_OFFSET_COMPENSATION 	BIT(5)
#define ZL3073X_MB_REF_SYNC_CTRL			BIT(6)
#define ZL3073X_MB_REF_ESYNC_DIV			BIT(7)

Then a reader can read this way (read freq and ratio of 3rd ref):
{
	struct zl3073x_mb_ref mb;
	...
	mb.flags = ZL3073X_MB_REF_FREQ_BASE |
		   ZL3073X_MB_REF_FREQ_MULT |
		   ZL3073X_MB_REF_RATIO_M |
		   ZL3073X_MB_REF_RATIO_N;
	rc = zl3073x_mb_ref_read(zldev, 3, &mb);
	if (rc)
		return rc;
	/* at this point mb fields requested via flags are filled */
}
A writer similarly (write config of 5th ref):
{
	struct zl3073x_mb_ref mb;
	...
	mb.flags = ZL3073X_MB_REF_CONFIG;
	mb.config = FIELD_PREP(SOME_MASK, SOME_VALUE);
	rc = zl3073x_mb_ref_write(zldev, 5, &mb);
	...
	/* config of 5th ref was commited */
}

The advantages:
* no explicit locking required from the callers
* locking is done inside mailbox API
* each mailbox type can have different mutex so multiple calls for
   different mailbox types (e.g ref & output) can be done in parallel

WDYT about this approach?

Thanks,
Ivan

> Anyway, I have a different idea... completely abstract mailboxes from the
> caller. The mailbox content can be large and the caller is barely interested
> in all registers from the mailbox but this could be resolved this way:
> 
> The proposed API e.g for Ref mailbox:
> 
> int zl3073x_mb_ref_read(struct zl3073x_dev *zldev, u8 index,
>                         struct zl3073x_mb_ref *mb);
> int zl3073x_mb_ref_write(struct zl3073x_dev *zldev, u8 index,
>                          struct zl3073x_mb_ref *mb);
> 
> struct zl3073x_mb_ref {
> 	u32	flags;
> 	u16	freq_base;
> 	u16	freq_mult;
> 	u16	ratio_m;
> 	u16	ratio_n;
> 	u8	config;
> 	u64	phase_offset_compensation;
> 	u8	sync_ctrl;
> 	u32	esync_div;
> }
> 
> #define ZL3073X_MB_REF_FREQ_BASE			BIT(0)
> #define ZL3073X_MB_REF_FREQ_MULT			BIT(1)
> #define ZL3073X_MB_REF_RATIO_M				BIT(2)
> #define ZL3073X_MB_REF_RATIO_N			 	BIT(3)
> #define ZL3073X_MB_REF_CONFIG			 	BIT(4)
> #define ZL3073X_MB_REF_PHASE_OFFSET_COMPENSATION 	BIT(5)
> #define ZL3073X_MB_REF_SYNC_CTRL			BIT(6)
> #define ZL3073X_MB_REF_ESYNC_DIV			BIT(7)
> 
> Then a reader can read this way (read freq and ratio of 3rd ref):
> {
> 	struct zl3073x_mb_ref mb;
> 	...
> 	mb.flags = ZL3073X_MB_REF_FREQ_BASE |
> 		   ZL3073X_MB_REF_FREQ_MULT |
> 		   ZL3073X_MB_REF_RATIO_M |
> 		   ZL3073X_MB_REF_RATIO_N;
> 	rc = zl3073x_mb_ref_read(zldev, 3, &mb);
> 	if (rc)
> 		return rc;
> 	/* at this point mb fields requested via flags are filled */
> }
> A writer similarly (write config of 5th ref):
> {
> 	struct zl3073x_mb_ref mb;
> 	...
> 	mb.flags = ZL3073X_MB_REF_CONFIG;
> 	mb.config = FIELD_PREP(SOME_MASK, SOME_VALUE);
> 	rc = zl3073x_mb_ref_write(zldev, 5, &mb);
> 	...
> 	/* config of 5th ref was commited */
> }
> 
> The advantages:
> * no explicit locking required from the callers
> * locking is done inside mailbox API
> * each mailbox type can have different mutex so multiple calls for
>   different mailbox types (e.g ref & output) can be done in parallel
> 
> WDYT about this approach?

I would say this is actually your next layer on top of the basic
mailbox API. This makes it more friendly to your sub driver and puts
all the locking in one place where it can easily be reviewed.

One question would be, where does this code belong. Is it in the MFD,
or in the subdrivers? I guess it is in the subdrivers.

	Andrew

On 17. 04. 25 3:27 odp., Andrew Lunn wrote:
>> Anyway, I have a different idea... completely abstract mailboxes from the
>> caller. The mailbox content can be large and the caller is barely interested
>> in all registers from the mailbox but this could be resolved this way:
>>
>> The proposed API e.g for Ref mailbox:
>>
>> int zl3073x_mb_ref_read(struct zl3073x_dev *zldev, u8 index,
>>                          struct zl3073x_mb_ref *mb);
>> int zl3073x_mb_ref_write(struct zl3073x_dev *zldev, u8 index,
>>                           struct zl3073x_mb_ref *mb);
>>
>> struct zl3073x_mb_ref {
>> 	u32	flags;
>> 	u16	freq_base;
>> 	u16	freq_mult;
>> 	u16	ratio_m;
>> 	u16	ratio_n;
>> 	u8	config;
>> 	u64	phase_offset_compensation;
>> 	u8	sync_ctrl;
>> 	u32	esync_div;
>> }
>>
>> #define ZL3073X_MB_REF_FREQ_BASE			BIT(0)
>> #define ZL3073X_MB_REF_FREQ_MULT			BIT(1)
>> #define ZL3073X_MB_REF_RATIO_M				BIT(2)
>> #define ZL3073X_MB_REF_RATIO_N			 	BIT(3)
>> #define ZL3073X_MB_REF_CONFIG			 	BIT(4)
>> #define ZL3073X_MB_REF_PHASE_OFFSET_COMPENSATION 	BIT(5)
>> #define ZL3073X_MB_REF_SYNC_CTRL			BIT(6)
>> #define ZL3073X_MB_REF_ESYNC_DIV			BIT(7)
>>
>> Then a reader can read this way (read freq and ratio of 3rd ref):
>> {
>> 	struct zl3073x_mb_ref mb;
>> 	...
>> 	mb.flags = ZL3073X_MB_REF_FREQ_BASE |
>> 		   ZL3073X_MB_REF_FREQ_MULT |
>> 		   ZL3073X_MB_REF_RATIO_M |
>> 		   ZL3073X_MB_REF_RATIO_N;
>> 	rc = zl3073x_mb_ref_read(zldev, 3, &mb);
>> 	if (rc)
>> 		return rc;
>> 	/* at this point mb fields requested via flags are filled */
>> }
>> A writer similarly (write config of 5th ref):
>> {
>> 	struct zl3073x_mb_ref mb;
>> 	...
>> 	mb.flags = ZL3073X_MB_REF_CONFIG;
>> 	mb.config = FIELD_PREP(SOME_MASK, SOME_VALUE);
>> 	rc = zl3073x_mb_ref_write(zldev, 5, &mb);
>> 	...
>> 	/* config of 5th ref was commited */
>> }
>>
>> The advantages:
>> * no explicit locking required from the callers
>> * locking is done inside mailbox API
>> * each mailbox type can have different mutex so multiple calls for
>>    different mailbox types (e.g ref & output) can be done in parallel
>>
>> WDYT about this approach?
> 
> I would say this is actually your next layer on top of the basic
> mailbox API. This makes it more friendly to your sub driver and puts
> all the locking in one place where it can easily be reviewed.
> 
> One question would be, where does this code belong. Is it in the MFD,
> or in the subdrivers? I guess it is in the subdrivers.

No, it should be part of MFD because it does not make sense to implement 
API above in each sub-driver separately.

Sub-driver would use this MB ABI for MB access and
zl3073x_{read,write}_u{8,16,32,48} for non-MB registers.

Ivan

On Thu, 17 Apr 2025, Ivan Vecera wrote:

> 
> 
> On 17. 04. 25 3:27 odp., Andrew Lunn wrote:
> > > Anyway, I have a different idea... completely abstract mailboxes from the
> > > caller. The mailbox content can be large and the caller is barely interested
> > > in all registers from the mailbox but this could be resolved this way:
> > > 
> > > The proposed API e.g for Ref mailbox:
> > > 
> > > int zl3073x_mb_ref_read(struct zl3073x_dev *zldev, u8 index,
> > >                          struct zl3073x_mb_ref *mb);
> > > int zl3073x_mb_ref_write(struct zl3073x_dev *zldev, u8 index,
> > >                           struct zl3073x_mb_ref *mb);
> > > 
> > > struct zl3073x_mb_ref {
> > > 	u32	flags;
> > > 	u16	freq_base;
> > > 	u16	freq_mult;
> > > 	u16	ratio_m;
> > > 	u16	ratio_n;
> > > 	u8	config;
> > > 	u64	phase_offset_compensation;
> > > 	u8	sync_ctrl;
> > > 	u32	esync_div;
> > > }
> > > 
> > > #define ZL3073X_MB_REF_FREQ_BASE			BIT(0)
> > > #define ZL3073X_MB_REF_FREQ_MULT			BIT(1)
> > > #define ZL3073X_MB_REF_RATIO_M				BIT(2)
> > > #define ZL3073X_MB_REF_RATIO_N			 	BIT(3)
> > > #define ZL3073X_MB_REF_CONFIG			 	BIT(4)
> > > #define ZL3073X_MB_REF_PHASE_OFFSET_COMPENSATION 	BIT(5)
> > > #define ZL3073X_MB_REF_SYNC_CTRL			BIT(6)
> > > #define ZL3073X_MB_REF_ESYNC_DIV			BIT(7)
> > > 
> > > Then a reader can read this way (read freq and ratio of 3rd ref):
> > > {
> > > 	struct zl3073x_mb_ref mb;
> > > 	...
> > > 	mb.flags = ZL3073X_MB_REF_FREQ_BASE |
> > > 		   ZL3073X_MB_REF_FREQ_MULT |
> > > 		   ZL3073X_MB_REF_RATIO_M |
> > > 		   ZL3073X_MB_REF_RATIO_N;
> > > 	rc = zl3073x_mb_ref_read(zldev, 3, &mb);
> > > 	if (rc)
> > > 		return rc;
> > > 	/* at this point mb fields requested via flags are filled */
> > > }
> > > A writer similarly (write config of 5th ref):
> > > {
> > > 	struct zl3073x_mb_ref mb;
> > > 	...
> > > 	mb.flags = ZL3073X_MB_REF_CONFIG;
> > > 	mb.config = FIELD_PREP(SOME_MASK, SOME_VALUE);
> > > 	rc = zl3073x_mb_ref_write(zldev, 5, &mb);
> > > 	...
> > > 	/* config of 5th ref was commited */
> > > }
> > > 
> > > The advantages:
> > > * no explicit locking required from the callers
> > > * locking is done inside mailbox API
> > > * each mailbox type can have different mutex so multiple calls for
> > >    different mailbox types (e.g ref & output) can be done in parallel
> > > 
> > > WDYT about this approach?
> > 
> > I would say this is actually your next layer on top of the basic
> > mailbox API. This makes it more friendly to your sub driver and puts
> > all the locking in one place where it can easily be reviewed.
> > 
> > One question would be, where does this code belong. Is it in the MFD,
> > or in the subdrivers? I guess it is in the subdrivers.
> 
> No, it should be part of MFD because it does not make sense to implement API
> above in each sub-driver separately.
> 
> Sub-driver would use this MB ABI for MB access and
> zl3073x_{read,write}_u{8,16,32,48} for non-MB registers.

Regardless of whether you decide to place the API in the sub-drivers or
not, it doesn't belong in MFD.  600 lines of any API is too heavyweight
to live here.  If you can't justify placing it in Mailbox, my next
suggestion would be drivers/platform.

-- 
Lee Jones [李琼斯]