Hi Jonathan,
On Sat, Nov 23, 2024 at 10:08 AM Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
>
> On Mon, 11 Nov 2024 22:23:10 +0000
> Jiasheng Jiang <jiashengjiangcool@xxxxxxxxx> wrote:
>
> > Add check for the return value of clk_enable() in order to catch the
> > potential exception.
> >
> > Signed-off-by: Jiasheng Jiang <jiashengjiangcool@xxxxxxxxx>
> Hi Jiasheng,
>
>
> Should definitely mention the changes to use guard() to simplify
> the resulting code.
Thanks, I have revised the "v2 -> v3" in the Changelog to clarify the changes.
> One minor comment on the code inline. Otherwise looks good to me.
>
> Thanks,
>
> Jonathan
>
> > ---
> > Changelog:
> >
> > v3 -> v4:
> >
> > 1. Place braces around the case body.
> >
> > v2 -> v3:
> >
> > 1. Simplify code with cleanup helpers.
> >
> > v1 -> v2:
> >
> > 1. Remove unsuitable dev_err_probe().
>
> > @@ -482,6 +484,7 @@ static int stm32_counter_write_raw(struct iio_dev *indio_dev,
> > int val, int val2, long mask)
> > {
> > struct stm32_timer_trigger *priv = iio_priv(indio_dev);
> > + int ret;
> >
> > switch (mask) {
> > case IIO_CHAN_INFO_RAW:
> > @@ -491,12 +494,14 @@ static int stm32_counter_write_raw(struct iio_dev *indio_dev,
> > /* fixed scale */
> > return -EINVAL;
> >
> > - case IIO_CHAN_INFO_ENABLE:
> > - mutex_lock(&priv->lock);
> > + case IIO_CHAN_INFO_ENABLE: {
> > + guard(mutex)(&priv->lock);
> > if (val) {
> > if (!priv->enabled) {
> > priv->enabled = true;
> > - clk_enable(priv->clk);
> > + ret = clk_enable(priv->clk);
> > + if (ret)
> > + return ret;
> > }
> > regmap_set_bits(priv->regmap, TIM_CR1, TIM_CR1_CEN);
> > } else {
> > @@ -506,9 +511,10 @@ static int stm32_counter_write_raw(struct iio_dev *indio_dev,
> > clk_disable(priv->clk);
> > }
> > }
> > - mutex_unlock(&priv->lock);
> > +
> > return 0;
> > }
> Add a default for reasons David mentioned and it also makes it visually clear
> that we expect to get in here for other cases but they are all errors.
> default:
> return -EINVAL;
> > + }
> >
> And drop this return as unreachable.
>
> > return -EINVAL;
> > }
Thanks, I have submitted v5 to include a default and remove the return.
-Jiasheng
