Maxime Ripard <[email protected]> 于2026年6月26日周五 17:53写道： > > On Fri, Jun 26, 2026 at 05:45:08PM +0800, Sunyun Yang wrote: > > Maxime Ripard <[email protected]> 于2026年6月26日周五 17:31写道： > > > > > > On Fri, Jun 26, 2026 at 05:05:38PM +0800, Sunyun Yang wrote: > > > > Sunyun Yang <[email protected]> 于2026年6月26日周五 16:40写道： > > > > > > > > > > > > > > Maxime Ripard <[email protected]> 于2026年6月26日周五 16:26写道： > > > > > > > > > > > > On Fri, Jun 26, 2026 at 04:13:18PM +0800, Sunyun Yang wrote: > > > > > > > Maxime Ripard <[email protected]> 于2026年6月26日周五 15:49写道： > > > > > > > > > > > > > > > > On Fri, Jun 26, 2026 at 10:15:03AM +0800, Sunyun Yang wrote: > > > > > > > > > Krzysztof Kozlowski <[email protected]> 于2026年6月25日周四 21:51写道： > > > > > > > > > > > > > > > > > > > > On 25/06/2026 15:40, Sunyun Yang wrote: > > > > > > > > > > > Sunyun Yang <[email protected]> 于2026年6月25日周四 21:26写道： > > > > > > > > > > >> > > > > > > > > > > >> Krzysztof Kozlowski <[email protected]> 于2026年6月25日周四 > > > > > > > > > > >> 21:17写道： > > > > > > > > > > >>> > > > > > > > > > > >>> On 25/06/2026 15:14, Sunyun Yang wrote: > > > > > > > > > > >>>> Krzysztof Kozlowski <[email protected]> 于2026年6月25日周四 > > > > > > > > > > >>>> 20:54写道： > > > > > > > > > > >>>>> > > > > > > > > > > >>>>> On 08/05/2026 15:40, [email protected] 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 [email protected]. 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? > > > > > > You're right it's wrong, but there's also no reason to allow that > > > firmware upgrade during the driver lifetime either through a debugfs > > > file. So you should get rid of the debugfs file and now you won't have > > > to bother with that comment anymore. > > > > > > > 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. > > > > > > Maybe, but it's not relevant either way. These drivers are merged, yours > > > isn't. Feel free to fix these if you spot any issue though. > > > > > > > When I get a reviewer's Reviewed-by flag, can I ignore the opinion of > > > > sashiko-bot (the AI bot)? > > > > > > Again, it depends on the comment. If you feel like one can be ignored, > > > you should at least argue why. > > > > > > > sashiko‑bot (AI bot) does not communicate or interact with me. > > Even if I explain the reasons, the AI bot never replies to me, and it > > only sends its review comments to me alone. > > Then put it into the cover letter? >
thanks Maxime, I will modify the driver patch, include the relevant explanation in the cover letter, and then submit it. > > If I do not make changes according to the AI bot's suggestions, will > > that affect my driver being merged into the upstream Linux kernel? I > > would like to know the answer to this question. > > I told you twice already.
