| Message ID | 20200113091521.5754-1-m.felsch@pengutronix.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | watchdog: da9062: make restart handler atomic safe | expand |
On Mon, Jan 13, 2020 at 10:15:21AM +0100, Marco Felsch wrote: > The restart handler is executed during the shutdown phase which is > atomic/irq-less. The i2c framework supports atomic transfers since > commit 63b96983a5dd ("i2c: core: introduce callbacks for atomic > transfers") but unfortunately the regmap framework doesn't support it > yet. Hard coding the i2c stuff can be done without worries since the > DA9062 is an i2c-only device. > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> > --- > Hi, > > This patch is based on Stefan Lengfeld's RFC Patch [1]. > > [1] https://patchwork.ozlabs.org/patch/1085942/ > --- > drivers/watchdog/da9062_wdt.c | 15 +++++++++++---- > 1 file changed, 11 insertions(+), 4 deletions(-) > > diff --git a/drivers/watchdog/da9062_wdt.c b/drivers/watchdog/da9062_wdt.c > index c9b9d6394525..84c5a0a455b2 100644 > --- a/drivers/watchdog/da9062_wdt.c > +++ b/drivers/watchdog/da9062_wdt.c > @@ -11,6 +11,7 @@ > #include <linux/platform_device.h> > #include <linux/uaccess.h> > #include <linux/slab.h> > +#include <linux/i2c.h> > #include <linux/delay.h> > #include <linux/jiffies.h> > #include <linux/mfd/da9062/registers.h> > @@ -149,12 +150,18 @@ static int da9062_wdt_restart(struct watchdog_device *wdd, unsigned long action, > void *data) > { > struct da9062_watchdog *wdt = watchdog_get_drvdata(wdd); > + struct i2c_client *client = to_i2c_client(wdt->hw->dev); > + u8 buf[] = {DA9062AA_CONTROL_F, DA9062AA_SHUTDOWN_MASK}; > + struct i2c_msg msg = { > + .addr = client->addr, > + .flags = 0, > + .len = sizeof(buf), > + .buf = buf, > + }; > int ret; > > - ret = regmap_write(wdt->hw->regmap, > - DA9062AA_CONTROL_F, > - DA9062AA_SHUTDOWN_MASK); > - if (ret) > + ret = i2c_transfer(client->adapter, &msg, 1); Why not i2c_smbus_write_byte_data() ? I don't immediately see the difference. Guenter > + if (ret < 0) > dev_alert(wdt->hw->dev, "Failed to shutdown (err = %d)\n", > ret); > > -- > 2.20.1 >
Hi, On 20-01-13 12:28, Guenter Roeck wrote: > On Mon, Jan 13, 2020 at 10:15:21AM +0100, Marco Felsch wrote: > > The restart handler is executed during the shutdown phase which is > > atomic/irq-less. The i2c framework supports atomic transfers since > > commit 63b96983a5dd ("i2c: core: introduce callbacks for atomic > > transfers") but unfortunately the regmap framework doesn't support it > > yet. Hard coding the i2c stuff can be done without worries since the > > DA9062 is an i2c-only device. > > > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> > > --- > > Hi, > > > > This patch is based on Stefan Lengfeld's RFC Patch [1]. > > > > [1] https://patchwork.ozlabs.org/patch/1085942/ > > --- > > drivers/watchdog/da9062_wdt.c | 15 +++++++++++---- > > 1 file changed, 11 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/watchdog/da9062_wdt.c b/drivers/watchdog/da9062_wdt.c > > index c9b9d6394525..84c5a0a455b2 100644 > > --- a/drivers/watchdog/da9062_wdt.c > > +++ b/drivers/watchdog/da9062_wdt.c > > @@ -11,6 +11,7 @@ > > #include <linux/platform_device.h> > > #include <linux/uaccess.h> > > #include <linux/slab.h> > > +#include <linux/i2c.h> > > #include <linux/delay.h> > > #include <linux/jiffies.h> > > #include <linux/mfd/da9062/registers.h> > > @@ -149,12 +150,18 @@ static int da9062_wdt_restart(struct watchdog_device *wdd, unsigned long action, > > void *data) > > { > > struct da9062_watchdog *wdt = watchdog_get_drvdata(wdd); > > + struct i2c_client *client = to_i2c_client(wdt->hw->dev); > > + u8 buf[] = {DA9062AA_CONTROL_F, DA9062AA_SHUTDOWN_MASK}; > > + struct i2c_msg msg = { > > + .addr = client->addr, > > + .flags = 0, > > + .len = sizeof(buf), > > + .buf = buf, > > + }; > > int ret; > > > > - ret = regmap_write(wdt->hw->regmap, > > - DA9062AA_CONTROL_F, > > - DA9062AA_SHUTDOWN_MASK); > > - if (ret) > > + ret = i2c_transfer(client->adapter, &msg, 1); > > Why not i2c_smbus_write_byte_data() ? I don't immediately see the difference. Because I didn't noticed it, sorry. I changed it and notice no differences. Thanks for the review =) Regards, Marco > Guenter > > > + if (ret < 0) > > dev_alert(wdt->hw->dev, "Failed to shutdown (err = %d)\n", > > ret); > > > > -- > > 2.20.1 > >
Hi Marco, On Mon, Jan 13, 2020 at 10:15:21AM +0100, Marco Felsch wrote: > The restart handler is executed during the shutdown phase which is > atomic/irq-less. The i2c framework supports atomic transfers since > commit 63b96983a5dd ("i2c: core: introduce callbacks for atomic > transfers") but unfortunately the regmap framework doesn't support it > yet. Hard coding the i2c stuff can be done without worries since the > DA9062 is an i2c-only device. > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> Reviewed-by: Stefan Lengfeld <contact@stefanchrist.eu> Thanks for picking up the patch and submitting it. Kind regards, Stefan
Hi Stefan, On 20-01-14 21:00, Stefan Lengfeld wrote: > Hi Marco, > > On Mon, Jan 13, 2020 at 10:15:21AM +0100, Marco Felsch wrote: > > The restart handler is executed during the shutdown phase which is > > atomic/irq-less. The i2c framework supports atomic transfers since > > commit 63b96983a5dd ("i2c: core: introduce callbacks for atomic > > transfers") but unfortunately the regmap framework doesn't support it > > yet. Hard coding the i2c stuff can be done without worries since the > > DA9062 is an i2c-only device. > > > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> > > Reviewed-by: Stefan Lengfeld <contact@stefanchrist.eu> > > Thanks for picking up the patch and submitting it. I will send a v2 to cover Guenter's suggestion. Can I keep your reviewed by tag? Regards, Marco > Kind regards, > Stefan
diff --git a/drivers/watchdog/da9062_wdt.c b/drivers/watchdog/da9062_wdt.c index c9b9d6394525..84c5a0a455b2 100644 --- a/drivers/watchdog/da9062_wdt.c +++ b/drivers/watchdog/da9062_wdt.c @@ -11,6 +11,7 @@ #include <linux/platform_device.h> #include <linux/uaccess.h> #include <linux/slab.h> +#include <linux/i2c.h> #include <linux/delay.h> #include <linux/jiffies.h> #include <linux/mfd/da9062/registers.h> @@ -149,12 +150,18 @@ static int da9062_wdt_restart(struct watchdog_device *wdd, unsigned long action, void *data) { struct da9062_watchdog *wdt = watchdog_get_drvdata(wdd); + struct i2c_client *client = to_i2c_client(wdt->hw->dev); + u8 buf[] = {DA9062AA_CONTROL_F, DA9062AA_SHUTDOWN_MASK}; + struct i2c_msg msg = { + .addr = client->addr, + .flags = 0, + .len = sizeof(buf), + .buf = buf, + }; int ret; - ret = regmap_write(wdt->hw->regmap, - DA9062AA_CONTROL_F, - DA9062AA_SHUTDOWN_MASK); - if (ret) + ret = i2c_transfer(client->adapter, &msg, 1); + if (ret < 0) dev_alert(wdt->hw->dev, "Failed to shutdown (err = %d)\n", ret);
The restart handler is executed during the shutdown phase which is atomic/irq-less. The i2c framework supports atomic transfers since commit 63b96983a5dd ("i2c: core: introduce callbacks for atomic transfers") but unfortunately the regmap framework doesn't support it yet. Hard coding the i2c stuff can be done without worries since the DA9062 is an i2c-only device. Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> --- Hi, This patch is based on Stefan Lengfeld's RFC Patch [1]. [1] https://patchwork.ozlabs.org/patch/1085942/ --- drivers/watchdog/da9062_wdt.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-)