lkml.org 
[lkml]   [2013]   [Feb]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRE: [PATCH 2/3] mmc: davinci_mmc: add DT support
Date
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

[...]



\
 
 \ /
  Last update: 2013-02-05 11:41    [W:1.265 / U:0.012 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site