From: Saravana Kannan <email@example.com> To: Stephen Boyd <firstname.lastname@example.org> Cc: Greg Kroah-Hartman <email@example.com>, LKML <firstname.lastname@example.org>, linux-arm-msm <email@example.com>, dri-devel <firstname.lastname@example.org>, freedreno <email@example.com>, Daniel Vetter <firstname.lastname@example.org>, "Rafael J. Wysocki" <email@example.com>, Rob Clark <firstname.lastname@example.org>, Russell King <email@example.com> Subject: Re: [PATCH 0/7] component: Make into an aggregate bus Date: Thu, 20 May 2021 12:30:09 -0700 [thread overview] Message-ID: <CAGETcx-mRrqC_sGiBk+wx8RtwjJjXf0KJo+ejU6SweEBiATaLw@mail.gmail.com> (raw) In-Reply-To: <CAE-0n522QRUfQOSGmYS59AbFdx2kmtz-CNszdWfLnPCbMkCryA@mail.gmail.com> On Wed, May 19, 2021 at 6:41 PM Stephen Boyd <firstname.lastname@example.org> wrote: > > Quoting Saravana Kannan (2021-05-19 18:27:50) > > On Wed, May 19, 2021 at 5:25 PM Stephen Boyd <email@example.com> wrote: > > > > > > This series is from discussion we had on reordering the device lists for > > > drm shutdown paths. I've introduced an 'aggregate' bus that we put > > > the aggregate device onto and then we probe the device once all the > > > components are probed and call component_add(). The probe/remove hooks > > > are where the bind/unbind calls go, and then a shutdown hook is added > > > that can be used to shutdown the drm display pipeline at the right time. > > > > > > This works for me on my sc7180 board, but I'm currently struggling with > > > the last patch where we migrate the msm driver. It runs into a runtime > > > PM problem where the parent device isn't runtime PM enabled yet. I'm > > > still trying to figure out a clean solution there. Moving runtime PM > > > around breaks boot and I think that's because the power domain is off. > > > > > > Cc: Daniel Vetter <firstname.lastname@example.org> > > > Cc: "Rafael J. Wysocki" <email@example.com> > > > Cc: Rob Clark <firstname.lastname@example.org> > > > Cc: Russell King <email@example.com> > > > Cc: Saravana Kannan <firstname.lastname@example.org> > > > > > >  https://email@example.com > > > > > > > I skimmed through the series and in general the idea is good, but I'm > > not sure why each component user needs to be converted/"modern" before > > it can make use of the benefits of this series. Why not just have > > wrapper functions around the component ops that the new aggregate bus > > driver can just call? That'll give all the existing component users > > the new ability to use the new ops without having to have two > > versions. > > The existing users can only have one or the other. Either use the ops > structure or use the struct aggregate_driver. What benefits of this > series are they not gaining? As I mentioned earlier, if we add device links between the aggregate device (consumer) and all the component devices (suppliers), it'll take care of a lot of the ordering issues (probe, suspend, runtime PM) and dependency issues (unbind the master device if a component driver unbinds). It'll allow us to delete a lot of the code in the component framework too. I can send the patch for the device links once your series settles. So having two implementations comes in the way of a clean up and code improvement because we'll have to keep a lot of the component code for the purpose of the "legacy" ops. > > That'll also allow us to do other improvements (I have some > > in mind) that'll apply to all the component users instead of only the > > converted ones. > > What do you have in mind? I didn't want to convert drivers over to the > new way of doing things without making them consciously change their > code. What ordering/behavior would you be changing with the new ops? If the new shutdown ops isn't used, it really shouldn't change anything. Put another way, if we ignore your msm driver changes, we should be able to switch to having a real device for the "master" without making any functional change. If you are causing any functional change with the new ops, maybe you can key it off a flag that needs to be set? That way, we'll have one API/ops but still be backward compatible if you are worried about breaking existing users? > Otherwise I worry it will break things in random, subtle ways. The > last patch, as I mentioned above in the cover, causes warnings because > the display driver is enabling runtime PM in an odd spot as part of the > bind callback of the aggregate/master. That should move out of there and > into the msm_pdev driver that registers the aggregate from what I can > tell. Can you give more context? I think if you create device links with RPM_ACTIVE and PM_RUNTIME flags, it should ensure runtime PM correctness. -Saravana
next prev parent reply other threads:[~2021-05-20 19:30 UTC|newest] Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-05-20 0:25 Stephen Boyd 2021-05-20 0:25 ` [PATCH 1/7] component: Drop 'dev' argument to component_match_realloc() Stephen Boyd 2021-05-20 0:25 ` [PATCH 2/7] component: Rename 'dev' to 'parent' Stephen Boyd 2021-05-20 0:25 ` [PATCH 3/7] component: Introduce struct aggregate_device Stephen Boyd 2021-05-20 20:20 ` Saravana Kannan 2021-05-24 6:01 ` Stephen Boyd 2021-05-20 0:25 ` [PATCH 4/7] component: Introduce the aggregate bus_type Stephen Boyd 2021-05-20 0:25 ` [PATCH 5/7] component: Use dev.parent instead of adev->parent Stephen Boyd 2021-05-20 0:25 ` [PATCH 6/7] component: Move struct aggregate_device out to header file Stephen Boyd 2021-05-20 0:25 ` [PATCH 7/7] drm/msm: Migrate to aggregate driver Stephen Boyd 2021-05-20 19:58 ` Daniel Vetter 2021-05-20 20:22 ` Saravana Kannan 2021-05-24 6:45 ` Stephen Boyd 2021-05-20 1:27 ` [PATCH 0/7] component: Make into an aggregate bus Saravana Kannan 2021-05-20 1:41 ` Stephen Boyd 2021-05-20 19:30 ` Saravana Kannan [this message] 2021-05-20 20:03 ` Daniel Vetter
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=CAGETcx-mRrqC_sGiBk+wx8RtwjJjXf0KJo+ejU6SweEBiATaLw@mail.gmail.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --subject='Re: [PATCH 0/7] component: Make into an aggregate bus' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).