Re: [PATCH v3 1/3] memory: tegra: Add devm_tegra_get_memory_controller()
... >> I don't think that the MC driver will stay built-in forever, although >> its modularization is complicated right now. Hence something shall keep >> the reference to the MC device resources while they are in use and this >> patch takes care of doing that. > > It looks to me like all the other places where we get a reference to the > MC also keep a reference to the device. That's obviously not going to be > enough once the code is turned into a module. At that point we need to > make sure to also grab a reference to the module. But that's orthogonal > to this discussion. > >> Secondly, the Nicolin's patch doesn't pretend on anything, but rather > > Yes, the patch does pretend to "look up" the memory controller device, > but in reality it will always return a singleton object, which can just > as easily be achieved by using a global variable. > >> brings the already existing duplicated code to a single common place. > > Where exactly is that duplicated code? The only places I see where we > get a reference to the memory controller are from the EMC drivers and > they properly look up the MC via the nvidia,memory-controller device > tree property. Yours observation is correct, all the drivers *do the lookup*. My point is that the nvidia,memory-controller usage isn't strictly necessary. The tegra20-devfreq driver doesn't use the phandle lookup because Tegra20 DTs don't have it, instead it uses the compatible lookup. Hence this patch doesn't really change the already existing behaviour for the drivers. The phandle usage is optional. This patch adds a common API that is usable by *all* the already existing drivers, starting from the Tegra20 drivers. > But that's not what this new helper does. This code will use the OF > lookup table to find any match and then returns that, completely > ignoring any links established by the device tree. As I already said in the other reply, it should be fine to add lookup by the phandle and then fall back to the compatible matching. On the other hand, this is not strictly necessary because we always have only a single MC device at a time. Please note that I don't have any objections to improving this patch. In the end either way will work, so we just need to choose the best option. I was merely explaining the rationale behind this patch and not trying to defend it. Yours suggestion about using static mc variable is also good to me since currently MC driver is built-in and at least that won't be a globally-visible kernel variable, which doesn't feel great to have. I think we can follow approach of the static mc variable for the starter and improve it once there will be a real need. This should be the simplest option right now. But again, we may slightly future-proof the API by adding the resource-managed variant. Either way will be good, IMO :) Currently I don't have a strong preference. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 1/3] memory: tegra: Add devm_tegra_get_memory_controller()
On Wed, Sep 30, 2020 at 07:26:00PM +0300, Dmitry Osipenko wrote: > 30.09.2020 19:15, Thierry Reding пишет: > > On Wed, Sep 30, 2020 at 07:06:27PM +0300, Dmitry Osipenko wrote: > >> 30.09.2020 19:03, Thierry Reding пишет: > >>> On Wed, Sep 30, 2020 at 06:53:06PM +0300, Dmitry Osipenko wrote: > 30.09.2020 18:23, Thierry Reding пишет: > > On Wed, Sep 30, 2020 at 01:42:56AM -0700, Nicolin Chen wrote: > >> From: Dmitry Osipenko > >> > >> Multiple Tegra drivers need to retrieve Memory Controller and hence > >> there > >> is quite some duplication of the retrieval code among the drivers. > >> Let's > >> add a new common helper for the retrieval of the MC. > >> > >> Signed-off-by: Dmitry Osipenko > >> Signed-off-by: Nicolin Chen > >> --- > >> > >> Changelog > >> v2->v3: > >> * Replaced with Dimtry's devm_tegra_get_memory_controller() > >> v1->v2: > >> * N/A > >> > >> drivers/memory/tegra/mc.c | 39 +++ > >> include/soc/tegra/mc.h| 17 + > >> 2 files changed, 56 insertions(+) > > > > Let's not add this helper, please. If a device needs a reference to the > > memory controller, it should have a phandle to the memory controller in > > device tree so that it can be looked up explicitly. > > > > Adding this helper is officially sanctioning that it's okay not to have > > that reference and that's a bad idea. > > And please explain why it's a bad idea, I don't see anything bad here at > all. > >>> > >>> Well, you said yourself in a recent comment that we should avoid global > >>> variables. devm_tegra_get_memory_controller() is nothing but a glorified > >>> global variable. > >> > >> This is not a variable, but a common helper function which will remove > >> the duplicated code and will help to avoid common mistakes like a missed > >> put_device(). > > > > Yeah, you're right: this is actually much worse than a global variable. > > It's a helper function that needs 50+ lines in order to effectively > > access a global variable. > > > > You could write this much simpler by doing something like this: > > > > static struct tegra_mc *global_mc; > > > > int tegra_mc_probe(...) > > { > > ... > > > > global_mc = mc; > > > > ... > > } > > > > struct tegra_mc *tegra_get_memory_controller(void) > > { > > return global_mc; > > } > > > > The result is *exactly* the same, except that this is actually more > > honest. Nicolin's patch *pretends* that it isn't using a global variable > > by wrapping a lot of complicated code around it. > > > > But that doesn't change the fact that this accesses a singleton object > > without actually being able to tie it to the device in the first place. > > I don't think that the MC driver will stay built-in forever, although > its modularization is complicated right now. Hence something shall keep > the reference to the MC device resources while they are in use and this > patch takes care of doing that. It looks to me like all the other places where we get a reference to the MC also keep a reference to the device. That's obviously not going to be enough once the code is turned into a module. At that point we need to make sure to also grab a reference to the module. But that's orthogonal to this discussion. > Secondly, the Nicolin's patch doesn't pretend on anything, but rather Yes, the patch does pretend to "look up" the memory controller device, but in reality it will always return a singleton object, which can just as easily be achieved by using a global variable. > brings the already existing duplicated code to a single common place. Where exactly is that duplicated code? The only places I see where we get a reference to the memory controller are from the EMC drivers and they properly look up the MC via the nvidia,memory-controller device tree property. But that's not what this new helper does. This code will use the OF lookup table to find any match and then returns that, completely ignoring any links established by the device tree. Thierry signature.asc Description: PGP signature ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 1/3] memory: tegra: Add devm_tegra_get_memory_controller()
30.09.2020 19:15, Thierry Reding пишет: > On Wed, Sep 30, 2020 at 07:06:27PM +0300, Dmitry Osipenko wrote: >> 30.09.2020 19:03, Thierry Reding пишет: >>> On Wed, Sep 30, 2020 at 06:53:06PM +0300, Dmitry Osipenko wrote: 30.09.2020 18:23, Thierry Reding пишет: > On Wed, Sep 30, 2020 at 01:42:56AM -0700, Nicolin Chen wrote: >> From: Dmitry Osipenko >> >> Multiple Tegra drivers need to retrieve Memory Controller and hence there >> is quite some duplication of the retrieval code among the drivers. Let's >> add a new common helper for the retrieval of the MC. >> >> Signed-off-by: Dmitry Osipenko >> Signed-off-by: Nicolin Chen >> --- >> >> Changelog >> v2->v3: >> * Replaced with Dimtry's devm_tegra_get_memory_controller() >> v1->v2: >> * N/A >> >> drivers/memory/tegra/mc.c | 39 +++ >> include/soc/tegra/mc.h| 17 + >> 2 files changed, 56 insertions(+) > > Let's not add this helper, please. If a device needs a reference to the > memory controller, it should have a phandle to the memory controller in > device tree so that it can be looked up explicitly. > > Adding this helper is officially sanctioning that it's okay not to have > that reference and that's a bad idea. And please explain why it's a bad idea, I don't see anything bad here at all. >>> >>> Well, you said yourself in a recent comment that we should avoid global >>> variables. devm_tegra_get_memory_controller() is nothing but a glorified >>> global variable. >> >> This is not a variable, but a common helper function which will remove >> the duplicated code and will help to avoid common mistakes like a missed >> put_device(). > > Yeah, you're right: this is actually much worse than a global variable. > It's a helper function that needs 50+ lines in order to effectively > access a global variable. > > You could write this much simpler by doing something like this: > > static struct tegra_mc *global_mc; > > int tegra_mc_probe(...) > { > ... > > global_mc = mc; > > ... > } > > struct tegra_mc *tegra_get_memory_controller(void) > { > return global_mc; > } > > The result is *exactly* the same, except that this is actually more > honest. Nicolin's patch *pretends* that it isn't using a global variable > by wrapping a lot of complicated code around it. > > But that doesn't change the fact that this accesses a singleton object > without actually being able to tie it to the device in the first place. I don't think that the MC driver will stay built-in forever, although its modularization is complicated right now. Hence something shall keep the reference to the MC device resources while they are in use and this patch takes care of doing that. Secondly, the Nicolin's patch doesn't pretend on anything, but rather brings the already existing duplicated code to a single common place. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 1/3] memory: tegra: Add devm_tegra_get_memory_controller()
On Wed, Sep 30, 2020 at 07:06:27PM +0300, Dmitry Osipenko wrote: > 30.09.2020 19:03, Thierry Reding пишет: > > On Wed, Sep 30, 2020 at 06:53:06PM +0300, Dmitry Osipenko wrote: > >> 30.09.2020 18:23, Thierry Reding пишет: > >>> On Wed, Sep 30, 2020 at 01:42:56AM -0700, Nicolin Chen wrote: > From: Dmitry Osipenko > > Multiple Tegra drivers need to retrieve Memory Controller and hence there > is quite some duplication of the retrieval code among the drivers. Let's > add a new common helper for the retrieval of the MC. > > Signed-off-by: Dmitry Osipenko > Signed-off-by: Nicolin Chen > --- > > Changelog > v2->v3: > * Replaced with Dimtry's devm_tegra_get_memory_controller() > v1->v2: > * N/A > > drivers/memory/tegra/mc.c | 39 +++ > include/soc/tegra/mc.h| 17 + > 2 files changed, 56 insertions(+) > >>> > >>> Let's not add this helper, please. If a device needs a reference to the > >>> memory controller, it should have a phandle to the memory controller in > >>> device tree so that it can be looked up explicitly. > >>> > >>> Adding this helper is officially sanctioning that it's okay not to have > >>> that reference and that's a bad idea. > >> > >> And please explain why it's a bad idea, I don't see anything bad here at > >> all. > > > > Well, you said yourself in a recent comment that we should avoid global > > variables. devm_tegra_get_memory_controller() is nothing but a glorified > > global variable. > > This is not a variable, but a common helper function which will remove > the duplicated code and will help to avoid common mistakes like a missed > put_device(). Yeah, you're right: this is actually much worse than a global variable. It's a helper function that needs 50+ lines in order to effectively access a global variable. You could write this much simpler by doing something like this: static struct tegra_mc *global_mc; int tegra_mc_probe(...) { ... global_mc = mc; ... } struct tegra_mc *tegra_get_memory_controller(void) { return global_mc; } The result is *exactly* the same, except that this is actually more honest. Nicolin's patch *pretends* that it isn't using a global variable by wrapping a lot of complicated code around it. But that doesn't change the fact that this accesses a singleton object without actually being able to tie it to the device in the first place. Thierry signature.asc Description: PGP signature ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 1/3] memory: tegra: Add devm_tegra_get_memory_controller()
30.09.2020 19:03, Thierry Reding пишет: > On Wed, Sep 30, 2020 at 06:53:06PM +0300, Dmitry Osipenko wrote: >> 30.09.2020 18:23, Thierry Reding пишет: >>> On Wed, Sep 30, 2020 at 01:42:56AM -0700, Nicolin Chen wrote: From: Dmitry Osipenko Multiple Tegra drivers need to retrieve Memory Controller and hence there is quite some duplication of the retrieval code among the drivers. Let's add a new common helper for the retrieval of the MC. Signed-off-by: Dmitry Osipenko Signed-off-by: Nicolin Chen --- Changelog v2->v3: * Replaced with Dimtry's devm_tegra_get_memory_controller() v1->v2: * N/A drivers/memory/tegra/mc.c | 39 +++ include/soc/tegra/mc.h| 17 + 2 files changed, 56 insertions(+) >>> >>> Let's not add this helper, please. If a device needs a reference to the >>> memory controller, it should have a phandle to the memory controller in >>> device tree so that it can be looked up explicitly. >>> >>> Adding this helper is officially sanctioning that it's okay not to have >>> that reference and that's a bad idea. >> >> And please explain why it's a bad idea, I don't see anything bad here at >> all. > > Well, you said yourself in a recent comment that we should avoid global > variables. devm_tegra_get_memory_controller() is nothing but a glorified > global variable. This is not a variable, but a common helper function which will remove the duplicated code and will help to avoid common mistakes like a missed put_device(). ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 1/3] memory: tegra: Add devm_tegra_get_memory_controller()
On Wed, Sep 30, 2020 at 06:53:06PM +0300, Dmitry Osipenko wrote: > 30.09.2020 18:23, Thierry Reding пишет: > > On Wed, Sep 30, 2020 at 01:42:56AM -0700, Nicolin Chen wrote: > >> From: Dmitry Osipenko > >> > >> Multiple Tegra drivers need to retrieve Memory Controller and hence there > >> is quite some duplication of the retrieval code among the drivers. Let's > >> add a new common helper for the retrieval of the MC. > >> > >> Signed-off-by: Dmitry Osipenko > >> Signed-off-by: Nicolin Chen > >> --- > >> > >> Changelog > >> v2->v3: > >> * Replaced with Dimtry's devm_tegra_get_memory_controller() > >> v1->v2: > >> * N/A > >> > >> drivers/memory/tegra/mc.c | 39 +++ > >> include/soc/tegra/mc.h| 17 + > >> 2 files changed, 56 insertions(+) > > > > Let's not add this helper, please. If a device needs a reference to the > > memory controller, it should have a phandle to the memory controller in > > device tree so that it can be looked up explicitly. > > > > Adding this helper is officially sanctioning that it's okay not to have > > that reference and that's a bad idea. > > And please explain why it's a bad idea, I don't see anything bad here at > all. Well, you said yourself in a recent comment that we should avoid global variables. devm_tegra_get_memory_controller() is nothing but a glorified global variable. Thierry signature.asc Description: PGP signature ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 1/3] memory: tegra: Add devm_tegra_get_memory_controller()
30.09.2020 18:23, Thierry Reding пишет: > On Wed, Sep 30, 2020 at 01:42:56AM -0700, Nicolin Chen wrote: >> From: Dmitry Osipenko >> >> Multiple Tegra drivers need to retrieve Memory Controller and hence there >> is quite some duplication of the retrieval code among the drivers. Let's >> add a new common helper for the retrieval of the MC. >> >> Signed-off-by: Dmitry Osipenko >> Signed-off-by: Nicolin Chen >> --- >> >> Changelog >> v2->v3: >> * Replaced with Dimtry's devm_tegra_get_memory_controller() >> v1->v2: >> * N/A >> >> drivers/memory/tegra/mc.c | 39 +++ >> include/soc/tegra/mc.h| 17 + >> 2 files changed, 56 insertions(+) > > Let's not add this helper, please. If a device needs a reference to the > memory controller, it should have a phandle to the memory controller in > device tree so that it can be looked up explicitly. > > Adding this helper is officially sanctioning that it's okay not to have > that reference and that's a bad idea. And please explain why it's a bad idea, I don't see anything bad here at all. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 1/3] memory: tegra: Add devm_tegra_get_memory_controller()
30.09.2020 18:23, Thierry Reding пишет: > On Wed, Sep 30, 2020 at 01:42:56AM -0700, Nicolin Chen wrote: >> From: Dmitry Osipenko >> >> Multiple Tegra drivers need to retrieve Memory Controller and hence there >> is quite some duplication of the retrieval code among the drivers. Let's >> add a new common helper for the retrieval of the MC. >> >> Signed-off-by: Dmitry Osipenko >> Signed-off-by: Nicolin Chen >> --- >> >> Changelog >> v2->v3: >> * Replaced with Dimtry's devm_tegra_get_memory_controller() >> v1->v2: >> * N/A >> >> drivers/memory/tegra/mc.c | 39 +++ >> include/soc/tegra/mc.h| 17 + >> 2 files changed, 56 insertions(+) > > Let's not add this helper, please. If a device needs a reference to the > memory controller, it should have a phandle to the memory controller in > device tree so that it can be looked up explicitly. > > Adding this helper is officially sanctioning that it's okay not to have > that reference and that's a bad idea. Maybe that's because the reference isn't really needed for the lookup? :) Secondly, we could use the reference and then fall back to the of-matching for devices that don't have the reference, like all Tegra20 devices + Tegra30/124 ACTMON devices. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 1/3] memory: tegra: Add devm_tegra_get_memory_controller()
On Wed, Sep 30, 2020 at 01:42:56AM -0700, Nicolin Chen wrote: > From: Dmitry Osipenko > > Multiple Tegra drivers need to retrieve Memory Controller and hence there > is quite some duplication of the retrieval code among the drivers. Let's > add a new common helper for the retrieval of the MC. > > Signed-off-by: Dmitry Osipenko > Signed-off-by: Nicolin Chen > --- > > Changelog > v2->v3: > * Replaced with Dimtry's devm_tegra_get_memory_controller() > v1->v2: > * N/A > > drivers/memory/tegra/mc.c | 39 +++ > include/soc/tegra/mc.h| 17 + > 2 files changed, 56 insertions(+) Let's not add this helper, please. If a device needs a reference to the memory controller, it should have a phandle to the memory controller in device tree so that it can be looked up explicitly. Adding this helper is officially sanctioning that it's okay not to have that reference and that's a bad idea. Thierry signature.asc Description: PGP signature ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 1/3] memory: tegra: Add devm_tegra_get_memory_controller()
30.09.2020 17:45, Krzysztof Kozlowski пишет: > On Wed, 30 Sep 2020 at 16:41, Dmitry Osipenko wrote: >> >> ... >>> +struct tegra_mc *devm_tegra_get_memory_controller(struct device *dev) >>> +{ >>> + struct platform_device *pdev; >>> + struct device_node *np; >>> + struct tegra_mc *mc; >>> + int err; >>> + >>> + np = of_find_matching_node_and_match(NULL, tegra_mc_of_match, NULL); >>> + if (!np) >>> + return ERR_PTR(-ENOENT); >>> + >>> + pdev = of_find_device_by_node(np); >>> + of_node_put(np); >>> + if (!pdev) >>> + return ERR_PTR(-ENODEV); >>> + >>> + mc = platform_get_drvdata(pdev); >>> + if (!mc) { >>> + put_device(mc->dev); >> >> This should be put_device(&pdev->dev). Please always be careful while >> copying someones else code :) > > Good catch. I guess devm_add_action_or_reset() would also work... or > running Smatch on new code. Smatch should point it out. The devm_add_action_or_reset() shouldn't help much in this particular case because it's too early for the devm_add_action here. Having an explicit put_device() in all error code paths also helps with improving readability of the code a tad, IMO. Smatch indeed should catch this bug, but Smatch usually isn't a part of the developers workflow. More often Smatch is a part of maintainers workflow, hence such problems usually are getting caught before patches are applied. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 1/3] memory: tegra: Add devm_tegra_get_memory_controller()
On Wed, 30 Sep 2020 at 16:41, Dmitry Osipenko wrote: > > ... > > +struct tegra_mc *devm_tegra_get_memory_controller(struct device *dev) > > +{ > > + struct platform_device *pdev; > > + struct device_node *np; > > + struct tegra_mc *mc; > > + int err; > > + > > + np = of_find_matching_node_and_match(NULL, tegra_mc_of_match, NULL); > > + if (!np) > > + return ERR_PTR(-ENOENT); > > + > > + pdev = of_find_device_by_node(np); > > + of_node_put(np); > > + if (!pdev) > > + return ERR_PTR(-ENODEV); > > + > > + mc = platform_get_drvdata(pdev); > > + if (!mc) { > > + put_device(mc->dev); > > This should be put_device(&pdev->dev). Please always be careful while > copying someones else code :) Good catch. I guess devm_add_action_or_reset() would also work... or running Smatch on new code. Smatch should point it out. Best regards, Krzysztof ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 1/3] memory: tegra: Add devm_tegra_get_memory_controller()
... > +struct tegra_mc *devm_tegra_get_memory_controller(struct device *dev) > +{ > + struct platform_device *pdev; > + struct device_node *np; > + struct tegra_mc *mc; > + int err; > + > + np = of_find_matching_node_and_match(NULL, tegra_mc_of_match, NULL); > + if (!np) > + return ERR_PTR(-ENOENT); > + > + pdev = of_find_device_by_node(np); > + of_node_put(np); > + if (!pdev) > + return ERR_PTR(-ENODEV); > + > + mc = platform_get_drvdata(pdev); > + if (!mc) { > + put_device(mc->dev); This should be put_device(&pdev->dev). Please always be careful while copying someones else code :) ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 1/3] memory: tegra: Add devm_tegra_get_memory_controller()
On Wed, Sep 30, 2020 at 02:41:45AM -0700, Nicolin Chen wrote: > On Wed, Sep 30, 2020 at 11:07:32AM +0200, Krzysztof Kozlowski wrote: > > "On Wed, 30 Sep 2020 at 10:48, Nicolin Chen wrote: > > > > > > From: Dmitry Osipenko > > > > > > Multiple Tegra drivers need to retrieve Memory Controller and hence there > > > is quite some duplication of the retrieval code among the drivers. Let's > > > add a new common helper for the retrieval of the MC. > > > > > > Signed-off-by: Dmitry Osipenko > > > Signed-off-by: Nicolin Chen > > > --- > > > > > > Changelog > > > v2->v3: > > > * Replaced with Dimtry's devm_tegra_get_memory_controller() > > > v1->v2: > > > * N/A > > > > > > drivers/memory/tegra/mc.c | 39 +++ > > > include/soc/tegra/mc.h| 17 + > > > 2 files changed, 56 insertions(+) > > > > > > diff --git a/drivers/memory/tegra/mc.c b/drivers/memory/tegra/mc.c > > > index ec8403557ed4..dd691dc3738e 100644 > > > --- a/drivers/memory/tegra/mc.c > > > +++ b/drivers/memory/tegra/mc.c > > > @@ -42,6 +42,45 @@ static const struct of_device_id tegra_mc_of_match[] = > > > { > > > }; > > > MODULE_DEVICE_TABLE(of, tegra_mc_of_match); > > > > > > +static void tegra_mc_devm_action_put_device(void *data) > > > > devm_tegra_memory_controller_put() My bad here, this is not a "put" helper so the previous name was actually good. No need to change. > > > > > +{ > > > + struct tegra_mc *mc = data; > > > + > > > + put_device(mc->dev); > > > +} > > > + > > > +struct tegra_mc *devm_tegra_get_memory_controller(struct device *dev) > > > > Usually 'get' is a suffix (e.g. clk, gpiod, iio, led), so: > > devm_tegra_memory_controller_get() > > > > > +{ > > > + struct platform_device *pdev; > > > + struct device_node *np; > > > + struct tegra_mc *mc; > > > + int err; > > > + > > > + np = of_find_matching_node_and_match(NULL, tegra_mc_of_match, > > > NULL); > > > + if (!np) > > > + return ERR_PTR(-ENOENT); > > > + > > > + pdev = of_find_device_by_node(np); > > > + of_node_put(np); > > > + if (!pdev) > > > + return ERR_PTR(-ENODEV); > > > + > > > + mc = platform_get_drvdata(pdev); > > > + if (!mc) { > > > + put_device(mc->dev); > > > + return ERR_PTR(-EPROBE_DEFER); > > > + } > > > + > > > + err = devm_add_action(dev, tegra_mc_devm_action_put_device, mc); This can be simpler with devm_add_action_or_reset. Best regards, Krzysztof ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 1/3] memory: tegra: Add devm_tegra_get_memory_controller()
On Wed, Sep 30, 2020 at 11:07:32AM +0200, Krzysztof Kozlowski wrote: > "On Wed, 30 Sep 2020 at 10:48, Nicolin Chen wrote: > > > > From: Dmitry Osipenko > > > > Multiple Tegra drivers need to retrieve Memory Controller and hence there > > is quite some duplication of the retrieval code among the drivers. Let's > > add a new common helper for the retrieval of the MC. > > > > Signed-off-by: Dmitry Osipenko > > Signed-off-by: Nicolin Chen > > --- > > > > Changelog > > v2->v3: > > * Replaced with Dimtry's devm_tegra_get_memory_controller() > > v1->v2: > > * N/A > > > > drivers/memory/tegra/mc.c | 39 +++ > > include/soc/tegra/mc.h| 17 + > > 2 files changed, 56 insertions(+) > > > > diff --git a/drivers/memory/tegra/mc.c b/drivers/memory/tegra/mc.c > > index ec8403557ed4..dd691dc3738e 100644 > > --- a/drivers/memory/tegra/mc.c > > +++ b/drivers/memory/tegra/mc.c > > @@ -42,6 +42,45 @@ static const struct of_device_id tegra_mc_of_match[] = { > > }; > > MODULE_DEVICE_TABLE(of, tegra_mc_of_match); > > > > +static void tegra_mc_devm_action_put_device(void *data) > > devm_tegra_memory_controller_put() > > > +{ > > + struct tegra_mc *mc = data; > > + > > + put_device(mc->dev); > > +} > > + > > +struct tegra_mc *devm_tegra_get_memory_controller(struct device *dev) > > Usually 'get' is a suffix (e.g. clk, gpiod, iio, led), so: > devm_tegra_memory_controller_get() > > > +{ > > + struct platform_device *pdev; > > + struct device_node *np; > > + struct tegra_mc *mc; > > + int err; > > + > > + np = of_find_matching_node_and_match(NULL, tegra_mc_of_match, NULL); > > + if (!np) > > + return ERR_PTR(-ENOENT); > > + > > + pdev = of_find_device_by_node(np); > > + of_node_put(np); > > + if (!pdev) > > + return ERR_PTR(-ENODEV); > > + > > + mc = platform_get_drvdata(pdev); > > + if (!mc) { > > + put_device(mc->dev); > > + return ERR_PTR(-EPROBE_DEFER); > > + } > > + > > + err = devm_add_action(dev, tegra_mc_devm_action_put_device, mc); > > + if (err) { > > + put_device(mc->dev); > > + return ERR_PTR(err); > > + } > > + > > + return mc; > > +} > > +EXPORT_SYMBOL_GPL(devm_tegra_get_memory_controller); > > + > > static int tegra_mc_block_dma_common(struct tegra_mc *mc, > > const struct tegra_mc_reset *rst) > > { > > diff --git a/include/soc/tegra/mc.h b/include/soc/tegra/mc.h > > index 1238e35653d1..c05142e3e244 100644 > > --- a/include/soc/tegra/mc.h > > +++ b/include/soc/tegra/mc.h > > @@ -184,4 +184,21 @@ struct tegra_mc { > > int tegra_mc_write_emem_configuration(struct tegra_mc *mc, unsigned long > > rate); > > unsigned int tegra_mc_get_emem_device_count(struct tegra_mc *mc); > > > > +#ifdef CONFIG_TEGRA_MC > > +/** > > + * devm_tegra_get_memory_controller() - Get the tegra_mc pointer. > > + * @dev: Device that will be interacted with > > This is not precise enough and there is no interaction with 'dev' in > devm_tegra_get_memory_controller(). Something like: "Device that owns > the pointer to tegra memory controller" > > > + * > > + * Return: ERR_PTR() on error or a valid pointer to a struct tegra_mc. > > + * > > + * The mc->dev counter will be automatically put by the device management > > code. > > 1. s/mc/tegra_mc/ (it's the first occurence of word mc here) > 2. "kerneldoc goes to the C file". Not to the header. I will send v4 after changing all of the places. Thanks for the comments! ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 1/3] memory: tegra: Add devm_tegra_get_memory_controller()
"On Wed, 30 Sep 2020 at 10:48, Nicolin Chen wrote: > > From: Dmitry Osipenko > > Multiple Tegra drivers need to retrieve Memory Controller and hence there > is quite some duplication of the retrieval code among the drivers. Let's > add a new common helper for the retrieval of the MC. > > Signed-off-by: Dmitry Osipenko > Signed-off-by: Nicolin Chen > --- > > Changelog > v2->v3: > * Replaced with Dimtry's devm_tegra_get_memory_controller() > v1->v2: > * N/A > > drivers/memory/tegra/mc.c | 39 +++ > include/soc/tegra/mc.h| 17 + > 2 files changed, 56 insertions(+) > > diff --git a/drivers/memory/tegra/mc.c b/drivers/memory/tegra/mc.c > index ec8403557ed4..dd691dc3738e 100644 > --- a/drivers/memory/tegra/mc.c > +++ b/drivers/memory/tegra/mc.c > @@ -42,6 +42,45 @@ static const struct of_device_id tegra_mc_of_match[] = { > }; > MODULE_DEVICE_TABLE(of, tegra_mc_of_match); > > +static void tegra_mc_devm_action_put_device(void *data) devm_tegra_memory_controller_put() > +{ > + struct tegra_mc *mc = data; > + > + put_device(mc->dev); > +} > + > +struct tegra_mc *devm_tegra_get_memory_controller(struct device *dev) Usually 'get' is a suffix (e.g. clk, gpiod, iio, led), so: devm_tegra_memory_controller_get() > +{ > + struct platform_device *pdev; > + struct device_node *np; > + struct tegra_mc *mc; > + int err; > + > + np = of_find_matching_node_and_match(NULL, tegra_mc_of_match, NULL); > + if (!np) > + return ERR_PTR(-ENOENT); > + > + pdev = of_find_device_by_node(np); > + of_node_put(np); > + if (!pdev) > + return ERR_PTR(-ENODEV); > + > + mc = platform_get_drvdata(pdev); > + if (!mc) { > + put_device(mc->dev); > + return ERR_PTR(-EPROBE_DEFER); > + } > + > + err = devm_add_action(dev, tegra_mc_devm_action_put_device, mc); > + if (err) { > + put_device(mc->dev); > + return ERR_PTR(err); > + } > + > + return mc; > +} > +EXPORT_SYMBOL_GPL(devm_tegra_get_memory_controller); > + > static int tegra_mc_block_dma_common(struct tegra_mc *mc, > const struct tegra_mc_reset *rst) > { > diff --git a/include/soc/tegra/mc.h b/include/soc/tegra/mc.h > index 1238e35653d1..c05142e3e244 100644 > --- a/include/soc/tegra/mc.h > +++ b/include/soc/tegra/mc.h > @@ -184,4 +184,21 @@ struct tegra_mc { > int tegra_mc_write_emem_configuration(struct tegra_mc *mc, unsigned long > rate); > unsigned int tegra_mc_get_emem_device_count(struct tegra_mc *mc); > > +#ifdef CONFIG_TEGRA_MC > +/** > + * devm_tegra_get_memory_controller() - Get the tegra_mc pointer. > + * @dev: Device that will be interacted with This is not precise enough and there is no interaction with 'dev' in devm_tegra_get_memory_controller(). Something like: "Device that owns the pointer to tegra memory controller" > + * > + * Return: ERR_PTR() on error or a valid pointer to a struct tegra_mc. > + * > + * The mc->dev counter will be automatically put by the device management > code. 1. s/mc/tegra_mc/ (it's the first occurence of word mc here) 2. "kerneldoc goes to the C file". Not to the header. Best regards, Krzysztof ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu