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

[David Lechner <dlechner@baylibre.com>]

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