Messages in this thread | | | From | Andy Shevchenko <> | Date | Sun, 18 Jun 2017 22:54:48 +0300 | Subject | Re: [patch v2 2/3] staging: speakup: check and convert dev name or ser to dev_t |
| |
On Sun, Jun 18, 2017 at 8:22 PM, Okash Khawaja <okash.khawaja@gmail.com> wrote: > On Sun, Jun 18, 2017 at 04:35:21PM +0300, Andy Shevchenko wrote:
>> > + if (ser < 0 || ser > (255 - 64)) { >> >> > + pr_err("speakup: Invalid ser param. \ >> > + Must be between 0 and 191 inclusive.\n"); >> >> Just make it one line. > Is it okay if it becomes larger than 80 chars?
Yes. Even checkpatch will not complain in this case.
>> > + pr_err("speakup: lp* is only supported on:"); >> >> > + for (i = 0; i < ARRAY_SIZE(lp_supported); i++) >> > + pr_cont(" %s", lp_supported[i]); >> > + pr_cont("\n"); >> >> pr_cont() is not the best idea, though I think it will be rare cases >> when it might be broken in pieces.
> Hmm... I would like to keep it if it doesn't incur an overhead. It also > indicates to the reader that this all part of same output line. Let me > know what you think.
For better user experience you need something like a temporary buffer and one pr_err(); line at the end. But as I said here is quite a chance that no one will see this message broken apart.
>> > --- a/drivers/staging/speakup/spk_types.h >> > +++ b/drivers/staging/speakup/spk_types.h >> > @@ -169,6 +169,7 @@ struct spk_synth { >> > int jiffies; >> > int full; >> > int ser; >> >> > + char *dev_name; >> >> const ? > This becomes the target of module_param in following patch. It complains > when set to const.
Fair enough.
-- With Best Regards, Andy Shevchenko
| |