| Message ID | 1378968200-16577-1-git-send-email-sachin.kamat@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 12 September 2013 12:13, Sachin Kamat <sachin.kamat@linaro.org> wrote: > pdata could be NULL if cd_gpio = -1. Hence move the NULL check > outside the if condition. > > Signed-off-by: Sachin Kamat <sachin.kamat@linaro.org> > Cc: Viresh Kumar <viresh.kumar@linaro.org> > --- > Only compile tested. > --- > drivers/mmc/host/sdhci-spear.c | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/drivers/mmc/host/sdhci-spear.c b/drivers/mmc/host/sdhci-spear.c > index 2dba9f8..4f3557a 100644 > --- a/drivers/mmc/host/sdhci-spear.c > +++ b/drivers/mmc/host/sdhci-spear.c > @@ -82,12 +82,12 @@ static struct sdhci_plat_data *sdhci_probe_config_dt(struct platform_device *pde > cd_gpio = -1; > > /* If pdata is required */ > - if (cd_gpio != -1) { > + if (cd_gpio != -1) > pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL); > - if (!pdata) { > - dev_err(&pdev->dev, "DT: kzalloc failed\n"); > - return ERR_PTR(-ENOMEM); > - } > + > + if (!pdata) { > + dev_err(&pdev->dev, "pdata is NULL\n"); > + return ERR_PTR(-ENOMEM); > } > > pdata->card_int_gpio = cd_gpio; Or better move above line in the if block? We are already printing error in probe.. -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12 September 2013 13:19, Viresh Kumar <viresh.kumar@linaro.org> wrote: > On 12 September 2013 12:13, Sachin Kamat <sachin.kamat@linaro.org> wrote: >> pdata could be NULL if cd_gpio = -1. Hence move the NULL check >> outside the if condition. >> >> Signed-off-by: Sachin Kamat <sachin.kamat@linaro.org> >> Cc: Viresh Kumar <viresh.kumar@linaro.org> >> --- >> Only compile tested. >> --- >> drivers/mmc/host/sdhci-spear.c | 10 +++++----- >> 1 file changed, 5 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/mmc/host/sdhci-spear.c b/drivers/mmc/host/sdhci-spear.c >> index 2dba9f8..4f3557a 100644 >> --- a/drivers/mmc/host/sdhci-spear.c >> +++ b/drivers/mmc/host/sdhci-spear.c >> @@ -82,12 +82,12 @@ static struct sdhci_plat_data *sdhci_probe_config_dt(struct platform_device *pde >> cd_gpio = -1; >> >> /* If pdata is required */ >> - if (cd_gpio != -1) { >> + if (cd_gpio != -1) >> pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL); >> - if (!pdata) { >> - dev_err(&pdev->dev, "DT: kzalloc failed\n"); >> - return ERR_PTR(-ENOMEM); >> - } >> + >> + if (!pdata) { >> + dev_err(&pdev->dev, "pdata is NULL\n"); >> + return ERR_PTR(-ENOMEM); >> } >> >> pdata->card_int_gpio = cd_gpio; > > Or better move above line in the if block? We are already printing > error in probe.. Yes. That was my second option. If we do that we would get something as below: 84 /* If pdata is required */ 85 if (cd_gpio != -1) { 86 pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL); 87 if (!pdata) { 88 dev_err(&pdev->dev, "DT: kzalloc failed\n"); 89 goto out; 90 } 91 pdata->card_int_gpio = cd_gpio; 92 return pdata; 93 } 94 95 out: 96 return ERR_PTR(-ENODATA); Does this look OK?
On 12 September 2013 13:41, Sachin Kamat <sachin.kamat@linaro.org> wrote: > Yes. That was my second option. If we do that we would get something as below: > > 84 /* If pdata is required */ > 85 if (cd_gpio != -1) { > 86 pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL); > 87 if (!pdata) { > 88 dev_err(&pdev->dev, "DT: kzalloc failed\n"); > 89 goto out; > 90 } > 91 pdata->card_int_gpio = cd_gpio; > 92 return pdata; > 93 } > 94 > 95 out: > 96 return ERR_PTR(-ENODATA); > > Does this look OK? Or this: > 85 if (cd_gpio != -1) { > 86 pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL); > 87 if (!pdata) > 88 dev_err(&pdev->dev, "DT: kzalloc failed\n"); else > 91 pdata->card_int_gpio = cd_gpio; > 93 } > 94 > 96 return ERR_PTR(-ENODATA); -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12 September 2013 13:43, Viresh Kumar <viresh.kumar@linaro.org> wrote: > On 12 September 2013 13:41, Sachin Kamat <sachin.kamat@linaro.org> wrote: >> Yes. That was my second option. If we do that we would get something as below: >> >> 84 /* If pdata is required */ >> 85 if (cd_gpio != -1) { >> 86 pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL); >> 87 if (!pdata) { >> 88 dev_err(&pdev->dev, "DT: kzalloc failed\n"); >> 89 goto out; >> 90 } >> 91 pdata->card_int_gpio = cd_gpio; >> 92 return pdata; >> 93 } >> 94 >> 95 out: >> 96 return ERR_PTR(-ENODATA); >> >> Does this look OK? > > Or this: > >> 85 if (cd_gpio != -1) { >> 86 pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL); >> 87 if (!pdata) >> 88 dev_err(&pdev->dev, "DT: kzalloc failed\n"); > else >> 91 pdata->card_int_gpio = cd_gpio; >> 93 } >> 94 >> 96 return ERR_PTR(-ENODATA); Wouldn't this be unconditional error return whether pdata is null or not?
On 12 September 2013 13:48, Sachin Kamat <sachin.kamat@linaro.org> wrote: >>> 96 return ERR_PTR(-ENODATA); > > Wouldn't this be unconditional error return whether pdata is null or not? Stupid me... I meant return pdata from this place.. Necessary checks are done in sdhci_probe() -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12 September 2013 13:50, Viresh Kumar <viresh.kumar@linaro.org> wrote: > On 12 September 2013 13:48, Sachin Kamat <sachin.kamat@linaro.org> wrote: >>>> 96 return ERR_PTR(-ENODATA); >> >> Wouldn't this be unconditional error return whether pdata is null or not? > > Stupid me... I meant return pdata from this place.. > > Necessary checks are done in sdhci_probe() sdhci_probe only checks for IS_ERR. We would need to change it to IS_ERR_OR_NULL (which I do not prefer personally as there is some discussion for its removal). In that case we would need to return as I mentioned in my earlier email. Let me know your opinion.
On 12 September 2013 13:52, Sachin Kamat <sachin.kamat@linaro.org> wrote: > sdhci_probe only checks for IS_ERR. We would need to change it to > IS_ERR_OR_NULL (which I do not prefer personally as there is some > discussion for its removal). In that case we would need to return as I > mentioned in my earlier email. Let me know your opinion. The problem with pdata normally is that it can't be NULL. Its not like clk/ regulator framework where NULL's are valid... And that's why we are checking NULL later in the probe().. Probably you can just move that up a bit.. and nothing else.. -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12 September 2013 14:26, Viresh Kumar <viresh.kumar@linaro.org> wrote: > On 12 September 2013 13:52, Sachin Kamat <sachin.kamat@linaro.org> wrote: >> sdhci_probe only checks for IS_ERR. We would need to change it to >> IS_ERR_OR_NULL (which I do not prefer personally as there is some >> discussion for its removal). In that case we would need to return as I >> mentioned in my earlier email. Let me know your opinion. > > The problem with pdata normally is that it can't be NULL. Its not like clk/ > regulator framework where NULL's are valid... > > And that's why we are checking NULL later in the probe().. Probably you can > just move that up a bit.. and nothing else.. Yes you are right. I had not seen that check which is done later in probe. I will leave the probe as it is and modify the sdhci_probe_config_dt as per your suggestion.
diff --git a/drivers/mmc/host/sdhci-spear.c b/drivers/mmc/host/sdhci-spear.c index 2dba9f8..4f3557a 100644 --- a/drivers/mmc/host/sdhci-spear.c +++ b/drivers/mmc/host/sdhci-spear.c @@ -82,12 +82,12 @@ static struct sdhci_plat_data *sdhci_probe_config_dt(struct platform_device *pde cd_gpio = -1; /* If pdata is required */ - if (cd_gpio != -1) { + if (cd_gpio != -1) pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL); - if (!pdata) { - dev_err(&pdev->dev, "DT: kzalloc failed\n"); - return ERR_PTR(-ENOMEM); - } + + if (!pdata) { + dev_err(&pdev->dev, "pdata is NULL\n"); + return ERR_PTR(-ENOMEM); } pdata->card_int_gpio = cd_gpio;
pdata could be NULL if cd_gpio = -1. Hence move the NULL check outside the if condition. Signed-off-by: Sachin Kamat <sachin.kamat@linaro.org> Cc: Viresh Kumar <viresh.kumar@linaro.org> --- Only compile tested. --- drivers/mmc/host/sdhci-spear.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)