[PATCH 00/27] Fix common bug in most nand drivers not showing a device in sysfs

[PATCH 00/27] Fix common bug in most nand drivers not showing a device in sysfs

[Alexander Holler]

A comment in mtdcore.c function add_mtd_device() which is called by
mtd_device_parse_register() made me wonder:

"Caller should have set dev.parent to match the physical device."

In fact this is not done by most nand drivers.

What follows is a series which fixes this.

Tested: orion and omap2
Compile-Tested: atmel, gpio, fsmc, gpmi, plat, pxa3xx, s3c2410, sh_flctl,
		sharpsl, tmio, docg4, davinci, lpc32xx_mlc, lpc32xx_slc, mxc
Not tested at all (only be view, patches 19-27): bcm47, fsl_elbc, fsl_upm,
		fsl_ifc, jz4740, mpc5121m, ndfc, txx9ndfmx, socrates

The overall stat is

27 files changed, 36 insertions(+), 80 deletions(-)

and it fixes 21 of these bugs.

Regards,

Alexander Holler

[PATCH 01/27] mtd: nand: introduce function to fix a common bug in most nand-drivers not showing a device in sysfs

[Alexander Holler]

Most nand drivers don't set a parent device for the mtd-device. The result
is that information in sysfs is missing (no folder device).

Comparing the output of

	git grep mtd_device_parse_register drivers/mtd/nand/

with

	git grep parent drivers/mtd/nand/ | cut -f 1 | sort -u

showed that this is a very common error. Looking at some of those drivers
a common pattern is visible which I put into a new inline function to
reduce source code size and to avoid future similiar errors.

Signed-off-by: Alexander Holler <holler@ahsoftware.de>
---
 include/linux/mtd/mtd.h | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h
index a1b0b4c..2e24afe 100644
--- a/include/linux/mtd/mtd.h
+++ b/include/linux/mtd/mtd.h
@@ -23,7 +23,7 @@
 #include <linux/types.h>
 #include <linux/uio.h>
 #include <linux/notifier.h>
-#include <linux/device.h>
+#include <linux/platform_device.h>
 
 #include <mtd/mtd-abi.h>
 
@@ -366,6 +366,15 @@ static inline int mtd_can_have_bb(const struct mtd_info *mtd)
 struct mtd_partition;
 struct mtd_part_parser_data;
 
+static inline void mtd_setup_common_members(struct mtd_info *mtd, void *priv,
+						struct platform_device *pdev)
+{
+	mtd->priv	= priv;
+	mtd->owner	= pdev->dev.driver->owner;
+	mtd->dev.parent	= &pdev->dev;
+	mtd->name	= pdev->dev.driver->name;
+}
+
 extern int mtd_device_parse_register(struct mtd_info *mtd,
 				     const char * const *part_probe_types,
 				     struct mtd_part_parser_data *parser_data,
-- 
1.8.3.2

Re: [PATCH 01/27] mtd: nand: introduce function to fix a common bug in most nand-drivers not showing a device in sysfs

[Brian Norris]

On Tue, May 27, 2014 at 12:12:26AM +0200, Alexander Holler wrote:
> --- a/include/linux/mtd/mtd.h
> +++ b/include/linux/mtd/mtd.h
> @@ -23,7 +23,7 @@
>  #include <linux/types.h>
>  #include <linux/uio.h>
>  #include <linux/notifier.h>
> -#include <linux/device.h>
> +#include <linux/platform_device.h>
>  
>  #include <mtd/mtd-abi.h>
>  
> @@ -366,6 +366,15 @@ static inline int mtd_can_have_bb(const struct mtd_info *mtd)
>  struct mtd_partition;
>  struct mtd_part_parser_data;
>  
> +static inline void mtd_setup_common_members(struct mtd_info *mtd, void *priv,
> +						struct platform_device *pdev)

Thanks for the diligence on catching these issues, but I'm not sure this
helper function is fully the correct approach here.

> +{
> +	mtd->priv	= priv;

I don't think you should hide this one here. It will be quite obvious if
a driver didn't stash its private data but tries to access it later. Are
there any drivers that missed this?

> +	mtd->owner	= pdev->dev.driver->owner;
> +	mtd->dev.parent	= &pdev->dev;
> +	mtd->name	= pdev->dev.driver->name;

I think this is a little dangerous. You're potentially clobbering the
name that a driver already chose here. And why did you pick to use the
driver name? This gives non-unique names if there is more than one
device instantiated for a driver. That's why some drivers already use
the device name, not the driver name:

	mtd->name = dev_name(&pev->dev);

And in fact, if any drivers are missing mtd->name, perhaps it's best to
just modify the MTD registration to give them a default:

	if (!mtd->name)
		mtd->name = dev_name(&pdev->dev);

> +}

BTW, nothing in this function actually makes sense to require a
platform_device, does it? And it's possible to have non-platform drivers
that want to do basic MTD initialization. So (if we still keep this
helper function at all), I'd recommend just a 'struct device *dev'
parameter.

> +
>  extern int mtd_device_parse_register(struct mtd_info *mtd,
>  				     const char * const *part_probe_types,
>  				     struct mtd_part_parser_data *parser_data,

How about we rethink the "helper" approach, and instead just do
validation in the core code? This would cover most of the important
parts of your helper, I think:

diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
index d201feeb3ca6..39ba5812a5a3 100644
--- a/drivers/mtd/mtdcore.c
+++ b/drivers/mtd/mtdcore.c
@@ -397,6 +397,11 @@ int add_mtd_device(struct mtd_info *mtd)
 	if (device_register(&mtd->dev) != 0)
 		goto fail_added;
 
+	if (mtd->dev.parent)
+		mtd->owner = mtd->dev.parent->driver->owner;
+	else
+		WARN_ON(1);
+
 	if (MTD_DEVT(i))
 		device_create(&mtd_class, mtd->dev.parent,
 			      MTD_DEVT(i) + 1,
diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
index 1ca9aec141ff..9869bbef50cf 100644
--- a/drivers/mtd/mtdpart.c
+++ b/drivers/mtd/mtdpart.c
@@ -370,7 +370,6 @@ static struct mtd_part *allocate_partition(struct mtd_info *master,
 	slave->mtd.subpage_sft = master->subpage_sft;
 
 	slave->mtd.name = name;
-	slave->mtd.owner = master->owner;
 	slave->mtd.backing_dev_info = master->backing_dev_info;
 
 	/* NOTE:  we don't arrange MTDs as a tree; it'd be error-prone

--
Brian

Re: [PATCH 01/27] mtd: nand: introduce function to fix a common bug in most nand-drivers not showing a device in sysfs

[Alexander Holler]

Am 28.05.2014 10:43, schrieb Brian Norris:
> On Tue, May 27, 2014 at 12:12:26AM +0200, Alexander Holler wrote:
>> --- a/include/linux/mtd/mtd.h
>> +++ b/include/linux/mtd/mtd.h
>> @@ -23,7 +23,7 @@
>>   #include <linux/types.h>
>>   #include <linux/uio.h>
>>   #include <linux/notifier.h>
>> -#include <linux/device.h>
>> +#include <linux/platform_device.h>
>>
>>   #include <mtd/mtd-abi.h>
>>
>> @@ -366,6 +366,15 @@ static inline int mtd_can_have_bb(const struct mtd_info *mtd)
>>   struct mtd_partition;
>>   struct mtd_part_parser_data;
>>
>> +static inline void mtd_setup_common_members(struct mtd_info *mtd, void *priv,
>> +						struct platform_device *pdev)
>
> Thanks for the diligence on catching these issues, but I'm not sure this
> helper function is fully the correct approach here.
>
>> +{
>> +	mtd->priv	= priv;
>
> I don't think you should hide this one here. It will be quite obvious if
> a driver didn't stash its private data but tries to access it later. Are
> there any drivers that missed this?

No, it just saves a line of source in all drivers and I think it fits 
there. I don't understand why do you think it is hidden.

>
>> +	mtd->owner	= pdev->dev.driver->owner;
>> +	mtd->dev.parent	= &pdev->dev;
>> +	mtd->name	= pdev->dev.driver->name;
>
> I think this is a little dangerous. You're potentially clobbering the
> name that a driver already chose here. And why did you pick to use the
> driver name? This gives non-unique names if there is more than one
> device instantiated for a driver. That's why some drivers already use
> the device name, not the driver name:
>
> 	mtd->name = dev_name(&pev->dev);
>
> And in fact, if any drivers are missing mtd->name, perhaps it's best to
> just modify the MTD registration to give them a default:
>
> 	if (!mtd->name)
> 		mtd->name = dev_name(&pdev->dev);
>
>> +}

I don't clobber any name. The name is set as default before drivers 
might do this. And the common pattern I've seen wasn't dev_name(foo) but 
the drivers name. And those drivers which do use dev_name(), still do so 
by overwriting the default I put into that function. But feel free to 
change this. I will not go again and again through the 26 drivers until 
all maintainers and other people are happy.

>
> BTW, nothing in this function actually makes sense to require a
> platform_device, does it? And it's possible to have non-platform drivers
> that want to do basic MTD initialization. So (if we still keep this
> helper function at all), I'd recommend just a 'struct device *dev'
> parameter.
>

Feel free to chgange it.


>> +
>>   extern int mtd_device_parse_register(struct mtd_info *mtd,
>>   				     const char * const *part_probe_types,
>>   				     struct mtd_part_parser_data *parser_data,
>
> How about we rethink the "helper" approach, and instead just do
> validation in the core code? This would cover most of the important
> parts of your helper, I think:

Feel free to change all drivers. I like my approach with fixing 21 bugs 
by reducing code by 44 lines.

And it offers a common function where future similiarities can be put 
into too. Of course you can just add 21 lines, but that is not how I do 
such stuff.

And I did the patches. If you don't like them, feel free to ignore them. 
I'm not playing remote keyboard but I do patches like I would do them, 
not like some arbitrary maintainer would do them. Sorry for the harsh words.

Regards,

Alexander Holler

Re: [PATCH 01/27] mtd: nand: introduce function to fix a common bug in most nand-drivers not showing a device in sysfs

[Brian Norris]

On Wed, May 28, 2014 at 08:52:06PM +0200, Alexander Holler wrote:
> Am 28.05.2014 10:43, schrieb Brian Norris:
> >On Tue, May 27, 2014 at 12:12:26AM +0200, Alexander Holler wrote:
> >>+{
> >>+	mtd->priv	= priv;
> >
> >I don't think you should hide this one here. It will be quite obvious if
> >a driver didn't stash its private data but tries to access it later. Are
> >there any drivers that missed this?
> 
> No, it just saves a line of source in all drivers and I think it
> fits there. I don't understand why do you think it is hidden.

The function name doesn't make it obvious what it's doing. So all code
readers will have to follow the definition to see what it's doing. But
this point is not that important, so I won't argue.

> >>+	mtd->owner	= pdev->dev.driver->owner;
> >>+	mtd->dev.parent	= &pdev->dev;
> >>+	mtd->name	= pdev->dev.driver->name;
> >
> >I think this is a little dangerous. You're potentially clobbering the
> >name that a driver already chose here. And why did you pick to use the
> >driver name? This gives non-unique names if there is more than one
> >device instantiated for a driver. That's why some drivers already use
> >the device name, not the driver name:
> >
> >	mtd->name = dev_name(&pev->dev);
> >
> >And in fact, if any drivers are missing mtd->name, perhaps it's best to
> >just modify the MTD registration to give them a default:
> >
> >	if (!mtd->name)
> >		mtd->name = dev_name(&pdev->dev);
> >
> >>+}
> 
> I don't clobber any name. The name is set as default before drivers
> might do this.

At the moment this is true, but the ordering is somewhat subtle if you
don't examine the details of mtd_setup_common_members() to see that it
is assigning a name. I can easily imagine some new driver in which
someone does:

	mtd->name = ...;
	[...]
	mtd_setup_common_members(mtd, priv, pdev);

And doesn't notice that the ordering matters.

You could make this simpler by either
(1) making mtd_setup_common_members() check for mtd->name==NULL first
(2) just move the (default) name assignment to the common MTD
    registration, like I suggested

> >>+
> >>  extern int mtd_device_parse_register(struct mtd_info *mtd,
> >>  				     const char * const *part_probe_types,
> >>  				     struct mtd_part_parser_data *parser_data,
> >
> >How about we rethink the "helper" approach, and instead just do
> >validation in the core code? This would cover most of the important
> >parts of your helper, I think:
> 
> Feel free to change all drivers. I like my approach with fixing 21
> bugs by reducing code by 44 lines.

I appreciate the bug fixes. I am just questioning the approach. Reduced
(source) code doesn't help anyone if it makes the code less
maintainable. My suggestions can probably make this more maintainable,
fix the same bugs, and reduce the source by a similar (albeit smaller)
number of lines.

> And it offers a common function where future similiarities can be
> put into too. Of course you can just add 21 lines, but that is not
> how I do such stuff.

I don't see how your common location helps much more than putting it in
mtdcore.c like I suggested, without the extra function.

> And I did the patches. If you don't like them, feel free to ignore
> them. I'm not playing remote keyboard but I do patches like I would
> do them, not like some arbitrary maintainer would do them. Sorry for
> the harsh words.

Arbitrary maintainer, eh? I'm simply suggesting that this could be
accomplished in a better way. If you don't want to take part in the
review process, then I have no obligation to engage either.

Regards,
Brian

Re: [PATCH 01/27] mtd: nand: introduce function to fix a common bug in most nand-drivers not showing a device in sysfs

[Alexander Holler]

Am 28.05.2014 22:10, schrieb Brian Norris:
> On Wed, May 28, 2014 at 08:52:06PM +0200, Alexander Holler wrote:
>> Am 28.05.2014 10:43, schrieb Brian Norris:
>>> On Tue, May 27, 2014 at 12:12:26AM +0200, Alexander Holler wrote:
>>>> +{
>>>> +	mtd->priv	= priv;
>>>
>>> I don't think you should hide this one here. It will be quite obvious if
>>> a driver didn't stash its private data but tries to access it later. Are
>>> there any drivers that missed this?
>>
>> No, it just saves a line of source in all drivers and I think it
>> fits there. I don't understand why do you think it is hidden.
>
> The function name doesn't make it obvious what it's doing. So all code
> readers will have to follow the definition to see what it's doing. But
> this point is not that important, so I won't argue.

So you think every function does hide important things? How's about C?
It hides which registers are used. Maybe we should go back to assembler.
But I prefer to just press ctrl-} in vim.

>>>> +	mtd->owner	= pdev->dev.driver->owner;
>>>> +	mtd->dev.parent	= &pdev->dev;
>>>> +	mtd->name	= pdev->dev.driver->name;
>>>
>>> I think this is a little dangerous. You're potentially clobbering the
>>> name that a driver already chose here. And why did you pick to use the

It's a dangerous world. And ordering sometimes matters. Feel free to
post another patch where you add an if() or an if (). I don't care which
style you prefer, but checkpatch would want the second one.

And as I currently don't see your argument to use the driver instead of
the platform device, here's an argument why I prefer to use pdev:
someone might want to use the upper layer (here the platform device) in
that silly function in the future. A very personal decision I did., but...

>>> driver name? This gives non-unique names if there is more than one
>>> device instantiated for a driver. That's why some drivers already use
>>> the device name, not the driver name:
>>>
>>> 	mtd->name = dev_name(&pev->dev);
>>>
>>> And in fact, if any drivers are missing mtd->name, perhaps it's best to
>>> just modify the MTD registration to give them a default:
>>>
>>> 	if (!mtd->name)
>>> 		mtd->name = dev_name(&pdev->dev);
>>>
>>>> +}
>>
>> I don't clobber any name. The name is set as default before drivers
>> might do this.

BTW. I don't like what dev_name produces. E.g. on one box I use here
devname would produce f4000000.nand instead orion_nand. So I would have
to name partitons with f4000000.nand. I don't understand why someone
would prefer f4000000.nand instead of orion_nand, but I accept that
other people do so. I don't have to understand everything.

> At the moment this is true, but the ordering is somewhat subtle if you
> don't examine the details of mtd_setup_common_members() to see that it
> is assigning a name. I can easily imagine some new driver in which
> someone does:
>
> 	mtd->name = ...;
> 	[...]
> 	mtd_setup_common_members(mtd, priv, pdev);
>
> And doesn't notice that the ordering matters.
>
> You could make this simpler by either
> (1) making mtd_setup_common_members() check for mtd->name==NULL first
> (2) just move the (default) name assignment to the common MTD
>     registration, like I suggested
>
>>>> +
>>>>  extern int mtd_device_parse_register(struct mtd_info *mtd,
>>>>  				     const char * const *part_probe_types,
>>>>  				     struct mtd_part_parser_data *parser_data,
>>>
>>> How about we rethink the "helper" approach, and instead just do
>>> validation in the core code? This would cover most of the important
>>> parts of your helper, I think:
>>
>> Feel free to change all drivers. I like my approach with fixing 21
>> bugs by reducing code by 44 lines.
>
> I appreciate the bug fixes. I am just questioning the approach. Reduced
> (source) code doesn't help anyone if it makes the code less
> maintainable. My suggestions can probably make this more maintainable,
> fix the same bugs, and reduce the source by a similar (albeit smaller)
> number of lines.
>
>> And it offers a common function where future similiarities can be
>> put into too. Of course you can just add 21 lines, but that is not
>> how I do such stuff.
>
> I don't see how your common location helps much more than putting it in
> mtdcore.c like I suggested, without the extra function.
>
>> And I did the patches. If you don't like them, feel free to ignore
>> them. I'm not playing remote keyboard but I do patches like I would
>> do them, not like some arbitrary maintainer would do them. Sorry for
>> the harsh words.
>
> Arbitrary maintainer, eh? I'm simply suggesting that this could be
> accomplished in a better way. If you don't want to take part in the
> review process, then I have no obligation to engage either.

TIMTOWTDI. Humans are different, they think different, they write
different code, they see code different and they prefer different
styles. And what you think is a better way, isn't one in my point of
view. And I prefer to not discuss such silly things like this simple 4
line function.

I'm very sorry, but I find such discussions extremly tiresome.

If you just would have suggested that one if to prevent that someone who
doesn't c&p existing code would end up with a clobbered name (which he
obviously can't miss if he ever would test his new or changed driver),
than I maybe would have send a v2 with that if. But all the other noise
just made me to want I never had send (again) a patch to LKML. It is
almost impossible to avoid such answers and it turns every patch posting
into a endless story where people do want to discuss every line or even
character of totally silly things.

Alexander Holler

Re: [PATCH 01/27] mtd: nand: introduce function to fix a common bug in most nand-drivers not showing a device in sysfs

[Brian Norris]

On Wed, May 28, 2014 at 11:09:05PM +0200, Alexander Holler wrote:
> I'm very sorry, but I find such discussions extremly tiresome.
> 
> If you just would have suggested that one if to prevent that someone who
> doesn't c&p existing code would end up with a clobbered name (which he
> obviously can't miss if he ever would test his new or changed driver),
> than I maybe would have send a v2 with that if. But all the other noise
> just made me to want I never had send (again) a patch to LKML. It is
> almost impossible to avoid such answers and it turns every patch posting
> into a endless story where people do want to discuss every line or even
> character of totally silly things.

It would help if you brought a more open attitude to the table.

https://lkml.org/lkml/2014/5/14/574

Re: [PATCH 01/27] mtd: nand: introduce function to fix a common bug in most nand-drivers not showing a device in sysfs

[Alexander Holler]

Am 28.05.2014 23:49, schrieb Brian Norris:
> On Wed, May 28, 2014 at 11:09:05PM +0200, Alexander Holler wrote:
>> I'm very sorry, but I find such discussions extremly tiresome.
>>
>> If you just would have suggested that one if to prevent that someone who
>> doesn't c&p existing code would end up with a clobbered name (which he
>> obviously can't miss if he ever would test his new or changed driver),
>> than I maybe would have send a v2 with that if. But all the other noise
>> just made me to want I never had send (again) a patch to LKML. It is
>> almost impossible to avoid such answers and it turns every patch posting
>> into a endless story where people do want to discuss every line or even
>> character of totally silly things.
>
> It would help if you brought a more open attitude to the table.

Your doesn't accept that other people do write code different than you 
and you want to tell me I have to bring a more open attitude to the table?

Re: [PATCH 01/27] mtd: nand: introduce function to fix a common bug in most nand-drivers not showing a device in sysfs

[Artem Bityutskiy]

On Wed, 2014-05-28 at 23:09 +0200, Alexander Holler wrote:
> I'm very sorry, but I find such discussions extremly tiresome.

Why discussing then at all, just go ahead and to something else.

-- 
Best Regards,
Artem Bityutskiy

Re: [PATCH 01/27] mtd: nand: introduce function to fix a common bug in most nand-drivers not showing a device in sysfs

[Alexander Holler]

Am 29.05.2014 08:17, schrieb Artem Bityutskiy:
> On Wed, 2014-05-28 at 23:09 +0200, Alexander Holler wrote:
>> I'm very sorry, but I find such discussions extremly tiresome.
>
> Why discussing then at all, just go ahead and to something else.

Agreed. In order to maintain psychical health it's better to not get in 
contact with this kindergarten.

Regards,

Alexander Holler

Re: [PATCH 01/27] mtd: nand: introduce function to fix a common bug in most nand-drivers not showing a device in sysfs

[Frans Klaver]

On Wed, May 28, 2014 at 01:43:44AM -0700, Brian Norris wrote:
> And in fact, if any drivers are missing mtd->name, perhaps it's best to
> just modify the MTD registration to give them a default:
> 
> 	if (!mtd->name)
> 		mtd->name = dev_name(&pdev->dev);
> 

...

> How about we rethink the "helper" approach, and instead just do
> validation in the core code? This would cover most of the important
> parts of your helper, I think:
> 
> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
> index d201feeb3ca6..39ba5812a5a3 100644
> --- a/drivers/mtd/mtdcore.c
> +++ b/drivers/mtd/mtdcore.c
> @@ -397,6 +397,11 @@ int add_mtd_device(struct mtd_info *mtd)
>  	if (device_register(&mtd->dev) != 0)
>  		goto fail_added;
>  
> +	if (mtd->dev.parent)
> +		mtd->owner = mtd->dev.parent->driver->owner;
> +	else
> +		WARN_ON(1);
> +

So I've picked this up now. I do largely agree with the suggested
approach where the validation and default settings are done in the core
code. There is a problem with this, though. There are MTD devices that
call mtd_device_parse_register() in the _init() function (such as the
maps drivers). These don't have a device ready to be used as parent, and
they would always be throwing this warning.

So either not having a parent device is bad, or it isn't. The comment
suggests it is, the existing code suggests it isn't. So we'll need to
make a decision about who's right.

>  	if (MTD_DEVT(i))
>  		device_create(&mtd_class, mtd->dev.parent,
>  			      MTD_DEVT(i) + 1,
> diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
> index 1ca9aec141ff..9869bbef50cf 100644
> --- a/drivers/mtd/mtdpart.c
> +++ b/drivers/mtd/mtdpart.c
> @@ -370,7 +370,6 @@ static struct mtd_part *allocate_partition(struct mtd_info *master,
>  	slave->mtd.subpage_sft = master->subpage_sft;
>  
>  	slave->mtd.name = name;
> -	slave->mtd.owner = master->owner;

What would be the purpose of removing this line? Owner is already set?
Can we rely on that?

Thanks,
Frans

Re: [PATCH 01/27] mtd: nand: introduce function to fix a common bug in most nand-drivers not showing a device in sysfs

[Brian Norris]

On Sun, Nov 02, 2014 at 10:03:53PM +0100, Frans Klaver wrote:
> On Wed, May 28, 2014 at 01:43:44AM -0700, Brian Norris wrote:
> > And in fact, if any drivers are missing mtd->name, perhaps it's best to
> > just modify the MTD registration to give them a default:
> > 
> > 	if (!mtd->name)
> > 		mtd->name = dev_name(&pdev->dev);
> > 
> 
> ...
> 
> > How about we rethink the "helper" approach, and instead just do
> > validation in the core code? This would cover most of the important
> > parts of your helper, I think:
> > 
> > diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
> > index d201feeb3ca6..39ba5812a5a3 100644
> > --- a/drivers/mtd/mtdcore.c
> > +++ b/drivers/mtd/mtdcore.c
> > @@ -397,6 +397,11 @@ int add_mtd_device(struct mtd_info *mtd)
> >  	if (device_register(&mtd->dev) != 0)
> >  		goto fail_added;
> >  
> > +	if (mtd->dev.parent)
> > +		mtd->owner = mtd->dev.parent->driver->owner;
> > +	else
> > +		WARN_ON(1);
> > +
> 
> So I've picked this up now. I do largely agree with the suggested
> approach where the validation and default settings are done in the core
> code. There is a problem with this, though. There are MTD devices that
> call mtd_device_parse_register() in the _init() function (such as the
> maps drivers). These don't have a device ready to be used as parent, and
> they would always be throwing this warning.

Yeah, I came across the same thing. I think gluebi is another example.

> So either not having a parent device is bad, or it isn't. The comment
> suggests it is, the existing code suggests it isn't. So we'll need to
> make a decision about who's right.

I think not having a parent is not really bad. It's helpful for tracking
the device hierarchy in sysfs, but it's not strictly necessary. So we
should probably not do anything drastic like WARN_ON() yet.

> >  	if (MTD_DEVT(i))
> >  		device_create(&mtd_class, mtd->dev.parent,
> >  			      MTD_DEVT(i) + 1,
> > diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
> > index 1ca9aec141ff..9869bbef50cf 100644
> > --- a/drivers/mtd/mtdpart.c
> > +++ b/drivers/mtd/mtdpart.c
> > @@ -370,7 +370,6 @@ static struct mtd_part *allocate_partition(struct mtd_info *master,
> >  	slave->mtd.subpage_sft = master->subpage_sft;
> >  
> >  	slave->mtd.name = name;
> > -	slave->mtd.owner = master->owner;
> 
> What would be the purpose of removing this line? Owner is already set?
> Can we rely on that?

I'm not completely sure why I wrote that, but I think the only call site
for alloc_partition() is in mtd_add_partition(), which calls
add_mtd_device().

Brian

Re: [PATCH 01/27] mtd: nand: introduce function to fix a common bug in most nand-drivers not showing a device in sysfs

[Frans Klaver]

On Wed, Nov 5, 2014 at 10:34 AM, Brian Norris
<computersforpeace@gmail.com> wrote:
> On Sun, Nov 02, 2014 at 10:03:53PM +0100, Frans Klaver wrote:
>> On Wed, May 28, 2014 at 01:43:44AM -0700, Brian Norris wrote:
>> > And in fact, if any drivers are missing mtd->name, perhaps it's best to
>> > just modify the MTD registration to give them a default:
>> >
>> >     if (!mtd->name)
>> >             mtd->name = dev_name(&pdev->dev);
>> >
>>
>> ...
>>
>> > How about we rethink the "helper" approach, and instead just do
>> > validation in the core code? This would cover most of the important
>> > parts of your helper, I think:
>> >
>> > diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
>> > index d201feeb3ca6..39ba5812a5a3 100644
>> > --- a/drivers/mtd/mtdcore.c
>> > +++ b/drivers/mtd/mtdcore.c
>> > @@ -397,6 +397,11 @@ int add_mtd_device(struct mtd_info *mtd)
>> >     if (device_register(&mtd->dev) != 0)
>> >             goto fail_added;
>> >
>> > +   if (mtd->dev.parent)
>> > +           mtd->owner = mtd->dev.parent->driver->owner;
>> > +   else
>> > +           WARN_ON(1);
>> > +
>>
>> So I've picked this up now. I do largely agree with the suggested
>> approach where the validation and default settings are done in the core
>> code. There is a problem with this, though. There are MTD devices that
>> call mtd_device_parse_register() in the _init() function (such as the
>> maps drivers). These don't have a device ready to be used as parent, and
>> they would always be throwing this warning.
>
> Yeah, I came across the same thing. I think gluebi is another example.
>
>> So either not having a parent device is bad, or it isn't. The comment
>> suggests it is, the existing code suggests it isn't. So we'll need to
>> make a decision about who's right.
>
> I think not having a parent is not really bad. It's helpful for tracking
> the device hierarchy in sysfs, but it's not strictly necessary. So we
> should probably not do anything drastic like WARN_ON() yet.

OK. I'll probably add some sane defaults to add_mtd_device() and have
the mtd drivers make use of that fact. Since this isn't really
critical, I guess fixing the sysfs entry won't need back-porting.


>> >     if (MTD_DEVT(i))
>> >             device_create(&mtd_class, mtd->dev.parent,
>> >                           MTD_DEVT(i) + 1,
>> > diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
>> > index 1ca9aec141ff..9869bbef50cf 100644
>> > --- a/drivers/mtd/mtdpart.c
>> > +++ b/drivers/mtd/mtdpart.c
>> > @@ -370,7 +370,6 @@ static struct mtd_part *allocate_partition(struct mtd_info *master,
>> >     slave->mtd.subpage_sft = master->subpage_sft;
>> >
>> >     slave->mtd.name = name;
>> > -   slave->mtd.owner = master->owner;
>>
>> What would be the purpose of removing this line? Owner is already set?
>> Can we rely on that?
>
> I'm not completely sure why I wrote that, but I think the only call site
> for alloc_partition() is in mtd_add_partition(), which calls
> add_mtd_device().

Alright, I'll have a brief look into that again, then.

Thanks,
Frans

[PATCH 02/27] mtd: nand: orion_nand: show device structure in sysfs

[Alexander Holler]

Fix a common error in nand-drivers which do not set up a parent device for
the mtd-device by using a new inline function.

Signed-off-by: Alexander Holler <holler@ahsoftware.de>
---
 drivers/mtd/nand/orion_nand.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/mtd/nand/orion_nand.c b/drivers/mtd/nand/orion_nand.c
index dd7fe81..3cd43f7 100644
--- a/drivers/mtd/nand/orion_nand.c
+++ b/drivers/mtd/nand/orion_nand.c
@@ -91,6 +91,7 @@ static int __init orion_nand_probe(struct platform_device *pdev)
 		goto no_res;
 	}
 	mtd = (struct mtd_info *)(nc + 1);
+	mtd_setup_common_members(mtd, nc, pdev);
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	if (!res) {
@@ -132,9 +133,6 @@ static int __init orion_nand_probe(struct platform_device *pdev)
 		board = dev_get_platdata(&pdev->dev);
 	}
 
-	mtd->priv = nc;
-	mtd->owner = THIS_MODULE;
-
 	nc->priv = board;
 	nc->IO_ADDR_R = nc->IO_ADDR_W = io_base;
 	nc->cmd_ctrl = orion_nand_cmd_ctrl;
@@ -169,7 +167,6 @@ static int __init orion_nand_probe(struct platform_device *pdev)
 		goto no_dev;
 	}
 
-	mtd->name = "orion_nand";
 	ppdata.of_node = pdev->dev.of_node;
 	ret = mtd_device_parse_register(mtd, NULL, &ppdata,
 			board->parts, board->nr_parts);
-- 
1.8.3.2

[PATCH 03/27] mtd: nand: omap2: show device structure in sysfs

[Alexander Holler]

Fix a common error in nand-drivers which do not set up a parent device for
the mtd-device by using a new inline function.

Signed-off-by: Alexander Holler <holler@ahsoftware.de>
---
 drivers/mtd/nand/omap2.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c
index 1ff49b8..c761334 100644
--- a/drivers/mtd/nand/omap2.c
+++ b/drivers/mtd/nand/omap2.c
@@ -1598,9 +1598,8 @@ static int omap_nand_probe(struct platform_device *pdev)
 	info->of_node		= pdata->of_node;
 	info->ecc_opt		= pdata->ecc_opt;
 	mtd			= &info->mtd;
-	mtd->priv		= &info->nand;
+	mtd_setup_common_members(mtd, &info->nand, pdev);
 	mtd->name		= dev_name(&pdev->dev);
-	mtd->owner		= THIS_MODULE;
 	nand_chip		= &info->nand;
 	nand_chip->ecc.priv	= NULL;
 	nand_chip->options	|= NAND_SKIP_BBTSCAN;
-- 
1.8.3.2

[PATCH 04/27] mtd: nand: atmel_nand: show device structure in sysfs

[Alexander Holler]

Fix a common error in nand-drivers which do not set up a parent device for
the mtd-device by using a new inline function.

Signed-off-by: Alexander Holler <holler@ahsoftware.de>
---
 drivers/mtd/nand/atmel_nand.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/mtd/nand/atmel_nand.c b/drivers/mtd/nand/atmel_nand.c
index 4ce181a..8056c36 100644
--- a/drivers/mtd/nand/atmel_nand.c
+++ b/drivers/mtd/nand/atmel_nand.c
@@ -1980,6 +1980,7 @@ static int atmel_nand_probe(struct platform_device *pdev)
 	host->io_phys = (dma_addr_t)mem->start;
 
 	mtd = &host->mtd;
+	mtd_setup_common_members(mtd, nand_chip, pdev);
 	nand_chip = &host->nand_chip;
 	host->dev = &pdev->dev;
 	if (IS_ENABLED(CONFIG_OF) && pdev->dev.of_node) {
@@ -1993,8 +1994,6 @@ static int atmel_nand_probe(struct platform_device *pdev)
 	}
 
 	nand_chip->priv = host;		/* link the private data structures */
-	mtd->priv = nand_chip;
-	mtd->owner = THIS_MODULE;
 
 	/* Set address of NAND IO lines */
 	nand_chip->IO_ADDR_R = host->io_base;
@@ -2124,7 +2123,6 @@ static int atmel_nand_probe(struct platform_device *pdev)
 		goto err_scan_tail;
 	}
 
-	mtd->name = "atmel_nand";
 	ppdata.of_node = pdev->dev.of_node;
 	res = mtd_device_parse_register(mtd, NULL, &ppdata,
 			host->board.parts, host->board.num_parts);
-- 
1.8.3.2

[PATCH 05/27] mtd: nand: gpio: show device structure in sysfs

[Alexander Holler]

Fix a common error in nand-drivers which do not set up a parent device for
the mtd-device by using a new inline function.

Signed-off-by: Alexander Holler <holler@ahsoftware.de>
---
 drivers/mtd/nand/gpio.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/mtd/nand/gpio.c b/drivers/mtd/nand/gpio.c
index 117ce33..bbe55ab 100644
--- a/drivers/mtd/nand/gpio.c
+++ b/drivers/mtd/nand/gpio.c
@@ -272,9 +272,7 @@ static int gpio_nand_probe(struct platform_device *pdev)
 	chip->chip_delay	= gpiomtd->plat.chip_delay;
 	chip->cmd_ctrl		= gpio_nand_cmd_ctrl;
 
-	gpiomtd->mtd_info.priv	= chip;
-	gpiomtd->mtd_info.owner	= THIS_MODULE;
-
+	mtd_setup_common_members(&gpiomtd->mtd_info, chip, pdev);
 	platform_set_drvdata(pdev, gpiomtd);
 
 	if (gpio_is_valid(gpiomtd->plat.gpio_nwp))
-- 
1.8.3.2

[PATCH 06/27] mtd: nand: fsmc: show device structure in sysfs

[Alexander Holler]

Fix a common error in nand-drivers which do not set up a parent device for
the mtd-device by using a new inline function.

Signed-off-by: Alexander Holler <holler@ahsoftware.de>
---
 drivers/mtd/nand/fsmc_nand.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/mtd/nand/fsmc_nand.c b/drivers/mtd/nand/fsmc_nand.c
index 1550692..33c9cce 100644
--- a/drivers/mtd/nand/fsmc_nand.c
+++ b/drivers/mtd/nand/fsmc_nand.c
@@ -1009,10 +1009,9 @@ static int __init fsmc_nand_probe(struct platform_device *pdev)
 	/* Link all private pointers */
 	mtd = &host->mtd;
 	nand = &host->nand;
-	mtd->priv = nand;
+	mtd_setup_common_members(mtd, nand, pdev);
 	nand->priv = host;
 
-	host->mtd.owner = THIS_MODULE;
 	nand->IO_ADDR_R = host->data_va;
 	nand->IO_ADDR_W = host->data_va;
 	nand->cmd_ctrl = fsmc_cmd_ctrl;
-- 
1.8.3.2

[PATCH 07/27] mtd: nand: gpmi: show device structure in sysfs

[Alexander Holler]

Fix a common error in nand-drivers which do not set up a parent device for
the mtd-device by using a new inline function.

Signed-off-by: Alexander Holler <holler@ahsoftware.de>
---
 drivers/mtd/nand/gpmi-nand/gpmi-nand.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
index bb77f75..35b0a60 100644
--- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
+++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
@@ -1682,11 +1682,6 @@ static int gpmi_nand_init(struct gpmi_nand_data *this)
 	/* init current chip */
 	this->current_chip	= -1;
 
-	/* init the MTD data structures */
-	mtd->priv		= chip;
-	mtd->name		= "gpmi-nand";
-	mtd->owner		= THIS_MODULE;
-
 	/* init the nand_chip{}, we don't support a 16-bit NAND Flash bus. */
 	chip->priv		= this;
 	chip->select_chip	= gpmi_select_chip;
@@ -1791,6 +1786,7 @@ static int gpmi_nand_probe(struct platform_device *pdev)
 	if (ret)
 		goto exit_nfc_init;
 
+	mtd_setup_common_members(&this->mtd, &this->nand, pdev);
 	ret = gpmi_nand_init(this);
 	if (ret)
 		goto exit_nfc_init;
-- 
1.8.3.2

[PATCH 08/27] mtd: nand: plat: show device structure in sysfs

[Alexander Holler]

Fix a common error in nand-drivers which do not set up a parent device for
the mtd-device by using a new inline function.

Signed-off-by: Alexander Holler <holler@ahsoftware.de>
---
 drivers/mtd/nand/plat_nand.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/mtd/nand/plat_nand.c b/drivers/mtd/nand/plat_nand.c
index 0b068a5..4d4cc25 100644
--- a/drivers/mtd/nand/plat_nand.c
+++ b/drivers/mtd/nand/plat_nand.c
@@ -60,8 +60,7 @@ static int plat_nand_probe(struct platform_device *pdev)
 		return PTR_ERR(data->io_base);
 
 	data->chip.priv = &data;
-	data->mtd.priv = &data->chip;
-	data->mtd.owner = THIS_MODULE;
+	mtd_setup_common_members(&data->mtd, &data->chip, pdev);
 	data->mtd.name = dev_name(&pdev->dev);
 
 	data->chip.IO_ADDR_R = data->io_base;
-- 
1.8.3.2

[PATCH 09/27] mtd: nand: pxa3xx: show device structure in sysfs

[Alexander Holler]

Fix a common error in nand-drivers which do not set up a parent device for
the mtd-device by using a new inline function.

Signed-off-by: Alexander Holler <holler@ahsoftware.de>
---
 drivers/mtd/nand/pxa3xx_nand.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c
index 7588fe2..7f62e7c 100644
--- a/drivers/mtd/nand/pxa3xx_nand.c
+++ b/drivers/mtd/nand/pxa3xx_nand.c
@@ -1584,8 +1584,7 @@ static int alloc_nand_resource(struct platform_device *pdev)
 		host->mtd = mtd;
 		host->cs = cs;
 		host->info_data = info;
-		mtd->priv = host;
-		mtd->owner = THIS_MODULE;
+		mtd_setup_common_members(mtd, host, pdev);
 
 		chip->ecc.read_page	= pxa3xx_nand_read_page_hwecc;
 		chip->ecc.write_page	= pxa3xx_nand_write_page_hwecc;
-- 
1.8.3.2

Re: [PATCH 09/27] mtd: nand: pxa3xx: show device structure in sysfs

[Alexander Shiyan]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=utf-8, Size: 1193 bytes --]

Tue, 27 May 2014 00:12:34 +0200 от Alexander Holler <holler@ahsoftware.de>:
> Fix a common error in nand-drivers which do not set up a parent device for
> the mtd-device by using a new inline function.
> 
> Signed-off-by: Alexander Holler <holler@ahsoftware.de>
> ---
>  drivers/mtd/nand/pxa3xx_nand.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c
> index 7588fe2..7f62e7c 100644
> --- a/drivers/mtd/nand/pxa3xx_nand.c
> +++ b/drivers/mtd/nand/pxa3xx_nand.c
> @@ -1584,8 +1584,7 @@ static int alloc_nand_resource(struct platform_device *pdev)
>  		host->mtd = mtd;
>  		host->cs = cs;
>  		host->info_data = info;
> -		mtd->priv = host;
> -		mtd->owner = THIS_MODULE;
> +		mtd_setup_common_members(mtd, host, pdev);
>  
>  		chip->ecc.read_page	= pxa3xx_nand_read_page_hwecc;
>  		chip->ecc.write_page	= pxa3xx_nand_write_page_hwecc;
> -- 

Should we add "driver.owner = THIS_MODULE" field for struct platform_driver in this case?

---

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH 09/27] mtd: nand: pxa3xx: show device structure in sysfs

[Alexander Holler]

Am 27.05.2014 05:12, schrieb Alexander Shiyan:
> Tue, 27 May 2014 00:12:34 +0200 от Alexander Holler <holler@ahsoftware.de>:
>> Fix a common error in nand-drivers which do not set up a parent devicefor
>> the mtd-device by using a new inline function.
>>
>> Signed-off-by: Alexander Holler <holler@ahsoftware.de>
>> ---
>>   drivers/mtd/nand/pxa3xx_nand.c | 3 +--
>>   1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c
>> index 7588fe2..7f62e7c 100644
>> --- a/drivers/mtd/nand/pxa3xx_nand.c
>> +++ b/drivers/mtd/nand/pxa3xx_nand.c
>> @@ -1584,8 +1584,7 @@ static int alloc_nand_resource(struct platform_device *pdev)
>>   		host->mtd = mtd;
>>   		host->cs = cs;
>>   		host->info_data = info;
>> -		mtd->priv = host;
>> -		mtd->owner = THIS_MODULE;
>> +		mtd_setup_common_members(mtd, host, pdev);
>>
>>   		chip->ecc.read_page	= pxa3xx_nand_read_page_hwecc;
>>   		chip->ecc.write_page	= pxa3xx_nand_write_page_hwecc;
>> --
>
> Should we add "driver.owner = THIS_MODULE" field for struct platform_driver in this case?

Yes. I assumed all drivers/modules already had an owner. I will check 
them all and will send a v2 for those which don't have one. I wonder 
what this field is used for if it works without. ;)

Regards,

Alexander Holler

Re: [PATCH 09/27] mtd: nand: pxa3xx: show device structure in sysfs

[Brian Norris]

On Tue, May 27, 2014 at 08:01:55AM +0200, Alexander Holler wrote:
> Am 27.05.2014 05:12, schrieb Alexander Shiyan:
> >Should we add "driver.owner = THIS_MODULE" field for struct platform_driver in this case?
> 
> Yes. I assumed all drivers/modules already had an owner. I will
> check them all and will send a v2 for those which don't have one. I
> wonder what this field is used for if it works without. ;)

Looks like the mtd->owner essentially filters down to a try_module_get()
(called in the get_mtd_device() API) which ensures that MTD users (e.g.,
mtdblock, UBI, etc.) hold a refcount on the driver module. If the owner
is not set properly by a driver, then try_module_get() just does a
silent no-op, so the user is none the wiser... until they try to rmmod
their MTD driver while it's being used by UBI/UBIFS!

Regards,
Brian

Re: [PATCH 09/27] mtd: nand: pxa3xx: show device structure in sysfs

[Ezequiel Garcia]

Picking on this old thread...

On 27 May 07:12 AM, Alexander Shiyan wrote:
> > diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c
> > index 7588fe2..7f62e7c 100644
> > --- a/drivers/mtd/nand/pxa3xx_nand.c
> > +++ b/drivers/mtd/nand/pxa3xx_nand.c
> > @@ -1584,8 +1584,7 @@ static int alloc_nand_resource(struct platform_device *pdev)
> >  		host->mtd = mtd;
> >  		host->cs = cs;
> >  		host->info_data = info;
> > -		mtd->priv = host;
> > -		mtd->owner = THIS_MODULE;
> > +		mtd_setup_common_members(mtd, host, pdev);
> >  
> >  		chip->ecc.read_page	= pxa3xx_nand_read_page_hwecc;
> >  		chip->ecc.write_page	= pxa3xx_nand_write_page_hwecc;
> > -- 
> 
> Should we add "driver.owner = THIS_MODULE" field for struct platform_driver in this case?
> 

As far as I can see, for platform drivers, the owner of the driver is fixed to
THIS_MODULE in __platform_driver_register().

If there's anything left to fix for this driver, please let me know.

Thanks,
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

Re: [PATCH 09/27] mtd: nand: pxa3xx: show device structure in sysfs

[Brian Norris]

On Fri, Aug 08, 2014 at 02:04:28PM -0300, Ezequiel Garcia wrote:
> Picking on this old thread...
> 
> On 27 May 07:12 AM, Alexander Shiyan wrote:
> > > diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c
> > > index 7588fe2..7f62e7c 100644
> > > --- a/drivers/mtd/nand/pxa3xx_nand.c
> > > +++ b/drivers/mtd/nand/pxa3xx_nand.c
> > > @@ -1584,8 +1584,7 @@ static int alloc_nand_resource(struct platform_device *pdev)
> > >  		host->mtd = mtd;
> > >  		host->cs = cs;
> > >  		host->info_data = info;
> > > -		mtd->priv = host;
> > > -		mtd->owner = THIS_MODULE;
> > > +		mtd_setup_common_members(mtd, host, pdev);
> > >  
> > >  		chip->ecc.read_page	= pxa3xx_nand_read_page_hwecc;
> > >  		chip->ecc.write_page	= pxa3xx_nand_write_page_hwecc;
> > > -- 
> > 
> > Should we add "driver.owner = THIS_MODULE" field for struct platform_driver in this case?
> > 
> 
> As far as I can see, for platform drivers, the owner of the driver is fixed to
> THIS_MODULE in __platform_driver_register().

Looks like it. So any driver using module_platform_driver() or
platform_driver_register() and also initializes
platform_driver.driver.owner is just being redundant.

Maybe another "cleanup" project for Jingoo Han? ;)

> If there's anything left to fix for this driver, please let me know.

pxa3xx_nand is still missing a parent for its MTD's (mtd->dev.parent). I
have a follow-up patch queued up locally that I need to send out
sometime.

Brian

Re: [PATCH 09/27] mtd: nand: pxa3xx: show device structure in sysfs

[Ezequiel Garcia]

On 08 Aug 10:16 AM, Brian Norris wrote:
> On Fri, Aug 08, 2014 at 02:04:28PM -0300, Ezequiel Garcia wrote:

> > If there's anything left to fix for this driver, please let me know.
> 
> pxa3xx_nand is still missing a parent for its MTD's (mtd->dev.parent). I
> have a follow-up patch queued up locally that I need to send out
> sometime.
> 

Maybe we can trade... you provide the reason for such parent, and I'll cook
a patch in exchange :)

-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

Re: [PATCH 09/27] mtd: nand: pxa3xx: show device structure in sysfs

[Alexander Holler]

Am 08.08.2014 20:11, schrieb Ezequiel Garcia:
> On 08 Aug 10:16 AM, Brian Norris wrote:
>> On Fri, Aug 08, 2014 at 02:04:28PM -0300, Ezequiel Garcia wrote:
>
>>> If there's anything left to fix for this driver, please let me know.
>>
>> pxa3xx_nand is still missing a parent for its MTD's (mtd->dev.parent). I
>> have a follow-up patch queued up locally that I need to send out
>> sometime.
>>
>
> Maybe we can trade... you provide the reason for such parent, and I'll cook
> a patch in exchange :)

Just read the subject.and the introductory mail to the simple 20+ 
pathches I wasted my time with.

Regards,

Alexander Holler

[PATCH 10/27] mtd: nand: s3c2410: show device structure in sysfs

[Alexander Holler]

Fix a common error in nand-drivers which do not set up a parent device for
the mtd-device by using a new inline function.

Signed-off-by: Alexander Holler <holler@ahsoftware.de>
---
 drivers/mtd/nand/s3c2410.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/mtd/nand/s3c2410.c b/drivers/mtd/nand/s3c2410.c
index 79acbb8..c3227f4 100644
--- a/drivers/mtd/nand/s3c2410.c
+++ b/drivers/mtd/nand/s3c2410.c
@@ -831,8 +831,6 @@ static void s3c2410_nand_init_chip(struct s3c2410_nand_info *info,
 	chip->IO_ADDR_R = chip->IO_ADDR_W;
 
 	nmtd->info	   = info;
-	nmtd->mtd.priv	   = chip;
-	nmtd->mtd.owner    = THIS_MODULE;
 	nmtd->set	   = set;
 
 #ifdef CONFIG_MTD_NAND_S3C2410_HWECC
@@ -1018,6 +1016,7 @@ static int s3c24xx_nand_probe(struct platform_device *pdev)
 		pr_debug("initialising set %d (%p, info %p)\n",
 			 setno, nmtd, info);
 
+		mtd_setup_common_members(&nmtd->mtd, &nmtd->chip, pdev);
 		s3c2410_nand_init_chip(info, nmtd, sets);
 
 		nmtd->scan_res = nand_scan_ident(&nmtd->mtd,
-- 
1.8.3.2

[PATCH 11/27] mtd: nand: sh_flctl: show device structure in sysfs

[Alexander Holler]

Fix a common error in nand-drivers which do not set up a parent device for
the mtd-device by using a new inline function.

Signed-off-by: Alexander Holler <holler@ahsoftware.de>
---
 drivers/mtd/nand/sh_flctl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mtd/nand/sh_flctl.c b/drivers/mtd/nand/sh_flctl.c
index c067023..9c3f2e5 100644
--- a/drivers/mtd/nand/sh_flctl.c
+++ b/drivers/mtd/nand/sh_flctl.c
@@ -1124,7 +1124,7 @@ static int flctl_probe(struct platform_device *pdev)
 	platform_set_drvdata(pdev, flctl);
 	flctl_mtd = &flctl->mtd;
 	nand = &flctl->chip;
-	flctl_mtd->priv = nand;
+	mtd_setup_common_members(flctl_mtd, nand, pdev);
 	flctl->pdev = pdev;
 	flctl->hwecc = pdata->has_hwecc;
 	flctl->holden = pdata->use_holden;
-- 
1.8.3.2

[PATCH 12/27] mtd: nand: sharpsl: show device structure in sysfs

[Alexander Holler]

Fix a common error in nand-drivers which do not set up a parent device for
the mtd-device by using a new inline function.

Signed-off-by: Alexander Holler <holler@ahsoftware.de>
---
 drivers/mtd/nand/sharpsl.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/mtd/nand/sharpsl.c b/drivers/mtd/nand/sharpsl.c
index e81059b..1a9824a 100644
--- a/drivers/mtd/nand/sharpsl.c
+++ b/drivers/mtd/nand/sharpsl.c
@@ -142,10 +142,7 @@ static int sharpsl_nand_probe(struct platform_device *pdev)
 	/* Get pointer to private data */
 	this = (struct nand_chip *)(&sharpsl->chip);
 
-	/* Link the private data with the MTD structure */
-	sharpsl->mtd.priv = this;
-	sharpsl->mtd.owner = THIS_MODULE;
-
+	mtd_setup_common_members(&sharpsl->mtd, this, pdev);
 	platform_set_drvdata(pdev, sharpsl);
 
 	/*
@@ -178,8 +175,6 @@ static int sharpsl_nand_probe(struct platform_device *pdev)
 		goto err_scan;
 
 	/* Register the partitions */
-	sharpsl->mtd.name = "sharpsl-nand";
-
 	err = mtd_device_parse_register(&sharpsl->mtd, NULL, NULL,
 					data->partitions, data->nr_partitions);
 	if (err)
-- 
1.8.3.2

[PATCH 13/27] mtd: nand: tmio: show device structure in sysfs

[Alexander Holler]

Fix a common error in nand-drivers which do not set up a parent device for
the mtd-device by using a new inline function.

Signed-off-by: Alexander Holler <holler@ahsoftware.de>
---
 drivers/mtd/nand/tmio_nand.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/mtd/nand/tmio_nand.c b/drivers/mtd/nand/tmio_nand.c
index fb8fd35..ff22a31 100644
--- a/drivers/mtd/nand/tmio_nand.c
+++ b/drivers/mtd/nand/tmio_nand.c
@@ -380,8 +380,7 @@ static int tmio_probe(struct platform_device *dev)
 	platform_set_drvdata(dev, tmio);
 	mtd = &tmio->mtd;
 	nand_chip = &tmio->chip;
-	mtd->priv = nand_chip;
-	mtd->name = "tmio-nand";
+	mtd_setup_common_members(mtd, nand_chip, dev);
 
 	tmio->ccr = devm_ioremap(&dev->dev, ccr->start, resource_size(ccr));
 	if (!tmio->ccr)
-- 
1.8.3.2

[PATCH 14/27] mtd: nand: docg4: show device structure in sysfs

[Alexander Holler]

Fix a common error in nand-drivers which do not set up a parent device for
the mtd-device by using a new inline function.

Signed-off-by: Alexander Holler <holler@ahsoftware.de>
---
 drivers/mtd/nand/docg4.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/mtd/nand/docg4.c b/drivers/mtd/nand/docg4.c
index 1b0265e..50ad7a7 100644
--- a/drivers/mtd/nand/docg4.c
+++ b/drivers/mtd/nand/docg4.c
@@ -1312,9 +1312,8 @@ static int __init probe_docg4(struct platform_device *pdev)
 	}
 	nand = (struct nand_chip *) (mtd + 1);
 	doc = (struct docg4_priv *) (nand + 1);
-	mtd->priv = nand;
+	mtd_setup_common_members(mtd, nand, pdev);
 	nand->priv = doc;
-	mtd->owner = THIS_MODULE;
 	doc->virtadr = virtadr;
 	doc->dev = dev;
 
-- 
1.8.3.2

[PATCH 15/27] mtd: nand: davinci: use mtd_setup_common_members()

[Alexander Holler]

Use the new common function mtd_setup_common_members()

Signed-off-by: Alexander Holler <holler@ahsoftware.de>
---
 drivers/mtd/nand/davinci_nand.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/mtd/nand/davinci_nand.c b/drivers/mtd/nand/davinci_nand.c
index b922c8e..ce409fc 100644
--- a/drivers/mtd/nand/davinci_nand.c
+++ b/drivers/mtd/nand/davinci_nand.c
@@ -656,11 +656,8 @@ static int nand_davinci_probe(struct platform_device *pdev)
 	info->base		= base;
 	info->vaddr		= vaddr;
 
-	info->mtd.priv		= &info->chip;
+	mtd_setup_common_members(&info->mtd, &info->chip, pdev);
 	info->mtd.name		= dev_name(&pdev->dev);
-	info->mtd.owner		= THIS_MODULE;
-
-	info->mtd.dev.parent	= &pdev->dev;
 
 	info->chip.IO_ADDR_R	= vaddr;
 	info->chip.IO_ADDR_W	= vaddr;
-- 
1.8.3.2

[PATCH 16/27] mtd: nand: lpc32xx_mlc: use mtd_setup_common_members()

[Alexander Holler]

Use the new common function mtd_setup_common_members()

Signed-off-by: Alexander Holler <holler@ahsoftware.de>
---
 drivers/mtd/nand/lpc32xx_mlc.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/mtd/nand/lpc32xx_mlc.c b/drivers/mtd/nand/lpc32xx_mlc.c
index 687478c..4fa7b13 100644
--- a/drivers/mtd/nand/lpc32xx_mlc.c
+++ b/drivers/mtd/nand/lpc32xx_mlc.c
@@ -681,9 +681,7 @@ static int lpc32xx_nand_probe(struct platform_device *pdev)
 	host->pdata = dev_get_platdata(&pdev->dev);
 
 	nand_chip->priv = host;		/* link the private data structures */
-	mtd->priv = nand_chip;
-	mtd->owner = THIS_MODULE;
-	mtd->dev.parent = &pdev->dev;
+	mtd_setup_common_members(mtd, nand_chip, pdev);
 
 	/* Get NAND clock */
 	host->clk = clk_get(&pdev->dev, NULL);
@@ -790,8 +788,6 @@ static int lpc32xx_nand_probe(struct platform_device *pdev)
 		goto err_exit4;
 	}
 
-	mtd->name = DRV_NAME;
-
 	ppdata.of_node = pdev->dev.of_node;
 	res = mtd_device_parse_register(mtd, NULL, &ppdata, host->ncfg->parts,
 					host->ncfg->num_parts);
-- 
1.8.3.2

[PATCH 17/27] mtd: nand: lpc32xx_slc: use mtd_setup_common_members()

[Alexander Holler]

Use the new common function mtd_setup_common_members()

Signed-off-by: Alexander Holler <holler@ahsoftware.de>
---
 drivers/mtd/nand/lpc32xx_slc.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/mtd/nand/lpc32xx_slc.c b/drivers/mtd/nand/lpc32xx_slc.c
index 53a6742..c8fb46b 100644
--- a/drivers/mtd/nand/lpc32xx_slc.c
+++ b/drivers/mtd/nand/lpc32xx_slc.c
@@ -799,9 +799,7 @@ static int lpc32xx_nand_probe(struct platform_device *pdev)
 	mtd = &host->mtd;
 	chip = &host->nand_chip;
 	chip->priv = host;
-	mtd->priv = chip;
-	mtd->owner = THIS_MODULE;
-	mtd->dev.parent = &pdev->dev;
+	mtd_setup_common_members(mtd, chip, pdev);
 
 	/* Get NAND clock */
 	host->clk = devm_clk_get(&pdev->dev, NULL);
-- 
1.8.3.2

[PATCH 18/27] mtd: nand: mxc: use mtd_setup_common_members()

[Alexander Holler]

Use the new common function mtd_setup_common_members()

Signed-off-by: Alexander Holler <holler@ahsoftware.de>
---
 drivers/mtd/nand/mxc_nand.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/mtd/nand/mxc_nand.c b/drivers/mtd/nand/mxc_nand.c
index dba262b..31114db4 100644
--- a/drivers/mtd/nand/mxc_nand.c
+++ b/drivers/mtd/nand/mxc_nand.c
@@ -1413,10 +1413,7 @@ static int mxcnd_probe(struct platform_device *pdev)
 	/* structures must be linked */
 	this = &host->nand;
 	mtd = &host->mtd;
-	mtd->priv = this;
-	mtd->owner = THIS_MODULE;
-	mtd->dev.parent = &pdev->dev;
-	mtd->name = DRIVER_NAME;
+	mtd_setup_common_members(mtd, this, pdev);
 
 	/* 50 us command delay time */
 	this->chip_delay = 5;
-- 
1.8.3.2

[PATCH 19/27] mtd: nand: bcm47: show device structure in sysfs

[Alexander Holler]

Fix a common error in nand-drivers which do not set up a parent device for
the mtd-device by using a new inline function.

Signed-off-by: Alexander Holler <holler@ahsoftware.de>
---
 drivers/mtd/nand/bcm47xxnflash/main.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/mtd/nand/bcm47xxnflash/main.c b/drivers/mtd/nand/bcm47xxnflash/main.c
index 1074459..f995d5c 100644
--- a/drivers/mtd/nand/bcm47xxnflash/main.c
+++ b/drivers/mtd/nand/bcm47xxnflash/main.c
@@ -34,8 +34,7 @@ static int bcm47xxnflash_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	b47n->nand_chip.priv = b47n;
-	b47n->mtd.owner = THIS_MODULE;
-	b47n->mtd.priv = &b47n->nand_chip; /* Required */
+	mtd_setup_common_members(&b47n->mtd, &b47n->nand_chip, pdev);
 	b47n->cc = container_of(nflash, struct bcma_drv_cc, nflash);
 
 	if (b47n->cc->core->bus->chipinfo.id == BCMA_CHIP_ID_BCM4706) {
-- 
1.8.3.2

[PATCH 20/27] mtd: nand: fsl_elbc: show device structure in sysfs

[Alexander Holler]

Fix a common error in nand-drivers which do not set up a parent device for
the mtd-device by using a new inline function.

Signed-off-by: Alexander Holler <holler@ahsoftware.de>
---
 drivers/mtd/nand/fsl_elbc_nand.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/mtd/nand/fsl_elbc_nand.c b/drivers/mtd/nand/fsl_elbc_nand.c
index ec549cd..dd2df90 100644
--- a/drivers/mtd/nand/fsl_elbc_nand.c
+++ b/drivers/mtd/nand/fsl_elbc_nand.c
@@ -732,10 +732,6 @@ static int fsl_elbc_chip_init(struct fsl_elbc_mtd *priv)
 
 	dev_dbg(priv->dev, "eLBC Set Information for bank %d\n", priv->bank);
 
-	/* Fill in fsl_elbc_mtd structure */
-	priv->mtd.priv = chip;
-	priv->mtd.owner = THIS_MODULE;
-
 	/* set timeout to maximum */
 	priv->fmr = 15 << FMR_CWTO_SHIFT;
 	if (in_be32(&lbc->bank[priv->bank].or) & OR_FCM_PGS)
@@ -873,6 +869,7 @@ static int fsl_elbc_nand_probe(struct platform_device *pdev)
 		goto err;
 	}
 
+	mtd_setup_common_members(&priv->mtd, &priv->chip, pdev);
 	priv->mtd.name = kasprintf(GFP_KERNEL, "%llx.flash", (u64)res.start);
 	if (!priv->mtd.name) {
 		ret = -ENOMEM;
-- 
1.8.3.2

[PATCH 21/27] mtd: nand: fsl_upm: show device structure in sysfs

[Alexander Holler]

Fix a common error in nand-drivers which do not set up a parent device for
the mtd-device by using a new inline function.

Signed-off-by: Alexander Holler <holler@ahsoftware.de>
---
 drivers/mtd/nand/fsl_upm.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/mtd/nand/fsl_upm.c b/drivers/mtd/nand/fsl_upm.c
index 4d203e8..bc2a188 100644
--- a/drivers/mtd/nand/fsl_upm.c
+++ b/drivers/mtd/nand/fsl_upm.c
@@ -175,9 +175,6 @@ static int fun_chip_init(struct fsl_upm_nand *fun,
 	if (fun->rnb_gpio[0] >= 0)
 		fun->chip.dev_ready = fun_chip_ready;
 
-	fun->mtd.priv = &fun->chip;
-	fun->mtd.owner = THIS_MODULE;
-
 	flash_np = of_get_next_child(upm_np, NULL);
 	if (!flash_np)
 		return -ENODEV;
@@ -300,6 +297,7 @@ static int fun_probe(struct platform_device *ofdev)
 	fun->dev = &ofdev->dev;
 	fun->last_ctrl = NAND_CLE;
 
+	mtd_setup_common_members(&fun->mtd, &fun->chip, ofdev);
 	ret = fun_chip_init(fun, ofdev->dev.of_node, &io_res);
 	if (ret)
 		goto err2;
-- 
1.8.3.2

[PATCH 22/27] mtd: nand: fsl_ifc: show device structure in sysfs

[Alexander Holler]

Fix a common error in nand-drivers which do not set up a parent device for
the mtd-device by using a new inline function.

Signed-off-by: Alexander Holler <holler@ahsoftware.de>
---
 drivers/mtd/nand/fsl_ifc_nand.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/mtd/nand/fsl_ifc_nand.c b/drivers/mtd/nand/fsl_ifc_nand.c
index cb45d2f..c1aaccb 100644
--- a/drivers/mtd/nand/fsl_ifc_nand.c
+++ b/drivers/mtd/nand/fsl_ifc_nand.c
@@ -874,10 +874,6 @@ static int fsl_ifc_chip_init(struct fsl_ifc_mtd *priv)
 	struct nand_ecclayout *layout;
 	u32 csor, ver;
 
-	/* Fill in fsl_ifc_mtd structure */
-	priv->mtd.priv = chip;
-	priv->mtd.owner = THIS_MODULE;
-
 	/* fill in nand_chip structure */
 	/* set up function call table */
 	if ((ioread32be(&ifc->cspr_cs[priv->bank].cspr)) & CSPR_PORT_SIZE_16)
@@ -1099,6 +1095,7 @@ static int fsl_ifc_nand_probe(struct platform_device *dev)
 		    IFC_NAND_EVTER_INTR_FTOERIR_EN |
 		    IFC_NAND_EVTER_INTR_WPERIR_EN,
 		    &ifc->ifc_nand.nand_evter_intr_en);
+	mtd_setup_common_members(&priv->mtd, &priv->chip, dev);
 	priv->mtd.name = kasprintf(GFP_KERNEL, "%llx.flash", (u64)res.start);
 	if (!priv->mtd.name) {
 		ret = -ENOMEM;
-- 
1.8.3.2

[PATCH 23/27] mtd: nand: jz4740: show device structure in sysfs

[Alexander Holler]

Fix a common error in nand-drivers which do not set up a parent device for
the mtd-device by using a new inline function.

Signed-off-by: Alexander Holler <holler@ahsoftware.de>
---
 drivers/mtd/nand/jz4740_nand.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/mtd/nand/jz4740_nand.c b/drivers/mtd/nand/jz4740_nand.c
index a2c804d..a30a8e5 100644
--- a/drivers/mtd/nand/jz4740_nand.c
+++ b/drivers/mtd/nand/jz4740_nand.c
@@ -435,9 +435,7 @@ static int jz_nand_probe(struct platform_device *pdev)
 
 	mtd		= &nand->mtd;
 	chip		= &nand->chip;
-	mtd->priv	= chip;
-	mtd->owner	= THIS_MODULE;
-	mtd->name	= "jz4740-nand";
+	mtd_setup_common_members(mtd, chip, pdev);
 
 	chip->ecc.hwctl		= jz_nand_hwctl;
 	chip->ecc.calculate	= jz_nand_calculate_ecc_rs;
-- 
1.8.3.2

[PATCH 24/27] mtd: nand: mpc5121_nfc: show device structure in sysfs

[Alexander Holler]

Fix a common error in nand-drivers which do not set up a parent device for
the mtd-device by using a new inline function.

Signed-off-by: Alexander Holler <holler@ahsoftware.de>
---
 drivers/mtd/nand/mpc5121_nfc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mtd/nand/mpc5121_nfc.c b/drivers/mtd/nand/mpc5121_nfc.c
index e78841a..3dd450c 100644
--- a/drivers/mtd/nand/mpc5121_nfc.c
+++ b/drivers/mtd/nand/mpc5121_nfc.c
@@ -657,8 +657,8 @@ static int mpc5121_nfc_probe(struct platform_device *op)
 
 	mtd = &prv->mtd;
 	chip = &prv->chip;
+	mtd_setup_common_members(mtd, chip, op);
 
-	mtd->priv = chip;
 	chip->priv = prv;
 	prv->dev = dev;
 
-- 
1.8.3.2

[PATCH 25/27] mtd: nand: ndfc: show device structure in sysfs

[Alexander Holler]

Fix a common error in nand-drivers which do not set up a parent device for
the mtd-device by using a new inline function.

Signed-off-by: Alexander Holler <holler@ahsoftware.de>
---
 drivers/mtd/nand/ndfc.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/mtd/nand/ndfc.c b/drivers/mtd/nand/ndfc.c
index 69eaba6..66366e0 100644
--- a/drivers/mtd/nand/ndfc.c
+++ b/drivers/mtd/nand/ndfc.c
@@ -170,9 +170,6 @@ static int ndfc_chip_init(struct ndfc_controller *ndfc,
 	chip->ecc.strength = 1;
 	chip->priv = ndfc;
 
-	ndfc->mtd.priv = chip;
-	ndfc->mtd.owner = THIS_MODULE;
-
 	flash_np = of_get_next_child(node, NULL);
 	if (!flash_np)
 		return -ENODEV;
@@ -248,6 +245,7 @@ static int ndfc_probe(struct platform_device *ofdev)
 		out_be32(ndfc->ndfcbase + offset, be32_to_cpup(reg));
 	}
 
+	mtd_setup_common_members(&ndfc->mtd, &ndfc->chip, ofdev);
 	err = ndfc_chip_init(ndfc, ofdev->dev.of_node);
 	if (err) {
 		iounmap(ndfc->ndfcbase);
-- 
1.8.3.2

[PATCH 26/27] mtd: nand: txx9ndfmc: show device structure in sysfs

[Alexander Holler]

Fix a common error in nand-drivers which do not set up a parent device for
the mtd-device by using a new inline function.

Signed-off-by: Alexander Holler <holler@ahsoftware.de>
---
 drivers/mtd/nand/txx9ndfmc.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/mtd/nand/txx9ndfmc.c b/drivers/mtd/nand/txx9ndfmc.c
index c1622a5..88b8d34 100644
--- a/drivers/mtd/nand/txx9ndfmc.c
+++ b/drivers/mtd/nand/txx9ndfmc.c
@@ -323,9 +323,7 @@ static int __init txx9ndfmc_probe(struct platform_device *dev)
 			continue;
 		chip = &txx9_priv->chip;
 		mtd = &txx9_priv->mtd;
-		mtd->owner = THIS_MODULE;
-
-		mtd->priv = chip;
+		mtd_setup_common_members(mtd, chip, dev);
 
 		chip->read_byte = txx9ndfmc_read_byte;
 		chip->read_buf = txx9ndfmc_read_buf;
-- 
1.8.3.2

[PATCH 27/27] mtd: nand: socrates: use mtd_setup_common_members()

[Alexander Holler]

Use the new common function mtd_setup_common_members().

Signed-off-by: Alexander Holler <holler@ahsoftware.de>
---
 drivers/mtd/nand/socrates_nand.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/mtd/nand/socrates_nand.c b/drivers/mtd/nand/socrates_nand.c
index fe8058a..0b87be2 100644
--- a/drivers/mtd/nand/socrates_nand.c
+++ b/drivers/mtd/nand/socrates_nand.c
@@ -165,10 +165,7 @@ static int socrates_nand_probe(struct platform_device *ofdev)
 	host->dev = &ofdev->dev;
 
 	nand_chip->priv = host;		/* link the private data structures */
-	mtd->priv = nand_chip;
-	mtd->name = "socrates_nand";
-	mtd->owner = THIS_MODULE;
-	mtd->dev.parent = &ofdev->dev;
+	mtd_setup_common_members(mtd, nand_chip, ofdev);
 	ppdata.of_node = ofdev->dev.of_node;
 
 	/*should never be accessed directly */
-- 
1.8.3.2

[PATCH 07/27 v2] mtd: nand: gpmi: show device structure in sysfs

[Alexander Holler]

Fix a common error in nand-drivers which do not set up a parent device for
the mtd-device by using a new inline function.

Signed-off-by: Alexander Holler <holler@ahsoftware.de>
---
 Changes to v1: add owner to module
 drivers/mtd/nand/gpmi-nand/gpmi-nand.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
index bb77f75..8ca225c 100644
--- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
+++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
@@ -1682,11 +1682,6 @@ static int gpmi_nand_init(struct gpmi_nand_data *this)
 	/* init current chip */
 	this->current_chip	= -1;
 
-	/* init the MTD data structures */
-	mtd->priv		= chip;
-	mtd->name		= "gpmi-nand";
-	mtd->owner		= THIS_MODULE;
-
 	/* init the nand_chip{}, we don't support a 16-bit NAND Flash bus. */
 	chip->priv		= this;
 	chip->select_chip	= gpmi_select_chip;
@@ -1791,6 +1786,7 @@ static int gpmi_nand_probe(struct platform_device *pdev)
 	if (ret)
 		goto exit_nfc_init;
 
+	mtd_setup_common_members(&this->mtd, &this->nand, pdev);
 	ret = gpmi_nand_init(this);
 	if (ret)
 		goto exit_nfc_init;
@@ -1819,6 +1815,7 @@ static int gpmi_nand_remove(struct platform_device *pdev)
 static struct platform_driver gpmi_nand_driver = {
 	.driver = {
 		.name = "gpmi-nand",
+		.owner = THIS_MODULE,
 		.of_match_table = gpmi_nand_id_table,
 	},
 	.probe   = gpmi_nand_probe,
-- 
1.8.3.2

[PATCH 09/27 v2] mtd: nand: pxa3xx: show device structure in sysfs

[Alexander Holler]

Fix a common error in nand-drivers which do not set up a parent device for
the mtd-device by using a new inline function.

Signed-off-by: Alexander Holler <holler@ahsoftware.de>
---
 Changes to v1: add owner to module

 drivers/mtd/nand/pxa3xx_nand.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c
index 7588fe2..9d4c914 100644
--- a/drivers/mtd/nand/pxa3xx_nand.c
+++ b/drivers/mtd/nand/pxa3xx_nand.c
@@ -1584,8 +1584,7 @@ static int alloc_nand_resource(struct platform_device *pdev)
 		host->mtd = mtd;
 		host->cs = cs;
 		host->info_data = info;
-		mtd->priv = host;
-		mtd->owner = THIS_MODULE;
+		mtd_setup_common_members(mtd, host, pdev);
 
 		chip->ecc.read_page	= pxa3xx_nand_read_page_hwecc;
 		chip->ecc.write_page	= pxa3xx_nand_write_page_hwecc;
@@ -1862,6 +1861,7 @@ static int pxa3xx_nand_resume(struct platform_device *pdev)
 static struct platform_driver pxa3xx_nand_driver = {
 	.driver = {
 		.name	= "pxa3xx-nand",
+		.owner	= THIS_MODULE,
 		.of_match_table = pxa3xx_nand_dt_ids,
 	},
 	.probe		= pxa3xx_nand_probe,
-- 
1.8.3.2

Re: [PATCH 00/27] Fix common bug in most nand drivers not showing a device in sysfs

[Alexander Holler]

Hello,

I wonder if anyone else will fix this in a maintainer-approved style 
which doesn't use these evil leftovers from C666 called functions.

Or will the sysfs for most NAND drivers be knowingly broken forever?

Regards,

Alexander Holler

Am 27.05.2014 00:12, schrieb Alexander Holler:
> A comment in mtdcore.c function add_mtd_device() which is called by
> mtd_device_parse_register() made me wonder:
>
> "Caller should have set dev.parent to match the physical device."
>
> In fact this is not done by most nand drivers.
>
> What follows is a series which fixes this.
>
> Tested: orion and omap2
> Compile-Tested: atmel, gpio, fsmc, gpmi, plat, pxa3xx, s3c2410, sh_flctl,
> 		sharpsl, tmio, docg4, davinci, lpc32xx_mlc, lpc32xx_slc, mxc
> Not tested at all (only be view, patches 19-27): bcm47, fsl_elbc, fsl_upm,
> 		fsl_ifc, jz4740, mpc5121m, ndfc, txx9ndfmx, socrates
>
> The overall stat is
>
> 27 files changed, 36 insertions(+), 80 deletions(-)
>
> and it fixes 21 of these bugs.
>
> Regards,
>
> Alexander Holler
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>

Re: [PATCH 00/27] Fix common bug in most nand drivers not showing a device in sysfs

[Alexander Holler]

Am 16.10.2014 08:37, schrieb Alexander Holler:
> Hello,
>
> I wonder if anyone else will fix this in a maintainer-approved style
> which doesn't use these evil leftovers from C666 called functions.
>
> Or will the sysfs for most NAND drivers be knowingly broken forever?


To explain that a bit more:

1. The reason I had to write 27 patches instead of just one is that such 
a function didn't exist. Therefor I've implemented one.

2. I was kind to write 27 patches to fix that silly bug.

3. I'm no fool and I will NOT write n * 21 patches in order to brutforce 
some maintainers prefered style.



>
> Regards,
>
> Alexander Holler
>
> Am 27.05.2014 00:12, schrieb Alexander Holler:
>> A comment in mtdcore.c function add_mtd_device() which is called by
>> mtd_device_parse_register() made me wonder:
>>
>> "Caller should have set dev.parent to match the physical device."
>>
>> In fact this is not done by most nand drivers.
>>
>> What follows is a series which fixes this.
>>
>> Tested: orion and omap2
>> Compile-Tested: atmel, gpio, fsmc, gpmi, plat, pxa3xx, s3c2410, sh_flctl,
>>         sharpsl, tmio, docg4, davinci, lpc32xx_mlc, lpc32xx_slc, mxc
>> Not tested at all (only be view, patches 19-27): bcm47, fsl_elbc,
>> fsl_upm,
>>         fsl_ifc, jz4740, mpc5121m, ndfc, txx9ndfmx, socrates
>>
>> The overall stat is
>>
>> 27 files changed, 36 insertions(+), 80 deletions(-)
>>
>> and it fixes 21 of these bugs.
>>
>> Regards,
>>
>> Alexander Holler
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe
>> linux-kernel" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at  http://www.tux.org/lkml/
>>
>

Re: [PATCH 00/27] Fix common bug in most nand drivers not showing a device in sysfs

[Frans Klaver]

On Thu, Oct 16, 2014 at 8:37 AM, Alexander Holler <holler@ahsoftware.de> wrote:
>
> I wonder if anyone else will fix this in a maintainer-approved style which
> doesn't use these evil leftovers from C666 called functions.

I'll pick this up then.

Still interested in following progress?

Frans

Re: [PATCH 00/27] Fix common bug in most nand drivers not showing a device in sysfs

[Alexander Holler]

Am 17.10.2014 um 12:53 schrieb Frans Klaver:
> On Thu, Oct 16, 2014 at 8:37 AM, Alexander Holler <holler@ahsoftware.de> wrote:
>>
>> I wonder if anyone else will fix this in a maintainer-approved style which
>> doesn't use these evil leftovers from C666 called functions.
>
> I'll pick this up then.

Thanks.

>
> Still interested in following progress?

As you wish. It's such a simple patch, that I think every word about it
is almost a waste of time. I assume the result will be just 21
one-line-patches anyway, missing the chance to avoid similiar silly
patch series in the future by creating a common function (which even
reduces code size over all).

I've originally just did it because I've assumed any maintainer would
just wave it through not seeing a chance for big discussions.

So, if you like, just add my Signed-off-by too, there isn't really much
one could do wrong. ;)

Thanks again,

Alexander Holler

Re: [PATCH 00/27] Fix common bug in most nand drivers not showing a device in sysfs

[Frans Klaver]

On Fri, Oct 17, 2014 at 05:54:03PM +0200, Alexander Holler wrote:
> I assume the result will be just 21 one-line-patches anyway, missing
> the chance to avoid similiar silly patch series in the future by
> creating a common function (which even reduces code size over all).

That's to be expected, yes. A single patch may even be defensible. I
share your concern that it's easier to get it wrong than it is to get it
right, though.

> So, if you like, just add my Signed-off-by too, there isn't really much
> one could do wrong. ;)

Sure.

Frans