Message-ID: <ipjzhwkvmdvoxuai3yl6mrl3jm4gahnhtnuqln473vlsz37axu@ptpef36fwcid>
Date: Fri, 5 Sep 2025 17:10:01 +0300
From: Dmitry Baryshkov <dmitry.baryshkov@....qualcomm.com>
To: 杨孙运 <yangsunyun1993@...il.com>
Cc: syyang <syyang@...tium.com>, robh@...nel.org, krzk+dt@...nel.org,
        conor+dt@...nel.org, andrzej.hajda@...el.com,
        neil.armstrong@...aro.org, rfoss@...nel.org,
        Laurent.pinchart@...asonboard.com, jonas@...boo.se,
        jernej.skrabec@...il.com, devicetree@...r.kernel.org,
        dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org,
        xmzhu@...tium.com, llzhang@...tium.com, rly@...tium.com
Subject: Re: [PATCH v1 2/2] This patch adds a new DRM bridge driver for the
 Lontium LT9611C chip.

On Fri, Sep 05, 2025 at 10:55:51AM +0800, 杨孙运 wrote:
> Dmitry Baryshkov <dmitry.baryshkov@....qualcomm.com> 于2025年9月4日周四 22:39写道：
> 
> >
> > On Thu, Sep 04, 2025 at 06:48:13PM +0800, 杨孙运 wrote:
> > > Dmitry Baryshkov <dmitry.baryshkov@....qualcomm.com> 于2025年9月4日周四 10:52写道：
> > > >
> > > > On Wed, Sep 03, 2025 at 05:38:25AM -0700, syyang wrote:
> > > > > The following changes are included:
> > > > >
> > > > > - Updated Kconfig and Makefile to include the new driver
> > > > > - Implementation of the bridge driver at
> > > > >   drivers/gpu/drm/bridge/lontium-lt9611c.c
> > > >
> > > > This is really not interesting, it can be seen from the patch itself.
> > > > Please read Documentation/process/submitting-patches.rst.
> > > >
> > > Sorry,  I will study submitting-patches.rst.
> > >
> > > > Is it possible to toggle infoframes?
> > >
> > > sorry, I don't understand the meaning of this sentence. Please explain
> > > it in detail.
> >
> > Is it possible to control InfoFrames being sent over the HDMI cable?
> > Both contents and enabling/disabling.
> >
> Can be controlled via I2C

Then please implement DRM_BRIDGE_OP_HDMI and corresponding InfoFrame
callbacks.

> > >
> > > > > +
> > > > > +#define FW_SIZE (64 * 1024)
> > > > > +#define LT_PAGE_SIZE 256
> > > > > +#define FW_FILE  "LT9611C.bin"
> > > >
> > > > Please land this firmware to the linux-firmware repository.
> > > >
> > >
> > > The LT9611C has built-in firmware, and when the chip is running, it
> > > uses the internal firmware.
> > > The firmware needs to be updated only when the customer encounters
> > > issues during the debugging phase due to changes in hardware design or
> > > parameters.
> > > When the customer needs to update the firmware, they will apply to our
> > > company for a customized firmware.
> > > They will place this firmware under /lib/firmware, and then reboot the
> > > platform. After that, the driver will update the firmware.
> > > So I think there is no need to upload the firmware.
> >
> > Then please make firmware updates opt-in, requiring some user action.
> > I'd suggest first getting the simplfified version of the driver in
> > (without fw update capabilities) and then adding FW upgrades in as a
> > separate patch.
> >
> I will research it.

Research... what?

> > > > > +static unsigned int get_crc(struct crc_info type, const u8 *buf, u64 buf_len)
> > > >
> > > > Use library functions for that.
> > > >
> > >
> > > I'm not sure whether the algorithms in the llibrary functions are
> > > consistent with those designed in our chip.
> > > If either of them changes, it will cause the firmware updated to the
> > > chip to fail to run.
> >
> > CRC polynoms don't change that easily
> >
> > > I think it's safer to implement it using our own code.
> >
> > No, it's not.
> >
> If we calculate CRC_1 using the library function and then burn it
> together with the firmware into the chip, when the chip boot, it will
> use the internal hardware to calculate the firmware CRC_2.
> If CRC_1 is not equal to CRC_2, the chip will fail to boot. The
> library function will not be changed. I'm worried that the algorithm
> in our chip's hardware is different from the library function. I'll
> research it.

