Re: [PATCH v6 2/2] drm/bridge: Add Lontium LT9611C(EX/UXD) MIPI DSI to HDMI driver

[Sunyun Yang]

Sunyun Yang <syyang@xxxxxxxxxxx> 于2026年6月26日周五 16:40写道：

>
> Maxime Ripard <mripard@xxxxxxxxxx> 于2026年6月26日周五 16:26写道：
> >
> > On Fri, Jun 26, 2026 at 04:13:18PM +0800, Sunyun Yang wrote:
> > > Maxime Ripard <mripard@xxxxxxxxxx> 于2026年6月26日周五 15:49写道：
> > > >
> > > > On Fri, Jun 26, 2026 at 10:15:03AM +0800, Sunyun Yang wrote:
> > > > > Krzysztof Kozlowski <krzk@xxxxxxxxxx> 于2026年6月25日周四 21:51写道：
> > > > > >
> > > > > > On 25/06/2026 15:40, Sunyun Yang wrote:
> > > > > > > Sunyun Yang <syyang@xxxxxxxxxxx> 于2026年6月25日周四 21:26写道：
> > > > > > >>
> > > > > > >> Krzysztof Kozlowski <krzk@xxxxxxxxxx> 于2026年6月25日周四 21:17写道：
> > > > > > >>>
> > > > > > >>> On 25/06/2026 15:14, Sunyun Yang wrote:
> > > > > > >>>> Krzysztof Kozlowski <krzk@xxxxxxxxxx> 于2026年6月25日周四 20:54写道：
> > > > > > >>>>>
> > > > > > >>>>> On 08/05/2026 15:40, syyang@xxxxxxxxxxx wrote:
> > > > > > >>>>>> +
> > > > > > >>>>>> +static void lt9611c_reset(struct lt9611c *lt9611c)
> > > > > > >>>>>> +{
> > > > > > >>>>>> +     gpiod_set_value_cansleep(lt9611c->reset_gpio, 1);
> > > > > > >>>>>> +     msleep(20);
> > > > > > >>>>>> +
> > > > > > >>>>>> +     gpiod_set_value_cansleep(lt9611c->reset_gpio, 0);
> > > > > > >>>>>> +     msleep(20);
> > > > > > >>>>>> +
> > > > > > >>>>>> +     gpiod_set_value_cansleep(lt9611c->reset_gpio, 1);
> > > > > > >>>>>
> > > > > > >>>>> This is just plain wrong. Why do you assert, then de-assert and then
> > > > > > >>>>> finally assert AGAIN the reset leaving the device in powerdown stage?
> > > > > > >>>>>
> > > > > > >>>> I am using software to emulate the hardware RESET button on our EVB.
> > > > > > >>>> When the hardware RESET button is pressed while our chip is running,
> > > > > > >>>> the signal level changes from HIGH to LOW and then back to HIGH.
> > > > > > >>>>
> > > > > > >>>> Of course, we can also use the following:
> > > > > > >>>> static void lt9611c_reset(struct lt9611c *lt9611c)
> > > > > > >>>> {
> > > > > > >>>>     gpiod_set_value_cansleep(lt9611c->reset_gpio, 0);
> > > > > > >>>>     msleep(50);
> > > > > > >>>>     gpiod_set_value_cansleep(lt9611c->reset_gpio, 1);
> > > > > > >>>>     msleep(20);
> > > > > > >>>> }
> > > > > > >>>
> > > > > > >>> Makes no sense either and you just did not get the point and did not
> > > > > > >>> answer my question. I asked WHY you leave asserted. Answer "we emulate"
> > > > > > >>> is just plain wrong.
> > > > > > >>>
> > > > > > >>> So again please answer:
> > > > > > >>>
> > > > > > >>> Why do you leave device with reset asserted?
> > > > > > >>>
> > > > > > >>
> > > > > > >>  devicetree:   reset-gpios = <&tlmm 128 GPIO_ACTIVE_HIGH>;
> > > > > > >>
> > > > > > >> GPIO_ACTIVE_HIGH:
> > > > > > >>
> > > > > > >> gpiod_set_value_cansleep(lt9611c->reset_gpio, 0);   ------   reset pin
> > > > > > >> is Low level : Clear the register configuration in the chip to stop
> > > > > > >> the chip from working.
> > > > > > >>
> > > > > > >> gpiod_set_value_cansleep(lt9611c->reset_gpio, 1);   ------  reset pin
> > > > > > >> is high level:  The chip resumes operation.
> > > > > > >>
> > > > > > >>
> > > > > > >
> > > > > > > Our purpose is: pull the level low to clear the register configuration
> > > > > > > in the chip, and then pull it high to allow the MCU inside the chip to
> > > > > > > re‑initialize the registers.
> > > > > >
> > > > > >
> > > > > > And you do completely opposite... so that confirms your code is just wrong.
> > > > > >
> > > > >
> > > > > The lontium-lt9611.yaml uses GPIO_ACTIVE_HIGH. I am just following the
> > > > > rule of this device tree. If I modify the device tree to use
> > > > > GPIO_ACTIVE_LOW,
> > > > > and use the following code in my driver, then my driver would be correct.
> > > > > However, would the existing kernel drivers lontium-lt9611uxc.c and
> > > > > lontium-lt9611.c be affected?
> > > >
> > > > It might, but then it's a DT problem. The GPIO API for drivers always
> > > > considers the logical state of a GPIO, so if you need to assert a
> > > > signal, you'll always need to set 1. That's what Krzysztof was trying to
> > > > explain.
> > > >
> > > > The DT will provide with GPIO_ACTIVE_* how that logical state translates
> > > > to a physical GPIO state.
> > > >
> > > > If the DT says that this particular GPIO is active-high, then it means
> > > > that we need to set the GPIO to 1 to assert reset. Now of course, it
> > > > might not make sense for the controller itself, but it might for the
> > > > board if there's a GPIO inverter in the middle for example.
> > > >
> > > > Anyway, in the case you're raising, the issue definitely lies in the DT,
> > > > and that's what would need to be fixed.
> > > >
> > > > I also wouldn't be too concerned about lontium-lt9611.yaml, it's just an
> > > > example.
> > > >
> > > > Maxime
> > >
> > > thanks Maxime, I will modify this code in the next version of the
> > > driver, and I hope you can accept these changes.
> > >
> > > Maxime:
> > > I have another question I would like to ask you
> > > regarding sashiko-bot@xxxxxxxxxx. Since sashiko-bot sometimes has
> > > opinions that differ from yours, whose advice should I follow?
> > >
> > > If I do not adopt sashiko-bot's suggestions, will my patches still be
> > > accepted into the upstream Linux kernel?
> >
> > I can't give a blanket answer. It depends on what you ignore exactly.
> >
>
> Okay, another question: sashiko-bot is an AI bot. Are its review
> comments optional, or must they be followed?
>

For example, in my driver, there is a function for upgrading the chip
firmware. During debugging or production, upgrading the chip firmware
will acquire a lock, which will block the DRM callback and affect
display. It will be fine after the upgrade is completed and some
devices are restarted. As long as there is no subsequent upgrade,
display can work normally.

>From a purely software perspective, the AI bot considered this
approach unacceptable and proposed synchronizing the pre-upgrade state
to the DRM framework. From my personal perspective, I think the AI
bot's suggestion would only make my driver more complex and redundant.
Do you think I need to adopt the AI bot's suggestion?

In addition, if I follow the AI bot's suggestion, the
lontium-lt9611uxc.c and lontium-lt8713sx.c drivers that have been
merged into the upstream Linux kernel would not meet the AI bot's
requirements.

When I get a reviewer's Reviewed-by flag, can I ignore the opinion of
sashiko-bot (the AI bot)?


> > Maxime