| Message ID | 20231213112322.1655236-1-arnd@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] media: i2c: mt9m114: use fsleep() in place of udelay() | expand |
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
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 { /*