Hi Dirk,
On Wed, May 11, 2016 at 10:55 AM, Dirk Behme <[email protected]> wrote:
> On 11.05.2016 10:41, Geert Uytterhoeven wrote:
>> On Wed, May 11, 2016 at 10:39 AM, Dirk Behme <[email protected]>
>> wrote:
>>> On 11.05.2016 09:54, Geert Uytterhoeven wrote:
>>>> On Wed, May 11, 2016 at 7:29 AM, Dirk Behme <[email protected]>
>>>> wrote:
>>>>> --- /dev/null
>>>>> +++ b/drivers/misc/boot-mode-reg/core.c
>>>>> +/**
>>>>> + * boot_mode_reg_set() - record boot mode register value
>>>>> + * @mode: implementation-dependent boot mode register value
>>>>> + *
>>>>> + * Records the boot mode register value which may subsequently
>>>>> + * be retrieved using boot_mode_reg_get().
>>>>> + *
>>>>> + * return: 0 on success
>>>>> + */
>>>>> +int boot_mode_reg_set(u32 mode)
>>>>> +{
>>>>> + int err = -EBUSY;
>>>>> +
>>>>> + mutex_lock(&boot_mode_mutex);
>>>>> + if (!boot_mode_is_set) {
>>>>
>>>> You've dropped the check for calling this function a subsequent time
>>>> with
>>>> a different value of mode?
>>>
>>> Sometimes inverting 'complex' if statements is not that easy ;)
>>>
>>> You mean
>>>
>>> if (!boot_mode_is_set || boot_mode != mode)
>>
>>
>> No, De Morgan says
>>
>> if (!boot_mode_is_set || boot_mode == mode)
>
> Hmm, sorry if I don't have enough coffee today ;)
>
> But why should we do
>
> if (... boot_mode == mode)
> boot_mode = mode;
>
> ?
>
> Or in other words:
>
> We want to execute the if code if
>
> boot_mode_is_set is false
>
> or
>
> boot_mode is not equal mode, i.e. mode has changed
>
> ?
We want to fail if the mode has changed.
Doing the assignment a second time doesn't hurt.
But as this seems to hurt readability, the better solution may be:
if (!boot_mode_is_set) {
boot_mode = mode;
boot_mode_is_set = true;
err = 0;
} else if (boot_mode == mode) {
err = 0;
} else {
err = -EBUSY;
}
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
