Re: [PATCH] media: i2c: mt9m114: add CONFIG_COMMON_CLK dependency

[Laurent Pinchart]

On Wed, Dec 13, 2023 at 09:10:19AM +0000, Sakari Ailus wrote:
> Hi Arnd,
> 
> On Wed, Dec 13, 2023 at 09:39:01AM +0100, Arnd Bergmann wrote:
> > On Wed, Dec 13, 2023, at 09:09, Sakari Ailus wrote:
> > > On Tue, Dec 12, 2023 at 10:18:04PM +0100, Arnd Bergmann wrote:
> > >> From: Arnd Bergmann <arnd@xxxxxxxx>
> > >> 
> > >> 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
> > >> 
> > >> Avoid this by adding a Kconfig dependency that avoids the broken build.
> > >
> > > This sounds like an odd way to fix an issue with udelay(). On which arch
> > > does it happen? Wouldn't it be better to fix the udelay() problem in the
> > > source?
> > >
> > > A quick check reveals there are about 2400 files using udelay.
> > 
> > I observed this on arm, but same sanity check exists on arc, m68k,
> > microblaze, nios2 and xtensa, all of which try to discourage
> > overly large constant delays busy loops. On Arm that limit is
> > any delay of over 2 miliseconds, at which time a driver should
> > generally use either msleep() to avoid a busy-loop, or (in extreme
> > cases) mdelay().
> > 
> > I first tried to rewrite the mt9m114_power_on() function to
> > have an upper bound on the udelay, but that didn't avoid the
> > link error because it still got into undefined C. Disabling
> > the driver without COMMON_CLK seemed easier since it already
> > fails to probe if mt9m114_clk_init() runs into a zero clk.
> > 
> > Maybe we could just fall back to the soft reset when the
> > clock rate is unknown?
> 
> The datasheet says the input clock range is between 6 MHz and 54 MHz. We
> could check that, and return an error if it's not in the range. The rate is
> needed for other reasons, too, and the sensor is unlikely to work if it's
> (more than little) off.
> 
> I wonder if this could address the Clang 16 issue, too? I'd replace
> udelay() with fsleep() in any case, and at the very least Clang should be
> happy with this.

I'm fine with both of those changes.

> > diff --git a/drivers/media/i2c/mt9m114.c b/drivers/media/i2c/mt9m114.c
> > index 0a22f328981d..88a67568edf8 100644
> > --- a/drivers/media/i2c/mt9m114.c
> > +++ b/drivers/media/i2c/mt9m114.c
> > @@ -2092,6 +2092,7 @@ static void mt9m114_ifp_cleanup(struct mt9m114 *sensor)
> >  
> >  static int mt9m114_power_on(struct mt9m114 *sensor)
> >  {
> > +       long freq;
> >         int ret;
> >  
> >         /* Enable power and clocks. */
> > @@ -2104,9 +2105,10 @@ static int mt9m114_power_on(struct mt9m114 *sensor)
> >         if (ret < 0)
> >                 goto error_regulator;
> >  
> > +       freq = clk_get_rate(sensor->clk);
> > +
> >         /* Perform a hard reset if available, or a soft reset otherwise. */
> > -       if (sensor->reset) {
> > -               long freq = clk_get_rate(sensor->clk);
> > +       if (sensor->reset && freq) {
> >                 unsigned int duration;
> >  
> >                 /*

-- 
Regards,

Laurent Pinchart