RE: [PWM PATCH 2/5] Emulates PWM hardware using a high-resolution timer and a GPIO pin
On Friday, February 12, 2010 9:27 AM, Bill Gatliff wrote: H Hartley Sweeten wrote: FWIW, the gpiolib API will accept any non-zero value to set a gpio pin and a zero value to clear the pin. It makes me sort of cringe to say this, but I'm going to assume that the API is intended to work that way so that I can take advantage of it. But I'd love to someday have the reassurance that gpiolib really *is* intended to work that way (might be a bad idea though, see below). Call me paranoid, but I've lost a lot of hair over the years undoing the damage of similar failed assumptions. I've found the Linux kernel code to be refreshingly forgiving of such things, however, so I'm willing to risk it this time. :) For that matter, some of the gpiolib drivers end up returning the bit position mask for a given gpio pin and not 1 or 0. For instance if the gpio pin in question is bit 6 in an 8-bit register and it is set, a __gpio_get_value will end up returning 0x40 and not '1'. Who's to say that gpios should always be boolean? On the output side, a gpio that's four bits wide might be a useful way of dealing with bar graphs, seven-segment displays, etc. (but more specialized abstractions might be more appropriate, of course). I think the original intention of gpiolib was to deal with individual pins. A two-bit gpio input might make it easier to deal with rotary encoders. True. But a two-bit gpio is no longer a pin it's now a port. I have been messing with a port extension for gpiolib but it's not even close to being mergable yet. But, that's a different topic... But for now, GPIOs are assumed to be booleans and that's why I'm hesitant to feed the API non-boolean values. Someday, those values might mean something subtly but disastrously different. And given my luck lately with such things... True. As such just use ! and !! to make the values boolean. gpio is bit 6 val = 0x40 - !(0x40) = 0 val = 0x40 - !!(0x40) = 1 Just my .02... Regards, Hartley -- To unsubscribe from this list: send the line unsubscribe linux-embedded in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PWM PATCH 2/5] Emulates PWM hardware using a high-resolution timer and a GPIO pin
Thursday, February 11, 2010 1:35 PM, Bill Gatliff wrote:
Pavel Machek wrote:
+static void
+gpio_pwm_work (struct work_struct *work)
+{
+ struct gpio_pwm *gp = container_of(work, struct gpio_pwm, work);
+
+ if (gp-active)
+ gpio_direction_output(gp-gpio, gp-polarity ? 1 : 0);
+ else
+ gpio_direction_output(gp-gpio, gp-polarity ? 0 : 1);
+}
...polarity ^ active ?
... except that if polarity and/or active are 1, I don't send the
values 1 or 0 to gpio_direction_output. I don't know if the API is
specifically intended to accept nonzero values that are greater than 1.
FWIW, the gpiolib API will accept any non-zero value to set a gpio pin
and a zero value to clear the pin.
For that matter, some of the gpiolib drivers end up returning the bit
position mask for a given gpio pin and not 1 or 0. For instance if the
gpio pin in question is bit 6 in an 8-bit register and it is set, a
__gpio_get_value will end up returning 0x40 and not '1'.
Regards,
Hartley
--
To unsubscribe from this list: send the line unsubscribe linux-embedded in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PWM PATCH 0/5] Implements a common PWM API
On Tuesday, February 02, 2010 10:44 AM, H Hartley Sweeten wrote: On Tuesday, February 02, 2010 12:15 AM, Bill Gatliff wrote: This patch series implements a common PWM API. This series incorporates the feedback from the linux-embedded mailing list and elsewhere; the author greatly appreciates the efforts of everyone who reviewed the previous version of this code. Bill Gatliff (5): API to consolidate PWM devices behind a common user and kernel interface Emulates PWM hardware using a high-resolution timer and a GPIO pin Expunge old Atmel PWMC driver, replacing it with one that conforms to the PWM API PWM-based LED control LED dimmer trigger Documentation/pwm.txt | 260 ++ drivers/leds/leds-pwm.c | 224 +--- drivers/leds/ledtrig-dim.c | 95 +++ drivers/misc/Makefile |6 +- drivers/misc/atmel_pwm.c| 409 drivers/pwm/atmel-pwm.c | 589 drivers/pwm/gpio.c | 307 + drivers/pwm/pwm.c | 633 +++ include/linux/pwm.h | 31 -- include/linux/pwm/pwm-led.h | 34 +++ include/linux/pwm/pwm.h | 170 11 files changed, 2217 insertions(+), 541 deletions(-) create mode 100644 Documentation/pwm.txt create mode 100644 drivers/leds/ledtrig-dim.c delete mode 100644 drivers/misc/atmel_pwm.c create mode 100644 drivers/pwm/atmel-pwm.c create mode 100644 drivers/pwm/gpio.c create mode 100644 drivers/pwm/pwm.c delete mode 100644 include/linux/pwm.h create mode 100644 include/linux/pwm/pwm-led.h create mode 100644 include/linux/pwm/pwm.h I think the following files/patches are missing from this series: drivers/Kconfig drivers/Makefile drivers/leds/Kconfig drivers/leds/Makefile drivers/pwm/Kconfig drivers/pwm/Makefile Bill, When you do repost this series please make sure that pwm support can only be selected if CONFIG_SYSFS is enabled. Your original patches did not have this. I look forward to seeing the update. BTW, setting pwm.dev = pdev-dev in my driver did fix the Ooops. Regards, Hartley -- To unsubscribe from this list: send the line unsubscribe linux-embedded in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PWM PATCH 1/5] API to consolidate PWM devices behind a common user and kernel interface
On Tuesday, February 02, 2010 12:15 AM, Bill Gatliff wrote:
Signed-off-by: Bill Gatliff b...@billgatliff.com
---
Documentation/pwm.txt | 260 +++
drivers/pwm/pwm.c | 633
+++
include/linux/pwm.h | 31 ---
include/linux/pwm/pwm.h | 170 +
4 files changed, 1063 insertions(+), 31 deletions(-)
create mode 100644 Documentation/pwm.txt
create mode 100644 drivers/pwm/pwm.c
delete mode 100644 include/linux/pwm.h
create mode 100644 include/linux/pwm/pwm.h
A couple quick comments, more after a better review.
[snip]
diff --git a/drivers/pwm/pwm.c b/drivers/pwm/pwm.c
new file mode 100644
index 000..f369384
--- /dev/null
+++ b/drivers/pwm/pwm.c
[snip]
+int pwm_unregister(struct pwm_device *pwm)
+{
[snip]
+ for (wchan = 0; wchan pwm-nchan; wchan++) {
+ if (pwm-channels[wchan].flags FLAG_REQUESTED) {
Shouldn't this be:
BIT(FLAG_REQUESTED)
[snip]
diff --git a/include/linux/pwm/pwm.h b/include/linux/pwm/pwm.h
new file mode 100644
index 000..848cd76
--- /dev/null
+++ b/include/linux/pwm/pwm.h
@@ -0,0 +1,170 @@
+#ifndef __LINUX_PWM_H
+#define __LINUX_PWM_H
Nitpick... Can you move the #ifndef/#define to after the comment?
Regards,
Hartley
--
To unsubscribe from this list: send the line unsubscribe linux-embedded in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 7/7] printk: provide a filtering macro for printk
On Tuesday, September 01, 2009 4:24 PM, Tim Bird wrote:
Marc Andre Tanner wrote:
The macro filters out printk messages based on a configurable verbosity
level (CONFIG_PRINTK_VERBOSITY).
Signed-off-by: Marc Andre Tanner m...@brain-dump.org
---
include/linux/kernel.h | 24
1 files changed, 24 insertions(+), 0 deletions(-)
diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index c2b3047..1f5d01f 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -242,6 +242,30 @@ asmlinkage int printk(const char * fmt, ...)
asmlinkage int printk_unfiltered(const char *fmt, ...)
__attribute__ ((format (printf, 1, 2))) __cold;
+#if defined(CONFIG_PRINTK_VERBOSITY) CONFIG_PRINTK_VERBOSITY 0
+/*
+ * The idea here is to wrap the actual printk function with a macro which
+ * will filter out all messages above a certain verbosity level. Because
+ * the if condition evaluates to a constant expression the compiler will be
+ * able to eliminate it and the resulting kernel image will be smaller.
+ *
+ * The check with sizeof(void*) should make sure that we don't operate on
+ * pointers, which the compiler wouldn't be able to optimize out, but only
+ * on string constants.
+ */
+
+#include linux/stringify.h
+
+#define printk(fmt, ...) ({
\
+if (sizeof(fmt) == sizeof(void *) ||
\
+(((const char *)(fmt))[0] != '' CONFIG_PRINTK_VERBOSITY = 4)
|| \
+(((const char *)(fmt))[0] == ''
\
+ ((const char *)(fmt))[1] =
*__stringify(CONFIG_PRINTK_VERBOSITY))) \
+printk((fmt), ##__VA_ARGS__);
\
+})
+
+#endif /* CONFIG_PRINTK_VERBOSITY */
+
extern struct ratelimit_state printk_ratelimit_state;
extern int printk_ratelimit(void);
extern bool printk_timed_ratelimit(unsigned long *caller_jiffies,
Some places in the kernel break the message into pieces, like so:
printk(KERN_ERR, Error: first part );
...
printk( more info for error.\n);
Technically, shouldn't the second part of the message actually be:
printk(KERN_CONT more info for error.\n);
Maybe some mechanism could be created to handle the continued message
if they have the KERN_CONT?
These parts would not be handled consistently under certain
conditions.
It would be confusing to see only part of the message,
but I don't know how often this construct is used. Maybe
another mechanism is needed to ensure that continuation
printk lines have the same log level as their start strings.
But, overall, very slick! It's nice to see a solution that doesn't
require changing all printks statements in the kernel.
I haven't looked over this patch series yet but does it work with the
pr_level macros (pr_info, pr_err, etc.)?
Regards,
Hartley
