* [PATCH v3] iio: trigger: stm32-timer-trigger: Add check for clk_enable()
@ 2024-11-11 19:19 Jiasheng Jiang
2024-11-11 19:45 ` David Lechner
0 siblings, 1 reply; 5+ messages in thread
From: Jiasheng Jiang @ 2024-11-11 19:19 UTC (permalink / raw)
To: jic23
Cc: lars, mcoquelin.stm32, alexandre.torgue, u.kleine-koenig,
tgamblin, fabrice.gasnier, benjamin.gaignard, lee, linux-iio,
linux-stm32, linux-arm-kernel, linux-kernel, Jiasheng Jiang
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().
---
drivers/iio/trigger/stm32-timer-trigger.c | 64 +++++++++++++----------
1 file changed, 37 insertions(+), 27 deletions(-)
diff --git a/drivers/iio/trigger/stm32-timer-trigger.c b/drivers/iio/trigger/stm32-timer-trigger.c
index 0684329956d9..9fb4f7eefa86 100644
--- a/drivers/iio/trigger/stm32-timer-trigger.c
+++ b/drivers/iio/trigger/stm32-timer-trigger.c
@@ -119,7 +119,7 @@ static int stm32_timer_start(struct stm32_timer_trigger *priv,
unsigned int frequency)
{
unsigned long long prd, div;
- int prescaler = 0;
+ int prescaler = 0, ret;
u32 ccer;
/* Period and prescaler values depends of clock rate */
@@ -150,10 +150,12 @@ static int stm32_timer_start(struct stm32_timer_trigger *priv,
if (ccer & TIM_CCER_CCXE)
return -EBUSY;
- mutex_lock(&priv->lock);
+ guard(mutex)(&priv->lock);
if (!priv->enabled) {
priv->enabled = true;
- clk_enable(priv->clk);
+ ret = clk_enable(priv->clk);
+ if (ret)
+ return ret;
}
regmap_write(priv->regmap, TIM_PSC, prescaler);
@@ -173,7 +175,6 @@ static int stm32_timer_start(struct stm32_timer_trigger *priv,
/* Enable controller */
regmap_set_bits(priv->regmap, TIM_CR1, TIM_CR1_CEN);
- mutex_unlock(&priv->lock);
return 0;
}
@@ -307,7 +308,7 @@ static ssize_t stm32_tt_store_master_mode(struct device *dev,
struct stm32_timer_trigger *priv = dev_get_drvdata(dev);
struct iio_trigger *trig = to_iio_trigger(dev);
u32 mask, shift, master_mode_max;
- int i;
+ int i, ret;
if (stm32_timer_is_trgo2_name(trig->name)) {
mask = TIM_CR2_MMS2;
@@ -322,15 +323,16 @@ static ssize_t stm32_tt_store_master_mode(struct device *dev,
for (i = 0; i <= master_mode_max; i++) {
if (!strncmp(master_mode_table[i], buf,
strlen(master_mode_table[i]))) {
- mutex_lock(&priv->lock);
+ guard(mutex)(&priv->lock);
if (!priv->enabled) {
/* Clock should be enabled first */
priv->enabled = true;
- clk_enable(priv->clk);
+ ret = clk_enable(priv->clk);
+ if (ret)
+ return ret;
}
regmap_update_bits(priv->regmap, TIM_CR2, mask,
i << shift);
- mutex_unlock(&priv->lock);
return len;
}
}
@@ -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:
@@ -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;
}
@@ -601,7 +608,7 @@ static int stm32_set_enable_mode(struct iio_dev *indio_dev,
unsigned int mode)
{
struct stm32_timer_trigger *priv = iio_priv(indio_dev);
- int sms = stm32_enable_mode2sms(mode);
+ int sms = stm32_enable_mode2sms(mode), ret;
if (sms < 0)
return sms;
@@ -609,12 +616,15 @@ static int stm32_set_enable_mode(struct iio_dev *indio_dev,
* Triggered mode sets CEN bit automatically by hardware. So, first
* enable counter clock, so it can use it. Keeps it in sync with CEN.
*/
- mutex_lock(&priv->lock);
- if (sms == 6 && !priv->enabled) {
- clk_enable(priv->clk);
- priv->enabled = true;
+ scoped_guard(mutex, &priv->lock) {
+ if (sms == 6 && !priv->enabled) {
+ ret = clk_enable(priv->clk);
+ if (ret)
+ return ret;
+
+ priv->enabled = true;
+ }
}
- mutex_unlock(&priv->lock);
regmap_update_bits(priv->regmap, TIM_SMCR, TIM_SMCR_SMS, sms);
--
2.25.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v3] iio: trigger: stm32-timer-trigger: Add check for clk_enable()
2024-11-11 19:19 [PATCH v3] iio: trigger: stm32-timer-trigger: Add check for clk_enable() Jiasheng Jiang
@ 2024-11-11 19:45 ` David Lechner
2024-11-11 20:36 ` Jiasheng Jiang
0 siblings, 1 reply; 5+ messages in thread
From: David Lechner @ 2024-11-11 19:45 UTC (permalink / raw)
To: Jiasheng Jiang, jic23
Cc: lars, mcoquelin.stm32, alexandre.torgue, u.kleine-koenig,
tgamblin, fabrice.gasnier, benjamin.gaignard, lee, linux-iio,
linux-stm32, linux-arm-kernel, linux-kernel
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;
}
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v3] iio: trigger: stm32-timer-trigger: Add check for clk_enable()
2024-11-11 19:45 ` David Lechner
@ 2024-11-11 20:36 ` Jiasheng Jiang
2024-11-11 21:14 ` David Lechner
0 siblings, 1 reply; 5+ messages in thread
From: Jiasheng Jiang @ 2024-11-11 20:36 UTC (permalink / raw)
To: David Lechner
Cc: jic23, lars, mcoquelin.stm32, alexandre.torgue, u.kleine-koenig,
tgamblin, fabrice.gasnier, benjamin.gaignard, lee, linux-iio,
linux-stm32, linux-arm-kernel, linux-kernel
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v3] iio: trigger: stm32-timer-trigger: Add check for clk_enable()
2024-11-11 20:36 ` Jiasheng Jiang
@ 2024-11-11 21:14 ` David Lechner
2024-11-11 22:18 ` Jiasheng Jiang
0 siblings, 1 reply; 5+ messages in thread
From: David Lechner @ 2024-11-11 21:14 UTC (permalink / raw)
To: Jiasheng Jiang
Cc: jic23, lars, mcoquelin.stm32, alexandre.torgue, u.kleine-koenig,
tgamblin, fabrice.gasnier, benjamin.gaignard, lee, linux-iio,
linux-stm32, linux-arm-kernel, linux-kernel
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.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v3] iio: trigger: stm32-timer-trigger: Add check for clk_enable()
2024-11-11 21:14 ` David Lechner
@ 2024-11-11 22:18 ` Jiasheng Jiang
0 siblings, 0 replies; 5+ messages in thread
From: Jiasheng Jiang @ 2024-11-11 22:18 UTC (permalink / raw)
To: David Lechner
Cc: jic23, lars, mcoquelin.stm32, alexandre.torgue, u.kleine-koenig,
tgamblin, fabrice.gasnier, benjamin.gaignard, lee, linux-iio,
linux-stm32, linux-arm-kernel, linux-kernel
On Mon, Nov 11, 2024 at 4:15 PM David Lechner <dlechner@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@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.
Thanks, I will submit a v4 patch.
-Jiasheng
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2024-11-11 22:31 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-11-11 19:19 [PATCH v3] iio: trigger: stm32-timer-trigger: Add check for clk_enable() Jiasheng Jiang
2024-11-11 19:45 ` David Lechner
2024-11-11 20:36 ` Jiasheng Jiang
2024-11-11 21:14 ` David Lechner
2024-11-11 22:18 ` Jiasheng Jiang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox