| Message ID | 20190826120013.183435-1-weiyongjun1@huawei.com (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | f70d9a2440346d942f1f9dbda31da5a77510a05f |
| Headers | show |
| Series | [-next] mmc: aspeed: Fix return value check in aspeed_sdc_probe() | expand |
> Fixes: 09eed7fffd33 ("mmc: Add support for the ASPEED SD controller")
^^^^
When we're adding new files, could we use the prefix for the new driver
instead of just the subsystem? "mmc: aspeed: Add new driver"?
Otherwise it's tricky to know what people want for the driver.
I just wrote this same patch and I swear I would have sent my patch
earlier but I spent hours thinking about the patch prefix and then the
census people came to the house and delayed me even more.
regards,
dan carpenter
On Mon, 26 Aug 2019, at 22:34, Dan Carpenter wrote: > > Fixes: 09eed7fffd33 ("mmc: Add support for the ASPEED SD controller") > ^^^^ > When we're adding new files, could we use the prefix for the new driver > instead of just the subsystem? "mmc: aspeed: Add new driver"? > Otherwise it's tricky to know what people want for the driver. I don't have any issue with the request, but I don't understand this last bit. What do you mean by "it's tricky to know what people want for the driver"? Andrew
On Mon, 26 Aug 2019, at 21:27, Wei Yongjun wrote: > In case of error, the function of_platform_device_create() returns > NULL pointer not ERR_PTR(). The IS_ERR() test in the return value > check should be replaced with NULL test. > > Fixes: 09eed7fffd33 ("mmc: Add support for the ASPEED SD controller") > Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com> > --- > drivers/mmc/host/sdhci-of-aspeed.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/mmc/host/sdhci-of-aspeed.c > b/drivers/mmc/host/sdhci-of-aspeed.c > index 8bb095ca2fa9..d5acb5afc50f 100644 > --- a/drivers/mmc/host/sdhci-of-aspeed.c > +++ b/drivers/mmc/host/sdhci-of-aspeed.c > @@ -261,9 +261,9 @@ static int aspeed_sdc_probe(struct platform_device > *pdev) > struct platform_device *cpdev; > > cpdev = of_platform_device_create(child, NULL, &pdev->dev); > - if (IS_ERR(cpdev)) { > + if (!cpdev) { > of_node_put(child); > - ret = PTR_ERR(cpdev); > + ret = -ENODEV; > goto err_clk; > } > } I ... have no idea why I wrote it that way. I must have just assumed it returned an ERR_PTR(). Thanks for finding/fixing that. Reviewed-by: Andrew Jeffery <andrew@aj.id.au>
On Tue, 27 Aug 2019, Andrew Jeffery wrote: > > > On Mon, 26 Aug 2019, at 22:34, Dan Carpenter wrote: > > > Fixes: 09eed7fffd33 ("mmc: Add support for the ASPEED SD controller") > > ^^^^ > > When we're adding new files, could we use the prefix for the new driver > > instead of just the subsystem? "mmc: aspeed: Add new driver"? > > Otherwise it's tricky to know what people want for the driver. > > I don't have any issue with the request, but I don't understand this last > bit. What do you mean by "it's tricky to know what people want for the > driver"? There is no obvious algorithm that tells how to go from a file name to an appropriate subject line prefix. julia
On Tue, 27 Aug 2019 at 02:47, Julia Lawall <julia.lawall@lip6.fr> wrote: > > > > On Tue, 27 Aug 2019, Andrew Jeffery wrote: > > > > > > > On Mon, 26 Aug 2019, at 22:34, Dan Carpenter wrote: > > > > Fixes: 09eed7fffd33 ("mmc: Add support for the ASPEED SD controller") > > > ^^^^ > > > When we're adding new files, could we use the prefix for the new driver > > > instead of just the subsystem? "mmc: aspeed: Add new driver"? > > > Otherwise it's tricky to know what people want for the driver. > > > > I don't have any issue with the request, but I don't understand this last > > bit. What do you mean by "it's tricky to know what people want for the > > driver"? > > There is no obvious algorithm that tells how to go from a file name to an > appropriate subject line prefix. For MMC we normally use the name of the host driver file (excluding ".c") as part of the prefix. For this case that means I amended the header into: mmc: sdhci-of-aspeed: Fix return value check in aspeed_sdc_probe() and applied it for next. I also took the liberty to change this for the other related patches for the "aspeed" driver to follow the same pattern. Kind regards Uffe
diff --git a/drivers/mmc/host/sdhci-of-aspeed.c b/drivers/mmc/host/sdhci-of-aspeed.c index 8bb095ca2fa9..d5acb5afc50f 100644 --- a/drivers/mmc/host/sdhci-of-aspeed.c +++ b/drivers/mmc/host/sdhci-of-aspeed.c @@ -261,9 +261,9 @@ static int aspeed_sdc_probe(struct platform_device *pdev) struct platform_device *cpdev; cpdev = of_platform_device_create(child, NULL, &pdev->dev); - if (IS_ERR(cpdev)) { + if (!cpdev) { of_node_put(child); - ret = PTR_ERR(cpdev); + ret = -ENODEV; goto err_clk; } }
In case of error, the function of_platform_device_create() returns NULL pointer not ERR_PTR(). The IS_ERR() test in the return value check should be replaced with NULL test. Fixes: 09eed7fffd33 ("mmc: Add support for the ASPEED SD controller") Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com> --- drivers/mmc/host/sdhci-of-aspeed.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)