* [PATCH v7 01/10] power: supply: sbs-battery: remove incorrect le16_to_cpu calls
2017-06-15 13:59 [PATCH v7 00/10] Add sbs-manager with smbalert support Phil Reid
@ 2017-06-15 13:59 ` Phil Reid
[not found] ` <1497535178-12001-2-git-send-email-preid-qgqNFa1JUf/o2iN0hyhwsIdd74u8MsAO@public.gmane.org>
2017-06-15 13:59 ` [PATCH v7 02/10] power: supply: bq24735: " Phil Reid
` (8 subsequent siblings)
9 siblings, 1 reply; 30+ messages in thread
From: Phil Reid @ 2017-06-15 13:59 UTC (permalink / raw)
To: wsa, robh+dt, mark.rutland, sre, jdelvare, jglauber, david.daney,
peda, preid, linux-i2c, devicetree, linux-pm
i2c_smbus commands handle the correct byte order for smbus transactions
internally. This will currently result in incorrect operation on big
endian systems.
Signed-off-by: Phil Reid <preid@electromag.com.au>
---
drivers/power/supply/sbs-battery.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/drivers/power/supply/sbs-battery.c b/drivers/power/supply/sbs-battery.c
index e3a114e..cf43e38 100644
--- a/drivers/power/supply/sbs-battery.c
+++ b/drivers/power/supply/sbs-battery.c
@@ -199,7 +199,7 @@ static int sbs_read_word_data(struct i2c_client *client, u8 address)
return ret;
}
- return le16_to_cpu(ret);
+ return ret;
}
static int sbs_read_string_data(struct i2c_client *client, u8 address,
@@ -265,7 +265,7 @@ static int sbs_read_string_data(struct i2c_client *client, u8 address,
memcpy(values, block_buffer + 1, block_length);
values[block_length] = '\0';
- return le16_to_cpu(ret);
+ return ret;
}
static int sbs_write_word_data(struct i2c_client *client, u8 address,
@@ -278,8 +278,7 @@ static int sbs_write_word_data(struct i2c_client *client, u8 address,
retries = chip->i2c_retry_count;
while (retries > 0) {
- ret = i2c_smbus_write_word_data(client, address,
- le16_to_cpu(value));
+ ret = i2c_smbus_write_word_data(client, address, value);
if (ret >= 0)
break;
retries--;
--
1.8.3.1
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PATCH v7 02/10] power: supply: bq24735: remove incorrect le16_to_cpu calls
2017-06-15 13:59 [PATCH v7 00/10] Add sbs-manager with smbalert support Phil Reid
2017-06-15 13:59 ` [PATCH v7 01/10] power: supply: sbs-battery: remove incorrect le16_to_cpu calls Phil Reid
@ 2017-06-15 13:59 ` Phil Reid
[not found] ` <1497535178-12001-3-git-send-email-preid-qgqNFa1JUf/o2iN0hyhwsIdd74u8MsAO@public.gmane.org>
2017-06-15 13:59 ` [PATCH v7 03/10] i2c: i2c-smbus: Use threaded irq for smbalert Phil Reid
` (7 subsequent siblings)
9 siblings, 1 reply; 30+ messages in thread
From: Phil Reid @ 2017-06-15 13:59 UTC (permalink / raw)
To: wsa, robh+dt, mark.rutland, sre, jdelvare, jglauber, david.daney,
peda, preid, linux-i2c, devicetree, linux-pm
i2c_smbus commands handle the correct byte order for smbus transactions
internally. This will currently result in incorrect operation on big
endian systems.
Signed-off-by: Phil Reid <preid@electromag.com.au>
---
drivers/power/supply/bq24735-charger.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/power/supply/bq24735-charger.c b/drivers/power/supply/bq24735-charger.c
index eb01453..6931e1d 100644
--- a/drivers/power/supply/bq24735-charger.c
+++ b/drivers/power/supply/bq24735-charger.c
@@ -81,14 +81,12 @@ static int bq24735_charger_property_is_writeable(struct power_supply *psy,
static inline int bq24735_write_word(struct i2c_client *client, u8 reg,
u16 value)
{
- return i2c_smbus_write_word_data(client, reg, le16_to_cpu(value));
+ return i2c_smbus_write_word_data(client, reg, value);
}
static inline int bq24735_read_word(struct i2c_client *client, u8 reg)
{
- s32 ret = i2c_smbus_read_word_data(client, reg);
-
- return ret < 0 ? ret : le16_to_cpu(ret);
+ return i2c_smbus_read_word_data(client, reg);
}
static int bq24735_update_word(struct i2c_client *client, u8 reg,
--
1.8.3.1
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PATCH v7 03/10] i2c: i2c-smbus: Use threaded irq for smbalert
2017-06-15 13:59 [PATCH v7 00/10] Add sbs-manager with smbalert support Phil Reid
2017-06-15 13:59 ` [PATCH v7 01/10] power: supply: sbs-battery: remove incorrect le16_to_cpu calls Phil Reid
2017-06-15 13:59 ` [PATCH v7 02/10] power: supply: bq24735: " Phil Reid
@ 2017-06-15 13:59 ` Phil Reid
2017-06-19 15:27 ` Wolfram Sang
2017-06-15 13:59 ` [PATCH v7 04/10] i2c: i2c-smbus: add of_i2c_setup_smbus_alert Phil Reid
` (6 subsequent siblings)
9 siblings, 1 reply; 30+ messages in thread
From: Phil Reid @ 2017-06-15 13:59 UTC (permalink / raw)
To: wsa, robh+dt, mark.rutland, sre, jdelvare, jglauber, david.daney,
peda, preid, linux-i2c, devicetree, linux-pm
Prior to this commit the smbalert_irq was handling in the hard irq
context. This change switch to using a thread irq which avoids the need
for the work thread. Using threaded irq also removes the need for the
edge_triggered flag as the enabling / disabling of the hard irq for level
triggered interrupts will be handled by the irq core.
Without this change have an irq connected to something like an i2c gpio
resulted in a null ptr deferences. Specifically handle_nested_irq calls
the threaded irq handler.
There are currently 3 in tree drivers affected by this change.
i2c-parport driver calls i2c_handle_smbus_alert in a hard irq context.
This driver use edge trigger interrupts which skip the enable / disable
calls. But it still need to handle the smbus transaction on a thread. So
the work thread is kept for this driver.
i2c-parport-light & i2c-thunderx-pcidrv provide the irq number in the
setup which will result in the thread irq being used.
i2c-parport-light is edge trigger so the enable / disable call was
skipped as well.
i2c-thunderx-pcidrv is getting the edge / level trigger setting from of
data and was setting the flag as required. However the irq core should
handle this automatically.
Signed-off-by: Phil Reid <preid@electromag.com.au>
---
drivers/i2c/busses/i2c-parport-light.c | 1 -
drivers/i2c/busses/i2c-parport.c | 1 -
drivers/i2c/busses/i2c-thunderx-pcidrv.c | 6 -----
drivers/i2c/i2c-smbus.c | 41 +++++++++++++-------------------
include/linux/i2c-smbus.h | 1 -
5 files changed, 17 insertions(+), 33 deletions(-)
diff --git a/drivers/i2c/busses/i2c-parport-light.c b/drivers/i2c/busses/i2c-parport-light.c
index 1bcdd10..e6e22b8 100644
--- a/drivers/i2c/busses/i2c-parport-light.c
+++ b/drivers/i2c/busses/i2c-parport-light.c
@@ -123,7 +123,6 @@ static int parport_getsda(void *data)
/* SMBus alert support */
static struct i2c_smbus_alert_setup alert_data = {
- .alert_edge_triggered = 1,
};
static struct i2c_client *ara;
static struct lineop parport_ctrl_irq = {
diff --git a/drivers/i2c/busses/i2c-parport.c b/drivers/i2c/busses/i2c-parport.c
index a8e54df..319209a 100644
--- a/drivers/i2c/busses/i2c-parport.c
+++ b/drivers/i2c/busses/i2c-parport.c
@@ -237,7 +237,6 @@ static void i2c_parport_attach(struct parport *port)
/* Setup SMBus alert if supported */
if (adapter_parm[type].smbus_alert) {
- adapter->alert_data.alert_edge_triggered = 1;
adapter->ara = i2c_setup_smbus_alert(&adapter->adapter,
&adapter->alert_data);
if (adapter->ara)
diff --git a/drivers/i2c/busses/i2c-thunderx-pcidrv.c b/drivers/i2c/busses/i2c-thunderx-pcidrv.c
index 1d4c2be..c27a50f 100644
--- a/drivers/i2c/busses/i2c-thunderx-pcidrv.c
+++ b/drivers/i2c/busses/i2c-thunderx-pcidrv.c
@@ -112,8 +112,6 @@ static void thunder_i2c_clock_disable(struct device *dev, struct clk *clk)
static int thunder_i2c_smbus_setup_of(struct octeon_i2c *i2c,
struct device_node *node)
{
- u32 type;
-
if (!node)
return -EINVAL;
@@ -121,10 +119,6 @@ static int thunder_i2c_smbus_setup_of(struct octeon_i2c *i2c,
if (!i2c->alert_data.irq)
return -EINVAL;
- type = irqd_get_trigger_type(irq_get_irq_data(i2c->alert_data.irq));
- i2c->alert_data.alert_edge_triggered =
- (type & IRQ_TYPE_LEVEL_MASK) ? 1 : 0;
-
i2c->ara = i2c_setup_smbus_alert(&i2c->adap, &i2c->alert_data);
if (!i2c->ara)
return -ENODEV;
diff --git a/drivers/i2c/i2c-smbus.c b/drivers/i2c/i2c-smbus.c
index f9271c7..d4af270 100644
--- a/drivers/i2c/i2c-smbus.c
+++ b/drivers/i2c/i2c-smbus.c
@@ -25,8 +25,6 @@
#include <linux/workqueue.h>
struct i2c_smbus_alert {
- unsigned int alert_edge_triggered:1;
- int irq;
struct work_struct alert;
struct i2c_client *ara; /* Alert response address */
};
@@ -72,13 +70,12 @@ static int smbus_do_alert(struct device *dev, void *addrp)
* The alert IRQ handler needs to hand work off to a task which can issue
* SMBus calls, because those sleeping calls can't be made in IRQ context.
*/
-static void smbus_alert(struct work_struct *work)
+static irqreturn_t smbus_alert(int irq, void *d)
{
- struct i2c_smbus_alert *alert;
+ struct i2c_smbus_alert *alert = d;
struct i2c_client *ara;
unsigned short prev_addr = 0; /* Not a valid address */
- alert = container_of(work, struct i2c_smbus_alert, alert);
ara = alert->ara;
for (;;) {
@@ -115,21 +112,17 @@ static void smbus_alert(struct work_struct *work)
prev_addr = data.addr;
}
- /* We handled all alerts; re-enable level-triggered IRQs */
- if (!alert->alert_edge_triggered)
- enable_irq(alert->irq);
+ return IRQ_HANDLED;
}
-static irqreturn_t smbalert_irq(int irq, void *d)
+static void smbalert_work(struct work_struct *work)
{
- struct i2c_smbus_alert *alert = d;
+ struct i2c_smbus_alert *alert;
+
+ alert = container_of(work, struct i2c_smbus_alert, alert);
- /* Disable level-triggered IRQs until we handle them */
- if (!alert->alert_edge_triggered)
- disable_irq_nosync(irq);
+ smbus_alert(0, alert);
- schedule_work(&alert->alert);
- return IRQ_HANDLED;
}
/* Setup SMBALERT# infrastructure */
@@ -139,28 +132,28 @@ static int smbalert_probe(struct i2c_client *ara,
struct i2c_smbus_alert_setup *setup = dev_get_platdata(&ara->dev);
struct i2c_smbus_alert *alert;
struct i2c_adapter *adapter = ara->adapter;
- int res;
+ int res, irq;
alert = devm_kzalloc(&ara->dev, sizeof(struct i2c_smbus_alert),
GFP_KERNEL);
if (!alert)
return -ENOMEM;
- alert->alert_edge_triggered = setup->alert_edge_triggered;
- alert->irq = setup->irq;
- INIT_WORK(&alert->alert, smbus_alert);
+ irq = setup->irq;
+ INIT_WORK(&alert->alert, smbalert_work);
alert->ara = ara;
- if (setup->irq > 0) {
- res = devm_request_irq(&ara->dev, setup->irq, smbalert_irq,
- 0, "smbus_alert", alert);
+ if (irq > 0) {
+ res = devm_request_threaded_irq(&ara->dev, irq,
+ NULL, smbus_alert,
+ IRQF_SHARED | IRQF_ONESHOT,
+ "smbus_alert", alert);
if (res)
return res;
}
i2c_set_clientdata(ara, alert);
- dev_info(&adapter->dev, "supports SMBALERT#, %s trigger\n",
- setup->alert_edge_triggered ? "edge" : "level");
+ dev_info(&adapter->dev, "supports SMBALERT#\n");
return 0;
}
diff --git a/include/linux/i2c-smbus.h b/include/linux/i2c-smbus.h
index a138502..19efbd1 100644
--- a/include/linux/i2c-smbus.h
+++ b/include/linux/i2c-smbus.h
@@ -42,7 +42,6 @@
* properly set.
*/
struct i2c_smbus_alert_setup {
- unsigned int alert_edge_triggered:1;
int irq;
};
--
1.8.3.1
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PATCH v7 03/10] i2c: i2c-smbus: Use threaded irq for smbalert
2017-06-15 13:59 ` [PATCH v7 03/10] i2c: i2c-smbus: Use threaded irq for smbalert Phil Reid
@ 2017-06-19 15:27 ` Wolfram Sang
2017-06-20 2:30 ` Phil Reid
2017-06-23 12:11 ` Benjamin Tissoires
0 siblings, 2 replies; 30+ messages in thread
From: Wolfram Sang @ 2017-06-19 15:27 UTC (permalink / raw)
To: Phil Reid, Benjamin Tissoires
Cc: robh+dt, mark.rutland, sre, jdelvare, jglauber, david.daney,
peda, linux-i2c, devicetree, linux-pm
[-- Attachment #1: Type: text/plain, Size: 7331 bytes --]
On Thu, Jun 15, 2017 at 09:59:31PM +0800, Phil Reid wrote:
> Prior to this commit the smbalert_irq was handling in the hard irq
> context. This change switch to using a thread irq which avoids the need
> for the work thread. Using threaded irq also removes the need for the
> edge_triggered flag as the enabling / disabling of the hard irq for level
> triggered interrupts will be handled by the irq core.
>
> Without this change have an irq connected to something like an i2c gpio
> resulted in a null ptr deferences. Specifically handle_nested_irq calls
> the threaded irq handler.
>
> There are currently 3 in tree drivers affected by this change.
>
> i2c-parport driver calls i2c_handle_smbus_alert in a hard irq context.
> This driver use edge trigger interrupts which skip the enable / disable
> calls. But it still need to handle the smbus transaction on a thread. So
> the work thread is kept for this driver.
>
> i2c-parport-light & i2c-thunderx-pcidrv provide the irq number in the
> setup which will result in the thread irq being used.
>
> i2c-parport-light is edge trigger so the enable / disable call was
> skipped as well.
>
> i2c-thunderx-pcidrv is getting the edge / level trigger setting from of
> data and was setting the flag as required. However the irq core should
> handle this automatically.
>
> Signed-off-by: Phil Reid <preid@electromag.com.au>
CCing Benjamin for smbus changes (like last time). Please CC him in the
future in case we need another revision of this series.
> ---
> drivers/i2c/busses/i2c-parport-light.c | 1 -
> drivers/i2c/busses/i2c-parport.c | 1 -
> drivers/i2c/busses/i2c-thunderx-pcidrv.c | 6 -----
> drivers/i2c/i2c-smbus.c | 41 +++++++++++++-------------------
> include/linux/i2c-smbus.h | 1 -
> 5 files changed, 17 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-parport-light.c b/drivers/i2c/busses/i2c-parport-light.c
> index 1bcdd10..e6e22b8 100644
> --- a/drivers/i2c/busses/i2c-parport-light.c
> +++ b/drivers/i2c/busses/i2c-parport-light.c
> @@ -123,7 +123,6 @@ static int parport_getsda(void *data)
>
> /* SMBus alert support */
> static struct i2c_smbus_alert_setup alert_data = {
> - .alert_edge_triggered = 1,
> };
> static struct i2c_client *ara;
> static struct lineop parport_ctrl_irq = {
> diff --git a/drivers/i2c/busses/i2c-parport.c b/drivers/i2c/busses/i2c-parport.c
> index a8e54df..319209a 100644
> --- a/drivers/i2c/busses/i2c-parport.c
> +++ b/drivers/i2c/busses/i2c-parport.c
> @@ -237,7 +237,6 @@ static void i2c_parport_attach(struct parport *port)
>
> /* Setup SMBus alert if supported */
> if (adapter_parm[type].smbus_alert) {
> - adapter->alert_data.alert_edge_triggered = 1;
> adapter->ara = i2c_setup_smbus_alert(&adapter->adapter,
> &adapter->alert_data);
> if (adapter->ara)
> diff --git a/drivers/i2c/busses/i2c-thunderx-pcidrv.c b/drivers/i2c/busses/i2c-thunderx-pcidrv.c
> index 1d4c2be..c27a50f 100644
> --- a/drivers/i2c/busses/i2c-thunderx-pcidrv.c
> +++ b/drivers/i2c/busses/i2c-thunderx-pcidrv.c
> @@ -112,8 +112,6 @@ static void thunder_i2c_clock_disable(struct device *dev, struct clk *clk)
> static int thunder_i2c_smbus_setup_of(struct octeon_i2c *i2c,
> struct device_node *node)
> {
> - u32 type;
> -
> if (!node)
> return -EINVAL;
>
> @@ -121,10 +119,6 @@ static int thunder_i2c_smbus_setup_of(struct octeon_i2c *i2c,
> if (!i2c->alert_data.irq)
> return -EINVAL;
>
> - type = irqd_get_trigger_type(irq_get_irq_data(i2c->alert_data.irq));
> - i2c->alert_data.alert_edge_triggered =
> - (type & IRQ_TYPE_LEVEL_MASK) ? 1 : 0;
> -
> i2c->ara = i2c_setup_smbus_alert(&i2c->adap, &i2c->alert_data);
> if (!i2c->ara)
> return -ENODEV;
> diff --git a/drivers/i2c/i2c-smbus.c b/drivers/i2c/i2c-smbus.c
> index f9271c7..d4af270 100644
> --- a/drivers/i2c/i2c-smbus.c
> +++ b/drivers/i2c/i2c-smbus.c
> @@ -25,8 +25,6 @@
> #include <linux/workqueue.h>
>
> struct i2c_smbus_alert {
> - unsigned int alert_edge_triggered:1;
> - int irq;
> struct work_struct alert;
> struct i2c_client *ara; /* Alert response address */
> };
> @@ -72,13 +70,12 @@ static int smbus_do_alert(struct device *dev, void *addrp)
> * The alert IRQ handler needs to hand work off to a task which can issue
> * SMBus calls, because those sleeping calls can't be made in IRQ context.
> */
> -static void smbus_alert(struct work_struct *work)
> +static irqreturn_t smbus_alert(int irq, void *d)
> {
> - struct i2c_smbus_alert *alert;
> + struct i2c_smbus_alert *alert = d;
> struct i2c_client *ara;
> unsigned short prev_addr = 0; /* Not a valid address */
>
> - alert = container_of(work, struct i2c_smbus_alert, alert);
> ara = alert->ara;
>
> for (;;) {
> @@ -115,21 +112,17 @@ static void smbus_alert(struct work_struct *work)
> prev_addr = data.addr;
> }
>
> - /* We handled all alerts; re-enable level-triggered IRQs */
> - if (!alert->alert_edge_triggered)
> - enable_irq(alert->irq);
> + return IRQ_HANDLED;
> }
>
> -static irqreturn_t smbalert_irq(int irq, void *d)
> +static void smbalert_work(struct work_struct *work)
> {
> - struct i2c_smbus_alert *alert = d;
> + struct i2c_smbus_alert *alert;
> +
> + alert = container_of(work, struct i2c_smbus_alert, alert);
>
> - /* Disable level-triggered IRQs until we handle them */
> - if (!alert->alert_edge_triggered)
> - disable_irq_nosync(irq);
> + smbus_alert(0, alert);
>
> - schedule_work(&alert->alert);
> - return IRQ_HANDLED;
> }
>
> /* Setup SMBALERT# infrastructure */
> @@ -139,28 +132,28 @@ static int smbalert_probe(struct i2c_client *ara,
> struct i2c_smbus_alert_setup *setup = dev_get_platdata(&ara->dev);
> struct i2c_smbus_alert *alert;
> struct i2c_adapter *adapter = ara->adapter;
> - int res;
> + int res, irq;
>
> alert = devm_kzalloc(&ara->dev, sizeof(struct i2c_smbus_alert),
> GFP_KERNEL);
> if (!alert)
> return -ENOMEM;
>
> - alert->alert_edge_triggered = setup->alert_edge_triggered;
> - alert->irq = setup->irq;
> - INIT_WORK(&alert->alert, smbus_alert);
> + irq = setup->irq;
> + INIT_WORK(&alert->alert, smbalert_work);
> alert->ara = ara;
>
> - if (setup->irq > 0) {
> - res = devm_request_irq(&ara->dev, setup->irq, smbalert_irq,
> - 0, "smbus_alert", alert);
> + if (irq > 0) {
> + res = devm_request_threaded_irq(&ara->dev, irq,
> + NULL, smbus_alert,
> + IRQF_SHARED | IRQF_ONESHOT,
> + "smbus_alert", alert);
> if (res)
> return res;
> }
>
> i2c_set_clientdata(ara, alert);
> - dev_info(&adapter->dev, "supports SMBALERT#, %s trigger\n",
> - setup->alert_edge_triggered ? "edge" : "level");
> + dev_info(&adapter->dev, "supports SMBALERT#\n");
>
> return 0;
> }
> diff --git a/include/linux/i2c-smbus.h b/include/linux/i2c-smbus.h
> index a138502..19efbd1 100644
> --- a/include/linux/i2c-smbus.h
> +++ b/include/linux/i2c-smbus.h
> @@ -42,7 +42,6 @@
> * properly set.
> */
> struct i2c_smbus_alert_setup {
> - unsigned int alert_edge_triggered:1;
> int irq;
> };
>
> --
> 1.8.3.1
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH v7 03/10] i2c: i2c-smbus: Use threaded irq for smbalert
2017-06-19 15:27 ` Wolfram Sang
@ 2017-06-20 2:30 ` Phil Reid
2017-06-23 12:11 ` Benjamin Tissoires
1 sibling, 0 replies; 30+ messages in thread
From: Phil Reid @ 2017-06-20 2:30 UTC (permalink / raw)
To: Wolfram Sang, Benjamin Tissoires
Cc: robh+dt, mark.rutland, sre, jdelvare, jglauber, david.daney,
peda, linux-i2c, devicetree, linux-pm
On 19/06/2017 23:27, Wolfram Sang wrote:
> On Thu, Jun 15, 2017 at 09:59:31PM +0800, Phil Reid wrote:
>> Prior to this commit the smbalert_irq was handling in the hard irq
>> context. This change switch to using a thread irq which avoids the need
>> for the work thread. Using threaded irq also removes the need for the
>> edge_triggered flag as the enabling / disabling of the hard irq for level
>> triggered interrupts will be handled by the irq core.
>>
>> Without this change have an irq connected to something like an i2c gpio
>> resulted in a null ptr deferences. Specifically handle_nested_irq calls
>> the threaded irq handler.
>>
>> There are currently 3 in tree drivers affected by this change.
>>
>> i2c-parport driver calls i2c_handle_smbus_alert in a hard irq context.
>> This driver use edge trigger interrupts which skip the enable / disable
>> calls. But it still need to handle the smbus transaction on a thread. So
>> the work thread is kept for this driver.
>>
>> i2c-parport-light & i2c-thunderx-pcidrv provide the irq number in the
>> setup which will result in the thread irq being used.
>>
>> i2c-parport-light is edge trigger so the enable / disable call was
>> skipped as well.
>>
>> i2c-thunderx-pcidrv is getting the edge / level trigger setting from of
>> data and was setting the flag as required. However the irq core should
>> handle this automatically.
>>
>> Signed-off-by: Phil Reid <preid@electromag.com.au>
>
> CCing Benjamin for smbus changes (like last time). Please CC him in the
> future in case we need another revision of this series.
>
Sorry forgot he was added manually.
I just used the get maintainers script to get the list.
And he wasn't listed against the i2c / smbus paths.
--
Regards
Phil Reid
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v7 03/10] i2c: i2c-smbus: Use threaded irq for smbalert
2017-06-19 15:27 ` Wolfram Sang
2017-06-20 2:30 ` Phil Reid
@ 2017-06-23 12:11 ` Benjamin Tissoires
1 sibling, 0 replies; 30+ messages in thread
From: Benjamin Tissoires @ 2017-06-23 12:11 UTC (permalink / raw)
To: Wolfram Sang
Cc: Phil Reid, robh+dt, mark.rutland, sre, jdelvare, jglauber,
david.daney, peda, linux-i2c, devicetree, linux-pm
Hi,
On Jun 19 2017 or thereabouts, Wolfram Sang wrote:
> On Thu, Jun 15, 2017 at 09:59:31PM +0800, Phil Reid wrote:
> > Prior to this commit the smbalert_irq was handling in the hard irq
> > context. This change switch to using a thread irq which avoids the need
> > for the work thread. Using threaded irq also removes the need for the
> > edge_triggered flag as the enabling / disabling of the hard irq for level
> > triggered interrupts will be handled by the irq core.
> >
> > Without this change have an irq connected to something like an i2c gpio
> > resulted in a null ptr deferences. Specifically handle_nested_irq calls
> > the threaded irq handler.
> >
> > There are currently 3 in tree drivers affected by this change.
> >
> > i2c-parport driver calls i2c_handle_smbus_alert in a hard irq context.
> > This driver use edge trigger interrupts which skip the enable / disable
> > calls. But it still need to handle the smbus transaction on a thread. So
> > the work thread is kept for this driver.
> >
> > i2c-parport-light & i2c-thunderx-pcidrv provide the irq number in the
> > setup which will result in the thread irq being used.
> >
> > i2c-parport-light is edge trigger so the enable / disable call was
> > skipped as well.
> >
> > i2c-thunderx-pcidrv is getting the edge / level trigger setting from of
> > data and was setting the flag as required. However the irq core should
> > handle this automatically.
> >
> > Signed-off-by: Phil Reid <preid@electromag.com.au>
>
> CCing Benjamin for smbus changes (like last time). Please CC him in the
> future in case we need another revision of this series.
Thanks Wolfram.
I wonder if we can not have something similar than what we do for host notify
for removing the worker all together:
In host notify, we request a new IRQ number, and when the Host Notify
function is called, we simply call generic_handle_irq(). Switching
i2c-parport to something similar could help us remove this worker
entirely. But given this change would require to actually have a device
handled by i2c-parport, we might postpone this later.
Other than that, the patch looks good to me. The commit message is a
little bit messy IMO, and it took me a while to understand all the
subtleties (most of the issues raised in the messaged are simpler to
understand when reading the code).
Anyway, not a big deal, this one is:
Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
Cheers,
Benjamin
>
> > ---
> > drivers/i2c/busses/i2c-parport-light.c | 1 -
> > drivers/i2c/busses/i2c-parport.c | 1 -
> > drivers/i2c/busses/i2c-thunderx-pcidrv.c | 6 -----
> > drivers/i2c/i2c-smbus.c | 41 +++++++++++++-------------------
> > include/linux/i2c-smbus.h | 1 -
> > 5 files changed, 17 insertions(+), 33 deletions(-)
> >
> > diff --git a/drivers/i2c/busses/i2c-parport-light.c b/drivers/i2c/busses/i2c-parport-light.c
> > index 1bcdd10..e6e22b8 100644
> > --- a/drivers/i2c/busses/i2c-parport-light.c
> > +++ b/drivers/i2c/busses/i2c-parport-light.c
> > @@ -123,7 +123,6 @@ static int parport_getsda(void *data)
> >
> > /* SMBus alert support */
> > static struct i2c_smbus_alert_setup alert_data = {
> > - .alert_edge_triggered = 1,
> > };
> > static struct i2c_client *ara;
> > static struct lineop parport_ctrl_irq = {
> > diff --git a/drivers/i2c/busses/i2c-parport.c b/drivers/i2c/busses/i2c-parport.c
> > index a8e54df..319209a 100644
> > --- a/drivers/i2c/busses/i2c-parport.c
> > +++ b/drivers/i2c/busses/i2c-parport.c
> > @@ -237,7 +237,6 @@ static void i2c_parport_attach(struct parport *port)
> >
> > /* Setup SMBus alert if supported */
> > if (adapter_parm[type].smbus_alert) {
> > - adapter->alert_data.alert_edge_triggered = 1;
> > adapter->ara = i2c_setup_smbus_alert(&adapter->adapter,
> > &adapter->alert_data);
> > if (adapter->ara)
> > diff --git a/drivers/i2c/busses/i2c-thunderx-pcidrv.c b/drivers/i2c/busses/i2c-thunderx-pcidrv.c
> > index 1d4c2be..c27a50f 100644
> > --- a/drivers/i2c/busses/i2c-thunderx-pcidrv.c
> > +++ b/drivers/i2c/busses/i2c-thunderx-pcidrv.c
> > @@ -112,8 +112,6 @@ static void thunder_i2c_clock_disable(struct device *dev, struct clk *clk)
> > static int thunder_i2c_smbus_setup_of(struct octeon_i2c *i2c,
> > struct device_node *node)
> > {
> > - u32 type;
> > -
> > if (!node)
> > return -EINVAL;
> >
> > @@ -121,10 +119,6 @@ static int thunder_i2c_smbus_setup_of(struct octeon_i2c *i2c,
> > if (!i2c->alert_data.irq)
> > return -EINVAL;
> >
> > - type = irqd_get_trigger_type(irq_get_irq_data(i2c->alert_data.irq));
> > - i2c->alert_data.alert_edge_triggered =
> > - (type & IRQ_TYPE_LEVEL_MASK) ? 1 : 0;
> > -
> > i2c->ara = i2c_setup_smbus_alert(&i2c->adap, &i2c->alert_data);
> > if (!i2c->ara)
> > return -ENODEV;
> > diff --git a/drivers/i2c/i2c-smbus.c b/drivers/i2c/i2c-smbus.c
> > index f9271c7..d4af270 100644
> > --- a/drivers/i2c/i2c-smbus.c
> > +++ b/drivers/i2c/i2c-smbus.c
> > @@ -25,8 +25,6 @@
> > #include <linux/workqueue.h>
> >
> > struct i2c_smbus_alert {
> > - unsigned int alert_edge_triggered:1;
> > - int irq;
> > struct work_struct alert;
> > struct i2c_client *ara; /* Alert response address */
> > };
> > @@ -72,13 +70,12 @@ static int smbus_do_alert(struct device *dev, void *addrp)
> > * The alert IRQ handler needs to hand work off to a task which can issue
> > * SMBus calls, because those sleeping calls can't be made in IRQ context.
> > */
> > -static void smbus_alert(struct work_struct *work)
> > +static irqreturn_t smbus_alert(int irq, void *d)
> > {
> > - struct i2c_smbus_alert *alert;
> > + struct i2c_smbus_alert *alert = d;
> > struct i2c_client *ara;
> > unsigned short prev_addr = 0; /* Not a valid address */
> >
> > - alert = container_of(work, struct i2c_smbus_alert, alert);
> > ara = alert->ara;
> >
> > for (;;) {
> > @@ -115,21 +112,17 @@ static void smbus_alert(struct work_struct *work)
> > prev_addr = data.addr;
> > }
> >
> > - /* We handled all alerts; re-enable level-triggered IRQs */
> > - if (!alert->alert_edge_triggered)
> > - enable_irq(alert->irq);
> > + return IRQ_HANDLED;
> > }
> >
> > -static irqreturn_t smbalert_irq(int irq, void *d)
> > +static void smbalert_work(struct work_struct *work)
> > {
> > - struct i2c_smbus_alert *alert = d;
> > + struct i2c_smbus_alert *alert;
> > +
> > + alert = container_of(work, struct i2c_smbus_alert, alert);
> >
> > - /* Disable level-triggered IRQs until we handle them */
> > - if (!alert->alert_edge_triggered)
> > - disable_irq_nosync(irq);
> > + smbus_alert(0, alert);
> >
> > - schedule_work(&alert->alert);
> > - return IRQ_HANDLED;
> > }
> >
> > /* Setup SMBALERT# infrastructure */
> > @@ -139,28 +132,28 @@ static int smbalert_probe(struct i2c_client *ara,
> > struct i2c_smbus_alert_setup *setup = dev_get_platdata(&ara->dev);
> > struct i2c_smbus_alert *alert;
> > struct i2c_adapter *adapter = ara->adapter;
> > - int res;
> > + int res, irq;
> >
> > alert = devm_kzalloc(&ara->dev, sizeof(struct i2c_smbus_alert),
> > GFP_KERNEL);
> > if (!alert)
> > return -ENOMEM;
> >
> > - alert->alert_edge_triggered = setup->alert_edge_triggered;
> > - alert->irq = setup->irq;
> > - INIT_WORK(&alert->alert, smbus_alert);
> > + irq = setup->irq;
> > + INIT_WORK(&alert->alert, smbalert_work);
> > alert->ara = ara;
> >
> > - if (setup->irq > 0) {
> > - res = devm_request_irq(&ara->dev, setup->irq, smbalert_irq,
> > - 0, "smbus_alert", alert);
> > + if (irq > 0) {
> > + res = devm_request_threaded_irq(&ara->dev, irq,
> > + NULL, smbus_alert,
> > + IRQF_SHARED | IRQF_ONESHOT,
> > + "smbus_alert", alert);
> > if (res)
> > return res;
> > }
> >
> > i2c_set_clientdata(ara, alert);
> > - dev_info(&adapter->dev, "supports SMBALERT#, %s trigger\n",
> > - setup->alert_edge_triggered ? "edge" : "level");
> > + dev_info(&adapter->dev, "supports SMBALERT#\n");
> >
> > return 0;
> > }
> > diff --git a/include/linux/i2c-smbus.h b/include/linux/i2c-smbus.h
> > index a138502..19efbd1 100644
> > --- a/include/linux/i2c-smbus.h
> > +++ b/include/linux/i2c-smbus.h
> > @@ -42,7 +42,6 @@
> > * properly set.
> > */
> > struct i2c_smbus_alert_setup {
> > - unsigned int alert_edge_triggered:1;
> > int irq;
> > };
> >
> > --
> > 1.8.3.1
> >
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v7 04/10] i2c: i2c-smbus: add of_i2c_setup_smbus_alert
2017-06-15 13:59 [PATCH v7 00/10] Add sbs-manager with smbalert support Phil Reid
` (2 preceding siblings ...)
2017-06-15 13:59 ` [PATCH v7 03/10] i2c: i2c-smbus: Use threaded irq for smbalert Phil Reid
@ 2017-06-15 13:59 ` Phil Reid
[not found] ` <1497535178-12001-5-git-send-email-preid-qgqNFa1JUf/o2iN0hyhwsIdd74u8MsAO@public.gmane.org>
2017-06-15 13:59 ` [PATCH v7 05/10] i2c: core: call of_i2c_setup_smbus_alert in i2c_register_adapter Phil Reid
` (5 subsequent siblings)
9 siblings, 1 reply; 30+ messages in thread
From: Phil Reid @ 2017-06-15 13:59 UTC (permalink / raw)
To: wsa, robh+dt, mark.rutland, sre, jdelvare, jglauber, david.daney,
peda, preid, linux-i2c, devicetree, linux-pm
This commit adds of_i2c_setup_smbus_alert which allows the smbalert
driver to be attached to an i2c adapter via the device tree.
Signed-off-by: Phil Reid <preid@electromag.com.au>
Acked-by: Rob Herring <robh@kernel.org>
---
Documentation/devicetree/bindings/i2c/i2c.txt | 4 ++--
drivers/i2c/i2c-smbus.c | 34 ++++++++++++++++++++++++---
include/linux/i2c-smbus.h | 9 +++++++
3 files changed, 42 insertions(+), 5 deletions(-)
diff --git a/Documentation/devicetree/bindings/i2c/i2c.txt b/Documentation/devicetree/bindings/i2c/i2c.txt
index cee9d50..1126398 100644
--- a/Documentation/devicetree/bindings/i2c/i2c.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c.txt
@@ -59,8 +59,8 @@ wants to support one of the below features, it should adapt the bindings below.
interrupts used by the device.
- interrupt-names
- "irq" and "wakeup" names are recognized by I2C core, other names are
- left to individual drivers.
+ "irq", "wakeup" and "smbus_alert" names are recognized by I2C core,
+ other names are left to individual drivers.
- host-notify
device uses SMBus host notify protocol instead of interrupt line.
diff --git a/drivers/i2c/i2c-smbus.c b/drivers/i2c/i2c-smbus.c
index d4af270..0ad7f7f 100644
--- a/drivers/i2c/i2c-smbus.c
+++ b/drivers/i2c/i2c-smbus.c
@@ -21,6 +21,7 @@
#include <linux/interrupt.h>
#include <linux/kernel.h>
#include <linux/module.h>
+#include <linux/of_irq.h>
#include <linux/slab.h>
#include <linux/workqueue.h>
@@ -131,7 +132,7 @@ static int smbalert_probe(struct i2c_client *ara,
{
struct i2c_smbus_alert_setup *setup = dev_get_platdata(&ara->dev);
struct i2c_smbus_alert *alert;
- struct i2c_adapter *adapter = ara->adapter;
+ struct i2c_adapter *adap = ara->adapter;
int res, irq;
alert = devm_kzalloc(&ara->dev, sizeof(struct i2c_smbus_alert),
@@ -139,7 +140,14 @@ static int smbalert_probe(struct i2c_client *ara,
if (!alert)
return -ENOMEM;
- irq = setup->irq;
+ if (setup) {
+ irq = setup->irq;
+ } else {
+ irq = of_irq_get_byname(adap->dev.of_node, "smbus_alert");
+ if (irq <= 0)
+ return irq;
+ }
+
INIT_WORK(&alert->alert, smbalert_work);
alert->ara = ara;
@@ -153,7 +161,7 @@ static int smbalert_probe(struct i2c_client *ara,
}
i2c_set_clientdata(ara, alert);
- dev_info(&adapter->dev, "supports SMBALERT#\n");
+ dev_info(&adap->dev, "supports SMBALERT#\n");
return 0;
}
@@ -214,6 +222,26 @@ struct i2c_client *i2c_setup_smbus_alert(struct i2c_adapter *adapter,
}
EXPORT_SYMBOL_GPL(i2c_setup_smbus_alert);
+int of_i2c_setup_smbus_alert(struct i2c_adapter *adap)
+{
+ struct i2c_client *client;
+ int irq;
+
+ irq = of_property_match_string(adap->dev.of_node, "interrupt-names",
+ "smbus_alert");
+ if (irq == -EINVAL || irq == -ENODATA)
+ return 0;
+ else if (irq < 0)
+ return irq;
+
+ client = i2c_setup_smbus_alert(adap, NULL);
+ if (!client)
+ return -ENODEV;
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(of_i2c_setup_smbus_alert);
+
/**
* i2c_handle_smbus_alert - Handle an SMBus alert
* @ara: the ARA client on the relevant adapter
diff --git a/include/linux/i2c-smbus.h b/include/linux/i2c-smbus.h
index 19efbd1..b5261c1 100644
--- a/include/linux/i2c-smbus.h
+++ b/include/linux/i2c-smbus.h
@@ -49,4 +49,13 @@ struct i2c_client *i2c_setup_smbus_alert(struct i2c_adapter *adapter,
struct i2c_smbus_alert_setup *setup);
int i2c_handle_smbus_alert(struct i2c_client *ara);
+#if IS_ENABLED(CONFIG_I2C_SMBUS)
+int of_i2c_setup_smbus_alert(struct i2c_adapter *adap);
+#else
+static inline int of_i2c_setup_smbus_alert(struct i2c_adapter *adap)
+{
+ return 0;
+}
+#endif
+
#endif /* _LINUX_I2C_SMBUS_H */
--
1.8.3.1
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PATCH v7 05/10] i2c: core: call of_i2c_setup_smbus_alert in i2c_register_adapter
2017-06-15 13:59 [PATCH v7 00/10] Add sbs-manager with smbalert support Phil Reid
` (3 preceding siblings ...)
2017-06-15 13:59 ` [PATCH v7 04/10] i2c: i2c-smbus: add of_i2c_setup_smbus_alert Phil Reid
@ 2017-06-15 13:59 ` Phil Reid
2017-06-19 15:28 ` Wolfram Sang
2017-06-15 13:59 ` [PATCH v7 06/10] i2c: mux: pca954x: Call request irq after adding mux segments Phil Reid
` (4 subsequent siblings)
9 siblings, 1 reply; 30+ messages in thread
From: Phil Reid @ 2017-06-15 13:59 UTC (permalink / raw)
To: wsa, robh+dt, mark.rutland, sre, jdelvare, jglauber, david.daney,
peda, preid, linux-i2c, devicetree, linux-pm
Add a call to of_i2c_setup_smbus_alert when a i2c adapter is registered
so the the smbalert driver can be registered.
Signed-off-by: Phil Reid <preid@electromag.com.au>
---
drivers/i2c/i2c-core.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index d2402bb..626471b 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -40,6 +40,7 @@
#include <linux/gpio.h>
#include <linux/hardirq.h>
#include <linux/i2c.h>
+#include <linux/i2c-smbus.h>
#include <linux/idr.h>
#include <linux/init.h>
#include <linux/irqflags.h>
@@ -2045,6 +2046,9 @@ static int i2c_register_adapter(struct i2c_adapter *adap)
dev_warn(&adap->dev,
"Failed to create compatibility class link\n");
#endif
+ res = of_i2c_setup_smbus_alert(adap);
+ if (res)
+ goto out_list;
i2c_init_recovery(adap);
--
1.8.3.1
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PATCH v7 05/10] i2c: core: call of_i2c_setup_smbus_alert in i2c_register_adapter
2017-06-15 13:59 ` [PATCH v7 05/10] i2c: core: call of_i2c_setup_smbus_alert in i2c_register_adapter Phil Reid
@ 2017-06-19 15:28 ` Wolfram Sang
2017-06-23 12:19 ` Benjamin Tissoires
0 siblings, 1 reply; 30+ messages in thread
From: Wolfram Sang @ 2017-06-19 15:28 UTC (permalink / raw)
To: Phil Reid, Benjamin Tissoires
Cc: robh+dt, mark.rutland, sre, jdelvare, jglauber, david.daney,
peda, linux-i2c, devicetree, linux-pm
[-- Attachment #1: Type: text/plain, Size: 1050 bytes --]
On Thu, Jun 15, 2017 at 09:59:33PM +0800, Phil Reid wrote:
> Add a call to of_i2c_setup_smbus_alert when a i2c adapter is registered
> so the the smbalert driver can be registered.
>
> Signed-off-by: Phil Reid <preid@electromag.com.au>
CCing Benjamin
> ---
> drivers/i2c/i2c-core.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> index d2402bb..626471b 100644
> --- a/drivers/i2c/i2c-core.c
> +++ b/drivers/i2c/i2c-core.c
> @@ -40,6 +40,7 @@
> #include <linux/gpio.h>
> #include <linux/hardirq.h>
> #include <linux/i2c.h>
> +#include <linux/i2c-smbus.h>
> #include <linux/idr.h>
> #include <linux/init.h>
> #include <linux/irqflags.h>
> @@ -2045,6 +2046,9 @@ static int i2c_register_adapter(struct i2c_adapter *adap)
> dev_warn(&adap->dev,
> "Failed to create compatibility class link\n");
> #endif
> + res = of_i2c_setup_smbus_alert(adap);
> + if (res)
> + goto out_list;
>
> i2c_init_recovery(adap);
>
> --
> 1.8.3.1
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v7 05/10] i2c: core: call of_i2c_setup_smbus_alert in i2c_register_adapter
2017-06-19 15:28 ` Wolfram Sang
@ 2017-06-23 12:19 ` Benjamin Tissoires
2017-06-28 6:44 ` Phil Reid
0 siblings, 1 reply; 30+ messages in thread
From: Benjamin Tissoires @ 2017-06-23 12:19 UTC (permalink / raw)
To: Wolfram Sang
Cc: Phil Reid, robh+dt, mark.rutland, sre, jdelvare, jglauber,
david.daney, peda, linux-i2c, devicetree, linux-pm
On Jun 19 2017 or thereabouts, Wolfram Sang wrote:
> On Thu, Jun 15, 2017 at 09:59:33PM +0800, Phil Reid wrote:
> > Add a call to of_i2c_setup_smbus_alert when a i2c adapter is registered
> > so the the smbalert driver can be registered.
> >
> > Signed-off-by: Phil Reid <preid@electromag.com.au>
>
> CCing Benjamin
>
> > ---
> > drivers/i2c/i2c-core.c | 4 ++++
> > 1 file changed, 4 insertions(+)
> >
> > diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> > index d2402bb..626471b 100644
> > --- a/drivers/i2c/i2c-core.c
> > +++ b/drivers/i2c/i2c-core.c
> > @@ -40,6 +40,7 @@
> > #include <linux/gpio.h>
> > #include <linux/hardirq.h>
> > #include <linux/i2c.h>
> > +#include <linux/i2c-smbus.h>
> > #include <linux/idr.h>
> > #include <linux/init.h>
> > #include <linux/irqflags.h>
> > @@ -2045,6 +2046,9 @@ static int i2c_register_adapter(struct i2c_adapter *adap)
> > dev_warn(&adap->dev,
> > "Failed to create compatibility class link\n");
> > #endif
> > + res = of_i2c_setup_smbus_alert(adap);
> > + if (res)
> > + goto out_list;
See my concerns in patch 4/10.
In addition, shouldn't this be placed before device_register() for the
least? pm_runtime_enable() would require a matching pm_runtime_disable(),
and device_register() some unregistering behavior too.
Cheers,
Benjamin
> >
> > i2c_init_recovery(adap);
> >
> > --
> > 1.8.3.1
> >
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v7 05/10] i2c: core: call of_i2c_setup_smbus_alert in i2c_register_adapter
2017-06-23 12:19 ` Benjamin Tissoires
@ 2017-06-28 6:44 ` Phil Reid
2017-06-28 12:45 ` Benjamin Tissoires
0 siblings, 1 reply; 30+ messages in thread
From: Phil Reid @ 2017-06-28 6:44 UTC (permalink / raw)
To: Benjamin Tissoires, Wolfram Sang
Cc: robh+dt, mark.rutland, sre, jdelvare, jglauber, david.daney,
peda, linux-i2c, devicetree, linux-pm
On 23/06/2017 20:19, Benjamin Tissoires wrote:
> On Jun 19 2017 or thereabouts, Wolfram Sang wrote:
>> On Thu, Jun 15, 2017 at 09:59:33PM +0800, Phil Reid wrote:
>>> Add a call to of_i2c_setup_smbus_alert when a i2c adapter is registered
>>> so the the smbalert driver can be registered.
>>>
>>> Signed-off-by: Phil Reid <preid@electromag.com.au>
>>
>> CCing Benjamin
>>
>>> ---
>>> drivers/i2c/i2c-core.c | 4 ++++
>>> 1 file changed, 4 insertions(+)
>>>
>>> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
>>> index d2402bb..626471b 100644
>>> --- a/drivers/i2c/i2c-core.c
>>> +++ b/drivers/i2c/i2c-core.c
>>> @@ -40,6 +40,7 @@
>>> #include <linux/gpio.h>
>>> #include <linux/hardirq.h>
>>> #include <linux/i2c.h>
>>> +#include <linux/i2c-smbus.h>
>>> #include <linux/idr.h>
>>> #include <linux/init.h>
>>> #include <linux/irqflags.h>
>>> @@ -2045,6 +2046,9 @@ static int i2c_register_adapter(struct i2c_adapter *adap)
>>> dev_warn(&adap->dev,
>>> "Failed to create compatibility class link\n");
>>> #endif
>>> + res = of_i2c_setup_smbus_alert(adap);
>>> + if (res)
>>> + goto out_list;
>
> See my concerns in patch 4/10.
>
> In addition, shouldn't this be placed before device_register() for the
> least? pm_runtime_enable() would require a matching pm_runtime_disable(),
> and device_register() some unregistering behavior too.
>
G'day Ben,
Thanks for the review.
Yes this makes sense. I tried having it before the device_register and I get an error
about a kobject not being initialised in the a call from of_i2c_setup_smbus_alert.
Having a look at what I'm doing in of_i2c_setup_smbus_alert now I'm not sure there's
a need to bail out on an error now. Originally I was registering the irq in the setup call.
Which need to handle probe defer. Now this should be handled in the alert probe call.
WDYR?
--
Regards
Phil Reid
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v7 05/10] i2c: core: call of_i2c_setup_smbus_alert in i2c_register_adapter
2017-06-28 6:44 ` Phil Reid
@ 2017-06-28 12:45 ` Benjamin Tissoires
2017-07-11 7:52 ` Phil Reid
0 siblings, 1 reply; 30+ messages in thread
From: Benjamin Tissoires @ 2017-06-28 12:45 UTC (permalink / raw)
To: Phil Reid
Cc: Wolfram Sang, robh+dt, mark.rutland, sre, jdelvare, jglauber,
david.daney, peda, linux-i2c, devicetree, linux-pm
On Jun 28 2017 or thereabouts, Phil Reid wrote:
> On 23/06/2017 20:19, Benjamin Tissoires wrote:
> > On Jun 19 2017 or thereabouts, Wolfram Sang wrote:
> > > On Thu, Jun 15, 2017 at 09:59:33PM +0800, Phil Reid wrote:
> > > > Add a call to of_i2c_setup_smbus_alert when a i2c adapter is registered
> > > > so the the smbalert driver can be registered.
> > > >
> > > > Signed-off-by: Phil Reid <preid@electromag.com.au>
> > >
> > > CCing Benjamin
> > >
> > > > ---
> > > > drivers/i2c/i2c-core.c | 4 ++++
> > > > 1 file changed, 4 insertions(+)
> > > >
> > > > diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> > > > index d2402bb..626471b 100644
> > > > --- a/drivers/i2c/i2c-core.c
> > > > +++ b/drivers/i2c/i2c-core.c
> > > > @@ -40,6 +40,7 @@
> > > > #include <linux/gpio.h>
> > > > #include <linux/hardirq.h>
> > > > #include <linux/i2c.h>
> > > > +#include <linux/i2c-smbus.h>
> > > > #include <linux/idr.h>
> > > > #include <linux/init.h>
> > > > #include <linux/irqflags.h>
> > > > @@ -2045,6 +2046,9 @@ static int i2c_register_adapter(struct i2c_adapter *adap)
> > > > dev_warn(&adap->dev,
> > > > "Failed to create compatibility class link\n");
> > > > #endif
> > > > + res = of_i2c_setup_smbus_alert(adap);
> > > > + if (res)
> > > > + goto out_list;
> >
> > See my concerns in patch 4/10.
> >
> > In addition, shouldn't this be placed before device_register() for the
> > least? pm_runtime_enable() would require a matching pm_runtime_disable(),
> > and device_register() some unregistering behavior too.
> >
>
> G'day Ben,
>
> Thanks for the review.
> Yes this makes sense. I tried having it before the device_register and I get an error
> about a kobject not being initialised in the a call from of_i2c_setup_smbus_alert.
>
> Having a look at what I'm doing in of_i2c_setup_smbus_alert now I'm not sure there's
> a need to bail out on an error now. Originally I was registering the irq in the setup call.
> Which need to handle probe defer. Now this should be handled in the alert probe call.
>
> WDYR?
Well, of_i2c_setup_smbus_alert() returns an error code, so it has to be
accounted for. If the error is not a reason to fail, you should at least
shout some error messages and act accordingly.
However, looking at of_i2c_setup_smbus_alert() again, I wonder if we
should not split it in 2: one part that checks for the OF flag presence
and bail out early if an error is encountered (before device
registration), and one part once the device is registered that calls
i2c_setup_smbus_alert(). In case of a failure here, we need to
unregister the adapter because we don't have all the elements at hands
(assuming we consider that smbus-alert should be mandatory).
Cheers,
Benjamin
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v7 05/10] i2c: core: call of_i2c_setup_smbus_alert in i2c_register_adapter
2017-06-28 12:45 ` Benjamin Tissoires
@ 2017-07-11 7:52 ` Phil Reid
2017-07-21 9:24 ` Benjamin Tissoires
0 siblings, 1 reply; 30+ messages in thread
From: Phil Reid @ 2017-07-11 7:52 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Wolfram Sang, robh+dt, mark.rutland, sre, jdelvare, jglauber,
david.daney, peda, linux-i2c, devicetree, linux-pm
On 28/06/2017 20:45, Benjamin Tissoires wrote:
> On Jun 28 2017 or thereabouts, Phil Reid wrote:
>> On 23/06/2017 20:19, Benjamin Tissoires wrote:
>>> On Jun 19 2017 or thereabouts, Wolfram Sang wrote:
>>>> On Thu, Jun 15, 2017 at 09:59:33PM +0800, Phil Reid wrote:
>>>>> Add a call to of_i2c_setup_smbus_alert when a i2c adapter is registered
>>>>> so the the smbalert driver can be registered.
>>>>>
>>>>> Signed-off-by: Phil Reid <preid@electromag.com.au>
>>>>
>>>> CCing Benjamin
>>>>
>>>>> ---
>>>>> drivers/i2c/i2c-core.c | 4 ++++
>>>>> 1 file changed, 4 insertions(+)
>>>>>
>>>>> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
>>>>> index d2402bb..626471b 100644
>>>>> --- a/drivers/i2c/i2c-core.c
>>>>> +++ b/drivers/i2c/i2c-core.c
>>>>> @@ -40,6 +40,7 @@
>>>>> #include <linux/gpio.h>
>>>>> #include <linux/hardirq.h>
>>>>> #include <linux/i2c.h>
>>>>> +#include <linux/i2c-smbus.h>
>>>>> #include <linux/idr.h>
>>>>> #include <linux/init.h>
>>>>> #include <linux/irqflags.h>
>>>>> @@ -2045,6 +2046,9 @@ static int i2c_register_adapter(struct i2c_adapter *adap)
>>>>> dev_warn(&adap->dev,
>>>>> "Failed to create compatibility class link\n");
>>>>> #endif
>>>>> + res = of_i2c_setup_smbus_alert(adap);
>>>>> + if (res)
>>>>> + goto out_list;
>>>
>>> See my concerns in patch 4/10.
>>>
>>> In addition, shouldn't this be placed before device_register() for the
>>> least? pm_runtime_enable() would require a matching pm_runtime_disable(),
>>> and device_register() some unregistering behavior too.
>>>
>>
>> G'day Ben,
>>
>> Thanks for the review.
>> Yes this makes sense. I tried having it before the device_register and I get an error
>> about a kobject not being initialised in the a call from of_i2c_setup_smbus_alert.
>>
>> Having a look at what I'm doing in of_i2c_setup_smbus_alert now I'm not sure there's
>> a need to bail out on an error now. Originally I was registering the irq in the setup call.
>> Which need to handle probe defer. Now this should be handled in the alert probe call.
>>
>> WDYR?
>
> Well, of_i2c_setup_smbus_alert() returns an error code, so it has to be
> accounted for. If the error is not a reason to fail, you should at least
> shout some error messages and act accordingly.
>
> However, looking at of_i2c_setup_smbus_alert() again, I wonder if we
> should not split it in 2: one part that checks for the OF flag presence
> and bail out early if an error is encountered (before device
> registration), and one part once the device is registered that calls
> i2c_setup_smbus_alert(). In case of a failure here, we need to
> unregister the adapter because we don't have all the elements at hands
> (assuming we consider that smbus-alert should be mandatory).
>
G'day Benjamin,
I've tried a couple of different ways of handling errors from of_i2c_setup_smbus_alert()
and not having much success.
Call of_i2c_setup_smbus_alert() before the device_register() call results in a kernel panic.
Call after device_register() and injecting an error into the of_i2c_setup_smbus_alert()
also results in a kernel panic.
On error I'm calling device_unregister()
I also tried spliting the device_unregister() call into
device_initialize(dev);
of_i2c_setup_smbus_alert()
device_add(dev);
but that results in a crash in of_i2c_setup_smbus_alert()
If i2c_new_device() is not called then I get:
Unable to handle kernel NULL pointer dereference at virtual address 00000000
PC is at __wake_up_common+0x2c/0x90
[<8015d940>] (__wake_up_common) from [<8015d9c8>] (__wake_up_locked+0x24/0x2c)
[<8015d9c8>] (__wake_up_locked) from [<8015e680>] (complete+0x48/0x58)
[<8015e680>] (complete) from [<805ba6c0>] (i2c_adapter_dev_release+0x1c/0x20)
[<805ba6c0>] (i2c_adapter_dev_release) from [<804d22f8>] (device_release+0x3c/0xa0)
[<804d22f8>] (device_release) from [<80419ab0>] (kobject_put+0xac/0x208)
[<80419ab0>] (kobject_put) from [<804d3778>] (device_unregister+0x64/0x70)
[<804d3778>] (device_unregister) from [<805bb234>] (i2c_register_adapter+0x2a4/0x480)
[<805bb234>] (i2c_register_adapter) from [<805bc5b4>] (i2c_add_adapter+0x8c/0xd0)
[<805bc5b4>] (i2c_add_adapter) from [<805c0690>] (i2c_mux_add_adapter+0x278/0x428)
[<805c0690>] (i2c_mux_add_adapter) from [<805c4b48>] (pca954x_probe+0x2b0/0x394)
[<805c4b48>] (pca954x_probe) from [<805b9c44>] (i2c_device_probe+0x200/0x2dc)
[<805b9c44>] (i2c_device_probe) from [<804d7e0c>] (driver_probe_device+0x338/0x460)
[<804d7e0c>] (driver_probe_device) from [<804d814c>] (__device_attach_driver+0xa4/0x128)
[<804d814c>] (__device_attach_driver) from [<804d5ad8>] (bus_for_each_drv+0x68/0x9c)
[<804d5ad8>] (bus_for_each_drv) from [<804d792c>] (__device_attach+0xc0/0x14c)
[<804d792c>] (__device_attach) from [<804d81ec>] (device_initial_probe+0x1c/0x20)
[<804d81ec>] (device_initial_probe) from [<804d6cf4>] (bus_probe_device+0x94/0x9c)
[<804d6cf4>] (bus_probe_device) from [<804d72dc>] (deferred_probe_work_func+0xa0/0xd4)
[<804d72dc>] (deferred_probe_work_func) from [<8013d7f0>] (process_one_work+0x14c/0x4c4)
[<8013d7f0>] (process_one_work) from [<8013dd94>] (worker_thread+0x22c/0x514)
[<8013dd94>] (worker_thread) from [<80143a20>] (kthread+0x140/0x170)
[<80143a20>] (kthread) from [<80108e38>] (ret_from_fork+0x14/0x3c)
And the system hangs.
If i2c_new_device() is called and I return an error after I get:
This one probably wouldn't happen.
WARNING: CPU: 0 PID: 70 at fs/proc/generic.c:580 remove_proc_entry+0x124/0x168
remove_proc_entry: removing non-empty directory 'irq/193', leaking at least 'smbus_alert'
Modules linked in:
CPU: 0 PID: 70 Comm: kworker/0:2 Not tainted 4.12.0-rc6+ #141
Hardware name: Altera SOCFPGA
Workqueue: events deferred_probe_work_func
[<801145a4>] (unwind_backtrace) from [<8010df44>] (show_stack+0x20/0x24)
[<8010df44>] (show_stack) from [<80418308>] (dump_stack+0x8c/0xa0)
[<80418308>] (dump_stack) from [<80123518>] (__warn+0xf8/0x110)
[<80123518>] (__warn) from [<80123578>] (warn_slowpath_fmt+0x48/0x50)
[<80123578>] (warn_slowpath_fmt) from [<802acfa0>] (remove_proc_entry+0x124/0x168)
[<802acfa0>] (remove_proc_entry) from [<80174d38>] (unregister_irq_proc+0xac/0xb4)
[<80174d38>] (unregister_irq_proc) from [<8016b404>] (free_desc+0x40/0x78)
[<8016b404>] (free_desc) from [<8016b494>] (irq_free_descs+0x58/0x90)
[<8016b494>] (irq_free_descs) from [<80174128>] (irq_dispose_mapping+0x54/0x7c)
[<80174128>] (irq_dispose_mapping) from [<805c45ac>] (pca954x_cleanup+0x4c/0x70)
[<805c45ac>] (pca954x_cleanup) from [<805c4ac0>] (pca954x_probe+0x210/0x394)
[<805c4ac0>] (pca954x_probe) from [<805b9c44>] (i2c_device_probe+0x200/0x2dc)
[<805b9c44>] (i2c_device_probe) from [<804d7e0c>] (driver_probe_device+0x338/0x460)
[<804d7e0c>] (driver_probe_device) from [<804d814c>] (__device_attach_driver+0xa4/0x128)
[<804d814c>] (__device_attach_driver) from [<804d5ad8>] (bus_for_each_drv+0x68/0x9c)
[<804d5ad8>] (bus_for_each_drv) from [<804d792c>] (__device_attach+0xc0/0x14c)
[<804d792c>] (__device_attach) from [<804d81ec>] (device_initial_probe+0x1c/0x20)
[<804d81ec>] (device_initial_probe) from [<804d6cf4>] (bus_probe_device+0x94/0x9c)
[<804d6cf4>] (bus_probe_device) from [<804d72dc>] (deferred_probe_work_func+0xa0/0xd4)
[<804d72dc>] (deferred_probe_work_func) from [<8013d7f0>] (process_one_work+0x14c/0x4c4)
[<8013d7f0>] (process_one_work) from [<8013dd94>] (worker_thread+0x22c/0x514)
[<8013dd94>] (worker_thread) from [<80143a20>] (kthread+0x140/0x170)
[<80143a20>] (kthread) from [<80108e38>] (ret_from_fork+0x14/0x3c)
I'm at a real loss on how to safely handle the error from of_i2c_setup_smbus_alert()
Any hints as to what the correct way is to unregister the device?
--
Regards
Phil Reid
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v7 05/10] i2c: core: call of_i2c_setup_smbus_alert in i2c_register_adapter
2017-07-11 7:52 ` Phil Reid
@ 2017-07-21 9:24 ` Benjamin Tissoires
[not found] ` <20170721092431.GA11387-/m+UfqrgI5QNLKR9yMNcA1aTQe2KTcn/@public.gmane.org>
0 siblings, 1 reply; 30+ messages in thread
From: Benjamin Tissoires @ 2017-07-21 9:24 UTC (permalink / raw)
To: Phil Reid
Cc: Wolfram Sang, robh+dt, mark.rutland, sre, jdelvare, jglauber,
david.daney, peda, linux-i2c, devicetree, linux-pm
On Jul 11 2017 or thereabouts, Phil Reid wrote:
> On 28/06/2017 20:45, Benjamin Tissoires wrote:
> > On Jun 28 2017 or thereabouts, Phil Reid wrote:
> > > On 23/06/2017 20:19, Benjamin Tissoires wrote:
> > > > On Jun 19 2017 or thereabouts, Wolfram Sang wrote:
> > > > > On Thu, Jun 15, 2017 at 09:59:33PM +0800, Phil Reid wrote:
> > > > > > Add a call to of_i2c_setup_smbus_alert when a i2c adapter is registered
> > > > > > so the the smbalert driver can be registered.
> > > > > >
> > > > > > Signed-off-by: Phil Reid <preid@electromag.com.au>
> > > > >
> > > > > CCing Benjamin
> > > > >
> > > > > > ---
> > > > > > drivers/i2c/i2c-core.c | 4 ++++
> > > > > > 1 file changed, 4 insertions(+)
> > > > > >
> > > > > > diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> > > > > > index d2402bb..626471b 100644
> > > > > > --- a/drivers/i2c/i2c-core.c
> > > > > > +++ b/drivers/i2c/i2c-core.c
> > > > > > @@ -40,6 +40,7 @@
> > > > > > #include <linux/gpio.h>
> > > > > > #include <linux/hardirq.h>
> > > > > > #include <linux/i2c.h>
> > > > > > +#include <linux/i2c-smbus.h>
> > > > > > #include <linux/idr.h>
> > > > > > #include <linux/init.h>
> > > > > > #include <linux/irqflags.h>
> > > > > > @@ -2045,6 +2046,9 @@ static int i2c_register_adapter(struct i2c_adapter *adap)
> > > > > > dev_warn(&adap->dev,
> > > > > > "Failed to create compatibility class link\n");
> > > > > > #endif
> > > > > > + res = of_i2c_setup_smbus_alert(adap);
> > > > > > + if (res)
> > > > > > + goto out_list;
> > > >
> > > > See my concerns in patch 4/10.
> > > >
> > > > In addition, shouldn't this be placed before device_register() for the
> > > > least? pm_runtime_enable() would require a matching pm_runtime_disable(),
> > > > and device_register() some unregistering behavior too.
> > > >
> > >
> > > G'day Ben,
> > >
> > > Thanks for the review.
> > > Yes this makes sense. I tried having it before the device_register and I get an error
> > > about a kobject not being initialised in the a call from of_i2c_setup_smbus_alert.
> > >
> > > Having a look at what I'm doing in of_i2c_setup_smbus_alert now I'm not sure there's
> > > a need to bail out on an error now. Originally I was registering the irq in the setup call.
> > > Which need to handle probe defer. Now this should be handled in the alert probe call.
> > >
> > > WDYR?
> >
> > Well, of_i2c_setup_smbus_alert() returns an error code, so it has to be
> > accounted for. If the error is not a reason to fail, you should at least
> > shout some error messages and act accordingly.
> >
> > However, looking at of_i2c_setup_smbus_alert() again, I wonder if we
> > should not split it in 2: one part that checks for the OF flag presence
> > and bail out early if an error is encountered (before device
> > registration), and one part once the device is registered that calls
> > i2c_setup_smbus_alert(). In case of a failure here, we need to
> > unregister the adapter because we don't have all the elements at hands
> > (assuming we consider that smbus-alert should be mandatory).
> >
>
> G'day Benjamin,
Hi Phil,
[sorry for the lag, been busy with other topics + public holidays]
>
> I've tried a couple of different ways of handling errors from of_i2c_setup_smbus_alert()
> and not having much success.
>
> Call of_i2c_setup_smbus_alert() before the device_register() call results in a kernel panic.
Yeah, i2c_setup_smbus_alert() basically creates an I2C device, so you
need to have the adapter registered before you can call it.
>
> Call after device_register() and injecting an error into the of_i2c_setup_smbus_alert()
> also results in a kernel panic.
I wonder if this is just not required at all to have a failure path that
will remove the smbusalert device. Because this is an I2C device, when
we unregister the adapter, it should properly remove all of its attached
devices.
>
> On error I'm calling device_unregister()
>
> I also tried spliting the device_unregister() call into
> device_initialize(dev);
> of_i2c_setup_smbus_alert()
> device_add(dev);
>
> but that results in a crash in of_i2c_setup_smbus_alert()
>
>
> If i2c_new_device() is not called then I get:
> Unable to handle kernel NULL pointer dereference at virtual address 00000000
> PC is at __wake_up_common+0x2c/0x90
>
> [<8015d940>] (__wake_up_common) from [<8015d9c8>] (__wake_up_locked+0x24/0x2c)
> [<8015d9c8>] (__wake_up_locked) from [<8015e680>] (complete+0x48/0x58)
> [<8015e680>] (complete) from [<805ba6c0>] (i2c_adapter_dev_release+0x1c/0x20)
> [<805ba6c0>] (i2c_adapter_dev_release) from [<804d22f8>] (device_release+0x3c/0xa0)
> [<804d22f8>] (device_release) from [<80419ab0>] (kobject_put+0xac/0x208)
> [<80419ab0>] (kobject_put) from [<804d3778>] (device_unregister+0x64/0x70)
> [<804d3778>] (device_unregister) from [<805bb234>] (i2c_register_adapter+0x2a4/0x480)
> [<805bb234>] (i2c_register_adapter) from [<805bc5b4>] (i2c_add_adapter+0x8c/0xd0)
> [<805bc5b4>] (i2c_add_adapter) from [<805c0690>] (i2c_mux_add_adapter+0x278/0x428)
> [<805c0690>] (i2c_mux_add_adapter) from [<805c4b48>] (pca954x_probe+0x2b0/0x394)
> [<805c4b48>] (pca954x_probe) from [<805b9c44>] (i2c_device_probe+0x200/0x2dc)
> [<805b9c44>] (i2c_device_probe) from [<804d7e0c>] (driver_probe_device+0x338/0x460)
> [<804d7e0c>] (driver_probe_device) from [<804d814c>] (__device_attach_driver+0xa4/0x128)
> [<804d814c>] (__device_attach_driver) from [<804d5ad8>] (bus_for_each_drv+0x68/0x9c)
> [<804d5ad8>] (bus_for_each_drv) from [<804d792c>] (__device_attach+0xc0/0x14c)
> [<804d792c>] (__device_attach) from [<804d81ec>] (device_initial_probe+0x1c/0x20)
> [<804d81ec>] (device_initial_probe) from [<804d6cf4>] (bus_probe_device+0x94/0x9c)
> [<804d6cf4>] (bus_probe_device) from [<804d72dc>] (deferred_probe_work_func+0xa0/0xd4)
> [<804d72dc>] (deferred_probe_work_func) from [<8013d7f0>] (process_one_work+0x14c/0x4c4)
> [<8013d7f0>] (process_one_work) from [<8013dd94>] (worker_thread+0x22c/0x514)
> [<8013dd94>] (worker_thread) from [<80143a20>] (kthread+0x140/0x170)
> [<80143a20>] (kthread) from [<80108e38>] (ret_from_fork+0x14/0x3c)
>
> And the system hangs.
>
>
> If i2c_new_device() is called and I return an error after I get:
> This one probably wouldn't happen.
>
> WARNING: CPU: 0 PID: 70 at fs/proc/generic.c:580 remove_proc_entry+0x124/0x168
> remove_proc_entry: removing non-empty directory 'irq/193', leaking at least 'smbus_alert'
> Modules linked in:
> CPU: 0 PID: 70 Comm: kworker/0:2 Not tainted 4.12.0-rc6+ #141
> Hardware name: Altera SOCFPGA
> Workqueue: events deferred_probe_work_func
> [<801145a4>] (unwind_backtrace) from [<8010df44>] (show_stack+0x20/0x24)
> [<8010df44>] (show_stack) from [<80418308>] (dump_stack+0x8c/0xa0)
> [<80418308>] (dump_stack) from [<80123518>] (__warn+0xf8/0x110)
> [<80123518>] (__warn) from [<80123578>] (warn_slowpath_fmt+0x48/0x50)
> [<80123578>] (warn_slowpath_fmt) from [<802acfa0>] (remove_proc_entry+0x124/0x168)
> [<802acfa0>] (remove_proc_entry) from [<80174d38>] (unregister_irq_proc+0xac/0xb4)
> [<80174d38>] (unregister_irq_proc) from [<8016b404>] (free_desc+0x40/0x78)
> [<8016b404>] (free_desc) from [<8016b494>] (irq_free_descs+0x58/0x90)
> [<8016b494>] (irq_free_descs) from [<80174128>] (irq_dispose_mapping+0x54/0x7c)
> [<80174128>] (irq_dispose_mapping) from [<805c45ac>] (pca954x_cleanup+0x4c/0x70)
> [<805c45ac>] (pca954x_cleanup) from [<805c4ac0>] (pca954x_probe+0x210/0x394)
> [<805c4ac0>] (pca954x_probe) from [<805b9c44>] (i2c_device_probe+0x200/0x2dc)
> [<805b9c44>] (i2c_device_probe) from [<804d7e0c>] (driver_probe_device+0x338/0x460)
> [<804d7e0c>] (driver_probe_device) from [<804d814c>] (__device_attach_driver+0xa4/0x128)
> [<804d814c>] (__device_attach_driver) from [<804d5ad8>] (bus_for_each_drv+0x68/0x9c)
> [<804d5ad8>] (bus_for_each_drv) from [<804d792c>] (__device_attach+0xc0/0x14c)
> [<804d792c>] (__device_attach) from [<804d81ec>] (device_initial_probe+0x1c/0x20)
> [<804d81ec>] (device_initial_probe) from [<804d6cf4>] (bus_probe_device+0x94/0x9c)
> [<804d6cf4>] (bus_probe_device) from [<804d72dc>] (deferred_probe_work_func+0xa0/0xd4)
> [<804d72dc>] (deferred_probe_work_func) from [<8013d7f0>] (process_one_work+0x14c/0x4c4)
> [<8013d7f0>] (process_one_work) from [<8013dd94>] (worker_thread+0x22c/0x514)
> [<8013dd94>] (worker_thread) from [<80143a20>] (kthread+0x140/0x170)
> [<80143a20>] (kthread) from [<80108e38>] (ret_from_fork+0x14/0x3c)
>
>
> I'm at a real loss on how to safely handle the error from of_i2c_setup_smbus_alert()
>
> Any hints as to what the correct way is to unregister the device?
I'd say try to register the smbus_alert device after the registering of
the adapter, and if it fails simply call the unregister from the
adapter. On remove, there might not need to do anything given that the
smbus_alert is an I2C client like every others.
Cheers,
Benjamin
PS: sorry if I am completely off-topic
>
>
> --
> Regards
> Phil Reid
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v7 06/10] i2c: mux: pca954x: Call request irq after adding mux segments
2017-06-15 13:59 [PATCH v7 00/10] Add sbs-manager with smbalert support Phil Reid
` (4 preceding siblings ...)
2017-06-15 13:59 ` [PATCH v7 05/10] i2c: core: call of_i2c_setup_smbus_alert in i2c_register_adapter Phil Reid
@ 2017-06-15 13:59 ` Phil Reid
2017-06-15 13:59 ` [PATCH v7 07/10] Documentation: Add sbs-manager device tree node documentation Phil Reid
` (3 subsequent siblings)
9 siblings, 0 replies; 30+ messages in thread
From: Phil Reid @ 2017-06-15 13:59 UTC (permalink / raw)
To: wsa, robh+dt, mark.rutland, sre, jdelvare, jglauber, david.daney,
peda, preid, linux-i2c, devicetree, linux-pm
The pca954x device do not have the ability to mask interrupts. For
i2c slave devices that also don't have masking ability (eg ltc1760
smbalert output) delay registering the irq until after the mux
segments have been configured. During the mux add_adaptor call the
core i2c system can register an smbalert handler which would then
be called immediately when the irq is registered. This smbalert
handler will then clear the pending irq.
Also add return check to irq_create_mapping call.
Signed-off-by: Phil Reid <preid@electromag.com.au>
---
drivers/i2c/muxes/i2c-mux-pca954x.c | 59 +++++++++++++++++++------------------
1 file changed, 30 insertions(+), 29 deletions(-)
diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c
index 2f21138..b152efa 100644
--- a/drivers/i2c/muxes/i2c-mux-pca954x.c
+++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
@@ -305,7 +305,7 @@ static int pca954x_irq_setup(struct i2c_mux_core *muxc)
{
struct pca954x *data = i2c_mux_priv(muxc);
struct i2c_client *client = data->client;
- int c, err, irq;
+ int c, irq;
if (!data->chip->has_irq || client->irq <= 0)
return 0;
@@ -320,29 +320,31 @@ static int pca954x_irq_setup(struct i2c_mux_core *muxc)
for (c = 0; c < data->chip->nchans; c++) {
irq = irq_create_mapping(data->irq, c);
+ if (irq <= 0) {
+ dev_err(&client->dev, "failed irq create map\n");
+ return -EINVAL;
+ }
irq_set_chip_data(irq, data);
irq_set_chip_and_handler(irq, &pca954x_irq_chip,
- handle_simple_irq);
+ handle_simple_irq);
}
- err = devm_request_threaded_irq(&client->dev, data->client->irq, NULL,
- pca954x_irq_handler,
- IRQF_ONESHOT | IRQF_SHARED,
- "pca954x", data);
- if (err)
- goto err_req_irq;
+ return 0;
+}
- disable_irq(data->client->irq);
+static void pca954x_cleanup(struct i2c_mux_core *muxc)
+{
+ struct pca954x *data = i2c_mux_priv(muxc);
+ int c, irq;
- return 0;
-err_req_irq:
- for (c = 0; c < data->chip->nchans; c++) {
- irq = irq_find_mapping(data->irq, c);
- irq_dispose_mapping(irq);
+ if (data->irq) {
+ for (c = 0; c < data->chip->nchans; c++) {
+ irq = irq_find_mapping(data->irq, c);
+ irq_dispose_mapping(irq);
+ }
+ irq_domain_remove(data->irq);
}
- irq_domain_remove(data->irq);
-
- return err;
+ i2c_mux_del_adapters(muxc);
}
/*
@@ -435,6 +437,15 @@ static int pca954x_probe(struct i2c_client *client,
}
}
+ if (data->irq) {
+ ret = devm_request_threaded_irq(&client->dev, data->client->irq,
+ NULL, pca954x_irq_handler,
+ IRQF_ONESHOT | IRQF_SHARED,
+ "pca954x", data);
+ if (ret)
+ goto fail_del_adapters;
+ }
+
dev_info(&client->dev,
"registered %d multiplexed busses for I2C %s %s\n",
num, data->chip->muxtype == pca954x_ismux
@@ -443,25 +454,15 @@ static int pca954x_probe(struct i2c_client *client,
return 0;
fail_del_adapters:
- i2c_mux_del_adapters(muxc);
+ pca954x_cleanup(muxc);
return ret;
}
static int pca954x_remove(struct i2c_client *client)
{
struct i2c_mux_core *muxc = i2c_get_clientdata(client);
- struct pca954x *data = i2c_mux_priv(muxc);
- int c, irq;
-
- if (data->irq) {
- for (c = 0; c < data->chip->nchans; c++) {
- irq = irq_find_mapping(data->irq, c);
- irq_dispose_mapping(irq);
- }
- irq_domain_remove(data->irq);
- }
- i2c_mux_del_adapters(muxc);
+ pca954x_cleanup(muxc);
return 0;
}
--
1.8.3.1
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PATCH v7 07/10] Documentation: Add sbs-manager device tree node documentation
2017-06-15 13:59 [PATCH v7 00/10] Add sbs-manager with smbalert support Phil Reid
` (5 preceding siblings ...)
2017-06-15 13:59 ` [PATCH v7 06/10] i2c: mux: pca954x: Call request irq after adding mux segments Phil Reid
@ 2017-06-15 13:59 ` Phil Reid
2017-07-03 13:57 ` Sebastian Reichel
2017-06-15 13:59 ` [PATCH v7 08/10] power: Adds support for Smart Battery System Manager Phil Reid
` (2 subsequent siblings)
9 siblings, 1 reply; 30+ messages in thread
From: Phil Reid @ 2017-06-15 13:59 UTC (permalink / raw)
To: wsa, robh+dt, mark.rutland, sre, jdelvare, jglauber, david.daney,
peda, preid, linux-i2c, devicetree, linux-pm
Cc: Karl-Heinz Schneider
From: Karl-Heinz Schneider <karl-heinz@schneider-inet.de>
This patch adds device tree documentation for the sbs-manager
Signed-off-by: Karl-Heinz Schneider <karl-heinz@schneider-inet.de>
Signed-off-by: Phil Reid <preid@electromag.com.au>
Acked-by: Rob Herring <robh@kernel.org>
---
.../bindings/power/supply/sbs,sbs-manager.txt | 66 ++++++++++++++++++++++
1 file changed, 66 insertions(+)
create mode 100644 Documentation/devicetree/bindings/power/supply/sbs,sbs-manager.txt
diff --git a/Documentation/devicetree/bindings/power/supply/sbs,sbs-manager.txt b/Documentation/devicetree/bindings/power/supply/sbs,sbs-manager.txt
new file mode 100644
index 0000000..4b219557
--- /dev/null
+++ b/Documentation/devicetree/bindings/power/supply/sbs,sbs-manager.txt
@@ -0,0 +1,66 @@
+Binding for sbs-manager
+
+Required properties:
+- compatible: "<vendor>,<part-number>", "sbs,sbs-charger" as fallback. The part
+ number compatible string might be used in order to take care of vendor
+ specific registers.
+- reg: integer, i2c address of the device. Should be <0xa>.
+Optional properties:
+- gpio-controller: Marks the port as GPIO controller.
+ See "gpio-specifier" in .../devicetree/bindings/gpio/gpio.txt.
+- #gpio-cells: Should be <2>. The first cell is the pin number, the second cell
+ is used to specify optional parameters:
+ See "gpio-specifier" in .../devicetree/bindings/gpio/gpio.txt.
+
+From OS view the device is basically an i2c-mux used to communicate with up to
+four smart battery devices at address 0xb. The driver actually implements this
+behaviour. So standard i2c-mux nodes can be used to register up to four slave
+batteries. Channels will be numerated starting from 1 to 4.
+
+Example:
+
+batman@a {
+ compatible = "lltc,ltc1760", "sbs,sbs-manager";
+ reg = <0x0a>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ gpio-controller;
+ #gpio-cells = <2>;
+
+ i2c@1 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ reg = <1>;
+
+ battery@b {
+ compatible = "ti,bq2060", "sbs,sbs-battery";
+ reg = <0x0b>;
+ sbs,battery-detect-gpios = <&batman 1 1>;
+ };
+ };
+
+ i2c@2 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ reg = <2>;
+
+ battery@b {
+ compatible = "ti,bq2060", "sbs,sbs-battery";
+ reg = <0x0b>;
+ sbs,battery-detect-gpios = <&batman 2 1>;
+ };
+ };
+
+ i2c@3 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ reg = <3>;
+
+ battery@b {
+ compatible = "ti,bq2060", "sbs,sbs-battery";
+ reg = <0x0b>;
+ sbs,battery-detect-gpios = <&batman 3 1>;
+ };
+ };
+};
--
1.8.3.1
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PATCH v7 07/10] Documentation: Add sbs-manager device tree node documentation
2017-06-15 13:59 ` [PATCH v7 07/10] Documentation: Add sbs-manager device tree node documentation Phil Reid
@ 2017-07-03 13:57 ` Sebastian Reichel
0 siblings, 0 replies; 30+ messages in thread
From: Sebastian Reichel @ 2017-07-03 13:57 UTC (permalink / raw)
To: Phil Reid
Cc: wsa, robh+dt, mark.rutland, jdelvare, jglauber, david.daney,
peda, linux-i2c, devicetree, linux-pm, Karl-Heinz Schneider
[-- Attachment #1: Type: text/plain, Size: 3168 bytes --]
Hi,
On Thu, Jun 15, 2017 at 09:59:35PM +0800, Phil Reid wrote:
> From: Karl-Heinz Schneider <karl-heinz@schneider-inet.de>
>
> This patch adds device tree documentation for the sbs-manager
>
> Signed-off-by: Karl-Heinz Schneider <karl-heinz@schneider-inet.de>
> Signed-off-by: Phil Reid <preid@electromag.com.au>
> Acked-by: Rob Herring <robh@kernel.org>
Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
-- Sebastian
> ---
> .../bindings/power/supply/sbs,sbs-manager.txt | 66 ++++++++++++++++++++++
> 1 file changed, 66 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/power/supply/sbs,sbs-manager.txt
>
> diff --git a/Documentation/devicetree/bindings/power/supply/sbs,sbs-manager.txt b/Documentation/devicetree/bindings/power/supply/sbs,sbs-manager.txt
> new file mode 100644
> index 0000000..4b219557
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/power/supply/sbs,sbs-manager.txt
> @@ -0,0 +1,66 @@
> +Binding for sbs-manager
> +
> +Required properties:
> +- compatible: "<vendor>,<part-number>", "sbs,sbs-charger" as fallback. The part
> + number compatible string might be used in order to take care of vendor
> + specific registers.
> +- reg: integer, i2c address of the device. Should be <0xa>.
> +Optional properties:
> +- gpio-controller: Marks the port as GPIO controller.
> + See "gpio-specifier" in .../devicetree/bindings/gpio/gpio.txt.
> +- #gpio-cells: Should be <2>. The first cell is the pin number, the second cell
> + is used to specify optional parameters:
> + See "gpio-specifier" in .../devicetree/bindings/gpio/gpio.txt.
> +
> +From OS view the device is basically an i2c-mux used to communicate with up to
> +four smart battery devices at address 0xb. The driver actually implements this
> +behaviour. So standard i2c-mux nodes can be used to register up to four slave
> +batteries. Channels will be numerated starting from 1 to 4.
> +
> +Example:
> +
> +batman@a {
> + compatible = "lltc,ltc1760", "sbs,sbs-manager";
> + reg = <0x0a>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + gpio-controller;
> + #gpio-cells = <2>;
> +
> + i2c@1 {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + reg = <1>;
> +
> + battery@b {
> + compatible = "ti,bq2060", "sbs,sbs-battery";
> + reg = <0x0b>;
> + sbs,battery-detect-gpios = <&batman 1 1>;
> + };
> + };
> +
> + i2c@2 {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + reg = <2>;
> +
> + battery@b {
> + compatible = "ti,bq2060", "sbs,sbs-battery";
> + reg = <0x0b>;
> + sbs,battery-detect-gpios = <&batman 2 1>;
> + };
> + };
> +
> + i2c@3 {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + reg = <3>;
> +
> + battery@b {
> + compatible = "ti,bq2060", "sbs,sbs-battery";
> + reg = <0x0b>;
> + sbs,battery-detect-gpios = <&batman 3 1>;
> + };
> + };
> +};
> --
> 1.8.3.1
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v7 08/10] power: Adds support for Smart Battery System Manager
2017-06-15 13:59 [PATCH v7 00/10] Add sbs-manager with smbalert support Phil Reid
` (6 preceding siblings ...)
2017-06-15 13:59 ` [PATCH v7 07/10] Documentation: Add sbs-manager device tree node documentation Phil Reid
@ 2017-06-15 13:59 ` Phil Reid
[not found] ` <1497535178-12001-9-git-send-email-preid-qgqNFa1JUf/o2iN0hyhwsIdd74u8MsAO@public.gmane.org>
2017-06-15 13:59 ` [PATCH v7 09/10] power: supply: sbs-manager: Add alert callback and battery change notification Phil Reid
2017-06-15 13:59 ` [PATCH v7 10/10] power: supply: sbs-battery: move gpio present detect to sbs_get_property Phil Reid
9 siblings, 1 reply; 30+ messages in thread
From: Phil Reid @ 2017-06-15 13:59 UTC (permalink / raw)
To: wsa, robh+dt, mark.rutland, sre, jdelvare, jglauber, david.daney,
peda, preid, linux-i2c, devicetree, linux-pm
Cc: Karl-Heinz Schneider
From: Karl-Heinz Schneider <karl-heinz@schneider-inet.de>
This patch adds support for Smart Battery System Manager.
A SBSM is a device listening at I2C/SMBus address 0x0a and is capable of
communicating up to four I2C smart battery devices. All smart battery
devices are listening at address 0x0b, so the SBSM muliplexes between
them. The driver makes use of the I2C-Mux framework to allow smart
batteries to be bound via device tree, i.e. the sbs-battery driver.
Via sysfs interface the online state and charge type are presented. If
the driver is bound as ltc1760 (an implementation of a Dual Smart Battery
System Manager) the charge type can also be changed from trickle to fast.
Tested-by: Phil Reid <preid@electromag.com.au>
Reviewed-by: Phil Reid <preid@electromag.com.au>
Signed-off-by: Karl-Heinz Schneider <karl-heinz@schneider-inet.de>
Signed-off-by: Phil Reid <preid@electromag.com.au>
---
drivers/power/supply/Kconfig | 13 ++
drivers/power/supply/Makefile | 1 +
drivers/power/supply/sbs-manager.c | 323 +++++++++++++++++++++++++++++++++++++
3 files changed, 337 insertions(+)
create mode 100644 drivers/power/supply/sbs-manager.c
diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig
index da54ac8..a435415 100644
--- a/drivers/power/supply/Kconfig
+++ b/drivers/power/supply/Kconfig
@@ -170,6 +170,19 @@ config CHARGER_SBS
help
Say Y to include support for SBS compilant battery chargers.
+config MANAGER_SBS
+ tristate "Smart Battery System Manager"
+ depends on I2C && I2C_MUX
+ help
+ Say Y here to include support for Smart Battery System Manager
+ ICs. The driver reports online and charging status via sysfs.
+ It presents itself also as I2C mux which allows to bind
+ smart battery driver to its ports.
+ Supported is for example LTC1760.
+
+ This driver can also be built as a module. If so, the module will be
+ called sbs-manager.
+
config BATTERY_BQ27XXX
tristate "BQ27xxx battery driver"
help
diff --git a/drivers/power/supply/Makefile b/drivers/power/supply/Makefile
index 3789a2c..350c0bb 100644
--- a/drivers/power/supply/Makefile
+++ b/drivers/power/supply/Makefile
@@ -33,6 +33,7 @@ obj-$(CONFIG_BATTERY_IPAQ_MICRO) += ipaq_micro_battery.o
obj-$(CONFIG_BATTERY_WM97XX) += wm97xx_battery.o
obj-$(CONFIG_BATTERY_SBS) += sbs-battery.o
obj-$(CONFIG_CHARGER_SBS) += sbs-charger.o
+obj-$(CONFIG_MANAGER_SBS) += sbs-manager.o
obj-$(CONFIG_BATTERY_BQ27XXX) += bq27xxx_battery.o
obj-$(CONFIG_BATTERY_BQ27XXX_I2C) += bq27xxx_battery_i2c.o
obj-$(CONFIG_BATTERY_DA9030) += da9030_battery.o
diff --git a/drivers/power/supply/sbs-manager.c b/drivers/power/supply/sbs-manager.c
new file mode 100644
index 0000000..47c09a1
--- /dev/null
+++ b/drivers/power/supply/sbs-manager.c
@@ -0,0 +1,323 @@
+/*
+ * Driver for SBS compliant Smart Battery System Managers
+ *
+ * The device communicates via i2c at address 0x0a and multiplexes access to up
+ * to four smart batteries at address 0x0b.
+ *
+ * Via sysfs interface the online state and charge type are presented.
+ *
+ * Datasheet SBSM: http://sbs-forum.org/specs/sbsm100b.pdf
+ * Datasheet LTC1760: http://cds.linear.com/docs/en/datasheet/1760fb.pdf
+ *
+ * Karl-Heinz Schneider <karl-heinz@schneider-inet.de>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/module.h>
+#include <linux/i2c.h>
+#include <linux/i2c-mux.h>
+#include <linux/power_supply.h>
+
+#define SBSM_MAX_BATS 4
+#define SBSM_RETRY_CNT 3
+
+/* registers addresses */
+#define SBSM_CMD_BATSYSSTATE 0x01
+#define SBSM_CMD_BATSYSSTATECONT 0x02
+#define SBSM_CMD_BATSYSINFO 0x04
+#define SBSM_CMD_LTC 0x3c
+
+#define SBSM_MASK_BAT_SUPPORTED GENMASK(3, 0)
+#define SBSM_MASK_CHARGE_BAT GENMASK(7, 4)
+#define SBSM_BIT_AC_PRESENT BIT(0)
+#define SBSM_BIT_TURBO BIT(7)
+
+#define SBSM_SMB_BAT_OFFSET 11
+struct sbsm_data {
+ struct i2c_client *client;
+ struct i2c_mux_core *muxc;
+
+ struct power_supply *psy;
+
+ u8 cur_chan; /* currently selected channel */
+ bool is_ltc1760; /* special capabilities */
+};
+
+static enum power_supply_property sbsm_props[] = {
+ POWER_SUPPLY_PROP_ONLINE,
+ POWER_SUPPLY_PROP_CHARGE_TYPE,
+};
+
+static int sbsm_read_word(struct i2c_client *client, u8 address)
+{
+ int reg, retries;
+
+ for (retries = SBSM_RETRY_CNT; retries > 0; retries--) {
+ reg = i2c_smbus_read_word_data(client, address);
+ if (reg >= 0)
+ break;
+ }
+
+ if (reg < 0) {
+ dev_err(&client->dev, "failed to read register 0x%02x\n",
+ address);
+ }
+
+ return reg;
+}
+
+static int sbsm_write_word(struct i2c_client *client, u8 address, u16 word)
+{
+ int ret, retries;
+
+ for (retries = SBSM_RETRY_CNT; retries > 0; retries--) {
+ ret = i2c_smbus_write_word_data(client, address, word);
+ if (ret >= 0)
+ break;
+ }
+ if (ret < 0)
+ dev_err(&client->dev, "failed to write to register 0x%02x\n",
+ address);
+
+ return ret;
+}
+
+static int sbsm_get_property(struct power_supply *psy,
+ enum power_supply_property psp,
+ union power_supply_propval *val)
+{
+ struct sbsm_data *data = power_supply_get_drvdata(psy);
+ int regval = 0;
+
+ switch (psp) {
+ case POWER_SUPPLY_PROP_ONLINE:
+ regval = sbsm_read_word(data->client, SBSM_CMD_BATSYSSTATECONT);
+ if (regval < 0)
+ return regval;
+ val->intval = !!(regval & SBSM_BIT_AC_PRESENT);
+ break;
+
+ case POWER_SUPPLY_PROP_CHARGE_TYPE:
+ regval = sbsm_read_word(data->client, SBSM_CMD_BATSYSSTATE);
+ if (regval < 0)
+ return regval;
+
+ if ((regval & SBSM_MASK_CHARGE_BAT) == 0) {
+ val->intval = POWER_SUPPLY_CHARGE_TYPE_NONE;
+ return 0;
+ }
+ val->intval = POWER_SUPPLY_CHARGE_TYPE_TRICKLE;
+
+ if (data->is_ltc1760) {
+ /* charge mode fast if turbo is active */
+ regval = sbsm_read_word(data->client, SBSM_CMD_LTC);
+ if (regval < 0)
+ return regval;
+ else if (regval & SBSM_BIT_TURBO)
+ val->intval = POWER_SUPPLY_CHARGE_TYPE_FAST;
+ }
+ break;
+
+ default:
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+static int sbsm_prop_is_writeable(struct power_supply *psy,
+ enum power_supply_property psp)
+{
+ struct sbsm_data *data = power_supply_get_drvdata(psy);
+
+ return (psp == POWER_SUPPLY_PROP_CHARGE_TYPE) && data->is_ltc1760;
+}
+
+static int sbsm_set_property(struct power_supply *psy,
+ enum power_supply_property psp,
+ const union power_supply_propval *val)
+{
+ struct sbsm_data *data = power_supply_get_drvdata(psy);
+ int ret = -EINVAL;
+ u16 regval;
+
+ switch (psp) {
+ case POWER_SUPPLY_PROP_CHARGE_TYPE:
+ /* write 1 to TURBO if type fast is given */
+ if (!data->is_ltc1760)
+ break;
+ regval = val->intval ==
+ POWER_SUPPLY_CHARGE_TYPE_FAST ? SBSM_BIT_TURBO : 0;
+ ret = sbsm_write_word(data->client, SBSM_CMD_LTC, regval);
+ break;
+
+ default:
+ break;
+ }
+
+ return ret;
+}
+
+/*
+ * Switch to battery
+ * Parameter chan is directly the content of SMB_BAT* nibble
+ */
+static int sbsm_select(struct i2c_mux_core *muxc, u32 chan)
+{
+ struct sbsm_data *data = i2c_mux_priv(muxc);
+ struct device *dev = &data->client->dev;
+ int ret = 0;
+ u16 reg;
+
+ if (data->cur_chan == chan)
+ return ret;
+
+ /* chan goes from 1 ... 4 */
+ reg = 1 << BIT(SBSM_SMB_BAT_OFFSET + chan);
+ ret = sbsm_write_word(data->client, SBSM_CMD_BATSYSSTATE, reg);
+ if (ret)
+ dev_err(dev, "Failed to select channel %i\n", chan);
+ else
+ data->cur_chan = chan;
+
+ return ret;
+}
+
+static const struct power_supply_desc sbsm_default_psy_desc = {
+ .type = POWER_SUPPLY_TYPE_MAINS,
+ .properties = sbsm_props,
+ .num_properties = ARRAY_SIZE(sbsm_props),
+ .get_property = &sbsm_get_property,
+ .set_property = &sbsm_set_property,
+ .property_is_writeable = &sbsm_prop_is_writeable,
+};
+
+static int sbsm_probe(struct i2c_client *client,
+ const struct i2c_device_id *id)
+{
+ struct i2c_adapter *adapter = to_i2c_adapter(client->dev.parent);
+ struct sbsm_data *data;
+ struct device *dev = &client->dev;
+ struct power_supply_desc *psy_desc;
+ struct power_supply_config psy_cfg = {};
+ int ret = 0, i, supported_bats;
+
+ /* Device listens only at address 0x0a */
+ if (client->addr != 0x0a)
+ return -EINVAL;
+
+ if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_WORD_DATA))
+ return -EPFNOSUPPORT;
+
+ data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
+ if (!data)
+ return -ENOMEM;
+
+ i2c_set_clientdata(client, data);
+
+ data->client = client;
+ data->is_ltc1760 = !!strstr(id->name, "ltc1760");
+
+ ret = sbsm_read_word(client, SBSM_CMD_BATSYSINFO);
+ if (ret < 0)
+ return ret;
+ supported_bats = ret & SBSM_MASK_BAT_SUPPORTED;
+
+ data->muxc = i2c_mux_alloc(adapter, dev, SBSM_MAX_BATS, 0,
+ I2C_MUX_LOCKED, &sbsm_select, NULL);
+ if (!data->muxc) {
+ dev_err(dev, "failed to alloc i2c mux\n");
+ ret = -ENOMEM;
+ goto err_mux_alloc;
+ }
+ data->muxc->priv = data;
+
+ /* register muxed i2c channels. One for each supported battery */
+ for (i = 0; i < SBSM_MAX_BATS; ++i) {
+ if (supported_bats & BIT(i)) {
+ ret = i2c_mux_add_adapter(data->muxc, 0, i + 1, 0);
+ if (ret)
+ break;
+ }
+ }
+ if (ret) {
+ dev_err(dev, "failed to register i2c mux channel %d\n", i + 1);
+ goto err_mux_register;
+ }
+
+ psy_desc = devm_kmemdup(dev, &sbsm_default_psy_desc,
+ sizeof(struct power_supply_desc),
+ GFP_KERNEL);
+ if (!psy_desc) {
+ ret = -ENOMEM;
+ goto err_psy;
+ }
+
+ psy_desc->name = devm_kasprintf(dev, GFP_KERNEL, "sbsm-%s",
+ dev_name(&client->dev));
+ if (!psy_desc->name) {
+ ret = -ENOMEM;
+ goto err_psy;
+ }
+
+ psy_cfg.drv_data = data;
+ psy_cfg.of_node = dev->of_node;
+ data->psy = devm_power_supply_register(dev, psy_desc, &psy_cfg);
+ if (IS_ERR(data->psy)) {
+ ret = PTR_ERR(data->psy);
+ dev_err(dev, "failed to register power supply %s\n",
+ psy_desc->name);
+ goto err_psy;
+ }
+
+ return 0;
+
+err_psy:
+err_mux_register:
+ i2c_mux_del_adapters(data->muxc);
+
+err_mux_alloc:
+ return ret;
+}
+
+static int sbsm_remove(struct i2c_client *client)
+{
+ struct sbsm_data *data = i2c_get_clientdata(client);
+
+ i2c_mux_del_adapters(data->muxc);
+ return 0;
+}
+
+static const struct i2c_device_id sbsm_ids[] = {
+ { "sbs-manager", 0 },
+ { "ltc1760", 0 },
+ { }
+};
+MODULE_DEVICE_TABLE(i2c, sbsm_ids);
+
+#ifdef CONFIG_OF
+static const struct of_device_id sbsm_dt_ids[] = {
+ { .compatible = "sbs,sbs-manager" },
+ { .compatible = "lltc,ltc1760" },
+ { }
+};
+MODULE_DEVICE_TABLE(of, sbsm_dt_ids);
+#endif
+
+static struct i2c_driver sbsm_driver = {
+ .driver = {
+ .name = "sbsm",
+ .of_match_table = of_match_ptr(sbsm_dt_ids),
+ },
+ .probe = sbsm_probe,
+ .remove = sbsm_remove,
+ .id_table = sbsm_ids
+};
+module_i2c_driver(sbsm_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Karl-Heinz Schneider <karl-heinz@schneider-inet.de>");
+MODULE_DESCRIPTION("SBSM Smart Battery System Manager");
--
1.8.3.1
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PATCH v7 09/10] power: supply: sbs-manager: Add alert callback and battery change notification
2017-06-15 13:59 [PATCH v7 00/10] Add sbs-manager with smbalert support Phil Reid
` (7 preceding siblings ...)
2017-06-15 13:59 ` [PATCH v7 08/10] power: Adds support for Smart Battery System Manager Phil Reid
@ 2017-06-15 13:59 ` Phil Reid
2017-06-15 13:59 ` [PATCH v7 10/10] power: supply: sbs-battery: move gpio present detect to sbs_get_property Phil Reid
9 siblings, 0 replies; 30+ messages in thread
From: Phil Reid @ 2017-06-15 13:59 UTC (permalink / raw)
To: wsa, robh+dt, mark.rutland, sre, jdelvare, jglauber, david.daney,
peda, preid, linux-i2c, devicetree, linux-pm
This adds smb alert support via the smbus_alert driver to generate
power_supply_changed notifications when either external power is
removed / applied or a battery inserted / removed.
Use the i2c alert callback to notify the attached battery driver that a
change has occurred.
Signed-off-by: Phil Reid <preid@electromag.com.au>
Acked-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
---
drivers/power/supply/Kconfig | 3 +-
drivers/power/supply/sbs-manager.c | 129 +++++++++++++++++++++++++++++++++++--
2 files changed, 127 insertions(+), 5 deletions(-)
diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig
index a435415..1753c6c 100644
--- a/drivers/power/supply/Kconfig
+++ b/drivers/power/supply/Kconfig
@@ -172,7 +172,8 @@ config CHARGER_SBS
config MANAGER_SBS
tristate "Smart Battery System Manager"
- depends on I2C && I2C_MUX
+ depends on I2C && I2C_MUX && GPIOLIB
+ select I2C_SMBUS
help
Say Y here to include support for Smart Battery System Manager
ICs. The driver reports online and charging status via sysfs.
diff --git a/drivers/power/supply/sbs-manager.c b/drivers/power/supply/sbs-manager.c
index 47c09a1..3e3ad09 100644
--- a/drivers/power/supply/sbs-manager.c
+++ b/drivers/power/supply/sbs-manager.c
@@ -16,10 +16,12 @@
* published by the Free Software Foundation.
*/
+#include <linux/gpio.h>
#include <linux/module.h>
#include <linux/i2c.h>
#include <linux/i2c-mux.h>
#include <linux/power_supply.h>
+#include <linux/property.h>
#define SBSM_MAX_BATS 4
#define SBSM_RETRY_CNT 3
@@ -43,7 +45,12 @@ struct sbsm_data {
struct power_supply *psy;
u8 cur_chan; /* currently selected channel */
+ struct gpio_chip chip;
bool is_ltc1760; /* special capabilities */
+
+ unsigned int supported_bats;
+ unsigned int last_state;
+ unsigned int last_state_cont;
};
static enum power_supply_property sbsm_props[] = {
@@ -186,6 +193,117 @@ static int sbsm_select(struct i2c_mux_core *muxc, u32 chan)
return ret;
}
+static int sbsm_gpio_get_value(struct gpio_chip *gc, unsigned off)
+{
+ struct sbsm_data *data = gpiochip_get_data(gc);
+ int ret;
+
+ ret = sbsm_read_word(data->client, SBSM_CMD_BATSYSSTATE);
+ if (ret < 0)
+ return ret;
+
+ return ret & BIT(off);
+}
+
+/*
+ * This needs to be defined or the GPIO lib fails to register the pin.
+ * But the 'gpio' is always an input.
+ */
+static int sbsm_gpio_direction_input(struct gpio_chip *gc, unsigned off)
+{
+ return 0;
+}
+
+static int sbsm_do_alert(struct device *dev, void *d)
+{
+ struct i2c_client *client = i2c_verify_client(dev);
+ struct i2c_driver *driver;
+
+ if (!client || client->addr != 0x0b)
+ return 0;
+
+ device_lock(dev);
+ if (client->dev.driver) {
+ driver = to_i2c_driver(client->dev.driver);
+ if (driver->alert)
+ driver->alert(client, I2C_PROTOCOL_SMBUS_ALERT, 0);
+ else
+ dev_warn(&client->dev, "no driver alert()!\n");
+ } else
+ dev_dbg(&client->dev, "alert with no driver\n");
+ device_unlock(dev);
+
+ return -EBUSY;
+}
+
+static void sbsm_alert(struct i2c_client *client, enum i2c_alert_protocol prot,
+ unsigned int d)
+{
+ struct sbsm_data *sbsm = i2c_get_clientdata(client);
+
+ int ret, i, irq_bat = 0, state = 0;
+
+ ret = sbsm_read_word(sbsm->client, SBSM_CMD_BATSYSSTATE);
+ if (ret >= 0) {
+ irq_bat = ret ^ sbsm->last_state;
+ sbsm->last_state = ret;
+ state = ret;
+ }
+
+ ret = sbsm_read_word(sbsm->client, SBSM_CMD_BATSYSSTATECONT);
+ if ((ret >= 0) &&
+ ((ret ^ sbsm->last_state_cont) & SBSM_BIT_AC_PRESENT)) {
+ irq_bat |= sbsm->supported_bats & state;
+ power_supply_changed(sbsm->psy);
+ }
+ sbsm->last_state_cont = ret;
+
+ for (i = 0; i < SBSM_MAX_BATS; i++) {
+ if (irq_bat & BIT(i)) {
+ device_for_each_child(&sbsm->muxc->adapter[i]->dev,
+ NULL, sbsm_do_alert);
+ }
+ }
+}
+
+static int sbsm_gpio_setup(struct sbsm_data *data)
+{
+ struct gpio_chip *gc = &data->chip;
+ struct i2c_client *client = data->client;
+ struct device *dev = &client->dev;
+ int ret;
+
+ if (!device_property_present(dev, "gpio-controller"))
+ return 0;
+
+ ret = sbsm_read_word(client, SBSM_CMD_BATSYSSTATE);
+ if (ret < 0)
+ return ret;
+ data->last_state = ret;
+
+ ret = sbsm_read_word(client, SBSM_CMD_BATSYSSTATECONT);
+ if (ret < 0)
+ return ret;
+ data->last_state_cont = ret;
+
+ gc->get = sbsm_gpio_get_value;
+ gc->direction_input = sbsm_gpio_direction_input;
+ gc->can_sleep = true;
+ gc->base = -1;
+ gc->ngpio = SBSM_MAX_BATS;
+ gc->label = client->name;
+ gc->parent = dev;
+ gc->owner = THIS_MODULE;
+
+ ret = devm_gpiochip_add_data(dev, gc, data);
+ if (ret) {
+ dev_err(dev, "devm_gpiochip_add_data failed: %d\n", ret);
+ return ret;
+ }
+
+ return ret;
+}
+
static const struct power_supply_desc sbsm_default_psy_desc = {
.type = POWER_SUPPLY_TYPE_MAINS,
.properties = sbsm_props,
@@ -203,7 +321,7 @@ static int sbsm_probe(struct i2c_client *client,
struct device *dev = &client->dev;
struct power_supply_desc *psy_desc;
struct power_supply_config psy_cfg = {};
- int ret = 0, i, supported_bats;
+ int ret = 0, i;
/* Device listens only at address 0x0a */
if (client->addr != 0x0a)
@@ -224,8 +342,7 @@ static int sbsm_probe(struct i2c_client *client,
ret = sbsm_read_word(client, SBSM_CMD_BATSYSINFO);
if (ret < 0)
return ret;
- supported_bats = ret & SBSM_MASK_BAT_SUPPORTED;
-
+ data->supported_bats = ret & SBSM_MASK_BAT_SUPPORTED;
data->muxc = i2c_mux_alloc(adapter, dev, SBSM_MAX_BATS, 0,
I2C_MUX_LOCKED, &sbsm_select, NULL);
if (!data->muxc) {
@@ -237,7 +354,7 @@ static int sbsm_probe(struct i2c_client *client,
/* register muxed i2c channels. One for each supported battery */
for (i = 0; i < SBSM_MAX_BATS; ++i) {
- if (supported_bats & BIT(i)) {
+ if (data->supported_bats & BIT(i)) {
ret = i2c_mux_add_adapter(data->muxc, 0, i + 1, 0);
if (ret)
break;
@@ -262,6 +379,9 @@ static int sbsm_probe(struct i2c_client *client,
ret = -ENOMEM;
goto err_psy;
}
+ ret = sbsm_gpio_setup(data);
+ if (ret < 0)
+ goto err_psy;
psy_cfg.drv_data = data;
psy_cfg.of_node = dev->of_node;
@@ -314,6 +434,7 @@ static int sbsm_remove(struct i2c_client *client)
},
.probe = sbsm_probe,
.remove = sbsm_remove,
+ .alert = sbsm_alert,
.id_table = sbsm_ids
};
module_i2c_driver(sbsm_driver);
--
1.8.3.1
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PATCH v7 10/10] power: supply: sbs-battery: move gpio present detect to sbs_get_property
2017-06-15 13:59 [PATCH v7 00/10] Add sbs-manager with smbalert support Phil Reid
` (8 preceding siblings ...)
2017-06-15 13:59 ` [PATCH v7 09/10] power: supply: sbs-manager: Add alert callback and battery change notification Phil Reid
@ 2017-06-15 13:59 ` Phil Reid
2017-07-03 15:21 ` Sebastian Reichel
9 siblings, 1 reply; 30+ messages in thread
From: Phil Reid @ 2017-06-15 13:59 UTC (permalink / raw)
To: wsa, robh+dt, mark.rutland, sre, jdelvare, jglauber, david.daney,
peda, preid, linux-i2c, devicetree, linux-pm
Currently when a gpio is defined for battery presence it is only used in
the sbs_get_battery_presence_and_health function for 2 properties.
All other properties currently try to read data form the battery before
returning an error if not present. We should know in advance that no
data is going to returned.
As the driver tries multiple times to access a property, this prevents
a lot of smbus accesses, which had a significant effect on device boot-up.
As when the device is registered lots of property accesses are attempted
during boot.
If no gpio is used for presence detection no change in behaviour should
occur.
Signed-off-by: Phil Reid <preid@electromag.com.au>
---
drivers/power/supply/sbs-battery.c | 23 +++++++++++++----------
1 file changed, 13 insertions(+), 10 deletions(-)
diff --git a/drivers/power/supply/sbs-battery.c b/drivers/power/supply/sbs-battery.c
index cf43e38..1e3a8b2 100644
--- a/drivers/power/supply/sbs-battery.c
+++ b/drivers/power/supply/sbs-battery.c
@@ -324,16 +324,6 @@ static int sbs_get_battery_presence_and_health(
union power_supply_propval *val)
{
s32 ret;
- struct sbs_info *chip = i2c_get_clientdata(client);
-
- if (psp == POWER_SUPPLY_PROP_PRESENT && chip->gpio_detect) {
- ret = gpiod_get_value_cansleep(chip->gpio_detect);
- if (ret < 0)
- return ret;
- val->intval = ret;
- chip->is_present = val->intval;
- return ret;
- }
/*
* Write to ManufacturerAccess with ManufacturerAccess command
@@ -599,6 +589,19 @@ static int sbs_get_property(struct power_supply *psy,
struct sbs_info *chip = power_supply_get_drvdata(psy);
struct i2c_client *client = chip->client;
+ if (chip->gpio_detect) {
+ ret = gpiod_get_value_cansleep(chip->gpio_detect);
+ if (ret < 0)
+ return ret;
+ if (psp == POWER_SUPPLY_PROP_PRESENT) {
+ val->intval = ret;
+ chip->is_present = val->intval;
+ return 0;
+ }
+ if (ret == 0)
+ return -ENODATA;
+ }
+
switch (psp) {
case POWER_SUPPLY_PROP_PRESENT:
case POWER_SUPPLY_PROP_HEALTH:
--
1.8.3.1
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PATCH v7 10/10] power: supply: sbs-battery: move gpio present detect to sbs_get_property
2017-06-15 13:59 ` [PATCH v7 10/10] power: supply: sbs-battery: move gpio present detect to sbs_get_property Phil Reid
@ 2017-07-03 15:21 ` Sebastian Reichel
0 siblings, 0 replies; 30+ messages in thread
From: Sebastian Reichel @ 2017-07-03 15:21 UTC (permalink / raw)
To: Phil Reid
Cc: wsa, robh+dt, mark.rutland, jdelvare, jglauber, david.daney,
peda, linux-i2c, devicetree, linux-pm
[-- Attachment #1: Type: text/plain, Size: 2355 bytes --]
Hi,
On Thu, Jun 15, 2017 at 09:59:38PM +0800, Phil Reid wrote:
> Currently when a gpio is defined for battery presence it is only used in
> the sbs_get_battery_presence_and_health function for 2 properties.
> All other properties currently try to read data form the battery before
> returning an error if not present. We should know in advance that no
> data is going to returned.
>
> As the driver tries multiple times to access a property, this prevents
> a lot of smbus accesses, which had a significant effect on device boot-up.
> As when the device is registered lots of property accesses are attempted
> during boot.
>
> If no gpio is used for presence detection no change in behaviour should
> occur.
>
> Signed-off-by: Phil Reid <preid@electromag.com.au>
> ---
> drivers/power/supply/sbs-battery.c | 23 +++++++++++++----------
> 1 file changed, 13 insertions(+), 10 deletions(-)
Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
-- Sebastian
> diff --git a/drivers/power/supply/sbs-battery.c b/drivers/power/supply/sbs-battery.c
> index cf43e38..1e3a8b2 100644
> --- a/drivers/power/supply/sbs-battery.c
> +++ b/drivers/power/supply/sbs-battery.c
> @@ -324,16 +324,6 @@ static int sbs_get_battery_presence_and_health(
> union power_supply_propval *val)
> {
> s32 ret;
> - struct sbs_info *chip = i2c_get_clientdata(client);
> -
> - if (psp == POWER_SUPPLY_PROP_PRESENT && chip->gpio_detect) {
> - ret = gpiod_get_value_cansleep(chip->gpio_detect);
> - if (ret < 0)
> - return ret;
> - val->intval = ret;
> - chip->is_present = val->intval;
> - return ret;
> - }
>
> /*
> * Write to ManufacturerAccess with ManufacturerAccess command
> @@ -599,6 +589,19 @@ static int sbs_get_property(struct power_supply *psy,
> struct sbs_info *chip = power_supply_get_drvdata(psy);
> struct i2c_client *client = chip->client;
>
> + if (chip->gpio_detect) {
> + ret = gpiod_get_value_cansleep(chip->gpio_detect);
> + if (ret < 0)
> + return ret;
> + if (psp == POWER_SUPPLY_PROP_PRESENT) {
> + val->intval = ret;
> + chip->is_present = val->intval;
> + return 0;
> + }
> + if (ret == 0)
> + return -ENODATA;
> + }
> +
> switch (psp) {
> case POWER_SUPPLY_PROP_PRESENT:
> case POWER_SUPPLY_PROP_HEALTH:
> --
> 1.8.3.1
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 30+ messages in thread