Re: [PATCH v2] ALSA: ctxfi: Simplify dao_clear_{left,right}_input() functions

[David Laight <david.laight.linux@xxxxxxxxx>]

On Mon, 30 Dec 2024 15:32:58 +0000
Ethan Carter Edwards <ethan@xxxxxxxxxxxxxxxxx> wrote:

> There was a lote of code duplication in the dao_clear_left_input() and
> dao_clear_right_input() functions. A new function, dao_clear_input(),
> was created and now the left and right functions call it instead of
> repeating themselves.
> 
> Link: https://lore.kernel.org/lkml/NyKCr2VHK_xCQDwNxFKKx2LVd2d_AC2f2j4eAvnD9uRPtb50i2AruCLOp6mHxsGiyYJ0Tgd3Z50Oy1JTi5gPhjd2WQM2skrv7asp3fLl8HU=@ethancedwards.com/
> 
> Signed-off-by: Ethan Carter Edwards <ethan@xxxxxxxxxxxxxxxxx>
> Co-developed-by: David Laight <david.laight.linux@xxxxxxxxx>
> ---
>  Changes in V2: Fixed formatting caused by email client. Sorry!
>  sound/pci/ctxfi/ctdaio.c | 50 ++++++++++++++--------------------------
>  1 file changed, 17 insertions(+), 33 deletions(-)
> 
> diff --git a/sound/pci/ctxfi/ctdaio.c b/sound/pci/ctxfi/ctdaio.c
> index 83aaf9441ef3..d46542d0b454 100644
> --- a/sound/pci/ctxfi/ctdaio.c
> +++ b/sound/pci/ctxfi/ctdaio.c
> @@ -211,52 +211,36 @@ static int dao_set_right_input(struct dao *dao, struct rsc *input)
>  	return 0;
>  }
>  
> -static int dao_clear_left_input(struct dao *dao)
> +static int dao_clear_input(struct dao *dao, unsigned int start, unsigned int end)
>  {
> -	struct imapper *entry;
> -	struct daio *daio = &dao->daio;
> -	int i;
> +	struct imapper *to_free = dao->imappers[start];
> +	unsigned int i;
>  
> -	if (!dao->imappers[0])
> +	if (!to_free)
>  		return 0;
> -
> -	entry = dao->imappers[0];
> -	dao->mgr->imap_delete(dao->mgr, entry);
> -	/* Program conjugate resources */
> -	for (i = 1; i < daio->rscl.msr; i++) {
> -		entry = dao->imappers[i];
> -		dao->mgr->imap_delete(dao->mgr, entry);
> +	for (i = start; i < end; i++) {
> +		dao->mgr->imap_delete(dao->mgr, dao->imappers[i]);
>  		dao->imappers[i] = NULL;
>  	}
>  
> -	kfree(dao->imappers[0]);
> -	dao->imappers[0] = NULL;
> -
> +	kfree(to_free);
>  	return 0;
>  }
>  
> -static int dao_clear_right_input(struct dao *dao)
> -{
> -	struct imapper *entry;
> -	struct daio *daio = &dao->daio;
> -	int i;
>  
> -	if (!dao->imappers[daio->rscl.msr])
> -		return 0;
> +static int dao_clear_left_input(struct dao *dao)
> +{
> +	u32 offset = dao->daio.rscl.msr;
>  
> -	entry = dao->imappers[daio->rscl.msr];
> -	dao->mgr->imap_delete(dao->mgr, entry);
> -	/* Program conjugate resources */
> -	for (i = 1; i < daio->rscr.msr; i++) {
> -		entry = dao->imappers[daio->rscl.msr + i];
> -		dao->mgr->imap_delete(dao->mgr, entry);
> -		dao->imappers[daio->rscl.msr + i] = NULL;
> -	}
> +	return dao_clear_input(dao, 0, 0 + offset);

I really wouldn't bother with the temporary variable.
It is also a 'length' not an offset and you can't decide on the type.
'unsigned int' is fine.

But I suspect it really is worth checking if that *imappers[] is needed at all.
(Rename and see where the build breaks!)
You might just need struct imappers *imappers_left and *imappers_right to hold
base of the kmalloc()ed block.
At that point the functions collapse down even more.

	David

> +}
>  
> -	kfree(dao->imappers[daio->rscl.msr]);
> -	dao->imappers[daio->rscl.msr] = NULL;
> +static int dao_clear_right_input(struct dao *dao)
> +{
> +	u32 start = dao->daio.rscl.msr;
> +	u32 offset = dao->daio.rscr.msr;
>  
> -	return 0;
> +	return dao_clear_input(dao, start, start + offset);
>  }
>  
>  static const struct dao_rsc_ops dao_ops = {