Hi All,
This patch series fixes trivial issues in RPC-IF driver.
Cheers,
Prabhakar
Lad Prabhakar (5):
memory: renesas-rpc-if: Return correct value to the caller of
rpcif_manual_xfer()
memory: renesas-rpc-if: Make rpcif_enable/disable_rpm() as static
inline
memory: renesas-rpc-if: Export symbols as GPL
memory: renesas-rpc-if: Avoid use of C++ style comments
memory: renesas-rpc-if: Fix a reference leak in rpcif_probe()
drivers/memory/renesas-rpc-if.c | 28 +++++++++-------------------
include/memory/renesas-rpc-if.h | 19 ++++++++++++++-----
2 files changed, 23 insertions(+), 24 deletions(-)
--
2.17.1
In the error path of rpcif_manual_xfer() the value of ret is overwritten
by value returned by reset_control_reset() function and thus returning
incorrect value to the caller.
This patch makes sure the correct value is returned to the caller of
rpcif_manual_xfer() by dropping the overwrite of ret in error path.
Also now we ignore the value returned by reset_control_reset() in the
error path and instead print a error message when it fails.
Fixes: ca7d8b980b67f ("memory: add Renesas RPC-IF driver")
Reported-by: Pavel Machek <[email protected]>
Signed-off-by: Lad Prabhakar <[email protected]>
Cc: [email protected]
---
drivers/memory/renesas-rpc-if.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/memory/renesas-rpc-if.c b/drivers/memory/renesas-rpc-if.c
index f2a33a1af836..69f2e2b4cd50 100644
--- a/drivers/memory/renesas-rpc-if.c
+++ b/drivers/memory/renesas-rpc-if.c
@@ -508,7 +508,8 @@ int rpcif_manual_xfer(struct rpcif *rpc)
return ret;
err_out:
- ret = reset_control_reset(rpc->rstc);
+ if (reset_control_reset(rpc->rstc))
+ dev_err(rpc->dev, "Failed to reset HW\n");
rpcif_hw_init(rpc, rpc->bus_size == 2);
goto exit;
}
--
2.17.1
Define rpcif_enable_rpm() and rpcif_disable_rpm() as static
inline in the header instead of exporting it.
Suggested-by: Pavel Machek <[email protected]>
Signed-off-by: Lad Prabhakar <[email protected]>
---
drivers/memory/renesas-rpc-if.c | 13 -------------
include/memory/renesas-rpc-if.h | 13 +++++++++++--
2 files changed, 11 insertions(+), 15 deletions(-)
diff --git a/drivers/memory/renesas-rpc-if.c b/drivers/memory/renesas-rpc-if.c
index 69f2e2b4cd50..c5b5691503d7 100644
--- a/drivers/memory/renesas-rpc-if.c
+++ b/drivers/memory/renesas-rpc-if.c
@@ -12,7 +12,6 @@
#include <linux/module.h>
#include <linux/platform_device.h>
#include <linux/of.h>
-#include <linux/pm_runtime.h>
#include <linux/regmap.h>
#include <linux/reset.h>
@@ -204,18 +203,6 @@ int rpcif_sw_init(struct rpcif *rpc, struct device *dev)
}
EXPORT_SYMBOL(rpcif_sw_init);
-void rpcif_enable_rpm(struct rpcif *rpc)
-{
- pm_runtime_enable(rpc->dev);
-}
-EXPORT_SYMBOL(rpcif_enable_rpm);
-
-void rpcif_disable_rpm(struct rpcif *rpc)
-{
- pm_runtime_put_sync(rpc->dev);
-}
-EXPORT_SYMBOL(rpcif_disable_rpm);
-
void rpcif_hw_init(struct rpcif *rpc, bool hyperflash)
{
u32 dummy;
diff --git a/include/memory/renesas-rpc-if.h b/include/memory/renesas-rpc-if.h
index 9ad136682c47..b8c7cc63065f 100644
--- a/include/memory/renesas-rpc-if.h
+++ b/include/memory/renesas-rpc-if.h
@@ -10,6 +10,7 @@
#ifndef __RENESAS_RPC_IF_H
#define __RENESAS_RPC_IF_H
+#include <linux/pm_runtime.h>
#include <linux/types.h>
enum rpcif_data_dir {
@@ -77,11 +78,19 @@ struct rpcif {
int rpcif_sw_init(struct rpcif *rpc, struct device *dev);
void rpcif_hw_init(struct rpcif *rpc, bool hyperflash);
-void rpcif_enable_rpm(struct rpcif *rpc);
-void rpcif_disable_rpm(struct rpcif *rpc);
void rpcif_prepare(struct rpcif *rpc, const struct rpcif_op *op, u64 *offs,
size_t *len);
int rpcif_manual_xfer(struct rpcif *rpc);
ssize_t rpcif_dirmap_read(struct rpcif *rpc, u64 offs, size_t len, void *buf);
+static inline void rpcif_enable_rpm(struct rpcif *rpc)
+{
+ pm_runtime_enable(rpc->dev);
+}
+
+static inline void rpcif_disable_rpm(struct rpcif *rpc)
+{
+ pm_runtime_put_sync(rpc->dev);
+}
+
#endif // __RENESAS_RPC_IF_H
--
2.17.1
Release the node reference by calling of_node_put(flash) in the probe.
Fixes: ca7d8b980b67f ("memory: add Renesas RPC-IF driver")
Reported-by: Pavel Machek <[email protected]>
Signed-off-by: Lad Prabhakar <[email protected]>
Cc: [email protected]
---
drivers/memory/renesas-rpc-if.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/memory/renesas-rpc-if.c b/drivers/memory/renesas-rpc-if.c
index f5cbc762fda7..99633986ffda 100644
--- a/drivers/memory/renesas-rpc-if.c
+++ b/drivers/memory/renesas-rpc-if.c
@@ -548,9 +548,11 @@ static int rpcif_probe(struct platform_device *pdev)
} else if (of_device_is_compatible(flash, "cfi-flash")) {
name = "rpc-if-hyperflash";
} else {
+ of_node_put(flash);
dev_warn(&pdev->dev, "unknown flash type\n");
return -ENODEV;
}
+ of_node_put(flash);
vdev = platform_device_alloc(name, pdev->id);
if (!vdev)
--
2.17.1
Hi Sergei,
On Tue, Nov 24, 2020 at 11:26 AM Lad Prabhakar
<[email protected]> wrote:
>
> Hi All,
>
> This patch series fixes trivial issues in RPC-IF driver.
>
> Cheers,
> Prabhakar
>
> Lad Prabhakar (5):
> memory: renesas-rpc-if: Return correct value to the caller of
> rpcif_manual_xfer()
> memory: renesas-rpc-if: Make rpcif_enable/disable_rpm() as static
> inline
> memory: renesas-rpc-if: Export symbols as GPL
> memory: renesas-rpc-if: Avoid use of C++ style comments
> memory: renesas-rpc-if: Fix a reference leak in rpcif_probe()
>
Patches sent to [email protected] have bounced back
so including gmail address (patchwork [1]).
[1] https://patchwork.kernel.org/project/linux-renesas-soc/list/?series=390163
Cheers,
Prabhakar
> drivers/memory/renesas-rpc-if.c | 28 +++++++++-------------------
> include/memory/renesas-rpc-if.h | 19 ++++++++++++++-----
> 2 files changed, 23 insertions(+), 24 deletions(-)
>
> --
> 2.17.1
>
Hi Prabhakar,
On Tue, Nov 24, 2020 at 12:27 PM Lad Prabhakar
<[email protected]> wrote:
> Define rpcif_enable_rpm() and rpcif_disable_rpm() as static
> inline in the header instead of exporting it.
>
> Suggested-by: Pavel Machek <[email protected]>
> Signed-off-by: Lad Prabhakar <[email protected]>
Thanks for your patch, which is an improvement.
> --- a/include/memory/renesas-rpc-if.h
> +++ b/include/memory/renesas-rpc-if.h
> @@ -10,6 +10,7 @@
> #ifndef __RENESAS_RPC_IF_H
> #define __RENESAS_RPC_IF_H
>
> +#include <linux/pm_runtime.h>
> #include <linux/types.h>
>
> enum rpcif_data_dir {
> @@ -77,11 +78,19 @@ struct rpcif {
>
> int rpcif_sw_init(struct rpcif *rpc, struct device *dev);
> void rpcif_hw_init(struct rpcif *rpc, bool hyperflash);
> -void rpcif_enable_rpm(struct rpcif *rpc);
> -void rpcif_disable_rpm(struct rpcif *rpc);
> void rpcif_prepare(struct rpcif *rpc, const struct rpcif_op *op, u64 *offs,
> size_t *len);
> int rpcif_manual_xfer(struct rpcif *rpc);
> ssize_t rpcif_dirmap_read(struct rpcif *rpc, u64 offs, size_t len, void *buf);
>
> +static inline void rpcif_enable_rpm(struct rpcif *rpc)
> +{
> + pm_runtime_enable(rpc->dev);
> +}
> +
> +static inline void rpcif_disable_rpm(struct rpcif *rpc)
> +{
> + pm_runtime_put_sync(rpc->dev);
Looking at how this is used, this should call pm_runtime_disable()
instead.
And probably this should be moved inside the core RPC-IF driver:
1. pm_runtime_enable() could be called from rpcif_sw_init(),
2. pm_runtime_put_sync() can be called from a new rpc_sw_deinit()
function, to be called by the SPI and MTD drivers on probe failure
and on remove.
> +}
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Hello!
On 11/24/20 2:25 PM, Lad Prabhakar wrote:
> Define rpcif_enable_rpm() and rpcif_disable_rpm() as static
Not sure why I didn't do it this way myself...
> inline in the header instead of exporting it.
s/it/them/.
> Suggested-by: Pavel Machek <[email protected]>
> Signed-off-by: Lad Prabhakar <[email protected]>
> ---
> drivers/memory/renesas-rpc-if.c | 13 -------------
> include/memory/renesas-rpc-if.h | 13 +++++++++++--
> 2 files changed, 11 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/memory/renesas-rpc-if.c b/drivers/memory/renesas-rpc-if.c
> index 69f2e2b4cd50..c5b5691503d7 100644
> --- a/drivers/memory/renesas-rpc-if.c
> +++ b/drivers/memory/renesas-rpc-if.c
[...]
> @@ -204,18 +203,6 @@ int rpcif_sw_init(struct rpcif *rpc, struct device *dev)
> }
> EXPORT_SYMBOL(rpcif_sw_init);
>
> -void rpcif_enable_rpm(struct rpcif *rpc)
> -{
> - pm_runtime_enable(rpc->dev);
> -}
> -EXPORT_SYMBOL(rpcif_enable_rpm);
> -
> -void rpcif_disable_rpm(struct rpcif *rpc)
> -{
> - pm_runtime_put_sync(rpc->dev);
Ugh... sorry for this blunder (that went unnoticed till now). Mind fixing
it to pm_runtime_disable() (before this patch)?
> -}
> -EXPORT_SYMBOL(rpcif_disable_rpm);
> -
> void rpcif_hw_init(struct rpcif *rpc, bool hyperflash)
> {
> u32 dummy;
[...]
MBR, Sergei
On 11/24/20 2:34 PM, Lad, Prabhakar wrote:
[...]
>> This patch series fixes trivial issues in RPC-IF driver.
>>
>> Cheers,
>> Prabhakar
>>
>> Lad Prabhakar (5):
>> memory: renesas-rpc-if: Return correct value to the caller of
>> rpcif_manual_xfer()
>> memory: renesas-rpc-if: Make rpcif_enable/disable_rpm() as static
>> inline
>> memory: renesas-rpc-if: Export symbols as GPL
>> memory: renesas-rpc-if: Avoid use of C++ style comments
>> memory: renesas-rpc-if: Fix a reference leak in rpcif_probe()
>>
> Patches sent to [email protected] have bounced back
> so including gmail address (patchwork [1]).
Sorry, I got laid off by Cogent last May. Thanks for CCing my gmail address...
> [1] https://patchwork.kernel.org/project/linux-renesas-soc/list/?series=390163
>
> Cheers,
> Prabhakar
[...]
MBR, Sergei
Replace C++ style comment with C style.
While at it also replace the tab with a space between struct and
struct name.
Suggested-by: Pavel Machek <[email protected]>
Signed-off-by: Lad Prabhakar <[email protected]>
---
include/memory/renesas-rpc-if.h | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/include/memory/renesas-rpc-if.h b/include/memory/renesas-rpc-if.h
index b8c7cc63065f..30ea6bd969b4 100644
--- a/include/memory/renesas-rpc-if.h
+++ b/include/memory/renesas-rpc-if.h
@@ -19,7 +19,7 @@ enum rpcif_data_dir {
RPCIF_DATA_OUT,
};
-struct rpcif_op {
+struct rpcif_op {
struct {
u8 buswidth;
u8 opcode;
@@ -57,7 +57,7 @@ struct rpcif_op {
} data;
};
-struct rpcif {
+struct rpcif {
struct device *dev;
void __iomem *dirmap;
struct regmap *regmap;
@@ -93,4 +93,4 @@ static inline void rpcif_disable_rpm(struct rpcif *rpc)
pm_runtime_put_sync(rpc->dev);
}
-#endif // __RENESAS_RPC_IF_H
+#endif /* __RENESAS_RPC_IF_H */
--
2.17.1
Renesas RPC-IF driver is licensed under GPL2.0, to be in sync export the
symbols as GPL.
Suggested-by: Pavel Machek <[email protected]>
Signed-off-by: Lad Prabhakar <[email protected]>
---
drivers/memory/renesas-rpc-if.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/memory/renesas-rpc-if.c b/drivers/memory/renesas-rpc-if.c
index c5b5691503d7..f5cbc762fda7 100644
--- a/drivers/memory/renesas-rpc-if.c
+++ b/drivers/memory/renesas-rpc-if.c
@@ -201,7 +201,7 @@ int rpcif_sw_init(struct rpcif *rpc, struct device *dev)
return PTR_ERR_OR_ZERO(rpc->rstc);
}
-EXPORT_SYMBOL(rpcif_sw_init);
+EXPORT_SYMBOL_GPL(rpcif_sw_init);
void rpcif_hw_init(struct rpcif *rpc, bool hyperflash)
{
@@ -249,7 +249,7 @@ void rpcif_hw_init(struct rpcif *rpc, bool hyperflash)
rpc->bus_size = hyperflash ? 2 : 1;
}
-EXPORT_SYMBOL(rpcif_hw_init);
+EXPORT_SYMBOL_GPL(rpcif_hw_init);
static int wait_msg_xfer_end(struct rpcif *rpc)
{
@@ -358,7 +358,7 @@ void rpcif_prepare(struct rpcif *rpc, const struct rpcif_op *op, u64 *offs,
RPCIF_SMENR_SPIDB(rpcif_bit_size(op->data.buswidth));
}
}
-EXPORT_SYMBOL(rpcif_prepare);
+EXPORT_SYMBOL_GPL(rpcif_prepare);
int rpcif_manual_xfer(struct rpcif *rpc)
{
@@ -500,7 +500,7 @@ int rpcif_manual_xfer(struct rpcif *rpc)
rpcif_hw_init(rpc, rpc->bus_size == 2);
goto exit;
}
-EXPORT_SYMBOL(rpcif_manual_xfer);
+EXPORT_SYMBOL_GPL(rpcif_manual_xfer);
ssize_t rpcif_dirmap_read(struct rpcif *rpc, u64 offs, size_t len, void *buf)
{
@@ -529,7 +529,7 @@ ssize_t rpcif_dirmap_read(struct rpcif *rpc, u64 offs, size_t len, void *buf)
return len;
}
-EXPORT_SYMBOL(rpcif_dirmap_read);
+EXPORT_SYMBOL_GPL(rpcif_dirmap_read);
static int rpcif_probe(struct platform_device *pdev)
{
--
2.17.1
On 11/24/20 2:25 PM, Lad Prabhakar wrote:
> Replace C++ style comment with C style.
Thanks, I've overlooked this, and the header files should use C style comment,
not C++.
> While at it also replace the tab with a space between struct and
> struct name.
No connection between these 2 changes, so there should be 2 patches, not 1.
Also, I'd like to ask you that they're left intact (unless it causes problems
for you).
> Suggested-by: Pavel Machek <[email protected]>
> Signed-off-by: Lad Prabhakar <[email protected]>
[...]
> diff --git a/include/memory/renesas-rpc-if.h b/include/memory/renesas-rpc-if.h
> index b8c7cc63065f..30ea6bd969b4 100644
> --- a/include/memory/renesas-rpc-if.h
> +++ b/include/memory/renesas-rpc-if.h
> @@ -19,7 +19,7 @@ enum rpcif_data_dir {
> RPCIF_DATA_OUT,
> };
>
> -struct rpcif_op {
> +struct rpcif_op {
> struct {
> u8 buswidth;
> u8 opcode;
> @@ -57,7 +57,7 @@ struct rpcif_op {
> } data;
> };
>
> -struct rpcif {
> +struct rpcif {
> struct device *dev;
> void __iomem *dirmap;
> struct regmap *regmap;
> @@ -93,4 +93,4 @@ static inline void rpcif_disable_rpm(struct rpcif *rpc)
> pm_runtime_put_sync(rpc->dev);
> }
>
> -#endif // __RENESAS_RPC_IF_H
> +#endif /* __RENESAS_RPC_IF_H */
MBR, Sergei
Hello!
On 24.11.2020 14:25, Lad Prabhakar wrote:
> In the error path of rpcif_manual_xfer() the value of ret is overwritten
> by value returned by reset_control_reset() function and thus returning
> incorrect value to the caller.
>
> This patch makes sure the correct value is returned to the caller of
> rpcif_manual_xfer() by dropping the overwrite of ret in error path.
> Also now we ignore the value returned by reset_control_reset() in the
> error path and instead print a error message when it fails.
>
> Fixes: ca7d8b980b67f ("memory: add Renesas RPC-IF driver")
> Reported-by: Pavel Machek <[email protected]>
> Signed-off-by: Lad Prabhakar <[email protected]>
> Cc: [email protected]
Reviewed-by: Sergei Shtylyov <[email protected]>
MBR, Sergei
On 24.11.2020 14:25, Lad Prabhakar wrote:
> Renesas RPC-IF driver is licensed under GPL2.0, to be in sync export the
> symbols as GPL.
Didn't know there's a connection...
> Suggested-by: Pavel Machek <[email protected]>
> Signed-off-by: Lad Prabhakar <[email protected]>
Reviewed-by: Sergei Shtylyov <[email protected]>
[...]
MBR, Sergei
On 11/24/20 2:25 PM, Lad Prabhakar wrote:
> Release the node reference by calling of_node_put(flash) in the probe.
Sorry about missing this...
> Fixes: ca7d8b980b67f ("memory: add Renesas RPC-IF driver")
> Reported-by: Pavel Machek <[email protected]>
> Signed-off-by: Lad Prabhakar <[email protected]>
> Cc: [email protected]
Reviewed-by: Sergei Shtylyov <[email protected]>
[...]
MBR, Sergei
On Tue, Nov 24, 2020 at 6:25 PM Sergei Shtylyov
<[email protected]> wrote:
>
> On 11/24/20 2:34 PM, Lad, Prabhakar wrote:
>
> [...]
> >> This patch series fixes trivial issues in RPC-IF driver.
> >>
> >> Cheers,
> >> Prabhakar
> >>
> >> Lad Prabhakar (5):
> >> memory: renesas-rpc-if: Return correct value to the caller of
> >> rpcif_manual_xfer()
> >> memory: renesas-rpc-if: Make rpcif_enable/disable_rpm() as static
> >> inline
> >> memory: renesas-rpc-if: Export symbols as GPL
> >> memory: renesas-rpc-if: Avoid use of C++ style comments
> >> memory: renesas-rpc-if: Fix a reference leak in rpcif_probe()
> >>
> > Patches sent to [email protected] have bounced back
> > so including gmail address (patchwork [1]).
>
> Sorry, I got laid off by Cogent last May. Thanks for CCing my gmail address...
>
Sorry to hear that.
Thank you for the review. I'll fix the review comments for patch 2/2
and post a v2.
Cheers,
Prabhakar
Hi Geert,
Thank you for the review.
On Tue, Nov 24, 2020 at 3:43 PM Geert Uytterhoeven <[email protected]> wrote:
>
> Hi Prabhakar,
>
> On Tue, Nov 24, 2020 at 12:27 PM Lad Prabhakar
> <[email protected]> wrote:
> > Define rpcif_enable_rpm() and rpcif_disable_rpm() as static
> > inline in the header instead of exporting it.
> >
> > Suggested-by: Pavel Machek <[email protected]>
> > Signed-off-by: Lad Prabhakar <[email protected]>
>
> Thanks for your patch, which is an improvement.
>
> > --- a/include/memory/renesas-rpc-if.h
> > +++ b/include/memory/renesas-rpc-if.h
> > @@ -10,6 +10,7 @@
> > #ifndef __RENESAS_RPC_IF_H
> > #define __RENESAS_RPC_IF_H
> >
> > +#include <linux/pm_runtime.h>
> > #include <linux/types.h>
> >
> > enum rpcif_data_dir {
> > @@ -77,11 +78,19 @@ struct rpcif {
> >
> > int rpcif_sw_init(struct rpcif *rpc, struct device *dev);
> > void rpcif_hw_init(struct rpcif *rpc, bool hyperflash);
> > -void rpcif_enable_rpm(struct rpcif *rpc);
> > -void rpcif_disable_rpm(struct rpcif *rpc);
> > void rpcif_prepare(struct rpcif *rpc, const struct rpcif_op *op, u64 *offs,
> > size_t *len);
> > int rpcif_manual_xfer(struct rpcif *rpc);
> > ssize_t rpcif_dirmap_read(struct rpcif *rpc, u64 offs, size_t len, void *buf);
> >
> > +static inline void rpcif_enable_rpm(struct rpcif *rpc)
> > +{
> > + pm_runtime_enable(rpc->dev);
> > +}
> > +
> > +static inline void rpcif_disable_rpm(struct rpcif *rpc)
> > +{
> > + pm_runtime_put_sync(rpc->dev);
>
> Looking at how this is used, this should call pm_runtime_disable()
> instead.
>
> And probably this should be moved inside the core RPC-IF driver:
> 1. pm_runtime_enable() could be called from rpcif_sw_init(),
> 2. pm_runtime_put_sync() can be called from a new rpc_sw_deinit()
> function, to be called by the SPI and MTD drivers on probe failure
> and on remove.
>
Totally agree.
Sergei are you OK with the above suggestions ?
Cheers,
Prabhakar