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

[Jiasheng Jiang]

On Mon, Nov 11, 2024 at 4:15 PM David Lechner <dlechner at baylibre.com> wrote:
>
> On 11/11/24 2:36 PM, Jiasheng Jiang wrote:
> > 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
>
>
> Usually, there is a default: case as well, so we could move the
> final return and make it look like this:
>
>         switch (mask) {
>         case IIO_CHAN_INFO_RAW:
>                 return regmap_write(priv->regmap, TIM_CNT, val);
>
>         case IIO_CHAN_INFO_SCALE:
>                 /* fixed scale */
>                 return -EINVAL;
>
>         case IIO_CHAN_INFO_ENABLE: {
>                 guard(mutex)(&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);
>                         }
>                 }
>                 return 0;
>         }
>                 default:
>                         return -EINVAL;
>         }
>
>
> And it is unusual, but I found kvm_arm_pmu_v3_get_attr() that
> also has this double inline brace at the end of a switch statement.
>
>         }
>         }
>
> So even if it doesn't look so nice, it does seem to be the
> "correct" style.

Thanks, I will submit a v4 patch.

-Jiasheng