On 11 February 2016 at 17:38, Jon Hunter <jonathanh@xxxxxxxxxx> wrote: > > On 11/02/16 10:28, Ulf Hansson wrote: >> On 11 February 2016 at 11:13, Jon Hunter <jonathanh@xxxxxxxxxx> wrote: >>> >>> On 11/02/16 09:57, Ulf Hansson wrote: >>>> On 11 February 2016 at 10:13, Jon Hunter <jonathanh@xxxxxxxxxx> wrote: >>>>> >>>>> On 10/02/16 18:25, Ulf Hansson wrote: >>>>> >>>>> [snip] >>>>> >>>>>>>> Perhaps there's a way to allow the generic PM domain to control this >>>>>>>> by itself. If we for example used the struct device corresponding to >>>>>>>> the powergate driver, genpd could use it to distinguish between >>>>>>>> various instances of genpd structs..!? Maybe it would simplify the way >>>>>>>> to deal with removing domains? >>>>>>> >>>>>>> Yes, that would be ideal. However, would have require changing >>>>>>> genpd_init()? I am not sure how genpd would be able to access the device >>>>>>> struct for the powergate driver because we don't provide this via any >>>>>>> API I am aware of? And I am guessing that you don't wish to expose the >>>>>>> gpd_list to the world either. >>>>>>> >>>>>>> If there is an easy way, I am open to it, but looking at it today, I am >>>>>>> not sure I see a simple way in which we could add a new API to do this. >>>>>>> However, may be I am missing something! >>>>>> >>>>>> If we add a new __pm_genpd_init() API, that could require a struct >>>>>> device to be provided. That API will thus invoke the existing >>>>>> pm_genpd_init() but also deal with the extra things needed here. >>>>>> >>>>>> I would also allow such an API to return an error code. >>>>>> >>>>>> Correspondingly, pm_genpd_remove() could be required to be provided >>>>>> with a struct device. >>>>>> >>>>>> Existing users of pm_genpd_init() can then convert to >>>>>> __pm_genpd_init() whenever suitable. >>>>>> >>>>>> Of course, another option is just to add new member in the genpd >>>>>> struct for the struct *device. The caller of pm_genpd_init() could >>>>>> check it, but allow it to be NULL. Although, the pm_genpd_remove() API >>>>>> would require that pointer to the struct device to be set... >>>>>> >>>>>> What do you think? >>>>> >>>>> Yes, sounds good. May be it is simpler just to add a new member and let >>>>> the platform genpd driver handle it. >>>>> >>>>> I am wondering if in addition to pm_genpd_remove(), we then just have a >>>>> function called pm_genpd_remove_tail(), which allows you to pass the >>>>> struct device pointer and will remove the last pm-domain from the >>>>> gpd_list and return the genpd pointer if successful. Internally, it will >>>>> call pm_genpd_remove(). It seems to me that if there are nested >>>>> pm-domains, then we probably want to remove them starting from the tail >>>>> as opposed to the head. >>>>> >>>>> How does that sound? >>>> >>>> Why not make pm_genpd_remove() to behave as you describe for >>>> pm_genpd_remove_tail()? >>>> That's probably the only sane way to remove genpds anyhow!? >>> >>> Simply to offer flexibility. I could see that for some devices that have >>> no dependencies between pm-domains and have a static list of pm-domains, >>> they can simply call pm_genpd_remove() for a given pm-domain. However, >>> that said, I can envision a case where a single pm-domain would be >>> removed by itself and so may be there is no benefit? >> >> If I understand correctly, you agree to try with the most simple >> approach first and thus without providing too much flexibility. >> >> Anyway, I am looking forward to review your next version of the patchset! :-) > > So one snag I hit with this, is that in order to remove a pm-domain, I > first need to check if it is a subdomain of any other domains and if so > remove it as a subdomain first. Before, this was simple because I called > pm_genpd_remove_subdomain if the domain had a parent, and then called > pm_genpd_remove(). You certainly have a point here. One more thing, we not only have to check whether the domain is a subdomain. we also need to check if the domain is a parent (master) for other subdomains. It being a parent, prevents us from removing it until all its subdomains are removed. > > With the proposal we have discussed, I no longer have any knowledge of > whether the pm-domain is a subdomain or not because pm_genpd_remove() > would remove the tail and then return it. So this means that I now need > to handle the removal of the subdomain within pm_genpd_remove(), which > is ok, but creates more changes as I need to parse the slave_links list, > etc. > > I am wondering if it would be better to add a simple function called > pm_genpd_list_get_tail(struct device *dev) that would return the last > pm-domain register for a given device and then simply call > pm_genpd_remove_subdomain() (if needed) and pm_genpd_remove()? That should work. Please go ahead and have try! Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
