| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Fri, 30 Dec 2011 11:35:58 +1100
From: NeilBrown <neilb@...e.de>
To: Andrew Morton <akpm@...ux-foundation.org>
Cc: Randy Dunlap <rdunlap@...otime.net>, rpurdie@...ys.net,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] leds-tca6507: allow driver to compile when GPIOLIB
is not available.
On Tue, 20 Dec 2011 14:23:31 -0800 Andrew Morton <akpm@...ux-foundation.org>
wrote:
> On Wed, 21 Dec 2011 08:03:00 +1100
> NeilBrown <neilb@...e.de> wrote:
>
> > I might just go an re-review all the code. And repeat the tests.
>
> Here's the current patch as I see it:
Thanks.
Here is a patch against that which is the result of a review and more testing
*after* making the changes :-)
NeilBrown
From a2081eb20047e9254137e37931e3b47193d8e363 Mon Sep 17 00:00:00 2001
From: NeilBrown <neilb@...e.de>
Date: Thu, 29 Dec 2011 17:37:20 +1100
Subject: [PATCH] LEDS tca6507 fixes
- various improvements to comments.
- bug in choose_times() which was counting down instead of up
- Change choice algorithm to allow change-time to be longer
if requested 'msec' is odd (i.e. use lsb of 'msec' as a flag).
- bug in set_code wasn't shifting mask properly.
- dev_dbg messages so we can monitor the choice funciton.
- make spinlock irq-safe so that gpio.set() and others can be
called from interrupt handler.
- set client_data before ->setup call back as it could set the
GPIO value immediately.
Signed-off-by: NeilBrown <neilb@...e.de>
diff --git a/drivers/leds/leds-tca6507.c b/drivers/leds/leds-tca6507.c
index b693de4..133f89f 100644
--- a/drivers/leds/leds-tca6507.c
+++ b/drivers/leds/leds-tca6507.c
@@ -24,29 +24,41 @@
* This driver does not support double-blink so 'second-off' always matches
* 'off'.
*
- * Only 16 different times can be programmed is a roughly logarithmic scale from
- * 64ms to 16320ms. Times that cannot be closely matched with these must be
+ * Only 16 different times can be programmed in a roughly logarithmic scale from
+ * 64ms to 16320ms. To be precise the possible times are:
+ * 0, 64, 128, 192, 256, 384, 512, 768,
+ * 1024, 1536, 2048, 3072, 4096, 5760, 8128, 16320
+ *
+ * Times that cannot be closely matched with these must be
* handled in software. This driver allows 12.5% error in matching.
*
* This driver does not allow rise/fall rates to be set explicitly. When trying
* to match a given 'on' or 'off' period, an appropriate pair of 'change' and
- * 'hold' times are chosen to get a close match, with the 'change' being the
- * smaller.
+ * 'hold' times are chosen to get a close match. If the target delay is even,
+ * the 'change' number will be the smaller; if odd, the 'hold' number will be
+ * the smaller.
+
+ * Choosing pairs of delays with 12.5% errors allows us to match delays in the
+ * ranges: 56-72, 112-144, 168-216, 224-27504, 28560-36720.
+ * 26% of the achievable sums can be matched by multiple pairings. For example
+ * 1536 == 1536+0, 1024+512, or 768+768. This driver will always choose the
+ * pairing with the least maximum - 768+768 in this case. Other pairings are
+ * not available.
*
* Access to the 3 levels and 2 blinks are on a first-come, first-served basis.
* Access can be shared by multiple leds if they have the same level and
* either same blink rates, or some don't blink.
* When a led changes, it relinquishes access and tries again, so it might
* lose access to hardware blink.
- * If a blink engine cannot be allocated, software blink is used. If the
- * desired brightness cannot be allocated, the closest available non-zero
+ * If a blink engine cannot be allocated, software blink is used.
+ * If the desired brightness cannot be allocated, the closest available non-zero
* brightness is used. As 'full' is always available, the worst case would be
* to have two different blink rates at '1', with Max at '2', then other leds
* will have to choose between '2' and '16'. Hopefully this is not likely.
*
- * Each bank (BANK0 and BANK1) have two usage counts - Leds using the brightness
- * and leds using the blink. It can only be reprogrammed when appropriate
- * counter is zero. The MASTER level has as single usage count.
+ * Each bank (BANK0 and BANK1) has two usage counts - LEDs using the brightness
+ * and LEDs using the blink. It can only be reprogrammed when the appropriate
+ * counter is zero. The MASTER level has a single usage count.
*
* Each Led has programmable 'on' and 'off' time as milliseconds. With each
* there is a flag saying if it was explicitly requested or defaulted.
@@ -58,8 +70,10 @@
* lists for each output: the name, default trigger, and whether the signal
* is being used as a GPiO rather than an led. 'struct led_plaform_data'
* is used for this. If 'name' is NULL, the output isn't used. If 'flags'
- * is non-zero, the output is a GPO. The 'flags' for the first GPIO should
- * be the base gpio number, or -1.
+ * is TCA6507_MAKE_CPIO, the output is a GPO.
+ * The "struct led_platform_data" can be embedded in a
+ * "struct tca6507_platform_data" which adds a 'gpio_base' for the GPiOs,
+ * and a 'setup' callback which is called once the GPiOs are available.
*
*/
@@ -74,6 +88,7 @@
/* LED select registers determine the source that drives LED outputs */
#define TCA6507_LS_LED_OFF 0x0 /* Output HI-Z (off) */
+#define TCA6507_LS_LED_OFF1 0x1 /* Output HI-Z (off) - not used */
#define TCA6507_LS_LED_PWM0 0x2 /* Output LOW with Bank0 rate */
#define TCA6507_LS_LED_PWM1 0x3 /* Output LOW with Bank1 rate */
#define TCA6507_LS_LED_ON 0x4 /* Output LOW (on) */
@@ -91,7 +106,7 @@ static int bank_source[3] = {
TCA6507_LS_LED_PWM1,
TCA6507_LS_LED_MIR,
};
-static int blink_source[3] = {
+static int blink_source[2] = {
TCA6507_LS_BLINK0,
TCA6507_LS_BLINK1,
};
@@ -99,6 +114,10 @@ static int blink_source[3] = {
/* PWM registers */
#define TCA6507_REG_CNT 11
+/*
+ * 0x00, 0x01, 0x02 encode the TCA6507_LS_* values, each output
+ * owns one bit in each register
+ */
#define TCA6507_FADE_ON 0x03
#define TCA6507_FULL_ON 0x04
#define TCA6507_FADE_OFF 0x05
@@ -132,10 +151,11 @@ static inline int TO_BRIGHT(int level)
#define NUM_LEDS 7
struct tca6507_chip {
- int reg_set; /* a '1' means the register
+ int reg_set; /* One bit per register where
+ * a '1' means the register
* should be written */
u8 reg_file[TCA6507_REG_CNT];
- /* Bank 0 is Master Intensity */
+ /* Bank 2 is Master Intensity and doesn't use times */
struct bank {
int level;
int ontime, offtime;
@@ -153,7 +173,7 @@ struct tca6507_chip {
int ontime, offtime;
int on_dflt, off_dflt;
int bank; /* Bank used, or -1 */
- int blink; /* 1 if we are hardware-blinking */
+ int blink; /* Set if hardware-blinking */
} leds[NUM_LEDS];
#ifdef CONFIG_GPIOLIB
struct gpio_chip gpio;
@@ -172,9 +192,13 @@ static int choose_times(int msec, int *c1p, int *c2p)
{
/*
* Choose two timecodes which add to 'msec' as near as possible.
- * The first returned should be the larger and is the 'on' of 'off'
- * time. The second will be used as a 'fade-on' or 'fade-off' time.
- * If cannot get within 1/8, fail.
+ * The first returned is the 'on' or 'off' time. The second is to be
+ * used as a 'fade-on' or 'fade-off' time. If 'msec' is even,
+ * the first will not be smaller than the second. If 'msec' is odd,
+ * the first will not be larger than the second.
+ * If we cannot get a sum within 1/8 of 'msec' fail with -EINVAL,
+ * otherwise return the sum that was achieved, plus 1 if the first is
+ * smaller.
* If two possibilities are equally good (e.g. 512+0, 256+256), choose
* the first pair so there is more change-time visible (i.e. it is
* softer).
@@ -193,7 +217,7 @@ static int choose_times(int msec, int *c1p, int *c2p)
continue;
if (t > tmax)
break;
- for (c2 = 0; c2 <= c1; c2--) {
+ for (c2 = 0; c2 <= c1; c2++) {
int tt = t + time_codes[c2];
int d;
if (tt < tmin)
@@ -209,11 +233,22 @@ static int choose_times(int msec, int *c1p, int *c2p)
*c2p = c2;
diff = d;
if (d == 0)
- return 0;
+ return msec;
}
}
- if (diff < 65536)
- return 0;
+ if (diff < 65536) {
+ int actual;
+ if (msec & 1) {
+ c1 = *c2p;
+ *c2p = *c1p;
+ *c1p = c1;
+ }
+ actual = time_codes[*c1p] + time_codes[*c2p];
+ if (*c1p < *c2p)
+ return actual + 1;
+ else
+ return actual;
+ }
/* No close match */
return -EINVAL;
}
@@ -244,10 +279,13 @@ static void set_select(struct tca6507_chip *tca, int led, int val)
*/
static void set_code(struct tca6507_chip *tca, int reg, int bank, int new)
{
- int mask = (0xF << bank);
- int n = tca->reg_file[reg] & ~mask;
- if (bank)
+ int mask = 0xF;
+ int n;
+ if (bank) {
+ mask <<= 4;
new <<= 4;
+ }
+ n = tca->reg_file[reg] & ~mask;
n |= new;
if (tca->reg_file[reg] != n) {
tca->reg_file[reg] = n;
@@ -274,17 +312,24 @@ static void set_level(struct tca6507_chip *tca, int bank, int level)
static void set_times(struct tca6507_chip *tca, int bank)
{
int c1, c2;
+ int result;
- choose_times(tca->bank[bank].ontime, &c1, &c2);
+ result = choose_times(tca->bank[bank].ontime, &c1, &c2);
+ dev_dbg(&tca->client->dev,
+ "Chose on times %d(%d) %d(%d) for %dms\n", c1, time_codes[c1],
+ c2, time_codes[c2], tca->bank[bank].ontime);
set_code(tca, TCA6507_FADE_ON, bank, c2);
set_code(tca, TCA6507_FULL_ON, bank, c1);
- tca->bank[bank].ontime = time_codes[c1] + time_codes[c2];
+ tca->bank[bank].ontime = result;
- choose_times(tca->bank[bank].offtime, &c1, &c2);
+ result = choose_times(tca->bank[bank].offtime, &c1, &c2);
+ dev_dbg(&tca->client->dev,
+ "Chose off times %d(%d) %d(%d) for %dms\n", c1, time_codes[c1],
+ c2, time_codes[c2], tca->bank[bank].offtime);
set_code(tca, TCA6507_FADE_OFF, bank, c2);
set_code(tca, TCA6507_FIRST_OFF, bank, c1);
set_code(tca, TCA6507_SECOND_OFF, bank, c1);
- tca->bank[bank].offtime = time_codes[c1] + time_codes[c2];
+ tca->bank[bank].offtime = result;
set_code(tca, TCA6507_INITIALIZE, bank, INIT_CODE);
}
@@ -300,11 +345,11 @@ static void tca6507_work(struct work_struct *work)
u8 file[TCA6507_REG_CNT];
int r;
- spin_lock(&tca->lock);
+ spin_lock_irq(&tca->lock);
set = tca->reg_set;
memcpy(file, tca->reg_file, TCA6507_REG_CNT);
tca->reg_set = 0;
- spin_unlock(&tca->lock);
+ spin_unlock_irq(&tca->lock);
for (r = 0; r < TCA6507_REG_CNT; r++)
if (set & (1<<r))
@@ -327,7 +372,7 @@ static void led_release(struct tca6507_led *led)
static int led_prepare(struct tca6507_led *led)
{
- /* Assign this led to a bank. configuring that bank if necessary */
+ /* Assign this led to a bank, configuring that bank if necessary. */
int level = TO_LEVEL(led->led_cdev.brightness);
struct tca6507_chip *tca = led->chip;
int c1, c2;
@@ -386,7 +431,11 @@ static int led_prepare(struct tca6507_led *led)
return 0;
}
- /* We have on/off time so we need to try to allocate a timing bank. */
+ /*
+ * We have on/off time so we need to try to allocate a timing bank.
+ * First check if times are compatible with hardware and give up if
+ * not.
+ */
if (choose_times(led->ontime, &c1, &c2) < 0)
return -EINVAL;
if (choose_times(led->offtime, &c1, &c2) < 0)
@@ -466,8 +515,9 @@ static int led_assign(struct tca6507_led *led)
{
struct tca6507_chip *tca = led->chip;
int err;
+ unsigned long flags;
- spin_lock(&tca->lock);
+ spin_lock_irqsave(&tca->lock, flags);
led_release(led);
err = led_prepare(led);
if (err) {
@@ -479,7 +529,7 @@ static int led_assign(struct tca6507_led *led)
led->offtime = 0;
led_prepare(led);
}
- spin_unlock(&tca->lock);
+ spin_unlock_irqrestore(&tca->lock, flags);
if (tca->reg_set)
schedule_work(&tca->work);
@@ -539,15 +589,16 @@ static void tca6507_gpio_set_value(struct gpio_chip *gc,
unsigned offset, int val)
{
struct tca6507_chip *tca = container_of(gc, struct tca6507_chip, gpio);
+ unsigned long flags;
- spin_lock(&tca->lock);
+ spin_lock_irqsave(&tca->lock, flags);
/*
* 'OFF' is floating high, and 'ON' is pulled down, so it has the
* inverse sense of 'val'.
*/
set_select(tca, tca->gpio_map[offset],
val ? TCA6507_LS_LED_OFF : TCA6507_LS_LED_ON);
- spin_unlock(&tca->lock);
+ spin_unlock_irqrestore(&tca->lock, flags);
if (tca->reg_set)
schedule_work(&tca->work);
}
@@ -558,6 +609,7 @@ static int tca6507_gpio_direction_output(struct gpio_chip *gc,
tca6507_gpio_set_value(gc, offset, val);
return 0;
}
+
static int tca6507_probe_gpios(struct i2c_client *client,
struct tca6507_chip *tca,
struct tca6507_platform_data *pdata)
@@ -643,6 +695,7 @@ static int __devinit tca6507_probe(struct i2c_client *client,
tca->client = client;
INIT_WORK(&tca->work, tca6507_work);
spin_lock_init(&tca->lock);
+ i2c_set_clientdata(client, tca);
for (i = 0; i < NUM_LEDS; i++) {
struct tca6507_led *l = tca->leds + i;
@@ -665,7 +718,6 @@ static int __devinit tca6507_probe(struct i2c_client *client,
err = tca6507_probe_gpios(client, tca, pdata);
if (err)
goto exit;
- i2c_set_clientdata(client, tca);
/* set all registers to known state - zero */
tca->reg_set = 0x7f;
schedule_work(&tca->work);
@@ -675,6 +727,8 @@ exit:
while (i--)
if (tca->leds[i].led_cdev.name)
led_classdev_unregister(&tca->leds[i].led_cdev);
+ cancel_work_sync(&tca->work);
+ i2c_set_clientdata(client, NULL);
kfree(tca);
return err;
}
@@ -691,8 +745,8 @@ static int __devexit tca6507_remove(struct i2c_client *client)
}
tca6507_remove_gpio(tca);
cancel_work_sync(&tca->work);
- kfree(tca);
i2c_set_clientdata(client, NULL);
+ kfree(tca);
return 0;
}
Download attachment "signature.asc" of type "application/pgp-signature" (829 bytes)
Powered by blists - more mailing lists