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

[Srinivas Kandagatla]

On 9/7/26 12:29 PM, Pierre-Louis Bossart wrote:
> On 9/7/26 10:37, Srinivas Kandagatla wrote:
>> 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.
> 
>> 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 <srinivas.kandagatla@xxxxxxxxxxxxxxxx>
>> ---
>>  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.
> 
>>  	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);
>>  }
>>  
>>  /**
>