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.
