[PATCH 2/3] mmc: davinci_mmc: add DT support

[Manjunathappa, Prakash]

Hi Mark,

On Mon, Feb 04, 2013 at 19:36:16, Mark Rutland wrote:
> Hi,
> 
> On Mon, Feb 04, 2013 at 01:28:14PM +0000, Manjunathappa, Prakash wrote:
> > Hi Mark,
> > 
> > On Thu, Jan 31, 2013 at 16:53:03, Mark Rutland wrote:
> > > Hello,
> > >
> > > I have a few comments on the devicetree binding and the way it's parsed.
> > >
> > 
> > Thanks for review.
> > 
> > > On Thu, Jan 31, 2013 at 10:33:06AM +0000, Manjunathappa, Prakash wrote:
> 
> [...]
[...]
> [...]
> 
> > > > @@ -1156,16 +1157,75 @@ static void __init init_mmcsd_host(struct mmc_davinci_host *host)
> > > >
> > > >     mmc_davinci_reset_ctrl(host, 0);
> > > >  }
> > > > +#ifdef CONFIG_OF
> > > > +static struct davinci_mmc_config
> > > > +   *mmc_of_get_pdata(struct platform_device *pdev)
> > > > +{
> > > > +   struct device_node *np;
> > > > +   struct davinci_mmc_config *pdata = NULL;
> > > > +   u32 data;
> > > > +   int ret;
> > > > +
> > > > +   pdata = pdev->dev.platform_data;
> > > > +   if (!pdata) {
> > > > +           pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
> > > > +           if (!pdata) {
> > > > +                   dev_err(&pdev->dev, "Failed to allocate memory for struct davinci_mmc_config\n");
> > > > +                   goto nodata;
> > > > +           }
> > > > +   }
> > >
> > > Why do you need to conditionally allocate this? This only seems to be called
> > > once.
> > >
> > 
> > This is common function for DT and non-DT kernel(will be removing #ifdef CONFIG_OF),
> > So above check is necessary for to allocate pdata for DT kernel.
> 
> Ah. Am I right in thinking if you moved the check for pdev->dev.of_node above
> the pdata allocation, it wouldn't have to be done conditionally?
> 

Agreed. Will move below check up.

> > 
> > > > +
> > > > +   np = pdev->dev.of_node;
> > > > +   if (!np)
> > > > +           goto nodata;
> > >
> > > Why not just return immediately here? You do nothing special at nodata.
> > >
> > 
> > Following convention to not have more than 1 return from function and have
> > Common exit point. May not be necessary now since we have devm_* calls now.
> > Can I still prefer to keep this goto?
> 
> It just looks a little odd to me. I have no strong feelings here.
> 
> [...]
> 

After considering your inputs on moving above statement up, "return" makes sense.

Thanks,
Prakash

[...]