On Tue, Feb 18, 2025 at 04:52:53PM +0100, Maxime Ripard wrote:
> On Tue, Feb 18, 2025 at 02:14:43PM +0200, Dmitry Baryshkov wrote:
> > On Tue, Feb 18, 2025 at 10:55:49AM +0100, Maxime Ripard wrote:
> > > On Fri, Feb 14, 2025 at 08:26:02AM -0800, Doug Anderson wrote:
> > > > Hi,
> > > >
> > > > On Thu, Feb 13, 2025 at 12:44 PM Anusha Srivatsa <asrivats@xxxxxxxxxx> wrote:
> > > > >
> > > > > A lot of mipi API are deprecated and have a _multi() variant
> > > > > which is the preferred way forward. This covers TODO in the
> > > > > gpu Documentation:[1]
> > > > >
> > > > > An incomplete effort was made in the previous version
> > > > > to address this[2]. It removed on the mipi_dsi_dcs_write_seq()
> > > > > and mipi_dsi_generic_write_seq_multi() with the respective
> > > > > replacemts and not the rest of the API.
> > > >
> > > > You didn't seem to take most of the suggestions I gave in response to
> > > > your v1 [3]. Specifically:
> > > >
> > > > a) I asked that you CC Tejas. I've added him again.
> > > >
> > > > b) I asked that you CC me on the whole patch series, which you didn't
> > > > do. I can find them, but I'd find it convenient in this case for them
> > > > to be in my Inbox.
> > > >
> > > > The first patch conflicts with what Tejas already landed in
> > > > drm-misc-next. See commit 8025f23728e9 ("drm/panel:
> > > > xinpeng-xpp055c272: transition to mipi_dsi wrapped functions"). The
> > > > second patch _also_ conflicts with what Tejas already landed. See
> > > > commit f4dd4cb79f9e ("drm/panel: visionox-r66451: transition to
> > > > mipi_dsi wrapped functions"). Later patches also also conflict. See
> > > > commit 0d6c9edf9e5b ("drm/panel: ebbg-ft8719: transition to mipi_dsi
> > > > wrapped functions"), commit ce8c69ec90ca ("drm/panel:
> > > > samsung-s6e88a0-ams452ef01: transition to mipi_dsi wrapped
> > > > functions"), and commit 7e3bf00047cd ("drm/panel: sharp-ls060t1sx01:
> > > > transition to mipi_dsi wrapped functions"). Maybe you should sync up
> > > > with drm-misc-next before submitting.
> > >
> > > Yes, you should definitely work from drm-misc-next there, and sync with
> > > Tejas.
> > >
> > > > I also questioned whether this really made sense to try to do with a
> > > > Coccinelle script and I still don't think so. It looks like Dmitry has
> > > > already reviewed the first few of your patches and has repeated my
> > > > advice. If you want to help with the effort of addressing this TODO
> > > > item then that's great, but I'll stop reviewing (and start silently
> > > > deleting) any future submissions of yours that say that they're done
> > > > entirely with a Coccinelle script unless you address this point and
> > > > convince me that your Coccinelle script is really smart enough to
> > > > handle all the corner cases. I'll also assert that you should review
> > > > Tejas's submissions to see how these conversions are expected to go.
> > >
> > > I couldn't find that in your first answer though. What corner cases do
> > > you have in mind, and why do you think coccinelle can't handle them?
> >
> > As can be seen from the reviews:
> >
> > - sleeps between DSI calls
> > - properly propagating the error at the end of the function
>
> These two are legitimate feedback, but I don't see how coccinelle cannot
> deal with them.
Maybe it can. both issues were pointed out during review of the first
series, there was no improvement here. I'd really ask to perform
conversion of a single driver, so that the script (or post-script
fixups) can be improved. I'd still expect that Anusha actually reviews
the changed driver before posting it and verifies that there is no
regression in the logic / error reporting.
>
> > - making decision whether to create the context at the caller or the
> > callee side. E.g. in patch 8 it is better to allocate context in
> > hx8394_enable() and pass it to .init_sequence() instead of keeping
> > some of error handling.
>
> Yeah, that one is definitely subjective, and is going to need manual
> review.
--
With best wishes
Dmitry
