| Message ID | 1429693667-28942-1-git-send-email-pascal.huerst@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
few nitpicks: i2c: omap: in $subject instead of omap: i2c: I usually would do: git log --no-merges --oneline drivers/i2c/busses/i2c-omap.c and look for common usage. On 04/22/2015 04:07 AM, pascal.huerst@gmail.com wrote: > From: Pascal Huerst <pascal.huerst@gmail.com> > > This adds calls to pinctrl subsystem in order to switch pin states > on suspend/resume if you provide a "sleep" state in DT. > > Signed-off-by: Pascal Huerst <pascal.huerst@gmail.com> > --- > drivers/i2c/busses/i2c-omap.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c > index 0e89419..8261941 100644 > --- a/drivers/i2c/busses/i2c-omap.c > +++ b/drivers/i2c/busses/i2c-omap.c > @@ -38,6 +38,7 @@ > #include <linux/slab.h> > #include <linux/i2c-omap.h> > #include <linux/pm_runtime.h> > +#include <linux/pinctrl/consumer.h> > > /* I2C controller revisions */ > #define OMAP_I2C_OMAP1_REV_2 0x20 > @@ -1423,6 +1424,8 @@ static int omap_i2c_runtime_suspend(struct device *dev) > omap_i2c_read_reg(_dev, OMAP_I2C_STAT_REG); > } > > + pinctrl_pm_select_sleep_state(dev); > + > return 0; > } > > @@ -1431,6 +1434,8 @@ static int omap_i2c_runtime_resume(struct device *dev) > struct platform_device *pdev = to_platform_device(dev); > struct omap_i2c_dev *_dev = platform_get_drvdata(pdev); > > + pinctrl_pm_select_default_state(dev); > + > if (!_dev->regs) > return 0; > > -- > 2.1.0 > Build tested with omap2plus_defconfig on linus master (27cf3a16b253) and next-20150422 While I do understand that the sleep state and default states are optional - might be nice to state it in commit message (as a result of which we dont do error checks). Would you think adding relevant documentation in Documentation/devicetree/bindings/i2c/i2c-omap.txt might be good as well? I mean, folks should know looking at dt documentation that this bus driver does indeed support this option..
On 23.04.2015 02:32, Nishanth Menon wrote: > few nitpicks: > i2c: omap: in $subject instead of omap: i2c: > I usually would do: > git log --no-merges --oneline drivers/i2c/busses/i2c-omap.c > and look for common usage. Aargh, thanks for pointing that out, I'll fix that. > On 04/22/2015 04:07 AM, pascal.huerst@gmail.com wrote: >> From: Pascal Huerst <pascal.huerst@gmail.com> >> >> This adds calls to pinctrl subsystem in order to switch pin states >> on suspend/resume if you provide a "sleep" state in DT. >> >> Signed-off-by: Pascal Huerst <pascal.huerst@gmail.com> >> --- >> drivers/i2c/busses/i2c-omap.c | 5 +++++ >> 1 file changed, 5 insertions(+) >> >> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c >> index 0e89419..8261941 100644 >> --- a/drivers/i2c/busses/i2c-omap.c >> +++ b/drivers/i2c/busses/i2c-omap.c >> @@ -38,6 +38,7 @@ >> #include <linux/slab.h> >> #include <linux/i2c-omap.h> >> #include <linux/pm_runtime.h> >> +#include <linux/pinctrl/consumer.h> >> >> /* I2C controller revisions */ >> #define OMAP_I2C_OMAP1_REV_2 0x20 >> @@ -1423,6 +1424,8 @@ static int omap_i2c_runtime_suspend(struct device *dev) >> omap_i2c_read_reg(_dev, OMAP_I2C_STAT_REG); >> } >> >> + pinctrl_pm_select_sleep_state(dev); >> + >> return 0; >> } >> >> @@ -1431,6 +1434,8 @@ static int omap_i2c_runtime_resume(struct device *dev) >> struct platform_device *pdev = to_platform_device(dev); >> struct omap_i2c_dev *_dev = platform_get_drvdata(pdev); >> >> + pinctrl_pm_select_default_state(dev); >> + >> if (!_dev->regs) >> return 0; >> >> -- >> 2.1.0 >> > Build tested with omap2plus_defconfig on linus master (27cf3a16b253) > and next-20150422 > While I do understand that the sleep state and default states are > optional - might be nice to state it in commit message (as a result of > which we dont do error checks). Thanks for testing. I'll fix the commit message and send in v3. > Would you think adding relevant documentation in > Documentation/devicetree/bindings/i2c/i2c-omap.txt might be good as > well? I mean, folks should know looking at dt documentation that this > bus driver does indeed support this option.. I'm not sure about that. Actually this is all handled by the pinctrl subsystem, so I would guess that documentation belongs there, since in: Documentation/devicetree/bindings/i2c/i2c-at91.txt Documentation/devicetree/bindings/i2c/i2c-nomadik.txt is no documentation about that as well. But there is documentation in: Documentation/pinctrl.txt right? Otherwise, I would say we should add documentation for all devices, so it is consistent. -- 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
On 04/28/2015 06:05 AM, Pascal Huerst wrote: [...] > On 23.04.2015 02:32, Nishanth Menon wrote: [...] >> Would you think adding relevant documentation in >> Documentation/devicetree/bindings/i2c/i2c-omap.txt might be good as >> well? I mean, folks should know looking at dt documentation that this >> bus driver does indeed support this option.. > > I'm not sure about that. Actually this is all handled by the pinctrl > subsystem, so I would guess that documentation belongs there, since in: > > Documentation/devicetree/bindings/i2c/i2c-at91.txt > Documentation/devicetree/bindings/i2c/i2c-nomadik.txt > > is no documentation about that as well. But there is documentation in: > > Documentation/pinctrl.txt > > right? Otherwise, I would say we should add documentation for all > devices, so it is consistent. that is probably good since drivers may or maynot support support the sleep configuration.
On 04/28/2015 06:05 AM, Pascal Huerst wrote: [...] > On 23.04.2015 02:32, Nishanth Menon wrote: [...] >> Would you think adding relevant documentation in >> Documentation/devicetree/bindings/i2c/i2c-omap.txt might be good as >> well? I mean, folks should know looking at dt documentation that this >> bus driver does indeed support this option.. > > I'm not sure about that. Actually this is all handled by the pinctrl > subsystem, so I would guess that documentation belongs there, since in: > > Documentation/devicetree/bindings/i2c/i2c-at91.txt > Documentation/devicetree/bindings/i2c/i2c-nomadik.txt > > is no documentation about that as well. But there is documentation in: > > Documentation/pinctrl.txt > > right? Otherwise, I would say we should add documentation for all > devices, so it is consistent. that is probably good since drivers may or maynot support the sleep configuration.
diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c index 0e89419..8261941 100644 --- a/drivers/i2c/busses/i2c-omap.c +++ b/drivers/i2c/busses/i2c-omap.c @@ -38,6 +38,7 @@ #include <linux/slab.h> #include <linux/i2c-omap.h> #include <linux/pm_runtime.h> +#include <linux/pinctrl/consumer.h> /* I2C controller revisions */ #define OMAP_I2C_OMAP1_REV_2 0x20 @@ -1423,6 +1424,8 @@ static int omap_i2c_runtime_suspend(struct device *dev) omap_i2c_read_reg(_dev, OMAP_I2C_STAT_REG); } + pinctrl_pm_select_sleep_state(dev); + return 0; } @@ -1431,6 +1434,8 @@ static int omap_i2c_runtime_resume(struct device *dev) struct platform_device *pdev = to_platform_device(dev); struct omap_i2c_dev *_dev = platform_get_drvdata(pdev); + pinctrl_pm_select_default_state(dev); + if (!_dev->regs) return 0;