Re: [PATCH 4/5] ASoC: SDCA: Add missing function type names

[Pierre-Louis Bossart <pierre-louis.bossart@xxxxxxxxx>]

On 12/20/24 11:35, Charles Keepax wrote:
> It is not helpful to error out on some SDCA function types, we
> might as well report the correct name and let the driver core
> simply not bind a driver to those functions for which the code
> lacks support. Also given no functions currently have support,
> it seems odd to select some as unsupported.
>
> Signed-off-by: Charles Keepax <ckeepax@xxxxxxxxxxxxxxxxxxxxx>
> ---
>   include/sound/sdca_function.h   |  1 +
>   sound/soc/sdca/sdca_functions.c | 12 ++++++++++--
>   2 files changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/include/sound/sdca_function.h b/include/sound/sdca_function.h
> index 6943df0851a9..89e42db6d591 100644
> --- a/include/sound/sdca_function.h
> +++ b/include/sound/sdca_function.h
> @@ -42,6 +42,7 @@ enum sdca_function_type {
>   #define	SDCA_FUNCTION_TYPE_RJ_NAME		"RJ"
>   #define	SDCA_FUNCTION_TYPE_SIMPLE_NAME		"SimpleJack"
>   #define	SDCA_FUNCTION_TYPE_HID_NAME		"HID"
> +#define	SDCA_FUNCTION_TYPE_IMP_DEF_NAME		"ImplementationDefined"
>   
>   enum sdca_entity0_controls {
>   	SDCA_CONTROL_ENTITY_0_COMMIT_GROUP_MASK		= 0x01,
> diff --git a/sound/soc/sdca/sdca_functions.c b/sound/soc/sdca/sdca_functions.c
> index a69fdb9c8b15..400763e056fa 100644
> --- a/sound/soc/sdca/sdca_functions.c
> +++ b/sound/soc/sdca/sdca_functions.c
> @@ -75,12 +75,20 @@ static int patch_sdca_function_type(struct device *dev,
>   		*function_name = SDCA_FUNCTION_TYPE_HID_NAME;
>   		break;
>   	case SDCA_FUNCTION_TYPE_SIMPLE_AMP:
> +		*function_name = SDCA_FUNCTION_TYPE_SIMPLE_AMP_NAME;
> +		break;
>   	case SDCA_FUNCTION_TYPE_SIMPLE_MIC:
> +		*function_name = SDCA_FUNCTION_TYPE_SIMPLE_MIC_NAME;
> +		break;
>   	case SDCA_FUNCTION_TYPE_SPEAKER_MIC:
> +		*function_name = SDCA_FUNCTION_TYPE_SPEAKER_MIC_NAME;
> +		break;
>   	case SDCA_FUNCTION_TYPE_RJ:
> +		*function_name = SDCA_FUNCTION_TYPE_RJ_NAME;
> +		break;

the changes above are fine...

>   	case SDCA_FUNCTION_TYPE_IMP_DEF:
> -		dev_warn(dev, "unsupported SDCA function type %d\n", *function_type);
> -		return -EINVAL;
> +		*function_name = SDCA_FUNCTION_TYPE_IMP_DEF_NAME;
> +		break;

but this one is controversial. We really have no idea what to do in this 
case. There could be completely different 'imp-def' functions that 
require different drivers. I don't know how the entire binding mechanism 
would work. Probably best to err on the side of throwing a 'not 
supported' error?

>   	default:
>   		dev_err(dev, "invalid SDCA function type %d\n", *function_type);
>   		return -EINVAL;