Re: [PATCH 6/9] soundwire: amd: introduce struct amd_sdw_hw_ops dispatch framework

[Mukunda,Vijendar]

On 03/10/26 13:26, Vinod Koul wrote:
> On 17-09-26, 14:32, Vijendar Mukunda wrote:
>> Add struct amd_sdw_hw_ops, a function-pointer dispatch table for
>> hardware-revision-specific SoundWire manager operations. Add inline
>> dispatch helpers for each callback and a const hw_ops pointer in
>> struct amd_sdw_manager.
>>
>> The irq_thread and update_slave_status_work slots take
>> struct amd_sdw_manager * so that different ACP revisions can register
>> different handler logic. hw_ops is populated and call sites are wired
>> through the dispatch helpers in subsequent patches.
> Lets solve with one more level! Do you really need this...
Agreed.  We can drop it.  Will modify the code and respin the patch series.
>
>> Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@xxxxxxx>
>> ---
>>  drivers/soundwire/amd_manager.h   | 193 ++++++++++++++++++++++++++++++
>>  include/linux/soundwire/sdw_amd.h |   4 +
>>  2 files changed, 197 insertions(+)
>>
>> diff --git a/drivers/soundwire/amd_manager.h b/drivers/soundwire/amd_manager.h
>> index 63fecff62079..2d08c6edd018 100644
>> --- a/drivers/soundwire/amd_manager.h
>> +++ b/drivers/soundwire/amd_manager.h
>> @@ -274,4 +274,197 @@ static u32 sdw_manager_reg_mask_array[AMD_ACP63_SDW_MAX_MANAGER_COUNT] =  {
>>  		AMD_SDW0_EXT_INTR_MASK,
>>  		AMD_SDW1_EXT_INTR_MASK
>>  };
>> +
>> +#define AMD_SDW_HW_OPS(amd_manager, cb)	((amd_manager)->hw_ops->cb)
>> +
>> +/**
>> + * struct amd_sdw_hw_ops - hardware-revision-specific SoundWire manager ops
>> + *
>> + * @init_sdw_manager: enable the SoundWire manager hardware and perform bus reset
>> + * @enable_sdw_manager: set the manager enable bit and wait for it to assert
>> + * @disable_sdw_manager: clear the manager enable bit and wait for it to deassert
>> + * @enable_sdw_interrupts: unmask ACP-level and per-manager interrupt bits
>> + * @disable_sdw_interrupts: mask ACP-level and per-manager interrupt bits
>> + * @set_frameshape: write the row/column frame-shape register
>> + * @send_cmd_get_resp: issue an immediate command and return the response;
>> + *   returns 0 on success with the raw u64 response in @response,
>> + *   or negative on hardware timeout
>> + * @wake_enable: enable or disable the in-band SoundWire wake interrupt
>> + * @host_wake_enable: enable or disable the host-side wake interrupt
>> + * @set_device_state: write the D0/D3 device power state for the manager instance
>> + * @read_and_process_ping_status: issue a PING command and process the slave
>> + *   attach/detach status; called from the IRQ work path
>> + * @process_wake_event: handle an in-band wake notification from a slave
>> + * @irq_thread: hw_ops callback invoked by the thin work_struct wrapper on
>> + *   each interrupt; reads status registers and dispatches slave events
>> + * @update_slave_status_work: hw_ops callback invoked by the status-update
>> + *   work item; reads slave status and updates the SoundWire core
>> + * @clk_init_ctrl: set the clock divider and update the frame shape registers
>> + *   based on the current bus clock frequency
>> + * @clock_stop_exit: restart the SoundWire bus clock after a CLK_STOP_MODE
>> + *   suspend; implementation differs across ACP revisions
>> + * @suspend: system-suspend hook; stops the bus and powers down the manager
>> + * @suspend_runtime: runtime-suspend hook; same sequence as @suspend
>> + * @resume_runtime: runtime-resume hook; reinitialises the manager and restarts
>> + *   the bus after a POWER_OFF_MODE or CLK_STOP_MODE suspend
>> + */
>> +struct amd_sdw_hw_ops {
>> +	int (*init_sdw_manager)(struct amd_sdw_manager *amd_manager);
>> +	int (*enable_sdw_manager)(struct amd_sdw_manager *amd_manager);
>> +	int (*disable_sdw_manager)(struct amd_sdw_manager *amd_manager);
>> +	void (*enable_sdw_interrupts)(struct amd_sdw_manager *amd_manager);
>> +	void (*disable_sdw_interrupts)(struct amd_sdw_manager *amd_manager);
>> +	void (*set_frameshape)(struct amd_sdw_manager *amd_manager);
>> +	int (*send_cmd_get_resp)(struct amd_sdw_manager *amd_manager, u32 lower_data,
>> +				 u32 upper_data, u64 *response);
>> +	void (*wake_enable)(struct amd_sdw_manager *amd_manager, bool enable);
>> +	int (*host_wake_enable)(struct amd_sdw_manager *amd_manager, bool enable);
>> +	int (*set_device_state)(struct amd_sdw_manager *amd_manager, u32 target_device_state);
>> +	void (*read_and_process_ping_status)(struct amd_sdw_manager *amd_manager);
>> +	void (*process_wake_event)(struct amd_sdw_manager *amd_manager);
>> +	void (*irq_thread)(struct amd_sdw_manager *amd_manager);
>> +	void (*update_slave_status_work)(struct amd_sdw_manager *amd_manager);
>> +	int (*clk_init_ctrl)(struct amd_sdw_manager *amd_manager);
>> +	int (*clock_stop_exit)(struct amd_sdw_manager *amd_manager);
>> +	int (*suspend)(struct amd_sdw_manager *amd_manager);
>> +	int (*suspend_runtime)(struct amd_sdw_manager *amd_manager);
>> +	int (*resume_runtime)(struct amd_sdw_manager *amd_manager);
> I would suggest splitting the driver and add common code to a lib and
> have specific drivers for older and new gen platforms...
 Agreed. We will restructure the code and respin the patch series.
>