| Message ID | 20231213112322.1655236-1-arnd@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] media: i2c: mt9m114: use fsleep() in place of udelay() | expand |
Hi Arnd, Thank you for the patch. On Wed, Dec 13, 2023 at 12:23:07PM +0100, Arnd Bergmann wrote: > From: Arnd Bergmann <arnd@arndb.de> > > With clang-16, building without COMMON_CLK triggers a range check on > udelay() because of a constant division-by-zero calculation: > > ld.lld: error: undefined symbol: __bad_udelay > >>> referenced by mt9m114.c > >>> drivers/media/i2c/mt9m114.o:(mt9m114_power_on) in archive vmlinux.a > > In this configuration, the driver already fails to probe, before > this function gets called, so it's enough to suppress the assertion. > > Do this by using fsleep(), which turns long delays into sleep() calls > in place of the link failure. > > This is probably a good idea regardless to avoid overly long dynamic > udelay() calls on a slow clock. The longest delay would be 16.66µs, which may not be worth a sleep, but I think that's fine. Using fsleep() by default is a good idea. > Cc: Sakari Ailus <sakari.ailus@linux.intel.com> > Fixes: 24d756e914fc ("media: i2c: Add driver for onsemi MT9M114 camera sensor") > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > drivers/media/i2c/mt9m114.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/media/i2c/mt9m114.c b/drivers/media/i2c/mt9m114.c > index 0a22f328981d..68adaecaf481 100644 > --- a/drivers/media/i2c/mt9m114.c > +++ b/drivers/media/i2c/mt9m114.c > @@ -2116,7 +2116,7 @@ static int mt9m114_power_on(struct mt9m114 *sensor) > duration = DIV_ROUND_UP(2 * 50 * 1000000, freq); > > gpiod_set_value(sensor->reset, 1); > - udelay(duration); > + fsleep(duration); > gpiod_set_value(sensor->reset, 0); > } else { > /*
On 13/12/2023 13:23, Arnd Bergmann wrote: > From: Arnd Bergmann <arnd@arndb.de> > > With clang-16, building without COMMON_CLK triggers a range check on > udelay() because of a constant division-by-zero calculation: > > ld.lld: error: undefined symbol: __bad_udelay >>>> referenced by mt9m114.c >>>> drivers/media/i2c/mt9m114.o:(mt9m114_power_on) in archive vmlinux.a > > In this configuration, the driver already fails to probe, before > this function gets called, so it's enough to suppress the assertion. > > Do this by using fsleep(), which turns long delays into sleep() calls > in place of the link failure. > > This is probably a good idea regardless to avoid overly long dynamic > udelay() calls on a slow clock. > > Cc: Sakari Ailus <sakari.ailus@linux.intel.com> > Fixes: 24d756e914fc ("media: i2c: Add driver for onsemi MT9M114 camera sensor") > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > drivers/media/i2c/mt9m114.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/media/i2c/mt9m114.c b/drivers/media/i2c/mt9m114.c > index 0a22f328981d..68adaecaf481 100644 > --- a/drivers/media/i2c/mt9m114.c > +++ b/drivers/media/i2c/mt9m114.c > @@ -2116,7 +2116,7 @@ static int mt9m114_power_on(struct mt9m114 *sensor) > duration = DIV_ROUND_UP(2 * 50 * 1000000, freq); > > gpiod_set_value(sensor->reset, 1); > - udelay(duration); > + fsleep(duration); > gpiod_set_value(sensor->reset, 0); > } else { > /* I think this is fine, so: Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> But: If we don't have COMMON_CLK (or rather, I think, HAVE_CLK), the freq will be zero at compile time. So won't the compiler give a warning for the DIV_ROUND_UP() call? Interestingly, for me, this doesn't give a div-by-zero warning: int x; int y = 0; x = DIV_ROUND_UP(10, y); but this does: int x; const int y = 0; x = DIV_ROUND_UP(10, y); And looks like this gives the warning too: int x; const int y = 0; if (y) x = DIV_ROUND_UP(10, y); So, I think, the code in the driver could fail to compile at some later point, if the compiler warnings are improved (?), or if someone adds a 'const' in front of 'long freq = clk_get_rate(sensor->clk);' line. Maybe worry about that if it actually happens =). Tomi
On Wed, Dec 13, 2023 at 01:40:54PM +0200, Tomi Valkeinen wrote: > On 13/12/2023 13:23, Arnd Bergmann wrote: > > From: Arnd Bergmann <arnd@arndb.de> > > > > With clang-16, building without COMMON_CLK triggers a range check on > > udelay() because of a constant division-by-zero calculation: > > > > ld.lld: error: undefined symbol: __bad_udelay > >>>> referenced by mt9m114.c > >>>> drivers/media/i2c/mt9m114.o:(mt9m114_power_on) in archive vmlinux.a > > > > In this configuration, the driver already fails to probe, before > > this function gets called, so it's enough to suppress the assertion. > > > > Do this by using fsleep(), which turns long delays into sleep() calls > > in place of the link failure. > > > > This is probably a good idea regardless to avoid overly long dynamic > > udelay() calls on a slow clock. > > > > Cc: Sakari Ailus <sakari.ailus@linux.intel.com> > > Fixes: 24d756e914fc ("media: i2c: Add driver for onsemi MT9M114 camera sensor") > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > > --- > > drivers/media/i2c/mt9m114.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/media/i2c/mt9m114.c b/drivers/media/i2c/mt9m114.c > > index 0a22f328981d..68adaecaf481 100644 > > --- a/drivers/media/i2c/mt9m114.c > > +++ b/drivers/media/i2c/mt9m114.c > > @@ -2116,7 +2116,7 @@ static int mt9m114_power_on(struct mt9m114 *sensor) > > duration = DIV_ROUND_UP(2 * 50 * 1000000, freq); > > > > gpiod_set_value(sensor->reset, 1); > > - udelay(duration); > > + fsleep(duration); > > gpiod_set_value(sensor->reset, 0); > > } else { > > /* > > I think this is fine, so: > > Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > > But: If we don't have COMMON_CLK (or rather, I think, HAVE_CLK), the > freq will be zero at compile time. So won't the compiler give a warning > for the DIV_ROUND_UP() call? > > Interestingly, for me, this doesn't give a div-by-zero warning: > > int x; > int y = 0; > x = DIV_ROUND_UP(10, y); > > but this does: > > int x; > const int y = 0; > x = DIV_ROUND_UP(10, y); > > And looks like this gives the warning too: > > int x; > const int y = 0; > if (y) > x = DIV_ROUND_UP(10, y); > > So, I think, the code in the driver could fail to compile at some later > point, if the compiler warnings are improved (?), or if someone adds a > 'const' in front of 'long freq = clk_get_rate(sensor->clk);' line. > > Maybe worry about that if it actually happens =). Maybe :-) I would be tempted to make VIDEO_CAMERA_SENSOR depend on COMMON_CLK.
On 13/12/2023 13:48, Laurent Pinchart wrote: > On Wed, Dec 13, 2023 at 01:40:54PM +0200, Tomi Valkeinen wrote: >> On 13/12/2023 13:23, Arnd Bergmann wrote: >>> From: Arnd Bergmann <arnd@arndb.de> >>> >>> With clang-16, building without COMMON_CLK triggers a range check on >>> udelay() because of a constant division-by-zero calculation: >>> >>> ld.lld: error: undefined symbol: __bad_udelay >>>>>> referenced by mt9m114.c >>>>>> drivers/media/i2c/mt9m114.o:(mt9m114_power_on) in archive vmlinux.a >>> >>> In this configuration, the driver already fails to probe, before >>> this function gets called, so it's enough to suppress the assertion. >>> >>> Do this by using fsleep(), which turns long delays into sleep() calls >>> in place of the link failure. >>> >>> This is probably a good idea regardless to avoid overly long dynamic >>> udelay() calls on a slow clock. >>> >>> Cc: Sakari Ailus <sakari.ailus@linux.intel.com> >>> Fixes: 24d756e914fc ("media: i2c: Add driver for onsemi MT9M114 camera sensor") >>> Signed-off-by: Arnd Bergmann <arnd@arndb.de> >>> --- >>> drivers/media/i2c/mt9m114.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/drivers/media/i2c/mt9m114.c b/drivers/media/i2c/mt9m114.c >>> index 0a22f328981d..68adaecaf481 100644 >>> --- a/drivers/media/i2c/mt9m114.c >>> +++ b/drivers/media/i2c/mt9m114.c >>> @@ -2116,7 +2116,7 @@ static int mt9m114_power_on(struct mt9m114 *sensor) >>> duration = DIV_ROUND_UP(2 * 50 * 1000000, freq); >>> >>> gpiod_set_value(sensor->reset, 1); >>> - udelay(duration); >>> + fsleep(duration); >>> gpiod_set_value(sensor->reset, 0); >>> } else { >>> /* >> >> I think this is fine, so: >> >> Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> >> >> But: If we don't have COMMON_CLK (or rather, I think, HAVE_CLK), the >> freq will be zero at compile time. So won't the compiler give a warning >> for the DIV_ROUND_UP() call? >> >> Interestingly, for me, this doesn't give a div-by-zero warning: >> >> int x; >> int y = 0; >> x = DIV_ROUND_UP(10, y); >> >> but this does: >> >> int x; >> const int y = 0; >> x = DIV_ROUND_UP(10, y); >> >> And looks like this gives the warning too: >> >> int x; >> const int y = 0; >> if (y) >> x = DIV_ROUND_UP(10, y); >> >> So, I think, the code in the driver could fail to compile at some later >> point, if the compiler warnings are improved (?), or if someone adds a >> 'const' in front of 'long freq = clk_get_rate(sensor->clk);' line. >> >> Maybe worry about that if it actually happens =). > > Maybe :-) I would be tempted to make VIDEO_CAMERA_SENSOR depend on > COMMON_CLK. I think HAVE_CLK would be more correct. Tomi
On Wed, Dec 13, 2023 at 01:56:48PM +0200, Tomi Valkeinen wrote: > On 13/12/2023 13:48, Laurent Pinchart wrote: > > On Wed, Dec 13, 2023 at 01:40:54PM +0200, Tomi Valkeinen wrote: > >> On 13/12/2023 13:23, Arnd Bergmann wrote: > >>> From: Arnd Bergmann <arnd@arndb.de> > >>> > >>> With clang-16, building without COMMON_CLK triggers a range check on > >>> udelay() because of a constant division-by-zero calculation: > >>> > >>> ld.lld: error: undefined symbol: __bad_udelay > >>>>>> referenced by mt9m114.c > >>>>>> drivers/media/i2c/mt9m114.o:(mt9m114_power_on) in archive vmlinux.a > >>> > >>> In this configuration, the driver already fails to probe, before > >>> this function gets called, so it's enough to suppress the assertion. > >>> > >>> Do this by using fsleep(), which turns long delays into sleep() calls > >>> in place of the link failure. > >>> > >>> This is probably a good idea regardless to avoid overly long dynamic > >>> udelay() calls on a slow clock. > >>> > >>> Cc: Sakari Ailus <sakari.ailus@linux.intel.com> > >>> Fixes: 24d756e914fc ("media: i2c: Add driver for onsemi MT9M114 camera sensor") > >>> Signed-off-by: Arnd Bergmann <arnd@arndb.de> > >>> --- > >>> drivers/media/i2c/mt9m114.c | 2 +- > >>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>> > >>> diff --git a/drivers/media/i2c/mt9m114.c b/drivers/media/i2c/mt9m114.c > >>> index 0a22f328981d..68adaecaf481 100644 > >>> --- a/drivers/media/i2c/mt9m114.c > >>> +++ b/drivers/media/i2c/mt9m114.c > >>> @@ -2116,7 +2116,7 @@ static int mt9m114_power_on(struct mt9m114 *sensor) > >>> duration = DIV_ROUND_UP(2 * 50 * 1000000, freq); > >>> > >>> gpiod_set_value(sensor->reset, 1); > >>> - udelay(duration); > >>> + fsleep(duration); > >>> gpiod_set_value(sensor->reset, 0); > >>> } else { > >>> /* > >> > >> I think this is fine, so: > >> > >> Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > >> > >> But: If we don't have COMMON_CLK (or rather, I think, HAVE_CLK), the > >> freq will be zero at compile time. So won't the compiler give a warning > >> for the DIV_ROUND_UP() call? > >> > >> Interestingly, for me, this doesn't give a div-by-zero warning: > >> > >> int x; > >> int y = 0; > >> x = DIV_ROUND_UP(10, y); > >> > >> but this does: > >> > >> int x; > >> const int y = 0; > >> x = DIV_ROUND_UP(10, y); > >> > >> And looks like this gives the warning too: > >> > >> int x; > >> const int y = 0; > >> if (y) > >> x = DIV_ROUND_UP(10, y); > >> > >> So, I think, the code in the driver could fail to compile at some later > >> point, if the compiler warnings are improved (?), or if someone adds a > >> 'const' in front of 'long freq = clk_get_rate(sensor->clk);' line. > >> > >> Maybe worry about that if it actually happens =). > > > > Maybe :-) I would be tempted to make VIDEO_CAMERA_SENSOR depend on > > COMMON_CLK. > > I think HAVE_CLK would be more correct. Indeed. Only arch/m68k (for Coldfire), arch/mips (for BCM63xx and Lantiq) and arch/sh (for pre-DT platforms) still make use of HAVE_LEGACY_CLK. It takes a long time to address the last users of a legacy API, but it seems we could get there one day.
diff --git a/drivers/media/i2c/mt9m114.c b/drivers/media/i2c/mt9m114.c index 0a22f328981d..68adaecaf481 100644 --- a/drivers/media/i2c/mt9m114.c +++ b/drivers/media/i2c/mt9m114.c @@ -2116,7 +2116,7 @@ static int mt9m114_power_on(struct mt9m114 *sensor) duration = DIV_ROUND_UP(2 * 50 * 1000000, freq); gpiod_set_value(sensor->reset, 1); - udelay(duration); + fsleep(duration); gpiod_set_value(sensor->reset, 0); } else { /*