From: Dmitry Osipenko <[email protected]>
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 <[email protected]>
Signed-off-by: Nicolin Chen <[email protected]>
---
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)
+{
+ struct tegra_mc *mc = data;
+
+ put_device(mc->dev);
+}
+
+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);
+ 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
+ *
+ * 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.
+ */
+struct tegra_mc *devm_tegra_get_memory_controller(struct device *dev);
+#else
+static inline struct tegra_mc *devm_tegra_get_memory_controller(struct device *dev)
+{
+ return ERR_PTR(-ENODEV);
+}
+#endif
+
#endif /* __SOC_TEGRA_MC_H__ */
--
2.17.1
"On Wed, 30 Sep 2020 at 10:48, Nicolin Chen <[email protected]> wrote:
>
> From: Dmitry Osipenko <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Nicolin Chen <[email protected]>
> ---
>
> 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
On Wed, Sep 30, 2020 at 11:07:32AM +0200, Krzysztof Kozlowski wrote:
> "On Wed, 30 Sep 2020 at 10:48, Nicolin Chen <[email protected]> wrote:
> >
> > From: Dmitry Osipenko <[email protected]>
> >
> > 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 <[email protected]>
> > Signed-off-by: Nicolin Chen <[email protected]>
> > ---
> >
> > 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!
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 <[email protected]> wrote:
> > >
> > > From: Dmitry Osipenko <[email protected]>
> > >
> > > 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 <[email protected]>
> > > Signed-off-by: Nicolin Chen <[email protected]>
> > > ---
> > >
> > > 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
...
> +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 :)
On Wed, 30 Sep 2020 at 16:41, Dmitry Osipenko <[email protected]> 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
30.09.2020 17:45, Krzysztof Kozlowski пишет:
> On Wed, 30 Sep 2020 at 16:41, Dmitry Osipenko <[email protected]> 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.
On Wed, Sep 30, 2020 at 01:42:56AM -0700, Nicolin Chen wrote:
> From: Dmitry Osipenko <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Nicolin Chen <[email protected]>
> ---
>
> 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
30.09.2020 18:23, Thierry Reding пишет:
> On Wed, Sep 30, 2020 at 01:42:56AM -0700, Nicolin Chen wrote:
>> From: Dmitry Osipenko <[email protected]>
>>
>> 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 <[email protected]>
>> Signed-off-by: Nicolin Chen <[email protected]>
>> ---
>>
>> 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.
30.09.2020 18:23, Thierry Reding пишет:
> On Wed, Sep 30, 2020 at 01:42:56AM -0700, Nicolin Chen wrote:
>> From: Dmitry Osipenko <[email protected]>
>>
>> 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 <[email protected]>
>> Signed-off-by: Nicolin Chen <[email protected]>
>> ---
>>
>> 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.
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 <[email protected]>
> >>
> >> 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 <[email protected]>
> >> Signed-off-by: Nicolin Chen <[email protected]>
> >> ---
> >>
> >> 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
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 <[email protected]>
>>>>
>>>> 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 <[email protected]>
>>>> Signed-off-by: Nicolin Chen <[email protected]>
>>>> ---
>>>>
>>>> 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().
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 <[email protected]>
> >>>>
> >>>> 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 <[email protected]>
> >>>> Signed-off-by: Nicolin Chen <[email protected]>
> >>>> ---
> >>>>
> >>>> 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
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 <[email protected]>
>>>>>>
>>>>>> 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 <[email protected]>
>>>>>> Signed-off-by: Nicolin Chen <[email protected]>
>>>>>> ---
>>>>>>
>>>>>> 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.
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 <[email protected]>
> >>>>>>
> >>>>>> 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 <[email protected]>
> >>>>>> Signed-off-by: Nicolin Chen <[email protected]>
> >>>>>> ---
> >>>>>>
> >>>>>> 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
...
>> 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.