[PATCH 4/4] misc: emif: add device tree support to emif driver

[Grant Likely]

On Fri, 09 Mar 2012 18:20:51 +0530, Aneesh V <aneesh at ti.com> wrote:
> Hi Grant,
> 
> On Friday 09 March 2012 11:07 AM, Grant Likely wrote:
> > On Thu,  8 Mar 2012 22:03:57 +0530, Aneesh V<aneesh at ti.com>  wrote:
> >> Cc: Rajendra Nayak<rnayak at ti.com>
> >> Cc: Benoit Cousson<b-cousson at ti.com>
> >> Signed-off-by: Aneesh V<aneesh at ti.com>
> >> ---
> >> Changes since RFC v4:
> >> - Rebased to the latest version of EMIF series
> >> - Replace kzalloc()/kfree() with devm_* variants
> >> ---
> >>   drivers/misc/emif.c |  289 ++++++++++++++++++++++++++++++++++++++++++++++++++-
> >>   1 files changed, 288 insertions(+), 1 deletions(-)
> >>
> >> diff --git a/drivers/misc/emif.c b/drivers/misc/emif.c
> >> index 79fb161..0aaa61e 100644
> >> --- a/drivers/misc/emif.c
> >> +++ b/drivers/misc/emif.c
> >> @@ -18,6 +18,7 @@
> >>   #include<linux/platform_device.h>
> >>   #include<linux/interrupt.h>
> >>   #include<linux/slab.h>
> >> +#include<linux/of.h>
> >>   #include<linux/debugfs.h>
> >>   #include<linux/seq_file.h>
> >>   #include<linux/module.h>
> >> @@ -49,6 +50,7 @@
> >>    *				frequency in effect at the moment)
> >>    * @plat_data:			Pointer to saved platform data.
> >>    * @debugfs_root:		dentry to the root folder for EMIF in debugfs
> >> + * @np_ddr:			Pointer to ddr device tree node
> >>    */
> >>   struct emif_data {
> >>   	u8				duplicate;
> >> @@ -63,6 +65,9 @@ struct emif_data {
> >>   	struct emif_regs		*curr_regs;
> >>   	struct emif_platform_data	*plat_data;
> >>   	struct dentry			*debugfs_root;
> >> +#if defined(CONFIG_OF)
> >> +	struct device_node		*np_ddr;
> >> +#endif
> >
> > Don't bother with the #if/#endif wrapper here.
> 
> Ok.
> 
> >
> >>   };
> >>
> >>   static struct emif_data *emif1;
> >> @@ -1147,6 +1152,270 @@ static int is_custom_config_valid(struct emif_custom_configs *cust_cfgs,
> >>   	return valid;
> >>   }
> >>
> >> +#if defined(CONFIG_OF)
> >> +static void __init of_get_custom_configs(struct device_node *np_emif,
> >
> > __devinit. Same through the rest of the file.
> 
> I am not sure if we need __devinit. I see that __devinit will not have
> any effect if CONFIG_HOTPLUG is enabled and CONFIG_HOTPLUG is always
> enabled in our configuration. EMIF devices are not hot-pluggable
> devices and are always added at boot-time from the board file. They are
> on-chip IP modules and dynamic discovery and addition doesn't make
> sense for them. So, can't we save some memory by making them __init.
> 
> AFAIU, __init doesn't have any effect in a module. However, I can make
> that more explicit by using '__init_or_module'. Is that ok?
> 
> >
> > [...]
> >> +	return NULL;
> >> +out:
> >> +	return emif;
> >> +}
> >> +#endif
> >> +
> >>   static struct emif_data * __init get_device_details(
> >
> > This function also must be __devinit
> >
> >>   		struct platform_device *pdev)
> >>   {
> >> @@ -1266,7 +1535,13 @@ static int __init emif_probe(struct platform_device *pdev)
> >>   	struct resource		*res;
> >>   	int			irq;
> >>
> >> -	emif = get_device_details(pdev);
> >> +#if defined(CONFIG_OF)
> >> +	if (pdev->dev.of_node)
> >> +		emif = of_get_device_details(pdev->dev.of_node,&pdev->dev);
> >> +	else
> >> +#endif
> >> +		emif = get_device_details(pdev);
> >> +
> >>   	if (!emif) {
> >>   		pr_err("%s: error getting device data\n", __func__);
> >>   		goto error;
> >> @@ -1643,11 +1918,23 @@ static void __attribute__((unused)) freq_post_notify_handling(void)
> >>   	spin_unlock_irqrestore(&emif_lock, irq_state);
> >>   }
> >>
> >> +#if defined(CONFIG_OF)
> >> +static const struct of_device_id emif_of_match[] = {
> >> +		{ .compatible = "ti,emif-4d" },
> >> +		{ .compatible = "ti,emif-4d5" },
> >> +		{},
> >> +};
> >> +MODULE_DEVICE_TABLE(of, emif_of_match);
> >> +#endif
> >> +
> >>   static struct platform_driver emif_driver = {
> >>   	.remove		= __exit_p(emif_remove),
> >
> > Not part of this patch I realize, but this is a bug.  .remove must
> > be in the __devexit section and the __devexit_p() wrapper must
> > be used.  Similarly, emif_probe must be in the __devinit section.
> 
> Again, in our case remove() is not needed unless the driver is built as
> a module because the devices will never be un-registered. When it is
> built as a module the effect is same for both:

It's just outside the pattern expected for device drivers.  I like to
be careful about things like this.  That's why I recommend __devinit
also, but I see now that the probe hook isn't added to the
platform_device so it isn't required.

> 
> #if defined(MODULE) || defined(CONFIG_HOTPLUG)
> #define __devexit_p(x) x
> #else
> #define __devexit_p(x) NULL
> #endif
> 
> #ifdef MODULE
> #define __exit_p(x) x
> #else
> #define __exit_p(x) NULL
> #endif

Don't go with what is written in the code here; go with what the
meaning of the sections are.  The code can be changed, but the drivers
are expected to honor what the sections mean.

__init can be freed after module init
__devinit in reality never gets freed because devices can show up later
__devexit can be freed if hotplug is not enabled
__exit can be freed after module unload

g.