[patch/rfc 2/4] pcf875x I2C GPIO expander driver - Kernel

This is a discussion on [patch/rfc 2/4] pcf875x I2C GPIO expander driver - Kernel ; On Friday 04 April 2008, Jean Delvare wrote: > > +static ssize_t gpio_direction_store(struct device *dev, > > +*************struct device_attribute *attr, const char *buf, size_t size) > > +{ > > +*****const struct gpio_desc *gdesc = dev_get_drvdata(dev); > > +*****int d, ...

+ Reply to Thread
Page 2 of 2 FirstFirst 1 2
Results 21 to 23 of 23

Thread: [patch/rfc 2/4] pcf875x I2C GPIO expander driver

  1. Re: [patch/rfc 2/4] pcf875x I2C GPIO expander driver

    On Friday 04 April 2008, Jean Delvare wrote:
    > > +static ssize_t gpio_direction_store(struct device *dev,
    > > +*************struct device_attribute *attr, const char *buf, size_t size)
    > > +{
    > > +*****const struct gpio_desc *gdesc = dev_get_drvdata(dev);
    > > +*****int d, n = gdesc - gpio_desc;
    > > +
    > > +*****if (size >= 3 && !strncmp(buf, "out", 3)) {
    > > +*************d = 1;
    > > +*****} else if (size >= 2 && !strncmp(buf, "in", 2)) {
    > > +*************d = 0;

    >
    > As far as I know, the string you receive from sysfs is guaranteed to be
    > NUL-terminated, so the size checks are not needed.
    >
    > > +*****} else {
    > > +*************d = simple_strtoul(buf, NULL, 0);

    >
    > This exposes to user-space the so far internal-only decision to encode
    > output as 1 and input as 0. I don't see much benefit in doing this, in
    > particular when you don't check for errors so for example an input of
    > "Out" would result in value 0 i.e. input mode. I'd rather support only
    > "in" and "out" as valid values and return -EINVAL otherwise.


    So, after trimming trailing whitespace:

    if (strcmp(buf, "out") == 0)
    gpio_direction_output(...)
    else if (strcmp(buf, "in") == 0)
    gpio_direction_input(...)

    Yes, that'd be a lot better.

    >
    > > +*****}
    > > +
    > > +*****if (d)
    > > +*************gpio_direction_output(n, 0);
    > > +*****else
    > > +*************gpio_direction_input(n);
    > > +
    > > +*****return size;
    > > +}



    --
    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: userspace GPIO access (WAS: [patch/rfc 2/4] pcf875x ...)

    On Thursday 03 April 2008, Ben Nizette wrote:
    > David, you're kinda the gatekeeper here; any input from you on which
    > approach is to be preferred, essential features etc?


    I won't much care about /dev/... vs /sys/... though I'd
    probably have used sysfs myself (just because it's much
    simpler and doesn't need to imply mdev/udev and classes).

    The configuration part of each driver bothers me:

    - Mike's simple_gpio requires manual kernel config
    to set up the platform_device nodes ... and thus
    rules out the first usage scenarios I ever heard
    of for such a userspace mechanism.

    - Trent's gpio_class exposes all GPIOs, even ones
    that are claimed by kernel drivers ... and thus
    makes it easy to clobber kernel driver state.
    (Plus it won't work on most built-in GPIOs, since
    they by and large don't have parent devices.)

    What I'd like to see is userspace config commands to
    cause the gpio_request() ... *maybe* something like

    echo 42 foo 0 > .../gpio_config

    ... causing error-checked versions of:

    gpio_request(42, "foo")
    gpio_direction_output(42, 0)

    ... then some .../gpio42 file, read/write, appears

    and

    echo 84 bar in > .../gpio_config

    ... causing error-checked versions of:

    gpio_request(84, "bar")
    gpio_direction_input(84)

    ... then some .../gpio84 file, read-only, appears

    Though arguably the label could just always be "userspace"
    (it's mostly for /sys/kernel/debug/gpio), and the default could
    be to configure as an input (unless an output value was given).
    Plus, there should be some way to cause gpio_free() too.

    A potential advantage of the /dev/... node approach would be
    that it's easier to support an IRQ-backed poll() mechanism
    for inputs.

    - Dave

    --
    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: userspace GPIO access (WAS: [patch/rfc 2/4] pcf875x ...)

    On Fri, 4 Apr 2008, David Brownell wrote:
    >
    > - Trent's gpio_class exposes all GPIOs, even ones
    > that are claimed by kernel drivers ... and thus
    > makes it easy to clobber kernel driver state.


    This was intentional. When you're developing said kernel drivers, or
    connecting hardware they're supposed to drive, it's very handy to be able
    to set and read the GPIOs from userspace. At least, when writing a
    gpiolib driver and code that used gpiolib, I found this ability very
    useful, so I thought other developers might as well.

    I suppose one could make the sys files read-only once a kernel driver
    allocates a gpio. But it would be nice to have the ability to make them
    writable, if one really wants that.

    > (Plus it won't work on most built-in GPIOs, since
    > they by and large don't have parent devices.)


    Couldn't they always add one? My GPIO driver is part of the CPU/SoC, and
    it has a device node. It's pretty easy to add a platform device, and
    probably cleaner than not associating a device with the gpio driver.
    From my understanding of sysfs, it seems any sysfs based approach has to
    be based on a device.

    > What I'd like to see is userspace config commands to
    > cause the gpio_request() ... *maybe* something like


    Suppose I took the code I had, and make the label file writable? Writing
    to it allocates the gpio with the written label? That would be
    relatively simple to add. Is there any reason why the GPIOs should
    appear in sysfs by default? They are devices, and most other devices
    appear in sysfs.

    > Plus, there should be some way to cause gpio_free() too.


    Write a blank label? Too bad one can't "rm" sysfs files, that would be a
    neat way to trigger stuff. I can see it used to hot-unplug a pci device,
    just delete the slot.
    --
    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
Page 2 of 2 FirstFirst 1 2