* [PATCH] staging: fbtft: Replace udelay with preferred usleep_range @ 2020-03-29 9:22 John B. Wyatt IV 2020-03-29 9:28 ` [Outreachy kernel] " Julia Lawall 0 siblings, 1 reply; 10+ messages in thread From: John B. Wyatt IV @ 2020-03-29 9:22 UTC (permalink / raw) To: outreachy-kernel, Greg Kroah-Hartman, Payal Kshirsagar, dri-devel, linux-fbdev, devel, linux-kernel Cc: John B. Wyatt IV Fix style issue with usleep_range being reported as preferred over udelay. Issue reported by checkpatch. Please review. As written in Documentation/timers/timers-howto.rst udelay is the generally preferred API. hrtimers, as noted in the docs, may be too expensive for this short timer. Are the docs out of date, or, is this a checkpatch issue? Signed-off-by: John B. Wyatt IV <jbwyatt4@gmail.com> --- drivers/staging/fbtft/fb_agm1264k-fl.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/staging/fbtft/fb_agm1264k-fl.c b/drivers/staging/fbtft/fb_agm1264k-fl.c index eeeeec97ad27..019c8cce6bab 100644 --- a/drivers/staging/fbtft/fb_agm1264k-fl.c +++ b/drivers/staging/fbtft/fb_agm1264k-fl.c @@ -85,7 +85,7 @@ static void reset(struct fbtft_par *par) dev_dbg(par->info->device, "%s()\n", __func__); gpiod_set_value(par->gpio.reset, 0); - udelay(20); + usleep_range(20, 20); gpiod_set_value(par->gpio.reset, 1); mdelay(120); } -- 2.25.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [Outreachy kernel] [PATCH] staging: fbtft: Replace udelay with preferred usleep_range 2020-03-29 9:22 [PATCH] staging: fbtft: Replace udelay with preferred usleep_range John B. Wyatt IV @ 2020-03-29 9:28 ` Julia Lawall 2020-03-29 9:38 ` John Wyatt 0 siblings, 1 reply; 10+ messages in thread From: Julia Lawall @ 2020-03-29 9:28 UTC (permalink / raw) To: John B. Wyatt IV Cc: outreachy-kernel, Greg Kroah-Hartman, Payal Kshirsagar, dri-devel, linux-fbdev, devel, linux-kernel On Sun, 29 Mar 2020, John B. Wyatt IV wrote: > Fix style issue with usleep_range being reported as preferred over > udelay. > > Issue reported by checkpatch. > > Please review. > > As written in Documentation/timers/timers-howto.rst udelay is the > generally preferred API. hrtimers, as noted in the docs, may be too > expensive for this short timer. > > Are the docs out of date, or, is this a checkpatch issue? > > Signed-off-by: John B. Wyatt IV <jbwyatt4@gmail.com> > --- > drivers/staging/fbtft/fb_agm1264k-fl.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/staging/fbtft/fb_agm1264k-fl.c b/drivers/staging/fbtft/fb_agm1264k-fl.c > index eeeeec97ad27..019c8cce6bab 100644 > --- a/drivers/staging/fbtft/fb_agm1264k-fl.c > +++ b/drivers/staging/fbtft/fb_agm1264k-fl.c > @@ -85,7 +85,7 @@ static void reset(struct fbtft_par *par) > dev_dbg(par->info->device, "%s()\n", __func__); > > gpiod_set_value(par->gpio.reset, 0); > - udelay(20); > + usleep_range(20, 20); usleep_range should have a range, eg usleep_range(50, 100);. But it is hard to know a priori what the range should be. So it is probably better to leave the code alone. julia > gpiod_set_value(par->gpio.reset, 1); > mdelay(120); > } > -- > 2.25.1 > > -- > You received this message because you are subscribed to the Google Groups "outreachy-kernel" group. > To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com. > To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/20200329092204.770405-1-jbwyatt4%40gmail.com. > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Outreachy kernel] [PATCH] staging: fbtft: Replace udelay with preferred usleep_range 2020-03-29 9:28 ` [Outreachy kernel] " Julia Lawall @ 2020-03-29 9:38 ` John Wyatt 2020-03-29 9:47 ` Julia Lawall 0 siblings, 1 reply; 10+ messages in thread From: John Wyatt @ 2020-03-29 9:38 UTC (permalink / raw) To: Julia Lawall Cc: outreachy-kernel, Greg Kroah-Hartman, Payal Kshirsagar, dri-devel, linux-fbdev, devel, linux-kernel On Sun, 2020-03-29 at 11:28 +0200, Julia Lawall wrote: > > On Sun, 29 Mar 2020, John B. Wyatt IV wrote: > > > Fix style issue with usleep_range being reported as preferred over > > udelay. > > > > Issue reported by checkpatch. > > > > Please review. > > > > As written in Documentation/timers/timers-howto.rst udelay is the > > generally preferred API. hrtimers, as noted in the docs, may be too > > expensive for this short timer. > > > > Are the docs out of date, or, is this a checkpatch issue? > > > > Signed-off-by: John B. Wyatt IV <jbwyatt4@gmail.com> > > --- > > drivers/staging/fbtft/fb_agm1264k-fl.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/staging/fbtft/fb_agm1264k-fl.c > > b/drivers/staging/fbtft/fb_agm1264k-fl.c > > index eeeeec97ad27..019c8cce6bab 100644 > > --- a/drivers/staging/fbtft/fb_agm1264k-fl.c > > +++ b/drivers/staging/fbtft/fb_agm1264k-fl.c > > @@ -85,7 +85,7 @@ static void reset(struct fbtft_par *par) > > dev_dbg(par->info->device, "%s()\n", __func__); > > > > gpiod_set_value(par->gpio.reset, 0); > > - udelay(20); > > + usleep_range(20, 20); > > usleep_range should have a range, eg usleep_range(50, 100);. But it > is > hard to know a priori what the range should be. So it is probably > better > to leave the code alone. Understood. With the question I wrote in the commit message: "As written in Documentation/timers/timers-howto.rst udelay is the generally preferred API. hrtimers, as noted in the docs, may be too expensive for this short timer. Are the docs out of date, or, is this a checkpatch issue?" Is usleep_range too expensive for this operation? Why does checkpatch favor usleep_range while the docs favor udelay? > > julia > > > gpiod_set_value(par->gpio.reset, 1); > > mdelay(120); > > } > > -- > > 2.25.1 > > > > -- > > You received this message because you are subscribed to the Google > > Groups "outreachy-kernel" group. > > To unsubscribe from this group and stop receiving emails from it, > > send an email to outreachy-kernel+unsubscribe@googlegroups.com. > > To view this discussion on the web visit > > https://groups.google.com/d/msgid/outreachy-kernel/20200329092204.770405-1-jbwyatt4%40gmail.com > > . > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Outreachy kernel] [PATCH] staging: fbtft: Replace udelay with preferred usleep_range 2020-03-29 9:38 ` John Wyatt @ 2020-03-29 9:47 ` Julia Lawall [not found] ` <CAMS7mKBEhqFat8fWi=QiFwfLV9+skwi1hE-swg=XxU48zk=_tQ@mail.gmail.com> 0 siblings, 1 reply; 10+ messages in thread From: Julia Lawall @ 2020-03-29 9:47 UTC (permalink / raw) To: John Wyatt Cc: outreachy-kernel, Greg Kroah-Hartman, Payal Kshirsagar, dri-devel, linux-fbdev, devel, linux-kernel On Sun, 29 Mar 2020, John Wyatt wrote: > On Sun, 2020-03-29 at 11:28 +0200, Julia Lawall wrote: > > > > On Sun, 29 Mar 2020, John B. Wyatt IV wrote: > > > > > Fix style issue with usleep_range being reported as preferred over > > > udelay. > > > > > > Issue reported by checkpatch. > > > > > > Please review. > > > > > > As written in Documentation/timers/timers-howto.rst udelay is the > > > generally preferred API. hrtimers, as noted in the docs, may be too > > > expensive for this short timer. > > > > > > Are the docs out of date, or, is this a checkpatch issue? > > > > > > Signed-off-by: John B. Wyatt IV <jbwyatt4@gmail.com> > > > --- > > > drivers/staging/fbtft/fb_agm1264k-fl.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/drivers/staging/fbtft/fb_agm1264k-fl.c > > > b/drivers/staging/fbtft/fb_agm1264k-fl.c > > > index eeeeec97ad27..019c8cce6bab 100644 > > > --- a/drivers/staging/fbtft/fb_agm1264k-fl.c > > > +++ b/drivers/staging/fbtft/fb_agm1264k-fl.c > > > @@ -85,7 +85,7 @@ static void reset(struct fbtft_par *par) > > > dev_dbg(par->info->device, "%s()\n", __func__); > > > > > > gpiod_set_value(par->gpio.reset, 0); > > > - udelay(20); > > > + usleep_range(20, 20); > > > > usleep_range should have a range, eg usleep_range(50, 100);. But it > > is > > hard to know a priori what the range should be. So it is probably > > better > > to leave the code alone. > > Understood. > > With the question I wrote in the commit message: > > "As written in Documentation/timers/timers-howto.rst udelay is the > generally preferred API. hrtimers, as noted in the docs, may be too > expensive for this short timer. > > Are the docs out of date, or, is this a checkpatch issue?" > > Is usleep_range too expensive for this operation? > > Why does checkpatch favor usleep_range while the docs favor udelay? I don't know the answer in detail, but it is quite possible that checkpatch doesn't pay any attention to the delay argument. Checkpatch is a perl script that highlights things that may be of concern. It is not a precise static analsis tool. As a matter of form, all of your Please review comments should have been put below the ---. Currently, if someone had wanted to apply the patch, you would make them do extra work to remove this information. julia > > > > > julia > > > > > gpiod_set_value(par->gpio.reset, 1); > > > mdelay(120); > > > } > > > -- > > > 2.25.1 > > > > > > -- > > > You received this message because you are subscribed to the Google > > > Groups "outreachy-kernel" group. > > > To unsubscribe from this group and stop receiving emails from it, > > > send an email to outreachy-kernel+unsubscribe@googlegroups.com. > > > To view this discussion on the web visit > > > https://groups.google.com/d/msgid/outreachy-kernel/20200329092204.770405-1-jbwyatt4%40gmail.com > > > . > > > > > ^ permalink raw reply [flat|nested] 10+ messages in thread
[parent not found: <CAMS7mKBEhqFat8fWi=QiFwfLV9+skwi1hE-swg=XxU48zk=_tQ@mail.gmail.com>]
* Re: [Outreachy kernel] [PATCH] staging: fbtft: Replace udelay with preferred usleep_range [not found] ` <CAMS7mKBEhqFat8fWi=QiFwfLV9+skwi1hE-swg=XxU48zk=_tQ@mail.gmail.com> @ 2020-03-29 10:37 ` Julia Lawall 2020-03-29 10:51 ` Sam Muhammed 2020-03-30 17:40 ` Stefano Brivio 0 siblings, 2 replies; 10+ messages in thread From: Julia Lawall @ 2020-03-29 10:37 UTC (permalink / raw) To: Soumyajit Deb Cc: John Wyatt, outreachy-kernel, Greg Kroah-Hartman, Payal Kshirsagar, dri-devel, linux-fbdev, devel, linux-kernel [-- Attachment #1: Type: text/plain, Size: 5158 bytes --] On Sun, 29 Mar 2020, Soumyajit Deb wrote: > I had the same doubt the other day about the replacement of udelay() with > usleep_range(). The corresponding range for the single argument value of > udelay() is quite confusing as I couldn't decide the range. But as much as I > noticed checkpatch.pl gives warning for replacing udelay() with > usleep_range() by checking the argument value of udelay(). In the > documentation, it is written udelay() should be used for a sleep time of at > most 10 microseconds but between 10 microseconds and 20 milliseconds, > usleep_range() should be used. > I think the range is code specific and will depend on what range is > acceptable and doesn't break the code. > Please correct me if I am wrong. The range depends on the associated hardware. Just because checkpatch suggests something doesn't mean that it is easy to address the problem. julia > > More clarification on this issue will be helpful. > > On Sun, 29 Mar 2020, 15:17 Julia Lawall, <julia.lawall@inria.fr> wrote: > > > On Sun, 29 Mar 2020, John Wyatt wrote: > > > On Sun, 2020-03-29 at 11:28 +0200, Julia Lawall wrote: > > > > > > On Sun, 29 Mar 2020, John B. Wyatt IV wrote: > > > > > > > Fix style issue with usleep_range being reported as > preferred over > > > > udelay. > > > > > > > > Issue reported by checkpatch. > > > > > > > > Please review. > > > > > > > > As written in Documentation/timers/timers-howto.rst udelay > is the > > > > generally preferred API. hrtimers, as noted in the docs, > may be too > > > > expensive for this short timer. > > > > > > > > Are the docs out of date, or, is this a checkpatch issue? > > > > > > > > Signed-off-by: John B. Wyatt IV <jbwyatt4@gmail.com> > > > > --- > > > > drivers/staging/fbtft/fb_agm1264k-fl.c | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/staging/fbtft/fb_agm1264k-fl.c > > > > b/drivers/staging/fbtft/fb_agm1264k-fl.c > > > > index eeeeec97ad27..019c8cce6bab 100644 > > > > --- a/drivers/staging/fbtft/fb_agm1264k-fl.c > > > > +++ b/drivers/staging/fbtft/fb_agm1264k-fl.c > > > > @@ -85,7 +85,7 @@ static void reset(struct fbtft_par *par) > > > > dev_dbg(par->info->device, "%s()\n", __func__); > > > > > > > > gpiod_set_value(par->gpio.reset, 0); > > > > - udelay(20); > > > > + usleep_range(20, 20); > > > > > > usleep_range should have a range, eg usleep_range(50, > 100);. But it > > > is > > > hard to know a priori what the range should be. So it is > probably > > > better > > > to leave the code alone. > > > > Understood. > > > > With the question I wrote in the commit message: > > > > "As written in Documentation/timers/timers-howto.rst udelay is > the > > generally preferred API. hrtimers, as noted in the docs, may > be too > > expensive for this short timer. > > > > Are the docs out of date, or, is this a checkpatch issue?" > > > > Is usleep_range too expensive for this operation? > > > > Why does checkpatch favor usleep_range while the docs favor > udelay? > > I don't know the answer in detail, but it is quite possible that > checkpatch doesn't pay any attention to the delay argument. > Checkpatch is > a perl script that highlights things that may be of concern. It > is not a > precise static analsis tool. > > As a matter of form, all of your Please review comments should > have been > put below the ---. Currently, if someone had wanted to apply > the patch, > you would make them do extra work to remove this information. > > julia > > > > > > > > > julia > > > > > > > gpiod_set_value(par->gpio.reset, 1); > > > > mdelay(120); > > > > } > > > > -- > > > > 2.25.1 > > > > > > > > -- > > > > You received this message because you are subscribed to > the Google > > > > Groups "outreachy-kernel" group. > > > > To unsubscribe from this group and stop receiving emails > from it, > > > > send an email to > outreachy-kernel+unsubscribe@googlegroups.com. > > > > To view this discussion on the web visit > > > >https://groups.google.com/d/msgid/outreachy-kernel/20200329092204.770405-1- > jbwyatt4%40gmail.com > > > > . > > > > > > > > > > -- > You received this message because you are subscribed to the > Google Groups "outreachy-kernel" group. > To unsubscribe from this group and stop receiving emails from > it, send an email to > outreachy-kernel+unsubscribe@googlegroups.com. > To view this discussion on the web visithttps://groups.google.com/d/msgid/outreachy-kernel/alpine.DEB.2.21.20032911 > 44460.2990%40hadrien. > > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Outreachy kernel] [PATCH] staging: fbtft: Replace udelay with preferred usleep_range 2020-03-29 10:37 ` Julia Lawall @ 2020-03-29 10:51 ` Sam Muhammed 2020-03-29 12:22 ` Andy Shevchenko 2020-03-30 17:40 ` Stefano Brivio 1 sibling, 1 reply; 10+ messages in thread From: Sam Muhammed @ 2020-03-29 10:51 UTC (permalink / raw) To: Julia Lawall, Soumyajit Deb Cc: John Wyatt, outreachy-kernel, Greg Kroah-Hartman, Payal Kshirsagar, dri-devel, linux-fbdev, devel, linux-kernel On Sun, 2020-03-29 at 12:37 +0200, Julia Lawall wrote: > > On Sun, 29 Mar 2020, Soumyajit Deb wrote: > > > I had the same doubt the other day about the replacement of udelay() with > > usleep_range(). The corresponding range for the single argument value of > > udelay() is quite confusing as I couldn't decide the range. But as much as I > > noticed checkpatch.pl gives warning for replacing udelay() with > > usleep_range() by checking the argument value of udelay(). In the > > documentation, it is written udelay() should be used for a sleep time of at > > most 10 microseconds but between 10 microseconds and 20 milliseconds, > > usleep_range() should be used. > > I think the range is code specific and will depend on what range is > > acceptable and doesn't break the code. > > Please correct me if I am wrong. > > The range depends on the associated hardware. Just because checkpatch > suggests something doesn't mean that it is easy to address the problem. > > julia > Hi all, i think when it comes to a significant change in the code, we should at least be familiar with the driver or be able to test the change. In the very beginning of the Documentation/timers/timers-howto.rst there is the question: "Is my code in an atomic context?" It's not just about the range, it's more of at which context this code runs, for atomic-context -> udelay must be used. for non-atomic context -> usleep-range is better for power-management. unless we are familiar with the driver we wouldn't really know in what context this code is run at. This thread though had the same conversation about this change, for the same driver. https://patchwork.kernel.org/patch/11137125/ Sam > > > > More clarification on this issue will be helpful. > > > > On Sun, 29 Mar 2020, 15:17 Julia Lawall, <julia.lawall@inria.fr> wrote: > > > > > > On Sun, 29 Mar 2020, John Wyatt wrote: > > > > > On Sun, 2020-03-29 at 11:28 +0200, Julia Lawall wrote: > > > > > > > > On Sun, 29 Mar 2020, John B. Wyatt IV wrote: > > > > > > > > > Fix style issue with usleep_range being reported as > > preferred over > > > > > udelay. > > > > > > > > > > Issue reported by checkpatch. > > > > > > > > > > Please review. > > > > > > > > > > As written in Documentation/timers/timers-howto.rst udelay > > is the > > > > > generally preferred API. hrtimers, as noted in the docs, > > may be too > > > > > expensive for this short timer. > > > > > > > > > > Are the docs out of date, or, is this a checkpatch issue? > > > > > > > > > > Signed-off-by: John B. Wyatt IV <jbwyatt4@gmail.com> > > > > > --- > > > > > drivers/staging/fbtft/fb_agm1264k-fl.c | 2 +- > > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > > > diff --git a/drivers/staging/fbtft/fb_agm1264k-fl.c > > > > > b/drivers/staging/fbtft/fb_agm1264k-fl.c > > > > > index eeeeec97ad27..019c8cce6bab 100644 > > > > > --- a/drivers/staging/fbtft/fb_agm1264k-fl.c > > > > > +++ b/drivers/staging/fbtft/fb_agm1264k-fl.c > > > > > @@ -85,7 +85,7 @@ static void reset(struct fbtft_par *par) > > > > > dev_dbg(par->info->device, "%s()\n", __func__); > > > > > > > > > > gpiod_set_value(par->gpio.reset, 0); > > > > > - udelay(20); > > > > > + usleep_range(20, 20); > > > > > > > > usleep_range should have a range, eg usleep_range(50, > > 100);. But it > > > > is > > > > hard to know a priori what the range should be. So it is > > probably > > > > better > > > > to leave the code alone. > > > > > > Understood. > > > > > > With the question I wrote in the commit message: > > > > > > "As written in Documentation/timers/timers-howto.rst udelay is > > the > > > generally preferred API. hrtimers, as noted in the docs, may > > be too > > > expensive for this short timer. > > > > > > Are the docs out of date, or, is this a checkpatch issue?" > > > > > > Is usleep_range too expensive for this operation? > > > > > > Why does checkpatch favor usleep_range while the docs favor > > udelay? > > > > I don't know the answer in detail, but it is quite possible that > > checkpatch doesn't pay any attention to the delay argument. > > Checkpatch is > > a perl script that highlights things that may be of concern. It > > is not a > > precise static analsis tool. > > > > As a matter of form, all of your Please review comments should > > have been > > put below the ---. Currently, if someone had wanted to apply > > the patch, > > you would make them do extra work to remove this information. > > > > julia > > > > > > > > > > > > > julia > > > > > > > > > gpiod_set_value(par->gpio.reset, 1); > > > > > mdelay(120); > > > > > } > > > > > -- > > > > > 2.25.1 > > > > > > > > > > -- > > > > > You received this message because you are subscribed to > > the Google > > > > > Groups "outreachy-kernel" group. > > > > > To unsubscribe from this group and stop receiving emails > > from it, > > > > > send an email to > > outreachy-kernel+unsubscribe@googlegroups.com. > > > > > To view this discussion on the web visit > > > > >https://groups.google.com/d/msgid/outreachy-kernel/20200329092204.770405-1- > > jbwyatt4%40gmail.com > > > > > . > > > > > > > > > > > > > > > -- > > You received this message because you are subscribed to the > > Google Groups "outreachy-kernel" group. > > To unsubscribe from this group and stop receiving emails from > > it, send an email to > > outreachy-kernel+unsubscribe@googlegroups.com. > > To view this discussion on the web visithttps://groups.google.com/d/msgid/outreachy-kernel/alpine.DEB.2.21.20032911 > > 44460.2990%40hadrien. > > > > > > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Outreachy kernel] [PATCH] staging: fbtft: Replace udelay with preferred usleep_range 2020-03-29 10:51 ` Sam Muhammed @ 2020-03-29 12:22 ` Andy Shevchenko 0 siblings, 0 replies; 10+ messages in thread From: Andy Shevchenko @ 2020-03-29 12:22 UTC (permalink / raw) To: Sam Muhammed Cc: Julia Lawall, Soumyajit Deb, John Wyatt, outreachy-kernel, Greg Kroah-Hartman, Payal Kshirsagar, dri-devel, linux-fbdev, open list:STAGING SUBSYSTEM, Linux Kernel Mailing List On Sun, Mar 29, 2020 at 2:23 PM Sam Muhammed <jane.pnx9@gmail.com> wrote: > On Sun, 2020-03-29 at 12:37 +0200, Julia Lawall wrote: > > On Sun, 29 Mar 2020, Soumyajit Deb wrote: > > First of all, let's stop topposting. > > > I had the same doubt the other day about the replacement of udelay() with > > > usleep_range(). The corresponding range for the single argument value of > > > udelay() is quite confusing as I couldn't decide the range. But as much as I > > > noticed checkpatch.pl gives warning for replacing udelay() with > > > usleep_range() by checking the argument value of udelay(). In the > > > documentation, it is written udelay() should be used for a sleep time of at > > > most 10 microseconds but between 10 microseconds and 20 milliseconds, > > > usleep_range() should be used. > > > I think the range is code specific and will depend on what range is > > > acceptable and doesn't break the code. > > > Please correct me if I am wrong. > > > > The range depends on the associated hardware. Just because checkpatch > > suggests something doesn't mean that it is easy to address the problem. > Hi all, i think when it comes to a significant change in the code, we > should at least be familiar with the driver or be able to test the > change. > > In the very beginning of the Documentation/timers/timers-howto.rst > there is the question: > "Is my code in an atomic context?" > It's not just about the range, it's more of at which context this code > runs, for atomic-context -> udelay must be used. > for non-atomic context -> usleep-range is better for power-management. > > unless we are familiar with the driver we wouldn't really know in what > context this code is run at. > > This thread though had the same conversation about this change, for the > same driver. > https://patchwork.kernel.org/patch/11137125/ While it's a good discussion it reminds me that this entire function, i.e. reset(), repeats the on provided by fbtft core. Yes, the only question if it's atomic or not. IIRC ->reset() is being called only in non-atomic contexts and keeping reset signal longer is fine (but better to check with datasheet). So, I would rather to drop the function completely in order to use fbtft's core one. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Outreachy kernel] [PATCH] staging: fbtft: Replace udelay with preferred usleep_range 2020-03-29 10:37 ` Julia Lawall 2020-03-29 10:51 ` Sam Muhammed @ 2020-03-30 17:40 ` Stefano Brivio 2020-03-30 22:03 ` John B. Wyatt IV 1 sibling, 1 reply; 10+ messages in thread From: Stefano Brivio @ 2020-03-30 17:40 UTC (permalink / raw) To: John Wyatt Cc: Julia Lawall, Soumyajit Deb, outreachy-kernel, Greg Kroah-Hartman, Payal Kshirsagar, dri-devel, linux-fbdev, devel, linux-kernel On Sun, 29 Mar 2020 12:37:18 +0200 (CEST) Julia Lawall <julia.lawall@inria.fr> wrote: > On Sun, 29 Mar 2020, Soumyajit Deb wrote: > > > I had the same doubt the other day about the replacement of udelay() with > > usleep_range(). The corresponding range for the single argument value of > > udelay() is quite confusing as I couldn't decide the range. But as much as I > > noticed checkpatch.pl gives warning for replacing udelay() with > > usleep_range() by checking the argument value of udelay(). In the > > documentation, it is written udelay() should be used for a sleep time of at > > most 10 microseconds but between 10 microseconds and 20 milliseconds, > > usleep_range() should be used. > > I think the range is code specific and will depend on what range is > > acceptable and doesn't break the code. > > Please correct me if I am wrong. > > The range depends on the associated hardware. John, by the way, here you could have checked the datasheet of this LCD controller. It's a pair of those: https://www.sparkfun.com/datasheets/LCD/ks0108b.pdf reset time is 1µs minimum, which is the only actual constraint here. The rise time should then be handled by power supply and reflected with some appropriate usage of the regulator framework. That 120ms delay, however, must be there for a reason, that is, most likely to develop this quickly without exposing a proper model of the power supplies to the driver. So... in this case, with the datasheet alone, you won't go very far, you would need the actual module (probably connected to a Raspberry Pi to catch a typical usage). Still, it's usually worth a check. In any case, most likely, as Andy suggested, this function can eventually be dropped. -- Stefano ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Outreachy kernel] [PATCH] staging: fbtft: Replace udelay with preferred usleep_range 2020-03-30 17:40 ` Stefano Brivio @ 2020-03-30 22:03 ` John B. Wyatt IV 2020-03-30 22:16 ` Stefano Brivio 0 siblings, 1 reply; 10+ messages in thread From: John B. Wyatt IV @ 2020-03-30 22:03 UTC (permalink / raw) To: Stefano Brivio Cc: Julia Lawall, Soumyajit Deb, outreachy-kernel, Greg Kroah-Hartman, Payal Kshirsagar, dri-devel, linux-fbdev, devel, linux-kernel On Mon, 2020-03-30 at 19:40 +0200, Stefano Brivio wrote: > On Sun, 29 Mar 2020 12:37:18 +0200 (CEST) > Julia Lawall <julia.lawall@inria.fr> wrote: > > > On Sun, 29 Mar 2020, Soumyajit Deb wrote: > > > > > I had the same doubt the other day about the replacement of > > > udelay() with > > > usleep_range(). The corresponding range for the single argument > > > value of > > > udelay() is quite confusing as I couldn't decide the range. But > > > as much as I > > > noticed checkpatch.pl gives warning for replacing udelay() with > > > usleep_range() by checking the argument value of udelay(). In the > > > documentation, it is written udelay() should be used for a sleep > > > time of at > > > most 10 microseconds but between 10 microseconds and 20 > > > milliseconds, > > > usleep_range() should be used. > > > I think the range is code specific and will depend on what range > > > is > > > acceptable and doesn't break the code. > > > Please correct me if I am wrong. > > > > The range depends on the associated hardware. > > John, by the way, here you could have checked the datasheet of this > LCD > controller. It's a pair of those: > https://www.sparkfun.com/datasheets/LCD/ks0108b.pdf > No I have not. This datasheet is a little over my head honestly. What would you recommend to get familiar with datasheets like this? ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Outreachy kernel] [PATCH] staging: fbtft: Replace udelay with preferred usleep_range 2020-03-30 22:03 ` John B. Wyatt IV @ 2020-03-30 22:16 ` Stefano Brivio 0 siblings, 0 replies; 10+ messages in thread From: Stefano Brivio @ 2020-03-30 22:16 UTC (permalink / raw) To: John B. Wyatt IV Cc: Julia Lawall, Soumyajit Deb, outreachy-kernel, Greg Kroah-Hartman, Payal Kshirsagar, dri-devel, linux-fbdev, devel, linux-kernel On Mon, 30 Mar 2020 15:03:55 -0700 "John B. Wyatt IV" <jbwyatt4@gmail.com> wrote: > On Mon, 2020-03-30 at 19:40 +0200, Stefano Brivio wrote: > > On Sun, 29 Mar 2020 12:37:18 +0200 (CEST) > > Julia Lawall <julia.lawall@inria.fr> wrote: > > > > > On Sun, 29 Mar 2020, Soumyajit Deb wrote: > > > > > > > I had the same doubt the other day about the replacement of > > > > udelay() with > > > > usleep_range(). The corresponding range for the single argument > > > > value of > > > > udelay() is quite confusing as I couldn't decide the range. But > > > > as much as I > > > > noticed checkpatch.pl gives warning for replacing udelay() with > > > > usleep_range() by checking the argument value of udelay(). In the > > > > documentation, it is written udelay() should be used for a sleep > > > > time of at > > > > most 10 microseconds but between 10 microseconds and 20 > > > > milliseconds, > > > > usleep_range() should be used. > > > > I think the range is code specific and will depend on what range > > > > is > > > > acceptable and doesn't break the code. > > > > Please correct me if I am wrong. > > > > > > The range depends on the associated hardware. > > > > John, by the way, here you could have checked the datasheet of this > > LCD > > controller. It's a pair of those: > > https://www.sparkfun.com/datasheets/LCD/ks0108b.pdf > > No I have not. This datasheet is a little over my head honestly. > > What would you recommend to get familiar with datasheets like this? Well, you don't necessarily have to, there are many subsystems in the kernel which are almost completely abstracted away from hardware. If you're interested, look around yourself for something simple chip, or get something that you can easily plug on a "maker board", Raspberry Pi, something like that. Perhaps via I²C or SPI. Some types of sensors (temperature, pressure) have very simple datasheets. If you are allergic to hardware, try: $ ls -Ssl drivers/iio/* pick the smallest sensor driver in the category that is the most likely to spark your interest, and go through it, checking it against the datasheet, at some point it will make sense. -- Stefano ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2020-03-30 22:17 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-03-29 9:22 [PATCH] staging: fbtft: Replace udelay with preferred usleep_range John B. Wyatt IV 2020-03-29 9:28 ` [Outreachy kernel] " Julia Lawall 2020-03-29 9:38 ` John Wyatt 2020-03-29 9:47 ` Julia Lawall [not found] ` <CAMS7mKBEhqFat8fWi=QiFwfLV9+skwi1hE-swg=XxU48zk=_tQ@mail.gmail.com> 2020-03-29 10:37 ` Julia Lawall 2020-03-29 10:51 ` Sam Muhammed 2020-03-29 12:22 ` Andy Shevchenko 2020-03-30 17:40 ` Stefano Brivio 2020-03-30 22:03 ` John B. Wyatt IV 2020-03-30 22:16 ` Stefano Brivio
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).