Please take a look first, before having fears or implenting yet another
CRC function.

> 
> > > I'll check it.
> > > > > +static int lt9611c_prepare_firmware_data(struct lt9611c *lt9611c)
> > > > > +{
> > > > > +     struct device *dev = lt9611c->dev;
> > > > > +     int ret;
> > > > > +
> > > > > +     /* ensure filesystem ready */
> > > > > +     msleep(3000);
> > > >
> > > > No. If the firmware is necessary and it's not ready, return
> > > > -EPROBE_DEFER.
> > > >
> > > The firmware is unnecessary . This part of the code is for customers
> > > who need to upgrade the chip firmware.
> > >
> > > Due to the different designs of the platform, the firmware used by
> > > each customer may be different.
> >
> > Well... That's a very bad way to go. We have had this issue with
> > LT9611UXC at one of my previous jobs. Our customers have had various
> > kinds of issues because of the wrong firmware.
> >
> > Please provide some reference, which works in a DSI-to-HDMI case and
> > make it _tunable_ rather than requiring to replace the firmware
> > completely.
> >
> i will research it.
> Yes, you worked together with my colleagues to handle the issue of
> LT9611UXC. (At that time, you used dmitry.baryshkov@...aro.org)
> 
> > >
> > > Therefore, when they need to update the firmware, they only need to
> > > compile the firmware into the /lib/firmware directory during the
> > > compilation
> > > process, and then burn the image into the platform.
> > >
> > > Once reboot platform, the firmware upgrade can be automatically completed.
> >
> > The firmware upgrade must be triggered by user, unless the FW is
> > completely empty.
> >
> Is it necessary for the authorities to insist on doing so?

I think so. The rootfs might be the same for different devices,
different use cases, etc. So your 'load and program firmware if it's
present in rootfs' doesn't work in a general case.

> > > > > +static const struct drm_edid *lt9611c_bridge_edid_read(struct drm_bridge *bridge,
> > > > > +                                                    struct drm_connector *connector)
> > > > > +{
> > > > > +     struct lt9611c *lt9611c = bridge_to_lt9611c(bridge);
> > > > > +
> > > > > +     usleep_range(10000, 20000);
> > > >
> > > > Why?
> > > >
> > > Delay for a while to ensure that EDID is ready.
> >
> > Your other chip had interrupt status to note that EDID is ready. I hope
> > you have that one too. Blindly calling usleep_range() is a bad idea.
> >
> Different chips have different logic. i will research it.

Thanks.

> > > > > +static int lt9611c_hdmi_hw_params(struct device *dev, void *data,
> > > > > +                               struct hdmi_codec_daifmt *fmt,
> > > > > +                              struct hdmi_codec_params *hparms)
> > > > > +{
> > > > > +     struct lt9611c *lt9611c = dev_get_drvdata(dev);
> > > > > +
> > > > > +      dev_info(lt9611c->dev, "SOC sample_rate: %d, sample_width: %d, fmt: %d\n",
> > > > > +               hparms->sample_rate, hparms->sample_width, fmt->fmt);
> > > > > +
> > > > > +     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:
> > > > > +             break;
> > > > > +     default:
> > > > > +             return -EINVAL;
> > > > > +     }
> > > > > +
> > > > > +     switch (fmt->fmt) {
> > > > > +     case HDMI_I2S:
> > > > > +     case HDMI_SPDIF:
> > > > > +             break;
> > > > > +     default:
> > > > > +             return -EINVAL;
> > > > > +     }
> > > >
> > > > Does that add anything on top of the limitations of hdmi-codec.c?
> > > >
> > > The parameters supported in the hdmi-codec.c may not be supported by
> > > my chip. Therefore, we can exclude the parameters that are not
> > > supported by the chip.
> >
> > Are they?
> >
> The firmware handles all parameter adaptation autonomously. This code
> merely needs to expose the chip's capabilities to hdmi-codec.c.

Which params are supported by hdmi-codec but not suppored by your chip?

> 
> > >
> > > > > +
> > > > > +     return 0;
> > > > > +}
> > > > > +
> > > > > +static void lt9611c_audio_shutdown(struct device *dev, void *data)
> > > > > +{
> > > > > +}
> > > > > +
> > > > > +static int lt9611c_audio_startup(struct device *dev, void *data)
> > > > > +{
> > > > > +     return 0;
> > > > > +}
> > > > > +
> > > > > +static void lt9611c_audio_update_connector_status(struct lt9611c *lt9611c)
> > > > > +{
> > > > > +     enum drm_connector_status status;
> > > > > +
> > > > > +     status = lt9611c->audio_status;
> > > > > +     if (lt9611c->plugged_cb && lt9611c->codec_dev)
> > > > > +             lt9611c->plugged_cb(lt9611c->codec_dev,
> > > > > +                              status == connector_status_connected);
> > > > > +}
> > > > > +
> > > > > +static int lt9611c_hdmi_audio_hook_plugged_cb(struct device *dev,
> > > > > +                                           void *data,
> > > > > +                                      hdmi_codec_plugged_cb fn,
> > > > > +                                      struct device *codec_dev)
> > > > > +{
> > > > > +     struct lt9611c *lt9611c = data;
> > > > > +
> > > > > +     lt9611c->plugged_cb = fn;
> > > > > +     lt9611c->codec_dev = codec_dev;
> > > > > +     lt9611c_audio_update_connector_status(lt9611c);
> > > > > +
> > > > > +     return 0;
> > > > > +}
> > > > > +
> > > > > +static const struct hdmi_codec_ops lt9611c_codec_ops = {
> > > > > +     .hw_params      = lt9611c_hdmi_hw_params,
> > > > > +     .audio_shutdown = lt9611c_audio_shutdown,
> > > > > +     .audio_startup = lt9611c_audio_startup,
> > > > > +     .hook_plugged_cb = lt9611c_hdmi_audio_hook_plugged_cb,
> > > > > +};
> > > >
> > > > No, we have HDMI audio helpers for that. Drop this and use the helpers
> > > > instead.
> > > >
> > > ？？？ I don't understand.
> >
> > See <drm/display/drm_hdmi_audio_helper.h> and
> > https://lore.kernel.org/dri-devel/20250803-lt9611uxc-hdmi-v1-2-cb9ce1793acf@oss.qualcomm.com/
> >
> i will research, thks.
> Could you please share the latest driver file for lt9611uxc.c that you
> have written? (to this email: syyang@...tium.com)

No, I will not. It's called upstream, because everything is either in
the tree or on the mailing list.

> > > > > +     if (lt9611c->audio_pdev)
> > > > > +             lt9611c_audio_exit(lt9611c);
> > > > > +
> > > > > +     if (lt9611c->fw) {
> > > >
> > > > You definitely don't need firmware when the bridge is up and running.
> > > >
> > > The previous text has already described the working logic of the firmware.
> >
> > It's another topic: you are storing the firmware in memory while the
> > driver is bound. It's not necessary. You can release it after updating
> > it on the chip.
> >
> I understand what you mean.

No, you don't. The driver can call release_firmware() after flashing the
chip. That's it at this point. Nothing more.

> Based on the above conversation, your intention is that when the
> customer needs to upgrade the firmware, they should modify the
> comparison conditions of the version, then compile and burn the
> kernel, and then perform the firmware upgrade, just like the LT9611UXC
> driver. Instead of loading the firmware every time.

Of course not. I've asked to add a way for the user to trigger firmware
update. It might be a sysfs file (like I did for lt9611uxc), it can be
modparam or anything else. But again, this is not relevant _here_.

I simply ask you to release the memory after it is no longer needed.


> My design intention is to avoid the need for recompiling the driver
> when upgrading. Instead, a file named "LT9611C.bin" can be directly
> sent to the "/lib/firmware" directory via scp. Then you can either
> perform a reboot for the upgrade or execute the command manually for
> the upgrade.

Rootfs might be read-only, it might be shared via NFS, it might be
coming from the cloud via Docker container, etc.

> Perhaps you are suggesting that we could follow the design approach of
> the LT9611UXC driver?
> 
> > > > > +
> > > > > +     lt9611c->kthread = kthread_run(lt9611c_main, lt9611c, "lt9611c");
> > > >
> > > > Why do you need extra kthread for that???
> 
> Upgrading the firmware takes time. execute it sequentially in the
> probe function, it will block the system boot.
> Using the kthread method will not block the system boot.

Using async probing. Or program firmware after checking all resources
that are required for the device.

Also, as you've written, firmware flashing is an exception, so delaying
one exceptional boot is not a real issue.

And anyway, this requires a comment in the source file and a comment in
the commit message.


-- 
With best wishes
Dmitry

Powered by blists - more mailing lists