[v1] mfd: core: Un-constify mfd_cell.of_reg

[PATCH] mfd: core: Un-constify mfd_cell.of_reg

Posted by Michał Mirosław 2 years, 4 months ago

of_reg is the only constant member of struct mfd_cell. It seems to be
accidental and prevents dynamically filling in mfd_cells that use of_reg.
Remove the `const`.

Fixes: 466a62d7642f ("mfd: core: Make a best effort attempt to match devices with the correct of_nodes")
Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---
 include/linux/mfd/core.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/mfd/core.h b/include/linux/mfd/core.h
index 47e7a3a61ce6..e8bcad641d8c 100644
--- a/include/linux/mfd/core.h
+++ b/include/linux/mfd/core.h
@@ -92,7 +92,7 @@ struct mfd_cell {
 	 * (above) when matching OF nodes with devices that have identical
 	 * compatible strings
 	 */
-	const u64 of_reg;
+	u64 of_reg;
 
 	/* Set to 'true' to use 'of_reg' (above) - allows for of_reg=0 */
 	bool use_of_reg;
-- 
2.39.2

Re: [PATCH] mfd: core: Un-constify mfd_cell.of_reg

Posted by Lee Jones 2 years, 4 months ago

On Wed, 16 Aug 2023, Michał Mirosław wrote:

> of_reg is the only constant member of struct mfd_cell. It seems to be

Sorry, what?

struct mfd_cell {
        const char              *name;
[...]
        const struct mfd_cell_acpi_match        *acpi_match;
[...]
        const struct software_node *swnode;
[...]
        const char              *of_compatible;
[...]
        const u64 of_reg;
[...]
        const struct resource   *resources;
[...]
        const char * const      *parent_supplies;
[...]
};

> accidental and prevents dynamically filling in mfd_cells that use of_reg.
> Remove the `const`.

Dynamically filling mfd_cells is generally frowned upon.

Please submit this with the code that will be using it.

> Fixes: 466a62d7642f ("mfd: core: Make a best effort attempt to match devices with the correct of_nodes")
> Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> ---
>  include/linux/mfd/core.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/linux/mfd/core.h b/include/linux/mfd/core.h
> index 47e7a3a61ce6..e8bcad641d8c 100644
> --- a/include/linux/mfd/core.h
> +++ b/include/linux/mfd/core.h
> @@ -92,7 +92,7 @@ struct mfd_cell {
>  	 * (above) when matching OF nodes with devices that have identical
>  	 * compatible strings
>  	 */
> -	const u64 of_reg;
> +	u64 of_reg;
>  
>  	/* Set to 'true' to use 'of_reg' (above) - allows for of_reg=0 */
>  	bool use_of_reg;
> -- 
> 2.39.2
> 

-- 
Lee Jones [李琼斯]

Re: [PATCH] mfd: core: Un-constify mfd_cell.of_reg

Posted by Michał Mirosław 2 years, 4 months ago

On Fri, Aug 18, 2023 at 04:58:15PM +0100, Lee Jones wrote:
> On Wed, 16 Aug 2023, Michał Mirosław wrote:
> 
> > of_reg is the only constant member of struct mfd_cell. It seems to be
> 
> Sorry, what?
> 
> struct mfd_cell {
>         const char              *name;
> [...]
>         const struct mfd_cell_acpi_match        *acpi_match;
> [...]
>         const struct software_node *swnode;
> [...]
>         const char              *of_compatible;
> [...]
>         const u64 of_reg;
> [...]
>         const struct resource   *resources;
> [...]
>         const char * const      *parent_supplies;
> [...]
> };
[...]

All those are pointers to const, not const fields themselves. Only
`of_reg` is const regardless of the whole structure.

-- 
Michał Mirosław

Re: [PATCH] mfd: core: Un-constify mfd_cell.of_reg

Posted by Lee Jones 2 years, 3 months ago

On Sat, 19 Aug 2023, Michał Mirosław wrote:

> On Fri, Aug 18, 2023 at 04:58:15PM +0100, Lee Jones wrote:
> > On Wed, 16 Aug 2023, Michał Mirosław wrote:
> > 
> > > of_reg is the only constant member of struct mfd_cell. It seems to be
> > 
> > Sorry, what?
> > 
> > struct mfd_cell {
> >         const char              *name;
> > [...]
> >         const struct mfd_cell_acpi_match        *acpi_match;
> > [...]
> >         const struct software_node *swnode;
> > [...]
> >         const char              *of_compatible;
> > [...]
> >         const u64 of_reg;
> > [...]
> >         const struct resource   *resources;
> > [...]
> >         const char * const      *parent_supplies;
> > [...]
> > };
> [...]
> 
> All those are pointers to const, not const fields themselves. Only
> `of_reg` is const regardless of the whole structure.

Please make that clear in the commit message.

-- 
Lee Jones [李琼斯]

Re: [PATCH] mfd: core: Un-constify mfd_cell.of_reg

Posted by Michał Mirosław 2 years, 3 months ago

On Mon, Aug 21, 2023 at 09:13:40AM +0100, Lee Jones wrote:
> On Sat, 19 Aug 2023, Michał Mirosław wrote:
> 
> > On Fri, Aug 18, 2023 at 04:58:15PM +0100, Lee Jones wrote:
> > > On Wed, 16 Aug 2023, Michał Mirosław wrote:
> > > 
> > > > of_reg is the only constant member of struct mfd_cell. It seems to be
> > > 
> > > Sorry, what?
> > > 
> > > struct mfd_cell {
> > >         const char              *name;
> > > [...]
> > >         const struct mfd_cell_acpi_match        *acpi_match;
> > > [...]
> > >         const struct software_node *swnode;
> > > [...]
> > >         const char              *of_compatible;
> > > [...]
> > >         const u64 of_reg;
> > > [...]
> > >         const struct resource   *resources;
> > > [...]
> > >         const char * const      *parent_supplies;
> > > [...]
> > > };
> > [...]
> > 
> > All those are pointers to const, not const fields themselves. Only
> > `of_reg` is const regardless of the whole structure.
> 
> Please make that clear in the commit message.

Could you please help with the wording? I'm surprised being asked to explain
a basic C language feature in a commit message to have a patch accepted.
Could it be due to me using 'constant' instead of 'const' that made it
confusing?

Best Regards
Michał Mirosław

Re: [PATCH] mfd: core: Un-constify mfd_cell.of_reg

Posted by Lee Jones 2 years, 3 months ago

On Tue, 22 Aug 2023, Michał Mirosław wrote:

> On Mon, Aug 21, 2023 at 09:13:40AM +0100, Lee Jones wrote:
> > On Sat, 19 Aug 2023, Michał Mirosław wrote:
> > 
> > > On Fri, Aug 18, 2023 at 04:58:15PM +0100, Lee Jones wrote:
> > > > On Wed, 16 Aug 2023, Michał Mirosław wrote:
> > > > 
> > > > > of_reg is the only constant member of struct mfd_cell. It seems to be
> > > > 
> > > > Sorry, what?
> > > > 
> > > > struct mfd_cell {
> > > >         const char              *name;
> > > > [...]
> > > >         const struct mfd_cell_acpi_match        *acpi_match;
> > > > [...]
> > > >         const struct software_node *swnode;
> > > > [...]
> > > >         const char              *of_compatible;
> > > > [...]
> > > >         const u64 of_reg;
> > > > [...]
> > > >         const struct resource   *resources;
> > > > [...]
> > > >         const char * const      *parent_supplies;
> > > > [...]
> > > > };
> > > [...]
> > > 
> > > All those are pointers to const, not const fields themselves. Only
> > > `of_reg` is const regardless of the whole structure.
> > 
> > Please make that clear in the commit message.
> 
> Could you please help with the wording? I'm surprised being asked to explain
> a basic C language feature in a commit message to have a patch accepted.
> Could it be due to me using 'constant' instead of 'const' that made it
> confusing?

I read the commit message to say that there aren't any consts in the
structure, but as you can see, there are plenty.  Just change the
language to be more specific about what you actually mean.  Or just drop
the sentence entirely.

-- 
Lee Jones [李琼斯]