Re: [PATCH v3 2/2] drm/bridge: This patch add new DRM bridge driver for LT9611C(EX/UXD) chip

[Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>]

On Wed, Apr 22, 2026 at 12:41:11PM +0800, 杨孙运 wrote:
> Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com> 于2026年4月21日周二 18:48写道：
> >
> > On Tue, Apr 21, 2026 at 03:37:52PM +0800, 杨孙运 wrote:
> > > Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com> 于2026年4月20日周一 20:18写道：
> > > >
> > > > On Mon, Apr 20, 2026 at 02:16:44PM +0800, syyang@lontium.com wrote:
> > > > > From: Sunyun Yang <syyang@lontium.com>
> > >
> > > > > +
> > > > > +static int lt9611c_hdmi_audio_prepare(struct drm_bridge *bridge,
> > > > > +                                   struct drm_connector *connector,
> > > > > +                                   struct hdmi_codec_daifmt *fmt,
> > > > > +                                   struct hdmi_codec_params *hparms)
> > > > > +{
> > > > > +     struct lt9611c *lt9611c = bridge_to_lt9611c(bridge);
> > > > > +     u8 audio_cmd[6] = {0x57, 0x48, 0x36, 0x3a};
> > > > > +     u8 data[5];
> > > > > +     int ret;
> > > > > +
> > > > > +     /* Validate sample rate and width (LT9611C auto-detects but we still check) */
> > > >
> > > > What for? You don't trust ASoC / ALSA core that the rates would match
> > > > HDMI_RATES?
> > >
> > > Chip has limitations on sample_rate, sample_width, and fmt, and cannot
> > > support all formats.
> > > The validation here is to ensure that only the formats supported by
> > > the chip are used.
> >
> > Again, for sample rates, is it different from HDMI_RATES? If not, it is
> > useless protective coding.
> >
> 
> I checked the definition of HDMI_RATES and ended up doing a redundant
> validation.
> 
> #define HDMI_RATES (SNDRV_PCM_RATE_32000 | SNDRV_PCM_RATE_44100 |\
> SNDRV_PCM_RATE_48000 | SNDRV_PCM_RATE_88200 |\
> SNDRV_PCM_RATE_96000 | SNDRV_PCM_RATE_176400 |\
> SNDRV_PCM_RATE_192000)
> 
> However, if HDMI_RATES is extended in the future to include sample
> rate not supported by LT9611C, this driver would require corresponding
> updates.
> If this is not expected to happen, I can remove the redundant check.

Updating HDMI_RATES would cause enough issues for the existign drivers
which don't have this kind of a check.

> 
> > > >
> > > > > +     switch (hparms->sample_rate) {
> > > > > +     case 32000:
> > > > > +     case 44100:
> > > > > +     case 48000:
> > > > > +     case 88200:
> > > > > +     case 96000:
> > > > > +     case 176400:
> > > > > +     case 192000:
> > > > > +             break;
> > > > > +     default:
> > > > > +             return -EINVAL;
> > > > > +     }
> > > > > +
> > > > > +     switch (hparms->sample_width) {
> > > > > +     case 16:
> > > > > +     case 18:
> > > > > +     case 20:
> > > > > +     case 24:
> > > >
> > > > and no support for 32?
> > > >
> > > no support for 32
> >
> > Then check for that rather than listing all the widths. If you check the
> > hdmi-codec, you can't get 18-bit samples at all.
> >
> You are right, there is no 18-bit support.
> 
> same reason with sample rate.

Yep. Check for 32-bit, the rest is fine.

> 
> > > > > +MODULE_LICENSE("GPL v2");
> > > >
> > > >
> > > > I think, checkpatch.pl should have flagged this.
> > > >
> > > yes,  it is need use GPL?
> >
> > If it flagged the line, why didn't you fix it? What did checkpatch say?
> >
> Sorry, it is my mistake. i will fix.
> checkpatch say:
> WARNING: Prefer "GPL" over "GPL v2" - see commit bf7fbeeae6db
> ("module: Cure the MODULE_LICENSE "GPL" vs. "GPL v2" bogosity")
> #1450: FILE: drivers/gpu/drm/bridge/lontium-lt9611c.c:1364:
> 

So... If there was a warning, why did you ignore it? Please don't,
unless you really know what you are doing.

> 
> > > I see that many of the latest code still use GPL v2.
> >
> > --
> > With best wishes
> > Dmitry

-- 
With best wishes
Dmitry