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, <[email protected]> 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 <[email protected]>
> > > > ---
> > > > 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
> [email protected].
> > > > 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
> [email protected].
> To view this discussion on the web visithttps://groups.google.com/d/msgid/outreachy-kernel/alpine.DEB.2.21.20032911
> 44460.2990%40hadrien.
>
>
>
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, <[email protected]> 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 <[email protected]>
> > > > > ---
> > > > > 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
> > [email protected].
> > > > > 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
> > [email protected].
> > To view this discussion on the web visithttps://groups.google.com/d/msgid/outreachy-kernel/alpine.DEB.2.21.20032911
> > 44460.2990%40hadrien.
> >
> >
> >
>
On Sun, Mar 29, 2020 at 2:23 PM Sam Muhammed <[email protected]> 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
On Sun, 29 Mar 2020 12:37:18 +0200 (CEST)
Julia Lawall <[email protected]> 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
On Mon, 2020-03-30 at 19:40 +0200, Stefano Brivio wrote:
> On Sun, 29 Mar 2020 12:37:18 +0200 (CEST)
> Julia Lawall <[email protected]> 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?
On Mon, 30 Mar 2020 15:03:55 -0700
"John B. Wyatt IV" <[email protected]> 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 <[email protected]> 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