Re: gpio patches in mmotm - Kernel

This is a discussion on Re: gpio patches in mmotm - Kernel ; Please, do not trim the CC: list. I've also added lkml. On Tue, 18 Mar 2008, Uwe Kleine-König wrote: > Guennadi Liakhovetski wrote: > > On Mon, 17 Mar 2008, Uwe Kleine-König wrote: > > > > > I'm nure ...

+ Reply to Thread
Results 1 to 7 of 7

Thread: Re: gpio patches in mmotm

  1. Re: gpio patches in mmotm

    Please, do not trim the CC: list. I've also added lkml.

    On Tue, 18 Mar 2008, Uwe Kleine-König wrote:

    > Guennadi Liakhovetski wrote:
    > > On Mon, 17 Mar 2008, Uwe Kleine-König wrote:
    > >
    > > > I'm nure sure if I like gpio_is_valid(). When do you think it should be
    > > > used? (i.e. in which situations gpio_request doesn't do the right
    > > > thing?)

    > >
    > > For example, in situations similar to what I have in mt9m001 and mt9v022
    > > camera drivers. Those cameras can be built with an i2c gpio extender,
    > > which can be used to switch between 8 and 10 bit data bus widths. But that
    > > extender is not always available. So, those drivers request a gpio, and if
    > > it is not available on the system, the gpio_is_valid() test fails.

    > I found your patch, but no tree where it applies. Can you point me to a
    > tree where it applies?


    These drivers are currently in the v4l-dvb tree
    http://git.kernel.org/?p=linux/kerne....git;a=summary in
    the devel branch.

    > Why isn't it enough that gpio_request fails in such a situation?


    I'm storing the GPIO number locally, and if the system doesn't have a
    valid GPIO for me, I'm storing an invalid GPIO number. Then at any time if
    the GPIO has to be used, I just verify if gpio_is_valid(), and if not,
    return an error code for this request, but the driver remains otherwise
    functional.

    Thanks
    Guennadi
    ---
    Guennadi Liakhovetski
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  2. Re: gpio patches in mmotm

    Hello Guennadi,


    Guennadi Liakhovetski wrote:
    > Please, do not trim the CC: list. I've also added lkml.

    Oh, thanks. I thought I'm used to hitting reply-to-all 8-(.
    I also added Andrew back (even though adding lkml might be just as good.
    :-))

    > On Tue, 18 Mar 2008, Uwe Kleine-KĞ–nig wrote:
    > > Guennadi Liakhovetski wrote:
    > > > On Mon, 17 Mar 2008, Uwe Kleine-KĞ–nig wrote:
    > > >
    > > > > I'm nure sure if I like gpio_is_valid(). When do you think it should be
    > > > > used? (i.e. in which situations gpio_request doesn't do the right
    > > > > thing?)
    > > >
    > > > For example, in situations similar to what I have in mt9m001 and mt9v022
    > > > camera drivers. Those cameras can be built with an i2c gpio extender,
    > > > which can be used to switch between 8 and 10 bit data bus widths. But that
    > > > extender is not always available. So, those drivers request a gpio, and if
    > > > it is not available on the system, the gpio_is_valid() test fails.

    > > I found your patch, but no tree where it applies. Can you point me to a
    > > tree where it applies?

    >
    > These drivers are currently in the v4l-dvb tree
    > http://git.kernel.org/?p=linux/kerne....git;a=summary in
    > the devel branch.

    OK, when I searched your driver I found the tree, but only looked in the
    master (=HEAD) branch.

    > > Why isn't it enough that gpio_request fails in such a situation?

    >
    > I'm storing the GPIO number locally, and if the system doesn't have a
    > valid GPIO for me, I'm storing an invalid GPIO number. Then at any time if
    > the GPIO has to be used, I just verify if gpio_is_valid(), and if not,
    > return an error code for this request, but the driver remains otherwise
    > functional.

    OK, so in your driver you have:

    if (gpio_is_valid(gpio)) {
    /* We have a data bus switch. */
    ret = gpio_request(gpio, "mt9m001");
    if (ret < 0) {
    dev_err(&mt9m001->client->dev, "Cannot get GPIO %u\n",
    gpio);
    return ret;
    }
    ret = gpio_direction_output(gpio, 0);
    if (ret < 0) {
    ...


    In my eyes the following is better:

    /* Do we have a data bus switch? */
    ret = gpio_request(gpio, "mt9m001");
    if (ret < 0) {
    if (ret != -EINVAL) {
    dev_err(...);
    return ret;
    }
    } else {
    ret = gpio_direction_output(gpio, 0);
    if (ret < 0) {
    ...

    Then you don't need to extend the API. Moreover with your variant the
    check that gpio is valid must be done twice[1].

    For me gpio_is_valid would only make sense if there might be situations
    where you want to know if a certain GPIO exists but even if it does you
    won't gpio_request it.

    Best regards
    Uwe

    [1] OK, gpio_is_valid and gpio_request might be inline functions, but
    for "my" architecture it is not.
    --
    Uwe Kleine-König, Software Engineer
    Digi International GmbH Branch Breisach, Küferstrasse 8, 79206 Breisach, Germany
    Tax: 315/5781/0242 / VAT: DE153662976 / Reg. Amtsgericht Dortmund HRB 13962
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  3. Re: gpio patches in mmotm

    On Tue, 8 Apr 2008, Uwe Kleine-König wrote:

    > > I'm storing the GPIO number locally, and if the system doesn't have a
    > > valid GPIO for me, I'm storing an invalid GPIO number. Then at any time if
    > > the GPIO has to be used, I just verify if gpio_is_valid(), and if not,
    > > return an error code for this request, but the driver remains otherwise
    > > functional.

    > OK, so in your driver you have:
    >
    > if (gpio_is_valid(gpio)) {
    > /* We have a data bus switch. */
    > ret = gpio_request(gpio, "mt9m001");
    > if (ret < 0) {
    > dev_err(&mt9m001->client->dev, "Cannot get GPIO %u\n",
    > gpio);
    > return ret;
    > }
    > ret = gpio_direction_output(gpio, 0);
    > if (ret < 0) {
    > ...
    >
    >
    > In my eyes the following is better:
    >
    > /* Do we have a data bus switch? */
    > ret = gpio_request(gpio, "mt9m001");
    > if (ret < 0) {
    > if (ret != -EINVAL) {
    > dev_err(...);
    > return ret;
    > }
    > } else {
    > ret = gpio_direction_output(gpio, 0);
    > if (ret < 0) {
    > ...


    Yes, you could do that. But then you have to test either before calling
    gpio_set_value_cansleep() or inside it. And the test you have to perform
    _is_ the validity check, so, you need it anyway.

    > Then you don't need to extend the API. Moreover with your variant the
    > check that gpio is valid must be done twice[1].


    Actually three times. The one before gpio_free() is not actually needed,
    right, it is anyway checked inside. But gpio_set_value_cansleep() doesn't
    check, so, it would be rude to call it with an invalid value.

    > [1] OK, gpio_is_valid and gpio_request might be inline functions, but
    > for "my" architecture it is not.


    Which arch is it? As I said, you could simplify the two specific camera
    drivers by removing the checks where they are redundant. But on other
    occasions the checks have to be done anyway, so, it is not a question of
    runtime performance (apart from maybe the difference between calling a
    function and executing inline), but just of an extra API member, which you
    can have different opinions about:-)

    Thanks
    Guennadi
    ---
    Guennadi Liakhovetski
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  4. Re: gpio patches in mmotm

    Hello Guennadi,

    Guennadi Liakhovetski wrote:
    > On Tue, 8 Apr 2008, Uwe Kleine-König wrote:
    >
    > > > I'm storing the GPIO number locally, and if the system doesn't have a
    > > > valid GPIO for me, I'm storing an invalid GPIO number. Then at any time if
    > > > the GPIO has to be used, I just verify if gpio_is_valid(), and if not,
    > > > return an error code for this request, but the driver remains otherwise
    > > > functional.

    > > OK, so in your driver you have:
    > >
    > > if (gpio_is_valid(gpio)) {
    > > /* We have a data bus switch. */
    > > ret = gpio_request(gpio, "mt9m001");
    > > if (ret < 0) {
    > > dev_err(&mt9m001->client->dev, "Cannot get GPIO %u\n",
    > > gpio);
    > > return ret;
    > > }
    > > ret = gpio_direction_output(gpio, 0);
    > > if (ret < 0) {
    > > ...
    > >
    > >
    > > In my eyes the following is better:
    > >
    > > /* Do we have a data bus switch? */
    > > ret = gpio_request(gpio, "mt9m001");
    > > if (ret < 0) {
    > > if (ret != -EINVAL) {
    > > dev_err(...);
    > > return ret;
    > > }
    > > } else {
    > > ret = gpio_direction_output(gpio, 0);
    > > if (ret < 0) {
    > > ...

    >
    > Yes, you could do that. But then you have to test either before calling
    > gpio_set_value_cansleep() or inside it. And the test you have to perform
    > _is_ the validity check, so, you need it anyway.

    Ah, OK. Before setting the value you must assert that you *requested* the
    gpio (and not that it is valid). In your driver that seems to be
    equivalent.

    Still I would prefer to store the information that the additional GPIO
    is not available explicitly in the driver (e.g. by setting gpio = -1)
    because gpio != -1 might be cheaper than gpio_is_valid(gpio).

    And I don't like extending an API only to provide a second way to do
    something without saving code or performance.

    > > Then you don't need to extend the API. Moreover with your variant the
    > > check that gpio is valid must be done twice[1].

    >
    > Actually three times. The one before gpio_free() is not actually needed,
    > right, it is anyway checked inside.

    That's wrong. gpio_free as provided by gpiolib does the check, the
    variant of ns9xxx does not. I think it's not explicit, but as gpio_free
    must only be called on a requested gpio, I don't see why this check
    should be done by gpio_free.

    > But gpio_set_value_cansleep() doesn't
    > check, so, it would be rude to call it with an invalid value.
    >
    > > [1] OK, gpio_is_valid and gpio_request might be inline functions, but
    > > for "my" architecture it is not.

    >
    > Which arch is it?

    arch/arm/mach-ns9xxx. It's not (yet) fully supported in vanilla, but it
    includes support for different SOCs that have a different handling of
    their GPIOs. E.g. the ns9360 has one gpio configuration register per 8
    gpios, the ns9215 has one per 4 gpios. Or another thing: ns9215 has
    108 gpios, ns9210 has only 54 where the first 50 gpios are identical to
    the first 50 of ns9215, and the last 4 gpios are identical to gpios
    105-108 on ns9215. So gpio_is_valid for ns9xxx has to look like:

    int gpio_is_valid(int gpio)
    {
    ...
    if (processor_is_ns9210())
    return gpio >= 0 && gpio < 108 && !(gpio >= 50 && gpio < 105);
    ...
    }

    (In my eyes that hole is ugly, but with it can calculate the address of
    the configuration register without case splitting and can handle ns9215
    and ns9210 identically---apart from the is-valid check.)

    If you're deeper interested you can compare
    - http://ftp1.digi.com/support/documen...90000675_C.pdf (ns9360);
    - http://ftp1.digi.com/support/documen...90000847_B.pdf (ns9215);
    and
    - http://ftp1.digi.com/support/documen...90000846_B.pdf (ns9210).

    > As I said, you could simplify the two specific camera
    > drivers by removing the checks where they are redundant. But on other
    > occasions the checks have to be done anyway, so, it is not a question of
    > runtime performance (apart from maybe the difference between calling a
    > function and executing inline), but just of an extra API member, which you
    > can have different opinions about:-)

    So you reason that the alternative approach allows only a slight
    simplification and so is not worth considering? But obviously
    yes, I have a different opinion. :-)

    Best regards
    Uwe

    --
    Uwe Kleine-König, Software Engineer
    Digi International GmbH Branch Breisach, Küferstrasse 8, 79206 Breisach, Germany
    Tax: 315/5781/0242 / VAT: DE153662976 / Reg. Amtsgericht Dortmund HRB 13962
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  5. Re: gpio patches in mmotm

    On Tue, 8 Apr 2008, Uwe Kleine-König wrote:

    > arch/arm/mach-ns9xxx. It's not (yet) fully supported in vanilla, but it
    > includes support for different SOCs that have a different handling of
    > their GPIOs. E.g. the ns9360 has one gpio configuration register per 8
    > gpios, the ns9215 has one per 4 gpios. Or another thing: ns9215 has
    > 108 gpios, ns9210 has only 54 where the first 50 gpios are identical to
    > the first 50 of ns9215, and the last 4 gpios are identical to gpios
    > 105-108 on ns9215. So gpio_is_valid for ns9xxx has to look like:
    >
    > int gpio_is_valid(int gpio)
    > {
    > ...
    > if (processor_is_ns9210())
    > return gpio >= 0 && gpio < 108 && !(gpio >= 50 && gpio < 105);
    > ...
    > }
    >
    > (In my eyes that hole is ugly, but with it can calculate the address of
    > the configuration register without case splitting and can handle ns9215
    > and ns9210 identically---apart from the is-valid check.)


    Ok, I thought it would be something like that. I think, these are two
    different things: GPIO valid and GPIO currently physically existing.
    gpio_is_valid() is a test whether the number being tested at all stands a
    chance to be a GPIO number on this architecture. As you see in
    include/asm-generic/gpio.h it only compares against ARCH_NR_GPIOS, which
    is just the theoretically highest GPIO number. It says nothing about
    whether or not all valid GPIOs are actually present on the system. Think
    about GPIO expanders, there might or might not be one currently available
    on the system. Still gpio_is_valid() will return the same result for any
    given number. PXA CPUs have the same "feature" as ns9xxx - different
    models have differeng GPIOs, and platform add their own GPIO controllers,
    which are often placed at a fixed start number, which means, on some CPUs
    there will be holes too. And gpio_is_valid is not (and should not be)
    checking for those - this is already the task for request_gpio().

    > So you reason that the alternative approach allows only a slight
    > simplification and so is not worth considering? But obviously
    > yes, I have a different opinion. :-)


    No, my reason is that I didn't want to put "intimate knowledge" of GPIO
    interna, like "-1 is not a valid GPIO" in the driver but use an
    abstraction instead. My original proposal was to introduce just one
    NO_GPIO macro to test against, however, David nicely managed to persuade
    me, that the gpio_is_valid approach is better. Unfortunately, I cannot
    argument as nicely as he did, maybe looking through his emails in LKML
    archives will help you:-)

    Thanks
    Guennadi
    ---
    Guennadi Liakhovetski
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  6. Re: gpio patches in mmotm

    Hello Guennadi,

    Guennadi Liakhovetski wrote:
    > My original proposal was to introduce just one
    > NO_GPIO macro to test against, however, David nicely managed to persuade
    > me, that the gpio_is_valid approach is better. Unfortunately, I cannot
    > argument as nicely as he did, maybe looking through his emails in LKML
    > archives will help you:-)

    I'm unable to find these mails. (Using gmane, looking for mails by
    David matching gpio_is_valid, the oldest post I can find is
    http://article.gmane.org/gmane.linux.kernel/637654.)

    Can you please point out the discussion in the archives?

    Best regards
    Uwe

    --
    Uwe Kleine-König, Software Engineer
    Digi International GmbH Branch Breisach, Küferstrasse 8, 79206 Breisach, Germany
    Tax: 315/5781/0242 / VAT: DE153662976 / Reg. Amtsgericht Dortmund HRB 13962
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  7. Re: gpio patches in mmotm

    On Wed, 9 Apr 2008, Uwe Kleine-König wrote:

    > Hello Guennadi,
    >
    > Guennadi Liakhovetski wrote:
    > > My original proposal was to introduce just one
    > > NO_GPIO macro to test against, however, David nicely managed to persuade
    > > me, that the gpio_is_valid approach is better. Unfortunately, I cannot
    > > argument as nicely as he did, maybe looking through his emails in LKML
    > > archives will help you:-)

    > I'm unable to find these mails. (Using gmane, looking for mails by
    > David matching gpio_is_valid, the oldest post I can find is
    > http://article.gmane.org/gmane.linux.kernel/637654.)
    >
    > Can you please point out the discussion in the archives?


    See also

    http://marc.info/?t=120179335100005&r=1&w=2
    http://marc.info/?t=120267097400002&r=1&w=2

    Thanks
    Guennadi
    ---
    Guennadi Liakhovetski
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

+ Reply to Thread