* [PATCH v1 1/5] tty: rpmsg: Assign returned id to a local variable
@ 2021-10-25 13:51 Andy Shevchenko
2021-10-25 13:51 ` [PATCH v1 2/5] tty: rpmsg: Unify variable used to keep an error code Andy Shevchenko
` (4 more replies)
0 siblings, 5 replies; 7+ messages in thread
From: Andy Shevchenko @ 2021-10-25 13:51 UTC (permalink / raw)
To: Andy Shevchenko, Arnaud Pouliquen, linux-kernel
Cc: Greg Kroah-Hartman, Jiri Slaby
Instead of putting garbage in the data structure, assign allocated id
or an error code to a temporary variable. This makes code cleaner.
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
drivers/tty/rpmsg_tty.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/tty/rpmsg_tty.c b/drivers/tty/rpmsg_tty.c
index 813076341ffd..8c17ddbf371d 100644
--- a/drivers/tty/rpmsg_tty.c
+++ b/drivers/tty/rpmsg_tty.c
@@ -121,15 +121,16 @@ static struct rpmsg_tty_port *rpmsg_tty_alloc_cport(void)
return ERR_PTR(-ENOMEM);
mutex_lock(&idr_lock);
- cport->id = idr_alloc(&tty_idr, cport, 0, MAX_TTY_RPMSG, GFP_KERNEL);
+ err = idr_alloc(&tty_idr, cport, 0, MAX_TTY_RPMSG, GFP_KERNEL);
mutex_unlock(&idr_lock);
- if (cport->id < 0) {
- err = cport->id;
+ if (err < 0) {
kfree(cport);
return ERR_PTR(err);
}
+ cport->id = err;
+
return cport;
}
--
2.33.0
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH v1 2/5] tty: rpmsg: Unify variable used to keep an error code
2021-10-25 13:51 [PATCH v1 1/5] tty: rpmsg: Assign returned id to a local variable Andy Shevchenko
@ 2021-10-25 13:51 ` Andy Shevchenko
2021-10-25 13:51 ` [PATCH v1 3/5] tty: rpmsg: Use dev_err_probe() in ->probe() Andy Shevchenko
` (3 subsequent siblings)
4 siblings, 0 replies; 7+ messages in thread
From: Andy Shevchenko @ 2021-10-25 13:51 UTC (permalink / raw)
To: Andy Shevchenko, Arnaud Pouliquen, linux-kernel
Cc: Greg Kroah-Hartman, Jiri Slaby
In some ret is used, in the other err. Let's unify it across the driver.
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
drivers/tty/rpmsg_tty.c | 26 +++++++++++++-------------
1 file changed, 13 insertions(+), 13 deletions(-)
diff --git a/drivers/tty/rpmsg_tty.c b/drivers/tty/rpmsg_tty.c
index 8c17ddbf371d..fc5d33dc4cb8 100644
--- a/drivers/tty/rpmsg_tty.c
+++ b/drivers/tty/rpmsg_tty.c
@@ -114,22 +114,22 @@ static const struct tty_operations rpmsg_tty_ops = {
static struct rpmsg_tty_port *rpmsg_tty_alloc_cport(void)
{
struct rpmsg_tty_port *cport;
- int err;
+ int ret;
cport = kzalloc(sizeof(*cport), GFP_KERNEL);
if (!cport)
return ERR_PTR(-ENOMEM);
mutex_lock(&idr_lock);
- err = idr_alloc(&tty_idr, cport, 0, MAX_TTY_RPMSG, GFP_KERNEL);
+ ret = idr_alloc(&tty_idr, cport, 0, MAX_TTY_RPMSG, GFP_KERNEL);
mutex_unlock(&idr_lock);
- if (err < 0) {
+ if (ret < 0) {
kfree(cport);
- return ERR_PTR(err);
+ return ERR_PTR(ret);
}
- cport->id = err;
+ cport->id = ret;
return cport;
}
@@ -217,7 +217,7 @@ static struct rpmsg_driver rpmsg_tty_rpmsg_drv = {
static int __init rpmsg_tty_init(void)
{
- int err;
+ int ret;
rpmsg_tty_driver = tty_alloc_driver(MAX_TTY_RPMSG, TTY_DRIVER_REAL_RAW |
TTY_DRIVER_DYNAMIC_DEV);
@@ -236,15 +236,15 @@ static int __init rpmsg_tty_init(void)
tty_set_operations(rpmsg_tty_driver, &rpmsg_tty_ops);
- err = tty_register_driver(rpmsg_tty_driver);
- if (err < 0) {
- pr_err("Couldn't install rpmsg tty driver: err %d\n", err);
+ ret = tty_register_driver(rpmsg_tty_driver);
+ if (ret < 0) {
+ pr_err("Couldn't install rpmsg tty driver: %d\n", ret);
goto error_put;
}
- err = register_rpmsg_driver(&rpmsg_tty_rpmsg_drv);
- if (err < 0) {
- pr_err("Couldn't register rpmsg tty driver: err %d\n", err);
+ ret = register_rpmsg_driver(&rpmsg_tty_rpmsg_drv);
+ if (ret < 0) {
+ pr_err("Couldn't register rpmsg tty driver: %d\n", ret);
goto error_unregister;
}
@@ -256,7 +256,7 @@ static int __init rpmsg_tty_init(void)
error_put:
tty_driver_kref_put(rpmsg_tty_driver);
- return err;
+ return ret;
}
static void __exit rpmsg_tty_exit(void)
--
2.33.0
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH v1 3/5] tty: rpmsg: Use dev_err_probe() in ->probe()
2021-10-25 13:51 [PATCH v1 1/5] tty: rpmsg: Assign returned id to a local variable Andy Shevchenko
2021-10-25 13:51 ` [PATCH v1 2/5] tty: rpmsg: Unify variable used to keep an error code Andy Shevchenko
@ 2021-10-25 13:51 ` Andy Shevchenko
2021-10-25 13:51 ` [PATCH v1 4/5] tty: rpmsg: Add pr_fmt() to prefix messages Andy Shevchenko
` (2 subsequent siblings)
4 siblings, 0 replies; 7+ messages in thread
From: Andy Shevchenko @ 2021-10-25 13:51 UTC (permalink / raw)
To: Andy Shevchenko, Arnaud Pouliquen, linux-kernel
Cc: Greg Kroah-Hartman, Jiri Slaby
It's fine to use dev_err_probe() in ->probe() even if we know
it won't be deferred.
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
drivers/tty/rpmsg_tty.c | 11 ++++-------
1 file changed, 4 insertions(+), 7 deletions(-)
diff --git a/drivers/tty/rpmsg_tty.c b/drivers/tty/rpmsg_tty.c
index fc5d33dc4cb8..d172dd331bc9 100644
--- a/drivers/tty/rpmsg_tty.c
+++ b/drivers/tty/rpmsg_tty.c
@@ -153,10 +153,8 @@ static int rpmsg_tty_probe(struct rpmsg_device *rpdev)
int ret;
cport = rpmsg_tty_alloc_cport();
- if (IS_ERR(cport)) {
- dev_err(dev, "Failed to alloc tty port\n");
- return PTR_ERR(cport);
- }
+ if (IS_ERR(cport))
+ return dev_err_probe(dev, PTR_ERR(cport), "Failed to alloc tty port\n");
tty_port_init(&cport->port);
cport->port.ops = &rpmsg_tty_port_ops;
@@ -164,9 +162,8 @@ static int rpmsg_tty_probe(struct rpmsg_device *rpdev)
tty_dev = tty_port_register_device(&cport->port, rpmsg_tty_driver,
cport->id, dev);
if (IS_ERR(tty_dev)) {
- dev_err(dev, "Failed to register tty port\n");
- ret = PTR_ERR(tty_dev);
- goto err_destroy;
+ ret = dev_err_probe(dev, PTR_ERR(tty_dev), "Failed to register tty port\n");
+ goto err_destroy;
}
cport->rpdev = rpdev;
--
2.33.0
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH v1 4/5] tty: rpmsg: Add pr_fmt() to prefix messages
2021-10-25 13:51 [PATCH v1 1/5] tty: rpmsg: Assign returned id to a local variable Andy Shevchenko
2021-10-25 13:51 ` [PATCH v1 2/5] tty: rpmsg: Unify variable used to keep an error code Andy Shevchenko
2021-10-25 13:51 ` [PATCH v1 3/5] tty: rpmsg: Use dev_err_probe() in ->probe() Andy Shevchenko
@ 2021-10-25 13:51 ` Andy Shevchenko
2021-10-25 13:51 ` [PATCH v1 5/5] tty: rpmsg: Define tty name via constant string literal Andy Shevchenko
2021-11-02 13:19 ` [PATCH v1 1/5] tty: rpmsg: Assign returned id to a local variable Arnaud POULIQUEN
4 siblings, 0 replies; 7+ messages in thread
From: Andy Shevchenko @ 2021-10-25 13:51 UTC (permalink / raw)
To: Andy Shevchenko, Arnaud Pouliquen, linux-kernel
Cc: Greg Kroah-Hartman, Jiri Slaby
Make all messages to be prefixed in a unified way.
Add pr_fmt() to achieve this.
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
drivers/tty/rpmsg_tty.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/tty/rpmsg_tty.c b/drivers/tty/rpmsg_tty.c
index d172dd331bc9..4bee6c5bbb53 100644
--- a/drivers/tty/rpmsg_tty.c
+++ b/drivers/tty/rpmsg_tty.c
@@ -10,6 +10,8 @@
* The "rpmsg-tty" service is directly used for data exchange. No flow control is implemented yet.
*/
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
#include <linux/module.h>
#include <linux/rpmsg.h>
#include <linux/slab.h>
@@ -235,13 +237,13 @@ static int __init rpmsg_tty_init(void)
ret = tty_register_driver(rpmsg_tty_driver);
if (ret < 0) {
- pr_err("Couldn't install rpmsg tty driver: %d\n", ret);
+ pr_err("Couldn't install driver: %d\n", ret);
goto error_put;
}
ret = register_rpmsg_driver(&rpmsg_tty_rpmsg_drv);
if (ret < 0) {
- pr_err("Couldn't register rpmsg tty driver: %d\n", ret);
+ pr_err("Couldn't register driver: %d\n", ret);
goto error_unregister;
}
--
2.33.0
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH v1 5/5] tty: rpmsg: Define tty name via constant string literal
2021-10-25 13:51 [PATCH v1 1/5] tty: rpmsg: Assign returned id to a local variable Andy Shevchenko
` (2 preceding siblings ...)
2021-10-25 13:51 ` [PATCH v1 4/5] tty: rpmsg: Add pr_fmt() to prefix messages Andy Shevchenko
@ 2021-10-25 13:51 ` Andy Shevchenko
2021-11-02 13:19 ` [PATCH v1 1/5] tty: rpmsg: Assign returned id to a local variable Arnaud POULIQUEN
4 siblings, 0 replies; 7+ messages in thread
From: Andy Shevchenko @ 2021-10-25 13:51 UTC (permalink / raw)
To: Andy Shevchenko, Arnaud Pouliquen, linux-kernel
Cc: Greg Kroah-Hartman, Jiri Slaby
Driver uses already twice the same string literal.
Define it in one place, so every user will have this
name consistent.
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
drivers/tty/rpmsg_tty.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/tty/rpmsg_tty.c b/drivers/tty/rpmsg_tty.c
index 4bee6c5bbb53..dae2a4e44f38 100644
--- a/drivers/tty/rpmsg_tty.c
+++ b/drivers/tty/rpmsg_tty.c
@@ -18,6 +18,7 @@
#include <linux/tty.h>
#include <linux/tty_flip.h>
+#define RPMSG_TTY_NAME "ttyRPMSG"
#define MAX_TTY_RPMSG 32
static DEFINE_IDR(tty_idr); /* tty instance id */
@@ -172,7 +173,7 @@ static int rpmsg_tty_probe(struct rpmsg_device *rpdev)
dev_set_drvdata(dev, cport);
- dev_dbg(dev, "New channel: 0x%x -> 0x%x : ttyRPMSG%d\n",
+ dev_dbg(dev, "New channel: 0x%x -> 0x%x: " RPMSG_TTY_NAME "%d\n",
rpdev->src, rpdev->dst, cport->id);
return 0;
@@ -224,7 +225,7 @@ static int __init rpmsg_tty_init(void)
return PTR_ERR(rpmsg_tty_driver);
rpmsg_tty_driver->driver_name = "rpmsg_tty";
- rpmsg_tty_driver->name = "ttyRPMSG";
+ rpmsg_tty_driver->name = RPMSG_TTY_NAME;
rpmsg_tty_driver->major = 0;
rpmsg_tty_driver->type = TTY_DRIVER_TYPE_CONSOLE;
--
2.33.0
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v1 1/5] tty: rpmsg: Assign returned id to a local variable
2021-10-25 13:51 [PATCH v1 1/5] tty: rpmsg: Assign returned id to a local variable Andy Shevchenko
` (3 preceding siblings ...)
2021-10-25 13:51 ` [PATCH v1 5/5] tty: rpmsg: Define tty name via constant string literal Andy Shevchenko
@ 2021-11-02 13:19 ` Arnaud POULIQUEN
2021-11-02 14:40 ` Andy Shevchenko
4 siblings, 1 reply; 7+ messages in thread
From: Arnaud POULIQUEN @ 2021-11-02 13:19 UTC (permalink / raw)
To: Andy Shevchenko, linux-kernel; +Cc: Greg Kroah-Hartman, Jiri Slaby
Hi Andy,
On 10/25/21 3:51 PM, Andy Shevchenko wrote:
> Instead of putting garbage in the data structure, assign allocated id
> or an error code to a temporary variable. This makes code cleaner.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
> drivers/tty/rpmsg_tty.c | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/tty/rpmsg_tty.c b/drivers/tty/rpmsg_tty.c
> index 813076341ffd..8c17ddbf371d 100644
> --- a/drivers/tty/rpmsg_tty.c
> +++ b/drivers/tty/rpmsg_tty.c
> @@ -121,15 +121,16 @@ static struct rpmsg_tty_port *rpmsg_tty_alloc_cport(void)
> return ERR_PTR(-ENOMEM);
>
> mutex_lock(&idr_lock);
> - cport->id = idr_alloc(&tty_idr, cport, 0, MAX_TTY_RPMSG, GFP_KERNEL);
> + err = idr_alloc(&tty_idr, cport, 0, MAX_TTY_RPMSG, GFP_KERNEL);
> mutex_unlock(&idr_lock);
>
> - if (cport->id < 0) {
> - err = cport->id;
> + if (err < 0) {
> kfree(cport);
> return ERR_PTR(err);
> }
>
> + cport->id = err;
Set the cport->id to the err variable on success doesn't seem completely clean
to me either.
What about renaming "err" by "id" as done in [1]?
[1]
https://elixir.bootlin.com/linux/latest/source/drivers/tty/serial/mps2-uart.c#L526
Regards,
Arnaud
> +
> return cport;
> }
>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v1 1/5] tty: rpmsg: Assign returned id to a local variable
2021-11-02 13:19 ` [PATCH v1 1/5] tty: rpmsg: Assign returned id to a local variable Arnaud POULIQUEN
@ 2021-11-02 14:40 ` Andy Shevchenko
0 siblings, 0 replies; 7+ messages in thread
From: Andy Shevchenko @ 2021-11-02 14:40 UTC (permalink / raw)
To: Arnaud POULIQUEN
Cc: Andy Shevchenko, Linux Kernel Mailing List, Greg Kroah-Hartman,
Jiri Slaby
On Tue, Nov 2, 2021 at 3:21 PM Arnaud POULIQUEN
<arnaud.pouliquen@foss.st.com> wrote:
> On 10/25/21 3:51 PM, Andy Shevchenko wrote:
...
> > + cport->id = err;
>
> Set the cport->id to the err variable on success doesn't seem completely clean
> to me either.
I totally agree with you, that's why the
https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git/commit/?id=408a507996e4
changed it.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-11-02 14:41 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-25 13:51 [PATCH v1 1/5] tty: rpmsg: Assign returned id to a local variable Andy Shevchenko
2021-10-25 13:51 ` [PATCH v1 2/5] tty: rpmsg: Unify variable used to keep an error code Andy Shevchenko
2021-10-25 13:51 ` [PATCH v1 3/5] tty: rpmsg: Use dev_err_probe() in ->probe() Andy Shevchenko
2021-10-25 13:51 ` [PATCH v1 4/5] tty: rpmsg: Add pr_fmt() to prefix messages Andy Shevchenko
2021-10-25 13:51 ` [PATCH v1 5/5] tty: rpmsg: Define tty name via constant string literal Andy Shevchenko
2021-11-02 13:19 ` [PATCH v1 1/5] tty: rpmsg: Assign returned id to a local variable Arnaud POULIQUEN
2021-11-02 14:40 ` Andy Shevchenko
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).