Messages in this thread | | | Date | Mon, 19 Jun 2006 22:22:23 -0700 | From | Andrew Morton <> | Subject | Re: [patch -mm 20/20 RFC] chardev: GPIO for SCx200 & PC-8736x: add sysfs-GPIO interface |
| |
On Sat, 17 Jun 2006 12:42:28 -0600 Jim Cromie <jim.cromie@gmail.com> wrote:
> Ok, > > heres the brand-spanking-new proto-sysfs-gpio interface, > preceded by some pseudo/proto-Documentation. >
Well I stuck it in -mm anyway. Let's see what happens.
We don't have a Signed-off-by: for this patch.
Fixup patches agains next -mm would be suitable. Please keep them super-short: basically one-patch-per-review-comment. That way I can easily instertion-sort the patches into place and we retain a nice patch series.
> Ive
Apostrophe aversion?
> > +/* the pin-mode-change 'commands' of the legacy device-file-interface, > + now refactored for reuse in a sysfs-interface. Includes some > + updates which arent exposed via sysfs attributes. > +*/ > +static int common_write(struct nsc_gpio_ops *amp, char c, unsigned m) > +{ > + struct device *dev = amp->dev; > + int err = 0; > + switch (c) { > + case '0': > + amp->gpio_set(m, 0); > + break; > + case '1': > + amp->gpio_set(m, 1); > + break; > + case 'O': > + dev_dbg(dev, "GPIO%d output enabled\n", m); > + amp->gpio_config(m, ~1, 1); > + break; > + case 'o': > + dev_dbg(dev, "GPIO%d output disabled\n", m); > + amp->gpio_config(m, ~1, 0); > + break; > + case 'T': > + dev_dbg(dev, "GPIO%d output is push pull\n", m); > + amp->gpio_config(m, ~2, 2); > + break; > + case 't': > + dev_dbg(dev, "GPIO%d output is open drain\n", m); > + amp->gpio_config(m, ~2, 0); > + break; > + case 'P': > + dev_dbg(dev, "GPIO%d pull up enabled\n", m); > + amp->gpio_config(m, ~4, 4); > + break; > + case 'p': > + dev_dbg(dev, "GPIO%d pull up disabled\n", m); > + amp->gpio_config(m, ~4, 0); > + break; > + case 'L': > + dev_dbg(dev, "GPIO%d lock pin\n", m); > + amp->gpio_config(m, ~8, 8); > + break; > + case 'l': > + dev_warn(dev, "GPIO%d cant unlock locked? pin\n", m); > + amp->gpio_config(m, ~8, 0); > + break; > + > + case 'D': > + dev_dbg(dev, "GPIO%d turn on debounce\n", m); > + amp->gpio_config(m, ~PF_DEBOUNCE, PF_DEBOUNCE); > + break; > + case 'd': > + dev_warn(dev, "GPIO%d cant unlock locked? pin\n", m); > + amp->gpio_config(m, ~PF_DEBOUNCE, 0); > + break; > + > + case 'v': > + /* View Current pin settings */ > + amp->gpio_dump(amp, m); > + break; > + case 'c': > + /* view pin's current values: driven and read */ > + dev_info(dev, "io%02d: driven %d, input %d\n", > + m, amp->gpio_current(m), amp->gpio_get(m)); > + break; > + case '\n': > + /* end of settings string, do nothing */ > + break; > + default: > + dev_err(dev, "GPIO-%2d bad setting: chr<0x%2x>\n", m, > + (int)c); > + err++; > + } > + return err; > +} > + > ssize_t nsc_gpio_write(struct file *file, const char __user *data, > size_t len, loff_t *ppos) > { > + int i, err = 0; > unsigned m = iminor(file->f_dentry->d_inode); > struct nsc_gpio_ops *amp = file->private_data; > - struct device *dev = amp->dev; > - size_t i; > - int err = 0; > > for (i = 0; i < len; ++i) { > char c; > if (get_user(c, data + i)) > return -EFAULT; > - switch (c) { > - case '0': > - amp->gpio_set(m, 0); > - break; > - case '1': > - amp->gpio_set(m, 1); > - break; > - case 'O': > - dev_dbg(dev, "GPIO%d output enabled\n", m); > - amp->gpio_config(m, ~1, 1); > - break; > - case 'o': > - dev_dbg(dev, "GPIO%d output disabled\n", m); > - amp->gpio_config(m, ~1, 0); > - break; > - case 'T': > - dev_dbg(dev, "GPIO%d output is push pull\n", m); > - amp->gpio_config(m, ~2, 2); > - break; > - case 't': > - dev_dbg(dev, "GPIO%d output is open drain\n", m); > - amp->gpio_config(m, ~2, 0); > - break; > - case 'P': > - dev_dbg(dev, "GPIO%d pull up enabled\n", m); > - amp->gpio_config(m, ~4, 4); > - break; > - case 'p': > - dev_dbg(dev, "GPIO%d pull up disabled\n", m); > - amp->gpio_config(m, ~4, 0); > - break; > - > - case 'v': > - /* View Current pin settings */ > - amp->gpio_dump(amp, m); > - break; > - case '\n': > - /* end of settings string, do nothing */ > - break; > - default: > - dev_err(dev, "GPIO-%2d bad setting: chr<0x%2x>\n", m, > - (int)c); > - err++; > - } > + > + err += common_write(amp, c, m); > } > if (err) > return -EINVAL; /* full string handled, report error */
This all spat a whopping great reject for reasons unknown, which I fixed by hand. Please check that it all still works.
> +ssize_t nsc_gpio_sysfs_set(struct device *dev, > + struct device_attribute *devattr, const char *buf, > + size_t count) > +{ > + struct sensor_device_attribute_2 *attr = to_sensor_dev_attr_2(devattr); > + int idx = attr->index; > + int func = attr->nr; > + struct nsc_gpio_ops *amp = dev->driver_data; > + int err, xor; > + > + /* invert cmd if setting low */ > + xor = simple_strtol(buf, NULL, 10) ? 0 : 'T'^'t';
whoa. How does this work?
> + dev_info(dev, "set func:%d Func:%d, flp%d\n", func, func^xor, xor); > + > + err = common_write(amp, func^xor, idx);
The tricksies in here aren't very understandable. Can it be simplified?
> + if (err) > + return -EINVAL; // full string handled, report error > + > + return strlen(buf); > +} > > ... > > static void __init pc8736x_init_shadow(void) > { > int port; > @@ -320,6 +368,12 @@ static int __init pc8736x_gpio_init(void > dev_dbg(&pdev->dev, ": got dynamic major %d\n", major); > } > > + pc8736x_sysfs_init(&pdev->dev); > + > + /* provide info wheresysfs callbacks can get them */ > + pc8736x_access.dev->driver_data = &pc8736x_access; > + > +; > return 0; > }
stray semicolon.
> +/* GPIO pins have 5 attributes we care about: group them */ > +struct gpio_pin_attributes { > + struct sensor_device_attribute_2 > + output_enabled, > + totem_pole, > + pullup_enabled, > + debounced, > + locked; > +};
A bit hard to read. Normally we'd do
struct gpio_pin_attributes { struct sensor_device_attribute_2 output_enabled; struct sensor_device_attribute_2 totem_pole; struct sensor_device_attribute_2 pullup_enabled; struct sensor_device_attribute_2 debounced; struct sensor_device_attribute_2 locked; };
because kernel-style is super-simple-style.
- 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/
| |