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@xxxxxxxxx> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Relevant Pages