Re: [PATCH v14 03/10] drm/edid: Add cea_sad helpers for freq/length

AngeloGioacchino Del Regno
2022-07-14T11:12:53Z

Il 12/07/22 13:12, Bo-Chen Chen ha scritto:
> From: Guillaume Ranquet <granquet@baylibre.com>
>
> This patch adds two helper functions that extract the frequency and word
> length from a struct cea_sad.
>
> For these helper functions new defines are added that help translate the
> 'freq' and 'byte2' fields into real numbers.
>
> Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com>
> Signed-off-by: Guillaume Ranquet <granquet@baylibre.com>
> Signed-off-by: Bo-Chen Chen <rex-bc.chen@mediatek.com>
> ---
> drivers/gpu/drm/drm_edid.c | 73 ++++++++++++++++++++++++++++++++++++++
> include/drm/drm_edid.h | 14 ++++++++
> 2 files changed, 87 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index bc43e1b32092..79316d7f1fd8 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -4916,6 +4916,79 @@ int drm_edid_to_speaker_allocation(const struct edid *edid, u8 **sadb)
> }
> EXPORT_SYMBOL(drm_edid_to_speaker_allocation);
>
> +/**
> + * drm_cea_sad_get_sample_rate - Extract the sample rate from cea_sad
> + * @sad: Pointer to the cea_sad struct
> + *
> + * Extracts the cea_sad frequency field and returns the sample rate in Hz.
> + *
> + * Return: Sample rate in Hz or a negative errno if parsing failed.
> + */
> +int drm_cea_sad_get_sample_rate(const struct cea_sad *sad)
> +{
> + switch (sad->freq) {
> + case DRM_CEA_SAD_FREQ_32KHZ:
> + return 32000;
> + case DRM_CEA_SAD_FREQ_44KHZ:
> + return 44100;
> + case DRM_CEA_SAD_FREQ_48KHZ:
> + return 48000;
> + case DRM_CEA_SAD_FREQ_88KHZ:
> + return 88200;
> + case DRM_CEA_SAD_FREQ_96KHZ:
> + return 96000;
> + case DRM_CEA_SAD_FREQ_176KHZ:
> + return 176400;
> + case DRM_CEA_SAD_FREQ_192KHZ:
> + return 192000;
> + default:
> + return -EINVAL;
> + }
> +}
> +EXPORT_SYMBOL(drm_cea_sad_get_sample_rate);
> +
> +static bool drm_cea_sad_is_pcm(const struct cea_sad *sad)
> +{
> + switch (sad->format) {
> + case HDMI_AUDIO_CODING_TYPE_PCM:
> + return true;
> + default:
> + return false;
> + }

Are you sure that you need this helper? That's used only in one function...
...if you really need this one, though, I don't think that using a switch
is the best option here.

Unless anyone is against that (please, reason?), I would be for doing it like:

return sad->format == HDMI_AUDIO_CODING_TYPE_PCM;

Everything else looks good to me (and working, too).

Cheers,
Angelo