Re: [PATCH v3] ASoC: Remove 'const' from the device_node pointers

Re: [PATCH v3] ASoC: Remove 'const' from the device_node pointers

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 819 bytes --]

On Tue, Nov 25, 2014 at 12:14:48PM +0100, Jean-Francois Moine wrote:
> As Russell King's explained it, there should not be pointers to
> struct device_node:
> 
> "struct device_node is a ref-counted structure.  That means if you
>  store a reference to it, you should "get" it, and you should "put"
>  it once you've done.  The act of "put"ing the pointed-to structure
>  involves writing to that structure, so it is totally unappropriate
>  to store a device_node structure as a const pointer.  It forces you
>  to have to cast it back to a non-const pointer at various points
>  in time to use various OF function calls."

So, we're not holding references here (we're just doing comparisons, the
references need to be owned before we get into the core) and I'm not
seeing anything here removing casts?

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH v3] ASoC: Remove 'const' from the device_node pointers

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 819 bytes --]

On Tue, Nov 25, 2014 at 12:14:48PM +0100, Jean-Francois Moine wrote:
> As Russell King's explained it, there should not be pointers to
> struct device_node:
> 
> "struct device_node is a ref-counted structure.  That means if you
>  store a reference to it, you should "get" it, and you should "put"
>  it once you've done.  The act of "put"ing the pointed-to structure
>  involves writing to that structure, so it is totally unappropriate
>  to store a device_node structure as a const pointer.  It forces you
>  to have to cast it back to a non-const pointer at various points
>  in time to use various OF function calls."

So, we're not holding references here (we're just doing comparisons, the
references need to be owned before we get into the core) and I'm not
seeing anything here removing casts?

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH v3] ASoC: Remove 'const' from the device_node pointers

[Lars-Peter Clausen]

On 11/25/2014 02:13 PM, Mark Brown wrote:
> On Tue, Nov 25, 2014 at 12:14:48PM +0100, Jean-Francois Moine wrote:
>> As Russell King's explained it, there should not be pointers to
>> struct device_node:
>>
>> "struct device_node is a ref-counted structure.  That means if you
>>   store a reference to it, you should "get" it, and you should "put"
>>   it once you've done.  The act of "put"ing the pointed-to structure
>>   involves writing to that structure, so it is totally unappropriate
>>   to store a device_node structure as a const pointer.  It forces you
>>   to have to cast it back to a non-const pointer at various points
>>   in time to use various OF function calls."
>
> So, we're not holding references here (we're just doing comparisons, the
> references need to be owned before we get into the core)

The core itself will only do the comparisons and it is the board drivers 
responsibility to get and put the references. Making the pointers non const 
allows the board driver to use them to put the reference once the card has 
been unregistered rather than having to keep a separate set of pointers 
around. This should probably be mentioned in the commit message though.

> I'm not seeing anything here removing casts?

This patch used to be part of a two part series where the second patch 
removed the casts. This patch as already been applied though.

Re: [PATCH v3] ASoC: Remove 'const' from the device_node pointers

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1250 bytes --]

On Tue, Nov 25, 2014 at 02:36:15PM +0100, Lars-Peter Clausen wrote:
> On 11/25/2014 02:13 PM, Mark Brown wrote:

> >So, we're not holding references here (we're just doing comparisons, the
> >references need to be owned before we get into the core)

> The core itself will only do the comparisons and it is the board drivers
> responsibility to get and put the references. Making the pointers non const
> allows the board driver to use them to put the reference once the card has
> been unregistered rather than having to keep a separate set of pointers

Wouldn't it be even better to have managed OF references and not need to
explicitly dereference at all?  Otherwise every time something uses
managed resources for the card or component we've got a (marginal
admittedly) reference management bug.

> around. This should probably be mentioned in the commit message though.

Yes, it really should - this is really what I'm getting at here since
I'm frequently having to push back on difficult to understand changes
here.

> >I'm not seeing anything here removing casts?

> This patch used to be part of a two part series where the second patch
> removed the casts. This patch as already been applied though.

We don't have warnings at the minute...

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH v3] ASoC: Remove 'const' from the device_node pointers

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 354 bytes --]

On Tue, Nov 25, 2014 at 12:14:48PM +0100, Jean-Francois Moine wrote:
> As Russell King's explained it, there should not be pointers to
> struct device_node:

OK, found a user that needs this so applying now.  Please do try to make
your changelogs clearer about why things are being done, it's often hard
to figure things out which slows everything down.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[PATCH v3] ASoC: Remove 'const' from the device_node pointers

[Jean-Francois Moine]

As Russell King's explained it, there should not be pointers to
struct device_node:

"struct device_node is a ref-counted structure.  That means if you
 store a reference to it, you should "get" it, and you should "put"
 it once you've done.  The act of "put"ing the pointed-to structure
 involves writing to that structure, so it is totally unappropriate
 to store a device_node structure as a const pointer.  It forces you
 to have to cast it back to a non-const pointer at various points
 in time to use various OF function calls."

Signed-off-by: Jean-Francois Moine <moinejf@free.fr>
---
v3: change the patch comment (Mark Brown)
v2: remove const from the pointers instead of modifying the
    OF functions (Russell King)
---
 include/sound/soc.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/sound/soc.h b/include/sound/soc.h
index 879e2b3..13e1648 100644
--- a/include/sound/soc.h
+++ b/include/sound/soc.h
@@ -883,7 +883,7 @@ struct snd_soc_platform_driver {
 
 struct snd_soc_dai_link_component {
 	const char *name;
-	const struct device_node *of_node;
+	struct device_node *of_node;
 	const char *dai_name;
 };
 
@@ -985,7 +985,7 @@ struct snd_soc_codec_conf {
 	 * DT/OF node, but not both.
 	 */
 	const char *dev_name;
-	const struct device_node *of_node;
+	struct device_node *of_node;
 
 	/*
 	 * optional map of kcontrol, widget and path name prefixes that are
@@ -1002,7 +1002,7 @@ struct snd_soc_aux_dev {
 	 * DT/OF node, but not both.
 	 */
 	const char *codec_name;
-	const struct device_node *codec_of_node;
+	struct device_node *codec_of_node;
 
 	/* codec/machine specific init - e.g. add machine controls */
 	int (*init)(struct snd_soc_component *component);
-- 
2.1.3