| Message ID | 20171009132641.27169-4-srinivas.kandagatla@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | nvmem: patches set-1 for v4.15 | expand |
On Mon, Oct 09, 2017 at 03:26:32PM +0200, srinivas.kandagatla@linaro.org wrote: > From: Masahiro Yamada <yamada.masahiro@socionext.com> > > Fix the following sparse warnings: > > drivers/nvmem/mtk-efuse.c:24:30: warning: incorrect type in initializer (different address spaces) > drivers/nvmem/mtk-efuse.c:24:30: expected void [noderef] <asn:2>*base > drivers/nvmem/mtk-efuse.c:24:30: got void *context > drivers/nvmem/mtk-efuse.c:37:30: warning: incorrect type in initializer (different address spaces) > drivers/nvmem/mtk-efuse.c:37:30: expected void [noderef] <asn:2>*base > drivers/nvmem/mtk-efuse.c:37:30: got void *context > drivers/nvmem/mtk-efuse.c:69:23: warning: incorrect type in assignment (different address spaces) > drivers/nvmem/mtk-efuse.c:69:23: expected void *priv > drivers/nvmem/mtk-efuse.c:69:23: got void [noderef] <asn:2>*[assigned] base > > The type of nvmem_config->priv is (void *), so sparse complains > about assignment of the base address with (void __iomem *) type. > > Even if we cast it out, sparse still warns: > warning: cast removes address space of expression > > Of course, we can shut up the sparse by marking __force, but a more > correct way is to put the base address into driver private data. Ick, no, really? That's horrid. Do the __force cast as that is what you are really doing here, and it is good to see the bad use of it (hint, perhaps the api needs to be fixed up so you do not have to do that???) > > Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com> > Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> > --- > drivers/nvmem/mtk-efuse.c | 26 +++++++++++++++++--------- > 1 file changed, 17 insertions(+), 9 deletions(-) > > diff --git a/drivers/nvmem/mtk-efuse.c b/drivers/nvmem/mtk-efuse.c > index fa7a0f66b37e..c4058b598703 100644 > --- a/drivers/nvmem/mtk-efuse.c > +++ b/drivers/nvmem/mtk-efuse.c > @@ -18,15 +18,19 @@ > #include <linux/nvmem-provider.h> > #include <linux/platform_device.h> > > +struct mtk_efuse_priv { > + void __iomem *base; > +}; > + > static int mtk_reg_read(void *context, Make the read call take a __iomem *, not a random void *. That should solve this issue, right? thanks, greg k-h
2017-10-20 22:34 GMT+09:00 Greg KH <gregkh@linuxfoundation.org>: > On Mon, Oct 09, 2017 at 03:26:32PM +0200, srinivas.kandagatla@linaro.org wrote: >> From: Masahiro Yamada <yamada.masahiro@socionext.com> >> >> Fix the following sparse warnings: >> >> drivers/nvmem/mtk-efuse.c:24:30: warning: incorrect type in initializer (different address spaces) >> drivers/nvmem/mtk-efuse.c:24:30: expected void [noderef] <asn:2>*base >> drivers/nvmem/mtk-efuse.c:24:30: got void *context >> drivers/nvmem/mtk-efuse.c:37:30: warning: incorrect type in initializer (different address spaces) >> drivers/nvmem/mtk-efuse.c:37:30: expected void [noderef] <asn:2>*base >> drivers/nvmem/mtk-efuse.c:37:30: got void *context >> drivers/nvmem/mtk-efuse.c:69:23: warning: incorrect type in assignment (different address spaces) >> drivers/nvmem/mtk-efuse.c:69:23: expected void *priv >> drivers/nvmem/mtk-efuse.c:69:23: got void [noderef] <asn:2>*[assigned] base >> >> The type of nvmem_config->priv is (void *), so sparse complains >> about assignment of the base address with (void __iomem *) type. >> >> Even if we cast it out, sparse still warns: >> warning: cast removes address space of expression >> >> Of course, we can shut up the sparse by marking __force, but a more >> correct way is to put the base address into driver private data. > > Ick, no, really? > > That's horrid. Do the __force cast as that is what you are really doing > here, and it is good to see the bad use of it (hint, perhaps the api > needs to be fixed up so you do not have to do that???) Other drivers take private data from the "void *context". In this driver, the priv just happens to have one member. What's the matter? >> diff --git a/drivers/nvmem/mtk-efuse.c b/drivers/nvmem/mtk-efuse.c >> index fa7a0f66b37e..c4058b598703 100644 >> --- a/drivers/nvmem/mtk-efuse.c >> +++ b/drivers/nvmem/mtk-efuse.c >> @@ -18,15 +18,19 @@ >> #include <linux/nvmem-provider.h> >> #include <linux/platform_device.h> >> >> +struct mtk_efuse_priv { >> + void __iomem *base; >> +}; >> + >> static int mtk_reg_read(void *context, > > > Make the read call take a __iomem *, not a random void *. That should > solve this issue, right? > NACK. See drivers/nvmem/imx-iim.c The ->reg_read hook needs more private data than __iomem *. -- Best Regards Masahiro Yamada
On Fri, Oct 20, 2017 at 10:55:04PM +0900, Masahiro Yamada wrote: > 2017-10-20 22:34 GMT+09:00 Greg KH <gregkh@linuxfoundation.org>: > > On Mon, Oct 09, 2017 at 03:26:32PM +0200, srinivas.kandagatla@linaro.org wrote: > >> From: Masahiro Yamada <yamada.masahiro@socionext.com> > >> > >> Fix the following sparse warnings: > >> > >> drivers/nvmem/mtk-efuse.c:24:30: warning: incorrect type in initializer (different address spaces) > >> drivers/nvmem/mtk-efuse.c:24:30: expected void [noderef] <asn:2>*base > >> drivers/nvmem/mtk-efuse.c:24:30: got void *context > >> drivers/nvmem/mtk-efuse.c:37:30: warning: incorrect type in initializer (different address spaces) > >> drivers/nvmem/mtk-efuse.c:37:30: expected void [noderef] <asn:2>*base > >> drivers/nvmem/mtk-efuse.c:37:30: got void *context > >> drivers/nvmem/mtk-efuse.c:69:23: warning: incorrect type in assignment (different address spaces) > >> drivers/nvmem/mtk-efuse.c:69:23: expected void *priv > >> drivers/nvmem/mtk-efuse.c:69:23: got void [noderef] <asn:2>*[assigned] base > >> > >> The type of nvmem_config->priv is (void *), so sparse complains > >> about assignment of the base address with (void __iomem *) type. > >> > >> Even if we cast it out, sparse still warns: > >> warning: cast removes address space of expression > >> > >> Of course, we can shut up the sparse by marking __force, but a more > >> correct way is to put the base address into driver private data. > > > > Ick, no, really? > > > > That's horrid. Do the __force cast as that is what you are really doing > > here, and it is good to see the bad use of it (hint, perhaps the api > > needs to be fixed up so you do not have to do that???) > > > Other drivers take private data from the "void *context". > > In this driver, the priv just happens to have one member. > What's the matter? > > > > > >> diff --git a/drivers/nvmem/mtk-efuse.c b/drivers/nvmem/mtk-efuse.c > >> index fa7a0f66b37e..c4058b598703 100644 > >> --- a/drivers/nvmem/mtk-efuse.c > >> +++ b/drivers/nvmem/mtk-efuse.c > >> @@ -18,15 +18,19 @@ > >> #include <linux/nvmem-provider.h> > >> #include <linux/platform_device.h> > >> > >> +struct mtk_efuse_priv { > >> + void __iomem *base; > >> +}; > >> + > >> static int mtk_reg_read(void *context, > > > > > > Make the read call take a __iomem *, not a random void *. That should > > solve this issue, right? > > > > NACK. > > See drivers/nvmem/imx-iim.c > > The ->reg_read hook needs more private data than __iomem *. Ok, that makes a bit more sense, but it still feels a bit of a hack, don't you agree? thanks, greg k-h
2017-10-20 23:49 GMT+09:00 Greg KH <gregkh@linuxfoundation.org>: > On Fri, Oct 20, 2017 at 10:55:04PM +0900, Masahiro Yamada wrote: >> 2017-10-20 22:34 GMT+09:00 Greg KH <gregkh@linuxfoundation.org>: >> > On Mon, Oct 09, 2017 at 03:26:32PM +0200, srinivas.kandagatla@linaro.org wrote: >> >> From: Masahiro Yamada <yamada.masahiro@socionext.com> >> >> >> >> Fix the following sparse warnings: >> >> >> >> drivers/nvmem/mtk-efuse.c:24:30: warning: incorrect type in initializer (different address spaces) >> >> drivers/nvmem/mtk-efuse.c:24:30: expected void [noderef] <asn:2>*base >> >> drivers/nvmem/mtk-efuse.c:24:30: got void *context >> >> drivers/nvmem/mtk-efuse.c:37:30: warning: incorrect type in initializer (different address spaces) >> >> drivers/nvmem/mtk-efuse.c:37:30: expected void [noderef] <asn:2>*base >> >> drivers/nvmem/mtk-efuse.c:37:30: got void *context >> >> drivers/nvmem/mtk-efuse.c:69:23: warning: incorrect type in assignment (different address spaces) >> >> drivers/nvmem/mtk-efuse.c:69:23: expected void *priv >> >> drivers/nvmem/mtk-efuse.c:69:23: got void [noderef] <asn:2>*[assigned] base >> >> >> >> The type of nvmem_config->priv is (void *), so sparse complains >> >> about assignment of the base address with (void __iomem *) type. >> >> >> >> Even if we cast it out, sparse still warns: >> >> warning: cast removes address space of expression >> >> >> >> Of course, we can shut up the sparse by marking __force, but a more >> >> correct way is to put the base address into driver private data. >> > >> > Ick, no, really? >> > >> > That's horrid. Do the __force cast as that is what you are really doing >> > here, and it is good to see the bad use of it (hint, perhaps the api >> > needs to be fixed up so you do not have to do that???) >> >> >> Other drivers take private data from the "void *context". >> >> In this driver, the priv just happens to have one member. >> What's the matter? >> >> >> >> >> >> diff --git a/drivers/nvmem/mtk-efuse.c b/drivers/nvmem/mtk-efuse.c >> >> index fa7a0f66b37e..c4058b598703 100644 >> >> --- a/drivers/nvmem/mtk-efuse.c >> >> +++ b/drivers/nvmem/mtk-efuse.c >> >> @@ -18,15 +18,19 @@ >> >> #include <linux/nvmem-provider.h> >> >> #include <linux/platform_device.h> >> >> >> >> +struct mtk_efuse_priv { >> >> + void __iomem *base; >> >> +}; >> >> + >> >> static int mtk_reg_read(void *context, >> > >> > >> > Make the read call take a __iomem *, not a random void *. That should >> > solve this issue, right? >> > >> >> NACK. >> >> See drivers/nvmem/imx-iim.c >> >> The ->reg_read hook needs more private data than __iomem *. > > Ok, that makes a bit more sense, but it still feels a bit of a hack, > don't you agree? But, I have no choice but __force. I disagree with using __force to hide things. In my opinion, __force should be used only where it is really legitimate. This driver is not the case, as we can fix it by writing correct code. -- Best Regards Masahiro Yamada
diff --git a/drivers/nvmem/mtk-efuse.c b/drivers/nvmem/mtk-efuse.c index fa7a0f66b37e..c4058b598703 100644 --- a/drivers/nvmem/mtk-efuse.c +++ b/drivers/nvmem/mtk-efuse.c @@ -18,15 +18,19 @@ #include <linux/nvmem-provider.h> #include <linux/platform_device.h> +struct mtk_efuse_priv { + void __iomem *base; +}; + static int mtk_reg_read(void *context, unsigned int reg, void *_val, size_t bytes) { - void __iomem *base = context; + struct mtk_efuse_priv *priv = context; u32 *val = _val; int i = 0, words = bytes / 4; while (words--) - *val++ = readl(base + reg + (i++ * 4)); + *val++ = readl(priv->base + reg + (i++ * 4)); return 0; } @@ -34,12 +38,12 @@ static int mtk_reg_read(void *context, static int mtk_reg_write(void *context, unsigned int reg, void *_val, size_t bytes) { - void __iomem *base = context; + struct mtk_efuse_priv *priv = context; u32 *val = _val; int i = 0, words = bytes / 4; while (words--) - writel(*val++, base + reg + (i++ * 4)); + writel(*val++, priv->base + reg + (i++ * 4)); return 0; } @@ -50,19 +54,23 @@ static int mtk_efuse_probe(struct platform_device *pdev) struct resource *res; struct nvmem_device *nvmem; struct nvmem_config econfig = {}; - void __iomem *base; + struct mtk_efuse_priv *priv; + + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); + if (!priv) + return -ENOMEM; res = platform_get_resource(pdev, IORESOURCE_MEM, 0); - base = devm_ioremap_resource(dev, res); - if (IS_ERR(base)) - return PTR_ERR(base); + priv->base = devm_ioremap_resource(dev, res); + if (IS_ERR(priv->base)) + return PTR_ERR(priv->base); econfig.stride = 4; econfig.word_size = 4; econfig.reg_read = mtk_reg_read; econfig.reg_write = mtk_reg_write; econfig.size = resource_size(res); - econfig.priv = base; + econfig.priv = priv; econfig.dev = dev; econfig.owner = THIS_MODULE; nvmem = nvmem_register(&econfig);