* Re: [PATCH] phylib: add mdio-gpio bus driver (v2)
From: Paulius Zaleckas @ 2008-10-28 7:37 UTC (permalink / raw)
To: Mike Frysinger; +Cc: netdev, linux-arm-kernel, linux-embedded
In-Reply-To: <8bd0f97a0810270753m740a2295i492532f055f13b17@mail.gmail.com>
Mike Frysinger wrote:
> On Mon, Oct 27, 2008 at 06:53, Paulius Zaleckas wrote:
>> + new_bus->irq = kmalloc(sizeof(int) * PHY_MAX_ADDR, GFP_KERNEL);
>
> oh, and this should be kcalloc()
Why? kcalloc() fills allocated memory with zeros and in this case it has
to be filled with -1... this is done by further for() routine. I don't
see the point to fill it with zeros before...
> -mike
^ permalink raw reply
* Re: [PATCH v2 RESEND] gpiolib: Add pin change notification
From: Ben Nizette @ 2008-10-27 22:18 UTC (permalink / raw)
To: David Brownell; +Cc: Andrew Morton, linux-kernel, linux-embedded
In-Reply-To: <200810271246.22149.david-b@pacbell.net>
On Mon, 2008-10-27 at 12:46 -0700, David Brownell wrote:
> By the way ... I noticed some new use cases for this sort of mechanism.
> In the OMAP git tree, say the copy at
>
> http://git.kernel.org/?p=linux/kernel/git/tmlind/linux-omap-2.6.git
>
> Have a look at arch/arm/plat-omap/gpio-switch.c ... current users
> include mach-omap2/board-n800.c and mach-omap1/board-palmte.c (in
> that tree), and report things like headphone jack insertion. Some
> of that fanciness seems like it shouldn't live in-kernel.
>
> I'll be glad to see the need for that driver vanish (except for
> the need to upgrade userspace, sigh), because of a more generic
> mechanism existing. :)
Cool, I'll check it out.
>
>
> On Monday 20 October 2008, Ben Nizette wrote:
> > This adds pin change notification to the gpiolib sysfs interface. It
> > requires 16 extra bytes in gpio_desc iff CONFIG_GPIO_SYSFS which in turn
> > means, eg, 4k of .bss usage on AVR32.
>
> Which seems quite problematic to me, for a mechanism that will
> in most cases not be used and certainly doesn't have the same
> level of fast-path considerations as gpio get/set operations.
>
> How about using a <linux/idr.h> table to map GPIOs to some
> notification structure ... and only when such notifications
> have been configured? A "busy" system might have one IDR
> and a handful of 16 byte structs ... lots less than 4KB, and
> coming out of kmalloc heaps (and thus barely observable).
Yeah since akpm's review a few days back, a few things have been
rearranged resulting in this figure blowing out to 13k on AVR32.
Obviously not acceptable so I'm looking for a nicer way around this.
I've been looking at such an IDR, I think it's the way to go yeah.
>
>
> > Due to limitations in sysfs, this
> > patch makes poll(2) and friends work as expected on the "value"
> > attribute, though reads on "value" will never block and there is no
> > facility for async reads and writes.
>
> I like poll() and friends working, and the rest seems like
> it's not something to worry about here.
>
>
> Have you thought about how to leverage the interrupt signals on
> chips like the pcf857x and pca953x, which have dedicated "pin
> changed" signals? They're nothing like the real interrupts,
> with per-pin irq status and disable masks, found in most
> SOC-integrated GPIO banks (and in fancier discrete ones).
> So they seem to me like bad matches for genirq... though I
> might be persuaded otherwise.
>
> Effectively, those IRQ are "poll now" advice ... advice that
> is not yet used. (The IRQs all seem to stay active until the
> GPIO bank status is read, FYI, so you can easily imagine how
> to fake out some IRQ-ish mechanism.)
So long as I allow the interrupt to be shared that should work fine
(assuming gpio_to_irq returns the irq of the chip-wide pin change signal
for all pins on the chip). akpm pointed out that sysfs_notify() cannot
be called from interrupt context so now all irqs are really taken as
"poll now" advice.
>
>
>
> > --- a/Documentation/gpio.txt
> > +++ b/Documentation/gpio.txt
> > @@ -529,6 +529,21 @@ and have the following read/write attrib
> > is configured as an output, this value may be written;
> > any nonzero value is treated as high.
> >
> > + "notify" ... Selects a method for the detection of pin change
> > + events. This can be written or read as one of "none",
> > + "irq" or a number. If "none", no detection will be
> > + done, if "irq" then the change detection will be done
> > + by registering an interrupt handler on the line given
> > + by gpio_to_irq for that gpio. If a number is written,
> > + the gpio will be polled for a state change every n
> > + milliseconds where n is the number written. The
> > + minimum interval is 10 milliseconds.
>
> That "10 milliseconds" seems a bit arbitrary. By analogy to the
> RTC framework's handling of periodic IRQs, maybe this should be a
> limit that a privileged user can change.
Right. Should this be a sysfs attribute of the gpio class? That seems
a sane place to stick it IMO.
>
> IMO it's worth noting that polling modes should not be used if
> IRQs could work ... and that polling *will* miss all changes
> that happen between polls.
Right, changed.
>
>
> > + "notify_filter" ... This can be written and read as one of
> > + "rising", "falling" or "both". If "rising", only
> > + rising edges will be reported, similarly for "falling"
> > + and "both".
>
> See below ... maybe writing those values to "notify" should be allowed,
> for IRQ-driven notification. Then this wouldn't be needed except
> when polling.
>
And have this attribute appear and disappear? This would break the
symmetry between irq and poll driven notification which I quite like.
IMO how the change is sensed and whether the change is reported are
fairly separate. Though I do agree with your point that we should
probably cater for chips which don't support both edge notification, see
below.
>
> > +
> > GPIO controllers have paths like /sys/class/gpio/chipchip42/ (for the
> > controller implementing GPIOs starting at #42) and have the following
> > read-only attributes:
> > @@ -549,6 +564,39 @@ gpiochip nodes (possibly in conjunction
> > the correct GPIO number to use for a given signal.
> >
> >
> > +Pin Change Notification
> > +-----------------------
> > +The "value" attribute of a gpio is pollable. For this to work, you
> > +must have set up the notify attribute of that gpio to the type of
> > +detection suitable for the underlying hardware.
> > +
> > +If the hardware supports interrupts on both edges and the gpio to
> > +interrupt line mapping is unique, you should write "irq" to the notify
> > +attribute.
>
> Suppose it only supports one edge (unlikely, to be sure)
> and that's the edge that's of interest? Maybe you should
> allow writing just "falling" (or "rising") to "notify" in
> those cases.
Yeah I was wondering about that case. Maybe writing an edge to "notify"
would set the filter mode to that edge and the notify mode to "irq",
taking care to do the right thing with respect to interrupt requests?
Much the same way as writing "high" to "direction" sets the direction to
"out" and the value to "1" taking care to do the right things in the
middle.
>
> I don't see what you mean by gpio_to_irq() being unique.
> Of course it is -- by definition, that can't return more
> than one value!
Badly worded. I was trying to say that the interrupt should uniquely
identify the gpio which generated it but in fact that's not true - an
early version of this patch used irq_to_gpio() but that's now gone and,
as mentioned above, the irq is currently used as "poll now" advice.
Changed :-)
>
>
> > +If the hardware doesn't support this, or you're unsure, you can write
> > +a number to the notify attribute. This number is the delay between
> > +successive polls of the gpio state in milliseconds. You may not poll
> > +more often than 10ms intervals.
>
> Surely if the hardware doesn't support rising/falling/both
> edge IRQ driven notifications, writing that value to the
> "notify" attribute will fail? There should be no need for
> any "unsure" option here... please restructure these user
> visible attributes to make sure "unsure" isn't an option.
Yeah trufax. Changed.
>
> This would seem to be the best place to explain why it's
> not a good idea to use polling. (System overhead, certain
> loss ove some events, etc.)
Agreed.
>
>
> > +
> > +You must use poll mode if communication with the gpio's chip requires
> > +sleeping. This is the case if, for example, the gpio is part of a
> > +I2C or SPI GPIO expander.
>
> Strike that ... surely the issue is that when IRQs can't
> be used, polling is the *only* other option?
It's more that if irq-driven notification is used then the new value
will be read from interrupt context and therefore can't sleep.
This has actually now changed too - irqs just trigger the poll work
which in turn gets the gpio value from a context which can sleep. Will
change.
>
> And there's nothing preventing I2C or SPI based expanders
> from presenting IRQs. (Other than current messiness imposed
> by the genirq framework, some of which will be resolved over
> time.) The drivers/gpio/twl4030-gpio.c code certainly presents
> IRQs right now, over I2C.
>
> The fact that some gpio chips, like those pcf857x ones, provide
> "poll me now" advice -- which we don't currently use -- instead
> of real interrupt controllers is perhaps deceptive.
>
>
> > +By default, both rising and falling edges are reported. To filter
> > +this, you can write one of "rising" or "falling" to the notify_filter
> > +attribute. To go back to being notified of both edge changes, write
> > +"both" to this attribute.
>
> If the "notify" attribute accepts rising/falling/both,
> then this paragraph can be removed.
Yup.
>
>
> > +Once the notification has been set up, you may use poll(2) and friends
> > +on the "value" attribute. This attribute will register as having new
> > +data ready to be read if and only if there has been a state change
> > +since the last read(2) to the "value" attribute.
>
> Not entirely true: (a) as noted, polling will overlook changes that
> happen between poll intervals, and (b) even for irq-driven notifications,
> the state may have changed back.
>
> Best to say the notification is only a hint that the "value" might now
> be different. It would be wrong for any userspace code to remember
> the last value, expect that the current value is different, and then
> act accordingly... instead of checking the current value.
Right.
>
>
> > +
> > +Note that unlike a regular device file, a read on the "value" attribute
> > +will never block whether or not there's new data to be read.
> > +
> > +
> > Exporting from Kernel code
> > --------------------------
> > Kernel code can explicitly manage exports of GPIOs which have already been
> > --- a/drivers/gpio/gpiolib.c
> > +++ b/drivers/gpio/gpiolib.c
> > @@ -1,6 +1,7 @@
> > #include <linux/kernel.h>
> > #include <linux/module.h>
> > #include <linux/irq.h>
> > +#include <linux/interrupt.h>
> > #include <linux/spinlock.h>
> > #include <linux/device.h>
> > #include <linux/err.h>
> > @@ -49,13 +50,35 @@ struct gpio_desc {
> > #define FLAG_RESERVED 2
> > #define FLAG_EXPORT 3 /* protected by sysfs_lock */
> > #define FLAG_SYSFS 4 /* exported via /sys/class/gpio/control */
> > +#define ASYNC_MODE_IRQ 5 /* using interrupts for async notification */
> > +#define ASYNC_MODE_POLL 6 /* using polling for async notification */
> > +#define ASYNC_RISING 7 /* will notify on rising edges */
> > +#define ASYNC_FALLING 8 /* will notify on falling edges */
>
> I suspect all you'd need here is a single flag to indicate
> that there's an IDR entry for this GPIO ...
Maybe not even that - depending on how fastpath this is you can just
check whether the IDR content is NULL or not.
>
> >
> > #ifdef CONFIG_DEBUG_FS
> > const char *label;
> > #endif
> > +
> > +#ifdef CONFIG_GPIO_SYSFS
> > + struct device *dev;
>
> This "dev" would be the exported sysfs node for the GPIO?
Yup, that'll have to stay so .bss will grow at least 1k on AVR32.
>
> > + struct poll_desc *poll;
> > +
> > + /* Poll interval in jiffies, here (rather than in struct poll_desc
> > + * so the user can change the value no matter what their current
> > + * notification mode is */
> > + long timeout;
> > +
> > + /* Last known value */
> > + int val;
>
> ... and these four elements would move to a struct that's
> stored in the IDR.
Right.
>
>
> > +#endif
> > };
> > static struct gpio_desc gpio_desc[ARCH_NR_GPIOS];
> >
> > +struct poll_desc {
> > + struct delayed_work work;
> > + unsigned gpio;
> > +};
> > +
> > static inline void desc_set_label(struct gpio_desc *d, const char *label)
> > {
> > #ifdef CONFIG_DEBUG_FS
> > @@ -184,12 +207,56 @@ static DEFINE_MUTEX(sysfs_lock);
> > * /value
> > * * always readable, subject to hardware behavior
> > * * may be writable, as zero/nonzero
> > - *
> > - * REVISIT there will likely be an attribute for configuring async
> > - * notifications, e.g. to specify polling interval or IRQ trigger type
> > - * that would for example trigger a poll() on the "value".
> > + * /notify
> > + * * read/write as "irq", numeric or "none"
> > + * /notify_filter
> > + * * read/write as "rising", "falling" or "both"
>
> Whoo! Someone actually updated a comment when changing the
> behavior being commented on! :)
>
:-)
> Note that Documentation/ABI/testing/sysfs-gpio would need
> updating here too.
Indeed, changed.
>
>
> > */
> >
> > +struct poll_desc *work_to_poll(struct work_struct *ws)
> > +{
> > + return container_of((struct delayed_work *)ws, struct poll_desc, work);
> > +}
> > +
> > +void gpio_poll_work(struct work_struct *ws)
> > +{
> > + struct poll_desc *poll = work_to_poll(ws);
> > +
> > + unsigned gpio = poll->gpio;
> > + struct gpio_desc *desc = &gpio_desc[gpio];
> > +
> > + int new = gpio_get_value_cansleep(gpio);
> > + int old = desc->val;
> > +
> > + if ((new && !old && test_bit(ASYNC_RISING, &desc->flags)) ||
> > + (!new && old && test_bit(ASYNC_FALLING, &desc->flags)))
> > + sysfs_notify(&desc->dev->kobj, NULL, "value");
> > +
> > + desc->val = new;
> > + schedule_delayed_work(&poll->work, desc->timeout);
> > +}
> > +
> > +static irqreturn_t gpio_irq_handler(int irq, void *dev_id)
> > +{
> > + struct gpio_desc *desc = dev_id;
> > + int gpio = desc - gpio_desc;
> > + int new, old;
> > +
> > + if (!gpio_is_valid(gpio))
> > + return IRQ_NONE;
>
> I'd make the data be the notification struct stored in the IDR, with
> the invariant that the IDR entry is NULL for all non-requested GPIOs...
> and for all GPIOs for which notification hasn't been explicitly enabled.
> (So this IRQ handler wouldn't need that flavor of error checking.)
>
Yup, sounds good.
>
> > +
> > + new = gpio_get_value(gpio);
> > + old = desc->val;
> > +
> > + if ((new && !old && test_bit(ASYNC_RISING, &desc->flags)) ||
> > + (!new && old && test_bit(ASYNC_FALLING, &desc->flags)))
> > + sysfs_notify(&desc->dev->kobj, NULL, "value");
> > +
> > + desc->val = new;
> > +
> > + return IRQ_HANDLED;
> > +}
> > +
> > static ssize_t gpio_direction_show(struct device *dev,
> > struct device_attribute *attr, char *buf)
> > {
> > @@ -284,9 +351,162 @@ static ssize_t gpio_value_store(struct d
> > static /*const*/ DEVICE_ATTR(value, 0644,
> > gpio_value_show, gpio_value_store);
> >
> > +static ssize_t gpio_notify_show(struct device *dev,
> > + struct device_attribute *attr, char *buf)
> > +{
> > + const struct gpio_desc *desc = dev_get_drvdata(dev);
> > + ssize_t ret;
> > +
> > + mutex_lock(&sysfs_lock);
>
> This might be "if the IDR lookup returns NULL, return 'none' else ..."
>
>
> > +
> > + if (test_bit(ASYNC_MODE_IRQ, &desc->flags))
> > + ret = sprintf(buf, "irq\n");
> > + else if (test_bit(ASYNC_MODE_POLL, &desc->flags))
> > + ret = sprintf(buf, "%ld\n", desc->timeout * 1000 / HZ);
> > + else
> > + ret = sprintf(buf, "none\n");
> > +
> > + mutex_unlock(&sysfs_lock);
> > + return ret;
> > +}
> > +
> > +static ssize_t gpio_notify_store(struct device *dev,
> > + struct device_attribute *attr, const char *buf, size_t size)
> > +{
> > + struct gpio_desc *desc = dev_get_drvdata(dev);
> > + unsigned gpio = desc - gpio_desc;
> > + ssize_t status = 0;
> > + int new;
> > + long value;
> > +
> > + mutex_lock(&sysfs_lock);
>
> This might be "if the IDR lookup returns NULL, initialize..."
>
> > +
> > + if (sysfs_streq(buf, "irq"))
> > + new = ASYNC_MODE_IRQ;
> > + else if (sysfs_streq(buf, "none"))
> > + new = -1;
>
> ... that might scrub out the IDR entry ...
>
>
> > + else {
> > + status = strict_strtol(buf, 0, &value);
> > +
> > + if (status || value < 10) {
> > + status = -EINVAL;
> > + goto out;
> > + }
> > +
> > + /* value will be in ms, convert to jiffies. */
> > + desc->timeout = DIV_ROUND_UP(value * HZ, 1000);
> > + new = ASYNC_MODE_POLL;
> > + }
> > +
> > + if (test_and_clear_bit(ASYNC_MODE_IRQ, &desc->flags))
> > + free_irq(gpio_to_irq(gpio), desc);
> > + else if (test_and_clear_bit(ASYNC_MODE_POLL, &desc->flags)) {
> > + BUG_ON(!desc->poll);
> > +
> > + cancel_delayed_work(&desc->poll->work);
> > + kfree(desc->poll);
> > + }
> > +
> > + if (new == ASYNC_MODE_IRQ) {
> > + if (desc->chip->can_sleep) {
> > + /* -EINVAL probably isn't appropriate here,
> > + * what's code for "not supported by
> > + * underlying hardware"? */
> > + status = -EINVAL;
> > + goto out;
> > + }
> > +
> > + desc->val = gpio_get_value(gpio);
> > +
> > + /* REVISIT: We always request both edges then filter in the
> > + * irq handler. How many devices don't support this..?? */
> > + status = request_irq(gpio_to_irq(gpio), gpio_irq_handler,
> > + IRQF_SHARED | IRQF_TRIGGER_RISING |
> > + IRQF_TRIGGER_FALLING,
> > + "gpiolib", desc);
>
> Can you avoid requesting the IRQ until something actually needs the
> notification? Like by receiving a poll() or fasync() call for the
> "value" attribute.
>
> Ditto starting the polling timer, below. Especially in that case,
> I don't like the notion that something could start a fast timer
> that nothing is really waiting for, needlessly increase the speed
> with which the battery drains.
>
> But I suspect you'll tell me sysfs won't cooperate here. :(
Indeed I can't immediately think of a way to make it happen, but I'll
have a think.
I'll mention wakeups and battery drainage etc in the above help text -
hopefully with that people will just turn the notification on when they
need it (wishful thinking I'm sure).
>
>
> > + } else if (new == ASYNC_MODE_POLL) {
> > +
> > + desc->poll = kmalloc(sizeof(struct poll_desc), GFP_KERNEL);
> > + if (!desc->poll) {
> > + status = -ENOMEM;
> > + goto out;
> > + }
> > +
> > + desc->poll->gpio = gpio;
> > +
> > + desc->val = gpio_get_value_cansleep(gpio);
> > +
> > + INIT_DELAYED_WORK(&desc->poll->work, gpio_poll_work);
> > + schedule_delayed_work(&desc->poll->work, desc->timeout);
> > + }
> > +
> > + if (new >= 0 && !status)
> > + set_bit(new, &desc->flags);
> > +
> > +out:
> > + if (status)
> > + dev_dbg(dev, "gpio notification mode set fail, err %d\n",
> > + (int)status);
> > +
> > + mutex_unlock(&sysfs_lock);
> > + return status ? : size;
> > +}
> > +
> > +static const DEVICE_ATTR(notify, 0644,
> > + gpio_notify_show, gpio_notify_store);
> > +
> > +static ssize_t gpio_notify_filter_show(struct device *dev,
> > + struct device_attribute *attr, char *buf)
> > +{
> > + struct gpio_desc *desc = dev_get_drvdata(dev);
> > + ssize_t status;
> > +
> > + mutex_lock(&sysfs_lock);
>
> This might be "if the IDR lookup returns NULL, return 'none' else ..."
>
>
> > +
> > + if (test_bit(ASYNC_FALLING, &desc->flags) &&
> > + test_bit(ASYNC_RISING, &desc->flags))
> > + status = sprintf(buf, "both\n");
> > + else if (test_bit(ASYNC_RISING, &desc->flags))
> > + status = sprintf(buf, "rising\n");
> > + else
> > + status = sprintf(buf, "falling\n");
> > +
> > + mutex_unlock(&sysfs_lock);
> > + return status;
> > +}
> > +
> > +static ssize_t gpio_notify_filter_store(struct device *dev,
> > + struct device_attribute *attr, const char *buf, size_t size)
> > +{
> > + struct gpio_desc *desc = dev_get_drvdata(dev);
> > + ssize_t status = 0;
> > +
> > + mutex_lock(&sysfs_lock);
>
> This might be "if the IDR lookup returns NULL, fail ..."
>
>
> > +
> > + if (sysfs_streq(buf, "rising")) {
> > + set_bit(ASYNC_RISING, &desc->flags);
> > + clear_bit(ASYNC_FALLING, &desc->flags);
> > + } else if (sysfs_streq(buf, "falling")) {
> > + clear_bit(ASYNC_RISING, &desc->flags);
> > + set_bit(ASYNC_FALLING, &desc->flags);
> > + } else if (sysfs_streq(buf, "both")) {
> > + set_bit(ASYNC_RISING, &desc->flags);
> > + set_bit(ASYNC_FALLING, &desc->flags);
> > + } else
> > + status = -EINVAL;
> > +
> > + mutex_unlock(&sysfs_lock);
> > + return status ? : size;
> > +}
> > +
> > +static const DEVICE_ATTR(notify_filter, 0644,
> > + gpio_notify_filter_show, gpio_notify_filter_store);
> > +
> > static const struct attribute *gpio_attrs[] = {
> > &dev_attr_direction.attr,
> > &dev_attr_value.attr,
> > + &dev_attr_notify.attr,
> > + &dev_attr_notify_filter.attr,
> > NULL,
> > };
> >
> > @@ -398,6 +618,17 @@ static ssize_t unexport_store(struct cla
> > status = 0;
> > gpio_free(gpio);
> > }
> > +
> > + if (test_and_clear_bit(ASYNC_MODE_IRQ, &gpio_desc[gpio].flags))
> > + free_irq(gpio_to_irq(gpio), &gpio_desc[gpio]);
> > +
> > + if (test_and_clear_bit(ASYNC_MODE_POLL, &gpio_desc[gpio].flags)) {
> > + BUG_ON(!gpio_desc[gpio].poll);
> > +
> > + cancel_delayed_work(&gpio_desc[gpio].poll->work);
> > + kfree(gpio_desc[gpio].poll);
> > + }
>
> This might be "if the IDR lookup doesn't return NULL, scrub it out ..."
>
>
> > +
> > done:
> > if (status)
> > pr_debug("%s: status %d\n", __func__, status);
> > @@ -479,6 +710,12 @@ int gpio_export(unsigned gpio, bool dire
> > status = -ENODEV;
> > if (status == 0)
> > set_bit(FLAG_EXPORT, &desc->flags);
> > +
> > + desc->dev = dev;
> > + /* 100ms default poll interval */
> > + desc->timeout = HZ / 10;
> > + set_bit(ASYNC_RISING, &desc->flags);
> > + set_bit(ASYNC_FALLING, &desc->flags);
>
> I'd rather see the notification infrastructure kick on only when
> it's explicitly requested by userspace ... so the cost of this
> mechanism is just some code and an IDR, except when it's in use.
>
> Plus, a default polling rate of 10 Hz is pretty unfriendly, on
> systems where NO_HZ would otherwise cause a tick rate that's
> a lot lower. Best IMO to just require userspace to specify a
> poll rate when that's required.
Yeah note that this doesn't actually kick off the poll timer or
anything, it just provides a default timeout. Actually that's a
leftover from v1 which had a slightly different user interface.
Removed.
>
>
> > }
> >
> > mutex_unlock(&sysfs_lock);
> > @@ -626,7 +863,6 @@ static int __init gpiolib_sysfs_init(voi
> > }
> > spin_unlock_irqrestore(&gpio_lock, flags);
> >
> > -
> > return status;
> > }
> > postcore_initcall(gpiolib_sysfs_init);
>
>
> Minor nit: the Kconfig for GPIO_SYSFS will deserve updating
> if this mechanism is added. It doesn't currently mention
> such notifications...
Right.
Thanks for this, I've got uni exams the next week or 2 but I'll try and
get a respun patch out shortly.
--Ben.
^ permalink raw reply
* Re: [PATCH v2 RESEND] gpiolib: Add pin change notification
From: David Brownell @ 2008-10-27 19:46 UTC (permalink / raw)
To: Ben Nizette; +Cc: Andrew Morton, linux-kernel, linux-embedded
In-Reply-To: <1224543007.3954.71.camel@moss.renham>
By the way ... I noticed some new use cases for this sort of mechanism.
In the OMAP git tree, say the copy at
http://git.kernel.org/?p=linux/kernel/git/tmlind/linux-omap-2.6.git
Have a look at arch/arm/plat-omap/gpio-switch.c ... current users
include mach-omap2/board-n800.c and mach-omap1/board-palmte.c (in
that tree), and report things like headphone jack insertion. Some
of that fanciness seems like it shouldn't live in-kernel.
I'll be glad to see the need for that driver vanish (except for
the need to upgrade userspace, sigh), because of a more generic
mechanism existing. :)
On Monday 20 October 2008, Ben Nizette wrote:
> This adds pin change notification to the gpiolib sysfs interface. It
> requires 16 extra bytes in gpio_desc iff CONFIG_GPIO_SYSFS which in turn
> means, eg, 4k of .bss usage on AVR32.
Which seems quite problematic to me, for a mechanism that will
in most cases not be used and certainly doesn't have the same
level of fast-path considerations as gpio get/set operations.
How about using a <linux/idr.h> table to map GPIOs to some
notification structure ... and only when such notifications
have been configured? A "busy" system might have one IDR
and a handful of 16 byte structs ... lots less than 4KB, and
coming out of kmalloc heaps (and thus barely observable).
> Due to limitations in sysfs, this
> patch makes poll(2) and friends work as expected on the "value"
> attribute, though reads on "value" will never block and there is no
> facility for async reads and writes.
I like poll() and friends working, and the rest seems like
it's not something to worry about here.
Have you thought about how to leverage the interrupt signals on
chips like the pcf857x and pca953x, which have dedicated "pin
changed" signals? They're nothing like the real interrupts,
with per-pin irq status and disable masks, found in most
SOC-integrated GPIO banks (and in fancier discrete ones).
So they seem to me like bad matches for genirq... though I
might be persuaded otherwise.
Effectively, those IRQ are "poll now" advice ... advice that
is not yet used. (The IRQs all seem to stay active until the
GPIO bank status is read, FYI, so you can easily imagine how
to fake out some IRQ-ish mechanism.)
> --- a/Documentation/gpio.txt
> +++ b/Documentation/gpio.txt
> @@ -529,6 +529,21 @@ and have the following read/write attrib
> is configured as an output, this value may be written;
> any nonzero value is treated as high.
>
> + "notify" ... Selects a method for the detection of pin change
> + events. This can be written or read as one of "none",
> + "irq" or a number. If "none", no detection will be
> + done, if "irq" then the change detection will be done
> + by registering an interrupt handler on the line given
> + by gpio_to_irq for that gpio. If a number is written,
> + the gpio will be polled for a state change every n
> + milliseconds where n is the number written. The
> + minimum interval is 10 milliseconds.
That "10 milliseconds" seems a bit arbitrary. By analogy to the
RTC framework's handling of periodic IRQs, maybe this should be a
limit that a privileged user can change.
IMO it's worth noting that polling modes should not be used if
IRQs could work ... and that polling *will* miss all changes
that happen between polls.
> + "notify_filter" ... This can be written and read as one of
> + "rising", "falling" or "both". If "rising", only
> + rising edges will be reported, similarly for "falling"
> + and "both".
See below ... maybe writing those values to "notify" should be allowed,
for IRQ-driven notification. Then this wouldn't be needed except
when polling.
> +
> GPIO controllers have paths like /sys/class/gpio/chipchip42/ (for the
> controller implementing GPIOs starting at #42) and have the following
> read-only attributes:
> @@ -549,6 +564,39 @@ gpiochip nodes (possibly in conjunction
> the correct GPIO number to use for a given signal.
>
>
> +Pin Change Notification
> +-----------------------
> +The "value" attribute of a gpio is pollable. For this to work, you
> +must have set up the notify attribute of that gpio to the type of
> +detection suitable for the underlying hardware.
> +
> +If the hardware supports interrupts on both edges and the gpio to
> +interrupt line mapping is unique, you should write "irq" to the notify
> +attribute.
Suppose it only supports one edge (unlikely, to be sure)
and that's the edge that's of interest? Maybe you should
allow writing just "falling" (or "rising") to "notify" in
those cases.
I don't see what you mean by gpio_to_irq() being unique.
Of course it is -- by definition, that can't return more
than one value!
> +If the hardware doesn't support this, or you're unsure, you can write
> +a number to the notify attribute. This number is the delay between
> +successive polls of the gpio state in milliseconds. You may not poll
> +more often than 10ms intervals.
Surely if the hardware doesn't support rising/falling/both
edge IRQ driven notifications, writing that value to the
"notify" attribute will fail? There should be no need for
any "unsure" option here... please restructure these user
visible attributes to make sure "unsure" isn't an option.
This would seem to be the best place to explain why it's
not a good idea to use polling. (System overhead, certain
loss ove some events, etc.)
> +
> +You must use poll mode if communication with the gpio's chip requires
> +sleeping. This is the case if, for example, the gpio is part of a
> +I2C or SPI GPIO expander.
Strike that ... surely the issue is that when IRQs can't
be used, polling is the *only* other option?
And there's nothing preventing I2C or SPI based expanders
from presenting IRQs. (Other than current messiness imposed
by the genirq framework, some of which will be resolved over
time.) The drivers/gpio/twl4030-gpio.c code certainly presents
IRQs right now, over I2C.
The fact that some gpio chips, like those pcf857x ones, provide
"poll me now" advice -- which we don't currently use -- instead
of real interrupt controllers is perhaps deceptive.
> +By default, both rising and falling edges are reported. To filter
> +this, you can write one of "rising" or "falling" to the notify_filter
> +attribute. To go back to being notified of both edge changes, write
> +"both" to this attribute.
If the "notify" attribute accepts rising/falling/both,
then this paragraph can be removed.
> +Once the notification has been set up, you may use poll(2) and friends
> +on the "value" attribute. This attribute will register as having new
> +data ready to be read if and only if there has been a state change
> +since the last read(2) to the "value" attribute.
Not entirely true: (a) as noted, polling will overlook changes that
happen between poll intervals, and (b) even for irq-driven notifications,
the state may have changed back.
Best to say the notification is only a hint that the "value" might now
be different. It would be wrong for any userspace code to remember
the last value, expect that the current value is different, and then
act accordingly... instead of checking the current value.
> +
> +Note that unlike a regular device file, a read on the "value" attribute
> +will never block whether or not there's new data to be read.
> +
> +
> Exporting from Kernel code
> --------------------------
> Kernel code can explicitly manage exports of GPIOs which have already been
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -1,6 +1,7 @@
> #include <linux/kernel.h>
> #include <linux/module.h>
> #include <linux/irq.h>
> +#include <linux/interrupt.h>
> #include <linux/spinlock.h>
> #include <linux/device.h>
> #include <linux/err.h>
> @@ -49,13 +50,35 @@ struct gpio_desc {
> #define FLAG_RESERVED 2
> #define FLAG_EXPORT 3 /* protected by sysfs_lock */
> #define FLAG_SYSFS 4 /* exported via /sys/class/gpio/control */
> +#define ASYNC_MODE_IRQ 5 /* using interrupts for async notification */
> +#define ASYNC_MODE_POLL 6 /* using polling for async notification */
> +#define ASYNC_RISING 7 /* will notify on rising edges */
> +#define ASYNC_FALLING 8 /* will notify on falling edges */
I suspect all you'd need here is a single flag to indicate
that there's an IDR entry for this GPIO ...
>
> #ifdef CONFIG_DEBUG_FS
> const char *label;
> #endif
> +
> +#ifdef CONFIG_GPIO_SYSFS
> + struct device *dev;
This "dev" would be the exported sysfs node for the GPIO?
> + struct poll_desc *poll;
> +
> + /* Poll interval in jiffies, here (rather than in struct poll_desc
> + * so the user can change the value no matter what their current
> + * notification mode is */
> + long timeout;
> +
> + /* Last known value */
> + int val;
... and these four elements would move to a struct that's
stored in the IDR.
> +#endif
> };
> static struct gpio_desc gpio_desc[ARCH_NR_GPIOS];
>
> +struct poll_desc {
> + struct delayed_work work;
> + unsigned gpio;
> +};
> +
> static inline void desc_set_label(struct gpio_desc *d, const char *label)
> {
> #ifdef CONFIG_DEBUG_FS
> @@ -184,12 +207,56 @@ static DEFINE_MUTEX(sysfs_lock);
> * /value
> * * always readable, subject to hardware behavior
> * * may be writable, as zero/nonzero
> - *
> - * REVISIT there will likely be an attribute for configuring async
> - * notifications, e.g. to specify polling interval or IRQ trigger type
> - * that would for example trigger a poll() on the "value".
> + * /notify
> + * * read/write as "irq", numeric or "none"
> + * /notify_filter
> + * * read/write as "rising", "falling" or "both"
Whoo! Someone actually updated a comment when changing the
behavior being commented on! :)
Note that Documentation/ABI/testing/sysfs-gpio would need
updating here too.
> */
>
> +struct poll_desc *work_to_poll(struct work_struct *ws)
> +{
> + return container_of((struct delayed_work *)ws, struct poll_desc, work);
> +}
> +
> +void gpio_poll_work(struct work_struct *ws)
> +{
> + struct poll_desc *poll = work_to_poll(ws);
> +
> + unsigned gpio = poll->gpio;
> + struct gpio_desc *desc = &gpio_desc[gpio];
> +
> + int new = gpio_get_value_cansleep(gpio);
> + int old = desc->val;
> +
> + if ((new && !old && test_bit(ASYNC_RISING, &desc->flags)) ||
> + (!new && old && test_bit(ASYNC_FALLING, &desc->flags)))
> + sysfs_notify(&desc->dev->kobj, NULL, "value");
> +
> + desc->val = new;
> + schedule_delayed_work(&poll->work, desc->timeout);
> +}
> +
> +static irqreturn_t gpio_irq_handler(int irq, void *dev_id)
> +{
> + struct gpio_desc *desc = dev_id;
> + int gpio = desc - gpio_desc;
> + int new, old;
> +
> + if (!gpio_is_valid(gpio))
> + return IRQ_NONE;
I'd make the data be the notification struct stored in the IDR, with
the invariant that the IDR entry is NULL for all non-requested GPIOs...
and for all GPIOs for which notification hasn't been explicitly enabled.
(So this IRQ handler wouldn't need that flavor of error checking.)
> +
> + new = gpio_get_value(gpio);
> + old = desc->val;
> +
> + if ((new && !old && test_bit(ASYNC_RISING, &desc->flags)) ||
> + (!new && old && test_bit(ASYNC_FALLING, &desc->flags)))
> + sysfs_notify(&desc->dev->kobj, NULL, "value");
> +
> + desc->val = new;
> +
> + return IRQ_HANDLED;
> +}
> +
> static ssize_t gpio_direction_show(struct device *dev,
> struct device_attribute *attr, char *buf)
> {
> @@ -284,9 +351,162 @@ static ssize_t gpio_value_store(struct d
> static /*const*/ DEVICE_ATTR(value, 0644,
> gpio_value_show, gpio_value_store);
>
> +static ssize_t gpio_notify_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + const struct gpio_desc *desc = dev_get_drvdata(dev);
> + ssize_t ret;
> +
> + mutex_lock(&sysfs_lock);
This might be "if the IDR lookup returns NULL, return 'none' else ..."
> +
> + if (test_bit(ASYNC_MODE_IRQ, &desc->flags))
> + ret = sprintf(buf, "irq\n");
> + else if (test_bit(ASYNC_MODE_POLL, &desc->flags))
> + ret = sprintf(buf, "%ld\n", desc->timeout * 1000 / HZ);
> + else
> + ret = sprintf(buf, "none\n");
> +
> + mutex_unlock(&sysfs_lock);
> + return ret;
> +}
> +
> +static ssize_t gpio_notify_store(struct device *dev,
> + struct device_attribute *attr, const char *buf, size_t size)
> +{
> + struct gpio_desc *desc = dev_get_drvdata(dev);
> + unsigned gpio = desc - gpio_desc;
> + ssize_t status = 0;
> + int new;
> + long value;
> +
> + mutex_lock(&sysfs_lock);
This might be "if the IDR lookup returns NULL, initialize..."
> +
> + if (sysfs_streq(buf, "irq"))
> + new = ASYNC_MODE_IRQ;
> + else if (sysfs_streq(buf, "none"))
> + new = -1;
... that might scrub out the IDR entry ...
> + else {
> + status = strict_strtol(buf, 0, &value);
> +
> + if (status || value < 10) {
> + status = -EINVAL;
> + goto out;
> + }
> +
> + /* value will be in ms, convert to jiffies. */
> + desc->timeout = DIV_ROUND_UP(value * HZ, 1000);
> + new = ASYNC_MODE_POLL;
> + }
> +
> + if (test_and_clear_bit(ASYNC_MODE_IRQ, &desc->flags))
> + free_irq(gpio_to_irq(gpio), desc);
> + else if (test_and_clear_bit(ASYNC_MODE_POLL, &desc->flags)) {
> + BUG_ON(!desc->poll);
> +
> + cancel_delayed_work(&desc->poll->work);
> + kfree(desc->poll);
> + }
> +
> + if (new == ASYNC_MODE_IRQ) {
> + if (desc->chip->can_sleep) {
> + /* -EINVAL probably isn't appropriate here,
> + * what's code for "not supported by
> + * underlying hardware"? */
> + status = -EINVAL;
> + goto out;
> + }
> +
> + desc->val = gpio_get_value(gpio);
> +
> + /* REVISIT: We always request both edges then filter in the
> + * irq handler. How many devices don't support this..?? */
> + status = request_irq(gpio_to_irq(gpio), gpio_irq_handler,
> + IRQF_SHARED | IRQF_TRIGGER_RISING |
> + IRQF_TRIGGER_FALLING,
> + "gpiolib", desc);
Can you avoid requesting the IRQ until something actually needs the
notification? Like by receiving a poll() or fasync() call for the
"value" attribute.
Ditto starting the polling timer, below. Especially in that case,
I don't like the notion that something could start a fast timer
that nothing is really waiting for, needlessly increase the speed
with which the battery drains.
But I suspect you'll tell me sysfs won't cooperate here. :(
> + } else if (new == ASYNC_MODE_POLL) {
> +
> + desc->poll = kmalloc(sizeof(struct poll_desc), GFP_KERNEL);
> + if (!desc->poll) {
> + status = -ENOMEM;
> + goto out;
> + }
> +
> + desc->poll->gpio = gpio;
> +
> + desc->val = gpio_get_value_cansleep(gpio);
> +
> + INIT_DELAYED_WORK(&desc->poll->work, gpio_poll_work);
> + schedule_delayed_work(&desc->poll->work, desc->timeout);
> + }
> +
> + if (new >= 0 && !status)
> + set_bit(new, &desc->flags);
> +
> +out:
> + if (status)
> + dev_dbg(dev, "gpio notification mode set fail, err %d\n",
> + (int)status);
> +
> + mutex_unlock(&sysfs_lock);
> + return status ? : size;
> +}
> +
> +static const DEVICE_ATTR(notify, 0644,
> + gpio_notify_show, gpio_notify_store);
> +
> +static ssize_t gpio_notify_filter_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct gpio_desc *desc = dev_get_drvdata(dev);
> + ssize_t status;
> +
> + mutex_lock(&sysfs_lock);
This might be "if the IDR lookup returns NULL, return 'none' else ..."
> +
> + if (test_bit(ASYNC_FALLING, &desc->flags) &&
> + test_bit(ASYNC_RISING, &desc->flags))
> + status = sprintf(buf, "both\n");
> + else if (test_bit(ASYNC_RISING, &desc->flags))
> + status = sprintf(buf, "rising\n");
> + else
> + status = sprintf(buf, "falling\n");
> +
> + mutex_unlock(&sysfs_lock);
> + return status;
> +}
> +
> +static ssize_t gpio_notify_filter_store(struct device *dev,
> + struct device_attribute *attr, const char *buf, size_t size)
> +{
> + struct gpio_desc *desc = dev_get_drvdata(dev);
> + ssize_t status = 0;
> +
> + mutex_lock(&sysfs_lock);
This might be "if the IDR lookup returns NULL, fail ..."
> +
> + if (sysfs_streq(buf, "rising")) {
> + set_bit(ASYNC_RISING, &desc->flags);
> + clear_bit(ASYNC_FALLING, &desc->flags);
> + } else if (sysfs_streq(buf, "falling")) {
> + clear_bit(ASYNC_RISING, &desc->flags);
> + set_bit(ASYNC_FALLING, &desc->flags);
> + } else if (sysfs_streq(buf, "both")) {
> + set_bit(ASYNC_RISING, &desc->flags);
> + set_bit(ASYNC_FALLING, &desc->flags);
> + } else
> + status = -EINVAL;
> +
> + mutex_unlock(&sysfs_lock);
> + return status ? : size;
> +}
> +
> +static const DEVICE_ATTR(notify_filter, 0644,
> + gpio_notify_filter_show, gpio_notify_filter_store);
> +
> static const struct attribute *gpio_attrs[] = {
> &dev_attr_direction.attr,
> &dev_attr_value.attr,
> + &dev_attr_notify.attr,
> + &dev_attr_notify_filter.attr,
> NULL,
> };
>
> @@ -398,6 +618,17 @@ static ssize_t unexport_store(struct cla
> status = 0;
> gpio_free(gpio);
> }
> +
> + if (test_and_clear_bit(ASYNC_MODE_IRQ, &gpio_desc[gpio].flags))
> + free_irq(gpio_to_irq(gpio), &gpio_desc[gpio]);
> +
> + if (test_and_clear_bit(ASYNC_MODE_POLL, &gpio_desc[gpio].flags)) {
> + BUG_ON(!gpio_desc[gpio].poll);
> +
> + cancel_delayed_work(&gpio_desc[gpio].poll->work);
> + kfree(gpio_desc[gpio].poll);
> + }
This might be "if the IDR lookup doesn't return NULL, scrub it out ..."
> +
> done:
> if (status)
> pr_debug("%s: status %d\n", __func__, status);
> @@ -479,6 +710,12 @@ int gpio_export(unsigned gpio, bool dire
> status = -ENODEV;
> if (status == 0)
> set_bit(FLAG_EXPORT, &desc->flags);
> +
> + desc->dev = dev;
> + /* 100ms default poll interval */
> + desc->timeout = HZ / 10;
> + set_bit(ASYNC_RISING, &desc->flags);
> + set_bit(ASYNC_FALLING, &desc->flags);
I'd rather see the notification infrastructure kick on only when
it's explicitly requested by userspace ... so the cost of this
mechanism is just some code and an IDR, except when it's in use.
Plus, a default polling rate of 10 Hz is pretty unfriendly, on
systems where NO_HZ would otherwise cause a tick rate that's
a lot lower. Best IMO to just require userspace to specify a
poll rate when that's required.
> }
>
> mutex_unlock(&sysfs_lock);
> @@ -626,7 +863,6 @@ static int __init gpiolib_sysfs_init(voi
> }
> spin_unlock_irqrestore(&gpio_lock, flags);
>
> -
> return status;
> }
> postcore_initcall(gpiolib_sysfs_init);
Minor nit: the Kconfig for GPIO_SYSFS will deserve updating
if this mechanism is added. It doesn't currently mention
such notifications...
^ permalink raw reply
* Re: USB, Host and Device on the same machine
From: David Brownell @ 2008-10-27 16:44 UTC (permalink / raw)
To: Fredrik Johansson; +Cc: linux-embedded
In-Reply-To: <53e45cc30810240018w1c3c73c2nf9a9b606e0e7dea4@mail.gmail.com>
On Friday 24 October 2008, Fredrik Johansson wrote:
> if I load the ehci-hcd driver with g_serial already loaded it
> screws everything up since ehci-hcd driver attaches to both the OTG
> and the host controller, how can i prevent it from using the OTG
> controller?
If the OTG controller exposes EHCI as its host interface, it
sure *ought* to be managed by the EHCI driver. Maybe you
should be submitting a patch adding OTG glue to EHCI?
If it's not exposing EHCI as its host interface, then it seems
your OTG driver is quite buggy...
^ permalink raw reply
* Re: [PATCH] phylib: add mdio-gpio bus driver (v2)
From: David Brownell @ 2008-10-27 16:41 UTC (permalink / raw)
To: Grant Likely, Paulius Zaleckas; +Cc: netdev, linux-arm-kernel, linux-embedded
In-Reply-To: <20081027143734.GA13565@secretlab.ca>
On Monday 27 October 2008, Grant Likely wrote:
> On Mon, Oct 27, 2008 at 12:53:22PM +0200, Paulius Zaleckas wrote:
> > Useful for machines where PHY control is connected to GPIO.
> > This driver also supports interrupts from PHY.
I get a kick out of seeing each new generic driver using
the generic GPIO interface. I *should* have expected it,
obviously. ;)
With a few exceptions I'll second Grant's comments, and
pick a few more nits.
> > +#include <linux/kernel.h>
> > +#include <linux/init.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/gpio.h>
> > +#include <linux/mdio-bitbang.h>
> > +#include <linux/mdio-gpio.h>
>
> Missing:
>
> MODULE_AUTHOR()
> MODULE_LICENSE()
> MODULE_DESCRIPTION()
... which many of us like to see at the *end* of the driver,
with other module housekeeping (driver registration), instead
of duplicating the header contents we just saw.
> > +static int __devinit mdio_gpio_probe(struct platform_device *pdev)
There are a few cases where platform drivers can't use __init
and platform_driver_probe(), instead of __devinit paired with
platform_driver_register(). Does this need to be one of them?
That is, are these platform devices going to be hotplugged?
(Usually because they are driver model children of other devices
which get hotplugged.)
> > +out:
> > + return ret;
>
> Nit: labels in column 0 will confuse diff when it tries to put the
> function name in the diff hunk header. If you indent the labels by 1
> space then future diffs will show the function name instead of the label
> name in diff hunk headers.
... but please don't change drivers to work around cosmitic diff bugs ...
> > +static int __devexit mdio_gpio_remove(struct platform_device *pdev)
As above: if these devices are really hotpluggable, so be it.
But that's the exception for platform_device nodes, not the rule,
so I'd normally use __exit here (and __exit_p in the driver
structure, later) to shrink the runtime code footprint.
> > +static void mdio_gpio_exit(void)
>
> static void __exit mdio_gpio_exit(void)
>
> > +{
> > + platform_driver_unregister(&mdio_gpio_driver);
> > +}
> > +
> > +module_init(mdio_gpio_init);
> > +module_exit(mdio_gpio_exit);
Also, snug the module_init()/module_exit() up against the
functions they affect, just as with EXPORT_SYMBOL().
^ permalink raw reply
* Re: [PATCH] phylib: add mdio-gpio bus driver (v2)
From: Mike Frysinger @ 2008-10-27 14:53 UTC (permalink / raw)
To: Paulius Zaleckas; +Cc: netdev, linux-arm-kernel, linux-embedded
In-Reply-To: <20081027105318.21923.24436.stgit@Programuotojas.82-135-208-232.ip.zebra.lt>
On Mon, Oct 27, 2008 at 06:53, Paulius Zaleckas wrote:
> + new_bus->irq = kmalloc(sizeof(int) * PHY_MAX_ADDR, GFP_KERNEL);
oh, and this should be kcalloc()
-mike
^ permalink raw reply
* Re: [PATCH] phylib: add mdio-gpio bus driver (v2)
From: Mike Frysinger @ 2008-10-27 14:52 UTC (permalink / raw)
To: Paulius Zaleckas; +Cc: netdev, linux-arm-kernel, linux-embedded
In-Reply-To: <20081027105318.21923.24436.stgit@Programuotojas.82-135-208-232.ip.zebra.lt>
On Mon, Oct 27, 2008 at 06:53, Paulius Zaleckas wrote:
> +config MDIO_GPIO
> + tristate "Support for GPIO bitbanged MDIO buses"
> + depends on MDIO_BITBANG && GENERIC_GPIO
> + help
> + Supports MDIO busses connected to GPIO.
please add a line stating the name of the module if it is built as such
> +static struct mdiobb_ops mdio_gpio_ops = {
> + .owner = THIS_MODULE,
> + .set_mdc = mdc,
> + .set_mdio_dir = mdio_dir,
> + .set_mdio_data = mdio,
> + .get_mdio_data = mdio_read,
> +};
shouldnt the function names match the gpio_ops ? things like "mdc"
and "mdio" are a little obscure ...
> +static int __devinit mdio_gpio_probe(struct platform_device *pdev)
> +{
> + struct mii_bus *new_bus;
> + struct mdio_gpio_info *bitbang;
> + struct mdio_gpio_platform_data *pdata;
> + int ret = -ENOMEM;
> + int i;
> +
> + pdata = pdev->dev.platform_data;
> + if (pdata == NULL)
> + goto out;
> +
> + bitbang = kzalloc(sizeof(struct mdio_gpio_info), GFP_KERNEL);
sizeof(*bitbang) tends to read better and be more resistant to bitrot
> + if (gpio_request(bitbang->mdc, "mdc"))
> + goto out_free_bitbang;
> +
> + if (gpio_request(bitbang->mdio, "mdio"))
> + goto out_free_mdc;
maybe include driver name and/or the platform id ? if you have
multiple mdio-gpio's running at the same time, coordinating may get a
little messy ...
> + new_bus->name = "GPIO Bitbanged MII",
platform id here too ?
> +static int mdio_gpio_init(void)
> +{
> + return platform_driver_register(&mdio_gpio_driver);
> +}
needs __init markings
> +static void mdio_gpio_exit(void)
> +{
> + platform_driver_unregister(&mdio_gpio_driver);
> +}
needs __exit markings
-mike
^ permalink raw reply
* Re: [PATCH] phylib: add mdio-gpio bus driver (v2)
From: Grant Likely @ 2008-10-27 14:37 UTC (permalink / raw)
To: Paulius Zaleckas; +Cc: netdev, linux-arm-kernel, linux-embedded
In-Reply-To: <20081027105318.21923.24436.stgit@Programuotojas.82-135-208-232.ip.zebra.lt>
On Mon, Oct 27, 2008 at 12:53:22PM +0200, Paulius Zaleckas wrote:
> Useful for machines where PHY control is connected to GPIO.
> This driver also supports interrupts from PHY.
>
> Changes since v1:
> - fixed releasing of gpio (thanks to Sascha Hauer)
> - fixed compiling due to bus->dev to bus->parent change
>
> Signed-off-by: Paulius Zaleckas <paulius.zaleckas@teltonika.lt>
> ---
Well structured driver. Comments below.
> diff --git a/drivers/net/phy/mdio-gpio.c b/drivers/net/phy/mdio-gpio.c
> new file mode 100644
> index 0000000..585897b
> --- /dev/null
> +++ b/drivers/net/phy/mdio-gpio.c
> @@ -0,0 +1,187 @@
> +/*
> + * GPIO based MDIO bitbang driver.
> + *
> + * Copyright (C) 2008, Paulius Zaleckas <paulius.zaleckas@teltonika.lt>
> + *
> + * Based on mdio-ofgpio.c:
> + *
> + * Copyright (c) 2008 CSE Semaphore Belgium.
> + * by Laurent Pinchart <laurentp@cse-semaphore.com>
> + *
> + * Copyright (c) 2003 Intracom S.A.
> + * by Pantelis Antoniou <panto@intracom.gr>
> + *
> + * 2005 (c) MontaVista Software, Inc.
> + * Vitaly Bordug <vbordug@ru.mvista.com>
> + *
> + * This file is licensed under the terms of the GNU General Public License
> + * version 2. This program is licensed "as is" without any warranty of any
> + * kind, whether express or implied.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/init.h>
> +#include <linux/platform_device.h>
> +#include <linux/gpio.h>
> +#include <linux/mdio-bitbang.h>
> +#include <linux/mdio-gpio.h>
Missing:
MODULE_AUTHOR()
MODULE_LICENSE()
MODULE_DESCRIPTION()
> +static int __devinit mdio_gpio_probe(struct platform_device *pdev)
> +{
> + struct mii_bus *new_bus;
> + struct mdio_gpio_info *bitbang;
> + struct mdio_gpio_platform_data *pdata;
> + int ret = -ENOMEM;
> + int i;
> +
> + pdata = pdev->dev.platform_data;
> + if (pdata == NULL)
> + goto out;
> +
> + bitbang = kzalloc(sizeof(struct mdio_gpio_info), GFP_KERNEL);
> + if (!bitbang)
> + goto out;
> +
> + bitbang->ctrl.ops = &mdio_gpio_ops;
> + bitbang->mdc = pdata->mdc;
> + bitbang->mdio = pdata->mdio;
> +
> + if (gpio_request(bitbang->mdc, "mdc"))
> + goto out_free_bitbang;
> +
> + if (gpio_request(bitbang->mdio, "mdio"))
> + goto out_free_mdc;
> +
> + new_bus = alloc_mdio_bitbang(&bitbang->ctrl);
> + if (!new_bus)
> + goto out_free_gpio;
> +
> + new_bus->name = "GPIO Bitbanged MII",
> + snprintf(new_bus->id, MII_BUS_ID_SIZE, "phy%i", pdev->id);
> +
> + new_bus->phy_mask = ~0;
> + new_bus->irq = kmalloc(sizeof(int) * PHY_MAX_ADDR, GFP_KERNEL);
> + if (!new_bus->irq)
> + goto out_free_bus;
The IRQ array is fixed size. You can add it to the mdio_gpio_info
structure and then just set the pointer here so that only one kzalloc
is needed.
> +
> + for (i = 0; i < PHY_MAX_ADDR; i++)
> + new_bus->irq[i] = PHY_POLL;
> +
> + for (i = 0; i < pdata->nr_phys; i++) {
> + unsigned int phy_addr = pdata->phys[i].addr;
> +
> + BUG_ON(phy_addr >= PHY_MAX_ADDR);
Is it really worth bringing down the whole kernel when too many phys are
specified in pdata?
> +
> + new_bus->phy_mask &= ~(1 << phy_addr);
> + new_bus->irq[phy_addr] = pdata->phys[i].irq;
> + }
> +
> + new_bus->parent = &pdev->dev;
> + platform_set_drvdata(pdev, new_bus);
> +
> + ret = mdiobus_register(new_bus);
> + if (ret)
> + goto out_free_all;
> +
> + return 0;
> +
> +out_free_all:
> + platform_set_drvdata(pdev, NULL);
> + kfree(new_bus->irq);
> +out_free_bus:
> + free_mdio_bitbang(new_bus);
> +out_free_gpio:
> + gpio_free(bitbang->mdio);
> +out_free_mdc:
> + gpio_free(bitbang->mdc);
> +out_free_bitbang:
> + kfree(bitbang);
> +out:
> + return ret;
Nit: labels in column 0 will confuse diff when it tries to put the
function name in the diff hunk header. If you indent the labels by 1
space then future diffs will show the function name instead of the label
name in diff hunk headers.
> +}
> +
> +static int __devexit mdio_gpio_remove(struct platform_device *pdev)
> +{
> + struct mii_bus *bus = platform_get_drvdata(pdev);
> + struct mdio_gpio_info *bitbang = bus->priv;
> +
> + mdiobus_unregister(bus);
> + kfree(bus->irq);
> + free_mdio_bitbang(bus);
> + platform_set_drvdata(pdev, NULL);
> + gpio_free(bitbang->mdc);
> + gpio_free(bitbang->mdio);
> + kfree(bitbang);
> +
> + return 0;
> +}
> +
> +static struct platform_driver mdio_gpio_driver = {
> + .probe = mdio_gpio_probe,
> + .remove = __devexit_p(mdio_gpio_remove),
> + .driver = {
> + .name = "mdio-gpio",
> + .owner = THIS_MODULE,
> + },
> +};
> +
> +static int mdio_gpio_init(void)
static int __init mdio_gpio_init(void)
> +{
> + return platform_driver_register(&mdio_gpio_driver);
> +}
> +
> +static void mdio_gpio_exit(void)
static void __exit mdio_gpio_exit(void)
> +{
> + platform_driver_unregister(&mdio_gpio_driver);
> +}
> +
> +module_init(mdio_gpio_init);
> +module_exit(mdio_gpio_exit);
^ permalink raw reply
* [PATCH] phylib: add mdio-gpio bus driver (v2)
From: Paulius Zaleckas @ 2008-10-27 10:53 UTC (permalink / raw)
To: netdev; +Cc: linux-arm-kernel, linux-embedded
Useful for machines where PHY control is connected to GPIO.
This driver also supports interrupts from PHY.
Changes since v1:
- fixed releasing of gpio (thanks to Sascha Hauer)
- fixed compiling due to bus->dev to bus->parent change
Signed-off-by: Paulius Zaleckas <paulius.zaleckas@teltonika.lt>
---
drivers/net/phy/Kconfig | 6 +
drivers/net/phy/Makefile | 1
drivers/net/phy/mdio-gpio.c | 187 +++++++++++++++++++++++++++++++++++++++++++
include/linux/mdio-gpio.h | 40 +++++++++
4 files changed, 234 insertions(+), 0 deletions(-)
create mode 100644 drivers/net/phy/mdio-gpio.c
create mode 100644 include/linux/mdio-gpio.h
diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
index d55932a..b6a0350 100644
--- a/drivers/net/phy/Kconfig
+++ b/drivers/net/phy/Kconfig
@@ -84,6 +84,12 @@ config MDIO_BITBANG
If in doubt, say N.
+config MDIO_GPIO
+ tristate "Support for GPIO bitbanged MDIO buses"
+ depends on MDIO_BITBANG && GENERIC_GPIO
+ help
+ Supports MDIO busses connected to GPIO.
+
config MDIO_OF_GPIO
tristate "Support for GPIO lib-based bitbanged MDIO buses"
depends on MDIO_BITBANG && OF_GPIO
diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
index eee329f..8629b09 100644
--- a/drivers/net/phy/Makefile
+++ b/drivers/net/phy/Makefile
@@ -15,4 +15,5 @@ obj-$(CONFIG_ICPLUS_PHY) += icplus.o
obj-$(CONFIG_REALTEK_PHY) += realtek.o
obj-$(CONFIG_FIXED_PHY) += fixed.o
obj-$(CONFIG_MDIO_BITBANG) += mdio-bitbang.o
+obj-$(CONFIG_MDIO_GPIO) += mdio-gpio.o
obj-$(CONFIG_MDIO_OF_GPIO) += mdio-ofgpio.o
diff --git a/drivers/net/phy/mdio-gpio.c b/drivers/net/phy/mdio-gpio.c
new file mode 100644
index 0000000..585897b
--- /dev/null
+++ b/drivers/net/phy/mdio-gpio.c
@@ -0,0 +1,187 @@
+/*
+ * GPIO based MDIO bitbang driver.
+ *
+ * Copyright (C) 2008, Paulius Zaleckas <paulius.zaleckas@teltonika.lt>
+ *
+ * Based on mdio-ofgpio.c:
+ *
+ * Copyright (c) 2008 CSE Semaphore Belgium.
+ * by Laurent Pinchart <laurentp@cse-semaphore.com>
+ *
+ * Copyright (c) 2003 Intracom S.A.
+ * by Pantelis Antoniou <panto@intracom.gr>
+ *
+ * 2005 (c) MontaVista Software, Inc.
+ * Vitaly Bordug <vbordug@ru.mvista.com>
+ *
+ * This file is licensed under the terms of the GNU General Public License
+ * version 2. This program is licensed "as is" without any warranty of any
+ * kind, whether express or implied.
+ */
+
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/platform_device.h>
+#include <linux/gpio.h>
+#include <linux/mdio-bitbang.h>
+#include <linux/mdio-gpio.h>
+
+struct mdio_gpio_info {
+ struct mdiobb_ctrl ctrl;
+ unsigned int mdc, mdio;
+};
+
+static void mdio_dir(struct mdiobb_ctrl *ctrl, int dir)
+{
+ struct mdio_gpio_info *bitbang =
+ container_of(ctrl, struct mdio_gpio_info, ctrl);
+
+ if (dir)
+ gpio_direction_output(bitbang->mdio, 1);
+ else
+ gpio_direction_input(bitbang->mdio);
+}
+
+static int mdio_read(struct mdiobb_ctrl *ctrl)
+{
+ struct mdio_gpio_info *bitbang =
+ container_of(ctrl, struct mdio_gpio_info, ctrl);
+
+ return gpio_get_value(bitbang->mdio);
+}
+
+static void mdio(struct mdiobb_ctrl *ctrl, int what)
+{
+ struct mdio_gpio_info *bitbang =
+ container_of(ctrl, struct mdio_gpio_info, ctrl);
+
+ gpio_set_value(bitbang->mdio, what);
+}
+
+static void mdc(struct mdiobb_ctrl *ctrl, int what)
+{
+ struct mdio_gpio_info *bitbang =
+ container_of(ctrl, struct mdio_gpio_info, ctrl);
+
+ gpio_set_value(bitbang->mdc, what);
+}
+
+static struct mdiobb_ops mdio_gpio_ops = {
+ .owner = THIS_MODULE,
+ .set_mdc = mdc,
+ .set_mdio_dir = mdio_dir,
+ .set_mdio_data = mdio,
+ .get_mdio_data = mdio_read,
+};
+
+static int __devinit mdio_gpio_probe(struct platform_device *pdev)
+{
+ struct mii_bus *new_bus;
+ struct mdio_gpio_info *bitbang;
+ struct mdio_gpio_platform_data *pdata;
+ int ret = -ENOMEM;
+ int i;
+
+ pdata = pdev->dev.platform_data;
+ if (pdata == NULL)
+ goto out;
+
+ bitbang = kzalloc(sizeof(struct mdio_gpio_info), GFP_KERNEL);
+ if (!bitbang)
+ goto out;
+
+ bitbang->ctrl.ops = &mdio_gpio_ops;
+ bitbang->mdc = pdata->mdc;
+ bitbang->mdio = pdata->mdio;
+
+ if (gpio_request(bitbang->mdc, "mdc"))
+ goto out_free_bitbang;
+
+ if (gpio_request(bitbang->mdio, "mdio"))
+ goto out_free_mdc;
+
+ new_bus = alloc_mdio_bitbang(&bitbang->ctrl);
+ if (!new_bus)
+ goto out_free_gpio;
+
+ new_bus->name = "GPIO Bitbanged MII",
+ snprintf(new_bus->id, MII_BUS_ID_SIZE, "phy%i", pdev->id);
+
+ new_bus->phy_mask = ~0;
+ new_bus->irq = kmalloc(sizeof(int) * PHY_MAX_ADDR, GFP_KERNEL);
+ if (!new_bus->irq)
+ goto out_free_bus;
+
+ for (i = 0; i < PHY_MAX_ADDR; i++)
+ new_bus->irq[i] = PHY_POLL;
+
+ for (i = 0; i < pdata->nr_phys; i++) {
+ unsigned int phy_addr = pdata->phys[i].addr;
+
+ BUG_ON(phy_addr >= PHY_MAX_ADDR);
+
+ new_bus->phy_mask &= ~(1 << phy_addr);
+ new_bus->irq[phy_addr] = pdata->phys[i].irq;
+ }
+
+ new_bus->parent = &pdev->dev;
+ platform_set_drvdata(pdev, new_bus);
+
+ ret = mdiobus_register(new_bus);
+ if (ret)
+ goto out_free_all;
+
+ return 0;
+
+out_free_all:
+ platform_set_drvdata(pdev, NULL);
+ kfree(new_bus->irq);
+out_free_bus:
+ free_mdio_bitbang(new_bus);
+out_free_gpio:
+ gpio_free(bitbang->mdio);
+out_free_mdc:
+ gpio_free(bitbang->mdc);
+out_free_bitbang:
+ kfree(bitbang);
+out:
+ return ret;
+}
+
+static int __devexit mdio_gpio_remove(struct platform_device *pdev)
+{
+ struct mii_bus *bus = platform_get_drvdata(pdev);
+ struct mdio_gpio_info *bitbang = bus->priv;
+
+ mdiobus_unregister(bus);
+ kfree(bus->irq);
+ free_mdio_bitbang(bus);
+ platform_set_drvdata(pdev, NULL);
+ gpio_free(bitbang->mdc);
+ gpio_free(bitbang->mdio);
+ kfree(bitbang);
+
+ return 0;
+}
+
+static struct platform_driver mdio_gpio_driver = {
+ .probe = mdio_gpio_probe,
+ .remove = __devexit_p(mdio_gpio_remove),
+ .driver = {
+ .name = "mdio-gpio",
+ .owner = THIS_MODULE,
+ },
+};
+
+static int mdio_gpio_init(void)
+{
+ return platform_driver_register(&mdio_gpio_driver);
+}
+
+static void mdio_gpio_exit(void)
+{
+ platform_driver_unregister(&mdio_gpio_driver);
+}
+
+module_init(mdio_gpio_init);
+module_exit(mdio_gpio_exit);
diff --git a/include/linux/mdio-gpio.h b/include/linux/mdio-gpio.h
new file mode 100644
index 0000000..82d5fa4
--- /dev/null
+++ b/include/linux/mdio-gpio.h
@@ -0,0 +1,40 @@
+/*
+ * MDIO-GPIO bus platform data structures
+ *
+ * Copyright (C) 2008, Paulius Zaleckas <paulius.zaleckas@teltonika.lt>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+ */
+
+#ifndef __LINUX_MDIO_GPIO_H
+#define __LINUX_MDIO_GPIO_H
+
+struct mdio_gpio_phy {
+ /* PHY address on MDIO bus */
+ unsigned int addr;
+ /* preconfigured irq connected to PHY or -1 if no irq */
+ int irq;
+};
+
+struct mdio_gpio_platform_data {
+ /* GPIO numbers for bus pins */
+ unsigned int mdc;
+ unsigned int mdio;
+
+ unsigned int nr_phys;
+ struct mdio_gpio_phy *phys;
+};
+
+#endif /* __LINUX_MDIO_GPIO_H */
^ permalink raw reply related
* Re: USB, Host and Device on the same machine
From: Ming Lei @ 2008-10-24 8:01 UTC (permalink / raw)
To: Fredrik Johansson
Cc: linux-embedded-u79uwXL29TY76Z2rM5mHXA,
linux-usb-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <53e45cc30810240018w1c3c73c2nf9a9b606e0e7dea4-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2008/10/24 Fredrik Johansson <frejo242-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>:
> Hello
>
> I am working on an embedded machine that has two USB controllers, one
> is a OTG and the other is a pure host.
>
> I have succesfully used g_serial driver on the OTG controller in
> device mode and talked to my pc using the cdc-acm driver.
> With the g_serial unloaded I have also enabled the ehci-hcd and
> cdc-acm drivers and connected the host port to another serial device.
>
> The problem I have now is that I want to be able to have my embedded
> machine work as a device on the OTG port and a host on the host port,
> but if I load the ehci-hcd driver with g_serial already loaded it
> screws everything up since ehci-hcd driver attaches to both the OTG
> and the host controller, how can i prevent it from using the OTG
> controller?
It is a platform dependent problem. what is your platform ?( musb? )
which buses do the otg controller and ehci controller attache to?
>
> Best regards
> Fredrik Johansson
> --
> To unsubscribe from this list: send the line "unsubscribe linux-embedded" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
Lei Ming
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* USB, Host and Device on the same machine
From: Fredrik Johansson @ 2008-10-24 7:18 UTC (permalink / raw)
To: linux-embedded
Hello
I am working on an embedded machine that has two USB controllers, one
is a OTG and the other is a pure host.
I have succesfully used g_serial driver on the OTG controller in
device mode and talked to my pc using the cdc-acm driver.
With the g_serial unloaded I have also enabled the ehci-hcd and
cdc-acm drivers and connected the host port to another serial device.
The problem I have now is that I want to be able to have my embedded
machine work as a device on the OTG port and a host on the host port,
but if I load the ehci-hcd driver with g_serial already loaded it
screws everything up since ehci-hcd driver attaches to both the OTG
and the host controller, how can i prevent it from using the OTG
controller?
Best regards
Fredrik Johansson
^ permalink raw reply
* Re: [PATCH v2 RESEND] gpiolib: Add pin change notification
From: Ben Nizette @ 2008-10-24 0:28 UTC (permalink / raw)
To: Andrew Morton; +Cc: david-b, linux-kernel, linux-embedded
In-Reply-To: <20081021133106.be5cc01c.akpm@linux-foundation.org>
On Tue, 2008-10-21 at 13:31 -0700, Andrew Morton wrote:
> On Tue, 21 Oct 2008 09:50:06 +1100
> Ben Nizette <bn@niasdigital.com> wrote:
> > +static irqreturn_t gpio_irq_handler(int irq, void *dev_id)
> > +{
> > + struct gpio_desc *desc = dev_id;
> > + int gpio = desc - gpio_desc;
> > + int new, old;
> > +
> > + if (!gpio_is_valid(gpio))
> > + return IRQ_NONE;
> > +
> > + new = gpio_get_value(gpio);
> > + old = desc->val;
> > +
> > + if ((new && !old && test_bit(ASYNC_RISING, &desc->flags)) ||
> > + (!new && old && test_bit(ASYNC_FALLING, &desc->flags)))
> > + sysfs_notify(&desc->dev->kobj, NULL, "value");
>
> eekeekeek! sysfs_notify() does mutex_lock() and will die horridly if
> called from an interrupt handler.
>
> You should have got a storm of warnings when runtime testing this code.
> Please ensure that all debug options are enabled when testing code.
> Documentation/SubmitChecklist has help.
0_o yea that isn't great... Thanks - shall respin and repost shortly.
--Ben.
^ permalink raw reply
* Re: Subject: [PATCH 12/16] Squashfs: header files
From: Phillip Lougher @ 2008-10-23 8:56 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: akpm, linux-embedded, linux-fsdevel, linux-kernel, tim.bird
In-Reply-To: <Pine.LNX.4.64.0810221801570.5634@vixen.sonytel.be>
Geert Uytterhoeven wrote:
> On Fri, 17 Oct 2008, Phillip Lougher wrote:
>> +#ifdef SQUASHFS_TRACE
>> +#define TRACE(s, args...) printk(KERN_NOTICE "SQUASHFS: "s, ## args)
>> +#else
>> +#define TRACE(s, args...) {}
>> +#endif
>
> Just use
>
> #define TRACE(s, args...) pr_debug("SQUASHFS: "s, ## args)
OK.
>> +
>> +#define WARNING(s, args...) printk(KERN_WARNING "SQUASHFS: "s, ## args)
> ^^^^^^^^^^^^^^^^
> pr_warning(
>
OK.
>
> SQUASHFS_MKFLAGS() isn't used by the kernel, only by the tools (mksquashfs)?
>
Only used by mksquashfs. I pulled out the user-space only stuff into a
separate include, but must have missed this one.
>> +/* meta index cache */
>> +#define SQUASHFS_META_INDEXES (SQUASHFS_METADATA_SIZE / sizeof(unsigned int))
> ^^^^^^^^^^^^
> I was wondering whether a meta index is an `unsigned int' or an `__le32', but I
> couldn't find it easily.
>
Unsigned int, it's used internally by the index cache code (file.c). It
works out how many block indexes can fit into one compressed metadata block.
Phillip
^ permalink raw reply
* Re: Subject: [PATCH 01/16] Squashfs: inode operations
From: Phillip Lougher @ 2008-10-23 8:42 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: akpm, linux-embedded, linux-fsdevel, linux-kernel, tim.bird
In-Reply-To: <Pine.LNX.4.64.0810221907080.20919@vixen.sonytel.be>
Geert Uytterhoeven wrote:
> Sparse with endian checking (make C=2 CHECKFLAGS="-D__CHECK_ENDIAN__") complains
> aibout these:
>
> | fs/squashfs/inode.c:306:25: warning: cast to restricted __le16
> | fs/squashfs/inode.c:324:25: warning: cast to restricted __le16
>
> and it seems to be right, as inode.i_mode is not __le16. I think the le16_to_cpu()
> should be removed.
Yes, you're right. Fixed thanks.
>
> BTW, there are also a few sparse warnings about different signednesses, so you
> probably want to run sparse yourself, too.
I'll do that.
Phillip
^ permalink raw reply
* Re: Subject: [PATCH 00/16] Squashfs: compressed read-only filesystem
From: Phillip Lougher @ 2008-10-23 8:40 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: akpm, linux-embedded, linux-fsdevel, linux-kernel, tim.bird
In-Reply-To: <Pine.LNX.4.64.0810221913330.20919@vixen.sonytel.be>
Geert Uytterhoeven wrote:
> Hi Phillip,
>
> On Fri, 17 Oct 2008, Phillip Lougher wrote:
>> This is a second attempt at mainlining Squashfs. The first attempt was way
>
> This is great news!
>
> I ran a quick test of squashfs 4.0 (the CVS version) on UML/ia32 and ppc64, and it
> seems to work fine! Great job! Let's hope we'll see it in mainline soon...
>
Thanks! I hope it gets into mainline soon too :)
> BTW, one minor gripe is that the current mksquashfs doesn't want to run on big
> endian yet, as there's no byteswapping support.
Yeah, I know about that. There's still some work needing to be done on
the squashfs-tools. I figured it was important to get the kernel stuff
submitted and discussed ASAP.
Phillip
^ permalink raw reply
* Re: Subject: [PATCH 00/16] Squashfs: compressed read-only filesystem
From: Geert Uytterhoeven @ 2008-10-22 17:14 UTC (permalink / raw)
To: Phillip Lougher
Cc: akpm, linux-embedded, linux-fsdevel, linux-kernel, tim.bird
In-Reply-To: <E1KqrTW-0001x8-3h@dylan>
[-- Attachment #1: Type: TEXT/PLAIN, Size: 867 bytes --]
Hi Phillip,
On Fri, 17 Oct 2008, Phillip Lougher wrote:
> This is a second attempt at mainlining Squashfs. The first attempt was way
This is great news!
I ran a quick test of squashfs 4.0 (the CVS version) on UML/ia32 and ppc64, and it
seems to work fine! Great job! Let's hope we'll see it in mainline soon...
BTW, one minor gripe is that the current mksquashfs doesn't want to run on big
endian yet, as there's no byteswapping support.
With kind regards,
Geert Uytterhoeven
Software Architect
Sony Techsoft Centre Europe
The Corporate Village · Da Vincilaan 7-D1 · B-1935 Zaventem · Belgium
Phone: +32 (0)2 700 8453
Fax: +32 (0)2 700 8622
E-mail: Geert.Uytterhoeven@sonycom.com
Internet: http://www.sony-europe.com/
A division of Sony Europe (Belgium) N.V.
VAT BE 0413.825.160 · RPR Brussels
Fortis · BIC GEBABEBB · IBAN BE41293037680010
^ permalink raw reply
* Re: Subject: [PATCH 01/16] Squashfs: inode operations
From: Geert Uytterhoeven @ 2008-10-22 17:13 UTC (permalink / raw)
To: Phillip Lougher
Cc: akpm, linux-embedded, linux-fsdevel, linux-kernel, tim.bird
In-Reply-To: <E1KqrTW-0001xC-9M@dylan>
[-- Attachment #1: Type: TEXT/PLAIN, Size: 2133 bytes --]
On Fri, 17 Oct 2008, Phillip Lougher wrote:
> --- /dev/null
> +++ b/fs/squashfs/inode.c
> + case SQUASHFS_BLKDEV_TYPE:
> + case SQUASHFS_CHRDEV_TYPE: {
> + struct squashfs_dev_inode *inodep = &id.dev;
> + unsigned int rdev;
> +
> + if (!squashfs_read_metadata(s, inodep, block, offset,
> + sizeof(*inodep), &next_block, &next_offset))
> + goto failed_read;
> +
> + i->i_nlink = le32_to_cpu(inodep->nlink);
> + i->i_mode |= (type == SQUASHFS_CHRDEV_TYPE) ? S_IFCHR : S_IFBLK;
> + rdev = le32_to_cpu(inodep->rdev);
> + init_special_inode(i, le16_to_cpu(i->i_mode),
^^^^^^^^^^^
> + new_decode_dev(rdev));
> +
> + TRACE("Device inode %x:%x, rdev %x\n",
> + SQUASHFS_INODE_BLK(inode), offset, rdev);
> + break;
> + }
> + case SQUASHFS_FIFO_TYPE:
> + case SQUASHFS_SOCKET_TYPE: {
> + struct squashfs_ipc_inode *inodep = &id.ipc;
> +
> + if (!squashfs_read_metadata(s, inodep, block, offset,
> + sizeof(*inodep), &next_block, &next_offset))
> + goto failed_read;
> +
> + i->i_nlink = le32_to_cpu(inodep->nlink);
> + i->i_mode |= (type == SQUASHFS_FIFO_TYPE) ? S_IFIFO : S_IFSOCK;
> + init_special_inode(i, le16_to_cpu(i->i_mode), 0);
^^^^^^^^^^^
> + break;
Sparse with endian checking (make C=2 CHECKFLAGS="-D__CHECK_ENDIAN__") complains
about these:
| fs/squashfs/inode.c:306:25: warning: cast to restricted __le16
| fs/squashfs/inode.c:324:25: warning: cast to restricted __le16
and it seems to be right, as inode.i_mode is not __le16. I think the le16_to_cpu()
should be removed.
BTW, there are also a few sparse warnings about different signednesses, so you
probably want to run sparse yourself, too.
With kind regards,
Geert Uytterhoeven
Software Architect
Sony Techsoft Centre Europe
The Corporate Village · Da Vincilaan 7-D1 · B-1935 Zaventem · Belgium
Phone: +32 (0)2 700 8453
Fax: +32 (0)2 700 8622
E-mail: Geert.Uytterhoeven@sonycom.com
Internet: http://www.sony-europe.com/
A division of Sony Europe (Belgium) N.V.
VAT BE 0413.825.160 · RPR Brussels
Fortis · BIC GEBABEBB · IBAN BE41293037680010
^ permalink raw reply
* Re: Subject: [PATCH 09/16] Squashfs: uid/gid lookup operations
From: Geert Uytterhoeven @ 2008-10-22 16:38 UTC (permalink / raw)
To: Phillip Lougher
Cc: akpm, linux-embedded, linux-fsdevel, linux-kernel, tim.bird
In-Reply-To: <E1KqrTX-0001xo-4N@dylan>
[-- Attachment #1: Type: TEXT/PLAIN, Size: 916 bytes --]
On Fri, 17 Oct 2008, Phillip Lougher wrote:
> --- /dev/null
> +++ b/fs/squashfs/id.c
> +__le64 *read_id_index_table(struct super_block *s, long long id_table_start,
> + unsigned short no_ids)
> +{
> + unsigned int length = SQUASHFS_ID_BLOCK_BYTES(no_ids);
> + __le64 *id_table;
> +
> + TRACE("In read_id_index_table, length %d\n", length);
> +
> + /* Allocate id index table */
> + id_table = kmalloc(length, GFP_KERNEL);
^^^^^^
Can this ever be larger than 64 KiB?
With kind regards,
Geert Uytterhoeven
Software Architect
Sony Techsoft Centre Europe
The Corporate Village · Da Vincilaan 7-D1 · B-1935 Zaventem · Belgium
Phone: +32 (0)2 700 8453
Fax: +32 (0)2 700 8622
E-mail: Geert.Uytterhoeven@sonycom.com
Internet: http://www.sony-europe.com/
A division of Sony Europe (Belgium) N.V.
VAT BE 0413.825.160 · RPR Brussels
Fortis · BIC GEBABEBB · IBAN BE41293037680010
^ permalink raw reply
* Re: Subject: [PATCH 04/16] Squashfs: regular file operations
From: Geert Uytterhoeven @ 2008-10-22 16:35 UTC (permalink / raw)
To: Phillip Lougher
Cc: akpm, linux-embedded, linux-fsdevel, linux-kernel, tim.bird
In-Reply-To: <E1KqrTW-0001xQ-Kx@dylan>
[-- Attachment #1: Type: TEXT/PLAIN, Size: 1238 bytes --]
On Fri, 17 Oct 2008, Phillip Lougher wrote:
> --- /dev/null
> +++ b/fs/squashfs/file.c
> +static long long read_blocklist(struct inode *inode, int index,
> + unsigned int *bsize)
> +{
> + long long start, block, blks;
> + int offset;
> + __le32 size;
> + int res = fill_meta_index(inode, index, &start, &offset, &block);
> +
> + TRACE("read_blocklist: res %d, index %d, start 0x%llx, offset"
> + " 0x%x, block 0x%llx\n", res, index, start, offset,
> + block);
> +
> + if (res == -1)
> + goto failure;
^^^^^^^^^^^^^
> + if (blks == -1)
> + goto failure;
^^^^^^^^^^^^^
> + if (res == 0)
> + goto failure;
^^^^^^^^^^^^^
> +failure:
> + return -1;
> +}
As there's nothing to clean up, you can just use `return -1' instead of `goto
failure', and save a few lines.
With kind regards,
Geert Uytterhoeven
Software Architect
Sony Techsoft Centre Europe
The Corporate Village · Da Vincilaan 7-D1 · B-1935 Zaventem · Belgium
Phone: +32 (0)2 700 8453
Fax: +32 (0)2 700 8622
E-mail: Geert.Uytterhoeven@sonycom.com
Internet: http://www.sony-europe.com/
A division of Sony Europe (Belgium) N.V.
VAT BE 0413.825.160 · RPR Brussels
Fortis · BIC GEBABEBB · IBAN BE41293037680010
^ permalink raw reply
* Re: Subject: [PATCH 01/16] Squashfs: inode operations
From: Geert Uytterhoeven @ 2008-10-22 16:32 UTC (permalink / raw)
To: Phillip Lougher
Cc: akpm, linux-embedded, linux-fsdevel, linux-kernel, tim.bird
In-Reply-To: <E1KqrTW-0001xC-9M@dylan>
[-- Attachment #1: Type: TEXT/PLAIN, Size: 983 bytes --]
On Fri, 17 Oct 2008, Phillip Lougher wrote:
> --- /dev/null
> +++ b/fs/squashfs/inode.c
> +static int squashfs_new_inode(struct super_block *s, struct inode *i,
> + struct squashfs_base_inode *inodeb)
> +{
> + if (squashfs_get_id(s, le16_to_cpu(inodeb->uid), &i->i_uid) == 0)
> + goto out;
^^^^^^^^
> + if (squashfs_get_id(s, le16_to_cpu(inodeb->guid), &i->i_gid) == 0)
> + goto out;
^^^^^^^^
> + return 1;
> +
> +out:
> + return 0;
> +}
As there's nothing to clean up, you can just use `return 0' instead of `goto
out'.
With kind regards,
Geert Uytterhoeven
Software Architect
Sony Techsoft Centre Europe
The Corporate Village · Da Vincilaan 7-D1 · B-1935 Zaventem · Belgium
Phone: +32 (0)2 700 8453
Fax: +32 (0)2 700 8622
E-mail: Geert.Uytterhoeven@sonycom.com
Internet: http://www.sony-europe.com/
A division of Sony Europe (Belgium) N.V.
VAT BE 0413.825.160 · RPR Brussels
Fortis · BIC GEBABEBB · IBAN BE41293037680010
^ permalink raw reply
* Re: Subject: [PATCH 00/16] Squashfs: compressed read-only filesystem
From: Tim Bird @ 2008-10-22 16:32 UTC (permalink / raw)
To: Phillip Lougher
Cc: David P. Quigley, akpm, linux-embedded, linux-fsdevel,
linux-kernel
In-Reply-To: <48FE68E9.2020401@lougher.demon.co.uk>
Phillip Lougher wrote:
> Yeah, Git is much better than CVS, however, I've got nowhere to host a
> public Git repository. If someone were to offer hosting I'd be only too
> happy to move over to Git.
I can offer you hosting on mirror.celinuxforum.org. If you are
interested, let me know and I'll set up an account immediately.
I haven't hosted a public git tree there, so there may be
some additional setup required.
-- Tim
=============================
Tim Bird
Architecture Group Chair, CE Linux Forum
Senior Staff Engineer, Sony Corporation of America
=============================
^ permalink raw reply
* Re: Subject: [PATCH 12/16] Squashfs: header files
From: Geert Uytterhoeven @ 2008-10-22 16:13 UTC (permalink / raw)
To: Phillip Lougher
Cc: akpm, linux-embedded, linux-fsdevel, linux-kernel, tim.bird
In-Reply-To: <E1KqrTX-0001y4-JE@dylan>
[-- Attachment #1: Type: TEXT/PLAIN, Size: 3558 bytes --]
On Fri, 17 Oct 2008, Phillip Lougher wrote:
> --- /dev/null
> +++ b/fs/squashfs/squashfs.h
> +#ifdef SQUASHFS_TRACE
> +#define TRACE(s, args...) printk(KERN_NOTICE "SQUASHFS: "s, ## args)
> +#else
> +#define TRACE(s, args...) {}
> +#endif
Just use
#define TRACE(s, args...) pr_debug("SQUASHFS: "s, ## args)
so we always get printf()-format checking, irrespective of whether DEBUG is
defined or not.
If you really want to keep the KERN_NOTICE (and the SQUASHFS_TRACE), you can
use
#ifdef SQUASHFS_TRACE
#define TRACE(s, args...) pr_notice("SQUASHFS: "s, ## args)
#else
#define TRACE(s, args...) pr_debug("SQUASHFS: "s, ## args)
#endif
> +#define ERROR(s, args...) printk(KERN_ERR "SQUASHFS error: "s, ## args)
^^^^^^^^^^^^^^^^
pr_err(
> +
> +#define SERROR(s, args...) \
> + do { \
> + if (!silent) \
> + printk(KERN_ERR "SQUASHFS error: "s, ## args);\
^^^^^^^^^^^^^^^^
pr_err(
> + } while (0)
(yes, I know SERROR() no longer exists in CVS)
> +
> +#define WARNING(s, args...) printk(KERN_WARNING "SQUASHFS: "s, ## args)
^^^^^^^^^^^^^^^^
pr_warning(
> --- /dev/null
> +++ b/include/linux/squashfs_fs.h
> +/* Filesystem flags */
> +#define SQUASHFS_NOI 0
> +#define SQUASHFS_NOD 1
> +#define SQUASHFS_NOF 3
> +#define SQUASHFS_NO_FRAG 4
> +#define SQUASHFS_ALWAYS_FRAG 5
> +#define SQUASHFS_DUPLICATE 6
> +#define SQUASHFS_EXPORT 7
> +
> +#define SQUASHFS_BIT(flag, bit) ((flag >> bit) & 1)
> +
> +#define SQUASHFS_UNCOMPRESSED_INODES(flags) SQUASHFS_BIT(flags, \
> + SQUASHFS_NOI)
> +
> +#define SQUASHFS_UNCOMPRESSED_DATA(flags) SQUASHFS_BIT(flags, \
> + SQUASHFS_NOD)
> +
> +#define SQUASHFS_UNCOMPRESSED_FRAGMENTS(flags) SQUASHFS_BIT(flags, \
> + SQUASHFS_NOF)
> +
> +#define SQUASHFS_NO_FRAGMENTS(flags) SQUASHFS_BIT(flags, \
> + SQUASHFS_NO_FRAG)
> +
> +#define SQUASHFS_ALWAYS_FRAGMENTS(flags) SQUASHFS_BIT(flags, \
> + SQUASHFS_ALWAYS_FRAG)
> +
> +#define SQUASHFS_DUPLICATES(flags) SQUASHFS_BIT(flags, \
> + SQUASHFS_DUPLICATE)
> +
> +#define SQUASHFS_EXPORTABLE(flags) SQUASHFS_BIT(flags, \
> + SQUASHFS_EXPORT)
> +
> +#define SQUASHFS_MKFLAGS(noi, nod, nof, no_frag, always_frag, \
> + duplicate_checking, exportable) (noi | (nod << 1) \
^
> + | (nof << 3) | (no_frag << 4) | (always_frag << 5) | \
^ ^ ^
> + (duplicate_checking << 6) | (exportable << 7))
^ ^
For the shifts, you can use the SQUASHFS_* filesystem flags defined above
instead of the hardcoded numbers.
SQUASHFS_MKFLAGS() isn't used by the kernel, only by the tools (mksquashfs)?
> +/* meta index cache */
> +#define SQUASHFS_META_INDEXES (SQUASHFS_METADATA_SIZE / sizeof(unsigned int))
^^^^^^^^^^^^
I was wondering whether a meta index is an `unsigned int' or an `__le32', but I
couldn't find it easily.
With kind regards,
Geert Uytterhoeven
Software Architect
Sony Techsoft Centre Europe
The Corporate Village · Da Vincilaan 7-D1 · B-1935 Zaventem · Belgium
Phone: +32 (0)2 700 8453
Fax: +32 (0)2 700 8622
E-mail: Geert.Uytterhoeven@sonycom.com
Internet: http://www.sony-europe.com/
A division of Sony Europe (Belgium) N.V.
VAT BE 0413.825.160 · RPR Brussels
Fortis · BIC GEBABEBB · IBAN BE41293037680010
^ permalink raw reply
* Re: Subject: [PATCH 01/16] Squashfs: inode operations
From: Christoph Hellwig @ 2008-10-22 8:48 UTC (permalink / raw)
To: David P. Quigley
Cc: J?rn Engel, Phillip Lougher, akpm, linux-embedded, linux-fsdevel,
linux-kernel, tim.bird
In-Reply-To: <1224605666.31157.71.camel@moss-terrapins.epoch.ncsc.mil>
On Tue, Oct 21, 2008 at 12:14:26PM -0400, David P. Quigley wrote:
> I looked at where filesystems such as ext3 store these and it seems to
> be in include/linux. I'm assuming this is because usespace utilities
> like fsck need them. It seems wrong for userspace tools to have their
> own private copy since you can potentially have them out of sync with
> the kernel you are running and it provides more chance for you
> forgetting to update a structure somewhere.
All modern filesystems have it in their directories, and ext3 will have
that soon too. The only thing that should go into include/linux are
defintions for ioctls if you have them. It is absolutely intention that
the tools can get out of sync with the kernel, because that actually
keeps them compiling when you update things in the kernel - note that
a single on disk format can be represented by lots of different things
in C, and for various reaosons those can change once in a while in the
kernel. It also allows you to actually compile the tools on a system
that doesn't have new enough kernel headers yet (e.g. debian
autobuilders) or even run the tools on various platforms that have no
or different kernel implementations.
^ permalink raw reply
* Re: Subject: [PATCH 00/16] Squashfs: compressed read-only filesystem
From: David Woodhouse @ 2008-10-22 7:23 UTC (permalink / raw)
To: Phillip Lougher
Cc: David P. Quigley, akpm, linux-embedded, linux-fsdevel,
linux-kernel, tim.bird
In-Reply-To: <48FE68E9.2020401@lougher.demon.co.uk>
On Wed, 2008-10-22 at 00:42 +0100, Phillip Lougher wrote:
> Yeah, Git is much better than CVS, however, I've got nowhere to host a
> public Git repository. If someone were to offer hosting I'd be only too
> happy to move over to Git.
Mail me a SSH public key (use a passphrase on it), and I'll give you an
account on git.infradead.org.
--
David Woodhouse Open Source Technology Centre
David.Woodhouse@intel.com Intel Corporation
^ permalink raw reply
* Re: Subject: [PATCH 00/16] Squashfs: compressed read-only filesystem
From: Peter Korsgaard @ 2008-10-22 7:21 UTC (permalink / raw)
To: Phillip Lougher
Cc: David P. Quigley, akpm, linux-embedded, linux-fsdevel,
linux-kernel, tim.bird
In-Reply-To: <48FE68E9.2020401@lougher.demon.co.uk>
>>>>> "Phillip" == Phillip Lougher <phillip@lougher.demon.co.uk> writes:
Hi,
Phillip> Yeah, Git is much better than CVS, however, I've got nowhere
Phillip> to host a public Git repository. If someone were to offer
Phillip> hosting I'd be only too happy to move over to Git.
You could apply for a kernel.org account:
http://kernel.org/faq/#account
--
Bye, Peter Korsgaard
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox