Since IORESOURCE_IO is used for PCI devices, it doesn't fit on
88PM860x PMIC device that lies on I2C bus. So use IORESOURCE_MEM
instead.
Avoid to use IORESOURCE_IO in mfd core & backlight driver.
Use IORESOURCE_MEM instead.
Signed-off-by: Haojian Zhuang <[email protected]>
---
drivers/mfd/88pm860x-core.c | 78 ++++++++++++----------
drivers/video/backlight/88pm860x_bl.c | 114 +++++++++++++--------------------
include/linux/mfd/88pm860x.h | 8 ---
3 files changed, 89 insertions(+), 111 deletions(-)
diff --git a/drivers/mfd/88pm860x-core.c b/drivers/mfd/88pm860x-core.c
index d09918c..ee2445c 100644
--- a/drivers/mfd/88pm860x-core.c
+++ b/drivers/mfd/88pm860x-core.c
@@ -21,10 +21,20 @@
#define INT_STATUS_NUM 3
-static struct resource bk_resources[] __devinitdata = {
- {PM8606_BACKLIGHT1, PM8606_BACKLIGHT1, "backlight-0", IORESOURCE_IO,},
- {PM8606_BACKLIGHT2, PM8606_BACKLIGHT2, "backlight-1", IORESOURCE_IO,},
- {PM8606_BACKLIGHT3, PM8606_BACKLIGHT3, "backlight-2", IORESOURCE_IO,},
+static struct resource bk0_resources[] __devinitdata = {
+ {2, 2, "duty cycle", IORESOURCE_MEM, },
+ {3, 3, "always on", IORESOURCE_MEM, },
+ {3, 3, "current", IORESOURCE_MEM, },
+};
+static struct resource bk1_resources[] __devinitdata = {
+ {4, 4, "duty cycle", IORESOURCE_MEM, },
+ {5, 5, "always on", IORESOURCE_MEM, },
+ {5, 5, "current", IORESOURCE_MEM, },
+};
+static struct resource bk2_resources[] __devinitdata = {
+ {6, 6, "duty cycle", IORESOURCE_MEM, },
+ {7, 7, "always on", IORESOURCE_MEM, },
+ {5, 5, "current", IORESOURCE_MEM, },
};
static struct resource led_resources[] __devinitdata = {
@@ -99,9 +109,22 @@ static struct resource rtc_resources[] __devinitdata = {
};
static struct mfd_cell bk_devs[] = {
- {"88pm860x-backlight", 0,},
- {"88pm860x-backlight", 1,},
- {"88pm860x-backlight", 2,},
+ {
+ .name = "88pm860x-backlight",
+ .id = 0,
+ .num_resources = ARRAY_SIZE(bk0_resources),
+ .resources = bk0_resources,
+ }, {
+ .name = "88pm860x-backlight",
+ .id = 1,
+ .num_resources = ARRAY_SIZE(bk1_resources),
+ .resources = bk1_resources,
+ }, {
+ .name = "88pm860x-backlight",
+ .id = 2,
+ .num_resources = ARRAY_SIZE(bk2_resources),
+ .resources = bk2_resources,
+ },
};
static struct mfd_cell led_devs[] = {
@@ -615,36 +638,21 @@ static void __devinit device_osc_init(struct i2c_client *i2c)
static void __devinit device_bk_init(struct pm860x_chip *chip,
struct pm860x_platform_data *pdata)
{
- int ret;
- int i, j, id;
-
- if ((pdata == NULL) || (pdata->backlight == NULL))
- return;
-
- if (pdata->num_backlights > ARRAY_SIZE(bk_devs))
- pdata->num_backlights = ARRAY_SIZE(bk_devs);
-
- for (i = 0; i < pdata->num_backlights; i++) {
- bk_devs[i].platform_data = &pdata->backlight[i];
- bk_devs[i].pdata_size = sizeof(struct pm860x_backlight_pdata);
-
- for (j = 0; j < ARRAY_SIZE(bk_devs); j++) {
- id = bk_resources[j].start;
- if (pdata->backlight[i].flags != id)
- continue;
-
- bk_devs[i].num_resources = 1;
- bk_devs[i].resources = &bk_resources[j];
- ret = mfd_add_devices(chip->dev, 0,
- &bk_devs[i], 1,
- &bk_resources[j], 0);
- if (ret < 0) {
- dev_err(chip->dev, "Failed to add "
- "backlight subdev\n");
- return;
- }
+ int ret, i;
+
+ if (pdata && pdata->backlight) {
+ if (pdata->num_backlights > ARRAY_SIZE(bk_devs))
+ pdata->num_backlights = ARRAY_SIZE(bk_devs);
+ for (i = 0; i < pdata->num_backlights; i++) {
+ bk_devs[i].platform_data = &pdata->backlight[i];
+ bk_devs[i].pdata_size =
+ sizeof(struct pm860x_backlight_pdata);
}
}
+ ret = mfd_add_devices(chip->dev, 0, bk_devs,
+ ARRAY_SIZE(bk_devs), NULL, 0);
+ if (ret < 0)
+ dev_err(chip->dev, "Failed to add backlight subdev\n");
}
static void __devinit device_led_init(struct pm860x_chip *chip,
diff --git a/drivers/video/backlight/88pm860x_bl.c b/drivers/video/backlight/88pm860x_bl.c
index f75da87..e48d846 100644
--- a/drivers/video/backlight/88pm860x_bl.c
+++ b/drivers/video/backlight/88pm860x_bl.c
@@ -31,57 +31,26 @@ struct pm860x_backlight_data {
int port;
int pwm;
int iset;
+ int reg_duty_cycle;
+ int reg_always_on;
+ int reg_current;
};
-static inline int wled_a(int port)
-{
- int ret;
-
- ret = ((port - PM8606_BACKLIGHT1) << 1) + 2;
- return ret;
-}
-
-static inline int wled_b(int port)
-{
- int ret;
-
- ret = ((port - PM8606_BACKLIGHT1) << 1) + 3;
- return ret;
-}
-
-/* WLED2 & WLED3 share the same IDC */
-static inline int wled_idc(int port)
-{
- int ret;
-
- switch (port) {
- case PM8606_BACKLIGHT1:
- case PM8606_BACKLIGHT2:
- ret = ((port - PM8606_BACKLIGHT1) << 1) + 3;
- break;
- case PM8606_BACKLIGHT3:
- default:
- ret = ((port - PM8606_BACKLIGHT2) << 1) + 3;
- break;
- }
- return ret;
-}
-
static int backlight_power_set(struct pm860x_chip *chip, int port,
int on)
{
int ret = -EINVAL;
switch (port) {
- case PM8606_BACKLIGHT1:
+ case 0:
ret = on ? pm8606_osc_enable(chip, WLED1_DUTY) :
pm8606_osc_disable(chip, WLED1_DUTY);
break;
- case PM8606_BACKLIGHT2:
+ case 1:
ret = on ? pm8606_osc_enable(chip, WLED2_DUTY) :
pm8606_osc_disable(chip, WLED2_DUTY);
break;
- case PM8606_BACKLIGHT3:
+ case 2:
ret = on ? pm8606_osc_enable(chip, WLED3_DUTY) :
pm8606_osc_disable(chip, WLED3_DUTY);
break;
@@ -104,13 +73,13 @@ static int pm860x_backlight_set(struct backlight_device *bl, int brightness)
if (brightness)
backlight_power_set(chip, data->port, 1);
- ret = pm860x_reg_write(data->i2c, wled_a(data->port), value);
+ ret = pm860x_reg_write(data->i2c, data->reg_duty_cycle, value);
if (ret < 0)
goto out;
if ((data->current_brightness == 0) && brightness) {
if (data->iset) {
- ret = pm860x_set_bits(data->i2c, wled_idc(data->port),
+ ret = pm860x_set_bits(data->i2c, data->reg_current,
CURRENT_BITMASK, data->iset);
if (ret < 0)
goto out;
@@ -123,17 +92,17 @@ static int pm860x_backlight_set(struct backlight_device *bl, int brightness)
}
if (brightness == MAX_BRIGHTNESS) {
/* set WLED_ON bit as 100% */
- ret = pm860x_set_bits(data->i2c, wled_b(data->port),
+ ret = pm860x_set_bits(data->i2c, data->reg_always_on,
PM8606_WLED_ON, PM8606_WLED_ON);
}
} else {
if (brightness == MAX_BRIGHTNESS) {
/* set WLED_ON bit as 100% */
- ret = pm860x_set_bits(data->i2c, wled_b(data->port),
+ ret = pm860x_set_bits(data->i2c, data->reg_always_on,
PM8606_WLED_ON, PM8606_WLED_ON);
} else {
/* clear WLED_ON bit since it's not 100% */
- ret = pm860x_set_bits(data->i2c, wled_b(data->port),
+ ret = pm860x_set_bits(data->i2c, data->reg_always_on,
PM8606_WLED_ON, 0);
}
}
@@ -174,7 +143,7 @@ static int pm860x_backlight_get_brightness(struct backlight_device *bl)
struct pm860x_chip *chip = data->chip;
int ret;
- ret = pm860x_reg_read(data->i2c, wled_a(data->port));
+ ret = pm860x_reg_read(data->i2c, data->reg_duty_cycle);
if (ret < 0)
goto out;
data->current_brightness = ret;
@@ -193,43 +162,50 @@ static const struct backlight_ops pm860x_backlight_ops = {
static int pm860x_backlight_probe(struct platform_device *pdev)
{
struct pm860x_chip *chip = dev_get_drvdata(pdev->dev.parent);
- struct pm860x_backlight_pdata *pdata = NULL;
+ struct pm860x_backlight_pdata *pdata = pdev->dev.platform_data;
struct pm860x_backlight_data *data;
struct backlight_device *bl;
struct resource *res;
struct backlight_properties props;
char name[MFD_NAME_SIZE];
- int ret;
-
- res = platform_get_resource(pdev, IORESOURCE_IO, 0);
- if (res == NULL) {
- dev_err(&pdev->dev, "No I/O resource!\n");
- return -EINVAL;
- }
-
- pdata = pdev->dev.platform_data;
- if (pdata == NULL) {
- dev_err(&pdev->dev, "platform data isn't assigned to "
- "backlight\n");
- return -EINVAL;
- }
+ int ret = 0;
data = devm_kzalloc(&pdev->dev, sizeof(struct pm860x_backlight_data),
GFP_KERNEL);
if (data == NULL)
return -ENOMEM;
- strncpy(name, res->name, MFD_NAME_SIZE);
+ res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "duty cycle");
+ if (!res) {
+ dev_err(&pdev->dev, "No memory resource for duty cycle\n");
+ ret = -ENXIO;
+ goto out;
+ }
+ data->reg_duty_cycle = res->start;
+ res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "always on");
+ if (!res) {
+ dev_err(&pdev->dev, "No memory resorce for always on\n");
+ ret = -ENXIO;
+ goto out;
+ }
+ data->reg_always_on = res->start;
+ res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "current");
+ if (!res) {
+ dev_err(&pdev->dev, "No memory resource for current\n");
+ ret = -ENXIO;
+ goto out;
+ }
+ data->reg_current = res->start;
+
+ memset(name, 0, MFD_NAME_SIZE);
+ sprintf(name, "backlight-%d", pdev->id);
+ data->port = pdev->id;
data->chip = chip;
data->i2c = (chip->id == CHIP_PM8606) ? chip->client \
: chip->companion;
data->current_brightness = MAX_BRIGHTNESS;
- data->pwm = pdata->pwm;
- data->iset = pdata->iset;
- data->port = pdata->flags;
- if (data->port < 0) {
- dev_err(&pdev->dev, "wrong platform data is assigned");
- kfree(data);
- return -EINVAL;
+ if (pdata) {
+ data->pwm = pdata->pwm;
+ data->iset = pdata->iset;
}
memset(&props, 0, sizeof(struct backlight_properties));
@@ -248,12 +224,14 @@ static int pm860x_backlight_probe(struct platform_device *pdev)
/* read current backlight */
ret = pm860x_backlight_get_brightness(bl);
if (ret < 0)
- goto out;
+ goto out_brt;
backlight_update_status(bl);
return 0;
-out:
+out_brt:
backlight_device_unregister(bl);
+out:
+ devm_kfree(&pdev->dev, data);
return ret;
}
diff --git a/include/linux/mfd/88pm860x.h b/include/linux/mfd/88pm860x.h
index 7b24943..b7e656d 100644
--- a/include/linux/mfd/88pm860x.h
+++ b/include/linux/mfd/88pm860x.h
@@ -35,12 +35,6 @@ enum {
};
enum {
- PM8606_BACKLIGHT1 = 0,
- PM8606_BACKLIGHT2,
- PM8606_BACKLIGHT3,
-};
-
-enum {
PM8606_LED1_RED = 0,
PM8606_LED1_GREEN,
PM8606_LED1_BLUE,
@@ -340,10 +334,8 @@ enum {
};
struct pm860x_backlight_pdata {
- int id;
int pwm;
int iset;
- unsigned long flags;
};
struct pm860x_led_pdata {
--
1.7.9.5
Avoid to use IORESOURCE_IO in 88pm860x leds drivers. Use
IORESOURCE_MEM instead.
Signed-off-by: Haojian Zhuang <[email protected]>
---
drivers/leds/leds-88pm860x.c | 176 +++++++++++++++++-------------------------
drivers/mfd/88pm860x-core.c | 114 +++++++++++++++++----------
include/linux/mfd/88pm860x.h | 12 ---
3 files changed, 143 insertions(+), 159 deletions(-)
diff --git a/drivers/leds/leds-88pm860x.c b/drivers/leds/leds-88pm860x.c
index 61897cf..701d006 100644
--- a/drivers/leds/leds-88pm860x.c
+++ b/drivers/leds/leds-88pm860x.c
@@ -20,18 +20,12 @@
#include <linux/mfd/88pm860x.h>
#include <linux/module.h>
-#define LED_PWM_SHIFT (3)
#define LED_PWM_MASK (0x1F)
#define LED_CURRENT_MASK (0x07 << 5)
-#define LED_BLINK_ON_MASK (0x07)
#define LED_BLINK_MASK (0x7F)
-#define LED_BLINK_ON(x) ((x & 0x7) * 66 + 66)
-#define LED_BLINK_ON_MIN LED_BLINK_ON(0)
-#define LED_BLINK_ON_MAX LED_BLINK_ON(0x7)
#define LED_ON_CONTINUOUS (0x0F << 3)
-#define LED_TO_ON(x) ((x - 66) / 66)
#define LED1_BLINK_EN (1 << 1)
#define LED2_BLINK_EN (1 << 2)
@@ -49,85 +43,25 @@ struct pm860x_led {
unsigned char brightness;
unsigned char current_brightness;
- int blink_data;
- int blink_time;
- int blink_on;
- int blink_off;
+ int reg_control;
+ int reg_blink;
+ int blink_mask;
};
-/* return offset of color register */
-static inline int __led_off(int port)
-{
- int ret = -EINVAL;
-
- switch (port) {
- case PM8606_LED1_RED:
- case PM8606_LED1_GREEN:
- case PM8606_LED1_BLUE:
- ret = port - PM8606_LED1_RED + PM8606_RGB1B;
- break;
- case PM8606_LED2_RED:
- case PM8606_LED2_GREEN:
- case PM8606_LED2_BLUE:
- ret = port - PM8606_LED2_RED + PM8606_RGB2B;
- break;
- }
- return ret;
-}
-
-/* return offset of blink register */
-static inline int __blink_off(int port)
-{
- int ret = -EINVAL;
-
- switch (port) {
- case PM8606_LED1_RED:
- case PM8606_LED1_GREEN:
- case PM8606_LED1_BLUE:
- ret = PM8606_RGB1A;
- break;
- case PM8606_LED2_RED:
- case PM8606_LED2_GREEN:
- case PM8606_LED2_BLUE:
- ret = PM8606_RGB2A;
- break;
- }
- return ret;
-}
-
-static inline int __blink_ctl_mask(int port)
-{
- int ret = -EINVAL;
-
- switch (port) {
- case PM8606_LED1_RED:
- case PM8606_LED1_GREEN:
- case PM8606_LED1_BLUE:
- ret = LED1_BLINK_EN;
- break;
- case PM8606_LED2_RED:
- case PM8606_LED2_GREEN:
- case PM8606_LED2_BLUE:
- ret = LED2_BLINK_EN;
- break;
- }
- return ret;
-}
-
static int led_power_set(struct pm860x_chip *chip, int port, int on)
{
int ret = -EINVAL;
switch (port) {
- case PM8606_LED1_RED:
- case PM8606_LED1_GREEN:
- case PM8606_LED1_BLUE:
+ case 0:
+ case 1:
+ case 2:
ret = on ? pm8606_osc_enable(chip, RGB1_ENABLE) :
pm8606_osc_disable(chip, RGB1_ENABLE);
break;
- case PM8606_LED2_RED:
- case PM8606_LED2_GREEN:
- case PM8606_LED2_BLUE:
+ case 3:
+ case 4:
+ case 5:
ret = on ? pm8606_osc_enable(chip, RGB2_ENABLE) :
pm8606_osc_disable(chip, RGB2_ENABLE);
break;
@@ -141,7 +75,7 @@ static void pm860x_led_work(struct work_struct *work)
struct pm860x_led *led;
struct pm860x_chip *chip;
unsigned char buf[3];
- int mask, ret;
+ int ret;
led = container_of(work, struct pm860x_led, work);
chip = led->chip;
@@ -149,34 +83,34 @@ static void pm860x_led_work(struct work_struct *work)
if ((led->current_brightness == 0) && led->brightness) {
led_power_set(chip, led->port, 1);
if (led->iset) {
- pm860x_set_bits(led->i2c, __led_off(led->port),
+ pm860x_set_bits(led->i2c, led->reg_control,
LED_CURRENT_MASK, led->iset);
}
- pm860x_set_bits(led->i2c, __blink_off(led->port),
+ pm860x_set_bits(led->i2c, led->reg_blink,
LED_BLINK_MASK, LED_ON_CONTINUOUS);
- mask = __blink_ctl_mask(led->port);
- pm860x_set_bits(led->i2c, PM8606_WLED3B, mask, mask);
+ pm860x_set_bits(led->i2c, PM8606_WLED3B, led->blink_mask,
+ led->blink_mask);
}
- pm860x_set_bits(led->i2c, __led_off(led->port), LED_PWM_MASK,
+ pm860x_set_bits(led->i2c, led->reg_control, LED_PWM_MASK,
led->brightness);
if (led->brightness == 0) {
- pm860x_bulk_read(led->i2c, __led_off(led->port), 3, buf);
+ pm860x_bulk_read(led->i2c, led->reg_control, 3, buf);
ret = buf[0] & LED_PWM_MASK;
ret |= buf[1] & LED_PWM_MASK;
ret |= buf[2] & LED_PWM_MASK;
if (ret == 0) {
/* unset current since no led is lighting */
- pm860x_set_bits(led->i2c, __led_off(led->port),
+ pm860x_set_bits(led->i2c, led->reg_control,
LED_CURRENT_MASK, 0);
- mask = __blink_ctl_mask(led->port);
- pm860x_set_bits(led->i2c, PM8606_WLED3B, mask, 0);
+ pm860x_set_bits(led->i2c, PM8606_WLED3B,
+ led->blink_mask, 0);
led_power_set(chip, led->port, 0);
}
}
led->current_brightness = led->brightness;
dev_dbg(chip->dev, "Update LED. (reg:%d, brightness:%d)\n",
- __led_off(led->port), led->brightness);
+ led->reg_control, led->brightness);
mutex_unlock(&led->lock);
}
@@ -192,36 +126,61 @@ static void pm860x_led_set(struct led_classdev *cdev,
static int pm860x_led_probe(struct platform_device *pdev)
{
struct pm860x_chip *chip = dev_get_drvdata(pdev->dev.parent);
- struct pm860x_led_pdata *pdata;
+ struct pm860x_led_pdata *pdata = pdev->dev.platform_data;
struct pm860x_led *data;
struct resource *res;
- int ret;
-
- res = platform_get_resource(pdev, IORESOURCE_IO, 0);
- if (res == NULL) {
- dev_err(&pdev->dev, "No I/O resource!\n");
- return -EINVAL;
- }
-
- pdata = pdev->dev.platform_data;
- if (pdata == NULL) {
- dev_err(&pdev->dev, "No platform data!\n");
- return -EINVAL;
- }
+ int ret = 0;
data = devm_kzalloc(&pdev->dev, sizeof(struct pm860x_led), GFP_KERNEL);
if (data == NULL)
return -ENOMEM;
- strncpy(data->name, res->name, MFD_NAME_SIZE - 1);
+ res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "control");
+ if (!res) {
+ dev_err(&pdev->dev, "No memory resource for control\n");
+ ret = -ENXIO;
+ goto out;
+ }
+ data->reg_control = res->start;
+ res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "blink");
+ if (!res) {
+ dev_err(&pdev->dev, "No memory resource for blink\n");
+ ret = -ENXIO;
+ goto out;
+ }
+ data->reg_blink = res->start;
+ memset(data->name, 0, MFD_NAME_SIZE);
+ switch (pdev->id) {
+ case 0:
+ data->blink_mask = LED1_BLINK_EN;
+ sprintf(data->name, "led0-red");
+ break;
+ case 1:
+ data->blink_mask = LED1_BLINK_EN;
+ sprintf(data->name, "led0-green");
+ break;
+ case 2:
+ data->blink_mask = LED1_BLINK_EN;
+ sprintf(data->name, "led0-blue");
+ break;
+ case 3:
+ data->blink_mask = LED2_BLINK_EN;
+ sprintf(data->name, "led1-red");
+ break;
+ case 4:
+ data->blink_mask = LED2_BLINK_EN;
+ sprintf(data->name, "led1-green");
+ break;
+ case 5:
+ data->blink_mask = LED2_BLINK_EN;
+ sprintf(data->name, "led1-blue");
+ break;
+ }
dev_set_drvdata(&pdev->dev, data);
data->chip = chip;
data->i2c = (chip->id == CHIP_PM8606) ? chip->client : chip->companion;
- data->iset = pdata->iset;
- data->port = pdata->flags;
- if (data->port < 0) {
- dev_err(&pdev->dev, "check device failed\n");
- return -EINVAL;
- }
+ data->port = pdev->id;
+ if (pdata && pdata->iset)
+ data->iset = pdata->iset;
data->current_brightness = 0;
data->cdev.name = data->name;
@@ -236,6 +195,9 @@ static int pm860x_led_probe(struct platform_device *pdev)
}
pm860x_led_set(&data->cdev, 0);
return 0;
+out:
+ devm_kfree(&pdev->dev, data);
+ return ret;
}
static int pm860x_led_remove(struct platform_device *pdev)
diff --git a/drivers/mfd/88pm860x-core.c b/drivers/mfd/88pm860x-core.c
index ee2445c..2934ea3 100644
--- a/drivers/mfd/88pm860x-core.c
+++ b/drivers/mfd/88pm860x-core.c
@@ -37,13 +37,35 @@ static struct resource bk2_resources[] __devinitdata = {
{5, 5, "current", IORESOURCE_MEM, },
};
-static struct resource led_resources[] __devinitdata = {
- {PM8606_LED1_RED, PM8606_LED1_RED, "led0-red", IORESOURCE_IO,},
- {PM8606_LED1_GREEN, PM8606_LED1_GREEN, "led0-green", IORESOURCE_IO,},
- {PM8606_LED1_BLUE, PM8606_LED1_BLUE, "led0-blue", IORESOURCE_IO,},
- {PM8606_LED2_RED, PM8606_LED2_RED, "led1-red", IORESOURCE_IO,},
- {PM8606_LED2_GREEN, PM8606_LED2_GREEN, "led1-green", IORESOURCE_IO,},
- {PM8606_LED2_BLUE, PM8606_LED2_BLUE, "led1-blue", IORESOURCE_IO,},
+static struct resource led0_resources[] __devinitdata = {
+ /* RGB1 Red LED */
+ {0xd, 0xd, "control", IORESOURCE_MEM, },
+ {0xc, 0xc, "blink", IORESOURCE_MEM, },
+};
+static struct resource led1_resources[] __devinitdata = {
+ /* RGB1 Green LED */
+ {0xe, 0xe, "control", IORESOURCE_MEM, },
+ {0xc, 0xc, "blink", IORESOURCE_MEM, },
+};
+static struct resource led2_resources[] __devinitdata = {
+ /* RGB1 Blue LED */
+ {0xf, 0xf, "control", IORESOURCE_MEM, },
+ {0xc, 0xc, "blink", IORESOURCE_MEM, },
+};
+static struct resource led3_resources[] __devinitdata = {
+ /* RGB2 Red LED */
+ {0x9, 0x9, "control", IORESOURCE_MEM, },
+ {0x8, 0x8, "blink", IORESOURCE_MEM, },
+};
+static struct resource led4_resources[] __devinitdata = {
+ /* RGB2 Green LED */
+ {0xa, 0xa, "control", IORESOURCE_MEM, },
+ {0x8, 0x8, "blink", IORESOURCE_MEM, },
+};
+static struct resource led5_resources[] __devinitdata = {
+ /* RGB2 Blue LED */
+ {0xb, 0xb, "control", IORESOURCE_MEM, },
+ {0x8, 0x8, "blink", IORESOURCE_MEM, },
};
static struct resource regulator_resources[] __devinitdata = {
@@ -128,12 +150,37 @@ static struct mfd_cell bk_devs[] = {
};
static struct mfd_cell led_devs[] = {
- {"88pm860x-led", 0,},
- {"88pm860x-led", 1,},
- {"88pm860x-led", 2,},
- {"88pm860x-led", 3,},
- {"88pm860x-led", 4,},
- {"88pm860x-led", 5,},
+ {
+ .name = "88pm860x-led",
+ .id = 0,
+ .num_resources = ARRAY_SIZE(led0_resources),
+ .resources = led0_resources,
+ }, {
+ .name = "88pm860x-led",
+ .id = 1,
+ .num_resources = ARRAY_SIZE(led1_resources),
+ .resources = led1_resources,
+ }, {
+ .name = "88pm860x-led",
+ .id = 2,
+ .num_resources = ARRAY_SIZE(led2_resources),
+ .resources = led2_resources,
+ }, {
+ .name = "88pm860x-led",
+ .id = 3,
+ .num_resources = ARRAY_SIZE(led3_resources),
+ .resources = led3_resources,
+ }, {
+ .name = "88pm860x-led",
+ .id = 4,
+ .num_resources = ARRAY_SIZE(led4_resources),
+ .resources = led4_resources,
+ }, {
+ .name = "88pm860x-led",
+ .id = 5,
+ .num_resources = ARRAY_SIZE(led5_resources),
+ .resources = led5_resources,
+ },
};
static struct mfd_cell regulator_devs[] = {
@@ -658,36 +705,23 @@ static void __devinit device_bk_init(struct pm860x_chip *chip,
static void __devinit device_led_init(struct pm860x_chip *chip,
struct pm860x_platform_data *pdata)
{
- int ret;
- int i, j, id;
-
- if ((pdata == NULL) || (pdata->led == NULL))
- return;
+ int ret, i;
- if (pdata->num_leds > ARRAY_SIZE(led_devs))
- pdata->num_leds = ARRAY_SIZE(led_devs);
-
- for (i = 0; i < pdata->num_leds; i++) {
- led_devs[i].platform_data = &pdata->led[i];
- led_devs[i].pdata_size = sizeof(struct pm860x_led_pdata);
-
- for (j = 0; j < ARRAY_SIZE(led_devs); j++) {
- id = led_resources[j].start;
- if (pdata->led[i].flags != id)
- continue;
-
- led_devs[i].num_resources = 1;
- led_devs[i].resources = &led_resources[j],
- ret = mfd_add_devices(chip->dev, 0,
- &led_devs[i], 1,
- &led_resources[j], 0);
- if (ret < 0) {
- dev_err(chip->dev, "Failed to add "
- "led subdev\n");
- return;
- }
+ if (pdata && pdata->led) {
+ if (pdata->num_leds > ARRAY_SIZE(led_devs))
+ pdata->num_leds = ARRAY_SIZE(led_devs);
+ for (i = 0; i < pdata->num_leds; i++) {
+ led_devs[i].platform_data = &pdata->led[i];
+ led_devs[i].pdata_size =
+ sizeof(struct pm860x_led_pdata);
}
}
+ ret = mfd_add_devices(chip->dev, 0, led_devs,
+ ARRAY_SIZE(led_devs), NULL, 0);
+ if (ret < 0) {
+ dev_err(chip->dev, "Failed to add led subdev\n");
+ return;
+ }
}
static void __devinit device_regulator_init(struct pm860x_chip *chip,
diff --git a/include/linux/mfd/88pm860x.h b/include/linux/mfd/88pm860x.h
index b7e656d..2d042f9 100644
--- a/include/linux/mfd/88pm860x.h
+++ b/include/linux/mfd/88pm860x.h
@@ -34,16 +34,6 @@ enum {
PM8606_ID_MAX,
};
-enum {
- PM8606_LED1_RED = 0,
- PM8606_LED1_GREEN,
- PM8606_LED1_BLUE,
- PM8606_LED2_RED,
- PM8606_LED2_GREEN,
- PM8606_LED2_BLUE,
- PM8607_LED_VIBRATOR,
-};
-
/* 8606 Registers */
#define PM8606_DCM_BOOST (0x00)
@@ -339,9 +329,7 @@ struct pm860x_backlight_pdata {
};
struct pm860x_led_pdata {
- int id;
int iset;
- unsigned long flags;
};
struct pm860x_rtc_pdata {
--
1.7.9.5
Avoid to use IORESOURCE_IO in 88pm860x regulator driver. Use
IORESOURCE_IO instead.
Signed-off-by: Haojian Zhuang <[email protected]>
---
drivers/mfd/88pm860x-core.c | 265 +++++++++++++++++++++++++++++++-----------
drivers/regulator/88pm8607.c | 7 +-
include/linux/mfd/88pm860x.h | 18 ++-
3 files changed, 218 insertions(+), 72 deletions(-)
diff --git a/drivers/mfd/88pm860x-core.c b/drivers/mfd/88pm860x-core.c
index 2934ea3..bde9231 100644
--- a/drivers/mfd/88pm860x-core.c
+++ b/drivers/mfd/88pm860x-core.c
@@ -68,25 +68,53 @@ static struct resource led5_resources[] __devinitdata = {
{0x8, 0x8, "blink", IORESOURCE_MEM, },
};
-static struct resource regulator_resources[] __devinitdata = {
- {PM8607_ID_BUCK1, PM8607_ID_BUCK1, "buck-1", IORESOURCE_IO,},
- {PM8607_ID_BUCK2, PM8607_ID_BUCK2, "buck-2", IORESOURCE_IO,},
- {PM8607_ID_BUCK3, PM8607_ID_BUCK3, "buck-3", IORESOURCE_IO,},
- {PM8607_ID_LDO1, PM8607_ID_LDO1, "ldo-01", IORESOURCE_IO,},
- {PM8607_ID_LDO2, PM8607_ID_LDO2, "ldo-02", IORESOURCE_IO,},
- {PM8607_ID_LDO3, PM8607_ID_LDO3, "ldo-03", IORESOURCE_IO,},
- {PM8607_ID_LDO4, PM8607_ID_LDO4, "ldo-04", IORESOURCE_IO,},
- {PM8607_ID_LDO5, PM8607_ID_LDO5, "ldo-05", IORESOURCE_IO,},
- {PM8607_ID_LDO6, PM8607_ID_LDO6, "ldo-06", IORESOURCE_IO,},
- {PM8607_ID_LDO7, PM8607_ID_LDO7, "ldo-07", IORESOURCE_IO,},
- {PM8607_ID_LDO8, PM8607_ID_LDO8, "ldo-08", IORESOURCE_IO,},
- {PM8607_ID_LDO9, PM8607_ID_LDO9, "ldo-09", IORESOURCE_IO,},
- {PM8607_ID_LDO10, PM8607_ID_LDO10, "ldo-10", IORESOURCE_IO,},
- {PM8607_ID_LDO11, PM8607_ID_LDO11, "ldo-11", IORESOURCE_IO,},
- {PM8607_ID_LDO12, PM8607_ID_LDO12, "ldo-12", IORESOURCE_IO,},
- {PM8607_ID_LDO13, PM8607_ID_LDO13, "ldo-13", IORESOURCE_IO,},
- {PM8607_ID_LDO14, PM8607_ID_LDO14, "ldo-14", IORESOURCE_IO,},
- {PM8607_ID_LDO15, PM8607_ID_LDO15, "ldo-15", IORESOURCE_IO,},
+static struct resource buck1_resources[] __devinitdata = {
+ {0x24, 0x24, "buck set", IORESOURCE_MEM, },
+};
+static struct resource buck2_resources[] __devinitdata = {
+ {0x25, 0x25, "buck set", IORESOURCE_MEM, },
+};
+static struct resource buck3_resources[] __devinitdata = {
+ {0x26, 0x26, "buck set", IORESOURCE_MEM, },
+};
+static struct resource ldo1_resources[] __devinitdata = {
+ {0x10, 0x10, "ldo set", IORESOURCE_MEM, },
+};
+static struct resource ldo2_resources[] __devinitdata = {
+ {0x11, 0x11, "ldo set", IORESOURCE_MEM, },
+};
+static struct resource ldo3_resources[] __devinitdata = {
+ {0x12, 0x12, "ldo set", IORESOURCE_MEM, },
+};
+static struct resource ldo4_resources[] __devinitdata = {
+ {0x13, 0x13, "ldo set", IORESOURCE_MEM, },
+};
+static struct resource ldo5_resources[] __devinitdata = {
+ {0x14, 0x14, "ldo set", IORESOURCE_MEM, },
+};
+static struct resource ldo6_resources[] __devinitdata = {
+ {0x15, 0x15, "ldo set", IORESOURCE_MEM, },
+};
+static struct resource ldo7_resources[] __devinitdata = {
+ {0x16, 0x16, "ldo set", IORESOURCE_MEM, },
+};
+static struct resource ldo8_resources[] __devinitdata = {
+ {0x17, 0x17, "ldo set", IORESOURCE_MEM, },
+};
+static struct resource ldo9_resources[] __devinitdata = {
+ {0x18, 0x18, "ldo set", IORESOURCE_MEM, },
+};
+static struct resource ldo10_resources[] __devinitdata = {
+ {0x19, 0x19, "ldo set", IORESOURCE_MEM, },
+};
+static struct resource ldo12_resources[] __devinitdata = {
+ {0x1a, 0x1a, "ldo set", IORESOURCE_MEM, },
+};
+static struct resource ldo_vibrator_resources[] __devinitdata = {
+ {0x28, 0x28, "ldo set", IORESOURCE_MEM, },
+};
+static struct resource ldo14_resources[] __devinitdata = {
+ {0x1b, 0x1b, "ldo set", IORESOURCE_MEM, },
};
static struct resource touch_resources[] __devinitdata = {
@@ -183,25 +211,88 @@ static struct mfd_cell led_devs[] = {
},
};
-static struct mfd_cell regulator_devs[] = {
- {"88pm860x-regulator", 0,},
- {"88pm860x-regulator", 1,},
- {"88pm860x-regulator", 2,},
- {"88pm860x-regulator", 3,},
- {"88pm860x-regulator", 4,},
- {"88pm860x-regulator", 5,},
- {"88pm860x-regulator", 6,},
- {"88pm860x-regulator", 7,},
- {"88pm860x-regulator", 8,},
- {"88pm860x-regulator", 9,},
- {"88pm860x-regulator", 10,},
- {"88pm860x-regulator", 11,},
- {"88pm860x-regulator", 12,},
- {"88pm860x-regulator", 13,},
- {"88pm860x-regulator", 14,},
- {"88pm860x-regulator", 15,},
- {"88pm860x-regulator", 16,},
- {"88pm860x-regulator", 17,},
+static struct mfd_cell reg_devs[] = {
+ {
+ .name = "88pm860x-regulator",
+ .id = 0,
+ .num_resources = ARRAY_SIZE(buck1_resources),
+ .resources = buck1_resources,
+ }, {
+ .name = "88pm860x-regulator",
+ .id = 1,
+ .num_resources = ARRAY_SIZE(buck2_resources),
+ .resources = buck2_resources,
+ }, {
+ .name = "88pm860x-regulator",
+ .id = 2,
+ .num_resources = ARRAY_SIZE(buck3_resources),
+ .resources = buck3_resources,
+ }, {
+ .name = "88pm860x-regulator",
+ .id = 3,
+ .num_resources = ARRAY_SIZE(ldo1_resources),
+ .resources = ldo1_resources,
+ }, {
+ .name = "88pm860x-regulator",
+ .id = 4,
+ .num_resources = ARRAY_SIZE(ldo2_resources),
+ .resources = ldo2_resources,
+ }, {
+ .name = "88pm860x-regulator",
+ .id = 5,
+ .num_resources = ARRAY_SIZE(ldo3_resources),
+ .resources = ldo3_resources,
+ }, {
+ .name = "88pm860x-regulator",
+ .id = 6,
+ .num_resources = ARRAY_SIZE(ldo4_resources),
+ .resources = ldo4_resources,
+ }, {
+ .name = "88pm860x-regulator",
+ .id = 7,
+ .num_resources = ARRAY_SIZE(ldo5_resources),
+ .resources = ldo5_resources,
+ }, {
+ .name = "88pm860x-regulator",
+ .id = 8,
+ .num_resources = ARRAY_SIZE(ldo6_resources),
+ .resources = ldo6_resources,
+ }, {
+ .name = "88pm860x-regulator",
+ .id = 9,
+ .num_resources = ARRAY_SIZE(ldo7_resources),
+ .resources = ldo7_resources,
+ }, {
+ .name = "88pm860x-regulator",
+ .id = 10,
+ .num_resources = ARRAY_SIZE(ldo8_resources),
+ .resources = ldo8_resources,
+ }, {
+ .name = "88pm860x-regulator",
+ .id = 11,
+ .num_resources = ARRAY_SIZE(ldo9_resources),
+ .resources = ldo9_resources,
+ }, {
+ .name = "88pm860x-regulator",
+ .id = 12,
+ .num_resources = ARRAY_SIZE(ldo10_resources),
+ .resources = ldo10_resources,
+ }, {
+ .name = "88pm860x-regulator",
+ .id = 13,
+ .num_resources = ARRAY_SIZE(ldo12_resources),
+ .resources = ldo12_resources,
+ }, {
+ .name = "88pm860x-regulator",
+ .id = 14,
+ .num_resources = ARRAY_SIZE(ldo_vibrator_resources),
+ .resources = ldo_vibrator_resources,
+ }, {
+ .name = "88pm860x-regulator",
+ .id = 15,
+ .num_resources = ARRAY_SIZE(ldo14_resources),
+ .resources = ldo14_resources,
+ },
};
static struct mfd_cell touch_devs[] = {
@@ -727,38 +818,80 @@ static void __devinit device_led_init(struct pm860x_chip *chip,
static void __devinit device_regulator_init(struct pm860x_chip *chip,
struct pm860x_platform_data *pdata)
{
- struct regulator_init_data *initdata;
int ret;
- int i, seq;
- if ((pdata == NULL) || (pdata->regulator == NULL))
+ if (pdata == NULL)
+ return;
+ if (pdata->buck1) {
+ reg_devs[0].platform_data = pdata->buck1;
+ reg_devs[0].pdata_size = sizeof(struct regulator_init_data);
+ }
+ if (pdata->buck2) {
+ reg_devs[1].platform_data = pdata->buck2;
+ reg_devs[1].pdata_size = sizeof(struct regulator_init_data);
+ }
+ if (pdata->buck3) {
+ reg_devs[2].platform_data = pdata->buck3;
+ reg_devs[2].pdata_size = sizeof(struct regulator_init_data);
+ }
+ if (pdata->ldo1) {
+ reg_devs[3].platform_data = pdata->ldo1;
+ reg_devs[3].pdata_size = sizeof(struct regulator_init_data);
+ }
+ if (pdata->ldo2) {
+ reg_devs[4].platform_data = pdata->ldo2;
+ reg_devs[4].pdata_size = sizeof(struct regulator_init_data);
+ }
+ if (pdata->ldo3) {
+ reg_devs[5].platform_data = pdata->ldo3;
+ reg_devs[5].pdata_size = sizeof(struct regulator_init_data);
+ }
+ if (pdata->ldo4) {
+ reg_devs[6].platform_data = pdata->ldo4;
+ reg_devs[6].pdata_size = sizeof(struct regulator_init_data);
+ }
+ if (pdata->ldo5) {
+ reg_devs[7].platform_data = pdata->ldo5;
+ reg_devs[7].pdata_size = sizeof(struct regulator_init_data);
+ }
+ if (pdata->ldo6) {
+ reg_devs[8].platform_data = pdata->ldo6;
+ reg_devs[8].pdata_size = sizeof(struct regulator_init_data);
+ }
+ if (pdata->ldo7) {
+ reg_devs[9].platform_data = pdata->ldo7;
+ reg_devs[9].pdata_size = sizeof(struct regulator_init_data);
+ }
+ if (pdata->ldo8) {
+ reg_devs[10].platform_data = pdata->ldo8;
+ reg_devs[10].pdata_size = sizeof(struct regulator_init_data);
+ }
+ if (pdata->ldo9) {
+ reg_devs[11].platform_data = pdata->ldo9;
+ reg_devs[11].pdata_size = sizeof(struct regulator_init_data);
+ }
+ if (pdata->ldo10) {
+ reg_devs[12].platform_data = pdata->ldo10;
+ reg_devs[12].pdata_size = sizeof(struct regulator_init_data);
+ }
+ if (pdata->ldo12) {
+ reg_devs[13].platform_data = pdata->ldo12;
+ reg_devs[13].pdata_size = sizeof(struct regulator_init_data);
+ }
+ if (pdata->ldo_vibrator) {
+ reg_devs[14].platform_data = pdata->ldo_vibrator;
+ reg_devs[14].pdata_size = sizeof(struct regulator_init_data);
+ }
+ if (pdata->ldo14) {
+ reg_devs[15].platform_data = pdata->ldo14;
+ reg_devs[15].pdata_size = sizeof(struct regulator_init_data);
+ }
+ ret = mfd_add_devices(chip->dev, 0, reg_devs,
+ ARRAY_SIZE(reg_devs), NULL, 0);
+ if (ret < 0) {
+ dev_err(chip->dev, "Failed to add regulator subdev\n");
return;
-
- if (pdata->num_regulators > ARRAY_SIZE(regulator_devs))
- pdata->num_regulators = ARRAY_SIZE(regulator_devs);
-
- for (i = 0, seq = -1; i < pdata->num_regulators; i++) {
- initdata = &pdata->regulator[i];
- seq = *(unsigned int *)initdata->driver_data;
- if ((seq < 0) || (seq > PM8607_ID_RG_MAX)) {
- dev_err(chip->dev, "Wrong ID(%d) on regulator(%s)\n",
- seq, initdata->constraints.name);
- goto out;
- }
- regulator_devs[i].platform_data = &pdata->regulator[i];
- regulator_devs[i].pdata_size = sizeof(struct regulator_init_data);
- regulator_devs[i].num_resources = 1;
- regulator_devs[i].resources = ®ulator_resources[seq];
-
- ret = mfd_add_devices(chip->dev, 0, ®ulator_devs[i], 1,
- ®ulator_resources[seq], 0);
- if (ret < 0) {
- dev_err(chip->dev, "Failed to add regulator subdev\n");
- goto out;
- }
}
-out:
- return;
}
static void __devinit device_rtc_init(struct pm860x_chip *chip,
diff --git a/drivers/regulator/88pm8607.c b/drivers/regulator/88pm8607.c
index c3482b9..33aad41 100644
--- a/drivers/regulator/88pm8607.c
+++ b/drivers/regulator/88pm8607.c
@@ -320,14 +320,14 @@ static int __devinit pm8607_regulator_probe(struct platform_device *pdev)
struct resource *res;
int i;
- res = platform_get_resource(pdev, IORESOURCE_IO, 0);
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
if (res == NULL) {
- dev_err(&pdev->dev, "No I/O resource!\n");
+ dev_err(&pdev->dev, "No memory resource!\n");
return -EINVAL;
}
for (i = 0; i < ARRAY_SIZE(pm8607_regulator_info); i++) {
info = &pm8607_regulator_info[i];
- if (info->desc.id == res->start)
+ if (info->desc.vsel_reg == res->start)
break;
}
if (i == ARRAY_SIZE(pm8607_regulator_info)) {
@@ -351,7 +351,6 @@ static int __devinit pm8607_regulator_probe(struct platform_device *pdev)
else
config.regmap = chip->regmap_companion;
- /* replace driver_data with info */
info->regulator = regulator_register(&info->desc, &config);
if (IS_ERR(info->regulator)) {
dev_err(&pdev->dev, "failed to register regulator %s\n",
diff --git a/include/linux/mfd/88pm860x.h b/include/linux/mfd/88pm860x.h
index 2d042f9..87c933d 100644
--- a/include/linux/mfd/88pm860x.h
+++ b/include/linux/mfd/88pm860x.h
@@ -359,7 +359,22 @@ struct pm860x_platform_data {
struct pm860x_rtc_pdata *rtc;
struct pm860x_touch_pdata *touch;
struct pm860x_power_pdata *power;
- struct regulator_init_data *regulator;
+ struct regulator_init_data *buck1;
+ struct regulator_init_data *buck2;
+ struct regulator_init_data *buck3;
+ struct regulator_init_data *ldo1;
+ struct regulator_init_data *ldo2;
+ struct regulator_init_data *ldo3;
+ struct regulator_init_data *ldo4;
+ struct regulator_init_data *ldo5;
+ struct regulator_init_data *ldo6;
+ struct regulator_init_data *ldo7;
+ struct regulator_init_data *ldo8;
+ struct regulator_init_data *ldo9;
+ struct regulator_init_data *ldo10;
+ struct regulator_init_data *ldo12;
+ struct regulator_init_data *ldo_vibrator;
+ struct regulator_init_data *ldo14;
unsigned short companion_addr; /* I2C address of companion chip */
int i2c_port; /* Controlled by GI2C or PI2C */
@@ -367,7 +382,6 @@ struct pm860x_platform_data {
int irq_base; /* IRQ base number of 88pm860x */
int num_leds;
int num_backlights;
- int num_regulators;
};
extern int pm8606_osc_enable(struct pm860x_chip *, unsigned short);
--
1.7.9.5
While touch field of 88pm860x platform_data isn't assigned, probe
function returns failure. Now update code to only return without
failure since touch field isn't always used in each usage scenario.
Signed-off-by: Haojian Zhuang <[email protected]>
---
drivers/mfd/88pm860x-core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/mfd/88pm860x-core.c b/drivers/mfd/88pm860x-core.c
index bde9231..60faf9b 100644
--- a/drivers/mfd/88pm860x-core.c
+++ b/drivers/mfd/88pm860x-core.c
@@ -551,7 +551,7 @@ static int __devinit device_gpadc_init(struct pm860x_chip *chip,
/* initialize GPADC without activating it */
if (!pdata || !pdata->touch)
- return -EINVAL;
+ return 0;
/* set GPADC MISC1 register */
data = 0;
--
1.7.9.5
Enable backlight & led components of 88pm860x PMIC in ttc dkb board.
Signed-off-by: Haojian Zhuang <[email protected]>
---
arch/arm/mach-mmp/ttc_dkb.c | 15 +++++++++++++++
1 file changed, 15 insertions(+)
diff --git a/arch/arm/mach-mmp/ttc_dkb.c b/arch/arm/mach-mmp/ttc_dkb.c
index 7a7de2b..1b51b81 100644
--- a/arch/arm/mach-mmp/ttc_dkb.c
+++ b/arch/arm/mach-mmp/ttc_dkb.c
@@ -138,8 +138,23 @@ static struct pca953x_platform_data max7312_data[] = {
},
};
+static struct pm860x_backlight_pdata ttc_dkb_backlight[] = {
+ { .iset = PM8606_WLED_CURRENT(4), },
+};
+
+static struct pm860x_led_pdata ttc_dkb_led[] = {
+ { .iset = PM8606_LED_CURRENT(12), },
+ { .iset = PM8606_LED_CURRENT(12), },
+ { .iset = PM8606_LED_CURRENT(12), },
+};
+
static struct pm860x_platform_data ttc_dkb_pm8607_info = {
.irq_base = IRQ_BOARD_START,
+ .companion_addr = 0x11,
+ .backlight = &ttc_dkb_backlight[0],
+ .num_backlights = ARRAY_SIZE(ttc_dkb_backlight),
+ .led = &ttc_dkb_led[0],
+ .num_leds = ARRAY_SIZE(ttc_dkb_led),
};
static struct i2c_board_info ttc_dkb_i2c_info[] = {
--
1.7.9.5
On Mon, Aug 06, 2012 at 12:32:48AM +0800, Haojian Zhuang wrote:
> Since IORESOURCE_IO is used for PCI devices, it doesn't fit on
> 88PM860x PMIC device that lies on I2C bus. So use IORESOURCE_MEM
> instead.
This isn't much more appropriate - _MEM is for memory ranges so isn't
directly relevant to register addresses either. If anything _IO is
slightly nearer.
On Mon, Aug 6, 2012 at 10:30 PM, Mark Brown
<[email protected]> wrote:
> On Mon, Aug 06, 2012 at 12:32:48AM +0800, Haojian Zhuang wrote:
>
>> Since IORESOURCE_IO is used for PCI devices, it doesn't fit on
>> 88PM860x PMIC device that lies on I2C bus. So use IORESOURCE_MEM
>> instead.
>
> This isn't much more appropriate - _MEM is for memory ranges so isn't
> directly relevant to register addresses either. If anything _IO is
> slightly nearer.
I use register resource to distinguish different components now. For
example, component driver
needs to access the registers in PMIC. These registers offsets are set
in 88pm860x-core.c.
So I think that it may not be called _IO.
Regards
Haojian
On Mon, Aug 06, 2012 at 10:42:12PM +0800, Haojian Zhuang wrote:
> On Mon, Aug 6, 2012 at 10:30 PM, Mark Brown
> > This isn't much more appropriate - _MEM is for memory ranges so isn't
> > directly relevant to register addresses either. If anything _IO is
> > slightly nearer.
> I use register resource to distinguish different components now. For
> example, component driver
> needs to access the registers in PMIC. These registers offsets are set
> in 88pm860x-core.c.
I understand this.
> So I think that it may not be called _IO.
Right, but _MEM isn't terribly relevant either. If anything _IO is a
bit better as ioports are *somewhat* similar to registers.
On Mon, Aug 6, 2012 at 11:46 PM, Mark Brown
<[email protected]> wrote:
> On Mon, Aug 06, 2012 at 10:42:12PM +0800, Haojian Zhuang wrote:
>> On Mon, Aug 6, 2012 at 10:30 PM, Mark Brown
>
>> > This isn't much more appropriate - _MEM is for memory ranges so isn't
>> > directly relevant to register addresses either. If anything _IO is
>> > slightly nearer.
>
>> I use register resource to distinguish different components now. For
>> example, component driver
>> needs to access the registers in PMIC. These registers offsets are set
>> in 88pm860x-core.c.
>
> I understand this.
>
>> So I think that it may not be called _IO.
>
> Right, but _MEM isn't terribly relevant either. If anything _IO is a
> bit better as ioports are *somewhat* similar to registers.
The problem is that each bit is already used in 32-bit IORESOURCE. I can't
find a empty bit to define the new IORESOURCE.
Regards
Haojian
On Mon, Aug 06, 2012 at 11:56:47PM +0800, Haojian Zhuang wrote:
> On Mon, Aug 6, 2012 at 11:46 PM, Mark Brown
> > Right, but _MEM isn't terribly relevant either. If anything _IO is a
> > bit better as ioports are *somewhat* similar to registers.
> The problem is that each bit is already used in 32-bit IORESOURCE. I can't
> find a empty bit to define the new IORESOURCE.
That's one reason why I've not attacked this problem myself, but frankly
I'm totally happy with using _IO here so I've not looked particularly
closely.
On Mon, Aug 06, 2012 at 04:58:06PM +0100, Mark Brown wrote:
> On Mon, Aug 06, 2012 at 11:56:47PM +0800, Haojian Zhuang wrote:
> > On Mon, Aug 6, 2012 at 11:46 PM, Mark Brown
>
> > > Right, but _MEM isn't terribly relevant either. If anything _IO is a
> > > bit better as ioports are *somewhat* similar to registers.
>
> > The problem is that each bit is already used in 32-bit IORESOURCE. I can't
> > find a empty bit to define the new IORESOURCE.
>
> That's one reason why I've not attacked this problem myself, but frankly
> I'm totally happy with using _IO here so I've not looked particularly
> closely.
NO. This is stupid. We've been here before, and I've said what I'm
saying below before too.
IORESOURCE_IO is for PCI/ISA IO resources.
IORESOURCE_MEM is for _memory mapped_ IO resources.
On ARM, we only have memory mapped IO resources, with the exception that
if we have a real PCI/ISA bus, we give them IORESOURCE_IO resources.
Never use IORESOURCE_IO for anything but PCI/ISA bus IO resources. Ever.
--
Russell King
Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/
maintainer of:
On Mon, Aug 06, 2012 at 08:22:09PM +0100, Russell King wrote:
> On Mon, Aug 06, 2012 at 04:58:06PM +0100, Mark Brown wrote:
> > That's one reason why I've not attacked this problem myself, but frankly
> > I'm totally happy with using _IO here so I've not looked particularly
> > closely.
> NO. This is stupid. We've been here before, and I've said what I'm
> saying below before too.
> IORESOURCE_IO is for PCI/ISA IO resources.
> IORESOURCE_MEM is for _memory mapped_ IO resources.
> On ARM, we only have memory mapped IO resources, with the exception that
> if we have a real PCI/ISA bus, we give them IORESOURCE_IO resources.
> Never use IORESOURCE_IO for anything but PCI/ISA bus IO resources. Ever.
*sigh* You must be aware that this isn't getting us anywhere. As you
know the issues here aren't practical ones if we make sure the resource
trees are split (which is what Haojian should really be doing if he's
not done so already) and that the resource code is sadly difficult to
modify to support new resource types due to the full bitmask that
Haojian mentioned.
Clearly nobody has the combination of time and interest to add a new
resource type and we do have actual systems running now (and for the
past several years) relying on this.
To be perfectly frank I have a hard time convincing myself that there's
any real problem with the current solution; obviously it's not what _IO
was originally intended for but having several different trees of
resources seems like a reasonable extension here and the effort involved
in any other changes seems disproportionately high. I guess we could
formalise it by making an alias for _IO but I doubt that'd address the
concerns you have.
On Mon, Aug 06, 2012 at 08:53:52PM +0100, Mark Brown wrote:
> On Mon, Aug 06, 2012 at 08:22:09PM +0100, Russell King wrote:
> > On Mon, Aug 06, 2012 at 04:58:06PM +0100, Mark Brown wrote:
>
> > > That's one reason why I've not attacked this problem myself, but frankly
> > > I'm totally happy with using _IO here so I've not looked particularly
> > > closely.
>
> > NO. This is stupid. We've been here before, and I've said what I'm
> > saying below before too.
>
> > IORESOURCE_IO is for PCI/ISA IO resources.
> > IORESOURCE_MEM is for _memory mapped_ IO resources.
>
> > On ARM, we only have memory mapped IO resources, with the exception that
> > if we have a real PCI/ISA bus, we give them IORESOURCE_IO resources.
>
> > Never use IORESOURCE_IO for anything but PCI/ISA bus IO resources. Ever.
>
> *sigh* You must be aware that this isn't getting us anywhere. As you
> know the issues here aren't practical ones if we make sure the resource
> trees are split (which is what Haojian should really be doing if he's
> not done so already) and that the resource code is sadly difficult to
> modify to support new resource types due to the full bitmask that
> Haojian mentioned.
>
> Clearly nobody has the combination of time and interest to add a new
> resource type and we do have actual systems running now (and for the
> past several years) relying on this.
>
> To be perfectly frank I have a hard time convincing myself that there's
> any real problem with the current solution; obviously it's not what _IO
> was originally intended for but having several different trees of
> resources seems like a reasonable extension here and the effort involved
> in any other changes seems disproportionately high. I guess we could
> formalise it by making an alias for _IO but I doubt that'd address the
> concerns you have.
So, the fact that platform devices will get any resource marked
IORESOURCE_IO registered against ioport_resource isn't a problem
then...
Anyway, given that this thread is broken, there's no way for me to find
out what the _original_ issue is that you're talking about. So I'm going
to guess that it's come up because we're out of IORESOURCE bits.
Let's look a little closer:
#define IORESOURCE_TYPE_BITS 0x00001f00 /* Resource type */
#define IORESOURCE_IO 0x00000100
#define IORESOURCE_MEM 0x00000200
#define IORESOURCE_IRQ 0x00000400
#define IORESOURCE_DMA 0x00000800
#define IORESOURCE_BUS 0x00001000
So, if we made this a numeric index, then we have 32 resource types
to deal with, and no need to bugger around with re-using an existing
type for something else.
This makes sense, MEM, IRQ and DMA are all mutually exclusive, as
should be MEM and IO (because they can't coexist in two resource trees
at the same time.) BUS only gets used in a hand-full of places and
not with any other flags.
So, looks like we can have 27 new resource types fairly easily.
--
Russell King
Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/
maintainer of:
On Mon, Aug 06, 2012 at 10:31:24PM +0100, Russell King wrote:
> So, the fact that platform devices will get any resource marked
> IORESOURCE_IO registered against ioport_resource isn't a problem
> then...
This is what providing a separate parent to ensure they're in a
different tree is there to fix.
> Anyway, given that this thread is broken, there's no way for me to find
> out what the _original_ issue is that you're talking about. So I'm going
> to guess that it's come up because we're out of IORESOURCE bits.
No, that's not it. What's happened is that Haojian has posted some
patching changing all the _IO resources to _MEM in the Marvell PMIC
drivers, I think because you yelled at him for using _IO when he
reported that the changes in ioport_resource broke things a few releases
ago. Obviously this doesn't achieve a huge amount, it's a misplaced
cleanup.
> So, if we made this a numeric index, then we have 32 resource types
> to deal with, and no need to bugger around with re-using an existing
> type for something else.
This seems sensible, and I'm sure if that change were made people would
be delighed to use new resource types, but like I say nobody who's
motivated to do anything here seems to have the time to do anything
about it.
Whoever looks at this would need to do some detective work, it does seem
like there must have been a reason to use a bitmask here...
On Tue, Aug 7, 2012 at 6:00 AM, Mark Brown
<[email protected]> wrote:
> On Mon, Aug 06, 2012 at 10:31:24PM +0100, Russell King wrote:
>
>> Anyway, given that this thread is broken, there's no way for me to find
>> out what the _original_ issue is that you're talking about. So I'm going
>> to guess that it's come up because we're out of IORESOURCE bits.
>
> No, that's not it. What's happened is that Haojian has posted some
> patching changing all the _IO resources to _MEM in the Marvell PMIC
> drivers, I think because you yelled at him for using _IO when he
> reported that the changes in ioport_resource broke things a few releases
> ago. Obviously this doesn't achieve a huge amount, it's a misplaced
> cleanup.
>
It's because IO_SPACE_LIMIT is set as 0 if there's no PCI devices. But
IORESOURCE_IO is also used in PMIC mfd drivers to distinguish
different components.
commit 04e1c83806e30ae339fc45def595960c7fef1697
Author: Russell King <[email protected]>
Date: Wed Jul 6 12:49:59 2011 +0100
ARM: io: add a default IO_SPACE_LIMIT definition
Add a default IO_SPACE_LIMIT definition. Explain the chosen value and
suggest why platforms would want to make it larger.
Signed-off-by: Russell King <[email protected]>
>> So, if we made this a numeric index, then we have 32 resource types
>> to deal with, and no need to bugger around with re-using an existing
>> type for something else.
>
> This seems sensible, and I'm sure if that change were made people would
> be delighed to use new resource types, but like I say nobody who's
> motivated to do anything here seems to have the time to do anything
> about it.
>
> Whoever looks at this would need to do some detective work, it does seem
> like there must have been a reason to use a bitmask here...
Changing bitmask to a value for IORESOURCE type is a risk. I agree on Mark
that someone will complain on this.
Could we consider to expand the usage of IORESOURCE_IO? Maybe we can
use it for both ISA/PCI and IO related in chip.
On Tue, Aug 07, 2012 at 09:47:25AM +0800, Haojian Zhuang wrote:
> On Tue, Aug 7, 2012 at 6:00 AM, Mark Brown
> <[email protected]> wrote:
> > On Mon, Aug 06, 2012 at 10:31:24PM +0100, Russell King wrote:
> >
> >> Anyway, given that this thread is broken, there's no way for me to find
> >> out what the _original_ issue is that you're talking about. So I'm going
> >> to guess that it's come up because we're out of IORESOURCE bits.
> >
> > No, that's not it. What's happened is that Haojian has posted some
> > patching changing all the _IO resources to _MEM in the Marvell PMIC
> > drivers, I think because you yelled at him for using _IO when he
> > reported that the changes in ioport_resource broke things a few releases
> > ago. Obviously this doesn't achieve a huge amount, it's a misplaced
> > cleanup.
> >
> It's because IO_SPACE_LIMIT is set as 0 if there's no PCI devices. But
> IORESOURCE_IO is also used in PMIC mfd drivers to distinguish
> different components.
>
> commit 04e1c83806e30ae339fc45def595960c7fef1697
> Author: Russell King <[email protected]>
> Date: Wed Jul 6 12:49:59 2011 +0100
>
> ARM: io: add a default IO_SPACE_LIMIT definition
>
> Add a default IO_SPACE_LIMIT definition. Explain the chosen value and
> suggest why platforms would want to make it larger.
>
> Signed-off-by: Russell King <[email protected]>
>
> >> So, if we made this a numeric index, then we have 32 resource types
> >> to deal with, and no need to bugger around with re-using an existing
> >> type for something else.
> >
> > This seems sensible, and I'm sure if that change were made people would
> > be delighed to use new resource types, but like I say nobody who's
> > motivated to do anything here seems to have the time to do anything
> > about it.
> >
> > Whoever looks at this would need to do some detective work, it does seem
> > like there must have been a reason to use a bitmask here...
>
> Changing bitmask to a value for IORESOURCE type is a risk. I agree on Mark
> that someone will complain on this.
We won't know that unless we try and propose to do it in patch form.
>From what I can see, there is nothing in the kernel which technically
prevents us from doing this.
> Could we consider to expand the usage of IORESOURCE_IO? Maybe we can
> use it for both ISA/PCI and IO related in chip.
If it's not clear, I am *completely* against this. It's a hack and bodge,
and therefore doesn't belong in the kernel.
--
Russell King
Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/
maintainer of:
On Mon, 2012-08-06 at 22:31 +0100, Russell King wrote:
>
> So, if we made this a numeric index, then we have 32 resource types
> to deal with, and no need to bugger around with re-using an existing
> type for something else.
>
> This makes sense, MEM, IRQ and DMA are all mutually exclusive, as
> should be MEM and IO (because they can't coexist in two resource trees
> at the same time.) BUS only gets used in a hand-full of places and
> not with any other flags.
>
> So, looks like we can have 27 new resource types fairly easily.
Besides we can easily use a single IORESOURCE_OTHER for most things
really, if we prefer, make it IORESOURCE_IO | IORESOURCE_MEM and have
platform device avoid that combo...
cheers,
Ben.
On Tue, 2012-08-07 at 09:47 +0800, Haojian Zhuang wrote:
> > Whoever looks at this would need to do some detective work, it does
> seem
> > like there must have been a reason to use a bitmask here...
>
> Changing bitmask to a value for IORESOURCE type is a risk. I agree on
> Mark
> that someone will complain on this.
>
> Could we consider to expand the usage of IORESOURCE_IO? Maybe we can
> use it for both ISA/PCI and IO related in chip.
No, I agree with Russell. I would suggest changing the bitmask.
However this can be done painlessly since the existing types don't
change value so the existing code that checks bits is still correct
in all cases we care about.
Cheers,
Ben.
On Tue, Aug 07, 2012 at 06:22:22PM +1000, Benjamin Herrenschmidt wrote:
> On Mon, 2012-08-06 at 22:31 +0100, Russell King wrote:
> >
> > So, if we made this a numeric index, then we have 32 resource types
> > to deal with, and no need to bugger around with re-using an existing
> > type for something else.
> >
> > This makes sense, MEM, IRQ and DMA are all mutually exclusive, as
> > should be MEM and IO (because they can't coexist in two resource trees
> > at the same time.) BUS only gets used in a hand-full of places and
> > not with any other flags.
> >
> > So, looks like we can have 27 new resource types fairly easily.
>
> Besides we can easily use a single IORESOURCE_OTHER for most things
> really, if we prefer, make it IORESOURCE_IO | IORESOURCE_MEM and have
> platform device avoid that combo...
That will work just the same way that I'm suggesting. We can keep
the existing bit-based numbers, and:
#define IORESOURCE_OTHER 0x00000300
and the platform code will avoid using the standard resource trees,
because it does things correctly here:
if (resource_type(r) == IORESOURCE_MEM)
p = &iomem_resource;
else if (resource_type(r) == IORESOURCE_IO)
p = &ioport_resource;
Same for the resource getting functions. Hardly surprising this, because
I wrote this code...
So, no need to touch any existing users or change their behaviour in
any way.
--
Russell King
Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/
maintainer of:
On Tue, Aug 07, 2012 at 09:47:25AM +0800, Haojian Zhuang wrote:
> It's because IO_SPACE_LIMIT is set as 0 if there's no PCI devices. But
> IORESOURCE_IO is also used in PMIC mfd drivers to distinguish
> different components.
The change to keep things working here (pending the other changes which
Russell wants) is to add a dummy resource with a wide enough range of
registers defined and make it the parent for all the _IO resouces the
PMIC has. This will put all the PMIC _IO resources in a separate tree
to ioport_resource which can have the resorces added.
If nothing else this seems much more suitable for stable and -rc (the
bug has been there since v3.4).
On Tue, Aug 07, 2012 at 11:38:51AM +0100, Mark Brown wrote:
> On Tue, Aug 07, 2012 at 09:47:25AM +0800, Haojian Zhuang wrote:
>
> > It's because IO_SPACE_LIMIT is set as 0 if there's no PCI devices. But
> > IORESOURCE_IO is also used in PMIC mfd drivers to distinguish
> > different components.
>
> The change to keep things working here (pending the other changes which
> Russell wants) is to add a dummy resource with a wide enough range of
> registers defined and make it the parent for all the _IO resouces the
> PMIC has. This will put all the PMIC _IO resources in a separate tree
> to ioport_resource which can have the resorces added.
>
> If nothing else this seems much more suitable for stable and -rc (the
> bug has been there since v3.4).
There is no need for such hacks.
Just do as I've outlined:
1. Create a new resource type:
#define IORESOURCE_TYPE_FOO 0x00000300
2. Use this resource type for your resources in MFD which are using
platform devices.
3. Use it with the platform API. It will not handle this new resource
type any differently, it will treat it as its own unique resource type.
4. Use platform_get_resource() with this new resource type, it will work.
5. Move along to some other problem and enjoy life.
There's no need to botch this in any way what so ever, or invent some
other solution only to have to (probably never) rework it.
--
Russell King
Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/
maintainer of:
On Tuesday 07 August 2012, Russell King wrote:
> On Tue, Aug 07, 2012 at 06:22:22PM +1000, Benjamin Herrenschmidt wrote:
> > On Mon, 2012-08-06 at 22:31 +0100, Russell King wrote:
> > >
> > > So, if we made this a numeric index, then we have 32 resource types
> > > to deal with, and no need to bugger around with re-using an existing
> > > type for something else.
> > >
> > > This makes sense, MEM, IRQ and DMA are all mutually exclusive, as
> > > should be MEM and IO (because they can't coexist in two resource trees
> > > at the same time.) BUS only gets used in a hand-full of places and
> > > not with any other flags.
> > >
> > > So, looks like we can have 27 new resource types fairly easily.
> >
> > Besides we can easily use a single IORESOURCE_OTHER for most things
> > really, if we prefer, make it IORESOURCE_IO | IORESOURCE_MEM and have
> > platform device avoid that combo...
>
> That will work just the same way that I'm suggesting. We can keep
> the existing bit-based numbers, and:
>
> #define IORESOURCE_OTHER 0x00000300
>
> and the platform code will avoid using the standard resource trees,
> because it does things correctly here:
>
> if (resource_type(r) == IORESOURCE_MEM)
> p = &iomem_resource;
> else if (resource_type(r) == IORESOURCE_IO)
> p = &ioport_resource;
>
I had not realized that we did this in platform_device_add(), which
means using IORESOURCE_IO in mfd is even more broken than I thought
previously.
I've looked through the code some more and your solution sounds
like the best option to get this sorted quickly. The entire
resource logic will probably come back to haunt us with ACPI 5.0
which adds region types for a number of new things (i2c, gpio,
ipmi, cmos, ...) and we might end up representing them
as resource. Or we might not.
If we introduce a new IORESOURCE_OTHER, I would actually prefer to
define it to 0x00000000 for purely aesthetic reasons, the effect
should be the same as using 0x00000300.
Arnd
On Tue, Aug 07, 2012 at 12:13:31PM +0100, Russell King wrote:
> On Tue, Aug 07, 2012 at 11:38:51AM +0100, Mark Brown wrote:
> > If nothing else this seems much more suitable for stable and -rc (the
> > bug has been there since v3.4).
> There is no need for such hacks.
Please read the text you quoted above.
> There's no need to botch this in any way what so ever, or invent some
> other solution only to have to (probably never) rework it.
The changes you're suggesting are extremely invasive for stable
especially given that we have a simple, driver local, fix available
which is already deployed and used in other drivers in the same kernels
(at least in v3.5 anyway, the change introducing the issue got pushed
into v3.4 unexpectedly so the fix didn't make it). It would be worrying
if such changes were being accepted into stable.
I agree that the changes you're suggeting seem sensible for v3.7 if
someone wants to work on them, possibly even v3.6 if people are willing
to do something like that after the merge window, but we've got an issue
in stable here which is the most urgent thing.
On Tue, Aug 07, 2012 at 12:28:44PM +0100, Mark Brown wrote:
> On Tue, Aug 07, 2012 at 12:13:31PM +0100, Russell King wrote:
> > On Tue, Aug 07, 2012 at 11:38:51AM +0100, Mark Brown wrote:
>
> > > If nothing else this seems much more suitable for stable and -rc (the
> > > bug has been there since v3.4).
>
> > There is no need for such hacks.
>
> Please read the text you quoted above.
I did.
> > There's no need to botch this in any way what so ever, or invent some
> > other solution only to have to (probably never) rework it.
>
> The changes you're suggesting are extremely invasive for stable
> especially given that we have a simple, driver local, fix available
*Rubbish*.
--
Russell King
Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/
maintainer of:
On Tue, Aug 07, 2012 at 11:28:27AM +0000, Arnd Bergmann wrote:
> If we introduce a new IORESOURCE_OTHER, I would actually prefer to
> define it to 0x00000000 for purely aesthetic reasons, the effect
> should be the same as using 0x00000300.
I'd suggest not, because we can use that to detect uninitialized
resources (and we probably do so in some places.) IOW, I think that's
asking for problems when this moves outside platform code.
--
Russell King
Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/
maintainer of:
On Tuesday 07 August 2012, Russell King wrote:
> On Tue, Aug 07, 2012 at 11:28:27AM +0000, Arnd Bergmann wrote:
> > If we introduce a new IORESOURCE_OTHER, I would actually prefer to
> > define it to 0x00000000 for purely aesthetic reasons, the effect
> > should be the same as using 0x00000300.
>
> I'd suggest not, because we can use that to detect uninitialized
> resources (and we probably do so in some places.) IOW, I think that's
> asking for problems when this moves outside platform code.
Good point. Let's use 0x00000300 then.
Arnd
On Tue, Aug 07, 2012 at 11:28:27AM +0000, Arnd Bergmann wrote:
> I've looked through the code some more and your solution sounds
> like the best option to get this sorted quickly. The entire
There's no disagreement here, if someone actually wrote a patch we might
get somewhere here. That said, as I just pointed out in my mail to
Russell this is an issue for stable too.
On Tue, Aug 07, 2012 at 12:31:21PM +0100, Russell King wrote:
> On Tue, Aug 07, 2012 at 12:28:44PM +0100, Mark Brown wrote:
> > On Tue, Aug 07, 2012 at 12:13:31PM +0100, Russell King wrote:
> > > On Tue, Aug 07, 2012 at 11:38:51AM +0100, Mark Brown wrote:
> >
> > > > If nothing else this seems much more suitable for stable and -rc (the
> > > > bug has been there since v3.4).
> >
> > > There is no need for such hacks.
> >
> > Please read the text you quoted above.
>
> I did.
>
> > > There's no need to botch this in any way what so ever, or invent some
> > > other solution only to have to (probably never) rework it.
> >
> > The changes you're suggesting are extremely invasive for stable
> > especially given that we have a simple, driver local, fix available
>
> *Rubbish*.
And, for those hard of thinking, I'll tell you exactly how invasive it
is.
1. You modify ioport.h to add the new type.
2. You change the mfd driver creating the resources to use the new type.
3. You change the mfd driver using the resources to call
platform_resource_get() with the new type.
4. You get on with life.
Yes, it's really that damned simple. Not invasive at all.
Mark, you're really making a mountain out of a bloody mole hill over this.
--
Russell King
Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/
maintainer of:
On Tue, Aug 07, 2012 at 12:31:21PM +0100, Russell King wrote:
> On Tue, Aug 07, 2012 at 12:28:44PM +0100, Mark Brown wrote:
> > The changes you're suggesting are extremely invasive for stable
> > especially given that we have a simple, driver local, fix available
> *Rubbish*.
This isn't helpful or constructive...
Which bit of the above are you referring to here? If it's the having a
fix bit then as pointed out repeatedly now in this and the previous
thread we've got the separete resource tree approach implemented in
stable right now making actual systems run.
On Tue, Aug 07, 2012 at 12:35:22PM +0100, Mark Brown wrote:
> On Tue, Aug 07, 2012 at 11:28:27AM +0000, Arnd Bergmann wrote:
>
> > I've looked through the code some more and your solution sounds
> > like the best option to get this sorted quickly. The entire
>
> There's no disagreement here, if someone actually wrote a patch we might
> get somewhere here. That said, as I just pointed out in my mail to
> Russell this is an issue for stable too.
Is this simple enough for you? Maybe if you told me:
1. what you'd like these new resources to be called
2. which are the affected mfd drivers
I could create a better patch.
include/linux/ioport.h | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/include/linux/ioport.h b/include/linux/ioport.h
index 589e0e7..9798319 100644
--- a/include/linux/ioport.h
+++ b/include/linux/ioport.h
@@ -31,6 +31,7 @@ struct resource {
#define IORESOURCE_TYPE_BITS 0x00001f00 /* Resource type */
#define IORESOURCE_IO 0x00000100
#define IORESOURCE_MEM 0x00000200
+#define IORESOURCE_FOO 0x00000300
#define IORESOURCE_IRQ 0x00000400
#define IORESOURCE_DMA 0x00000800
#define IORESOURCE_BUS 0x00001000
--
Russell King
Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/
maintainer of:
On Tue, Aug 07, 2012 at 12:38:02PM +0100, Mark Brown wrote:
> On Tue, Aug 07, 2012 at 12:31:21PM +0100, Russell King wrote:
> > On Tue, Aug 07, 2012 at 12:28:44PM +0100, Mark Brown wrote:
>
> > > The changes you're suggesting are extremely invasive for stable
> > > especially given that we have a simple, driver local, fix available
>
> > *Rubbish*.
>
> This isn't helpful or constructive...
>
> Which bit of the above are you referring to here? If it's the having a
> fix bit then as pointed out repeatedly now in this and the previous
> thread we've got the separete resource tree approach implemented in
> stable right now making actual systems run.
All of your above statement. It is, basically, completely wrong, and
shows that you haven't thought about the solution I'm proposing at all.
I've shown you in simple steps how easy it is. It is not invasive. It
is not complex. It is local to the affected drivers. So, all your
points above are plain wrong. Hence "rubbish".
--
Russell King
Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/
maintainer of:
On Tue, Aug 07, 2012 at 12:36:52PM +0100, Russell King wrote:
> And, for those hard of thinking, I'll tell you exactly how invasive it
> is.
> 1. You modify ioport.h to add the new type.
> Yes, it's really that damned simple. Not invasive at all.
Your step 1 is the bit that strikes me as invasive here - that's not
something I'd be touching in a stable release if I didn't have to, it's
visible to half the kernel in an area where we clearly don't have ideal
review of the code (otherwise we'd not have a problem here in the first
place) which seems totally disproportionate to the benefit here. We're
talking about an issue which affects one device which is used only on
Marvell systems here.
I think everyone agrees that this is the best route forward for future
kernels.
On Tue, Aug 07, 2012 at 12:45:57PM +0100, Mark Brown wrote:
> On Tue, Aug 07, 2012 at 12:36:52PM +0100, Russell King wrote:
>
> > And, for those hard of thinking, I'll tell you exactly how invasive it
> > is.
>
> > 1. You modify ioport.h to add the new type.
>
> > Yes, it's really that damned simple. Not invasive at all.
>
> Your step 1 is the bit that strikes me as invasive here - that's not
> something I'd be touching in a stable release if I didn't have to, it's
> visible to half the kernel in an area where we clearly don't have ideal
> review of the code (otherwise we'd not have a problem here in the first
> place) which seems totally disproportionate to the benefit here. We're
> talking about an issue which affects one device which is used only on
> Marvell systems here.
>
> I think everyone agrees that this is the best route forward for future
> kernels.
For fuck sake Mark. You are insane.
How can:
#define IORESOURCE_FOO 0x00000300
in ioport.h be called "invasive" ? The best chance of error is that the
identifier is already in use. So learn to use grep to check the whole
sodding tree first to make sure that the identifier you're choosing to
use isn't already in use somewhere.
And in any case, I suspect you've lost the plot, because I suspect the
driver you are referring to is wm831x, which has already had your solution
patched into it by you back in May.
And you still haven't done me the curtesy of answering my repeated
questions about WHAT BLOODY DRIVER you are referring to has the problem.
There is no point in discussing this any further unless you START answering
some of the basic questions, rather than constantly trying to poke holes
in a solution you did not invent. (Do you suffer from not-invented-here
syndrome? Because you *are* showing all the signs of that.)
commit ce7e4e11221dd7fbe82c8ad28d1875b0dfa20de4
Author: Mark Brown <[email protected]>
Date: Mon May 7 10:03:20 2012 +0100
mfd: Fix wm831x register range passing for recent ARM updates
The removal of mach/io.h from most ARM platforms also set the range of
valid IO ports to be empty for most platforms when previously any 32
bit integer had been valid. This makes it impossible to add IO resources
as the added range is smaller than that of the root resource for IO ports.
Since we're not really using IO memory at all fix this by defining our
own root resource outside the normal tree and make that the parent of
all IO resources. This also ensures we won't conflict with read IO ports
if we ever run on a platform which happens to use them.
Signed-off-by: Mark Brown <[email protected]>
Signed-off-by: Samuel Ortiz <[email protected]>
--
Russell King
Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/
maintainer of:
On Tue, Aug 07, 2012 at 12:44:15PM +0100, Russell King wrote:
> On Tue, Aug 07, 2012 at 12:38:02PM +0100, Mark Brown wrote:
> > On Tue, Aug 07, 2012 at 12:31:21PM +0100, Russell King wrote:
> > > On Tue, Aug 07, 2012 at 12:28:44PM +0100, Mark Brown wrote:
> >
> > > > The changes you're suggesting are extremely invasive for stable
> > > > especially given that we have a simple, driver local, fix available
> >
> > > *Rubbish*.
> >
> > This isn't helpful or constructive...
> >
> > Which bit of the above are you referring to here? If it's the having a
> > fix bit then as pointed out repeatedly now in this and the previous
> > thread we've got the separete resource tree approach implemented in
> > stable right now making actual systems run.
>
> All of your above statement. It is, basically, completely wrong, and
> shows that you haven't thought about the solution I'm proposing at all.
>
> I've shown you in simple steps how easy it is. It is not invasive. It
> is not complex. It is local to the affected drivers. So, all your
> points above are plain wrong. Hence "rubbish".
And, because you WILL NOT tell me which are the affected drivers, I've
assumed in the patch below that you're referring to the wm831x drivers.
So here's the patch in full for this driver. Oh look how simple and
non-invasive it is.
$ git grep IORESOURCE_REG
drivers/mfd/wm831x-core.c: .flags = IORESOURCE_REG,
... repeated ...
drivers/mfd/wm831x-core.c: .flags = IORESOURCE_REG,
drivers/regulator/wm831x-dcdc.c: res = platform_get_resource(pdev, IORESO
drivers/regulator/wm831x-dcdc.c: res = platform_get_resource(pdev, IORESO
drivers/regulator/wm831x-dcdc.c: res = platform_get_resource(pdev, IORESO
drivers/regulator/wm831x-isink.c: res = platform_get_resource(pdev, IORESO
drivers/regulator/wm831x-ldo.c: res = platform_get_resource(pdev, IORESOURCE_REG
drivers/regulator/wm831x-ldo.c: res = platform_get_resource(pdev, IORESOURCE_REG
drivers/regulator/wm831x-ldo.c: res = platform_get_resource(pdev, IORESOURCE_REG
include/linux/ioport.h:#define IORESOURCE_REG 0x00000300 /* Regis
$
So no identifier clashes.
No, I haven't tested it, I don't have the hardware. That's your job
as the driver maintainer.
The only open questions on this are:
1. Is there another driver you are concerned about.
2. Choosing a better name.
But I won't put question marks at the end of those because you never seem
to answer questions. You will just produce yet more justifications why
this approach is not one you're willing to accept. So I really don't know
why I wasted my time to produce this patch.
drivers/mfd/wm831x-core.c | 66 +++++++++++--------------------------
drivers/regulator/wm831x-dcdc.c | 12 +++---
drivers/regulator/wm831x-isink.c | 4 +-
drivers/regulator/wm831x-ldo.c | 12 +++---
include/linux/ioport.h | 1 +
5 files changed, 35 insertions(+), 60 deletions(-)
diff --git a/drivers/mfd/wm831x-core.c b/drivers/mfd/wm831x-core.c
index 946698f..2e5d58e 100644
--- a/drivers/mfd/wm831x-core.c
+++ b/drivers/mfd/wm831x-core.c
@@ -614,18 +614,11 @@ int wm831x_set_bits(struct wm831x *wm831x, unsigned short reg,
}
EXPORT_SYMBOL_GPL(wm831x_set_bits);
-static struct resource wm831x_io_parent = {
- .start = 0,
- .end = 0xffffffff,
- .flags = IORESOURCE_IO,
-};
-
static struct resource wm831x_dcdc1_resources[] = {
{
- .parent = &wm831x_io_parent,
.start = WM831X_DC1_CONTROL_1,
.end = WM831X_DC1_DVS_CONTROL,
- .flags = IORESOURCE_IO,
+ .flags = IORESOURCE_REG,
},
{
.name = "UV",
@@ -644,10 +637,9 @@ static struct resource wm831x_dcdc1_resources[] = {
static struct resource wm831x_dcdc2_resources[] = {
{
- .parent = &wm831x_io_parent,
.start = WM831X_DC2_CONTROL_1,
.end = WM831X_DC2_DVS_CONTROL,
- .flags = IORESOURCE_IO,
+ .flags = IORESOURCE_REG,
},
{
.name = "UV",
@@ -665,10 +657,9 @@ static struct resource wm831x_dcdc2_resources[] = {
static struct resource wm831x_dcdc3_resources[] = {
{
- .parent = &wm831x_io_parent,
.start = WM831X_DC3_CONTROL_1,
.end = WM831X_DC3_SLEEP_CONTROL,
- .flags = IORESOURCE_IO,
+ .flags = IORESOURCE_REG,
},
{
.name = "UV",
@@ -680,10 +671,9 @@ static struct resource wm831x_dcdc3_resources[] = {
static struct resource wm831x_dcdc4_resources[] = {
{
- .parent = &wm831x_io_parent,
.start = WM831X_DC4_CONTROL,
.end = WM831X_DC4_SLEEP_CONTROL,
- .flags = IORESOURCE_IO,
+ .flags = IORESOURCE_REG,
},
{
.name = "UV",
@@ -695,10 +685,9 @@ static struct resource wm831x_dcdc4_resources[] = {
static struct resource wm8320_dcdc4_buck_resources[] = {
{
- .parent = &wm831x_io_parent,
.start = WM831X_DC4_CONTROL,
.end = WM832X_DC4_SLEEP_CONTROL,
- .flags = IORESOURCE_IO,
+ .flags = IORESOURCE_REG,
},
{
.name = "UV",
@@ -718,10 +707,9 @@ static struct resource wm831x_gpio_resources[] = {
static struct resource wm831x_isink1_resources[] = {
{
- .parent = &wm831x_io_parent,
.start = WM831X_CURRENT_SINK_1,
.end = WM831X_CURRENT_SINK_1,
- .flags = IORESOURCE_IO,
+ .flags = IORESOURCE_REG,
},
{
.start = WM831X_IRQ_CS1,
@@ -732,10 +720,9 @@ static struct resource wm831x_isink1_resources[] = {
static struct resource wm831x_isink2_resources[] = {
{
- .parent = &wm831x_io_parent,
.start = WM831X_CURRENT_SINK_2,
.end = WM831X_CURRENT_SINK_2,
- .flags = IORESOURCE_IO,
+ .flags = IORESOURCE_REG,
},
{
.start = WM831X_IRQ_CS2,
@@ -746,10 +733,9 @@ static struct resource wm831x_isink2_resources[] = {
static struct resource wm831x_ldo1_resources[] = {
{
- .parent = &wm831x_io_parent,
.start = WM831X_LDO1_CONTROL,
.end = WM831X_LDO1_SLEEP_CONTROL,
- .flags = IORESOURCE_IO,
+ .flags = IORESOURCE_REG,
},
{
.name = "UV",
@@ -761,10 +747,9 @@ static struct resource wm831x_ldo1_resources[] = {
static struct resource wm831x_ldo2_resources[] = {
{
- .parent = &wm831x_io_parent,
.start = WM831X_LDO2_CONTROL,
.end = WM831X_LDO2_SLEEP_CONTROL,
- .flags = IORESOURCE_IO,
+ .flags = IORESOURCE_REG,
},
{
.name = "UV",
@@ -776,10 +761,9 @@ static struct resource wm831x_ldo2_resources[] = {
static struct resource wm831x_ldo3_resources[] = {
{
- .parent = &wm831x_io_parent,
.start = WM831X_LDO3_CONTROL,
.end = WM831X_LDO3_SLEEP_CONTROL,
- .flags = IORESOURCE_IO,
+ .flags = IORESOURCE_REG,
},
{
.name = "UV",
@@ -791,10 +775,9 @@ static struct resource wm831x_ldo3_resources[] = {
static struct resource wm831x_ldo4_resources[] = {
{
- .parent = &wm831x_io_parent,
.start = WM831X_LDO4_CONTROL,
.end = WM831X_LDO4_SLEEP_CONTROL,
- .flags = IORESOURCE_IO,
+ .flags = IORESOURCE_REG,
},
{
.name = "UV",
@@ -806,10 +789,9 @@ static struct resource wm831x_ldo4_resources[] = {
static struct resource wm831x_ldo5_resources[] = {
{
- .parent = &wm831x_io_parent,
.start = WM831X_LDO5_CONTROL,
.end = WM831X_LDO5_SLEEP_CONTROL,
- .flags = IORESOURCE_IO,
+ .flags = IORESOURCE_REG,
},
{
.name = "UV",
@@ -821,10 +803,9 @@ static struct resource wm831x_ldo5_resources[] = {
static struct resource wm831x_ldo6_resources[] = {
{
- .parent = &wm831x_io_parent,
.start = WM831X_LDO6_CONTROL,
.end = WM831X_LDO6_SLEEP_CONTROL,
- .flags = IORESOURCE_IO,
+ .flags = IORESOURCE_REG,
},
{
.name = "UV",
@@ -836,10 +817,9 @@ static struct resource wm831x_ldo6_resources[] = {
static struct resource wm831x_ldo7_resources[] = {
{
- .parent = &wm831x_io_parent,
.start = WM831X_LDO7_CONTROL,
.end = WM831X_LDO7_SLEEP_CONTROL,
- .flags = IORESOURCE_IO,
+ .flags = IORESOURCE_REG,
},
{
.name = "UV",
@@ -851,10 +831,9 @@ static struct resource wm831x_ldo7_resources[] = {
static struct resource wm831x_ldo8_resources[] = {
{
- .parent = &wm831x_io_parent,
.start = WM831X_LDO8_CONTROL,
.end = WM831X_LDO8_SLEEP_CONTROL,
- .flags = IORESOURCE_IO,
+ .flags = IORESOURCE_REG,
},
{
.name = "UV",
@@ -866,10 +845,9 @@ static struct resource wm831x_ldo8_resources[] = {
static struct resource wm831x_ldo9_resources[] = {
{
- .parent = &wm831x_io_parent,
.start = WM831X_LDO9_CONTROL,
.end = WM831X_LDO9_SLEEP_CONTROL,
- .flags = IORESOURCE_IO,
+ .flags = IORESOURCE_REG,
},
{
.name = "UV",
@@ -881,10 +859,9 @@ static struct resource wm831x_ldo9_resources[] = {
static struct resource wm831x_ldo10_resources[] = {
{
- .parent = &wm831x_io_parent,
.start = WM831X_LDO10_CONTROL,
.end = WM831X_LDO10_SLEEP_CONTROL,
- .flags = IORESOURCE_IO,
+ .flags = IORESOURCE_REG,
},
{
.name = "UV",
@@ -896,10 +873,9 @@ static struct resource wm831x_ldo10_resources[] = {
static struct resource wm831x_ldo11_resources[] = {
{
- .parent = &wm831x_io_parent,
.start = WM831X_LDO11_ON_CONTROL,
.end = WM831X_LDO11_SLEEP_CONTROL,
- .flags = IORESOURCE_IO,
+ .flags = IORESOURCE_REG,
},
};
@@ -998,19 +974,17 @@ static struct resource wm831x_rtc_resources[] = {
static struct resource wm831x_status1_resources[] = {
{
- .parent = &wm831x_io_parent,
.start = WM831X_STATUS_LED_1,
.end = WM831X_STATUS_LED_1,
- .flags = IORESOURCE_IO,
+ .flags = IORESOURCE_REG,
},
};
static struct resource wm831x_status2_resources[] = {
{
- .parent = &wm831x_io_parent,
.start = WM831X_STATUS_LED_2,
.end = WM831X_STATUS_LED_2,
- .flags = IORESOURCE_IO,
+ .flags = IORESOURCE_REG,
},
};
diff --git a/drivers/regulator/wm831x-dcdc.c b/drivers/regulator/wm831x-dcdc.c
index 7413885..64111f4 100644
--- a/drivers/regulator/wm831x-dcdc.c
+++ b/drivers/regulator/wm831x-dcdc.c
@@ -476,9 +476,9 @@ static __devinit int wm831x_buckv_probe(struct platform_device *pdev)
dcdc->wm831x = wm831x;
- res = platform_get_resource(pdev, IORESOURCE_IO, 0);
+ res = platform_get_resource(pdev, IORESOURCE_REG, 0);
if (res == NULL) {
- dev_err(&pdev->dev, "No I/O resource\n");
+ dev_err(&pdev->dev, "No REG resource\n");
ret = -EINVAL;
goto err;
}
@@ -651,9 +651,9 @@ static __devinit int wm831x_buckp_probe(struct platform_device *pdev)
dcdc->wm831x = wm831x;
- res = platform_get_resource(pdev, IORESOURCE_IO, 0);
+ res = platform_get_resource(pdev, IORESOURCE_REG, 0);
if (res == NULL) {
- dev_err(&pdev->dev, "No I/O resource\n");
+ dev_err(&pdev->dev, "No REG resource\n");
ret = -EINVAL;
goto err;
}
@@ -795,9 +795,9 @@ static __devinit int wm831x_boostp_probe(struct platform_device *pdev)
dcdc->wm831x = wm831x;
- res = platform_get_resource(pdev, IORESOURCE_IO, 0);
+ res = platform_get_resource(pdev, IORESOURCE_REG, 0);
if (res == NULL) {
- dev_err(&pdev->dev, "No I/O resource\n");
+ dev_err(&pdev->dev, "No REG resource\n");
ret = -EINVAL;
goto err;
}
diff --git a/drivers/regulator/wm831x-isink.c b/drivers/regulator/wm831x-isink.c
index 0d207c2..2646a19 100644
--- a/drivers/regulator/wm831x-isink.c
+++ b/drivers/regulator/wm831x-isink.c
@@ -172,9 +172,9 @@ static __devinit int wm831x_isink_probe(struct platform_device *pdev)
isink->wm831x = wm831x;
- res = platform_get_resource(pdev, IORESOURCE_IO, 0);
+ res = platform_get_resource(pdev, IORESOURCE_REG, 0);
if (res == NULL) {
- dev_err(&pdev->dev, "No I/O resource\n");
+ dev_err(&pdev->dev, "No REG resource\n");
ret = -EINVAL;
goto err;
}
diff --git a/drivers/regulator/wm831x-ldo.c b/drivers/regulator/wm831x-ldo.c
index 5cb70ca..da73daf 100644
--- a/drivers/regulator/wm831x-ldo.c
+++ b/drivers/regulator/wm831x-ldo.c
@@ -269,9 +269,9 @@ static __devinit int wm831x_gp_ldo_probe(struct platform_device *pdev)
ldo->wm831x = wm831x;
- res = platform_get_resource(pdev, IORESOURCE_IO, 0);
+ res = platform_get_resource(pdev, IORESOURCE_REG, 0);
if (res == NULL) {
- dev_err(&pdev->dev, "No I/O resource\n");
+ dev_err(&pdev->dev, "No REG resource\n");
ret = -EINVAL;
goto err;
}
@@ -520,9 +520,9 @@ static __devinit int wm831x_aldo_probe(struct platform_device *pdev)
ldo->wm831x = wm831x;
- res = platform_get_resource(pdev, IORESOURCE_IO, 0);
+ res = platform_get_resource(pdev, IORESOURCE_REG, 0);
if (res == NULL) {
- dev_err(&pdev->dev, "No I/O resource\n");
+ dev_err(&pdev->dev, "No REG resource\n");
ret = -EINVAL;
goto err;
}
@@ -675,9 +675,9 @@ static __devinit int wm831x_alive_ldo_probe(struct platform_device *pdev)
ldo->wm831x = wm831x;
- res = platform_get_resource(pdev, IORESOURCE_IO, 0);
+ res = platform_get_resource(pdev, IORESOURCE_REG, 0);
if (res == NULL) {
- dev_err(&pdev->dev, "No I/O resource\n");
+ dev_err(&pdev->dev, "No REG resource\n");
ret = -EINVAL;
goto err;
}
diff --git a/include/linux/ioport.h b/include/linux/ioport.h
index 589e0e7..bfee885 100644
--- a/include/linux/ioport.h
+++ b/include/linux/ioport.h
@@ -31,6 +31,7 @@ struct resource {
#define IORESOURCE_TYPE_BITS 0x00001f00 /* Resource type */
#define IORESOURCE_IO 0x00000100
#define IORESOURCE_MEM 0x00000200
+#define IORESOURCE_REG 0x00000300 /* Register offsets */
#define IORESOURCE_IRQ 0x00000400
#define IORESOURCE_DMA 0x00000800
#define IORESOURCE_BUS 0x00001000
--
Russell King
Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/
maintainer of:
On Tue, Aug 07, 2012 at 12:51:40PM +0100, Russell King wrote:
> For fuck sake Mark. You are insane.
Please take a step back from the ad hominem remarks.
> How can:
> #define IORESOURCE_FOO 0x00000300
> in ioport.h be called "invasive" ? The best chance of error is that the
> identifier is already in use. So learn to use grep to check the whole
> sodding tree first to make sure that the identifier you're choosing to
> use isn't already in use somewhere.
My concern there (and that of others who've looked at adding a new
resource type) is that this value can also be written as
#define IORESOURCE_FOO (IORESOURCE_IO | IORESOURCE_MEM)
and the selection of values chosen for the resource types clearly looks
like it's supposed to be interpreted as a bitmask for some reason. This
is the main reason nobody touched the code already, it sets off alarm
bells from a code review point of view.
It may well be the case that the constants are only ever viewed as plain
old numbers in which case we're fine but it really doesn't seem too
clevr for stable.
> And in any case, I suspect you've lost the plot, because I suspect the
> driver you are referring to is wm831x, which has already had your solution
> patched into it by you back in May.
The urgent issue is for the Marvell PMIC drivers (drivers/mfd/88pm*)
which are also affected but have not had a fix implemented so have been
broken since v3.4. The wm831x drivers should be updated to use the new
API too but don't now have an issue in stable right now. There may be
others but presumably they're not even testing stable releases so again
probably not urgent.
> And you still haven't done me the curtesy of answering my repeated
> questions about WHAT BLOODY DRIVER you are referring to has the problem.
You've only asked this once that I've seen, in the mail where you posted
your patch (which is a helpful step forward, thanks!) which very recent.
It's possible that you've asked this elsewhere, though I did just scan
through a good chunk of the mails and didn't see the question.
> There is no point in discussing this any further unless you START answering
> some of the basic questions, rather than constantly trying to poke holes
> in a solution you did not invent. (Do you suffer from not-invented-here
> syndrome? Because you *are* showing all the signs of that.)
As I keep saying I don't think there's been any disagreement that adding
one or more new resource types is the best approach going forward, the
only issues have been around what happens in stable and someone having
the time to add the new resource type.
On Tue, Aug 07, 2012 at 01:11:57PM +0100, Russell King wrote:
> The only open questions on this are:
> 1. Is there another driver you are concerned about.
As I said elsewhere 88pm* needs this as a stable bugfix and wm831x
should be converted over too.
> 2. Choosing a better name.
I'm not sure we need one, _REG seems perfectly fine here unless we want
to go with Arnd's suggestion of _OTHER. Do you have any concerns with
the use of reg?
> But I won't put question marks at the end of those because you never seem
> to answer questions. You will just produce yet more justifications why
> this approach is not one you're willing to accept. So I really don't know
> why I wasted my time to produce this patch.
As I mentioned in my mail a few moments ago I had not replied to your
mails asking these question about which drivers are affected because
those mails have been arriving so quickly since the first one I saw with
the question in that there has not been a chance to do so.
> index 589e0e7..bfee885 100644
> --- a/include/linux/ioport.h
> +++ b/include/linux/ioport.h
> @@ -31,6 +31,7 @@ struct resource {
> #define IORESOURCE_TYPE_BITS 0x00001f00 /* Resource type */
> #define IORESOURCE_IO 0x00000100
> #define IORESOURCE_MEM 0x00000200
> +#define IORESOURCE_REG 0x00000300 /* Register offsets */
> #define IORESOURCE_IRQ 0x00000400
> #define IORESOURCE_DMA 0x00000800
> #define IORESOURCE_BUS 0x00001000
As I've said before I'm fine with the driver changes. I do feel that it
would be better to also renumber all the existing resource types while
we're at it in order to make it clear that these are just plain numbers,
that's the reason nobody wrote this patch previously. This will avoid
any future confusion.
On Tue, Aug 07, 2012 at 01:58:20PM +0100, Mark Brown wrote:
> On Tue, Aug 07, 2012 at 12:51:40PM +0100, Russell King wrote:
>
> > For fuck sake Mark. You are insane.
>
> Please take a step back from the ad hominem remarks.
Well, stop causing frustration at this end. Yes, you're the cause of my
frustration, because you're doing everything you possibly can to give
reasons why not without even looking at the facts. And you continue to
do so at the bottom of this mail.
If you want to be constructive, then actually take a bloody look at my
suggestion, try it out and report back whether it works. Stop attacking
my proposal in whatever weak ways you can, especially in ways that I've
already dismissed as being totally irrelevant.
As far as I'm concerned, your arguments so far against my proposal are
all completely and utterly petty, and that's putting it mildly.
> My concern there (and that of others who've looked at adding a new
> resource type) is that this value can also be written as
>
> #define IORESOURCE_FOO (IORESOURCE_IO | IORESOURCE_MEM)
>
> and the selection of values chosen for the resource types clearly looks
> like it's supposed to be interpreted as a bitmask for some reason. This
> is the main reason nobody touched the code already, it sets off alarm
> bells from a code review point of view.
Sigh. Here we go a fucking again. I've already covered this. But
in case you haven't read, here it is again.
And that combination is illegal today. But the amount of code which
the value will be exposed to is limited to _just_ the platform code,
which, for the parts affeced, I'm the author of. And I've read it
before creating the patch to make sure it will work as I expect it
do.
> > And in any case, I suspect you've lost the plot, because I suspect the
> > driver you are referring to is wm831x, which has already had your solution
> > patched into it by you back in May.
>
> The urgent issue is for the Marvell PMIC drivers (drivers/mfd/88pm*)
> which are also affected but have not had a fix implemented so have been
> broken since v3.4. The wm831x drivers should be updated to use the new
> API too but don't now have an issue in stable right now. There may be
> others but presumably they're not even testing stable releases so again
> probably not urgent.
Right, thank you, finally you've said something constructive at last.
But I'm all out of patience with you to create a patch; you've wasted
all my good will away through your petty arguments.
> > And you still haven't done me the curtesy of answering my repeated
> > questions about WHAT BLOODY DRIVER you are referring to has the problem.
>
> You've only asked this once that I've seen, in the mail where you posted
> your patch (which is a helpful step forward, thanks!) which very recent.
> It's possible that you've asked this elsewhere, though I did just scan
> through a good chunk of the mails and didn't see the question.
Twice I asked. The first time was with the IORESOURCE_FOO mail, which
you had time to read, and reply to - and your reply was yet another
"why we can't do it this way" whinge. Yes, you had time to read it,
you had time to answer it with reasons why not, but not to give any
useful information back.
Maybe you should be the one to take a step back and look at the quality
of your replies in this thread. Because none of them have been
constructive.
Instead, you'll notice that many of my replies have been outlining
solutions to the problem, and *actually* contain patches to address
the issue. Yours? All whinges about why not.
> > There is no point in discussing this any further unless you START answering
> > some of the basic questions, rather than constantly trying to poke holes
> > in a solution you did not invent. (Do you suffer from not-invented-here
> > syndrome? Because you *are* showing all the signs of that.)
>
> As I keep saying I don't think there's been any disagreement that adding
> one or more new resource types is the best approach going forward, the
> only issues have been around what happens in stable and someone having
> the time to add the new resource type.
"someone having the time to add the new resource type" ? Oh for fuck sake
Mark, what the hell are you talking about? How long does it take to apply
a bloody patch?
So yet again, this is another bloody whinge from you about why not.
So I'm wasting my time on this thread. Someone else can fix the other
driver in the way I've outlined. I've shown you how. I've shown you
that there's agreement from other people for my approach. There isn't
an issue with -stable with this approach. There isn't an issue with
-rc.
All it needs is for someone to do the search and replace on the relevant
drivers.
--
Russell King
Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/
maintainer of:
On Tuesday 07 August 2012, Mark Brown wrote:
> On Tue, Aug 07, 2012 at 01:11:57PM +0100, Russell King wrote:
>
> > The only open questions on this are:
> > 1. Is there another driver you are concerned about.
>
> As I said elsewhere 88pm* needs this as a stable bugfix and wm831x
> should be converted over too.
I've looked through the remaining MFD drivers and found one more,
max8925-core, which is using IORESOURCE_IO for something that is
not ISA/PCI IO-space.
> > 2. Choosing a better name.
>
> I'm not sure we need one, _REG seems perfectly fine here unless we want
> to go with Arnd's suggestion of _OTHER. Do you have any concerns with
> the use of reg?
BenH actually suggested _OTHER. I think either one is fine.
> > But I won't put question marks at the end of those because you never seem
> > to answer questions. You will just produce yet more justifications why
> > this approach is not one you're willing to accept. So I really don't know
> > why I wasted my time to produce this patch.
>
> As I mentioned in my mail a few moments ago I had not replied to your
> mails asking these question about which drivers are affected because
> those mails have been arriving so quickly since the first one I saw with
> the question in that there has not been a chance to do so.
>
> > index 589e0e7..bfee885 100644
> > --- a/include/linux/ioport.h
> > +++ b/include/linux/ioport.h
> > @@ -31,6 +31,7 @@ struct resource {
> > #define IORESOURCE_TYPE_BITS 0x00001f00 /* Resource type */
> > #define IORESOURCE_IO 0x00000100
> > #define IORESOURCE_MEM 0x00000200
> > +#define IORESOURCE_REG 0x00000300 /* Register offsets */
> > #define IORESOURCE_IRQ 0x00000400
> > #define IORESOURCE_DMA 0x00000800
> > #define IORESOURCE_BUS 0x00001000
>
> As I've said before I'm fine with the driver changes. I do feel that it
> would be better to also renumber all the existing resource types while
> we're at it in order to make it clear that these are just plain numbers,
> that's the reason nobody wrote this patch previously. This will avoid
> any future confusion.
This gets into a lot more tricky territory: We have a bunch of drivers
doing their own bitmask operations on these, like drivers/video/offb.c
testing
if ((flags & (IORESOURCE_IO | IORESOURCE_MEM)) == 0)
return NULL;
or drivers/scsi/gdth.c doing
if (!(base0 & IORESOURCE_MEM) ||
!(base2 & IORESOURCE_MEM) ||
!(base1 & IORESOURCE_IO))
return -ENODEV;
Now I've looked at the three drivers with the immediate problem of
IORESOURCE_IO abuse (max8925, wm831x, 88pm860x) and none of them are
doing such bitmask operations, so I'm reasonably sure we are fine
for those drivers. I also agree that renumbering the resources in a
way that makes it impossible to use bitmasks is a good idea, but
that would actually be pretty invasive because then we have to rewrite
all the functions that currently do it.
Arnd
On Tue, Aug 07, 2012 at 02:28:15PM +0000, Arnd Bergmann wrote:
> On Tuesday 07 August 2012, Mark Brown wrote:
> > On Tue, Aug 07, 2012 at 01:11:57PM +0100, Russell King wrote:
> > > index 589e0e7..bfee885 100644
> > > --- a/include/linux/ioport.h
> > > +++ b/include/linux/ioport.h
> > > @@ -31,6 +31,7 @@ struct resource {
> > > #define IORESOURCE_TYPE_BITS 0x00001f00 /* Resource type */
> > > #define IORESOURCE_IO 0x00000100
> > > #define IORESOURCE_MEM 0x00000200
> > > +#define IORESOURCE_REG 0x00000300 /* Register offsets */
> > > #define IORESOURCE_IRQ 0x00000400
> > > #define IORESOURCE_DMA 0x00000800
> > > #define IORESOURCE_BUS 0x00001000
> >
> > As I've said before I'm fine with the driver changes. I do feel that it
> > would be better to also renumber all the existing resource types while
> > we're at it in order to make it clear that these are just plain numbers,
> > that's the reason nobody wrote this patch previously. This will avoid
> > any future confusion.
>
> This gets into a lot more tricky territory: We have a bunch of drivers
> doing their own bitmask operations on these, like drivers/video/offb.c
> testing
>
> if ((flags & (IORESOURCE_IO | IORESOURCE_MEM)) == 0)
> return NULL;
>
> or drivers/scsi/gdth.c doing
>
> if (!(base0 & IORESOURCE_MEM) ||
> !(base2 & IORESOURCE_MEM) ||
> !(base1 & IORESOURCE_IO))
> return -ENODEV;
>
> Now I've looked at the three drivers with the immediate problem of
> IORESOURCE_IO abuse (max8925, wm831x, 88pm860x) and none of them are
> doing such bitmask operations, so I'm reasonably sure we are fine
> for those drivers. I also agree that renumbering the resources in a
> way that makes it impossible to use bitmasks is a good idea, but
> that would actually be pretty invasive because then we have to rewrite
> all the functions that currently do it.
Don't feed the troll :)
None of the code you list above would be affected in any way by the
changes I propose; we're not changing the existing values, and these
drivers would not see the new IORESOURCE_REG type.
That's not to say that they wouldn't need fixing (they do), but they
are not a reason to reject my proposal, even for -stable trees.
--
Russell King
Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/
maintainer of:
On Tue, Aug 7, 2012 at 1:51 PM, Russell King <[email protected]> wrote:
> How can:
>
> #define IORESOURCE_FOO 0x00000300
>
> in ioport.h be called "invasive" ? The best chance of error is that the
> identifier is already in use. So learn to use grep to check the whole
> sodding tree first to make sure that the identifier you're choosing to
> use isn't already in use somewhere.
Perhaps it's not invasive enough? :-)
Don't you need an extra file in /proc, too (cfr. /proc/ioports and /proc/iomem)?
And as Arnd pointed out, if resources will be used for various new buses,
"IORESOURCE_FOO" or "IORESOURCE_OTHER" is a bit vague.
What about conflicts where one driver means i2c addresses and another
one means gpio addresses? The resource system will reject them?
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
> #define IORESOURCE_TYPE_BITS 0x00001f00 /* Resource type */
> #define IORESOURCE_IO 0x00000100
> #define IORESOURCE_MEM 0x00000200
> +#define IORESOURCE_FOO 0x00000300
These are bit masks and checked as such in many places. This makes no
sense at all.
Moving to IO_RESOURCE_TYPE() being 0-31 values might be smart but its a
massive all kernel change.
Alan
On Tue, Aug 07, 2012 at 05:22:45PM +0200, Geert Uytterhoeven wrote:
> And as Arnd pointed out, if resources will be used for various new buses,
> "IORESOURCE_FOO" or "IORESOURCE_OTHER" is a bit vague.
> What about conflicts where one driver means i2c addresses and another
> one means gpio addresses? The resource system will reject them?
I changed the subsequent patch to use IORESOURCE_REG - at least that
better describes what it's for. Maybe IORESOURCE_REGRANGE would be
better (so it can be used for any register range resource on any bus
type) ?
However, one issue that I hope has already been addressed is what space
the ranges are in, and how does a sub-driver get to know that. To put
it another way, how does a sub-driver get to know about the 'base' for
these register ranges. I hope that problem has been thought about in
MFD land _before_ the approach of passing around register ranges
through resources was allowed to happen.
--
Russell King
Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/
maintainer of:
On Tue, Aug 07, 2012 at 04:45:55PM +0100, Alan Cox wrote:
> > #define IORESOURCE_TYPE_BITS 0x00001f00 /* Resource type */
> > #define IORESOURCE_IO 0x00000100
> > #define IORESOURCE_MEM 0x00000200
> > +#define IORESOURCE_FOO 0x00000300
>
> These are bit masks and checked as such in many places. This makes no
> sense at all.
Correct, but nowhere are they checked as masks in the platform
device/driver code nor the MFD driver code. Here's the relevant
extracts from the platform driver code:
struct resource *platform_get_resource(struct platform_device *dev,
unsigned int type, unsigned int num)
{
int i;
for (i = 0; i < dev->num_resources; i++) {
struct resource *r = &dev->resource[i];
if (type == resource_type(r) && num-- == 0)
return r;
}
return NULL;
}
...
if (resource_type(r) == IORESOURCE_MEM)
p = &iomem_resource;
else if (resource_type(r) == IORESOURCE_IO)
p = &ioport_resource;
This is modern code, written using the accessors provided in ioport.h.
resource_type() is defined as:
static inline unsigned long resource_type(const struct resource *res)
{
return res->flags & IORESOURCE_TYPE_BITS;
}
So, provided these don't leak outside of the platform and the affected
MFD drivers, what the rest of the kernel does doesn't matter.
> Moving to IO_RESOURCE_TYPE() being 0-31 values might be smart but its a
> massive all kernel change.
Only if we want to change the existing numbering _or_ propagate them
outside of platform devices etc, and when that happens that's the time
to start fixing stuff one subsystem at a time.
Of course, if the above helper was being used, we'd already be set.
I don't see that as a blocker to its local use, contained completely
within the MFD and platform device subsystems.
--
Russell King
Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/
maintainer of:
On Tue, Aug 07, 2012 at 02:47:50PM +0100, Russell King wrote:
> If you want to be constructive, then actually take a bloody look at my
> suggestion, try it out and report back whether it works. Stop attacking
> my proposal in whatever weak ways you can, especially in ways that I've
> already dismissed as being totally irrelevant.
I have looked at your suggestion, and I have repeatedly agreed that your
suggestion is the best suggestion going forwards. I don't think there
is any way in which I could be clearer on that.
> > My concern there (and that of others who've looked at adding a new
> > resource type) is that this value can also be written as
> > #define IORESOURCE_FOO (IORESOURCE_IO | IORESOURCE_MEM)
> > and the selection of values chosen for the resource types clearly looks
> > like it's supposed to be interpreted as a bitmask for some reason. This
> > is the main reason nobody touched the code already, it sets off alarm
> > bells from a code review point of view.
> Sigh. Here we go a fucking again. I've already covered this. But
> in case you haven't read, here it is again.
> And that combination is illegal today. But the amount of code which
> the value will be exposed to is limited to _just_ the platform code,
> which, for the parts affeced, I'm the author of. And I've read it
> before creating the patch to make sure it will work as I expect it
> do.
Well, I guess we have to agree to disagree on that. I'm just very much
more conservative than you about what I'd be willing to put into a
stable release - my instincts on this are very strongly towards avoiding
any possibility of introducing unintended side effects and one way of
doing that is minimising the scope of any change. Even where things
should be correct and look correct minimising the risk of exposing any
latent bugs (including those in systems derived from the stable release)
is a major consideration for me with any sort of stable fix. Clearly
any code that's affected is buggy but having people be able to trust
stable releases is a very big factor.
This is all that we're disagreeing on, everyone including me agrees that
your change is clearly the best change going forwards but I'm much more
paranoid about stable releases than you are.
> > You've only asked this once that I've seen, in the mail where you posted
> > your patch (which is a helpful step forward, thanks!) which very recent.
> > It's possible that you've asked this elsewhere, though I did just scan
> > through a good chunk of the mails and didn't see the question.
> Twice I asked. The first time was with the IORESOURCE_FOO mail, which
> you had time to read, and reply to - and your reply was yet another
> "why we can't do it this way" whinge. Yes, you had time to read it,
> you had time to answer it with reasons why not, but not to give any
> useful information back.
Hrm, if you're referring to <[email protected]>
I don't actually seem to be able to see the question in your mail.
> Instead, you'll notice that many of my replies have been outlining
> solutions to the problem, and *actually* contain patches to address
> the issue. Yours? All whinges about why not.
The main reason I haven't send any patches was that by the time we were
satisfied that this was OK people were already sending code changes so
it seemed like that was in hand (and indeed you have sent a patch for
this).
> > As I keep saying I don't think there's been any disagreement that adding
> > one or more new resource types is the best approach going forward, the
> > only issues have been around what happens in stable and someone having
> > the time to add the new resource type.
> "someone having the time to add the new resource type" ? Oh for fuck sake
> Mark, what the hell are you talking about? How long does it take to apply
> a bloody patch?
It's the bit with confirming that we're OK not treating the resource
types as a bitmask that's time consuming; until this thread nobody had
gone through and checked that this would be OK.
On Tue, Aug 07, 2012 at 02:28:15PM +0000, Arnd Bergmann wrote:
> On Tuesday 07 August 2012, Mark Brown wrote:
> > As I said elsewhere 88pm* needs this as a stable bugfix and wm831x
> > should be converted over too.
> I've looked through the remaining MFD drivers and found one more,
> max8925-core, which is using IORESOURCE_IO for something that is
> not ISA/PCI IO-space.
Ah, yes - that's also used on Marvell platforms which presumably aren't
getting much mainline testing either.
> > > 2. Choosing a better name.
> > I'm not sure we need one, _REG seems perfectly fine here unless we want
> > to go with Arnd's suggestion of _OTHER. Do you have any concerns with
> > the use of reg?
> BenH actually suggested _OTHER. I think either one is fine.
Sorry Ben.
> Now I've looked at the three drivers with the immediate problem of
> IORESOURCE_IO abuse (max8925, wm831x, 88pm860x) and none of them are
> doing such bitmask operations, so I'm reasonably sure we are fine
Hopefully nobody will add this to the core code either. I wonder if
it's possible to change the definition of the constants for the resource
types so that they generate a warning if anyone does bitops on them...
that'd also get all the drivers fixed up eventually.
> for those drivers. I also agree that renumbering the resources in a
> way that makes it impossible to use bitmasks is a good idea, but
> that would actually be pretty invasive because then we have to rewrite
> all the functions that currently do it.
Yeah, that's why the new resource type has never been added - I don't
think anyone had considered the possibility of adding a new type without
also doing the renumbering, I know I didn't when I looked.
On Tue, Aug 07, 2012 at 04:44:58PM +0100, Russell King wrote:
> However, one issue that I hope has already been addressed is what space
> the ranges are in, and how does a sub-driver get to know that. To put
> it another way, how does a sub-driver get to know about the 'base' for
> these register ranges. I hope that problem has been thought about in
> MFD land _before_ the approach of passing around register ranges
> through resources was allowed to happen.
That's been thought through - the subdevice drivers already have to rely
heavily on the fact that they know about their parent and part of that
contract is that the base address for these resources is always address
zero in the register space of the MFD and there's never more than one
resource in the tree. Nothing will ever do anything like reserve the
resources, they just get looked up to retrieve the base address for the
relevant range.
Someone could do a device which does something different but nobody
wanted to yet and it's very unclear that anyone would, if they did then
they'd have to deal with the issues that will result.
On Tuesday 07 August 2012, Geert Uytterhoeven wrote:
>Don't you need an extra file in /proc, too (cfr. /proc/ioports and /proc/iomem)?
>
> And as Arnd pointed out, if resources will be used for various new buses,
> "IORESOURCE_FOO" or "IORESOURCE_OTHER" is a bit vague.
> What about conflicts where one driver means i2c addresses and another
> one means gpio addresses? The resource system will reject them?
The platform code currently only inserts the resources of type IO or
MEM. For the new type, we would not insert them to any resource
tree and they consequently are not available in any /proc file.
No driver should ever try to request access to those resources either.
The only thing we do with them is attach them to a platform device
in the mfd driver and take them out of it again in the child driver.
Arnd