Re: [PATCH] staging: panel: Fix single-open policy race condition
On Wed, Nov 05, 2014 at 12:51:22PM +0100, Mariusz Gorski wrote: > On Wed, Nov 05, 2014 at 01:19:10PM +0300, Dan Carpenter wrote: > > On Tue, Nov 04, 2014 at 10:47:19PM +0100, Mariusz Gorski wrote: > > > Fix the implementation of a single-open policy for both > > > devices (lcd and keypad) by using atomic_t instead of plain ints. > > > > > > > This seems like it might be a real life bug that you have experienced? > > No, I don't think it might really happen in real life. I found it just > by reading the code. A similar solution is used in Chapter 6 of the LDD3 > book, so I thought it might be a good idea to fix is here. BTW, it should be kept in mind that I first wrote this driver on 2.0 or 2.2 and it used to run on an i386. So it's extremely likely that a lot of locking was missing by then and that until it gets discovered by code review as Mariusz did, issues may still be present. Note that I'm currently using this driver on production systems where this issue cannot happen since a few scripts are allowed to send data to the LCD (which might most always be the case by design given that nobody wants to build a system where many scripts send unreadable crap at the same time on the display). Thanks Mariusz for fixing this. Willy ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 0/4] staging: panel: Module parameters clean-up
On Wed, Nov 12, 2014 at 02:08:05AM +0100, Mariusz Gorski wrote: > This small set of patches (although it also could be a single patch...) > rearranges groups of defines, redefines and module parameter declarations, > so that they always appear in the same order defined by Kconfig, which > makes it more pleasant to read and understand. It's a preparation for > a bigger clean-up of current init code. > > Mariusz Gorski (4): > staging: panel: Reorder initial DEFAULT_* defines > staging: panel: Reorder DEFAULT_* values redefines > staging: panel: Reorder module parameter declarations > staging: panel: Use better names for two defined values Whole patchset Acked-By: Willy Tarreau BTW Mariusz, please use this e-mail address instead of the meta-x.org one in the future. Thanks, Willy ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/9] staging: panel: Set default parport module param value
On Tue, Nov 18, 2014 at 09:56:06PM +0100, Mariusz Gorski wrote: > Set default parport module param value to DEFAULT_PARPORT so that > a if-block can be avoided. > > Signed-off-by: Mariusz Gorski Acked-by: Willy Tarreau ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 3/9] staging: panel: Remove magic numbers
On Tue, Nov 18, 2014 at 09:56:08PM +0100, Mariusz Gorski wrote: > Get rid of magic numbers indicating that the value of a module param > is not set. Use a defined value instead. > > Signed-off-by: Mariusz Gorski Acked-by: Willy Tarreau ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 2/9] staging: panel: Call init function directly
On Tue, Nov 18, 2014 at 09:56:07PM +0100, Mariusz Gorski wrote: > Remove useless function and let the kernel call the actual > init function directly. > > Signed-off-by: Mariusz Gorski Acked-by: Willy Tarreau ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 4/9] staging: panel: Use a macro for checking module params state
On Tue, Nov 18, 2014 at 09:56:09PM +0100, Mariusz Gorski wrote:
> Avoid values comparison and use a macro instead that checks
> whether module param has been set by the user to some value
> at loading time.
>
> Signed-off-by: Mariusz Gorski
> ---
> drivers/staging/panel/panel.c | 88
> ++-
> 1 file changed, 45 insertions(+), 43 deletions(-)
>
> diff --git a/drivers/staging/panel/panel.c b/drivers/staging/panel/panel.c
> index 1b4a211..97fc4ca 100644
> --- a/drivers/staging/panel/panel.c
> +++ b/drivers/staging/panel/panel.c
> @@ -135,6 +135,8 @@
>
> #define NOT_SET -1
>
> +#define IS_NOT_SET(x)(x == NOT_SET)
> +
> /* macros to simplify use of the parallel port */
> #define r_ctr(x)(parport_read_control((x)->port))
> #define r_dtr(x)(parport_read_data((x)->port))
> @@ -1411,29 +1413,29 @@ static void lcd_init(void)
> switch (lcd_type) {
> case LCD_TYPE_OLD:
> /* parallel mode, 8 bits */
> - if (lcd_proto < 0)
> + if (IS_NOT_SET(lcd_proto))
> lcd_proto = LCD_PROTO_PARALLEL;
> - if (lcd_charset < 0)
> + if (IS_NOT_SET(lcd_charset))
> lcd_charset = LCD_CHARSET_NORMAL;
> if (lcd_e_pin == PIN_NOT_SET)
> lcd_e_pin = PIN_STROBE;
> if (lcd_rs_pin == PIN_NOT_SET)
> lcd_rs_pin = PIN_AUTOLF;
>
> - if (lcd_width < 0)
> + if (IS_NOT_SET(lcd_width))
> lcd_width = 40;
(...)
I'm not convinced at all by the increased readability of this one.
To me it adds obfuscation since I have to look for the macro definition
to see how it checks whether the type is set or not.
I think you'd rather simply open-code the macro here and keep the natural
comparison. The fields were already initialized to the NOT_SET value, better
check agaist the same value here especially since it matches other tests as
well. That would give :
- if (lcd_proto < 0)
+ if (lcd_proto == NOT_SET)
lcd_proto = LCD_PROTO_PARALLEL;
etc... To me it's more readable.
Willy
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 5/9] staging: panel: Start making module params read-only
On Tue, Nov 18, 2014 at 09:56:10PM +0100, Mariusz Gorski wrote: > Start decoupling module params from the actual device state, > both for lcd and keypad, by keeping the params read-only > and moving the device state to related structs. > > Signed-off-by: Mariusz Gorski Acked-by: Willy Tarreau ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 6/9] staging: panel: Make two more module params read-only
On Tue, Nov 18, 2014 at 09:56:11PM +0100, Mariusz Gorski wrote:
> Make keypad_type and lcd_type module params read-only.
> This step also starts making it more clear what is
> the precedence of device params coming from different
> sources (device profile, runtime module param values etc).
>
> Signed-off-by: Mariusz Gorski
> ---
> drivers/staging/panel/panel.c | 71
> ++-
> 1 file changed, 37 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/staging/panel/panel.c b/drivers/staging/panel/panel.c
> index 7f2f5f8..5b4f0a4 100644
> --- a/drivers/staging/panel/panel.c
> +++ b/drivers/staging/panel/panel.c
> @@ -229,6 +229,9 @@ static struct {
> bool enabled;
> } lcd;
>
> +/* Needed only for init */
> +static int selected_lcd_type = NOT_SET;
> +
> /* contains the LCD config state */
> static unsigned long int lcd_flags;
> /* contains the LCD X offset */
> @@ -1417,7 +1420,7 @@ static void panel_lcd_print(const char *s)
> /* initialize the LCD driver */
> static void lcd_init(void)
> {
> - switch (lcd_type) {
> + switch (selected_lcd_type) {
(...)
stupid question : why not move that to the lcd struct you just
created instead of creating a new variable ? It would make sense
to me to have lcd.type here just like you did with enabled.
Same for keypad.
Willy
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 7/9] staging: panel: Refactor LCD init code
On Tue, Nov 18, 2014 at 09:56:12PM +0100, Mariusz Gorski wrote: > Rework lcd_init method to make it a little bit more clear about > the precedence of the params, move LCD geometry and pins layout > to the LCD struct and thus make the LCD-related module params > effectively read-only. > > Signed-off-by: Mariusz Gorski Acked-by: Willy Tarreau I like this refactoring. However I don't know if you got your LCD module to work or not, but for such important changes you should definitely test the code, since it's very easy to introduce minor bugs or even to fix a bug that used to make the whole driver work... Willy ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 8/9] staging: panel: Remove more magic number comparison
On Tue, Nov 18, 2014 at 09:56:13PM +0100, Mariusz Gorski wrote: > Use a macro instead of magic number comparison for checking > whether a module param value has been set. > > Signed-off-by: Mariusz Gorski > --- > drivers/staging/panel/panel.c | 21 +++-- > 1 file changed, 11 insertions(+), 10 deletions(-) > > diff --git a/drivers/staging/panel/panel.c b/drivers/staging/panel/panel.c > index ee48bca..268ad2e 100644 > --- a/drivers/staging/panel/panel.c > +++ b/drivers/staging/panel/panel.c > @@ -136,6 +136,7 @@ > #define NOT_SET -1 > > #define IS_NOT_SET(x)(x == NOT_SET) > +#define IS_SET(x)(x > NOT_SET) > > /* macros to simplify use of the parallel port */ > #define r_ctr(x)(parport_read_control((x)->port)) > @@ -1496,17 +1497,17 @@ static void lcd_init(void) > } > > /* Overwrite with module params set on loading */ > - if (lcd_height > -1) > + if (IS_SET(lcd_height)) > lcd.height = lcd_height; > - if (lcd_width > -1) > + if (IS_SET(lcd_width)) > lcd.width = lcd_width; (...) Same as for the other patch, better open-code the test for readability : - if (lcd_height > -1) + if (lcd_height != NOT_SET) lcd.height = lcd_height; Note that we take the freedom to change the operator since we only want to check equality and not sign in practice. Willy ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 9/9] staging: panel: Move LCD-related state into struct lcd
On Tue, Nov 18, 2014 at 09:56:14PM +0100, Mariusz Gorski wrote: > Move more or less all LCD-related state into struct lcd > in order to get better cohesion; use bool instead of int > where it makes sense. > > Signed-off-by: Mariusz Gorski Acked-by: Willy Tarreau ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 6/9] staging: panel: Make two more module params read-only
On Tue, Nov 18, 2014 at 10:50:35PM +0100, Mariusz Gorski wrote:
> On Tue, Nov 18, 2014 at 10:20:34PM +0100, Willy Tarreau wrote:
> > On Tue, Nov 18, 2014 at 09:56:11PM +0100, Mariusz Gorski wrote:
> > > Make keypad_type and lcd_type module params read-only.
> > > This step also starts making it more clear what is
> > > the precedence of device params coming from different
> > > sources (device profile, runtime module param values etc).
> > >
> > > Signed-off-by: Mariusz Gorski
> > > ---
> > > drivers/staging/panel/panel.c | 71
> > > ++-
> > > 1 file changed, 37 insertions(+), 34 deletions(-)
> > >
> > > diff --git a/drivers/staging/panel/panel.c b/drivers/staging/panel/panel.c
> > > index 7f2f5f8..5b4f0a4 100644
> > > --- a/drivers/staging/panel/panel.c
> > > +++ b/drivers/staging/panel/panel.c
> > > @@ -229,6 +229,9 @@ static struct {
> > > bool enabled;
> > > } lcd;
> > >
> > > +/* Needed only for init */
> > > +static int selected_lcd_type = NOT_SET;
> > > +
> > > /* contains the LCD config state */
> > > static unsigned long int lcd_flags;
> > > /* contains the LCD X offset */
> > > @@ -1417,7 +1420,7 @@ static void panel_lcd_print(const char *s)
> > > /* initialize the LCD driver */
> > > static void lcd_init(void)
> > > {
> > > - switch (lcd_type) {
> > > + switch (selected_lcd_type) {
> >
> > (...)
> >
> > stupid question : why not move that to the lcd struct you just
> > created instead of creating a new variable ? It would make sense
> > to me to have lcd.type here just like you did with enabled.
> > Same for keypad.
> >
> > Willy
> >
>
> I get your point here. This var is here only because it's set
> in init_panel_module() and then used in lcd_init(). So it's needed
> really only for the init code. It doesn't directly_ describe the
> lcd's state, so I decided to keep it out.
OK but isn't it the same for some of the other variables you moved there ?
Anyway that's not important, I have nothing against this, it was just a
question.
Willy
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 7/9] staging: panel: Refactor LCD init code
On Tue, Nov 18, 2014 at 10:51:08PM +0100, Mariusz Gorski wrote: > On Tue, Nov 18, 2014 at 10:23:26PM +0100, Willy Tarreau wrote: > > On Tue, Nov 18, 2014 at 09:56:12PM +0100, Mariusz Gorski wrote: > > > Rework lcd_init method to make it a little bit more clear about > > > the precedence of the params, move LCD geometry and pins layout > > > to the LCD struct and thus make the LCD-related module params > > > effectively read-only. > > > > > > Signed-off-by: Mariusz Gorski > > > > Acked-by: Willy Tarreau > > > > I like this refactoring. However I don't know if you got your LCD module > > to work or not, but for such important changes you should definitely test > > the code, since it's very easy to introduce minor bugs or even to fix a > > bug that used to make the whole driver work... > > > > Willy > > > Yes, I have tested this code with my LCD module and it works > as before. great! > This is what I've emphasized in the cover letter - > - I made sure (and tested), that all lcd and keypad params are > set to the same state as before this patch series, and that > the current behaviour of the module doesn't change. I've tested > all profile/lcd_type combinations in an automated way to catch > any edge cases :) "tested" was not mentionned in the cover letter, hence my asking :-) Willy ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 4/9] staging: panel: Use defined value or checking module params state
On Wed, Nov 19, 2014 at 09:38:46PM +0100, Mariusz Gorski wrote: > Avoid magic number and use a comparison with a defined value instead > that checks whether module param has been set by the user to some > value at loading time. > > Signed-off-by: Mariusz Gorski Acked-by: Willy Tarreau ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 6/9] staging: panel: Make two more module params read-only
On Wed, Nov 19, 2014 at 09:38:48PM +0100, Mariusz Gorski wrote: > Make keypad_type and lcd_type module params read-only. > This step also starts making it more clear what is > the precedence of device params coming from different > sources (device profile, runtime module param values etc). > > Signed-off-by: Mariusz Gorski Acked-by: Willy Tarreau ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 8/9] staging: panel: Remove more magic number comparison
On Wed, Nov 19, 2014 at 09:38:50PM +0100, Mariusz Gorski wrote: > Use a defined value instead of magic number comparison > for checking whether a module param value has been set. > > Signed-off-by: Mariusz Gorski Acked-by: Willy Tarreau ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 7/9] staging: panel: Refactor LCD init code
On Wed, Nov 19, 2014 at 09:38:49PM +0100, Mariusz Gorski wrote: > Rework lcd_init method to make it a little bit more clear about > the precedence of the params, move LCD geometry and pins layout > to the LCD struct and thus make the LCD-related module params > effectively read-only. > > Signed-off-by: Mariusz Gorski Acked-by: Willy Tarreau ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 0/9] staging: panel: Refactor panel initialization
Hi Mariusz, On Wed, Nov 19, 2014 at 09:38:42PM +0100, Mariusz Gorski wrote: > This set of patches focuses on making current initialization > process easier to understand - especially it tries to emphasize > what are the priorities of the params coming from different > sources (Kconfig values, device profiles and module param values > set on loading). I paid attention not to change to behaviour of > the code itself (at least for now), so all hacky places are kept. > > v2: Don't introduce new macros for param value check (...) Great work, the code is already significantly cleaner now, thanks for doing this work! I've acked all the new patches. Best regards, Willy ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 4/9] staging: panel: Use defined value or checking module params state
On Thu, Nov 27, 2014 at 07:57:06AM -0800, Greg Kroah-Hartman wrote: > And the reason I got confused was because you didn't label your second > set of patches "v2", which it was, I saw two separate series, one with a > few patches, and then 2 sets of 9, the second set labeled "v2" so I > thought they were independant. Please think of the poor maintainer who > has to decipher things like this when you send them out... For Mariusz's defense, this was his first batch. He didn't feel comfortable and asked me how to proceed when sending a series and I forgot to warn him about the "v2" as initially I didn't think there would be a v2, and after I obviously forgot I didn't speak about that. So I share some responsibility for this one. Mariusz, if you need some help, tell me so. Regards, Willy ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 4/9] staging: panel: Use defined value or checking module params state
On Thu, Nov 27, 2014 at 08:50:55PM +0100, Mariusz Gorski wrote: > > And the reason I got confused was because you didn't label your second > > set of patches "v2", which it was, I saw two separate series, one with a > > few patches, and then 2 sets of 9, the second set labeled "v2" so I > > thought they were independant. Please think of the poor maintainer who > > has to decipher things like this when you send them out... > > I'm confused right now. As you say, first I've sent a patchset of 4: > https://lkml.org/lkml/2014/11/11/963 > > Then, a couple of days later, I've sent the initial patchset of 9: > https://lkml.org/lkml/2014/11/18/922 > > And a day I've sent a fixed version of the above patchset, labeled with v2: > https://lkml.org/lkml/2014/11/19/653 > > Isn't this the right way to do? I still don't get my mistake. Because > what I was just about to do is to resend the v2 patchset, but now I'm > not sure anymore if this is what I'm supposed to do. > > BTW: Out of these 3 patchsets, 1st and 3rd should be applied. Mariusz, for people who have to parse hundreds to thousands of e-mails a day, dealing with non-trivial operation modes like this is never easy. I think (I'll let Greg suggest what he prefers) that the most reliable thing to do *right now* is to rebase your tree on top of Greg's staging tree, and you send the resulting series (what you apply *after* staging) at once, maybe even tagged as v3 to avoid any confusion. Sometimes for the recipient, things apparently as simple as sorting e-mails by subjects to find something can cause some confusion when it's not obvious what replaces what, and tagging with the version or ensuring that each series is different enough helps avoiding this. If you need any help, contact me off-list and I'll gladly help you. Don't worry, issues like this commonly happen and will happen again, whatever you do against them will only reduce the likeliness that they happen again (and that's important to care about this) :-) Thanks, Willy ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: panel: Remove unused variable
On Wed, Dec 03, 2014 at 07:53:47PM +0100, Mariusz Gorski wrote: > Remove lcd.left_shift because it is only written to at some places but > never read from. Good catch, I think some code got dropped at some point because it was used to shift the window of displayed text. Anyway nobody seems to be missing it :-) Acked-by: Willy Tarreau Willy ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: panel: Remove magic numbers in LCD commands
On Fri, Dec 05, 2014 at 10:10:43PM +0100, Mariusz Gorski wrote: > Get rid of magic numbers in LCD commands and replace them with defined > values, so that it's more obvious that the commands are doing. > > Signed-off-by: Mariusz Gorski I didn't remember I wrote something that awful! Obvious ack. Acked-by: Willy Tarreau Thanks, willy ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: speakup: separate 80+ chars lines.
On Thu, Apr 19, 2018 at 03:47:10AM -0300, Andrew Jye Shih Chuang wrote:
> Increase readability of code following the Kernel coding style by breaking
> long lines and thus eliminating the checkpatch.pl warning.
>
> Signed-off-by: Andrew Jye Shih Chuang
> ---
> drivers/staging/speakup/main.c | 15 +--
> 1 file changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/staging/speakup/main.c b/drivers/staging/speakup/main.c
> index af30b70..2ba9989 100644
> --- a/drivers/staging/speakup/main.c
> +++ b/drivers/staging/speakup/main.c
> @@ -899,12 +899,13 @@ static int get_sentence_buf(struct vc_data *vc, int
> read_punc)
> while (start < end) {
> sentbuf[bn][i] = get_char(vc, (u_short *)start, &tmp);
> if (i > 0) {
> - if (sentbuf[bn][i] == SPACE && sentbuf[bn][i - 1] ==
> '.' &&
> + if (sentbuf[bn][i] == SPACE &&
> + sentbuf[bn][i - 1] == '.' &&
> numsentences[bn] < 9) {
> /* Sentence Marker */
> numsentences[bn]++;
> sentmarks[bn][numsentences[bn]] =
> - &sentbuf[bn][i];
> + &sentbuf[bn][i];
> }
> }
FWIW I personally feel that this multi-line condition block becomes even less
readable than it already was, and is one example where it's possibly better
to ignore the limit or organize the code different. Just as an example, this
one doing the same already looks more readable to me :
if (i > 0 && numsentences[bn] < 9 &&
sentbuf[bn][i] == SPACE && sentbuf[bn][i - 1] == '.') {
/* Sentence Marker */
numsentences[bn]++;
sentmarks[bn][numsentences[bn]] = &sentbuf[bn][i];
}
Willy
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: panel: remove duplicate code
On Tue, Apr 07, 2015 at 12:12:06PM +0200, Greg Kroah-Hartman wrote: > On Tue, Apr 07, 2015 at 03:26:58PM +0530, Sudip Mukherjee wrote: > > On Tue, Apr 07, 2015 at 11:44:29AM +0200, Greg Kroah-Hartman wrote: > > > On Tue, Apr 07, 2015 at 02:55:02PM +0530, Sudip Mukherjee wrote: > > > > On Tue, Apr 07, 2015 at 11:49:30AM +0300, Dan Carpenter wrote: > > > > > On Tue, Apr 07, 2015 at 01:55:01PM +0530, Sudip Mukherjee wrote: > > > > > > > > > > I hadn't looked at this driver much before. It sucks that > > > > > parport_driver ->attach() functions can't fail... > > > > > > > > then maybe, we can change the code of parport. currently attach and > > > > parport_register_driver never fails. we can modify it so that if attach > > > > fails then parport_register_driver will also fail. will not be that much > > > > difficult as it has been used only in 13 places. > > > > your views ? > > > > > > > > and since we are discussing parallel ports, few days back i saw one > > > > post in ubuntuforums that his scanner is not working because of > > > > lack of ppscsi.I mailed Tim Waugh, but he is not interested to work > > > > with ppscsi anymore. parallel port scanners are almost a thing of the > > > > past > > > > now. do you think it is worth that i pick up the code and modify > > > > it for our latest kernel and submit to Greg ? > > > > > > If you have some parport hardware, and want to take it on, that would be > > > great. The code needs a maintainer, and the apis are _really_ old and > > > messy, as you have found out. > > what kind of parport hardware will you suggest? > > No idea, go look at the drivers and code to figure that out. Not-so-old laptops used to have parport connectors for a while, and can be found at a very cheap price or even for free by now with dead batteries. I've been using mine as a JTAG connector exclusively until I replaced this laptop. A number of old mini-itx boards used to have a parallell port and were easy to hack on (eg: those with a jack power connector, onboard video and a PXE capable NIC to boot the freshly built kernel over the network). They're less easy to find though. If you want to run on your PC, you'll have to order a PCIe parallel port NIC I guess. Cheers, Willy ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: panel: remove duplicate code
On Tue, Apr 07, 2015 at 07:51:17PM +0530, Sudip Mukherjee wrote: > On Tue, Apr 07, 2015 at 04:14:59PM +0200, Willy Tarreau wrote: > > Not-so-old laptops used to have parport connectors for a while, and can be > > found at a very cheap price or even for free by now with dead batteries. > free laptop sounds good.. :) > where can i get some? Friends replacing theirs in general, or your employer's dead stock :-) > > If you want to run on your PC, you'll have to order a PCIe parallel port NIC s/NIC/card above > > I guess. > i already have pc with parallel port, and on that i check your panel > codes before sending. but here we were discussing about the required > hardware to test ppscsi module. Ah sorry for being out of topic then, I missed this point, I thought it was for PP in general. Regards, Willy ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 01/14] parport: return value of attach and parport_register_driver
On Wed, Apr 08, 2015 at 03:27:37PM +0300, Dan Carpenter wrote: > On Wed, Apr 08, 2015 at 05:20:10PM +0530, Sudip Mukherjee wrote: > > On Wed, Apr 08, 2015 at 02:38:32PM +0300, Dan Carpenter wrote: > > > 1) We can't apply this patch on its own so this way of breaking up the > > > patches doesn't work. > > yes, if the first patch is reverted for any reason all the others need > > to be reverted also. so then everything in one single patch? > > The problem is that patch 1/1 breaks the build. The rule is that we > should be able to apply part of a patch series and nothing breaks. If > we apply the patch series out of order than things break that's our > problem, yes. But if we apply only 1/1 and it breaks, that's a problem > with the series. Yep, keep in mind that "git bisect" will randomly land in the middle of any set of patches during a debugging session and it could very well land on this one. If it breaks, that's a real pain for the people trying to bisect their bug because suddenly they have to deal with a second bug totally different from theirs. Regards, Willy ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: panel: fix stackdump
On Mon, May 11, 2015 at 04:42:19PM +0530, Sudip Mukherjee wrote: > if we load the module, unload and then again try to load the module, we > will get a stackdump. In the module_exit function we are unregistering > the device and releasing the parport. So when we reach the detach > function parport is already null and the unregister_reboot_notifier() > is never called. When we again try to load the module it again tries > register_reboot_notifier() and gives us a stackdump as its earlier > registration is still not removed. > Fix this by moving all the unregistering and releasing in the detach > function, which should be the ideal case as the detach will be called if > we try to unregister the driver or if the parport is removed. > > Signed-off-by: Sudip Mukherjee > --- > > This was caused by one of my patch :( > Faced this problem while working on the device-model code of > parallelport. Introducing regressions (and fixing them) is part of the job :-) However please mention in the commit message the commit of your other patch which introduced the regression, and if you noticed it was backported to stable, please mention it as well by a Cc: stable and the idea of the commit. That will help everyone involved in the chain. Thanks, Willy ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [RFC PATCH] checkpatch: check for 2 or more spaces around assignment of a declaration
On Wed, May 20, 2015 at 06:02:06PM -0700, Joe Perches wrote: > Perhaps space aligned declarations should have some > checkpatch --strict warning for 2 or more spaces > around any assignment of the declaration. > > int a = 1;// 2+ spaces before = > int b = 2; // 2+ spaces after = > int c = 3; // uses tabs around =, no warning > int d = 4;// uses 2 tabs before =, no warning FWIW, in my own code I've stopped using tabs for this and replaced them with spaces. Tabs are fine *before* code but they completely mangle the display for people using different tab sizes, or even when reading diffs, because they heavily depend on the number of characters *before* them while what you want is to ensure that what is *after* is on a fixed position. I would even go further and report warnings when tabs are used after text. Tabs after text were very useful in ASM editors in the past because it was possible to define 3 fixed positions (mnemonic, operands, comments) and that was the primary purpose of tabs. But in todays editors, tabs do not mean "jump to next position" but "add between 1 and 8 to the current position" which doesn't make sense at all if what preceeds can be larger than 8 (which is most often the case in C code). Willy ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 6/6] MAINTAINERS: maintain parport
On Wed, May 20, 2015 at 05:46:44PM +0200, Richard Weinberger wrote: > On Wed, May 20, 2015 at 5:27 PM, Sudip Mukherjee > wrote: > > Lets give the parport subsystem a proper name and start > > maintaining the files. > > Excuse me, but usually someone takes over the maintainer role after > proving that he > cares for a sub system for a certain period of time. > Or did I miss something? Sudip has been volunteering for fixing this code which is marked as "orphan". So whatever his experience and care in this area, it will be better for the driver to have a maintainer than none. Good luck Sudip, as drivers which have become orphans are probably the ones that deserve the least love :-) Willy ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: panel: use schedule_timeout_interruptible()
Hi Nicholas, On Fri, May 29, 2015 at 07:02:32PM +0200, Nicholas Mc Guire wrote: > API consolidation with coccinelle found: > ./drivers/staging/panel/panel.c:782:2-18: > consolidation with schedule_timeout_*() recommended > > This is a 1:1 conversion with respect to schedule_timeout() to the > schedule_timeout_interruptible() helper only - so only an API > consolidation to improve readability. The timeout was being passed > as (ms * HZ + 999) / 1000 but that simply looks wrong - rather than > "manual" converting to jiffies, msecs_to_jiffies which handles all > corner-cases correctly, was used. > > Patch was compile tested with x86_64_defconfig + CONFIG_STAGING=y, > CONFIG_PARPORT=m, CONFIG_PANEL=m > > Signed-off-by: Nicholas Mc Guire Acked-by: Willy Tarreau > --- > Patch is against 4.1-rc5 (localversion-next is -next-20150529) > > not really clear what the intent of (ms * HZ + 999) / 1000 was - this > is HZ dependent and does not really make sense - the comment states > "sleeps that many milliseconds" so it probably simply should be > msecs_to_jiffies(ms) - but someone that knows the intention of this code > needs to check this. Oh it's very simple, we call this a bug :-) The code was written for kernel 2.2 and by then there was no msecs_to_jiffies(). I did a mistake with this +999, the purpose was to round up, but it's not 999 that should have been used, but HZ-1 since the result is supposed to be in units of HZ. Thanks for fixing this one! Willy ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging : panel : fix checkpatch warnings (linux-next)
On Thu, Apr 03, 2014 at 06:59:09PM +0200, armand.bast...@laposte.net wrote: > This patch fixes "Missing a blank line after declarations" checkpatch warning > in panel.c. Acked-By: Willy Tarreau Thanks Bastien for this. Willy ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: system hang
On Thu, Apr 10, 2014 at 12:05:52AM -0700, narasimharo bolisetti wrote: > My kernel version is: > 2.6.35.6-45.fc14.i686 Maintenance for this kernel has been dropped years ago, so it very likely contains many bugs that were fixed in more recent ones. You should definitely upgrade. Hoping this helps, Willy ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 0/2] staging: panel: fix sparse warnings.
Hi Bastien, On Fri, Apr 18, 2014 at 06:09:18PM +0200, Bastien Armand wrote: > This serie of two patch fix the following sparse warnings in panel.c : > panel.c:1235:26: warning: incorrect type in argument 1 (different address > spaces) > panel.c:1235:26:expected void const volatile [noderef] * > panel.c:1235:26:got char const *tmp > panel.c:1353:20: warning: incorrect type in initializer (incompatible > argument 2 (different address spaces)) > panel.c:1353:20:expected int ( *write )( ... ) > panel.c:1353:20:got int ( static [toplevel] * )( ... ) > panel.c:1591:17: warning: incorrect type in argument 1 (different address > spaces) > panel.c:1591:17:expected void const volatile [noderef] * > panel.c:1591:17:got char *tmp > panel.c:1620:20: warning: incorrect type in initializer (incompatible > argument 2 (different address spaces)) > panel.c:1620:20:expected int ( *read )( ... ) > panel.c:1620:20:got int ( static [toplevel] * )( ... ) > > Changes since v1 : > - splitted patch in two > > Bastien Armand (2): > staging: panel: fix sparse warnings in keypad_read > staging: panel: fix sparse warnings in lcd_write Just reviewed them both and I'm fine with these patches. You can add my : Acked-by: Willy Tarreau Willy ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 2/2] staging: panel: fix sparse warnings in lcd_write
On Wed, Apr 23, 2014 at 07:35:02PM +0200, Bastien Armand wrote:
> On Tue, Apr 22, 2014 at 01:01:45PM +0300, Dan Carpenter wrote:
> > Btw, this whole function is terrible. It should be reading larger
> > chunks at once instead of get_user() for each character.
Just for the record, very small character counts may be sent to an LCD
panel, in general these are 2 lines of 16 characters, and at most 2x40,
and changes are very rare. The worst case will be if someone displays
the time of day, it will change every second.
> > > + if (lcd_enabled && lcd_initialized) {
> > > + for (; count-- > 0; tmp++) {
> > > + if (!in_interrupt() && (((count + 1) & 0x1f) == 0))
> > > + /* let's be a little nice with other processes
> > > +that need some CPU */
> > > + schedule();
> >
> > This schedule() isn't needed. It just prints a line or two at start up
> > and some other debug output or something. Small small.
> >
>
> I hesitated to remove it. I leave it here as it was allready in lcd_write.
> Perhaps, I could send another patch to remove it.
I believe it can go. I have some memories of it when I was developing the
driver because I didn't know if some LCDs would need long pauses between
each character. Hmmm well, thinking about it after re-reading the code,
we could wait up to 20+40+120 = 180 microseconds when sending one command
(eg: position change), or 20+40+45 = 105 microseconds when sending one
char. That's basically 180+40*105 = 4.185 milliseconds for one full line,
or 8 milliseconds for two lines of 40 chars. So maybe we should keep the
schedule() in the end...
Best regards,
Willy
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] Staging: panel: Fixed a spacing after cast coding style issue
On Tue, Nov 10, 2015 at 07:35:30PM +0100, Daniel H. Hemmingsen wrote: > Fixed a spacing after cast coding style issue. > > Signed-off-by: Daniel H. Hemmingsen Acked-by: Willy Tarreau Thanks, Willy ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/1] staging: coding style cleanups for staging/panel driver
On Fri, Dec 18, 2015 at 02:48:58PM +0530, Bijosh T wrote: > From: Bijosh T > > This patch fixes coding style errors for staging/panel driver. > > Signed-off-by: Bijosh T Acked-by: Willy Tarreau Thanks Bijosh, Willy ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] staging: update panel driver's author address
I'm realizing that I sometimes miss e-mails sent to my old address, so better update it. Since I only have the relevant hardware at my work place, let's use this address so that I can test code if needed. Signed-off-by: Willy Tarreau --- MAINTAINERS| 2 +- drivers/staging/panel/TODO | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/MAINTAINERS b/MAINTAINERS index 9bff63c..456148a 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -10178,7 +10178,7 @@ S: Maintained F: drivers/staging/olpc_dcon/ STAGING - PARALLEL LCD/KEYPAD PANEL DRIVER -M: Willy Tarreau +M: Willy Tarreau S: Odd Fixes F: drivers/staging/panel/ diff --git a/drivers/staging/panel/TODO b/drivers/staging/panel/TODO index 2db3f99..3a6405a 100644 --- a/drivers/staging/panel/TODO +++ b/drivers/staging/panel/TODO @@ -5,4 +5,4 @@ TODO: - see if all of this could be easier done in userspace instead. Please send patches to Greg Kroah-Hartman and -Willy Tarreau +Willy Tarreau -- 1.7.12.1 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/1] staging: coding style cleanups for staging/panel driver
On Sat, Dec 19, 2015 at 11:55:13AM +0530, Sudip Mukherjee wrote: > On Fri, Dec 18, 2015 at 08:01:58AM -0800, Greg KH wrote: > > On Fri, Dec 18, 2015 at 02:48:58PM +0530, Bijosh T wrote: > > > From: Bijosh T > > > > > > This patch fixes coding style errors for staging/panel driver. > > > > > > Signed-off-by: Bijosh T > > > > I need a "full" name here, not just "T" as a last name, as odds are it's > > a bit longer than just that one character... > > > > Please fix up and resend. > > While you are resending please also mention which coding style error you > have fixed. Well, they are so minor (mostly spaces) that anyone interested should read the patch. Willy ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: Move panel driver out of staging?
Hi Ksenija, On Mon, Dec 28, 2015 at 12:32:39PM +0100, Ksenija Stanojevi?? wrote: > Hi Willy, > > I'm helping Greg do a bit of cleanup in the staging tree, I noticed that > panel driver is maybe ready to be moved to drivers/misc. I think so as well, we discussed this with Greg 6 months ago or so but none of us had the time to re-check. > Are there any TODO tasks left to do? I think we're fine now. > I already sent checkpatch clean-up patches. I've seen them. Thanks for this. I believe the second one was also proposed by someone else a week ago, but that's not a problem. Thanks, Willy ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: Move panel driver out of staging?
Hi Dan,
Thanks for your review, I'm adding a few extra comments below.
On Mon, Dec 28, 2015 at 06:27:36PM +0300, Dan Carpenter wrote:
> On Mon, Dec 28, 2015 at 12:32:39PM +0100, Ksenija Stanojevi?? wrote:
> > Hi Willy,
> >
> > I'm helping Greg do a bit of cleanup in the staging tree, I noticed that
> > panel driver is maybe ready to be moved to drivers/misc. Are there any
> > TODO tasks left to do? I already sent checkpatch clean-up patches.
> >
>
> I feel like lcd_write_data() should take a u8 instead of an int. Or
> possibly a char, I suppose.
Looks like so indeed.
> Could we tighten the checking in input_name2mask() a bit?
>
> drivers/staging/panel/panel.c
> 2044 static int input_name2mask(const char *name, pmask_t *mask, pmask_t
> *value,
> 2045 char *imask, char *omask)
> 2046 {
> 2047 static char sigtab[10] = "EeSsPpAaBb";
> 2048 char im, om;
>
> Om is 8 bits (signed or not depending on the arch).
>
> 2049 pmask_t m, v;
I don't know what pmask_t is but I think it should be an opportunity to get
rid of it if it's a scalar.
> 2050
> 2051 om = 0ULL;
> 2052 im = 0ULL;
> 2053 m = 0ULL;
> 2054 v = 0ULL;
ULL is useless here BTW.
> 2055 while (*name) {
> 2056 int in, out, bit, neg;
> 2057
> 2058 for (in = 0; (in < sizeof(sigtab)) && (sigtab[in] !=
> *name);
> 2059 in++)
> 2060 ;
The 80-chars limit makes this statement even more confusing than needed.
Possibly it should be broken into 3 different lines.
> 2061
> 2062 if (in >= sizeof(sigtab))
> 2063 return 0; /* input name not found */
> 2064 neg = (in & 1); /* odd (lower) names are negated */
> 2065 in >>= 1;
> 2066 im |= BIT(in);
> 2067
> 2068 name++;
> 2069 if (isdigit(*name)) {
> 2070 out = *name - '0';
> 2071 om |= BIT(out);
> ^^
> out is 0-9 so it's too much for "om". I don't know if this causes a
> problem, but let's remove the question by adding a check for illegal
> values.
> if (*name >= '0' && *name <= '7') {
It's very old memories for me now but looking at this code I guess these
are the digits corresponding to the data bits of the parallel port. Thus
indeed such a control is needed to remove any doubt.
> 2072 } else if (*name == '-') {
> 2073 out = 8;
> 2074 } else {
> 2075 return 0; /* unknown bit name */
> 2076 }
> 2077
> 2078 bit = (out * 5) + in;
> 2079
> 2080 m |= 1ULL << bit;
> 2081 if (!neg)
> 2082 v |= 1ULL << bit;
We can remove ULL here and there as well I guess.
> 2083 name++;
> 2084 }
> 2085 *mask = m;
> 2086 *value = v;
> 2087 if (imask)
> 2088 *imask |= im;
> 2089 if (imask)
> 2090 *imask |= im;
> 2091 if (omask)
> 2092 *omask |= om;
>
> It's too much for omask also.
Yes, let's have om and friends be declared u8 to remove any confusion if
that describes the 8 bits of the data bus on the parallel port. It will
be much saner.
Ksenija, are you interested in trying to address this ?
Thanks!
Willy
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 3/5] Staging: panel: Remove ULL
On Tue, Dec 29, 2015 at 03:59:26PM -0500, Ilia Mirkin wrote:
> On Tue, Dec 29, 2015 at 3:08 PM, Ksenija Stanojevic
> wrote:
> > Remove ULL since it's useless.
> >
> > Signed-off-by: Ksenija Stanojevic
> > ---
> > drivers/staging/panel/panel.c | 12 ++--
> > 1 file changed, 6 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/staging/panel/panel.c b/drivers/staging/panel/panel.c
> > index 7138ee7..6dc3efb 100644
> > --- a/drivers/staging/panel/panel.c
> > +++ b/drivers/staging/panel/panel.c
> > @@ -2047,10 +2047,10 @@ static u8 input_name2mask(const char *name, __u64
> > *mask, __u64 *value,
> > u8 im, om;
> > __u64 m, v;
> >
> > - om = 0ULL;
> > - im = 0ULL;
> > - m = 0ULL;
> > - v = 0ULL;
> > + om = 0;
> > + im = 0;
> > + m = 0;
> > + v = 0;
> > while (*name) {
> > int in, out, bit, neg;
> >
> > @@ -2076,9 +2076,9 @@ static u8 input_name2mask(const char *name, __u64
> > *mask, __u64 *value,
> >
> > bit = (out * 5) + in;
> >
> > - m |= 1ULL << bit;
> > + m |= 1 << bit;
>
> m and v are 64-bit, if bit can be >= 32, then you definitely do need
> the ULL here...
If that's it, indeed. I thought they were 8-bit from a previous patch.
I'm sorry for having suggested this cleanup, I've not put my nose in
this code for quite a while I must confess :-/
Regards,
Willy
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 5/5] Staging: panel: Make statement more readable
Hi Ksenija,
several points, see below.
On Tue, Dec 29, 2015 at 09:11:03PM +0100, Ksenija Stanojevic wrote:
> Broke statement into 3 different lines to make it more readable.
>
> Signeded-off-by: Ksenija Stanojevic
^^
Extra "ed". Don't waste your time typing it by hand, several git commands
(including git-commit and git-format-patch) will do it for you when you
pass "-s".
> diff --git a/drivers/staging/panel/panel.c b/drivers/staging/panel/panel.c
> index 70cb9f3..207d8d5 100644
> --- a/drivers/staging/panel/panel.c
> +++ b/drivers/staging/panel/panel.c
> @@ -2054,8 +2054,8 @@ static u8 input_name2mask(const char *name, __u64
> *mask, __u64 *value,
> while (*name) {
> int in, out, bit, neg;
>
> - for (in = 0; (in < sizeof(sigtab)) && (sigtab[in] != *name);
> - in++)
> + for (in = 0;
> + (in < sizeof(sigtab)) && (sigtab[in] != *name); in++)
> ;
>
> if (in >= sizeof(sigtab))
well, this one is still ugly in my opinion. I've just looked at the code
and it was crap originally :
- sigtab[] is declared as static and without a trailing zero
- the for loop uses extra parenthesis and basically only reimplements
strchr()
- the end condition is tested again after the for loop
=> I'd rather use strchr(), and clean up this part, approx like this, but
do it as you want :
- static char sigtab[10] = "EeSsPpAaBb";
+ const char sigtab[] = "EeSsPpAaBb";
...
while (*name) {
int in, out, bit, neg;
+ const char *idx;
- for (in = 0; (in < sizeof(sigtab)) && (sigtab[in] != *name);
-in++)
-
- if (in >= sizeof(sigtab))
- return 0;
+ idx = strchr(sigtab, *name);
+ if (!idx)
+ return 0;
+
+ in = idx - sigtab;
I think it's more readable this way.
Regards,
Willy
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] Staging: panel: Fix sparse warnings (line length, space after a cast) in panel.c
Hello Arthur, On Wed, Jan 20, 2016 at 03:09:20PM -0600, Arthur Marble wrote: > This patch fixes 'Lines longer than 80 columns' and 'No space is necessary > after > a cast' warnings in /drivers/staging/panel/panel.c. Thank you but this one has already been proposed twice and I think the last one was queued with a few more changes as well recently. I guess some time is needed to move that queue to mainline, I don't know more unfortunately :-/ Thanks, Willy ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] Revert "Staging: panel: usleep_range is preferred over udelay"
On Mon, Jan 25, 2016 at 06:19:44PM +0530, Sudip Mukherjee wrote: > On Mon, Jan 25, 2016 at 03:40:41PM +0300, Dan Carpenter wrote: > > Ying, could you CC the subsystem list for these reports? This one was > > CC'd to Sirnam, Greg and LKML. Sirnam is too new to understand what > > they mean, Greg is too busy, and only Sudip and Alan Cox read LKML. > > I only read the mails whose subject is interesting and something which > I can understand. I am also still new to understand many of the things. > I am sure Alan, Greg, Linus, they will read all the mails. You could be disappointed then. Linus has always said that he doesn't read it and even pretends he's not even subscribed. Greg probably doesn't have the time given that he's flooded with the stable@ messages and virtually every e-mail on the kernel where he's CC'd like this one. As for Alan I'm not even sure he still manages to catch up with this volume. I personally stopped 7 years ago after I didn't have this dedicated display constantly on it anymore, and since then the traffic has doubled. And by then I was only reading the subjects... However LKML is great as a searchable public archive. Willy ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3] misc: Move panel driver out of staging
On Tue, Feb 02, 2016 at 12:59:50PM -0800, Greg KH wrote: > On Tue, Feb 02, 2016 at 07:39:02PM +0100, Ksenija Stanojevic wrote: > > Move panel driver from drivers/staging/panel to drivers/misc. > > > > Signed-off-by: Ksenija Stanojevic > > --- > > Changes in v3: > > - modify driver location in MAINTAINERS file > > > > Changes in v2: > > - modify MAINTAINERS file > > - move lcd-panel-cgram.txt to Documentation/ > > I'd like to get an ack from Willy about this patch as well, before I can > accept it. Sorry for the delay, still trying to catch up with my e-mails. Acked-by: Willy Tarreau Thanks, willy ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: panel: Fix coding style issue.
On Wed, Aug 05, 2015 at 10:43:23PM +0300, Lior Pugatch wrote: > This patch fixes checkpatch.pl warning, > all warnings are releted to comments code style. Acked-by: Willy Tarreau Thanks! Willy ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 2/2] staging: panel: initialize lcd if lcd enabled
On Tue, Feb 10, 2015 at 05:26:03PM +0530, Sudip Mukherjee wrote:
> initialiaze lcd parameters only if lcd is enabled.
>
> Signed-off-by: Sudip Mukherjee
One minor comment below (cosmetic), but after this it's OK.
Acked-by: Willy Tarreau
> + if (lcd.enabled) {
> + /*
> + * Init lcd struct with load-time values to preserve exact
> + * current functionality (at least for now).
> + */
> + lcd.height = lcd_height;
> + lcd.width = lcd_width;
> + lcd.bwidth = lcd_bwidth;
> + lcd.hwidth = lcd_hwidth;
> + lcd.charset = lcd_charset;
> + lcd.proto = lcd_proto;
> + lcd.pins.e = lcd_e_pin;
> + lcd.pins.rs = lcd_rs_pin;
> + lcd.pins.rw = lcd_rw_pin;
> + lcd.pins.cl = lcd_cl_pin;
> + lcd.pins.da = lcd_da_pin;
> + lcd.pins.bl = lcd_bl_pin;
> +
> + /* Leave it for now, just in case */
^^^
Please fix indenting the comment here.
> + lcd.esc_seq.len = -1;
> + }
> +
Willy
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/2] staging: panel: register driver after checking device
On Tue, Feb 10, 2015 at 05:26:02PM +0530, Sudip Mukherjee wrote: > register the driver only if lcd or keypad has been enabled and if > both are disabled then just exit. > > Signed-off-by: Sudip Mukherjee Acked-by: Willy Tarreau Willy ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/3] staging: panel: register reboot
On Sun, Mar 08, 2015 at 11:07:24PM +0530, Sudip Mukherjee wrote: > we donot need the reboot notifier in module init section, as the > notifier is used after lcd is initialized. so lets register for the > reboot notifier only after we have successfully attached to the > parallel port. and similarly unregister at detach. > > Signed-off-by: Sudip Mukherjee Acked-by: Willy Tarreau Willy ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 3/3] staging: panel: remove initialization check
On Sun, Mar 08, 2015 at 11:07:26PM +0530, Sudip Mukherjee wrote: > no need to monitor init_in_progress now as keypad_send_key() can only > be called after the timer is initialized. and timer is initialized > from keypad_init() which is in the attach section and can only execute > after the module has initialized. > > Signed-off-by: Sudip Mukherjee Acked-by: Willy Tarreau willy ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 2/3] staging: panel: return register value
On Sun, Mar 08, 2015 at 11:07:25PM +0530, Sudip Mukherjee wrote:
> we were returning success even if the module failed to register.
> now we are returning the actual return value, success or error.
>
> Signed-off-by: Sudip Mukherjee
> ---
> drivers/staging/panel/panel.c | 7 +--
> 1 file changed, 1 insertion(+), 6 deletions(-)
>
> diff --git a/drivers/staging/panel/panel.c b/drivers/staging/panel/panel.c
> index 3ef3dcf..b4c143a 100644
> --- a/drivers/staging/panel/panel.c
> +++ b/drivers/staging/panel/panel.c
> @@ -2360,11 +2360,6 @@ static int __init panel_init_module(void)
> return -ENODEV;
> }
>
> - if (parport_register_driver(&panel_driver)) {
> - pr_err("could not register with parport. Aborting.\n");
> - return -EIO;
> - }
> -
> if (pprt)
> pr_info("driver version " PANEL_VERSION
> " registered on parport%d (io=0x%lx).\n", parport,
> @@ -2375,7 +2370,7 @@ static int __init panel_init_module(void)
> /* tells various subsystems about the fact that initialization
> is finished */
> init_in_progress = 0;
> - return 0;
> + return parport_register_driver(&panel_driver);
Here I'd rather keep the error message, as it's quite annoying when a
driver does not load and you don't know why, especially with something
like parport where there are many possible causes. Something like this
would be better in my opinion :
- return 0;
+ err = parport_register_driver(&panel_driver);
+ if (err)
+ pr_err("could not register with parport. Aborting.\n");
+ return err;
Well, I just found that currently parport_register_driver() always
succeeds, but I still think it's better to verbosely handle errors.
Thanks,
Willy
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 2/3] staging: panel: return register value
On Mon, Mar 09, 2015 at 07:05:59PM +0530, Sudip Mukherjee wrote:
> On Mon, Mar 09, 2015 at 07:30:07AM +0100, Willy Tarreau wrote:
> > On Sun, Mar 08, 2015 at 11:07:25PM +0530, Sudip Mukherjee wrote:
> > >
> > > - if (parport_register_driver(&panel_driver)) {
> > > - pr_err("could not register with parport. Aborting.\n");
> > > - return -EIO;
> > > - }
> > > -
> > > if (pprt)
> > > pr_info("driver version " PANEL_VERSION
> > > " registered on parport%d (io=0x%lx).\n", parport,
> > > @@ -2375,7 +2370,7 @@ static int __init panel_init_module(void)
> > > /* tells various subsystems about the fact that initialization
> > > is finished */
> > > init_in_progress = 0;
> > > - return 0;
> > > + return parport_register_driver(&panel_driver);
> >
> > Here I'd rather keep the error message, as it's quite annoying when a
> > driver does not load and you don't know why, especially with something
> > like parport where there are many possible causes. Something like this
> > would be better in my opinion :
> >
> > - return 0;
> > + err = parport_register_driver(&panel_driver);
> > + if (err)
> > + pr_err("could not register with parport. Aborting.\n");
> > + return err;
> >
> > Well, I just found that currently parport_register_driver() always
> > succeeds, but I still think it's better to verbosely handle errors.
>
> ok, i will send a v2, but is the error message really required? considering
> the fact that parport_register_driver() always succeeds ..
It's a matter of choice :
- either we consider that it can fail and we emit an appropriate
error message ;
- or we consider it cannot fail and instead of returning its own
code we always call it without checking it then return zero. That
way if it were ever to change, it would be easy to spot the change.
I still find that the first choice is appropriate, comes with a very low
cost and is in line with what the parrallel port driver does as well.
Regards,
Willy
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 2/3] staging: panel: return register value
On Mon, Mar 09, 2015 at 08:08:24PM +0530, Sudip Mukherjee wrote: > we were returning success even if the module failed to register. > now we are returning the actual return value, success or error. > > Signed-off-by: Sudip Mukherjee Thanks for this. Acked-by: Willy Tarreau Willy ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3] staging: panel: change struct bits to a bit array
Hello, On Sat, Mar 14, 2015 at 11:14:43AM +0100, Isaac Lleida wrote: > This path implements a bit array representing the LCD signal states instead > of the old "struct bits", which used char to represent a single bit. This > will reduce the memory usage. > > Signed-off-by: Isaac Lleida > --- > v3: some more stupid errors I introduced in last patch fixed. Let me guess, you have never tested this patch series, right ? I have not checked the code with the patch applied, but I'm seeing this : > /* > + * LCD signal states > + */ > +#define LCD_BIT_E_MASK 0x1 /* E (data latch on falling > edge) */ > +#define LCD_BIT_RS_MASK 0x2 /* RS (0 = cmd, 1 = data) */ > +#define LCD_BIT_RW_MASK 0x4 /* R/W (0 = W, 1 = R) */ > +#define LCD_BIT_BL_MASK 0x8 /* backlight (0 = off, 1 = on) > */ > +#define LCD_BIT_CL_MASK 0x10/* clock (latch on rising edge) > */ > +#define LCD_BIT_DA_MASK 0x20/* data */ > + > +/* > + * Bit array operations > + */ > +#define BIT_ON(b, m) (b |= m) > +#define BIT_OFF(b, m)(b &= ~m) > +#define BIT_CHK(b, m)(b & m) Then this : > - val |= lcd_bits[LCD_PORT_D][LCD_BIT_E][bits.e] > - | lcd_bits[LCD_PORT_D][LCD_BIT_RS][bits.rs] > - | lcd_bits[LCD_PORT_D][LCD_BIT_RW][bits.rw] > - | lcd_bits[LCD_PORT_D][LCD_BIT_BL][bits.bl] > - | lcd_bits[LCD_PORT_D][LCD_BIT_CL][bits.cl] > - | lcd_bits[LCD_PORT_D][LCD_BIT_DA][bits.da]; > + val |= lcd_bits[LCD_PORT_D][LCD_BIT_E][BIT_CHK(bits, LCD_BIT_E_MASK)] > + | lcd_bits[LCD_PORT_D][LCD_BIT_RS][BIT_CHK(bits, LCD_BIT_RS_MASK)] > + | lcd_bits[LCD_PORT_D][LCD_BIT_RW][BIT_CHK(bits, LCD_BIT_RW_MASK)] > + | lcd_bits[LCD_PORT_D][LCD_BIT_BL][BIT_CHK(bits, LCD_BIT_BL_MASK)] > + | lcd_bits[LCD_PORT_D][LCD_BIT_CL][BIT_CHK(bits, LCD_BIT_CL_MASK)] > + | lcd_bits[LCD_PORT_D][LCD_BIT_DA][BIT_CHK(bits, LCD_BIT_DA_MASK)]; So as you can see, previously lcd_bits[x][y][z] was used with values 0 or 1 for bits, and now it's being used with values of z matching the bit position in your array. So it seems to me that it's a significant overflow. Thus I think you should modify your macro this way : #define BIT_CHK(b, m) (!!(b & m)) That said, given the amount of sensitive changes, and the risk of breakage (as was apparently done here), you *must* absolutely test you changes on real hardware before risking to break the driver for every user. Also, the fact that you needed 3 iterations of this patch after discovering breakage is clearly a sign of the fact that you need to test it. Thanks, Willy ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4] staging: panel: change struct bits to a bit array
On Mon, Mar 16, 2015 at 12:25:31PM +0300, Dan Carpenter wrote:
> Both "bits.e" and "BIT_CHK(bits, LCD_BIT_E_MASK)" are terrible. The new
> one is worse because it takes more words to tell you nothing and because
> it is wrong since E is a flag not a mask.
Yep, I agree. Maybe simply renaming "bits" to "lcd_pin" in the original
code would make it more obvious what the original ones meant.
Isaac BTW, if you only want to shrink the structure, you can do it using
a single bit per pin this way :
struct {
char e:1;
char rw:1;
char rs:1;
char sda:1;
char scl:1;
...
} pins;
Sorry I don't remember exactly the list of pins, but you get the idea.
Willy
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: panel: fix lcd type
Hi Sudip! On Tue, Mar 24, 2015 at 10:18:10AM +0530, Sudip Mukherjee wrote: > the lcd type as defined in the Kconfig is not matching in the code. > as a result the rs, rw and en pins were getting interchanged. > Kconfig defines the value of PANEL_LCD to be 1 if we select custom > configuration but in the code LCD_TYPE_CUSTOM is defined as 5. > > my hardware is LCD_TYPE_CUSTOM, but the pins were assigned to it > as pins of LCD_TYPE_OLD, and it was not working. > Now values are corrected with referenece to the values defined in > Kconfig and it is working. > checked on JHD204A lcd with LCD_TYPE_CUSTOM configuration. > > Signed-off-by: Sudip Mukherjee OK but you also need to hange the MODULE_PARM_DESC(lcd_type) to match these new settings. Given that this is a bug, you can even think about adding a Cc: stable tag. Thanks, Willy ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: panel: fix lcd type
On Tue, Mar 24, 2015 at 01:22:16PM +0530, Sudip Mukherjee wrote: > On Tue, Mar 24, 2015 at 08:04:11AM +0100, Willy Tarreau wrote: > > Hi Sudip! > > > > On Tue, Mar 24, 2015 at 10:18:10AM +0530, Sudip Mukherjee wrote: > > > the lcd type as defined in the Kconfig is not matching in the code. > > > as a result the rs, rw and en pins were getting interchanged. > > > Kconfig defines the value of PANEL_LCD to be 1 if we select custom > > > configuration but in the code LCD_TYPE_CUSTOM is defined as 5. > > > > > > my hardware is LCD_TYPE_CUSTOM, but the pins were assigned to it > > > as pins of LCD_TYPE_OLD, and it was not working. > > > Now values are corrected with referenece to the values defined in > > > Kconfig and it is working. > > > checked on JHD204A lcd with LCD_TYPE_CUSTOM configuration. > > > > > > Signed-off-by: Sudip Mukherjee > > > > OK but you also need to hange the MODULE_PARM_DESC(lcd_type) to > > match these new settings. Given that this is a bug, you can even > > think about adding a Cc: stable tag. > i missed seeing that MODULE_PARM_DESC. i wanted to have your > confirmation first before ccing stable. > > will cc stable in v2. > but before that i think i need to read the Documentation about how to > cc stable for those many stable versions till 2.6.32 Just put this before your signed-off-by at the bottom fo the commit message : Cc: # 2.6.32+ Willy ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 2/2] staging: panel: fix lcd type in module parameters
On Tue, Mar 24, 2015 at 02:56:33PM +0530, Sudip Mukherjee wrote: > with reference to the previous patch of the series, fixed the > lcd type in module parameters. Sudip, it's better to avoid fragmenting patches like you did, because it will result in a kernel state where there is an inconsistency between the parameters actually used by the kernel and those reported by modinfo. This can happen for example if someone does a bisect and ends up on patch 1/2 applied only. Obviously in this case there is very little harm, but you get the idea : each patch should be a functional change, address one thing and do it consistently. So if you change the #defines or enums or whatever, all the locations where their old values were referenced must be changed in the same patch, simply because in fact they are duplicate entries. Another point is that someone who notices your patch v1 and does not notice patch v2 could pick v1 for his kernel and end up with something inconsistent. For this reason it would be better to merge your patches into a single one here. > might not apply properly to old versions, some reordering was done > in commit <98e0e762e> Appreciated, thanks for checking this! Best regards, Willy ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3] staging: panel: fix lcd type
On Tue, Mar 24, 2015 at 04:29:32PM +0530, Sudip Mukherjee wrote: > the lcd type as defined in the Kconfig is not matching in the code. > as a result the rs, rw and en pins were getting interchanged. > Kconfig defines the value of PANEL_LCD to be 1 if we select custom > configuration but in the code LCD_TYPE_CUSTOM is defined as 5. > > my hardware is LCD_TYPE_CUSTOM, but the pins were assigned to it > as pins of LCD_TYPE_OLD, and it was not working. > Now values are corrected with referenece to the values defined in > Kconfig and it is working. > checked on JHD204A lcd with LCD_TYPE_CUSTOM configuration. > > Cc: # 2.6.32+ > Signed-off-by: Sudip Mukherjee > --- > > v3: added the module_parameters > v2: cc-ed stable Acked-by: Willy Tarreau [ next time I'll read my mails in reverse order :-) ] Willy ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: Anybody working on panel?
Hi Kristina, On Fri, Jun 20, 2014 at 06:35:03PM +0300, Kristina Mart?enko wrote: > Hi Willy, > > I'm helping Greg do a bit of cleanup in the staging tree. I noticed that > nobody seems to have worked towards moving panel out of staging in over > a year. Are there any plans to clean it up and move it out soon? Because > otherwise we're going to have to delete the driver, as we don't want > staging to become a permanent place for unfinished code. In fact I think we're facing the edge of the staging driver model : it works fine for everyone and people are used to use it from there. I'm absolutely convinced that this is the worst thing to do, but we're in the situation where doing nothing ensures it continues to work. It was submitted a long time ago by a user. I was not very happy by this since I knew I wouldn't be the person doing the clean up, but I understand that it allows users to have it ready. Since then, there have been multiple cleanup passes, I'm not even sure if it still makes sense that it remains in staging now. If some help is needed from my side to move it to drivers/misc, I'll participate, of course, but I clearly won't spend a whole week rewriting it differently for example. So please tell me what you think. Thanks, Willy ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: Anybody working on panel?
On Fri, Jun 20, 2014 at 11:46:45AM -0700, Greg KH wrote: > On Fri, Jun 20, 2014 at 09:39:26PM +0300, Kristina Mart??enko wrote: > > On 20/06/14 19:52, Willy Tarreau wrote: > > > Hi Kristina, > > > > > > On Fri, Jun 20, 2014 at 06:35:03PM +0300, Kristina Mart?enko wrote: > > >> Hi Willy, > > >> > > >> I'm helping Greg do a bit of cleanup in the staging tree. I noticed that > > >> nobody seems to have worked towards moving panel out of staging in over > > >> a year. Are there any plans to clean it up and move it out soon? Because > > >> otherwise we're going to have to delete the driver, as we don't want > > >> staging to become a permanent place for unfinished code. > > > > > > In fact I think we're facing the edge of the staging driver model : it > > > works fine for everyone and people are used to use it from there. I'm > > > absolutely convinced that this is the worst thing to do, but we're in > > > the situation where doing nothing ensures it continues to work. It was > > > submitted a long time ago by a user. I was not very happy by this since > > > I knew I wouldn't be the person doing the clean up, but I understand > > > that it allows users to have it ready. > > > > Yes, I know what you mean. On the other hand, if we keep it around, we > > have to keep every other staging driver around for the same reason - > > that it works, and somebody might want to use it. The end result is that > > there's an ever increasing amount of code in the kernel that's low > > quality, and will probably always be so. > > > > > Since then, there have been > > > multiple cleanup passes, I'm not even sure if it still makes sense that > > > it remains in staging now. > > > > That's good, though if you don't know, then I think probably no one > > does. It's the problem of staging : it works too well ; most users are fine with what they find there, and they don't comment on what they use. Maybe we should make staging more painful to use (eg: cause a 1-second pause after the warning is emitted and say that if you want this pause to disappear, you should help take that driver out of staging). > > Someone should probably review it. Agreed! > > > If some help is needed from my side to move > > > it to drivers/misc, I'll participate, of course, but I clearly won't > > > spend a whole week rewriting it differently for example. > > > > Do you want to review it? The TODO file says the userspace API and > > major/minor usages need checking. Since nobody updates the TODO after even minor updates, I don't know if that's still relevant. > > If not, then Greg, what do you think would be the best thing to do with > > this driver? > > Let me put it on my list of things to review, and I will look at it in a > few weeks when I get back from my next round of trips. That would be great. My own review would be biased because I wrote it myself (a long time ago) so I would not find it ugly even if it is. Greg, don't waste your time though. Just take a quick look and if you think it is OK, fine, otherwise if you think that issues are fixable in 2-3 evenings, I'll do them, otherwise we'll simply mark it for deletion and wait to see if someone steps up. I can also ask among my coworkers if someone is interested in taking over it if some work is still needed. Thanks, Willy ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: panel: Use designated initializers
Hi Mariusz, On Wed, Oct 29, 2014 at 11:32:30PM +0100, Mariusz Gorski wrote: > Fix "warning: missing initializer [-Wmissing-field-initializers]" > by using designated struct initializers. > > Signed-off-by: Mariusz Gorski Acked-by: Willy Tarreau Thanks, Willy ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: panel: panel.c: Fixed coding style issues
On Thu, Aug 27, 2015 at 09:02:46PM +0530, nayeem wrote: > From: Nayeemahmed Badebade > > Fixed warnings reported by checkpatch.pl: > - Block comments use a trailing */ on a separate line > - Block comments use * on subsequent lines > > Signed-off-by: Nayeemahmed Badebade Acked-by: Willy Tarreau Thanks! Willy ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] Staging: panel: fixed frivilous else statement warning
On Thu, Jul 10, 2014 at 03:34:28AM -0700, Vincent Heuken wrote: > Fixed one instance of the following checkpatch.pl warning in > panel.c: > WARNING: else is not generally useful after a break or return > > Signed-off-by: Vincent Heuken Acked-By: Willy Tarreau Vincent, are you currently reviewing the driver ? We'd like someone to completely review it to decide if it can be moved out of staging or needs to be dropped (or fixed). Thanks, Willy ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
