* [PATCH v6 1/8] i2c: i2c-smbus: Support threaded irqs.
2017-05-02 9:12 [PATCH v6 0/8] Add sbs-manager with smbalert support Phil Reid
@ 2017-05-02 9:12 ` Phil Reid
2017-05-02 9:12 ` [PATCH v6 2/8] i2c: i2c-smbus: Add null ptr guard in smb_alert_probe Phil Reid
` (5 subsequent siblings)
6 siblings, 0 replies; 17+ messages in thread
From: Phil Reid @ 2017-05-02 9:12 UTC (permalink / raw)
To: wsa, robh+dt, mark.rutland, sre, peda, preid, linux-i2c,
devicetree, linux-pm, benjamin.tissoires
handle_nested_irq calls the threaded irq handler. So if the smbus_alert
irq is being generated via this an null address is dereferenced. Split
irq up into separate functions to allow thread / non thread irq to work
correctly.
Signed-off-by: Phil Reid <preid@electromag.com.au>
---
drivers/i2c/i2c-smbus.c | 24 ++++++++++++++++++------
1 file changed, 18 insertions(+), 6 deletions(-)
diff --git a/drivers/i2c/i2c-smbus.c b/drivers/i2c/i2c-smbus.c
index f9271c7..b272493 100644
--- a/drivers/i2c/i2c-smbus.c
+++ b/drivers/i2c/i2c-smbus.c
@@ -72,13 +72,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,6 +114,17 @@ static void smbus_alert(struct work_struct *work)
prev_addr = data.addr;
}
+ return IRQ_HANDLED;
+}
+
+static void smbalert_work(struct work_struct *work)
+{
+ struct i2c_smbus_alert *alert;
+
+ alert = container_of(work, struct i2c_smbus_alert, alert);
+
+ smbus_alert(alert->irq, alert);
+
/* We handled all alerts; re-enable level-triggered IRQs */
if (!alert->alert_edge_triggered)
enable_irq(alert->irq);
@@ -148,12 +158,14 @@ static int smbalert_probe(struct i2c_client *ara,
alert->alert_edge_triggered = setup->alert_edge_triggered;
alert->irq = setup->irq;
- INIT_WORK(&alert->alert, smbus_alert);
+ 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);
+ res = devm_request_threaded_irq(&ara->dev, alert->irq,
+ smbalert_irq, smbus_alert,
+ IRQF_SHARED | IRQF_ONESHOT,
+ "smbus_alert", alert);
if (res)
return res;
}
--
1.8.3.1
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH v6 2/8] i2c: i2c-smbus: Add null ptr guard in smb_alert_probe
2017-05-02 9:12 [PATCH v6 0/8] Add sbs-manager with smbalert support Phil Reid
2017-05-02 9:12 ` [PATCH v6 1/8] i2c: i2c-smbus: Support threaded irqs Phil Reid
@ 2017-05-02 9:12 ` Phil Reid
[not found] ` <1493716346-58517-1-git-send-email-preid-qgqNFa1JUf/o2iN0hyhwsIdd74u8MsAO@public.gmane.org>
` (4 subsequent siblings)
6 siblings, 0 replies; 17+ messages in thread
From: Phil Reid @ 2017-05-02 9:12 UTC (permalink / raw)
To: wsa, robh+dt, mark.rutland, sre, peda, preid, linux-i2c,
devicetree, linux-pm, benjamin.tissoires
If platdata is not correct setup before registering the device
a null ptr derefence can occur.
Signed-off-by: Phil Reid <preid@electromag.com.au>
---
drivers/i2c/i2c-smbus.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/i2c/i2c-smbus.c b/drivers/i2c/i2c-smbus.c
index b272493..df0e2fa 100644
--- a/drivers/i2c/i2c-smbus.c
+++ b/drivers/i2c/i2c-smbus.c
@@ -151,6 +151,11 @@ static int smbalert_probe(struct i2c_client *ara,
struct i2c_adapter *adapter = ara->adapter;
int res;
+ if (!setup) {
+ dev_err(&adapter->dev, "setup not defined\n");
+ return -ENODATA;
+ }
+
alert = devm_kzalloc(&ara->dev, sizeof(struct i2c_smbus_alert),
GFP_KERNEL);
if (!alert)
--
1.8.3.1
^ permalink raw reply related [flat|nested] 17+ messages in thread[parent not found: <1493716346-58517-1-git-send-email-preid-qgqNFa1JUf/o2iN0hyhwsIdd74u8MsAO@public.gmane.org>]
* [PATCH v6 3/8] i2c: i2c-smbus: add of_i2c_setup_smbus_alert
[not found] ` <1493716346-58517-1-git-send-email-preid-qgqNFa1JUf/o2iN0hyhwsIdd74u8MsAO@public.gmane.org>
@ 2017-05-02 9:12 ` Phil Reid
2017-05-02 10:20 ` Benjamin Tissoires
2017-05-08 15:24 ` Rob Herring
2017-05-02 9:12 ` [PATCH v6 8/8] power: supply: sbs-manager: Add alert callback and battery change notification Phil Reid
1 sibling, 2 replies; 17+ messages in thread
From: Phil Reid @ 2017-05-02 9:12 UTC (permalink / raw)
To: wsa-z923LK4zBo2bacvFa/9K2g, robh+dt-DgEjT+Ai2ygdnm+yROfE0A,
mark.rutland-5wv7dgnIgG8, sre-DgEjT+Ai2ygdnm+yROfE0A,
peda-koto5C5qi+TLoDKTGw+V6w,
preid-qgqNFa1JUf/o2iN0hyhwsIdd74u8MsAO,
linux-i2c-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-pm-u79uwXL29TY76Z2rM5mHXA,
benjamin.tissoires-H+wXaHxf7aLQT0dZR+AlfA
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-qgqNFa1JUf/o2iN0hyhwsIdd74u8MsAO@public.gmane.org>
---
Documentation/devicetree/bindings/i2c/i2c.txt | 4 +--
drivers/i2c/i2c-smbus.c | 35 +++++++++++++++++++++++++++
include/linux/i2c-smbus.h | 9 +++++++
3 files changed, 46 insertions(+), 2 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 df0e2fa..a8f8439 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>
@@ -238,6 +239,40 @@ 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;
+ struct i2c_smbus_alert_setup *setup;
+ struct i2c_board_info info = {
+ I2C_BOARD_INFO("smbus_alert", 0x0c),
+ };
+ int irq;
+
+ if (!adap->dev.of_node)
+ return 0;
+
+ irq = of_irq_get_byname(adap->dev.of_node, "smbus_alert");
+ if (irq == -EINVAL || irq == -ENODATA)
+ return 0;
+ else if (irq < 0)
+ return irq;
+
+ setup = devm_kzalloc(&adap->dev, sizeof(struct i2c_smbus_alert_setup),
+ GFP_KERNEL);
+ if (!setup)
+ return -ENOMEM;
+
+ setup->irq = irq;
+ info.platform_data = setup;
+
+ client = i2c_new_device(adap, &info);
+ 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 a138502..4732d09 100644
--- a/include/linux/i2c-smbus.h
+++ b/include/linux/i2c-smbus.h
@@ -50,4 +50,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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH v6 3/8] i2c: i2c-smbus: add of_i2c_setup_smbus_alert
2017-05-02 9:12 ` [PATCH v6 3/8] i2c: i2c-smbus: add of_i2c_setup_smbus_alert Phil Reid
@ 2017-05-02 10:20 ` Benjamin Tissoires
2017-05-03 8:31 ` Phil Reid
2017-05-08 15:24 ` Rob Herring
1 sibling, 1 reply; 17+ messages in thread
From: Benjamin Tissoires @ 2017-05-02 10:20 UTC (permalink / raw)
To: Phil Reid
Cc: wsa, robh+dt, mark.rutland, sre, peda, linux-i2c, devicetree,
linux-pm
Hi Phil,
On May 02 2017 or thereabouts, Phil Reid wrote:
> 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>
> ---
> Documentation/devicetree/bindings/i2c/i2c.txt | 4 +--
> drivers/i2c/i2c-smbus.c | 35 +++++++++++++++++++++++++++
> include/linux/i2c-smbus.h | 9 +++++++
> 3 files changed, 46 insertions(+), 2 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 df0e2fa..a8f8439 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>
>
> @@ -238,6 +239,40 @@ 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;
> + struct i2c_smbus_alert_setup *setup;
> + struct i2c_board_info info = {
> + I2C_BOARD_INFO("smbus_alert", 0x0c),
> + };
Shouldn't you use i2c_setup_smbus_alert() instead of manually recreating
the board_info?
Also i2c_setup_smbus_alert() mentions that the level trigger has to be
explicitely mentioned, and I can't find if this is the case in your
patch.
> + int irq;
> +
> + if (!adap->dev.of_node)
> + return 0;
> +
> + irq = of_irq_get_byname(adap->dev.of_node, "smbus_alert");
> + if (irq == -EINVAL || irq == -ENODATA)
> + return 0;
> + else if (irq < 0)
> + return irq;
> +
> + setup = devm_kzalloc(&adap->dev, sizeof(struct i2c_smbus_alert_setup),
> + GFP_KERNEL);
Problem is i2c-core doesn't use devres at all for now. So the code is
correct here as it won't segfault but mixing both devres and non-devres
is error prone.
> + if (!setup)
> + return -ENOMEM;
> +
> + setup->irq = irq;
> + info.platform_data = setup;
> +
> + client = i2c_new_device(adap, &info);
> + 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 a138502..4732d09 100644
> --- a/include/linux/i2c-smbus.h
> +++ b/include/linux/i2c-smbus.h
> @@ -50,4 +50,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
Cheers,
Benjamin
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH v6 3/8] i2c: i2c-smbus: add of_i2c_setup_smbus_alert
2017-05-02 10:20 ` Benjamin Tissoires
@ 2017-05-03 8:31 ` Phil Reid
0 siblings, 0 replies; 17+ messages in thread
From: Phil Reid @ 2017-05-03 8:31 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: wsa, robh+dt, mark.rutland, sre, peda, linux-i2c, devicetree,
linux-pm
G'day Benjamin,
On 2/05/2017 18:20, Benjamin Tissoires wrote:
> Hi Phil,
>
> On May 02 2017 or thereabouts, Phil Reid wrote:
>> 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>
>> ---
>> Documentation/devicetree/bindings/i2c/i2c.txt | 4 +--
>> drivers/i2c/i2c-smbus.c | 35 +++++++++++++++++++++++++++
>> include/linux/i2c-smbus.h | 9 +++++++
>> 3 files changed, 46 insertions(+), 2 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 df0e2fa..a8f8439 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>
>>
>> @@ -238,6 +239,40 @@ 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;
>> + struct i2c_smbus_alert_setup *setup;
>> + struct i2c_board_info info = {
>> + I2C_BOARD_INFO("smbus_alert", 0x0c),
>> + };
>
> Shouldn't you use i2c_setup_smbus_alert() instead of manually recreating
> the board_info?
Yes looks like that could be cleaned up I think.
Look at i2c_new_device also it looks like client needs to be kept around and
i2c_unregister_device. I was assuming the apdater would clean it up.
But I'm not sure that's the case when looking again. That makes the change
more invasive into the adpater. would need to add an i2c_client ptr to the
i2c_adapter struct. Thoughts?
> Also i2c_setup_smbus_alert() mentions that the level trigger has to be
> explicitely mentioned, and I can't find if this is the case in your
> patch.
I'm not too sure how this wrks with the device tree / OF stuff.
But initally I had the device tree irq type set incorrectly and things
didn't work. Fixing that up and everything flow thru to the request irq
and was set for the correct mode. So I guessed there was some "magic" in
the core irq code somewhere.
I actually think the thread irq handler should be able to handle everything
and the work queue should disappear. Pretty sure the irq core handles disabling
level interrupts will the threaded handler services things now. But I don't
quite understand how the original code was handle edge vs level trigger irq's.
Not having access to test that I was reluctant to change the hard irq.
Any suggestions helpful...
>
>> + int irq;
>> +
>> + if (!adap->dev.of_node)
>> + return 0;
>> +
>> + irq = of_irq_get_byname(adap->dev.of_node, "smbus_alert");
>> + if (irq == -EINVAL || irq == -ENODATA)
>> + return 0;
>> + else if (irq < 0)
>> + return irq;
>> +
>> + setup = devm_kzalloc(&adap->dev, sizeof(struct i2c_smbus_alert_setup),
>> + GFP_KERNEL);
>
> Problem is i2c-core doesn't use devres at all for now. So the code is
> correct here as it won't segfault but mixing both devres and non-devres
> is error prone.
Ok I look at how to avoid the devm alloc.
>
>> + if (!setup)
>> + return -ENOMEM;
>> +
>> + setup->irq = irq;
>> + info.platform_data = setup;
>> +
>> + client = i2c_new_device(adap, &info);
>> + 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 a138502..4732d09 100644
>> --- a/include/linux/i2c-smbus.h
>> +++ b/include/linux/i2c-smbus.h
>> @@ -50,4 +50,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
>
> Cheers,
> Benjamin
>
>
--
Regards
Phil Reid
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v6 3/8] i2c: i2c-smbus: add of_i2c_setup_smbus_alert
2017-05-02 9:12 ` [PATCH v6 3/8] i2c: i2c-smbus: add of_i2c_setup_smbus_alert Phil Reid
2017-05-02 10:20 ` Benjamin Tissoires
@ 2017-05-08 15:24 ` Rob Herring
1 sibling, 0 replies; 17+ messages in thread
From: Rob Herring @ 2017-05-08 15:24 UTC (permalink / raw)
To: Phil Reid
Cc: wsa, mark.rutland, sre, peda, linux-i2c, devicetree, linux-pm,
benjamin.tissoires
On Tue, May 02, 2017 at 05:12:21PM +0800, Phil Reid wrote:
> 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>
> ---
> Documentation/devicetree/bindings/i2c/i2c.txt | 4 +--
Acked-by: Rob Herring <robh@kernel.org>
> drivers/i2c/i2c-smbus.c | 35 +++++++++++++++++++++++++++
> include/linux/i2c-smbus.h | 9 +++++++
> 3 files changed, 46 insertions(+), 2 deletions(-)
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v6 8/8] power: supply: sbs-manager: Add alert callback and battery change notification
[not found] ` <1493716346-58517-1-git-send-email-preid-qgqNFa1JUf/o2iN0hyhwsIdd74u8MsAO@public.gmane.org>
2017-05-02 9:12 ` [PATCH v6 3/8] i2c: i2c-smbus: add of_i2c_setup_smbus_alert Phil Reid
@ 2017-05-02 9:12 ` Phil Reid
2017-05-04 15:21 ` Sebastian Reichel
1 sibling, 1 reply; 17+ messages in thread
From: Phil Reid @ 2017-05-02 9:12 UTC (permalink / raw)
To: wsa-z923LK4zBo2bacvFa/9K2g, robh+dt-DgEjT+Ai2ygdnm+yROfE0A,
mark.rutland-5wv7dgnIgG8, sre-DgEjT+Ai2ygdnm+yROfE0A,
peda-koto5C5qi+TLoDKTGw+V6w,
preid-qgqNFa1JUf/o2iN0hyhwsIdd74u8MsAO,
linux-i2c-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-pm-u79uwXL29TY76Z2rM5mHXA,
benjamin.tissoires-H+wXaHxf7aLQT0dZR+AlfA
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-qgqNFa1JUf/o2iN0hyhwsIdd74u8MsAO@public.gmane.org>
---
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 8842f6e..6c34157 100644
--- a/drivers/power/supply/sbs-manager.c
+++ b/drivers/power/supply/sbs-manager.c
@@ -16,11 +16,13 @@
* 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/of_device.h>
#include <linux/power_supply.h>
+#include <linux/property.h>
#define SBSM_MAX_BATS 4
#define SBSM_RETRY_CNT 3
@@ -31,6 +33,8 @@
#define SBSM_CMD_BATSYSINFO 0x04
#define SBSM_CMD_LTC 0x3c
+#define SBSM_BIT_AC_PRESENT BIT(0)
+
struct sbsm_data {
struct i2c_client *client;
struct i2c_mux_core *muxc;
@@ -38,7 +42,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[] = {
@@ -181,6 +190,115 @@ 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;
+
+ ret = sbsm_read_word(sbsm->client, SBSM_CMD_BATSYSSTATE);
+ if (ret >= 0)
+ irq_bat = ret ^ sbsm->last_state;
+ sbsm->last_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;
+ 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 of_device_id sbsm_dt_ids[] = {
{ .compatible = "sbs,sbs-manager" },
{ .compatible = "lltc,ltc1760" },
@@ -204,7 +322,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)
@@ -225,8 +343,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 & 0xf;
-
+ data->supported_bats = ret & 0xf;
data->muxc = i2c_mux_alloc(adapter, dev, SBSM_MAX_BATS, 0,
I2C_MUX_LOCKED, &sbsm_select, NULL);
if (!data->muxc) {
@@ -238,7 +355,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) {
dev_err(dev,
@@ -263,6 +380,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;
@@ -308,6 +428,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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH v6 8/8] power: supply: sbs-manager: Add alert callback and battery change notification
2017-05-02 9:12 ` [PATCH v6 8/8] power: supply: sbs-manager: Add alert callback and battery change notification Phil Reid
@ 2017-05-04 15:21 ` Sebastian Reichel
0 siblings, 0 replies; 17+ messages in thread
From: Sebastian Reichel @ 2017-05-04 15:21 UTC (permalink / raw)
To: Phil Reid
Cc: wsa, robh+dt, mark.rutland, peda, linux-i2c, devicetree, linux-pm,
benjamin.tissoires
[-- Attachment #1: Type: text/plain, Size: 499 bytes --]
Hi,
On Tue, May 02, 2017 at 05:12:26PM +0800, Phil Reid wrote:
> 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>
-- Sebastian
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v6 4/8] i2c: core: call of_i2c_setup_smbus_alert in i2c_register_adapter
2017-05-02 9:12 [PATCH v6 0/8] Add sbs-manager with smbalert support Phil Reid
` (2 preceding siblings ...)
[not found] ` <1493716346-58517-1-git-send-email-preid-qgqNFa1JUf/o2iN0hyhwsIdd74u8MsAO@public.gmane.org>
@ 2017-05-02 9:12 ` Phil Reid
2017-05-02 9:12 ` [PATCH v6 5/8] i2c: mux: pca954x: Call request irq after adding mux segments Phil Reid
` (2 subsequent siblings)
6 siblings, 0 replies; 17+ messages in thread
From: Phil Reid @ 2017-05-02 9:12 UTC (permalink / raw)
To: wsa, robh+dt, mark.rutland, sre, peda, preid, linux-i2c,
devicetree, linux-pm, benjamin.tissoires
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] 17+ messages in thread* [PATCH v6 5/8] i2c: mux: pca954x: Call request irq after adding mux segments
2017-05-02 9:12 [PATCH v6 0/8] Add sbs-manager with smbalert support Phil Reid
` (3 preceding siblings ...)
2017-05-02 9:12 ` [PATCH v6 4/8] i2c: core: call of_i2c_setup_smbus_alert in i2c_register_adapter Phil Reid
@ 2017-05-02 9:12 ` Phil Reid
2017-05-02 9:57 ` Peter Rosin
2017-05-02 9:12 ` [PATCH v6 6/8] Documentation: Add sbs-manager device tree node documentation Phil Reid
2017-05-02 9:12 ` [PATCH v6 7/8] power: Adds support for Smart Battery System Manager Phil Reid
6 siblings, 1 reply; 17+ messages in thread
From: Phil Reid @ 2017-05-02 9:12 UTC (permalink / raw)
To: wsa, robh+dt, mark.rutland, sre, peda, preid, linux-i2c,
devicetree, linux-pm, benjamin.tissoires
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.
Signed-off-by: Phil Reid <preid@electromag.com.au>
---
drivers/i2c/muxes/i2c-mux-pca954x.c | 52 +++++++++++++++++--------------------
1 file changed, 24 insertions(+), 28 deletions(-)
diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c
index ad31d21..4299738 100644
--- a/drivers/i2c/muxes/i2c-mux-pca954x.c
+++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
@@ -294,7 +294,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;
@@ -314,24 +314,22 @@ static int pca954x_irq_setup(struct i2c_mux_core *muxc)
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);
}
/*
@@ -422,6 +420,14 @@ static int pca954x_probe(struct i2c_client *client,
}
}
+ if (data->chip->has_irq || client->irq > 0) {
+ 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
@@ -430,25 +436,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] 17+ messages in thread* Re: [PATCH v6 5/8] i2c: mux: pca954x: Call request irq after adding mux segments
2017-05-02 9:12 ` [PATCH v6 5/8] i2c: mux: pca954x: Call request irq after adding mux segments Phil Reid
@ 2017-05-02 9:57 ` Peter Rosin
2017-05-03 8:36 ` Phil Reid
0 siblings, 1 reply; 17+ messages in thread
From: Peter Rosin @ 2017-05-02 9:57 UTC (permalink / raw)
To: Phil Reid, wsa, robh+dt, mark.rutland, sre, linux-i2c, devicetree,
linux-pm, benjamin.tissoires
On 2017-05-02 11:12, Phil Reid wrote:
> 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.
>
> Signed-off-by: Phil Reid <preid@electromag.com.au>
> ---
> drivers/i2c/muxes/i2c-mux-pca954x.c | 52 +++++++++++++++++--------------------
> 1 file changed, 24 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c
> index ad31d21..4299738 100644
> --- a/drivers/i2c/muxes/i2c-mux-pca954x.c
> +++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
> @@ -294,7 +294,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;
> @@ -314,24 +314,22 @@ static int pca954x_irq_setup(struct i2c_mux_core *muxc)
> 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);
> }
>
> /*
> @@ -422,6 +420,14 @@ static int pca954x_probe(struct i2c_client *client,
> }
> }
>
> + if (data->chip->has_irq || client->irq > 0) {
This should be && instead of ||. However, I think it's better to not
try to replicate the inverse of the test in pca954x_irq_setup and
instead just check if the irq domain is there with "if (data->irq)".
Assuming that is the intent...
> + ret = devm_request_threaded_irq(&client->dev, data->client->irq,
> + NULL, pca954x_irq_handler, IRQF_ONESHOT | IRQF_SHARED,
> + "pca954x", data);
The indentation is horrific. Please fix.
Acked with those two fixed.
I also noticed that there is no check for failure of the call to
irq_create_mapping in pca954x_irq_setup. For bonus points, can you
fix that error path too, please? Or should failure to create those
irq mappings not be fatal for some reason?
Cheers,
peda
> + 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
> @@ -430,25 +436,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;
> }
>
>
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH v6 5/8] i2c: mux: pca954x: Call request irq after adding mux segments
2017-05-02 9:57 ` Peter Rosin
@ 2017-05-03 8:36 ` Phil Reid
0 siblings, 0 replies; 17+ messages in thread
From: Phil Reid @ 2017-05-03 8:36 UTC (permalink / raw)
To: Peter Rosin, wsa, robh+dt, mark.rutland, sre, linux-i2c,
devicetree, linux-pm, benjamin.tissoires
On 2/05/2017 17:57, Peter Rosin wrote:
> On 2017-05-02 11:12, Phil Reid wrote:
>> 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.
>>
>> Signed-off-by: Phil Reid <preid@electromag.com.au>
>> ---
>> drivers/i2c/muxes/i2c-mux-pca954x.c | 52 +++++++++++++++++--------------------
>> 1 file changed, 24 insertions(+), 28 deletions(-)
>>
>> diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c
>> index ad31d21..4299738 100644
>> --- a/drivers/i2c/muxes/i2c-mux-pca954x.c
>> +++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
>> @@ -294,7 +294,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;
>> @@ -314,24 +314,22 @@ static int pca954x_irq_setup(struct i2c_mux_core *muxc)
>> 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);
>> }
>>
>> /*
>> @@ -422,6 +420,14 @@ static int pca954x_probe(struct i2c_client *client,
>> }
>> }
>>
>> + if (data->chip->has_irq || client->irq > 0) {
>
> This should be && instead of ||. However, I think it's better to not
> try to replicate the inverse of the test in pca954x_irq_setup and
> instead just check if the irq domain is there with "if (data->irq)".
> Assuming that is the intent...
Yeah, that's better.
>
>> + ret = devm_request_threaded_irq(&client->dev, data->client->irq,
>> + NULL, pca954x_irq_handler, IRQF_ONESHOT | IRQF_SHARED,
>> + "pca954x", data);
>
> The indentation is horrific. Please fix.
oops
>
> Acked with those two fixed.
>
> I also noticed that there is no check for failure of the call to
> irq_create_mapping in pca954x_irq_setup. For bonus points, can you
> fix that error path too, please? Or should failure to create those
> irq mappings not be fatal for some reason?
Seems reasonable to fail. I'll fix that as well.
>
> Cheers,
> peda
>
>> + 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
>> @@ -430,25 +436,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;
>> }
>>
>>
>
>
>
--
Regards
Phil Reid
ElectroMagnetic Imaging Technology Pty Ltd
Development of Geophysical Instrumentation & Software
www.electromag.com.au
3 The Avenue, Midland WA 6056, AUSTRALIA
Ph: +61 8 9250 8100
Fax: +61 8 9250 7100
Email: preid@electromag.com.au
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v6 6/8] Documentation: Add sbs-manager device tree node documentation
2017-05-02 9:12 [PATCH v6 0/8] Add sbs-manager with smbalert support Phil Reid
` (4 preceding siblings ...)
2017-05-02 9:12 ` [PATCH v6 5/8] i2c: mux: pca954x: Call request irq after adding mux segments Phil Reid
@ 2017-05-02 9:12 ` Phil Reid
2017-05-08 15:26 ` Rob Herring
2017-05-02 9:12 ` [PATCH v6 7/8] power: Adds support for Smart Battery System Manager Phil Reid
6 siblings, 1 reply; 17+ messages in thread
From: Phil Reid @ 2017-05-02 9:12 UTC (permalink / raw)
To: wsa, robh+dt, mark.rutland, sre, peda, preid, linux-i2c,
devicetree, linux-pm, benjamin.tissoires
Cc: Karl-Heinz Schneider
From: Karl-Heinz Schneider <karl-heinz@schneider-inet.de>
This patch adds device tree documentation for the sbs-manager
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>
---
.../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..389016c
--- /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@0a {
+ 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@0b {
+ 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@0b {
+ 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@0b {
+ 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] 17+ messages in thread* Re: [PATCH v6 6/8] Documentation: Add sbs-manager device tree node documentation
2017-05-02 9:12 ` [PATCH v6 6/8] Documentation: Add sbs-manager device tree node documentation Phil Reid
@ 2017-05-08 15:26 ` Rob Herring
0 siblings, 0 replies; 17+ messages in thread
From: Rob Herring @ 2017-05-08 15:26 UTC (permalink / raw)
To: Phil Reid
Cc: wsa, mark.rutland, sre, peda, linux-i2c, devicetree, linux-pm,
benjamin.tissoires, Karl-Heinz Schneider
On Tue, May 02, 2017 at 05:12:24PM +0800, Phil Reid wrote:
> From: Karl-Heinz Schneider <karl-heinz@schneider-inet.de>
>
> This patch adds device tree documentation for the sbs-manager
>
> 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>
> ---
> .../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..389016c
> --- /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@0a {
Drop the leading 0.
> + 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@0b {
Drop the leading 0.
> + 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@0b {
And here...
With that,
Acked-by: Rob Herring <robh@kernel.org>
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v6 7/8] power: Adds support for Smart Battery System Manager
2017-05-02 9:12 [PATCH v6 0/8] Add sbs-manager with smbalert support Phil Reid
` (5 preceding siblings ...)
2017-05-02 9:12 ` [PATCH v6 6/8] Documentation: Add sbs-manager device tree node documentation Phil Reid
@ 2017-05-02 9:12 ` Phil Reid
2017-05-04 15:18 ` Sebastian Reichel
6 siblings, 1 reply; 17+ messages in thread
From: Phil Reid @ 2017-05-02 9:12 UTC (permalink / raw)
To: wsa, robh+dt, mark.rutland, sre, peda, preid, linux-i2c,
devicetree, linux-pm, benjamin.tissoires
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 | 317 +++++++++++++++++++++++++++++++++++++
3 files changed, 331 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..8842f6e
--- /dev/null
+++ b/drivers/power/supply/sbs-manager.c
@@ -0,0 +1,317 @@
+/*
+ * 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/of_device.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
+
+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 & 0x1);
+ break;
+
+ case POWER_SUPPLY_PROP_CHARGE_TYPE:
+ regval = sbsm_read_word(data->client, SBSM_CMD_BATSYSSTATE);
+ if (regval < 0)
+ return regval;
+
+ if ((regval & 0x00f0) == 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 & 0x80)
+ 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 ? (0x1 << 7) : 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 << (11 + 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 of_device_id sbsm_dt_ids[] = {
+ { .compatible = "sbs,sbs-manager" },
+ { .compatible = "lltc,ltc1760" },
+ { }
+};
+
+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 & 0xf;
+
+ 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) {
+ 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;
+ }
+
+ dev_info(dev, "sbsm registered\n");
+ 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);
+MODULE_DEVICE_TABLE(of, sbsm_dt_ids);
+
+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] 17+ messages in thread* Re: [PATCH v6 7/8] power: Adds support for Smart Battery System Manager
2017-05-02 9:12 ` [PATCH v6 7/8] power: Adds support for Smart Battery System Manager Phil Reid
@ 2017-05-04 15:18 ` Sebastian Reichel
0 siblings, 0 replies; 17+ messages in thread
From: Sebastian Reichel @ 2017-05-04 15:18 UTC (permalink / raw)
To: Phil Reid
Cc: wsa, robh+dt, mark.rutland, peda, linux-i2c, devicetree, linux-pm,
benjamin.tissoires, Karl-Heinz Schneider
[-- Attachment #1: Type: text/plain, Size: 12822 bytes --]
Hi Phil,
I spotted a few more small issues.
On Tue, May 02, 2017 at 05:12:25PM +0800, Phil Reid wrote:
> 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 | 317 +++++++++++++++++++++++++++++++++++++
> 3 files changed, 331 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..8842f6e
> --- /dev/null
> +++ b/drivers/power/supply/sbs-manager.c
> @@ -0,0 +1,317 @@
> +/*
> + * 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/of_device.h>
I think you do not need of_device.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
> +
> +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 & 0x1);
Please add a define for 0x1 and use BIT(0).
> + break;
> +
> + case POWER_SUPPLY_PROP_CHARGE_TYPE:
> + regval = sbsm_read_word(data->client, SBSM_CMD_BATSYSSTATE);
> + if (regval < 0)
> + return regval;
> +
> + if ((regval & 0x00f0) == 0) {
Please add a define for 0x00f0.
> + 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 & 0x80)
Please add a define for 0x80 and use BIT(7).
> + 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 ? (0x1 << 7) : 0;
Use the same define here. Also is it ok to reset the other bits
to 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 << (11 + chan);
use BIT macro and add define for 11.
> + 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 of_device_id sbsm_dt_ids[] = {
> + { .compatible = "sbs,sbs-manager" },
> + { .compatible = "lltc,ltc1760" },
> + { }
> +};
No need to have this here, so move it down next to the i2c
device id table.
> +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 & 0xf;
Let's also have a define for this 0xf.
> + 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) {
> + dev_err(dev,
> + "failed to register i2c mux channel %d\n",
> + i + 1);
> + goto err_mux_register;
> + }
how about
if (ret)
break;
and then after the for loop
if (ret) {
dev_err(dev, ...);
goto err_mux_register;
}
that way the dev_err should fit into one line.
> + }
> + }
> +
> + 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;
> + }
> +
> + dev_info(dev, "sbsm registered\n");
No need to spam boot log. Drop or use dev_dbg.
> + 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);
> +MODULE_DEVICE_TABLE(of, sbsm_dt_ids);
You can put MODULE_DEVICE_TABLE(of, sbsm_dt_ids) together
with the actual table inside of #ifdef CONFIG_OF to save
some bytes on architectures not using device tree.
> +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");
-- Sebastian
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 17+ messages in thread