Re: [PATCH] drm: bridge: adv7511: Accept audio sample widths of 32 bits via I2S

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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 ?

-- 
Regards,

Laurent Pinchart