lkml.org 
[lkml]   [2011]   [May]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [PATCH 1/4] drivers: create a pinmux subsystem
    From
    2011/5/2 Joe Perches <joe@perches.com>:
    > On Mon, 2011-05-02 at 21:16 +0200, Linus Walleij wrote:
    >> From: Linus Walleij <linus.walleij@linaro.org>
    >> diff --git a/drivers/pinmux/core.c b/drivers/pinmux/core.c
    >
    > Trivial comments follow
    >
    > []
    >> +static ssize_t pinmux_name_show(struct device *dev,
    >> +                             struct device_attribute *attr, char *buf)
    >> +{
    >> +     struct pinmux_dev *pmxdev = dev_get_drvdata(dev);
    >> +
    >> +     return sprintf(buf, "%s\n", pmxdev_get_name(pmxdev));
    >> +}
    >
    > Unsized buffer, maybe snprintf?

    This is the idiomatic way of providing sysfs strings (compare e.g.
    *_show() in drivers/regulator/core.c), the char *buf comes
    from the sysfs core in struct device_attribute in <linus/device.h>
    with this prototype:

    ssize_t (*show)(struct device *dev, struct device_attribute *attr,
    char *buf);


    and I have no way of knowing how large that buffer is. Migrating all
    of sysfs to provide the size of its buffers may help but do you really
    mean I should do that as part of this patchset? It will require
    refactoring the entire kernel :-(

    >> +static int pin_request(int pin, const char *function, bool gpio)
    >> +{
    >> +     struct pin_desc *desc;
    >> +     struct pinmux_dev *pmxdev;
    >> +     struct pinmux_ops *ops;
    >> +     int status = -EINVAL;
    >> +     unsigned long flags;
    >> +
    >> +     pr_debug("pin_request: request pin %d for %s\n", pin, function);
    >
    > pr_debug("%s: request pin...", __func__?
    >
    >> +             pr_err("pin_request: pin is invalid\n");
    >
    > same here, etc...

    What I am referring to here is not the name of the C function being
    executed but the function that this group of pins is performing,
    so a different ontology altogether.

    But the prefix is indeed the function name so I get what you mean,
    fixing it!

    >> +     if (!pmxdev) {
    >> +             pr_warning("pin_warning: no pinmux device is handling %d!\n",
    >
    > You use both pr_warning and pr_warn.  Please just use pr_warn.

    Sure.

    > Why use "pin_warning: "?

    I was drunk.

    > Maybe it'd be better to add
    >
    > #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
    > or
    > #define pr_fmt(fmt) KBUILD_MODNAME ":%s: " fmt, __func__
    >
    > if you really want __func__.
    > I suggest that __func__ isn't useful.

    Yep, I'll use the first one and replace all prefixes with pure meaningful
    messages instead.

    (Omitting such comments below - all fixed.)

    >> +static int pinmux_devices_show(struct seq_file *s, void *what)
    >> +{
    >> +     struct pinmux_dev *pmxdev;
    >> +
    >> +     seq_printf(s, "Available pinmux settings per pinmux device:\n");
    >> +     list_for_each_entry(pmxdev, &pinmuxdev_list, node) {
    >> +             struct pinmux_ops *ops = pmxdev->desc->ops;
    >
    > const struct pinmux_ops?

    Yepps, const:ed it everywhere!

    >> +             unsigned selector = 0;
    >> +
    >> +             seq_printf(s, "\nDevice %s:\n", pmxdev->desc->name);
    >
    > I think the initial newline isn't necessary.

    Nope. Leftover.

    >> +             while (ops->list_functions(pmxdev, selector) >= 0) {
    >> +                     unsigned *pins;
    >> +                     unsigned num_pins;
    >> +                     const char *func = ops->get_function_name(pmxdev,
    >> +                                                               selector);
    >> +                     int ret;
    >> +                     int i;
    >> +
    >> +                     ret = ops->get_function_pins(pmxdev, selector,
    >> +                                                  &pins, &num_pins);
    >> +
    >> +                     if (ret)
    >> +                             seq_printf(s, "%s [ERROR GETTING PINS]\n",
    >> +                                        func);
    >> +
    >> +                     else {
    >> +                             seq_printf(s, "function: %s, pins = [ ", func);
    >> +                             for (i = 0; i < num_pins; i++)
    >> +                                     seq_printf(s, "%d ", pins[i]);
    >> +                             seq_printf(s, "]\n");
    >
    > seq_printf used without additional arguments could be seq_puts

    Yep, fixed everywhere I sent in a non-argumented string.

    >> +     (void) debugfs_create_file("devices", S_IFREG | S_IRUGO,
    >> +                                debugfs_root, NULL, &pinmux_devices_ops);
    >> +     (void) debugfs_create_file("maps", S_IFREG | S_IRUGO,
    >> +                                debugfs_root, NULL, &pinmux_maps_ops);
    >> +     (void) debugfs_create_file("pins", S_IFREG | S_IRUGO,
    >> +                                debugfs_root, NULL, &pinmux_pins_ops);
    >
    > Unnecessary casts to (void)?

    Yep lost them.

    >> +static int __init pinmux_init(void)
    >> +{
    >> +     int ret;
    >> +
    >> +     ret = class_register(&pinmux_class);
    >> +     pr_info("pinmux framwork: handle up to %d pins\n", MACH_NR_PINS);
    >
    > framework?

    Should be subsystem. Fixed it.

    >> diff --git a/include/linux/pinmux.h b/include/linux/pinmux.h
    > []
    >> +/*
    >> + * Valid pin numbers are nonnegative and < MACH_NR_PINS. Invalid numbers can
    >> + * be used to indicate no-such-pin.
    >> + */
    >> +static inline int pin_is_valid(int pin)
    >> +{
    >> +     return ((unsigned)pin) < MACH_NR_PINS;
    >> +}
    >
    > Couldn't pin just be declared unsigned or maybe u32?

    No, because like in the GPIO subsystem you *may* want to send in invalid
    pins, and those are identified by negative numbers.

    Thanks a *lot* for your detailed review Joe, please supply your Reviewed-by:
    on the next (v2) patch set if you think it looks alright.

    Yours,
    Linus Walleij
    --
    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/

    \
     
     \ /
      Last update: 2011-05-11 00:21    [W:0.034 / U:31.496 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site