diff mbox series

drm: bridge: adv7511: Accept audio sample widths of 32 bits via I2S

Message ID 91472c14-3aeb-766a-1716-8219af6e8782@axis.com (mailing list archive)
State New, archived
Headers show
Series drm: bridge: adv7511: Accept audio sample widths of 32 bits via I2S | expand

Commit Message

Ricard Wanderlof May 28, 2024, 10:04 a.m. UTC
Even though data is truncated to 24 bits, the I2S interface does
accept 32 bit data (the slot widths according to the data sheet
can be 16 or 32 bits) so let the hw_params callback reflect this,
even if the lowest 8 bits are not used when 32 bits are specified.

This is normally how 24 bit audio data is handled (i.e. as 32 bit
data, with the LSB:s unused) and this is also reflected in other
bridge drivers which handle audio, for instance sii902x.c and
synopsis/dw-hdmi-i2s-audio.c .

Signed-off-by: Ricard Wanderlof <ricard.wanderlof@axis.com>
---
 drivers/gpu/drm/bridge/adv7511/adv7511_audio.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

Comments

Dmitry Baryshkov May 28, 2024, 2:40 p.m. UTC | #1
On Tue, May 28, 2024 at 12:04:49PM +0200, Ricard Wanderlof wrote:
> 
> Even though data is truncated to 24 bits, the I2S interface does
> accept 32 bit data (the slot widths according to the data sheet
> can be 16 or 32 bits) so let the hw_params callback reflect this,
> even if the lowest 8 bits are not used when 32 bits are specified.
> 
> This is normally how 24 bit audio data is handled (i.e. as 32 bit
> data, with the LSB:s unused) and this is also reflected in other
> bridge drivers which handle audio, for instance sii902x.c and
> synopsis/dw-hdmi-i2s-audio.c .
> 
> Signed-off-by: Ricard Wanderlof <ricard.wanderlof@axis.com>
> ---
>  drivers/gpu/drm/bridge/adv7511/adv7511_audio.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

What about:

Fixes: ae053fa234f4 ("drm: bridge: adv7511: Support I2S IEC958 encoded PCM format")

?
Ricard Wanderlof May 30, 2024, 7:17 a.m. UTC | #2
On Tue, 28 May 2024, Dmitry Baryshkov wrote:

> On Tue, May 28, 2024 at 12:04:49PM +0200, Ricard Wanderlof wrote:
> > 
> > Even though data is truncated to 24 bits, the I2S interface does
> > accept 32 bit data (the slot widths according to the data sheet
> > can be 16 or 32 bits) so let the hw_params callback reflect this,
> > even if the lowest 8 bits are not used when 32 bits are specified.
> ...
> > ---
> >  drivers/gpu/drm/bridge/adv7511/adv7511_audio.c | 11 +++++++----
> >  1 file changed, 7 insertions(+), 4 deletions(-)
> > 
> 
> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> 
> What about:
> 
> Fixes: ae053fa234f4 ("drm: bridge: adv7511: Support I2S IEC958 encoded PCM format")
> 
> ?

IMHO, commit ae053fa234f4 doesn't break anything, so there's nothing to be 
fixed. It adds S/PDIF support, which uses a 32 bit format, but the 32 bit 
format was not supported at all prior to that commit.

I don't really have a problem adding the Fixes tag if you think it's 
useful, but it doesn't seem quite right to me.

/Ricard
Dmitry Baryshkov May 30, 2024, 8:20 a.m. UTC | #3
On Thu, May 30, 2024 at 09:17:03AM +0200, Ricard Wanderlof wrote:
> 
> On Tue, 28 May 2024, Dmitry Baryshkov wrote:
> 
> > On Tue, May 28, 2024 at 12:04:49PM +0200, Ricard Wanderlof wrote:
> > > 
> > > Even though data is truncated to 24 bits, the I2S interface does
> > > accept 32 bit data (the slot widths according to the data sheet
> > > can be 16 or 32 bits) so let the hw_params callback reflect this,
> > > even if the lowest 8 bits are not used when 32 bits are specified.
> > ...
> > > ---
> > >  drivers/gpu/drm/bridge/adv7511/adv7511_audio.c | 11 +++++++----
> > >  1 file changed, 7 insertions(+), 4 deletions(-)
> > > 
> > 
> > Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > 
> > What about:
> > 
> > Fixes: ae053fa234f4 ("drm: bridge: adv7511: Support I2S IEC958 encoded PCM format")
> > 
> > ?
> 
> IMHO, commit ae053fa234f4 doesn't break anything, so there's nothing to be 
> fixed. It adds S/PDIF support, which uses a 32 bit format, but the 32 bit 
> format was not supported at all prior to that commit.

Agreed, let's keep it out of the question.

> 
> I don't really have a problem adding the Fixes tag if you think it's 
> useful, but it doesn't seem quite right to me.
> 
> /Ricard
> -- 
> Ricard Wolf Wanderlof                           ricardw(at)axis.com
> Axis Communications AB, Lund, Sweden            www.axis.com
> Phone +46 46 272 2016                           Fax +46 46 13 61 30
Ricard Wanderlof July 29, 2024, 8:15 a.m. UTC | #4
Hi,

I submitted the patch below a while ago (two months) but as far as I can 
make out it has not been included. There was an initial concern from 
Dmitry Baryshkov which was subsequently addressed but no other objections. 

On Tue, 28 May 2024, Ricard Wanderlof wrote:

> 
> Even though data is truncated to 24 bits, the I2S interface does
> accept 32 bit data (the slot widths according to the data sheet
> can be 16 or 32 bits) so let the hw_params callback reflect this,
> even if the lowest 8 bits are not used when 32 bits are specified.
> 
> This is normally how 24 bit audio data is handled (i.e. as 32 bit
> data, with the LSB:s unused) and this is also reflected in other
> bridge drivers which handle audio, for instance sii902x.c and
> synopsis/dw-hdmi-i2s-audio.c .
> 
> Signed-off-by: Ricard Wanderlof <ricard.wanderlof@axis.com>
> ---
>  drivers/gpu/drm/bridge/adv7511/adv7511_audio.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_audio.c b/drivers/gpu/drm/bridge/adv7511/adv7511_audio.c
> index 61f4a38e7d2b..4563f5d8136f 100644
> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_audio.c
> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_audio.c
> @@ -101,11 +101,14 @@ static int adv7511_hdmi_hw_params(struct device *dev, void *data,
>  	case 20:
>  		len = ADV7511_I2S_SAMPLE_LEN_20;
>  		break;
> -	case 32:
> -		if (fmt->bit_fmt != SNDRV_PCM_FORMAT_IEC958_SUBFRAME_LE)
> -			return -EINVAL;
> -		fallthrough;
>  	case 24:
> +	case 32:
> +		/*
> +		 * 32 bits are handled like 24 bits, except that the lowest
> +		 * 8 bits are discarded. In fact, the accepted I2S slot widths
> +		 * are 16 and 32 bits, so the chip is fully compatible with
> +		 * 32 bit data.
> +		 */
>  		len = ADV7511_I2S_SAMPLE_LEN_24;
>  		break;
>  	default:
> -- 
> 2.30.2
> 
> 

I recently discovered that the maintainer for the ADV7511 driver (in the 
I2C) framework is not included by the get_maintainers script, so perhaps 
this is the reason?

Otherwise, please enlighten me on what I need to do to get this patch 
accepted!

/Ricard
Laurent Pinchart Aug. 1, 2024, 5:30 p.m. UTC | #5
On Mon, Jul 29, 2024 at 10:15:55AM +0200, Ricard Wanderlof wrote:
> 
> Hi,
> 
> I submitted the patch below a while ago (two months) but as far as I can 
> make out it has not been included. There was an initial concern from 
> Dmitry Baryshkov which was subsequently addressed but no other objections. 
> 
> On Tue, 28 May 2024, Ricard Wanderlof wrote:
> 
> > 
> > Even though data is truncated to 24 bits, the I2S interface does
> > accept 32 bit data (the slot widths according to the data sheet
> > can be 16 or 32 bits) so let the hw_params callback reflect this,
> > even if the lowest 8 bits are not used when 32 bits are specified.
> > 
> > This is normally how 24 bit audio data is handled (i.e. as 32 bit
> > data, with the LSB:s unused) and this is also reflected in other
> > bridge drivers which handle audio, for instance sii902x.c and
> > synopsis/dw-hdmi-i2s-audio.c .
> > 
> > Signed-off-by: Ricard Wanderlof <ricard.wanderlof@axis.com>
> > ---
> >  drivers/gpu/drm/bridge/adv7511/adv7511_audio.c | 11 +++++++----
> >  1 file changed, 7 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_audio.c b/drivers/gpu/drm/bridge/adv7511/adv7511_audio.c
> > index 61f4a38e7d2b..4563f5d8136f 100644
> > --- a/drivers/gpu/drm/bridge/adv7511/adv7511_audio.c
> > +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_audio.c
> > @@ -101,11 +101,14 @@ static int adv7511_hdmi_hw_params(struct device *dev, void *data,
> >  	case 20:
> >  		len = ADV7511_I2S_SAMPLE_LEN_20;
> >  		break;
> > -	case 32:
> > -		if (fmt->bit_fmt != SNDRV_PCM_FORMAT_IEC958_SUBFRAME_LE)
> > -			return -EINVAL;
> > -		fallthrough;
> >  	case 24:
> > +	case 32:
> > +		/*
> > +		 * 32 bits are handled like 24 bits, except that the lowest
> > +		 * 8 bits are discarded. In fact, the accepted I2S slot widths
> > +		 * are 16 and 32 bits, so the chip is fully compatible with
> > +		 * 32 bit data.
> > +		 */
> >  		len = ADV7511_I2S_SAMPLE_LEN_24;
> >  		break;
> >  	default:
> 
> I recently discovered that the maintainer for the ADV7511 driver (in the 
> I2C) framework is not included by the get_maintainers script, so perhaps 
> this is the reason?
> 
> Otherwise, please enlighten me on what I need to do to get this patch 
> accepted!

I have no experience with HDMI audio, so I didn't comment on your patch.

Hans, is this within your area of expertise ?
diff mbox series

Patch

diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_audio.c b/drivers/gpu/drm/bridge/adv7511/adv7511_audio.c
index 61f4a38e7d2b..4563f5d8136f 100644
--- a/drivers/gpu/drm/bridge/adv7511/adv7511_audio.c
+++ b/drivers/gpu/drm/bridge/adv7511/adv7511_audio.c
@@ -101,11 +101,14 @@  static int adv7511_hdmi_hw_params(struct device *dev, void *data,
 	case 20:
 		len = ADV7511_I2S_SAMPLE_LEN_20;
 		break;
-	case 32:
-		if (fmt->bit_fmt != SNDRV_PCM_FORMAT_IEC958_SUBFRAME_LE)
-			return -EINVAL;
-		fallthrough;
 	case 24:
+	case 32:
+		/*
+		 * 32 bits are handled like 24 bits, except that the lowest
+		 * 8 bits are discarded. In fact, the accepted I2S slot widths
+		 * are 16 and 32 bits, so the chip is fully compatible with
+		 * 32 bit data.
+		 */
 		len = ADV7511_I2S_SAMPLE_LEN_24;
 		break;
 	default: