Prev: OMAP: DSS2: Add Innolux 7" display for DEVKIT8000
Next: fix kfifo miss use of nozami.c
From: Stanislav O. Bezzubtsev on 12 Feb 2010 02:30

11.02.2010, � 23:58, Pavel Machek �������(�):

> On Thu 2010-02-11 14:35:14, Bill Gatliff wrote:
>> Pavel Machek wrote:
>>>> +static void
>>>> +gpio_pwm_work (struct work_struct *work)
>>>> +{
>>>> + struct gpio_pwm *gp = container_of(work, struct gpio_pwm, work);
>>>> +
>>>> + if (gp->active)
>>>> + gpio_direction_output(gp->gpio, gp->polarity ? 1 : 0);
>>>> + else
>>>> + gpio_direction_output(gp->gpio, gp->polarity ? 0 : 1);
>>>> +}
>>>>
>>>
>>> ...polarity ^ active ?
>>>
>>
>> ... except that if polarity and/or active are >1, I don't send the
>> values 1 or 0 to gpio_direction_output. I don't know if the API is
>> specifically intended to accept nonzero values that are greater than 1.
>
> !polarity ^ !active ? :-).


One the one hand that wouldn't be 100% right because according to ANSI C !(0) is just != 0 but no one says it is 1.
On another hand as far as I can see polarity and active fields are both defined as "unsigned long <name> :1" in the gpio_pwm structure. And that means they can be equal only to 0 or 1. So simple (polarity ^ active) is the right choice as far as the original decision.
What is strange for me is that resulting control flow is not right representation of what is happening. I mean that actually one should perform a call to an external function with an argument that depends on values of two variables. While this code is equal to call function A if some variable is not equal to 0 with argument depending on value of some other variable else call function B. I hope you understood what I'm trying to say.
Therefore even if you wish to leave if statement (not sure that gcc would optimize that) the right control flow representation should be the following:
......
if (gp->active)
value = ((gp->polarity)?1:0);
else
value = ((gp->polarity)?0:1);

gpio_direction_output(gp->gpio, value);
......

The second strange thing is "unsigned long <name>:1". I'm not sure but as far as I can remember the right way to define several-bits field is "int <name>:1". But I might be mistaken.


Best regards. Stas.



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: H Hartley Sweeten on 12 Feb 2010 09:00
Thursday, February 11, 2010 1:35 PM, Bill Gatliff wrote:
> Pavel Machek wrote:
>>> +static void
>>> +gpio_pwm_work (struct work_struct *work)
>>> +{
>>> + struct gpio_pwm *gp = container_of(work, struct gpio_pwm, work);
>>> +
>>> + if (gp->active)
>>> + gpio_direction_output(gp->gpio, gp->polarity ? 1 : 0);
>>> + else
>>> + gpio_direction_output(gp->gpio, gp->polarity ? 0 : 1);
>>> +}
>>>
>>
>> ...polarity ^ active ?
>>
>
> ... except that if polarity and/or active are >1, I don't send the
> values 1 or 0 to gpio_direction_output. I don't know if the API is
> specifically intended to accept nonzero values that are greater than 1.

FWIW, the gpiolib API will accept any non-zero value to "set" a gpio pin
and a zero value to "clear" the pin.

For that matter, some of the gpiolib drivers end up returning the bit
position mask for a given gpio pin and not 1 or 0. For instance if the
gpio pin in question is bit 6 in an 8-bit register and it is set, a
__gpio_get_value will end up returning 0x40 and not '1'.

Regards,
Hartley
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: H Hartley Sweeten on 16 Feb 2010 13:20
On Friday, February 12, 2010 9:27 AM, Bill Gatliff wrote:
> H Hartley Sweeten wrote:
>>
>> FWIW, the gpiolib API will accept any non-zero value to "set" a gpio pin
>> and a zero value to "clear" the pin.
>>
>
> It makes me sort of cringe to say this, but I'm going to assume that the
> API is intended to work that way so that I can take advantage of it.
> But I'd love to someday have the reassurance that gpiolib really *is*
> intended to work that way (might be a bad idea though, see below). Call
> me paranoid, but I've lost a lot of hair over the years undoing the
> damage of similar failed assumptions.
>
> I've found the Linux kernel code to be refreshingly forgiving of such
> things, however, so I'm willing to risk it this time. :)
>
>> For that matter, some of the gpiolib drivers end up returning the bit
>> position mask for a given gpio pin and not 1 or 0. For instance if the
>> gpio pin in question is bit 6 in an 8-bit register and it is set, a
>> __gpio_get_value will end up returning 0x40 and not '1'.
>>
>
> Who's to say that gpios should always be boolean? On the output side, a
> "gpio" that's four bits wide might be a useful way of dealing with bar
> graphs, seven-segment displays, etc. (but more specialized abstractions
> might be more appropriate, of course).

I think the original intention of gpiolib was to deal with individual pins.

> A two-bit "gpio" input might make it easier to deal with rotary encoders.

True. But a two-bit "gpio" is no longer a "pin" it's now a "port".

I have been messing with a "port" extension for gpiolib but it's not even
close to being mergable yet. But, that's a different topic...

> But for now, GPIOs are assumed to be booleans and that's why I'm
> hesitant to feed the API non-boolean values. Someday, those values
> might mean something subtly but disastrously different. And given my
> luck lately with such things...

True. As such just use ! and !! to make the values boolean.

gpio is bit 6
val = 0x40 -> !(0x40) = 0
val = 0x40 -> !!(0x40) = 1

Just my .02... Regards,
Hartley
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
 | 
Pages: 1
Prev: OMAP: DSS2: Add Innolux 7" display for DEVKIT8000
Next: fix kfifo miss use of nozami.c