On Mon, Nov 11, 2024 at 4:15 PM David Lechner <dlechner@xxxxxxxxxxxx> wrote:
>
> On 11/11/24 2:36 PM, Jiasheng Jiang wrote:
> > On Mon, Nov 11, 2024 at 2:45 PM David Lechner <dlechner@xxxxxxxxxxxx> 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@xxxxxxxxx>
> >>> ---
> >>> 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
