Re: [PATCH v2 04/11] ASoC: SDCA: add hw_ops with hw_init hook

On 9/7/26 12:29 PM, Pierre-Louis Bossart wrote:

On 9/7/26 10:37, Srinivas Kandagatla wrote:

quoted

Add struct sdca_class_hw_ops with a hw_init callback that runs from
sdca_class_probe() before the class regmap is created.  Codec drivers
use it to enable supplies, toggle reset GPIOs and program initial
vendor register state.

It'd be good to clarify when this hw_init() is supposed to run. Probe
and hardware being available are usually two different things. I think
this relies on a behavior at the device level where the function
subdevices are only created after SoundWire device enumeration.

This callback is now removed in v3 and using probe to do most of it.

--srini

Also the 'hw_init' naming could be confusing, this is used in many codec
drivers to track if the hardware has previously been initialized.

quoted

sdca_class_probe() gains an optional const struct sdca_class_hw_ops *
argument (NULL for pure-generic SDCA parts) and stashes it on
sdca_class_drv for later use.

No functional change for the built-in class_sdw_driver, which passes
NULL.

Signed-off-by: Srinivas Kandagatla <redacted>
---
 include/sound/sdca_class.h  | 17 ++++++++++++++++-
 sound/soc/sdca/sdca_class.c | 15 +++++++++++++--
 2 files changed, 29 insertions(+), 3 deletions(-)

diff --git a/include/sound/sdca_class.h b/include/sound/sdca_class.h
index c6063f22be7a..3342937d09fd 100644
--- a/include/sound/sdca_class.h
+++ b/include/sound/sdca_class.h

@@ -20,6 +20,17 @@ struct regmap;
 struct sdw_slave;
 struct sdca_function_data;
 
+/**
+ * struct sdca_class_hw_ops - optional codec hardware callbacks
+ * @hw_init: enable supplies, toggle reset, etc.  Runs from sdca_class_probe()
+ *           before the class regmap is created and before the slave is
+ *           ATTACHED; callers needing bus I/O must sdw_slave_wait_for_init()
+ *           first.
+ */
+struct sdca_class_hw_ops {
+	int (*hw_init)(struct sdw_slave *slave);
+};
+
 struct sdca_class_drv {
 	struct device *dev;
 	struct regmap *dev_regmap;

@@ -27,6 +38,8 @@ struct sdca_class_drv {
 
 	struct sdca_interrupt_info *irq_info;
 
+	const struct sdca_class_hw_ops *hw_ops;
+
 	struct mutex regmap_lock;
 	/* Serialise function initialisations */
 	struct mutex init_lock;

@@ -35,7 +48,9 @@ struct sdca_class_drv {
 
 /* Library helpers used by codec-specific SDCA SoundWire drivers. */
 int sdca_class_read_prop(struct sdw_slave *sdw);
-int sdca_class_probe(struct sdw_slave *sdw, struct sdca_class_drv *drv);
+int sdca_class_probe(struct sdw_slave *sdw,
+		     struct sdca_class_drv *drv,
+		     const struct sdca_class_hw_ops *hw_ops);
 void sdca_class_remove(struct sdca_class_drv *drv);
 
 /*

diff --git a/sound/soc/sdca/sdca_class.c b/sound/soc/sdca/sdca_class.c
index b952fa6eb802..374de7d9e0b5 100644
--- a/sound/soc/sdca/sdca_class.c
+++ b/sound/soc/sdca/sdca_class.c

@@ -153,6 +153,8 @@ static void class_boot_work(struct work_struct *work)
  *       allocation and sets its own dev_set_drvdata() -- the framework
  *       does not touch drvdata.  Typically embedded in the codec's own
  *       priv struct so codec drivers can keep per-slave state.
+ * @hw_ops: optional device-specific hw_ops (may be NULL for pure-generic
+ *          SDCA parts that need no quirks)
  *
  * Codec-specific SoundWire drivers call this from their .probe after
  * allocating a struct sdca_class_drv (usually embedded in their own

@@ -160,7 +162,9 @@ static void class_boot_work(struct work_struct *work)
  * sdca_class_drv fields, sets up the class regmap, and queues the
  * deferred boot work.
  */
-int sdca_class_probe(struct sdw_slave *sdw, struct sdca_class_drv *drv)
+int sdca_class_probe(struct sdw_slave *sdw,
+		     struct sdca_class_drv *drv,
+		     const struct sdca_class_hw_ops *hw_ops)
 {
 	struct device *dev = &sdw->dev;
 	struct regmap_config *dev_config;

@@ -178,9 +182,16 @@ int sdca_class_probe(struct sdw_slave *sdw, struct sdca_class_drv *drv)
 
 	drv->dev = dev;
 	drv->sdw = sdw;
+	drv->hw_ops = hw_ops;
 	mutex_init(&drv->regmap_lock);
 	mutex_init(&drv->init_lock);
 
+	if (hw_ops && hw_ops->hw_init) {
+		ret = hw_ops->hw_init(sdw);
+		if (ret)
+			return dev_err_probe(dev, ret, "hw_init failed\n");
+	}
+

nit-pick: should the INIT_WORK be moved higher before this hw_init()? It
has nothing to do with regmap and we'd lose the requirement that
hw_init() be run before regmap inits.

quoted

 	INIT_WORK(&drv->boot_work, class_boot_work);
 
 	dev_config->lock_arg = &drv->regmap_lock;

@@ -222,7 +233,7 @@ static int class_sdw_probe(struct sdw_slave *sdw, const struct sdw_device_id *id
 
 	dev_set_drvdata(&sdw->dev, drv);
 
-	return sdca_class_probe(sdw, drv);
+	return sdca_class_probe(sdw, drv, NULL);
 }
 
 /**