[PATCH v3] iio: trigger: stm32-timer-trigger: Add check for clk_enable()

[Jiasheng Jiang]

On Mon, Nov 11, 2024 at 2:45 PM David Lechner <dlechner at baylibre.com> wrote:
>
> On 11/11/24 1:19 PM, Jiasheng Jiang wrote:
> > Add check for the return value of clk_enable() in order to catch the
> > potential exception.
> >
> > Signed-off-by: Jiasheng Jiang <jiashengjiangcool at gmail.com>
> > ---
> > Changelog:
> >
> > v2 -> v3:
> >
> > 1. Simplify code with cleanup helpers.
> >
> > v1 -> v2:
> >
> > 1. Remove unsuitable dev_err_probe().
> > ---
>
> ...
>
> > @@ -492,21 +495,25 @@ static int stm32_counter_write_raw(struct iio_dev *indio_dev,
> >               return -EINVAL;
> >
> >       case IIO_CHAN_INFO_ENABLE:
> > -             mutex_lock(&priv->lock);
> > -             if (val) {
> > -                     if (!priv->enabled) {
> > -                             priv->enabled = true;
> > -                             clk_enable(priv->clk);
> > -                     }
> > -                     regmap_set_bits(priv->regmap, TIM_CR1, TIM_CR1_CEN);
> > -             } else {
> > -                     regmap_clear_bits(priv->regmap, TIM_CR1, TIM_CR1_CEN);
> > -                     if (priv->enabled) {
> > -                             priv->enabled = false;
> > -                             clk_disable(priv->clk);
> > +
> > +             scoped_guard(mutex, &priv->lock) {
> > +                     if (val) {
> > +                             if (!priv->enabled) {
> > +                                     priv->enabled = true;
> > +                                     ret = clk_enable(priv->clk);
> > +                                     if (ret)
> > +                                             return ret;
> > +                             }
> > +                             regmap_set_bits(priv->regmap, TIM_CR1, TIM_CR1_CEN);
> > +                     } else {
> > +                             regmap_clear_bits(priv->regmap, TIM_CR1, TIM_CR1_CEN);
> > +                             if (priv->enabled) {
> > +                                     priv->enabled = false;
> > +                                     clk_disable(priv->clk);
> > +                             }
> >                       }
> >               }
> > -             mutex_unlock(&priv->lock);
> > +
> >               return 0;
> >       }
>
>
> Another way to do this that avoids changing the indent
> so much is placing braces around the case body like this.
> This also avoids the compile error from using guard after
> case directly.
>
>
>         case IIO_CHAN_INFO_ENABLE: {
>                 guard(mutex)(&priv->lock);
>
>                 if (val) {
>                         if (!priv->enabled) {
>                                 priv->enabled = true;
>                                 ret = clk_enable(priv->clk);
>                                 if (ret)
>                                         return ret;
>                         }
>                         regmap_set_bits(priv->regmap, TIM_CR1, TIM_CR1_CEN);
>                 } else {
>                         regmap_clear_bits(priv->regmap, TIM_CR1, TIM_CR1_CEN);
>                         if (priv->enabled) {
>                                 priv->enabled = false;
>                                 clk_disable(priv->clk);
>                         }
>                 }
>
>                 return 0;
>         }
>

Looks great.
But there is no indentation between "switch" and "case".
As a result, the closing braces of "switch" and "case" will
be placed in the same column.

Like this:

switch(mask) {
case IIO_CHAN_INFO_ENABLE: {

}
}

-Jiasheng