| Message ID | 1493716346-58517-4-git-send-email-preid@electromag.com.au |
|---|---|
| State | Superseded |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 > >
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(-) -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 */
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(-)