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

[Vinod Koul]

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...

> 
> 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...

-- 
~Vinod