RE: [PATCH] tty: Add MOXA NPort Real TTY Driver
Hi Jiri,
Thanks for your good coding review for this patch, it helps us a lot!
> From: Jiri Slaby
> Sent: Tuesday, July 14, 2020 5:34 PM
> To: Johnson CH Chen (陳昭勳) ; Greg
> Kroah-Hartman
> Cc: linux-kernel@vger.kernel.org; linux-ser...@vger.kernel.org
> Subject: Re: [PATCH] tty: Add MOXA NPort Real TTY Driver
>
> On 14. 07. 20, 8:24, Johnson CH Chen (陳昭勳) wrote:
> > This driver supports tty functions for all of MOXA's NPort series with
> > v5.0. Using this driver, host part can use tty to connect NPort device
> > server by ethernet.
> ...
> > Signed-off-by: Johnson Chen
> > Signed-off-by: Jason Chen
> > Signed-off-by: Danny Lin
> > Signed-off-by: Victor Yu
> > ---
> ...
> > --- a/drivers/tty/Kconfig
> > +++ b/drivers/tty/Kconfig
> > @@ -259,6 +259,17 @@ config MOXA_SMARTIO
> > This driver can also be built as a module. The module will be called
> > mxser. If you want to do that, say M here.
> >
> > +config MOXA_NPORT_REAL_TTY
> > + tristate "Moxa NPort Real TTY support v5.0"
>
> MOXA_SMARTIO is lexicographically after MOXA_NPORT_REAL_TTY. So move
> this before MOXA_SMARTIO.
>
> > + help
> > + Say Y here if you have a Moxa NPort serial device server.
> > +
> > + The purpose of this driver is to map NPort serial port to host tty
> > + port. Using this driver, you can use NPort serial port as local tty
> > port.
> > +
> > + This driver can also be built as a module. The module will be called
> > + npreal2 by setting M.
> > +
> > config SYNCLINK
> > tristate "Microgate SyncLink card support"
> > depends on SERIAL_NONSTANDARD && PCI && ISA_DMA_API diff --git
> > a/drivers/tty/Makefile b/drivers/tty/Makefile index
> > 020b1cd9294f..6d07985d6962 100644
> > --- a/drivers/tty/Makefile
> > +++ b/drivers/tty/Makefile
> > @@ -24,6 +24,7 @@ obj-$(CONFIG_CYCLADES)+= cyclades.o
> > obj-$(CONFIG_ISI) += isicom.o
> > obj-$(CONFIG_MOXA_INTELLIO)+= moxa.o
> > obj-$(CONFIG_MOXA_SMARTIO) += mxser.o
> > +obj-$(CONFIG_MOXA_NPORT_REAL_TTY) += npreal2.o
>
> The same.
>
> > obj-$(CONFIG_NOZOMI) += nozomi.o
> > obj-$(CONFIG_NULL_TTY) += ttynull.o
> > obj-$(CONFIG_ROCKETPORT) += rocket.o
> > diff --git a/drivers/tty/npreal2.c b/drivers/tty/npreal2.c new file
> > mode 100644 index ..65c773420755
> > --- /dev/null
> > +++ b/drivers/tty/npreal2.c
> > @@ -0,0 +1,3042 @@
> ...
> > +#include
> > +#include
> > +#include
> > +#include
>
> What do you need version.h for? Are you sure, you need all these headers?
>
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include "npreal2.h"
> > +
> > +static int ttymajor = NPREALMAJOR;
>
> You want dynamic major anyway. So kill this all.
>
> > +static int verbose = 1;
> > +
> > +MODULE_AUTHOR("");
> > +MODULE_DESCRIPTION("MOXA Async/NPort Server Family Real TTY
> Driver");
> > +module_param(ttymajor, int, 0); module_param(verbose, int, 0644);
> > +MODULE_VERSION(NPREAL_VERSION); MODULE_LICENSE("GPL");
> > +
> > +struct server_setting_struct {
> > + int32_t server_type;
> > + int32_t disable_fifo;
> > +};
> > +
> > +struct npreal_struct {
> > + struct tty_port ttyPort;
> > + struct work_struct tqueue;
> > + struct work_struct process_flip_tqueue;
> > + struct ktermios normal_termios;
> > + struct ktermios callout_termios;
>
> callout in 2020?
>
> > + /* kernel counters for the 4 input interrupts */
> > + struct async_icount icount;
> > + struct semaphore rx_semaphore;
>
> semaphores in new code? You have to explain why are you not using simpler
> and faset mutexes.
>
> > + struct nd_struct *net_node;
> > + struct tty_struct *tty;
> > + struct pid *session;
> > + struct pid *pgrp;
>
> Why does a driver need to care about pgrp? And session? You should kill all
> these set, but unused fields. Note that you should also use fields from struct
> tty_port instead of the duplicates here.
>
> > + wait_queue_head_t open_wait;
> > +
[PATCH] ARM: dts: am335x: add common dtsi for MOXA UC-8100 series
Add am335x-moxa-uc-8100-common.dtsi for many products of MOXA UC-8100
series, and remove common nodes from am335x-moxa-uc-8100-me-t.dts.
Signed-off-by: Johnson Chen
---
.../boot/dts/am335x-moxa-uc-8100-common.dtsi | 427 ++
.../arm/boot/dts/am335x-moxa-uc-8100-me-t.dts | 404 +
2 files changed, 428 insertions(+), 403 deletions(-)
create mode 100644 arch/arm/boot/dts/am335x-moxa-uc-8100-common.dtsi
diff --git a/arch/arm/boot/dts/am335x-moxa-uc-8100-common.dtsi
b/arch/arm/boot/dts/am335x-moxa-uc-8100-common.dtsi
new file mode 100644
index ..98d8ed4ad967
--- /dev/null
+++ b/arch/arm/boot/dts/am335x-moxa-uc-8100-common.dtsi
@@ -0,0 +1,427 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2020 MOXA Inc. - https://www.moxa.com/
+ *
+ * Author: Johnson Chen
+ */
+
+#include "am33xx.dtsi"
+
+/ {
+
+ cpus {
+ cpu@0 {
+ cpu0-supply = <_reg>;
+ };
+ };
+
+ vbat: vbat-regulator {
+ compatible = "regulator-fixed";
+ };
+
+ /* Power supply provides a fixed 3.3V @3A */
+ vmmcsd_fixed: vmmcsd-regulator {
+ compatible = "regulator-fixed";
+ regulator-name = "vmmcsd_fixed";
+ regulator-min-microvolt = <330>;
+ regulator-max-microvolt = <330>;
+ regulator-boot-on;
+ };
+
+ buttons: push_button {
+ compatible = "gpio-keys";
+ };
+
+};
+
+_pinmux {
+ pinctrl-names = "default";
+ pinctrl-0 = <_pins>;
+
+ minipcie_pins: pinmux_minipcie {
+ pinctrl-single,pins = <
+ AM33XX_PADCONF(AM335X_PIN_LCD_PCLK, PIN_INPUT_PULLDOWN,
MUX_MODE7) /* lcd_pclk.gpio2_24 */
+ AM33XX_PADCONF(AM335X_PIN_LCD_AC_BIAS_EN,
PIN_INPUT_PULLDOWN, MUX_MODE7)/* lcd_ac_bias_en.gpio2_25 */
+ AM33XX_PADCONF(AM335X_PIN_LCD_VSYNC,
PIN_INPUT_PULLDOWN, MUX_MODE7) /* lcd_vsync.gpio2_22 Power off PIN*/
+ >;
+ };
+
+ push_button_pins: pinmux_push_button {
+ pinctrl-single,pins = <
+ AM33XX_PADCONF(AM335X_PIN_MCASP0_AHCLKX,
PIN_INPUT_PULLDOWN, MUX_MODE7) /* mcasp0_ahcklx.gpio3_21 */
+ >;
+ };
+
+ i2c0_pins: pinmux_i2c0_pins {
+ pinctrl-single,pins = <
+ AM33XX_PADCONF(AM335X_PIN_I2C0_SDA, PIN_INPUT_PULLUP,
MUX_MODE0)
+ AM33XX_PADCONF(AM335X_PIN_I2C0_SCL, PIN_INPUT_PULLUP,
MUX_MODE0)
+ >;
+ };
+
+
+ i2c1_pins: pinmux_i2c1_pins {
+ pinctrl-single,pins = <
+ AM33XX_PADCONF(AM335X_PIN_UART0_CTSN, PIN_INPUT_PULLUP,
MUX_MODE3) /* uart0_ctsn.i2c1_sda */
+ AM33XX_PADCONF(AM335X_PIN_UART0_RTSN, PIN_INPUT_PULLUP,
MUX_MODE3) /* uart0_rtsn.i2c1_scl */
+ >;
+ };
+
+ uart0_pins: pinmux_uart0_pins {
+ pinctrl-single,pins = <
+ AM33XX_PADCONF(AM335X_PIN_UART0_RXD, PIN_INPUT_PULLUP,
MUX_MODE0)
+ AM33XX_PADCONF(AM335X_PIN_UART0_TXD,
PIN_OUTPUT_PULLDOWN, MUX_MODE0)
+ >;
+ };
+
+ uart1_pins: pinmux_uart1_pins {
+ pinctrl-single,pins = <
+ AM33XX_PADCONF(AM335X_PIN_UART1_CTSN, PIN_INPUT,
MUX_MODE0)
+ AM33XX_PADCONF(AM335X_PIN_UART1_RTSN,
PIN_OUTPUT_PULLDOWN, MUX_MODE0)
+ AM33XX_PADCONF(AM335X_PIN_UART1_RXD, PIN_INPUT_PULLUP,
MUX_MODE0)
+ AM33XX_PADCONF(AM335X_PIN_UART1_TXD, PIN_OUTPUT,
MUX_MODE0)
+ >;
+ };
+
+ uart2_pins: pinmux_uart2_pins {
+ pinctrl-single,pins = <
+ AM33XX_PADCONF(AM335X_PIN_LCD_DATA14, PIN_INPUT,
MUX_MODE6) /* lcd_data14.uart5_ctsn */
+ AM33XX_PADCONF(AM335X_PIN_LCD_DATA15,
PIN_OUTPUT_PULLDOWN, MUX_MODE6) /* lcd_data15.uart5_rtsn */
+ AM33XX_PADCONF(AM335X_PIN_LCD_DATA9, PIN_INPUT_PULLUP,
MUX_MODE4) /* lcd_data9.uart5_rxd */
+ AM33XX_PADCONF(AM335X_PIN_LCD_DATA8, PIN_OUTPUT,
MUX_MODE4) /* lcd_data8.uart5_txd */
+ >;
+ };
+
+ cpsw_default: cpsw_default {
+ pinctrl-single,pins = <
+ /* Slave 1 */
+ AM33XX_PADCONF(AM335X_PIN_MII1_CRS, PIN_INPUT_PULLDOWN,
MUX_MODE1)
+ AM33XX_PADCONF(AM335X_PIN_MII1_RX_ER, PIN_INPUT_PULLUP,
MUX_MODE1)
+ AM33XX_PADCONF(AM335X_PIN_MII1_TX_EN,
PIN_OUTPUT_PULLDOWN, MUX_MODE1)
+ AM33XX_PADCONF(AM335X_PIN_MII1_TXD1,
PIN_OUTPUT_PULLDOWN, MUX_MODE1)
+ AM33XX_PADCONF(AM335X_PIN_MII1_TXD0,
PIN_OUTPUT_PULLDOWN, MUX_MODE1)
+ AM33XX_PADCONF(AM335X_PIN_MII1_RXD1,
RE: [PATCH] tty: Add MOXA NPort Real TTY Driver
Hi Greg, Thanks for your detailed good review! > From: Greg Kroah-Hartman > Sent: Tuesday, July 14, 2020 3:12 PM > To: Johnson CH Chen (陳昭勳) > Cc: Jiri Slaby ; linux-kernel@vger.kernel.org; > linux-ser...@vger.kernel.org > Subject: Re: [PATCH] tty: Add MOXA NPort Real TTY Driver > > On Tue, Jul 14, 2020 at 06:24:42AM +0000, Johnson CH Chen (陳昭勳) wrote: > > This driver supports tty functions for all of MOXA's NPort series with > > v5.0. Using this driver, host part can use tty to connect NPort device > > server by ethernet. > > A new serial driver, nice! > > > > > The following Moxa products are supported: > > * CN2600 Series > > * CN2500 Series > > * NPort DE Series > > * NPort 5000A-M12 Series > > * NPort 5100 Series > > * NPort 5200 Series > > * NPort 5400 Series > > * NPort 5600 Desktop Series > > * NPort 5600 Rackmount Series > > * NPort Wireless Series > > * NPort IA5000 Series > > * NPort 6000 Series > > * NPort S8000 Series > > * NPort S8455I Series > > * NPort S9000 Series > > * NE-4100 Series > > * MiiNePort Series > > > > Signed-off-by: Johnson Chen > > Signed-off-by: Jason Chen > > Signed-off-by: Danny Lin > > Signed-off-by: Victor Yu > > --- > > drivers/tty/Kconfig | 11 + > > drivers/tty/Makefile |1 + > > drivers/tty/npreal2.c | 3042 > > + > > drivers/tty/npreal2.h | 140 ++ > > 4 files changed, 3194 insertions(+) > > create mode 100644 drivers/tty/npreal2.c create mode 100644 > > drivers/tty/npreal2.h > > > > diff --git a/drivers/tty/Kconfig b/drivers/tty/Kconfig index > > 93fd984eb2f5..79b545269b71 100644 > > --- a/drivers/tty/Kconfig > > +++ b/drivers/tty/Kconfig > > @@ -259,6 +259,17 @@ config MOXA_SMARTIO > > This driver can also be built as a module. The module will be called > > mxser. If you want to do that, say M here. > > > > +config MOXA_NPORT_REAL_TTY > > + tristate "Moxa NPort Real TTY support v5.0" > > + help > > + Say Y here if you have a Moxa NPort serial device server. > > + > > + The purpose of this driver is to map NPort serial port to host tty > > + port. Using this driver, you can use NPort serial port as local tty > > port. > > + > > + This driver can also be built as a module. The module will be called > > + npreal2 by setting M. > > + > > config SYNCLINK > > tristate "Microgate SyncLink card support" > > depends on SERIAL_NONSTANDARD && PCI && ISA_DMA_API diff --git > > a/drivers/tty/Makefile b/drivers/tty/Makefile index > > 020b1cd9294f..6d07985d6962 100644 > > --- a/drivers/tty/Makefile > > +++ b/drivers/tty/Makefile > > @@ -24,6 +24,7 @@ obj-$(CONFIG_CYCLADES)+= cyclades.o > > obj-$(CONFIG_ISI) += isicom.o > > obj-$(CONFIG_MOXA_INTELLIO)+= moxa.o > > obj-$(CONFIG_MOXA_SMARTIO) += mxser.o > > +obj-$(CONFIG_MOXA_NPORT_REAL_TTY) += npreal2.o > > obj-$(CONFIG_NOZOMI) += nozomi.o > > obj-$(CONFIG_NULL_TTY) += ttynull.o > > obj-$(CONFIG_ROCKETPORT) += rocket.o > > diff --git a/drivers/tty/npreal2.c b/drivers/tty/npreal2.c new file > > mode 100644 index ..65c773420755 > > --- /dev/null > > +++ b/drivers/tty/npreal2.c > > @@ -0,0 +1,3042 @@ > > +// SPDX-License-Identifier: GPL-2.0+ > > +/* > > + * npreal2.c -- MOXA NPort Server family Real TTY driver. > > + * > > + * Copyright (c) 1999-2020 Moxa Technologies (supp...@moxa.com) > > + * > > + * Supports the following Moxa Product: > > + * CN2600 Series > > + * CN2500 Series > > + * NPort DE Series > > + * NPort 5000A-M12 Series > > + * NPort 5100 Series > > + * NPort 5200 Series > > + * NPort 5400 Series > > + * NPort 5600 Desktop Series > > + * NPort 5600 Rackmount Series > > + * NPort Wireless Series > > + * NPort IA5000 Series > > + * NPort 6000 Series > > + * NPort S8000 Series > > + * NPort S8455I Series > > + * NPort S9000 Series > > + * NE-4100 Series > > + * MiiNePort Series > > + */ > > + > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > &g
RE: [PATCH] tty: Add MOXA NPort Real TTY Driver
Hi Greg, Thanks for your response! > > > > + unsigned long flag; > > > > + unsigned char cmd_buffer[84]; > > > > + unsigned char rsp_buffer[84]; > > > > > > You seem to have two "static" buffers here, for your device, that > > > you semi-randomly write to all over the place, but I can't find > > > any locking or coordination between things that prevents multiple > > > commands from not just overwritting each other. > > > > > For cmd_buffer[], we use npreal_wait_and_set_command() to make sure > > cmd_buffer[] is safe to be written by checking "cmd_buffer[0] == 0". > > And what locks are protecting you there? > > > For rsp_buffer[], we use npreal_wait_command_completed() to make > > sure rsp_buffer[] is desired by checking rsp_buffer[0] and rsp_buffer[1]. > > Command_set and command should be checked. Besides, rsp_buffer[] is > > got from user space by "NPREAL_NET_CMD_RESPONSE" in > > npreal_net_ioctl(). > > Again, what locking is really handling this? > It's better to protect cmd_buffer[84] and rsp_buffer[84] by locking completely. They are safe because NPort driver should be worked with NPort daemon before, and NPort daemon is designed to be simple. > > > Also, how does the data get sent to the hardware at all? I see > > > cmd_buffer[] being written to, but what reads from it and how does > > > the hardware get the data? > > > > Actually we need to both NPort driver (this driver) and Npreal > > daemon > > (userspace) to let HW work. Npreal daemon can communicate with HW by > > socket, and Npreal deamon communicates with Nport driver by > > "npreal_net_fops". When commands are ready for driver part, it will > > wake up poll event to let Nport daemon know. > > That is not obvious at all, and needs to be really really really documented > here. > Why not put the userspace chunk in the tree too? At the least, you > need to point at it. > > And why is a userspace part needed? We have tty-over-ethernet drivers > that do not require such a thing in the tree somewhere... > Because we need hardware serial to Ethernet converter (NPort device server) to manage some serial devices, so we still need to use MOXA Nport's commands and responses between host computer and converter. We will have an internal discussion about release of Nport daemon and related document, or using free tty to Ethernet daemon such as (ser2net/ socat/ remtty) and improved nport driver later. Thanks a lot! Best regards, Johnson
RE: [PATCH v5] rtc: rtc-ds1374: wdt: Use watchdog core for watchdog part
Hi Alexandre, > > v4->v5: > > - Fix reported build error by replacing RTC_DRV_DS1374_WDT with > > WATCHDOG_CORE > > > > This is not the correct solution as this will remove the alarm functionality > for > anyone enabling WATCHDOG. I already submitted a proper fix. > It's an appropriate and better solution. Thanks! RTC_DRV_DS1374_WDT still should select WATCHDOG_CORE to avoid build error if WATCHDOG_CORE is set to "m", and it should be depended on RTC_DRV_DS1374 and WATCHDOG. > > v3->v4: > > - Fix coding styles > > - Remove dev_info() in ds1374_wdt_settimeout() > > - Fix missing error check > > > > -- Best regards, Johnson
[PATCH v5] rtc: rtc-ds1374: wdt: Use watchdog core for watchdog part
Let ds1374 watchdog use watchdog core functions. It also includes
improving watchdog timer setting and nowayout, and just uses ioctl()
of watchdog core.
Signed-off-by: Johnson Chen
Reported-by: Randy Dunlap
---
v4->v5:
- Fix reported build error by replacing RTC_DRV_DS1374_WDT with WATCHDOG_CORE
v3->v4:
- Fix coding styles
- Remove dev_info() in ds1374_wdt_settimeout()
- Fix missing error check
v2->v3:
- Fix a problem reported by WATCHDOG_CORE if WATCHDOG
- Remove save_client
- Let wdt_margin be 0 for watchdog_init_timeout()
- Use dev_info() rather than pr_info()
- Avoid more strings in this driver
v1->v2:
- Use ds1374_wdt_settimeout() before registering the watchdog
- Remove watchdog_unregister_device() because devm_watchdog_register_device()
is used
- Remove ds1374_wdt_ping()
- TIMER_MARGIN_MAX to 4095 for 24-bit value
- Keep wdt_margin
- Fix coding styles
drivers/rtc/Kconfig | 13 +-
drivers/rtc/rtc-ds1374.c | 272 ++-
2 files changed, 72 insertions(+), 213 deletions(-)
diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
index b54d87d45c89..fcc47a613635 100644
--- a/drivers/rtc/Kconfig
+++ b/drivers/rtc/Kconfig
@@ -271,22 +271,17 @@ config RTC_DRV_DS1307_CENTURY
config RTC_DRV_DS1374
tristate "Dallas/Maxim DS1374"
+ select WATCHDOG_CORE if WATCHDOG
help
If you say yes here you get support for Dallas Semiconductor
DS1374 real-time clock chips. If an interrupt is associated
- with the device, the alarm functionality is supported.
+ with the device, the alarm functionality is supported. If
+ WATCHDOG_CORE is enabled, Dallas/Maxim DS1374 watchdog timer
+ is supported rather than alarm timer.
This driver can also be built as a module. If so, the module
will be called rtc-ds1374.
-config RTC_DRV_DS1374_WDT
- bool "Dallas/Maxim DS1374 watchdog timer"
- depends on RTC_DRV_DS1374
- help
- If you say Y here you will get support for the
- watchdog timer in the Dallas Semiconductor DS1374
- real-time clock chips.
-
config RTC_DRV_DS1672
tristate "Dallas/Maxim DS1672"
help
diff --git a/drivers/rtc/rtc-ds1374.c b/drivers/rtc/rtc-ds1374.c
index 9c51a12cf70f..fc6470f5c847 100644
--- a/drivers/rtc/rtc-ds1374.c
+++ b/drivers/rtc/rtc-ds1374.c
@@ -29,7 +29,7 @@
#include
#include
#include
-#ifdef CONFIG_RTC_DRV_DS1374_WDT
+#ifdef CONFIG_WATCHDOG_CORE
#include
#include
#include
@@ -46,6 +46,7 @@
#define DS1374_REG_WDALM2 0x06
#define DS1374_REG_CR 0x07 /* Control */
#define DS1374_REG_CR_AIE 0x01 /* Alarm Int. Enable */
+#define DS1374_REG_CR_WDSTR0x08 /* 1=INT, 0=RST */
#define DS1374_REG_CR_WDALM0x20 /* 1=Watchdog, 0=Alarm */
#define DS1374_REG_CR_WACE 0x40 /* WD/Alarm counter enable */
#define DS1374_REG_SR 0x08 /* Status */
@@ -71,7 +72,9 @@ struct ds1374 {
struct i2c_client *client;
struct rtc_device *rtc;
struct work_struct work;
-
+#ifdef CONFIG_WATCHDOG_CORE
+ struct watchdog_device wdt;
+#endif
/* The mutex protects alarm operations, and prevents a race
* between the enable_irq() in the workqueue and the free_irq()
* in the remove function.
@@ -177,7 +180,7 @@ static int ds1374_set_time(struct device *dev, struct
rtc_time *time)
return ds1374_write_rtc(client, itime, DS1374_REG_TOD0, 4);
}
-#ifndef CONFIG_RTC_DRV_DS1374_WDT
+#ifndef CONFIG_WATCHDOG_CORE
/* The ds1374 has a decrementer for an alarm, rather than a comparator.
* If the time of day is changed, then the alarm will need to be
* reset.
@@ -324,7 +327,7 @@ static void ds1374_work(struct work_struct *work)
mutex_unlock(>mutex);
}
-#ifndef CONFIG_RTC_DRV_DS1374_WDT
+#ifndef CONFIG_WATCHDOG_CORE
static int ds1374_alarm_irq_enable(struct device *dev, unsigned int enabled)
{
struct i2c_client *client = to_i2c_client(dev);
@@ -354,14 +357,14 @@ static int ds1374_alarm_irq_enable(struct device *dev,
unsigned int enabled)
static const struct rtc_class_ops ds1374_rtc_ops = {
.read_time = ds1374_read_time,
.set_time = ds1374_set_time,
-#ifndef CONFIG_RTC_DRV_DS1374_WDT
+#ifndef CONFIG_WATCHDOG_CORE
.read_alarm = ds1374_read_alarm,
.set_alarm = ds1374_set_alarm,
.alarm_irq_enable = ds1374_alarm_irq_enable,
#endif
};
-#ifdef CONFIG_RTC_DRV_DS1374_WDT
+#ifdef CONFIG_WATCHDOG_CORE
/*
*
*
@@ -369,239 +372,99 @@ static const struct rtc_class_ops ds1374_rtc_ops = {
*
*
*/
-static struct i2c_client *save_client;
/* Default margin */
-#define WD_TIMO 131762
+#define TIMER_MARGIN_DEFAULT 32
+#define TIMER_MARGIN_MIN 1
+#define TIMER_MARGIN_MAX 4095 /* 24-bit value */
#define DRV_NAME "DS1374
RE: linux-next: Tree for Jul 17 (drivers/rtc/rtc-ds1374.o)
Hi Stephen,
> From: Stephen Rothwell
> Sent: Saturday, July 18, 2020 9:39 AM
> To: Randy Dunlap
> Cc: Linux Next Mailing List ; Linux Kernel Mailing
> List ; linux-...@vger.kernel.org; Alessandro
> Zummo ; Alexandre Belloni
> ; Scott Wood ;
> Johnson CH Chen (陳昭勳)
> Subject: Re: linux-next: Tree for Jul 17 (drivers/rtc/rtc-ds1374.o)
>
> Hi Randy,
>
> [Please trim your emails a bit more, thanks]
>
> On Fri, 17 Jul 2020 09:49:05 -0700 Randy Dunlap
> wrote:
> > on x86_64:
> > # CONFIG_WATCHDOG is not set
> >
Thanks for your information.
It seems RTC_DRV_DS1374_WDT should be depended on RTC_DRV_DS1374 &&
WATCHDOG_CORE. Otherwise error will happen if RTC_DRV_DS1374_WDT is enabled but
WATCHDOG_CORE not. I'll put this into v5 if there are no other suggestions.
Should I add " Reported-by: Stephen Rothwell " into v5?
So many thanks!
> > ld: drivers/rtc/rtc-ds1374.o: in function `ds1374_probe':
> > rtc-ds1374.c:(.text+0x736): undefined reference to
> `watchdog_init_timeout'
> > ld: rtc-ds1374.c:(.text+0x77e): undefined reference to
> `devm_watchdog_register_device'
>
> Caused by commit
>
> d3de4beb14a8 ("rtc: ds1374: wdt: Use watchdog core for watchdog
> part")
>
> from the rtc tree.
> --
> Cheers,
> Stephen Rothwell
Best regards,
Johnson
RE: [PATCH] tty: Add MOXA NPort Real TTY Driver
Hi Greg, Thanks for detailed and good suggestions! > From: linux-serial-ow...@vger.kernel.org > On Behalf Of Greg Kroah-Hartman > Sent: Tuesday, July 14, 2020 3:36 PM > To: Johnson CH Chen (陳昭勳) > Cc: Jiri Slaby ; linux-kernel@vger.kernel.org; > linux-ser...@vger.kernel.org > Subject: Re: [PATCH] tty: Add MOXA NPort Real TTY Driver > > On Tue, Jul 14, 2020 at 06:24:42AM +, Johnson CH Chen (陳昭勳) wrote: > > This driver supports tty functions for all of MOXA's NPort series with > > v5.0. Using this driver, host part can use tty to connect NPort device > > server by ethernet. > > > > The following Moxa products are supported: > > * CN2600 Series > > * CN2500 Series > > * NPort DE Series > > * NPort 5000A-M12 Series > > * NPort 5100 Series > > * NPort 5200 Series > > * NPort 5400 Series > > * NPort 5600 Desktop Series > > * NPort 5600 Rackmount Series > > * NPort Wireless Series > > * NPort IA5000 Series > > * NPort 6000 Series > > * NPort S8000 Series > > * NPort S8455I Series > > * NPort S9000 Series > > * NE-4100 Series > > * MiiNePort Series > > > > Signed-off-by: Johnson Chen > > Signed-off-by: Jason Chen > > Signed-off-by: Danny Lin > > Signed-off-by: Victor Yu > > --- > > drivers/tty/Kconfig | 11 + > > drivers/tty/Makefile |1 + > > drivers/tty/npreal2.c | 3042 > > + > > drivers/tty/npreal2.h | 140 ++ > > 4 files changed, 3194 insertions(+) > > create mode 100644 drivers/tty/npreal2.c create mode 100644 > > drivers/tty/npreal2.h > > > > diff --git a/drivers/tty/Kconfig b/drivers/tty/Kconfig index > > 93fd984eb2f5..79b545269b71 100644 > > --- a/drivers/tty/Kconfig > > +++ b/drivers/tty/Kconfig > > @@ -259,6 +259,17 @@ config MOXA_SMARTIO > > This driver can also be built as a module. The module will be called > > mxser. If you want to do that, say M here. > > > > +config MOXA_NPORT_REAL_TTY > > + tristate "Moxa NPort Real TTY support v5.0" > > + help > > + Say Y here if you have a Moxa NPort serial device server. > > + > > + The purpose of this driver is to map NPort serial port to host tty > > + port. Using this driver, you can use NPort serial port as local tty > > port. > > + > > + This driver can also be built as a module. The module will be called > > + npreal2 by setting M. > > + > > config SYNCLINK > > tristate "Microgate SyncLink card support" > > depends on SERIAL_NONSTANDARD && PCI && ISA_DMA_API diff --git > > a/drivers/tty/Makefile b/drivers/tty/Makefile index > > 020b1cd9294f..6d07985d6962 100644 > > --- a/drivers/tty/Makefile > > +++ b/drivers/tty/Makefile > > @@ -24,6 +24,7 @@ obj-$(CONFIG_CYCLADES)+= cyclades.o > > obj-$(CONFIG_ISI) += isicom.o > > obj-$(CONFIG_MOXA_INTELLIO)+= moxa.o > > obj-$(CONFIG_MOXA_SMARTIO) += mxser.o > > +obj-$(CONFIG_MOXA_NPORT_REAL_TTY) += npreal2.o > > obj-$(CONFIG_NOZOMI) += nozomi.o > > obj-$(CONFIG_NULL_TTY) += ttynull.o > > obj-$(CONFIG_ROCKETPORT) += rocket.o > > diff --git a/drivers/tty/npreal2.c b/drivers/tty/npreal2.c new file > > mode 100644 index ..65c773420755 > > --- /dev/null > > +++ b/drivers/tty/npreal2.c > > @@ -0,0 +1,3042 @@ > > +// SPDX-License-Identifier: GPL-2.0+ > > +/* > > + * npreal2.c -- MOXA NPort Server family Real TTY driver. > > + * > > + * Copyright (c) 1999-2020 Moxa Technologies (supp...@moxa.com) > > + * > > + * Supports the following Moxa Product: > > + * CN2600 Series > > + * CN2500 Series > > + * NPort DE Series > > + * NPort 5000A-M12 Series > > + * NPort 5100 Series > > + * NPort 5200 Series > > + * NPort 5400 Series > > + * NPort 5600 Desktop Series > > + * NPort 5600 Rackmount Series > > + * NPort Wireless Series > > + * NPort IA5000 Series > > + * NPort 6000 Series > > + * NPort S8000 Series > > + * NPort S8455I Series > > + * NPort S9000 Series > > + * NE-4100 Series > > + * MiiNePort Series > > + */ > > + > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include >
[PATCH] tty: Add MOXA NPort Real TTY Driver
This driver supports tty functions for all of MOXA's NPort series
with v5.0. Using this driver, host part can use tty to connect NPort
device server by ethernet.
The following Moxa products are supported:
* CN2600 Series
* CN2500 Series
* NPort DE Series
* NPort 5000A-M12 Series
* NPort 5100 Series
* NPort 5200 Series
* NPort 5400 Series
* NPort 5600 Desktop Series
* NPort 5600 Rackmount Series
* NPort Wireless Series
* NPort IA5000 Series
* NPort 6000 Series
* NPort S8000 Series
* NPort S8455I Series
* NPort S9000 Series
* NE-4100 Series
* MiiNePort Series
Signed-off-by: Johnson Chen
Signed-off-by: Jason Chen
Signed-off-by: Danny Lin
Signed-off-by: Victor Yu
---
drivers/tty/Kconfig | 11 +
drivers/tty/Makefile |1 +
drivers/tty/npreal2.c | 3042 +
drivers/tty/npreal2.h | 140 ++
4 files changed, 3194 insertions(+)
create mode 100644 drivers/tty/npreal2.c
create mode 100644 drivers/tty/npreal2.h
diff --git a/drivers/tty/Kconfig b/drivers/tty/Kconfig
index 93fd984eb2f5..79b545269b71 100644
--- a/drivers/tty/Kconfig
+++ b/drivers/tty/Kconfig
@@ -259,6 +259,17 @@ config MOXA_SMARTIO
This driver can also be built as a module. The module will be called
mxser. If you want to do that, say M here.
+config MOXA_NPORT_REAL_TTY
+ tristate "Moxa NPort Real TTY support v5.0"
+ help
+ Say Y here if you have a Moxa NPort serial device server.
+
+ The purpose of this driver is to map NPort serial port to host tty
+ port. Using this driver, you can use NPort serial port as local tty
port.
+
+ This driver can also be built as a module. The module will be called
+ npreal2 by setting M.
+
config SYNCLINK
tristate "Microgate SyncLink card support"
depends on SERIAL_NONSTANDARD && PCI && ISA_DMA_API
diff --git a/drivers/tty/Makefile b/drivers/tty/Makefile
index 020b1cd9294f..6d07985d6962 100644
--- a/drivers/tty/Makefile
+++ b/drivers/tty/Makefile
@@ -24,6 +24,7 @@ obj-$(CONFIG_CYCLADES)+= cyclades.o
obj-$(CONFIG_ISI) += isicom.o
obj-$(CONFIG_MOXA_INTELLIO)+= moxa.o
obj-$(CONFIG_MOXA_SMARTIO) += mxser.o
+obj-$(CONFIG_MOXA_NPORT_REAL_TTY) += npreal2.o
obj-$(CONFIG_NOZOMI) += nozomi.o
obj-$(CONFIG_NULL_TTY) += ttynull.o
obj-$(CONFIG_ROCKETPORT) += rocket.o
diff --git a/drivers/tty/npreal2.c b/drivers/tty/npreal2.c
new file mode 100644
index ..65c773420755
--- /dev/null
+++ b/drivers/tty/npreal2.c
@@ -0,0 +1,3042 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * npreal2.c -- MOXA NPort Server family Real TTY driver.
+ *
+ * Copyright (c) 1999-2020 Moxa Technologies (supp...@moxa.com)
+ *
+ * Supports the following Moxa Product:
+ * CN2600 Series
+ * CN2500 Series
+ * NPort DE Series
+ * NPort 5000A-M12 Series
+ * NPort 5100 Series
+ * NPort 5200 Series
+ * NPort 5400 Series
+ * NPort 5600 Desktop Series
+ * NPort 5600 Rackmount Series
+ * NPort Wireless Series
+ * NPort IA5000 Series
+ * NPort 6000 Series
+ * NPort S8000 Series
+ * NPort S8455I Series
+ * NPort S9000 Series
+ * NE-4100 Series
+ * MiiNePort Series
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include "npreal2.h"
+
+static int ttymajor = NPREALMAJOR;
+static int verbose = 1;
+
+MODULE_AUTHOR("");
+MODULE_DESCRIPTION("MOXA Async/NPort Server Family Real TTY Driver");
+module_param(ttymajor, int, 0);
+module_param(verbose, int, 0644);
+MODULE_VERSION(NPREAL_VERSION);
+MODULE_LICENSE("GPL");
+
+struct server_setting_struct {
+ int32_t server_type;
+ int32_t disable_fifo;
+};
+
+struct npreal_struct {
+ struct tty_port ttyPort;
+ struct work_struct tqueue;
+ struct work_struct process_flip_tqueue;
+ struct ktermios normal_termios;
+ struct ktermios callout_termios;
+ /* kernel counters for the 4 input interrupts */
+ struct async_icount icount;
+ struct semaphore rx_semaphore;
+ struct nd_struct *net_node;
+ struct tty_struct *tty;
+ struct pid *session;
+ struct pid *pgrp;
+ wait_queue_head_t open_wait;
+ wait_queue_head_t close_wait;
+ wait_queue_head_t delta_msr_wait;
+ unsigned long baud_base;
+ unsigned long event;
+ unsigned short closing_wait;
+ int port;
+ int flags;
+ int type; /* UART type */
+ int xmit_fifo_size;
+ int custom_divisor;
+ int x_char; /* xon/xoff character */
+ int close_delay;
+ int modem_control; /* Modem control register */
+ int modem_status; /* Line status */
+ int count; /* # of fd on device */
+ int xmit_head;
+ int xmit_tail;
+ int xmit_cnt;
+ unsigned char *xmit_buf;
+
+ /*
+* We
RE: [PATCH v4] rtc: rtc-ds1374: wdt: Use watchdog core for watchdog part
Dear Alexandre Belloni,
> From: Guenter Roeck On Behalf Of Guenter Roeck
> Sent: Thursday, July 9, 2020 11:28 PM
> To: Johnson CH Chen (陳昭勳) ;
> linux-kernel@vger.kernel.org; linux-...@vger.kernel.org;
> linux-watch...@vger.kernel.org
> Cc: Wim Van Sebroeck ; a.zu...@towertech.it;
> alexandre.bell...@bootlin.com
> Subject: Re: [PATCH v4] rtc: rtc-ds1374: wdt: Use watchdog core for watchdog
> part
>
> On 7/8/20 11:34 PM, Johnson CH Chen (陳昭勳) wrote:
> > Let ds1374 watchdog use watchdog core functions. It also includes
> > improving watchdog timer setting and nowayout, and just uses ioctl()
> > of watchdog core.
> >
> > Signed-off-by: Johnson Chen
>
> Reviewed-by: Guenter Roeck
>
If there is no other big issue, could you please apply this patch? So many
thanks!
Best regards,
Johnson
> > ---
> > v3->v4:
> > - Fix coding styles
> > - Remove dev_info() in ds1374_wdt_settimeout()
> > - Fix missing error check
> >
> > v2->v3:
> > - Fix a problem reported by WATCHDOG_CORE if WATCHDOG
> > - Remove save_client
> > - Let wdt_margin be 0 for watchdog_init_timeout()
> > - Use dev_info() rather than pr_info()
> > - Avoid more strings in this driver
> >
> > v1->v2:
> > - Use ds1374_wdt_settimeout() before registering the watchdog
> > - Remove watchdog_unregister_device() because
> > devm_watchdog_register_device() is used
> > - Remove ds1374_wdt_ping()
> > - TIMER_MARGIN_MAX to 4095 for 24-bit value
> > - Keep wdt_margin
> > - Fix coding styles
> >
> > drivers/rtc/Kconfig | 1 +
> > drivers/rtc/rtc-ds1374.c | 258
> > +--
> > 2 files changed, 62 insertions(+), 197 deletions(-)
> >
> > diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig index
> > b54d87d45c89..c25d51f35f0c 100644
> > --- a/drivers/rtc/Kconfig
> > +++ b/drivers/rtc/Kconfig
> > @@ -282,6 +282,7 @@ config RTC_DRV_DS1374 config
> RTC_DRV_DS1374_WDT
> > bool "Dallas/Maxim DS1374 watchdog timer"
> > depends on RTC_DRV_DS1374
> > + select WATCHDOG_CORE if WATCHDOG
> > help
> > If you say Y here you will get support for the
> > watchdog timer in the Dallas Semiconductor DS1374 diff --git
> > a/drivers/rtc/rtc-ds1374.c b/drivers/rtc/rtc-ds1374.c index
> > 9c51a12cf70f..c71065d26cd2 100644
> > --- a/drivers/rtc/rtc-ds1374.c
> > +++ b/drivers/rtc/rtc-ds1374.c
> > @@ -46,6 +46,7 @@
> > #define DS1374_REG_WDALM2 0x06
> > #define DS1374_REG_CR 0x07 /* Control */
> > #define DS1374_REG_CR_AIE 0x01 /* Alarm Int. Enable */
> > +#define DS1374_REG_CR_WDSTR0x08 /* 1=INT, 0=RST */
> > #define DS1374_REG_CR_WDALM0x20 /* 1=Watchdog, 0=Alarm */
> > #define DS1374_REG_CR_WACE 0x40 /* WD/Alarm counter enable */
> > #define DS1374_REG_SR 0x08 /* Status */
> > @@ -71,7 +72,9 @@ struct ds1374 {
> > struct i2c_client *client;
> > struct rtc_device *rtc;
> > struct work_struct work;
> > -
> > +#ifdef CONFIG_RTC_DRV_DS1374_WDT
> > + struct watchdog_device wdt;
> > +#endif
> > /* The mutex protects alarm operations, and prevents a race
> > * between the enable_irq() in the workqueue and the free_irq()
> > * in the remove function.
> > @@ -369,238 +372,98 @@ static const struct rtc_class_ops ds1374_rtc_ops
> = {
> > *
> >
>
> *
> > */
> > -static struct i2c_client *save_client;
> > /* Default margin */
> > -#define WD_TIMO 131762
> > +#define TIMER_MARGIN_DEFAULT 32
> > +#define TIMER_MARGIN_MIN 1
> > +#define TIMER_MARGIN_MAX 4095 /* 24-bit value */
> >
> > #define DRV_NAME "DS1374 Watchdog"
> >
> > -static int wdt_margin = WD_TIMO;
> > -static unsigned long wdt_is_open;
> > +static int wdt_margin;
> > module_param(wdt_margin, int, 0);
> > MODULE_PARM_DESC(wdt_margin, "Watchdog timeout in seconds
> (default
> > 32s)");
> >
> > +static bool nowayout = WATCHDOG_NOWAYOUT; module_param(nowayout,
> > +bool, 0); MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped
> once
> > +started (default ="
> > + __MODULE_STRING(WATCHDOG_NOWAYOUT)")");
> > +
> > static const struct watchdog_info ds1374_wdt_info = {
> > .identity = "DS1374 WTD",
> > .options= WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING |
> >
[PATCH v4] rtc: rtc-ds1374: wdt: Use watchdog core for watchdog part
Let ds1374 watchdog use watchdog core functions. It also includes
improving watchdog timer setting and nowayout, and just uses ioctl()
of watchdog core.
Signed-off-by: Johnson Chen
---
v3->v4:
- Fix coding styles
- Remove dev_info() in ds1374_wdt_settimeout()
- Fix missing error check
v2->v3:
- Fix a problem reported by WATCHDOG_CORE if WATCHDOG
- Remove save_client
- Let wdt_margin be 0 for watchdog_init_timeout()
- Use dev_info() rather than pr_info()
- Avoid more strings in this driver
v1->v2:
- Use ds1374_wdt_settimeout() before registering the watchdog
- Remove watchdog_unregister_device() because devm_watchdog_register_device()
is used
- Remove ds1374_wdt_ping()
- TIMER_MARGIN_MAX to 4095 for 24-bit value
- Keep wdt_margin
- Fix coding styles
drivers/rtc/Kconfig | 1 +
drivers/rtc/rtc-ds1374.c | 258 +--
2 files changed, 62 insertions(+), 197 deletions(-)
diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
index b54d87d45c89..c25d51f35f0c 100644
--- a/drivers/rtc/Kconfig
+++ b/drivers/rtc/Kconfig
@@ -282,6 +282,7 @@ config RTC_DRV_DS1374
config RTC_DRV_DS1374_WDT
bool "Dallas/Maxim DS1374 watchdog timer"
depends on RTC_DRV_DS1374
+ select WATCHDOG_CORE if WATCHDOG
help
If you say Y here you will get support for the
watchdog timer in the Dallas Semiconductor DS1374
diff --git a/drivers/rtc/rtc-ds1374.c b/drivers/rtc/rtc-ds1374.c
index 9c51a12cf70f..c71065d26cd2 100644
--- a/drivers/rtc/rtc-ds1374.c
+++ b/drivers/rtc/rtc-ds1374.c
@@ -46,6 +46,7 @@
#define DS1374_REG_WDALM2 0x06
#define DS1374_REG_CR 0x07 /* Control */
#define DS1374_REG_CR_AIE 0x01 /* Alarm Int. Enable */
+#define DS1374_REG_CR_WDSTR0x08 /* 1=INT, 0=RST */
#define DS1374_REG_CR_WDALM0x20 /* 1=Watchdog, 0=Alarm */
#define DS1374_REG_CR_WACE 0x40 /* WD/Alarm counter enable */
#define DS1374_REG_SR 0x08 /* Status */
@@ -71,7 +72,9 @@ struct ds1374 {
struct i2c_client *client;
struct rtc_device *rtc;
struct work_struct work;
-
+#ifdef CONFIG_RTC_DRV_DS1374_WDT
+ struct watchdog_device wdt;
+#endif
/* The mutex protects alarm operations, and prevents a race
* between the enable_irq() in the workqueue and the free_irq()
* in the remove function.
@@ -369,238 +372,98 @@ static const struct rtc_class_ops ds1374_rtc_ops = {
*
*
*/
-static struct i2c_client *save_client;
/* Default margin */
-#define WD_TIMO 131762
+#define TIMER_MARGIN_DEFAULT 32
+#define TIMER_MARGIN_MIN 1
+#define TIMER_MARGIN_MAX 4095 /* 24-bit value */
#define DRV_NAME "DS1374 Watchdog"
-static int wdt_margin = WD_TIMO;
-static unsigned long wdt_is_open;
+static int wdt_margin;
module_param(wdt_margin, int, 0);
MODULE_PARM_DESC(wdt_margin, "Watchdog timeout in seconds (default 32s)");
+static bool nowayout = WATCHDOG_NOWAYOUT;
+module_param(nowayout, bool, 0);
+MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default ="
+ __MODULE_STRING(WATCHDOG_NOWAYOUT)")");
+
static const struct watchdog_info ds1374_wdt_info = {
.identity = "DS1374 WTD",
.options= WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING |
WDIOF_MAGICCLOSE,
};
-static int ds1374_wdt_settimeout(unsigned int timeout)
+static int ds1374_wdt_settimeout(struct watchdog_device *wdt, unsigned int
timeout)
{
- int ret = -ENOIOCTLCMD;
- int cr;
+ struct ds1374 *ds1374 = watchdog_get_drvdata(wdt);
+ struct i2c_client *client = ds1374->client;
+ int ret, cr;
- ret = cr = i2c_smbus_read_byte_data(save_client, DS1374_REG_CR);
- if (ret < 0)
- goto out;
+ wdt->timeout = timeout;
+
+ cr = i2c_smbus_read_byte_data(client, DS1374_REG_CR);
+ if (cr < 0)
+ return cr;
/* Disable any existing watchdog/alarm before setting the new one */
cr &= ~DS1374_REG_CR_WACE;
- ret = i2c_smbus_write_byte_data(save_client, DS1374_REG_CR, cr);
+ ret = i2c_smbus_write_byte_data(client, DS1374_REG_CR, cr);
if (ret < 0)
- goto out;
+ return ret;
/* Set new watchdog time */
- ret = ds1374_write_rtc(save_client, timeout, DS1374_REG_WDALM0, 3);
- if (ret) {
- pr_info("couldn't set new watchdog time\n");
- goto out;
- }
+ timeout = timeout * 4096;
+ ret = ds1374_write_rtc(client, timeout, DS1374_REG_WDALM0, 3);
+ if (ret)
+ return ret;
/* Enable watchdog timer */
cr |= DS1374_REG_CR_WACE | DS1374_REG_CR_WDALM;
+ cr &= ~DS1374_REG_CR_WDSTR;/* for RST PIN */
cr &= ~DS1374_REG_CR_AIE;
- ret = i2c_smbus_write_byte_data(save_client, DS1374_REG_CR, cr);
+ ret
RE: [PATCH 2/2] ARM: dts: am335x: add support for Moxa UC-8100A-ME open platform
Hi, > Subject: Re: [PATCH 2/2] ARM: dts: am335x: add support for Moxa > UC-8100A-ME open platform > > * Johnson CH Chen (陳昭勳) [200707 03:24]: > > UC-8100A-ME is advanced of UC-8100-ME-T, and UC-8100-ME-T is > deprecated. > > > > UC-8100A-ME provides larger RAM and eMMC, better input current than > > UC-8100-ME-T's, and it supports selectable LTE module for US/EU/APAC. > > So what about the existing users of UC-8100-ME-T? > > To me it seems you should just add am335x-moxa-uc-8100-common.dtsi or > similar. And then have minimal dts files for each model. That way you can > easily keep the support UC-8100-ME-T around. Thanks for your good suggestion, I'll take this info for our internal discussion. Best regards, Johnson > > Regards, > > Tony
[PATCH 0/2] Add support for Moxa UC-8100A-ME open platform
Hello all,
UC-8100A-ME is advanced of UC-8100-ME-T, and UC-8100-ME-T is deprecated.
UC-8100A-ME provides larger RAM and eMMC, better input current than
UC-8100-ME-T's, and it supports selectable LTE module for US/EU/APAC.
1. Replace UC-8100-ME-T with UC-8100A-ME for dt-bindings
2. add dts support for Moxa UC-8100A-ME open platform
Thanks,
Johnson
Johnson Chen (2):
dt-bindings: arm: omap: Replace UC-8100-ME-T with UC-8100A-ME
ARM: dts: am335x: add support for Moxa UC-8100A-ME open platform
.../devicetree/bindings/arm/omap/omap.txt | 4 +-
arch/arm/boot/dts/Makefile | 2 +-
...c-8100-me-t.dts => am335x-moxa-uc-8100a-me.dts} | 56 ++
3 files changed, 29 insertions(+), 33 deletions(-)
rename arch/arm/boot/dts/{am335x-moxa-uc-8100-me-t.dts =>
am335x-moxa-uc-8100a-me.dts} (93%)
--
2.11.0
[PATCH 1/2] dt-bindings: arm: omap: Replace UC-8100-ME-T with UC-8100A-ME
UC-8100-ME-T is deprecated, and UC-8100A-ME is advanced of UC-8100-ME-T, so replace UC-8100-ME-T with UC-8100A-ME. Signed-off-by: Johnson Chen --- Documentation/devicetree/bindings/arm/omap/omap.txt | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Documentation/devicetree/bindings/arm/omap/omap.txt b/Documentation/devicetree/bindings/arm/omap/omap.txt index e77635c5422c..f02265a8a0d4 100644 --- a/Documentation/devicetree/bindings/arm/omap/omap.txt +++ b/Documentation/devicetree/bindings/arm/omap/omap.txt @@ -167,8 +167,8 @@ Boards (incomplete list of examples): - AM335x phyBOARD-REGOR: Single Board Computer compatible = "phytec,am335x-regor", "phytec,am335x-phycore-som", "ti,am33xx" -- AM335X UC-8100-ME-T: Communication-centric industrial computing platform - compatible = "moxa,uc-8100-me-t", "ti,am33xx"; +- AM335X UC-8100A-ME: Communication-centric industrial computing platform + compatible = "moxa,uc-8100a-me", "ti,am33xx"; - OMAP5 EVM : Evaluation Module compatible = "ti,omap5-evm", "ti,omap5" -- 2.11.0
[PATCH 2/2] ARM: dts: am335x: add support for Moxa UC-8100A-ME open platform
UC-8100A-ME is advanced of UC-8100-ME-T, and UC-8100-ME-T is deprecated.
UC-8100A-ME provides larger RAM and eMMC, better input current than
UC-8100-ME-T's, and it supports selectable LTE module for US/EU/APAC.
UC-8100A-ME computing platform is designed for embedded data acquisition
industrial applications. Features of UC-8100A-ME series are:
* eMMC
* SPI flash
* SD slot
* 2x LAN
* 2 RS-232/422/485 ports, software-selectable
* Mini PCIe form factor with USB signal
* USB host
* EEPROM
* TPM
* Watchdog
* RTC
* User gpio-keys
* User LEDs
* User button
Signed-off-by: Johnson Chen
Signed-off-by: Wes Huang
Signed-off-by: Fero JD Zhou
---
arch/arm/boot/dts/Makefile | 2 +-
...c-8100-me-t.dts => am335x-moxa-uc-8100a-me.dts} | 56 ++
2 files changed, 27 insertions(+), 31 deletions(-)
rename arch/arm/boot/dts/{am335x-moxa-uc-8100-me-t.dts =>
am335x-moxa-uc-8100a-me.dts} (93%)
diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
index e6a1cac0bfc7..f6ec58f1dd7e 100644
--- a/arch/arm/boot/dts/Makefile
+++ b/arch/arm/boot/dts/Makefile
@@ -793,7 +793,7 @@ dtb-$(CONFIG_SOC_AM33XX) += \
am335x-icev2.dtb \
am335x-lxm.dtb \
am335x-moxa-uc-2101.dtb \
- am335x-moxa-uc-8100-me-t.dtb \
+ am335x-moxa-uc-8100a-me.dtb \
am335x-nano.dtb \
am335x-netcan-plus-1xx.dtb \
am335x-netcom-plus-2xx.dtb \
diff --git a/arch/arm/boot/dts/am335x-moxa-uc-8100-me-t.dts
b/arch/arm/boot/dts/am335x-moxa-uc-8100a-me.dts
similarity index 93%
rename from arch/arm/boot/dts/am335x-moxa-uc-8100-me-t.dts
rename to arch/arm/boot/dts/am335x-moxa-uc-8100a-me.dts
index f03e72cada41..536bbfeaa6e1 100644
--- a/arch/arm/boot/dts/am335x-moxa-uc-8100-me-t.dts
+++ b/arch/arm/boot/dts/am335x-moxa-uc-8100a-me.dts
@@ -1,8 +1,6 @@
-// SPDX-License-Identifier: GPL-2.0-only
+// SPDX-License-Identifier: GPL-2.0
/*
- * Copyright (C) 2017 MOXA Inc. - https://www.moxa.com/
- *
- * Author: SZ Lin (林上智)
+ * Copyright (C) 2020 MOXA Inc. - https://www.moxa.com/
*/
/dts-v1/;
@@ -10,8 +8,8 @@
#include "am33xx.dtsi"
/ {
- model = "Moxa UC-8100-ME-T";
- compatible = "moxa,uc-8100-me-t", "ti,am33xx";
+ model = "Moxa UC-8100A-ME";
+ compatible = "moxa,uc-8100a-me", "ti,am33xx";
cpus {
cpu@0 {
@@ -19,11 +17,6 @@
};
};
- memory {
- device_type = "memory";
- reg = <0x8000 0x2000>; /* 512 MB */
- };
-
vbat: vbat-regulator {
compatible = "regulator-fixed";
};
@@ -40,51 +33,51 @@
leds {
compatible = "gpio-leds";
led1 {
- label = "uc8100me:CEL1";
+ label = "UC-8100A-ME:RED:SGN1";
gpios = <_xten 8 0>;
default-state = "off";
};
led2 {
- label = "uc8100me:CEL2";
+ label = "UC-8100A-ME:YELLOW:SGN2";
gpios = <_xten 9 0>;
default-state = "off";
};
led3 {
- label = "uc8100me:CEL3";
+ label = "UC-8100A-ME:GREEN:SGN3";
gpios = <_xten 10 0>;
default-state = "off";
};
led4 {
- label = "uc8100me:DIA1";
+ label = "UC-8100A-ME:RED:DIA1";
gpios = <_xten 11 0>;
default-state = "off";
};
led5 {
- label = "uc8100me:DIA2";
+ label = "UC-8100A-ME:YELLOW:DIA2";
gpios = <_xten 12 0>;
default-state = "off";
};
led6 {
- label = "uc8100me:DIA3";
+ label = "UC-8100A-ME:GREEN:DIA3";
gpios = <_xten 13 0>;
default-state = "off";
};
led7 {
- label = "uc8100me:SD";
+ label = "UC-8100A-ME:GREEN:SD";
gpios = <_xten 14 0>;
default-state = "off";
};
led8 {
- label = "uc8100me:USB";
+ label = "UC-8100A-ME:GREEN:USB";
gpios = <_xten 15 0>;
default-state = "off";
};
led9 {
- label = "uc8100me:USER";
- gpios = < 20 GPIO_ACTIVE_HIGH>;
+ label = "UC-8100A-ME:GREEN:USER";
+ gpios = < 20 0>;
default-state = "off";
};
};
@@ -219,6 +212,7 @@
pinctrl-single,pins = <
[PATCH v3] rtc: rtc-ds1374: wdt: Use watchdog core for watchdog part
Let ds1374 watchdog use watchdog core functions. It also includes
improving watchdog timer setting and nowayout, and just uses ioctl()
of watchdog core.
Signed-off-by: Johnson Chen
Reported-by: kernel test robot
v2->v3:
- Fix a problem reported by WATCHDOG_CORE if WATCHDOG
- Remove save_client
- Let wdt_margin be 0 for watchdog_init_timeout()
- Use dev_info() rather than pr_info()
- Avoid more strings in this driver
v1->v2:
- Use ds1374_wdt_settimeout() before registering the watchdog
- Remove watchdog_unregister_device() because devm_watchdog_register_device()
is used
- Remove ds1374_wdt_ping()
- TIMER_MARGIN_MAX to 4095 for 24-bit value
- Keep wdt_margin
- Fix coding styles
---
drivers/rtc/Kconfig | 1 +
drivers/rtc/rtc-ds1374.c | 253 ++-
2 files changed, 61 insertions(+), 193 deletions(-)
diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
index b54d87d45c89..c25d51f35f0c 100644
--- a/drivers/rtc/Kconfig
+++ b/drivers/rtc/Kconfig
@@ -282,6 +282,7 @@ config RTC_DRV_DS1374
config RTC_DRV_DS1374_WDT
bool "Dallas/Maxim DS1374 watchdog timer"
depends on RTC_DRV_DS1374
+ select WATCHDOG_CORE if WATCHDOG
help
If you say Y here you will get support for the
watchdog timer in the Dallas Semiconductor DS1374
diff --git a/drivers/rtc/rtc-ds1374.c b/drivers/rtc/rtc-ds1374.c
index 9c51a12cf70f..e86e381e52a0 100644
--- a/drivers/rtc/rtc-ds1374.c
+++ b/drivers/rtc/rtc-ds1374.c
@@ -46,6 +46,7 @@
#define DS1374_REG_WDALM2 0x06
#define DS1374_REG_CR 0x07 /* Control */
#define DS1374_REG_CR_AIE 0x01 /* Alarm Int. Enable */
+#define DS1374_REG_CR_WDSTR0x08 /* 1=INT, 0=RST */
#define DS1374_REG_CR_WDALM0x20 /* 1=Watchdog, 0=Alarm */
#define DS1374_REG_CR_WACE 0x40 /* WD/Alarm counter enable */
#define DS1374_REG_SR 0x08 /* Status */
@@ -71,7 +72,9 @@ struct ds1374 {
struct i2c_client *client;
struct rtc_device *rtc;
struct work_struct work;
-
+#ifdef CONFIG_RTC_DRV_DS1374_WDT
+ struct watchdog_device wdt;
+#endif
/* The mutex protects alarm operations, and prevents a race
* between the enable_irq() in the workqueue and the free_irq()
* in the remove function.
@@ -369,237 +372,99 @@ static const struct rtc_class_ops ds1374_rtc_ops = {
*
*
*/
-static struct i2c_client *save_client;
/* Default margin */
-#define WD_TIMO 131762
+#define TIMER_MARGIN_DEFAULT 32
+#define TIMER_MARGIN_MIN 1
+#define TIMER_MARGIN_MAX 4095 /* 24-bit value */
#define DRV_NAME "DS1374 Watchdog"
-static int wdt_margin = WD_TIMO;
-static unsigned long wdt_is_open;
+static int wdt_margin;
module_param(wdt_margin, int, 0);
MODULE_PARM_DESC(wdt_margin, "Watchdog timeout in seconds (default 32s)");
+static bool nowayout = WATCHDOG_NOWAYOUT;
+module_param(nowayout, bool, 0);
+MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default ="
+ __MODULE_STRING(WATCHDOG_NOWAYOUT)")");
+
+
static const struct watchdog_info ds1374_wdt_info = {
.identity = "DS1374 WTD",
.options= WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING |
WDIOF_MAGICCLOSE,
};
-static int ds1374_wdt_settimeout(unsigned int timeout)
+static int ds1374_wdt_settimeout(struct watchdog_device *wdt, unsigned int
timeout)
{
- int ret = -ENOIOCTLCMD;
- int cr;
+ int ret, cr;
+ struct ds1374 *ds1374 = watchdog_get_drvdata(wdt);
+ struct i2c_client *client = ds1374->client;
- ret = cr = i2c_smbus_read_byte_data(save_client, DS1374_REG_CR);
- if (ret < 0)
- goto out;
+ wdt->timeout = timeout;
+
+ cr = i2c_smbus_read_byte_data(client, DS1374_REG_CR);
+ if (cr < 0)
+ return cr;
/* Disable any existing watchdog/alarm before setting the new one */
cr &= ~DS1374_REG_CR_WACE;
- ret = i2c_smbus_write_byte_data(save_client, DS1374_REG_CR, cr);
+ ret = i2c_smbus_write_byte_data(client, DS1374_REG_CR, cr);
if (ret < 0)
- goto out;
+ return ret;
/* Set new watchdog time */
- ret = ds1374_write_rtc(save_client, timeout, DS1374_REG_WDALM0, 3);
+ timeout = timeout * 4096;
+ ret = ds1374_write_rtc(client, timeout, DS1374_REG_WDALM0, 3);
if (ret) {
- pr_info("couldn't set new watchdog time\n");
- goto out;
+ dev_info(>dev, "couldn't set new watchdog time\n");
+ return ret;
}
/* Enable watchdog timer */
cr |= DS1374_REG_CR_WACE | DS1374_REG_CR_WDALM;
+ cr &= ~DS1374_REG_CR_WDSTR;/* for RST PIN */
cr &= ~DS1374_REG_CR_AIE;
- ret = i2c_smbus_write_byte_data(save_client, DS1374_REG_CR, cr);
+ ret =
RE: [PATCH v2] rtc: rtc-ds1374: wdt: Use watchdog core for watchdog part
Hi,
> > > > + ret = devm_watchdog_register_device(>dev, >wdt);
> > > > if (ret) {
> > > > - misc_deregister(_miscdev);
> > > > + dev_err(>dev, "failed to register DS1374
> > > > watchdog
> > > > +device\n");
>
> There was no error message before, I don't think this one is necessary.
>
> > > > return ret;
> > > > }
> > > > - ds1374_wdt_settimeout(131072);
> > > > +
> > > > + dev_info(>dev, "DS1374 watchdog device enabled\n");
> > >
> > > Is that necessary ?
> > >
> >
> > I think it's good to show watchdog initial timeout. I'll include above
> suggestions in v3, thanks!
> >
>
> No, please avoid adding more strings in that driver.
>
I get it, thanks for review!
Best regards,
Johnson
>
> --
> Alexandre Belloni, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
RE: [PATCH v2] rtc: rtc-ds1374: wdt: Use watchdog core for watchdog part
Hi,
Thanks for your good suggestions!
> > diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig index
> > b54d87d45c89..5e2444af5657 100644
> > --- a/drivers/rtc/Kconfig
> > +++ b/drivers/rtc/Kconfig
> > @@ -282,6 +282,7 @@ config RTC_DRV_DS1374 config
> RTC_DRV_DS1374_WDT
> > bool "Dallas/Maxim DS1374 watchdog timer"
> > depends on RTC_DRV_DS1374
> > + select WATCHDOG_CORE
>
> This has to be
>
> select WATCHDOG_CORE if WATCHDOG
>
> to fix the problem reported by 0-day.
>
> > help
> > If you say Y here you will get support for the
> > watchdog timer in the Dallas Semiconductor DS1374 diff --git
> > a/drivers/rtc/rtc-ds1374.c b/drivers/rtc/rtc-ds1374.c index
> > 9c51a12cf70f..57a4e503b34a 100644
> > --- a/drivers/rtc/rtc-ds1374.c
> > +++ b/drivers/rtc/rtc-ds1374.c
> > @@ -46,6 +46,7 @@
> > #define DS1374_REG_WDALM2 0x06
> > #define DS1374_REG_CR 0x07 /* Control */
> > #define DS1374_REG_CR_AIE 0x01 /* Alarm Int. Enable */
> > +#define DS1374_REG_CR_WDSTR0x08 /* 1=INT, 0=RST */
> > #define DS1374_REG_CR_WDALM0x20 /* 1=Watchdog, 0=Alarm */
> > #define DS1374_REG_CR_WACE 0x40 /* WD/Alarm counter enable */
> > #define DS1374_REG_SR 0x08 /* Status */
> > @@ -71,7 +72,9 @@ struct ds1374 {
> > struct i2c_client *client;
> > struct rtc_device *rtc;
> > struct work_struct work;
> > -
> > +#ifdef CONFIG_RTC_DRV_DS1374_WDT
> > + struct watchdog_device wdt;
> > +#endif
> > /* The mutex protects alarm operations, and prevents a race
> > * between the enable_irq() in the workqueue and the free_irq()
> > * in the remove function.
> > @@ -371,72 +374,76 @@ static const struct rtc_class_ops ds1374_rtc_ops =
> {
> > */
> > static struct i2c_client *save_client;
>
> This is no longer necessary. struct watchdog_device is part of struct ds1374,
> so
> it is possible to derive the pointer to struct ds1374 from it and get the
> client
> pointer from there.
>
> > /* Default margin */
> > -#define WD_TIMO 131762
> > +#define TIMER_MARGIN_DEFAULT 32
> > +#define TIMER_MARGIN_MIN 1
> > +#define TIMER_MARGIN_MAX 4095 /* 24-bit value */
> >
> > #define DRV_NAME "DS1374 Watchdog"
> >
> > -static int wdt_margin = WD_TIMO;
> > -static unsigned long wdt_is_open;
> > +static int wdt_margin = TIMER_MARGIN_DEFAULT;
>
> Should be 0, not TIMER_MARGIN_DEFAULT.
>
> > module_param(wdt_margin, int, 0);
> > MODULE_PARM_DESC(wdt_margin, "Watchdog timeout in seconds
> (default
> > 32s)");
> >
> > +static bool nowayout = WATCHDOG_NOWAYOUT; module_param(nowayout,
> > +bool, 0); MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped
> once
> > +started (default ="
> > + __MODULE_STRING(WATCHDOG_NOWAYOUT)")");
> > +
> > +
> > static const struct watchdog_info ds1374_wdt_info = {
> > .identity = "DS1374 WTD",
> > .options= WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING |
> > WDIOF_MAGICCLOSE,
> > };
> >
> > -static int ds1374_wdt_settimeout(unsigned int timeout)
> > +static int ds1374_wdt_settimeout(struct watchdog_device *wdt,
> > +unsigned int timeout)
> > {
> > - int ret = -ENOIOCTLCMD;
> > - int cr;
> > + int ret, cr;
> >
> > - ret = cr = i2c_smbus_read_byte_data(save_client, DS1374_REG_CR);
> > - if (ret < 0)
> > - goto out;
> > + wdt->timeout = timeout;
> > +
> > + cr = i2c_smbus_read_byte_data(save_client, DS1374_REG_CR);
> > + if (cr < 0)
> > + return cr;
> >
> > /* Disable any existing watchdog/alarm before setting the new one */
> > cr &= ~DS1374_REG_CR_WACE;
> >
> > ret = i2c_smbus_write_byte_data(save_client, DS1374_REG_CR, cr);
> > if (ret < 0)
> > - goto out;
> > + return ret;
> >
> > /* Set new watchdog time */
> > + timeout = timeout * 4096;
> > ret = ds1374_write_rtc(save_client, timeout, DS1374_REG_WDALM0, 3);
> > if (ret) {
> > pr_info("couldn't set new watchdog time\n");
>
> dev_info() can now be used since we have a device. _info seems to be wrong,
> though, since this is an error.
>
> > - goto out;
> > + return ret;
> > }
> >
> > /* Enable watchdog timer */
> > cr |= DS1374_REG_CR_WACE | DS1374_REG_CR_WDALM;
> > + cr &= ~DS1374_REG_CR_WDSTR;/* for RST PIN */
> > cr &= ~DS1374_REG_CR_AIE;
> >
> > ret = i2c_smbus_write_byte_data(save_client, DS1374_REG_CR, cr);
> > if (ret < 0)
> > - goto out;
> > + return ret;
> >
> > return 0;
> > -out:
> > - return ret;
> > }
> >
> > -
> > /*
> > * Reload the watchdog timer. (ie, pat the watchdog)
> > */
> > -static void ds1374_wdt_ping(void)
> > +static int ds1374_wdt_start(struct watchdog_device *wdt)
> > {
> > u32 val;
> > - int ret = 0;
> >
> > - ret = ds1374_read_rtc(save_client, , DS1374_REG_WDALM0, 3);
> > - if (ret)
> > - pr_info("WD TICK FAIL!! %i\n", ret);
> > + return
[PATCH v2] rtc: rtc-ds1374: wdt: Use watchdog core for watchdog part
Let ds1374 watchdog use watchdog core functions. It also includes
improving watchdog timer setting and nowayout, and just uses ioctl()
of watchdog core.
Signed-off-by: Johnson Chen
Reported-by: kernel test robot
v1->v2:
- Use ds1374_wdt_settimeout() before registering the watchdog
- Remove watchdog_unregister_device() because devm_watchdog_register_device()
is used
- Remove ds1374_wdt_ping()
- TIMER_MARGIN_MAX to 4095 for 24-bit value
- Keep wdt_margin
- Fix coding styles
---
drivers/rtc/Kconfig | 1 +
drivers/rtc/rtc-ds1374.c | 236 +--
2 files changed, 52 insertions(+), 185 deletions(-)
diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
index b54d87d45c89..5e2444af5657 100644
--- a/drivers/rtc/Kconfig
+++ b/drivers/rtc/Kconfig
@@ -282,6 +282,7 @@ config RTC_DRV_DS1374
config RTC_DRV_DS1374_WDT
bool "Dallas/Maxim DS1374 watchdog timer"
depends on RTC_DRV_DS1374
+ select WATCHDOG_CORE
help
If you say Y here you will get support for the
watchdog timer in the Dallas Semiconductor DS1374
diff --git a/drivers/rtc/rtc-ds1374.c b/drivers/rtc/rtc-ds1374.c
index 9c51a12cf70f..57a4e503b34a 100644
--- a/drivers/rtc/rtc-ds1374.c
+++ b/drivers/rtc/rtc-ds1374.c
@@ -46,6 +46,7 @@
#define DS1374_REG_WDALM2 0x06
#define DS1374_REG_CR 0x07 /* Control */
#define DS1374_REG_CR_AIE 0x01 /* Alarm Int. Enable */
+#define DS1374_REG_CR_WDSTR0x08 /* 1=INT, 0=RST */
#define DS1374_REG_CR_WDALM0x20 /* 1=Watchdog, 0=Alarm */
#define DS1374_REG_CR_WACE 0x40 /* WD/Alarm counter enable */
#define DS1374_REG_SR 0x08 /* Status */
@@ -71,7 +72,9 @@ struct ds1374 {
struct i2c_client *client;
struct rtc_device *rtc;
struct work_struct work;
-
+#ifdef CONFIG_RTC_DRV_DS1374_WDT
+ struct watchdog_device wdt;
+#endif
/* The mutex protects alarm operations, and prevents a race
* between the enable_irq() in the workqueue and the free_irq()
* in the remove function.
@@ -371,72 +374,76 @@ static const struct rtc_class_ops ds1374_rtc_ops = {
*/
static struct i2c_client *save_client;
/* Default margin */
-#define WD_TIMO 131762
+#define TIMER_MARGIN_DEFAULT 32
+#define TIMER_MARGIN_MIN 1
+#define TIMER_MARGIN_MAX 4095 /* 24-bit value */
#define DRV_NAME "DS1374 Watchdog"
-static int wdt_margin = WD_TIMO;
-static unsigned long wdt_is_open;
+static int wdt_margin = TIMER_MARGIN_DEFAULT;
module_param(wdt_margin, int, 0);
MODULE_PARM_DESC(wdt_margin, "Watchdog timeout in seconds (default 32s)");
+static bool nowayout = WATCHDOG_NOWAYOUT;
+module_param(nowayout, bool, 0);
+MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default ="
+ __MODULE_STRING(WATCHDOG_NOWAYOUT)")");
+
+
static const struct watchdog_info ds1374_wdt_info = {
.identity = "DS1374 WTD",
.options= WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING |
WDIOF_MAGICCLOSE,
};
-static int ds1374_wdt_settimeout(unsigned int timeout)
+static int ds1374_wdt_settimeout(struct watchdog_device *wdt, unsigned int
timeout)
{
- int ret = -ENOIOCTLCMD;
- int cr;
+ int ret, cr;
- ret = cr = i2c_smbus_read_byte_data(save_client, DS1374_REG_CR);
- if (ret < 0)
- goto out;
+ wdt->timeout = timeout;
+
+ cr = i2c_smbus_read_byte_data(save_client, DS1374_REG_CR);
+ if (cr < 0)
+ return cr;
/* Disable any existing watchdog/alarm before setting the new one */
cr &= ~DS1374_REG_CR_WACE;
ret = i2c_smbus_write_byte_data(save_client, DS1374_REG_CR, cr);
if (ret < 0)
- goto out;
+ return ret;
/* Set new watchdog time */
+ timeout = timeout * 4096;
ret = ds1374_write_rtc(save_client, timeout, DS1374_REG_WDALM0, 3);
if (ret) {
pr_info("couldn't set new watchdog time\n");
- goto out;
+ return ret;
}
/* Enable watchdog timer */
cr |= DS1374_REG_CR_WACE | DS1374_REG_CR_WDALM;
+ cr &= ~DS1374_REG_CR_WDSTR;/* for RST PIN */
cr &= ~DS1374_REG_CR_AIE;
ret = i2c_smbus_write_byte_data(save_client, DS1374_REG_CR, cr);
if (ret < 0)
- goto out;
+ return ret;
return 0;
-out:
- return ret;
}
-
/*
* Reload the watchdog timer. (ie, pat the watchdog)
*/
-static void ds1374_wdt_ping(void)
+static int ds1374_wdt_start(struct watchdog_device *wdt)
{
u32 val;
- int ret = 0;
- ret = ds1374_read_rtc(save_client, , DS1374_REG_WDALM0, 3);
- if (ret)
- pr_info("WD TICK FAIL!! %i\n", ret);
+ return ds1374_read_rtc(save_client, , DS1374_REG_WDALM0, 3);
}
-static void ds1374_wdt_disable(void)
+static int ds1374_wdt_stop(struct
RE: [PATCH] rtc: ds1374: wdt: Use watchdog core for watchdog part
Hi,
> > diff --git a/drivers/rtc/rtc-ds1374.c b/drivers/rtc/rtc-ds1374.c index
> > 9c51a12cf70f..25b28f7546ff 100644
> > --- a/drivers/rtc/rtc-ds1374.c
> > +++ b/drivers/rtc/rtc-ds1374.c
> > @@ -1,3 +1,4 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > /*
> > * RTC client/driver for the Maxim/Dallas DS1374 Real-Time Clock over
> I2C
> > *
> > @@ -6,11 +7,7 @@
> > *
> > * Copyright (C) 2014 Rose Technology
> > * Copyright (C) 2006-2007 Freescale Semiconductor
> > - *
> > - * 2005 (c) MontaVista Software, Inc. This file is licensed under
> > - * the terms of the GNU General Public License version 2. This
> > program
> > - * is licensed "as is" without any warranty of any kind, whether
> > express
> > - * or implied.
> > + * Copyright (C) 2005 (c) MontaVista Software, Inc.
> > */
>
> The above should be a separate patch.
>
> > /*
> > * It would be more efficient to use i2c msgs/i2c_transfer directly
> > but, as @@ -46,6 +43,7 @@
> > #define DS1374_REG_WDALM2 0x06
> > #define DS1374_REG_CR 0x07 /* Control */
> > #define DS1374_REG_CR_AIE 0x01 /* Alarm Int. Enable */
> > +#define DS1374_REG_CR_WDSTR0x08 /* 1=INT, 0=RST */
> > #define DS1374_REG_CR_WDALM0x20 /* 1=Watchdog, 0=Alarm */
> > #define DS1374_REG_CR_WACE 0x40 /* WD/Alarm counter enable */
> > #define DS1374_REG_SR 0x08 /* Status */
> > @@ -71,7 +69,9 @@ struct ds1374 {
> > struct i2c_client *client;
> > struct rtc_device *rtc;
> > struct work_struct work;
> > -
> > +#ifdef CONFIG_RTC_DRV_DS1374_WDT
> > + struct watchdog_device wdt;
> > +#endif
> > /* The mutex protects alarm operations, and prevents a race
> > * between the enable_irq() in the workqueue and the free_irq()
> > * in the remove function.
> > @@ -257,7 +257,8 @@ static int ds1374_set_alarm(struct device *dev,
> struct rtc_wkalrm *alarm)
> > goto out;
> >
> > /* Disable any existing alarm before setting the new one
> > -* (or lack thereof). */
> > +* (or lack thereof).
> > +*/
>
> Unrelated (and cosmetic). Drop or separate patch.
>
> > cr &= ~DS1374_REG_CR_WACE;
> >
> > ret = i2c_smbus_write_byte_data(client, DS1374_REG_CR, cr); @@
> > -371,14 +372,21 @@ static const struct rtc_class_ops ds1374_rtc_ops = {
> > */
> > static struct i2c_client *save_client;
> > /* Default margin */
> > -#define WD_TIMO 131762
> > +#define TIMER_MARGIN_DEFAULT 32
>
> Parameter alignment is off. Please use tab after TIMER_MARGIN_DEFAULT.
>
> > +#define TIMER_MARGIN_MIN 1
> > +#define TIMER_MARGIN_MAX (60*60*24) /* one day */
>
> Real limits are necessary. The old limit was 16777216/4096 = 4096 seconds.
>
> Ok, time to look up the datasheet. The counter is a 24-bit value.
> The maximum value is thus 0xff, or 16777215. 16777215 / 4096 = 4095,
> which should be the maximum timeout in seconds to set here.
>
> >
> > #define DRV_NAME "DS1374 Watchdog"
> >
> > -static int wdt_margin = WD_TIMO;
> > -static unsigned long wdt_is_open;
> > -module_param(wdt_margin, int, 0);
> > -MODULE_PARM_DESC(wdt_margin, "Watchdog timeout in seconds (default
> > 32s)");
> > +static int timeout = TIMER_MARGIN_DEFAULT; module_param(timeout, int,
> > +0); MODULE_PARM_DESC(timeout, "Watchdog timeout in seconds (default
> > +32s)");
>
> This changes the module parameter which may be unexpected.
> I would suggest to keep using wdt_margin.
>
> > +
> > +static bool nowayout = WATCHDOG_NOWAYOUT; module_param(nowayout,
> > +bool, 0); MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped
> once
> > +started (default ="
> > + __MODULE_STRING(WATCHDOG_NOWAYOUT)")");
> > +
> >
> > static const struct watchdog_info ds1374_wdt_info = {
> > .identity = "DS1374 WTD",
> > @@ -386,57 +394,61 @@ static const struct watchdog_info
> ds1374_wdt_info = {
> > WDIOF_MAGICCLOSE,
> > };
> >
> > -static int ds1374_wdt_settimeout(unsigned int timeout)
> > +static int ds1374_wdt_settimeout(struct watchdog_device *wdt,
> > + unsigned int timeout)
>
> This fits into one line (new line length limit)
>
> > {
> > - int ret = -ENOIOCTLCMD;
> > - int cr;
> > + int ret, cr;
> > +
> > + wdt->timeout = timeout;
> >
> > - ret = cr = i2c_smbus_read_byte_data(save_client, DS1374_REG_CR);
> > + cr = i2c_smbus_read_byte_data(save_client, DS1374_REG_CR);
> > + ret = cr;
> > if (ret < 0)
> > - goto out;
> > + return ret;
> >
> Assignment to ret is pointless. Just return cr if negative.
>
> > /* Disable any existing watchdog/alarm before setting the new one */
> > cr &= ~DS1374_REG_CR_WACE;
> >
> > ret = i2c_smbus_write_byte_data(save_client, DS1374_REG_CR, cr);
> > if (ret < 0)
> > - goto out;
> > + return ret;
> >
> > /* Set new watchdog time */
> > + timeout = timeout * 4096;
> > ret = ds1374_write_rtc(save_client, timeout, DS1374_REG_WDALM0,
[PATCH] rtc: ds1374: wdt: Use watchdog core for watchdog part
Let ds1374 watchdog use watchdog core. It also includes
improving watchdog timer setting and nowayout, and just uses ioctl()
of watchdog core.
Signed-off-by: Johnson Chen
---
drivers/rtc/rtc-ds1374.c | 257 ++-
1 file changed, 67 insertions(+), 190 deletions(-)
diff --git a/drivers/rtc/rtc-ds1374.c b/drivers/rtc/rtc-ds1374.c
index 9c51a12cf70f..25b28f7546ff 100644
--- a/drivers/rtc/rtc-ds1374.c
+++ b/drivers/rtc/rtc-ds1374.c
@@ -1,3 +1,4 @@
+// SPDX-License-Identifier: GPL-2.0
/*
* RTC client/driver for the Maxim/Dallas DS1374 Real-Time Clock over I2C
*
@@ -6,11 +7,7 @@
*
* Copyright (C) 2014 Rose Technology
* Copyright (C) 2006-2007 Freescale Semiconductor
- *
- * 2005 (c) MontaVista Software, Inc. This file is licensed under
- * the terms of the GNU General Public License version 2. This program
- * is licensed "as is" without any warranty of any kind, whether express
- * or implied.
+ * Copyright (C) 2005 (c) MontaVista Software, Inc.
*/
/*
* It would be more efficient to use i2c msgs/i2c_transfer directly but, as
@@ -46,6 +43,7 @@
#define DS1374_REG_WDALM2 0x06
#define DS1374_REG_CR 0x07 /* Control */
#define DS1374_REG_CR_AIE 0x01 /* Alarm Int. Enable */
+#define DS1374_REG_CR_WDSTR0x08 /* 1=INT, 0=RST */
#define DS1374_REG_CR_WDALM0x20 /* 1=Watchdog, 0=Alarm */
#define DS1374_REG_CR_WACE 0x40 /* WD/Alarm counter enable */
#define DS1374_REG_SR 0x08 /* Status */
@@ -71,7 +69,9 @@ struct ds1374 {
struct i2c_client *client;
struct rtc_device *rtc;
struct work_struct work;
-
+#ifdef CONFIG_RTC_DRV_DS1374_WDT
+ struct watchdog_device wdt;
+#endif
/* The mutex protects alarm operations, and prevents a race
* between the enable_irq() in the workqueue and the free_irq()
* in the remove function.
@@ -257,7 +257,8 @@ static int ds1374_set_alarm(struct device *dev, struct
rtc_wkalrm *alarm)
goto out;
/* Disable any existing alarm before setting the new one
-* (or lack thereof). */
+* (or lack thereof).
+*/
cr &= ~DS1374_REG_CR_WACE;
ret = i2c_smbus_write_byte_data(client, DS1374_REG_CR, cr);
@@ -371,14 +372,21 @@ static const struct rtc_class_ops ds1374_rtc_ops = {
*/
static struct i2c_client *save_client;
/* Default margin */
-#define WD_TIMO 131762
+#define TIMER_MARGIN_DEFAULT 32
+#define TIMER_MARGIN_MIN 1
+#define TIMER_MARGIN_MAX (60*60*24) /* one day */
#define DRV_NAME "DS1374 Watchdog"
-static int wdt_margin = WD_TIMO;
-static unsigned long wdt_is_open;
-module_param(wdt_margin, int, 0);
-MODULE_PARM_DESC(wdt_margin, "Watchdog timeout in seconds (default 32s)");
+static int timeout = TIMER_MARGIN_DEFAULT;
+module_param(timeout, int, 0);
+MODULE_PARM_DESC(timeout, "Watchdog timeout in seconds (default 32s)");
+
+static bool nowayout = WATCHDOG_NOWAYOUT;
+module_param(nowayout, bool, 0);
+MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default ="
+ __MODULE_STRING(WATCHDOG_NOWAYOUT)")");
+
static const struct watchdog_info ds1374_wdt_info = {
.identity = "DS1374 WTD",
@@ -386,57 +394,61 @@ static const struct watchdog_info ds1374_wdt_info = {
WDIOF_MAGICCLOSE,
};
-static int ds1374_wdt_settimeout(unsigned int timeout)
+static int ds1374_wdt_settimeout(struct watchdog_device *wdt,
+ unsigned int timeout)
{
- int ret = -ENOIOCTLCMD;
- int cr;
+ int ret, cr;
+
+ wdt->timeout = timeout;
- ret = cr = i2c_smbus_read_byte_data(save_client, DS1374_REG_CR);
+ cr = i2c_smbus_read_byte_data(save_client, DS1374_REG_CR);
+ ret = cr;
if (ret < 0)
- goto out;
+ return ret;
/* Disable any existing watchdog/alarm before setting the new one */
cr &= ~DS1374_REG_CR_WACE;
ret = i2c_smbus_write_byte_data(save_client, DS1374_REG_CR, cr);
if (ret < 0)
- goto out;
+ return ret;
/* Set new watchdog time */
+ timeout = timeout * 4096;
ret = ds1374_write_rtc(save_client, timeout, DS1374_REG_WDALM0, 3);
if (ret) {
pr_info("couldn't set new watchdog time\n");
- goto out;
+ return ret;
}
/* Enable watchdog timer */
cr |= DS1374_REG_CR_WACE | DS1374_REG_CR_WDALM;
+ cr &= ~DS1374_REG_CR_WDSTR;/* for RST PIN */
cr &= ~DS1374_REG_CR_AIE;
ret = i2c_smbus_write_byte_data(save_client, DS1374_REG_CR, cr);
if (ret < 0)
- goto out;
+ return ret;
return 0;
-out:
- return ret;
}
-
/*
* Reload the watchdog timer. (ie, pat the watchdog)
*/
-static void ds1374_wdt_ping(void)
+static int ds1374_wdt_ping(struct watchdog_device *wdt)
{
RE: [PATCH 3/3] watchdog: ds1374_wdt: Introduce Dallas/Maxim DS1374 Watchdog driver
Hi,
> On 6/22/20 11:28 PM, Johnson CH Chen (陳昭勳) wrote:
> > Hi,
> >
> > Thanks for your good detailed suggestions!
> >
>
> Other feedback suggests to convert the driver to use the watchdog core in the
> rtc code. I would suggest to follow that suggestion.
>
Understand the following suggestions for watchdog timer setting and I will
improve them with watchdog core in rtc code later.
Best regards,
Johnson
> >>> + * It would be more efficient to use i2c msgs/i2c_transfer directly
> >>> +but, as
> >>> + * recommened in .../Documentation/i2c/writing-clients section
> >>> + * "Sending and receiving", using SMBus level communication is
> preferred.
> >>> + */
> >>> +
> >>> +#include
> >>> +#include
> >>> +#include
> >>> +#include
> >>> +#include
> >>> +#include
> >>> +#include
> >>> +#include
> >>> +#include
> >>> +#include
> >>
> >> Alphabetic order please.
> >>
> >>> +
> >>> +#define DEVNAME "ds1374_wdt"
> >>> +
> >>> +#define DS1374_REG_WDALM00x04 /* Watchdog/Alarm */
> >>> +#define DS1374_REG_WDALM10x05
> >>> +#define DS1374_REG_WDALM20x06
> >>> +#define DS1374_REG_CR0x07 /* Control */
> >>> +#define DS1374_REG_CR_AIE0x01 /* Alarm Int. Enable */
> >>> +#define DS1374_REG_CR_WDSTR 0x08 /* 1=INT, 0=RST */
> >>> +#define DS1374_REG_CR_WDALM 0x20 /* 1=Watchdog, 0=Alarm */
> >>> +#define DS1374_REG_CR_WACE 0x40 /* WD/Alarm counter enable */
> >>> +
> >>> +/* Default margin */
> >>> +#define WD_TIMO 131762
> >>> +#define TIMER_MARGIN_MIN 4096 /* 1s */
> >>> +#define TIMER_MARGIN_MAX (60*60*24*4096) /* one day */
> >>> +
> >>> +static int wdt_margin = WD_TIMO;
> >>
> >> Sjould not be pre-initialized. Also, 131762 isn't 32 seconds, it is
> >> 131,762 seconds.
> >>
> >
> > 131762 is 32 seconds actually because watchdog counter increases per
> > 1/4096 seconds for DS1374. If DS1374_REG_CR_WDALM is set to 0 (alarm),
> > alarm counter will increase per 1 second.
> >
>
> The watchdog core keeps timeouts in seconds. For the watchdog core,
> 131762 is 131,762 seconds. Your code assumes that the watchdog core
> does not care about / need to scale, which is wrong. Besides,
> MODULE_PARM_DESC below clearly states "Watchdog timeout _in seconds_"
> (emphasis added).
>
> >>> +module_param(wdt_margin, int, 0);
> >>> +MODULE_PARM_DESC(wdt_margin, "Watchdog timeout in seconds
> (default
> >>> +32s)");
> >>> +
> >>
> >> As a new driver, it would be better to just use the customary "timeout".
> >>
> >>> +static bool nowayout = WATCHDOG_NOWAYOUT;
> module_param(nowayout,
> >>> +bool, 0); MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped
> >> once
> >>> +started default ="
> >>> + __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
> >>> +
> >>> +struct ds1374_wdt {
> >>> + struct i2c_client *client;
> >>> + struct watchdog_device *wdt;
> >>> + struct work_struct work;
> >>
> >> Not used.
> >>
> >>> +
> >>> + /* The mutex protects alarm operations, and prevents a race
> >>> + * between the enable_irq() in the workqueue and the free_irq()
> >>> + * in the remove function.
> >>> + */
> >>
> >> There is no workqueue here, and the remove function is not needed.
> >>
> >>> + struct mutex mutex;
> >>> +};
> >>> +
> >>> +static const struct watchdog_info ds1374_wdt_info = {
> >>> + .identity = DEVNAME,
> >>> + .options= WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING |
> >>> + WDIOF_MAGICCLOSE,
> >>> +};
> >>> +
> >>> +static struct watchdog_device ds1374_wdd;
> >>> +
> >> Move declaration please.
> >>
> >>> +static int ds1374_wdt_settimeout(struct watchdog_device *wdt,
> >>> + unsigned int timeout)
> >>> +{
> >>
> >> How is this synchronized against accesses by the RTC driver ?
> >>
> > By original des
RE: [PATCH 1/3] mfd: ds1374: Introduce Dallas/Maxim DS1374 MFD core driver
Hi, > On 22/06/2020 07:26:56-0700, Guenter Roeck wrote: > > On Mon, Jun 22, 2020 at 12:14:13PM +0100, Lee Jones wrote: > > > On Mon, 22 Jun 2020, Johnson CH Chen (陳昭勳) wrote: > > > > > > > Dallas/Maxim DS1374 is a counter designed to continuously count > > > > time in seconds. It provides an I2C interface to the host to > > > > access RTC clock or Alarm/Watchdog timer. > > > > > > > > Add MFD Core driver, supporting the I2C communication to RTC and > > > > Watchdog devices. > > > > > > > > Signed-off-by: Johnson Chen > > > > --- > > > > drivers/mfd/Kconfig | 11 + > > > > drivers/mfd/Makefile | 2 + > > > > drivers/mfd/ds1374.c | 101 > > > > +++ > > > > 3 files changed, 114 insertions(+) create mode 100644 > > > > drivers/mfd/ds1374.c > > > > > > Not sure I see the point of this driver. > > > > > > > Not entirely sure either. Seems to me the idea is to use the watchdog > > subsystem for watchdog functionality, but that is just a guess and not > > really necessary (the conversion could be done in the rtc driver). > > I don't think the code as written works - the rtc code uses a mutex > > which the watchdog driver obviously isn't aware of. The mutex would > > have to be moved into the mfd code, with respective access functions. > > > > Overall this adds a lot of complexity, and it seems the > > interdependencies between rtc and watchdog functionality are not well > > understood. Plus, other watchdog drivers have recently been added to > > other rtc clock chips, so this adds some inconsistencies in the rtc > > subsystem. Are we going to see this change for all those combined > rtc/watchdog drivers ? > > If so, it might make sense to communicate that now to ensure consistency. > > > > I read the datasheet again and I agree the watchdog part can live in the rtc > driver. As only the RTC alarm and the watchdog are mutually exclusive. I don't > think an MFD driver is necessary. Converting the current driver to the > watchdog subsystem seems to be the correct way forward. > Thanks for your good thinking. If we want to add watchdog function such as "nowayout" to the driver, it's good to try to upstream this in rtc-ds1374.c in rtc subsystem? It seems like more complexity if we want to separate rtc and watchdog for one chip supports. For one chip which supports rtc/alarm and watchdog, can we upstream rtc/alarm and watchdog functions to these driver no matter where it's in rtc or watchdog subsystem? > -- > Alexandre Belloni, Bootlin > Embedded Linux and Kernel engineering > https://bootlin.com Best regards, Johnson
RE: [PATCH 3/3] watchdog: ds1374_wdt: Introduce Dallas/Maxim DS1374 Watchdog driver
Hi,
Thanks for your good detailed suggestions!
> > + * It would be more efficient to use i2c msgs/i2c_transfer directly
> > +but, as
> > + * recommened in .../Documentation/i2c/writing-clients section
> > + * "Sending and receiving", using SMBus level communication is preferred.
> > + */
> > +
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
>
> Alphabetic order please.
>
> > +
> > +#define DEVNAME "ds1374_wdt"
> > +
> > +#define DS1374_REG_WDALM0 0x04 /* Watchdog/Alarm */
> > +#define DS1374_REG_WDALM1 0x05
> > +#define DS1374_REG_WDALM2 0x06
> > +#define DS1374_REG_CR 0x07 /* Control */
> > +#define DS1374_REG_CR_AIE 0x01 /* Alarm Int. Enable */
> > +#define DS1374_REG_CR_WDSTR 0x08 /* 1=INT, 0=RST */
> > +#define DS1374_REG_CR_WDALM0x20 /* 1=Watchdog, 0=Alarm */
> > +#define DS1374_REG_CR_WACE 0x40 /* WD/Alarm counter enable */
> > +
> > +/* Default margin */
> > +#define WD_TIMO 131762
> > +#define TIMER_MARGIN_MIN 4096 /* 1s */
> > +#define TIMER_MARGIN_MAX (60*60*24*4096) /* one day */
> > +
> > +static int wdt_margin = WD_TIMO;
>
> Sjould not be pre-initialized. Also, 131762 isn't 32 seconds, it is 131,762
> seconds.
>
131762 is 32 seconds actually because watchdog counter increases per
1/4096 seconds for DS1374. If DS1374_REG_CR_WDALM is set to 0 (alarm),
alarm counter will increase per 1 second.
> > +module_param(wdt_margin, int, 0);
> > +MODULE_PARM_DESC(wdt_margin, "Watchdog timeout in seconds (default
> > +32s)");
> > +
>
> As a new driver, it would be better to just use the customary "timeout".
>
> > +static bool nowayout = WATCHDOG_NOWAYOUT; module_param(nowayout,
> > +bool, 0); MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped
> once
> > +started default ="
> > + __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
> > +
> > +struct ds1374_wdt {
> > + struct i2c_client *client;
> > + struct watchdog_device *wdt;
> > + struct work_struct work;
>
> Not used.
>
> > +
> > + /* The mutex protects alarm operations, and prevents a race
> > +* between the enable_irq() in the workqueue and the free_irq()
> > +* in the remove function.
> > +*/
>
> There is no workqueue here, and the remove function is not needed.
>
> > + struct mutex mutex;
> > +};
> > +
> > +static const struct watchdog_info ds1374_wdt_info = {
> > + .identity = DEVNAME,
> > + .options= WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING |
> > + WDIOF_MAGICCLOSE,
> > +};
> > +
> > +static struct watchdog_device ds1374_wdd;
> > +
> Move declaration please.
>
> > +static int ds1374_wdt_settimeout(struct watchdog_device *wdt,
> > + unsigned int timeout)
> > +{
>
> How is this synchronized against accesses by the RTC driver ?
>
By original design in rtc-ds1374.c, it seems no synchronized problem between
rtc and watchdog, but I think we can add mutex lock to deal with it.
> > + int ret = -ENOIOCTLCMD;
>
> Not an appropriate error code here.
>
> > + u8 buf[4];
> > + int cr, i;
> > + struct ds1374_wdt *drv_data = watchdog_get_drvdata(wdt);
> > +
> > + ret = cr = i2c_smbus_read_byte_data(drv_data->client, DS1374_REG_CR);
> > + if (ret < 0)
> > + goto out;
>
> "goto out;" is unnecessary. Just return the error. Same everywhere else below.
>
> > +
> > + /* Disable any existing watchdog/alarm before setting the new one */
> > + cr &= ~DS1374_REG_CR_WACE;
> > +
> > + ret = i2c_smbus_write_byte_data(drv_data->client, DS1374_REG_CR, cr);
> > + if (ret < 0)
> > + goto out;
> > +
> > + /* Set new watchdog time */
> > + for (i = 0; i < 3; i++) {
> > + buf[i] = timeout & 0xff;
> > + timeout >>= 8;
> > + }
>
> The value passed to this function from the watchdog core is the timeout in
> seconds. I don't see a conversion to internal values here.
>
For original design in rtc-ds1374.c, ds1374_wdt_settimeout () will call
ds1374_write_rtc() to write hardware register. To separate watchdog and rtc
functions, expand code from ds1374_write_rtc() here, and final buf[] values
will be written into hardware register for DS1374.
> > +
> > + ret = i2c_smbus_write_i2c_block_data(drv_data->client,
> > + DS1374_REG_WDALM0, 3, buf);
> > + if (ret) {
> > + pr_info("couldn't set new watchdog time\n");
> > + goto out;
> > + }
>
> > +
> > + /* Enable watchdog timer */
> > + cr |= DS1374_REG_CR_WACE | DS1374_REG_CR_WDALM;
> > + cr &= ~DS1374_REG_CR_WDSTR;/* for RST PIN */
> > + cr &= ~DS1374_REG_CR_AIE;
> > +
> > + ret = i2c_smbus_write_byte_data(drv_data->client, DS1374_REG_CR, cr);
> > + if (ret < 0)
> > + goto out;
> > +
> > + return 0;
>
> Pointless. Just return ret.
>
> Also, this function needs to store the new
RE: [PATCH 0/3] Use MFD for Dallas/Maxim DS1374 driver series
Hi, > > Hi, > > On 22/06/2020 10:03:25+, Johnson CH Chen (陳昭勳) wrote: > > Hello all, > > > > This patch set uses MFD structure for DS1374 so that RTC and Watchdog > > functions can be separately. Therefore, we can add more Watchdog > > subfunctions here. > > > > A DS1374 MFD core driver supports the I2C communication to RTC and > > Watchdog devices. > > > > 1. Add DS1374 MFD core driver with I2C bus. > > 2. Let DS1374 RTC driver has RTC and Alarm functions only. > > 3. Add DS1374 Watchdog driver. > > > > For reference, this was the last attempt: > > https://lore.kernel.org/linux-rtc/20170718092245.tc5oosbbb6lzvqpy@dell/ > > The main issue I see with your series is that there is no way to select which > of > the rtc or the watchdog driver has to be used as IIRC each function is > mutually > exclusive. I think you should work on the DT bindings, addressing the few > remaining comments. > Thanks for your review and good suggestion! Usage of rtc and watchdog should be defined in DT binding if we want to keep them separate. I'll work towards these later. > -- > Alexandre Belloni, Bootlin > Embedded Linux and Kernel engineering > https://bootlin.com Best regards, Johnson
[PATCH 3/3] watchdog: ds1374_wdt: Introduce Dallas/Maxim DS1374 Watchdog driver
Here provide Watchdog function from rtc-ds1374.c which is in RTC subsystem
originally. Besides, add nowayout and implement Watchdog margin time when
DS1374 Watchdog device is found.
Signed-off-by: Johnson Chen
---
drivers/watchdog/Kconfig | 11 ++
drivers/watchdog/Makefile | 1 +
drivers/watchdog/ds1374_wdt.c | 330 ++
3 files changed, 342 insertions(+)
create mode 100644 drivers/watchdog/ds1374_wdt.c
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index b739c476955b..b4ff4b3c18da 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -316,6 +316,17 @@ config ZIIRAVE_WATCHDOG
To compile this driver as a module, choose M here: the
module will be called ziirave_wdt.
+config DS1374_WATCHDOG
+ tristate "Dallas/Maxim DS1374 Watchdog timer"
+ depends on MFD_DS1374
+ select WATCHDOG_CORE
+ help
+ If you say Y here you will get support for the watchdog timer
+ in the Dallas/Maxim Semiconductor DS1374 real-time clock chips.
+
+ This driver can also be built as a module. If so the module
+ will be called ds1374_wdt.
+
config RAVE_SP_WATCHDOG
tristate "RAVE SP Watchdog timer"
depends on RAVE_SP_CORE
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index 6de2e4ceef19..1c468f5d9e5f 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -224,3 +224,4 @@ obj-$(CONFIG_MENF21BMC_WATCHDOG) += menf21bmc_wdt.o
obj-$(CONFIG_MENZ069_WATCHDOG) += menz69_wdt.o
obj-$(CONFIG_RAVE_SP_WATCHDOG) += rave-sp-wdt.o
obj-$(CONFIG_STPMIC1_WATCHDOG) += stpmic1_wdt.o
+obj-$(CONFIG_DS1374_WATCHDOG) += ds1374_wdt.o
diff --git a/drivers/watchdog/ds1374_wdt.c b/drivers/watchdog/ds1374_wdt.c
new file mode 100644
index ..ce69e45973fd
--- /dev/null
+++ b/drivers/watchdog/ds1374_wdt.c
@@ -0,0 +1,330 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * RTC client/driver for the Maxim/Dallas DS1374 Real-Time Clock over I2C
+ *
+ * Based on code by Randy Vinson ,
+ * which was based on the m41t00.c by Mark Greer .
+ *
+ * Copyright (C) 2020 Johnson Chen
+ * Copyright (C) 2014 Rose Technology
+ * Copyright (C) 2006-2007 Freescale Semiconductor
+ * Copyright (C) 2005 MontaVista Software, Inc.
+ */
+
+/*
+ * It would be more efficient to use i2c msgs/i2c_transfer directly but, as
+ * recommened in .../Documentation/i2c/writing-clients section
+ * "Sending and receiving", using SMBus level communication is preferred.
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#define DEVNAME "ds1374_wdt"
+
+#define DS1374_REG_WDALM0 0x04 /* Watchdog/Alarm */
+#define DS1374_REG_WDALM1 0x05
+#define DS1374_REG_WDALM2 0x06
+#define DS1374_REG_CR 0x07 /* Control */
+#define DS1374_REG_CR_AIE 0x01 /* Alarm Int. Enable */
+#define DS1374_REG_CR_WDSTR 0x08 /* 1=INT, 0=RST */
+#define DS1374_REG_CR_WDALM0x20 /* 1=Watchdog, 0=Alarm */
+#define DS1374_REG_CR_WACE 0x40 /* WD/Alarm counter enable */
+
+/* Default margin */
+#define WD_TIMO 131762
+#define TIMER_MARGIN_MIN 4096 /* 1s */
+#define TIMER_MARGIN_MAX (60*60*24*4096) /* one day */
+
+static int wdt_margin = WD_TIMO;
+module_param(wdt_margin, int, 0);
+MODULE_PARM_DESC(wdt_margin, "Watchdog timeout in seconds (default 32s)");
+
+static bool nowayout = WATCHDOG_NOWAYOUT;
+module_param(nowayout, bool, 0);
+MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started default ="
+ __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
+
+struct ds1374_wdt {
+ struct i2c_client *client;
+ struct watchdog_device *wdt;
+ struct work_struct work;
+
+ /* The mutex protects alarm operations, and prevents a race
+* between the enable_irq() in the workqueue and the free_irq()
+* in the remove function.
+*/
+ struct mutex mutex;
+};
+
+static const struct watchdog_info ds1374_wdt_info = {
+ .identity = DEVNAME,
+ .options= WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING |
+ WDIOF_MAGICCLOSE,
+};
+
+static struct watchdog_device ds1374_wdd;
+
+static int ds1374_wdt_settimeout(struct watchdog_device *wdt,
+ unsigned int timeout)
+{
+ int ret = -ENOIOCTLCMD;
+ u8 buf[4];
+ int cr, i;
+ struct ds1374_wdt *drv_data = watchdog_get_drvdata(wdt);
+
+ ret = cr = i2c_smbus_read_byte_data(drv_data->client, DS1374_REG_CR);
+ if (ret < 0)
+ goto out;
+
+ /* Disable any existing watchdog/alarm before setting the new one */
+ cr &= ~DS1374_REG_CR_WACE;
+
+ ret = i2c_smbus_write_byte_data(drv_data->client, DS1374_REG_CR, cr);
+ if (ret < 0)
+ goto out;
+
+ /* Set new watchdog time */
+ for (i = 0; i < 3; i++) {
+
[PATCH 2/3] rtc: rtc-ds1374: Move out Watchdog function and I2C client
Move Watchdog function to ds1374_wdt.c which is in the Watchdog
subsystem, and just keep RTC and Alarm function in rtc-ds1374.c.
Besides, Move I2C client to ds1374.c which is in MFD subsystem.
Signed-off-by: Johnson Chen
---
drivers/rtc/Kconfig | 9 +-
drivers/rtc/rtc-ds1374.c | 458 +++
2 files changed, 76 insertions(+), 391 deletions(-)
diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
index ec873f09c763..cc020fb7a2a9 100644
--- a/drivers/rtc/Kconfig
+++ b/drivers/rtc/Kconfig
@@ -271,6 +271,8 @@ config RTC_DRV_DS1307_CENTURY
config RTC_DRV_DS1374
tristate "Dallas/Maxim DS1374"
+ depends on MFD_DS1374
+
help
If you say yes here you get support for Dallas Semiconductor
DS1374 real-time clock chips. If an interrupt is associated
@@ -279,13 +281,6 @@ config RTC_DRV_DS1374
This driver can also be built as a module. If so, the module
will be called rtc-ds1374.
-config RTC_DRV_DS1374_WDT
- bool "Dallas/Maxim DS1374 watchdog timer"
- depends on RTC_DRV_DS1374
- help
- If you say Y here you will get support for the
- watchdog timer in the Dallas Semiconductor DS1374
- real-time clock chips.
config RTC_DRV_DS1672
tristate "Dallas/Maxim DS1672"
diff --git a/drivers/rtc/rtc-ds1374.c b/drivers/rtc/rtc-ds1374.c
index 9c51a12cf70f..d8ff0fbba52c 100644
--- a/drivers/rtc/rtc-ds1374.c
+++ b/drivers/rtc/rtc-ds1374.c
@@ -1,17 +1,16 @@
+// SPDX-License-Identifier: GPL-2.0
/*
* RTC client/driver for the Maxim/Dallas DS1374 Real-Time Clock over I2C
*
* Based on code by Randy Vinson ,
* which was based on the m41t00.c by Mark Greer .
*
+ * Copyright (C) 2020 Johnson Chen
* Copyright (C) 2014 Rose Technology
* Copyright (C) 2006-2007 Freescale Semiconductor
- *
- * 2005 (c) MontaVista Software, Inc. This file is licensed under
- * the terms of the GNU General Public License version 2. This program
- * is licensed "as is" without any warranty of any kind, whether express
- * or implied.
+ * Copyright (C) 2005 MontaVista Software, Inc.
*/
+
/*
* It would be more efficient to use i2c msgs/i2c_transfer directly but, as
* recommended in .../Documentation/i2c/writing-clients.rst section
@@ -25,17 +24,10 @@
#include
#include
#include
-#include
#include
-#include
-#include
-#ifdef CONFIG_RTC_DRV_DS1374_WDT
-#include
-#include
-#include
-#include
-#include
-#endif
+#include
+
+#define DEVNAME "ds1374_rtc"
#define DS1374_REG_TOD00x00 /* Time of Day */
#define DS1374_REG_TOD10x01
@@ -53,21 +45,7 @@
#define DS1374_REG_SR_AF 0x01 /* Alarm Flag */
#define DS1374_REG_TCR 0x09 /* Trickle Charge */
-static const struct i2c_device_id ds1374_id[] = {
- { "ds1374", 0 },
- { }
-};
-MODULE_DEVICE_TABLE(i2c, ds1374_id);
-
-#ifdef CONFIG_OF
-static const struct of_device_id ds1374_of_match[] = {
- { .compatible = "dallas,ds1374" },
- { }
-};
-MODULE_DEVICE_TABLE(of, ds1374_of_match);
-#endif
-
-struct ds1374 {
+struct ds1374_rtc {
struct i2c_client *client;
struct rtc_device *rtc;
struct work_struct work;
@@ -80,8 +58,6 @@ struct ds1374 {
int exiting;
};
-static struct i2c_driver ds1374_driver;
-
static int ds1374_read_rtc(struct i2c_client *client, u32 *time,
int reg, int nbytes)
{
@@ -158,7 +134,7 @@ static int ds1374_check_rtc_status(struct i2c_client
*client)
static int ds1374_read_time(struct device *dev, struct rtc_time *time)
{
- struct i2c_client *client = to_i2c_client(dev);
+ struct i2c_client *client = to_i2c_client(dev->parent);
u32 itime;
int ret;
@@ -171,21 +147,21 @@ static int ds1374_read_time(struct device *dev, struct
rtc_time *time)
static int ds1374_set_time(struct device *dev, struct rtc_time *time)
{
- struct i2c_client *client = to_i2c_client(dev);
+ struct i2c_client *client = to_i2c_client(dev->parent);
unsigned long itime = rtc_tm_to_time64(time);
return ds1374_write_rtc(client, itime, DS1374_REG_TOD0, 4);
}
-#ifndef CONFIG_RTC_DRV_DS1374_WDT
+#ifndef CONFIG_DS1374_WATCHDOG
/* The ds1374 has a decrementer for an alarm, rather than a comparator.
* If the time of day is changed, then the alarm will need to be
* reset.
*/
static int ds1374_read_alarm(struct device *dev, struct rtc_wkalrm *alarm)
{
- struct i2c_client *client = to_i2c_client(dev);
- struct ds1374 *ds1374 = i2c_get_clientdata(client);
+ struct i2c_client *client = to_i2c_client(dev->parent);
+ struct ds1374_rtc *drv_data = dev->driver_data;
u32 now, cur_alarm;
int cr, sr;
int ret = 0;
@@ -193,7 +169,7 @@ static int ds1374_read_alarm(struct device *dev, struct
rtc_wkalrm *alarm)
if (client->irq <= 0)
return -EINVAL;
- mutex_lock(>mutex);
+
[PATCH 1/3] mfd: ds1374: Introduce Dallas/Maxim DS1374 MFD core driver
Dallas/Maxim DS1374 is a counter designed to continuously count
time in seconds. It provides an I2C interface to the host to
access RTC clock or Alarm/Watchdog timer.
Add MFD Core driver, supporting the I2C communication to RTC and
Watchdog devices.
Signed-off-by: Johnson Chen
---
drivers/mfd/Kconfig | 11 +
drivers/mfd/Makefile | 2 +
drivers/mfd/ds1374.c | 101 +++
3 files changed, 114 insertions(+)
create mode 100644 drivers/mfd/ds1374.c
diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 687e9c848053..d16031f4b487 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -1980,6 +1980,17 @@ config MFD_STM32_LPTIMER
To compile this driver as a module, choose M here: the
module will be called stm32-lptimer.
+config MFD_DS1374
+ tristate "Support for Dallas/Maxim DS1374"
+ depends on I2C
+ select MFD_CORE
+ help
+ Say yes here to add support for Dallas/Maxim DS1374 Semiconductor.
+ This driver provides RTC and watchdog.
+
+ This driver can also be built as a module. If so, module will be
+ called ds1374.
+
config MFD_STM32_TIMERS
tristate "Support for STM32 Timers"
depends on (ARCH_STM32 && OF) || COMPILE_TEST
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index bea2be419822..a6463dd4aa33 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -243,6 +243,8 @@ obj-$(CONFIG_INTEL_SOC_PMIC_CHTWC) +=
intel_soc_pmic_chtwc.o
obj-$(CONFIG_INTEL_SOC_PMIC_CHTDC_TI) += intel_soc_pmic_chtdc_ti.o
mt6397-objs:= mt6397-core.o mt6397-irq.o
obj-$(CONFIG_MFD_MT6397) += mt6397.o
+obj-$(CONFIG_MFD_DS1374) += ds1374.o
+
obj-$(CONFIG_INTEL_SOC_PMIC_MRFLD) += intel_soc_pmic_mrfld.o
obj-$(CONFIG_MFD_ALTERA_A10SR) += altera-a10sr.o
diff --git a/drivers/mfd/ds1374.c b/drivers/mfd/ds1374.c
new file mode 100644
index ..6e9041aba5d2
--- /dev/null
+++ b/drivers/mfd/ds1374.c
@@ -0,0 +1,101 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Mamim/Dallas DS1374 MFD Core Driver.
+ *
+ * Copyright (C) 2020 Johnson Chen
+ */
+
+#include
+#include
+#include
+#include
+#include
+
+
+static struct mfd_cell ds1374_cell[] = {
+ { .name = "ds1374_rtc", },
+ { .name = "ds1374_wdt", },
+};
+
+static int ds1374_probe(struct i2c_client *client,
+ const struct i2c_device_id *id)
+{
+ int ret;
+
+ ret = i2c_check_functionality(client->adapter,
+ I2C_FUNC_SMBUS_BYTE_DATA |
+ I2C_FUNC_SMBUS_WORD_DATA |
+ I2C_FUNC_SMBUS_BYTE);
+ if (!ret)
+ return -ENODEV;
+
+ ret = devm_mfd_add_devices(>dev, 0, ds1374_cell,
+ ARRAY_SIZE(ds1374_cell), NULL, 0, NULL);
+
+ if (ret < 0) {
+ dev_err(>dev, "failed to add DS1374 sub-devices\n");
+ return ret;
+ }
+
+ return 0;
+}
+
+static int ds1374_remove(struct i2c_client *client)
+{
+ mfd_remove_devices(>dev);
+
+ return 0;
+}
+
+#ifdef CONFIG_PM_SLEEP
+static int ds1374_suspend(struct device *dev)
+{
+ struct i2c_client *client = to_i2c_client(dev);
+
+ if (client->irq > 0 && device_may_wakeup(>dev))
+ enable_irq_wake(client->irq);
+ return 0;
+}
+
+static int ds1374_resume(struct device *dev)
+{
+ struct i2c_client *client = to_i2c_client(dev);
+
+ if (client->irq > 0 && device_may_wakeup(>dev))
+ disable_irq_wake(client->irq);
+ return 0;
+}
+#endif
+
+static const struct i2c_device_id ds1374_id[] = {
+ { "ds1374", 0 },
+ { }
+};
+MODULE_DEVICE_TABLE(i2c, ds1374_id);
+
+#ifdef CONFIG_OF
+static const struct of_device_id ds1374_of_match[] = {
+ { .compatible = "dallas,ds1374" },
+ { }
+};
+MODULE_DEVICE_TABLE(of, ds1374_of_match);
+#endif
+
+static SIMPLE_DEV_PM_OPS(ds1374_pm, ds1374_suspend, ds1374_resume);
+
+static struct i2c_driver ds1374_driver = {
+ .driver = {
+ .name = "ds1374",
+ .of_match_table = of_match_ptr(ds1374_of_match),
+ .pm = _pm,
+ },
+ .probe = ds1374_probe,
+ .remove = ds1374_remove,
+ .id_table = ds1374_id,
+};
+
+module_i2c_driver(ds1374_driver);
+
+MODULE_DESCRIPTION("Dallas/Maxim DS1374 MFD Core Driver");
+MODULE_AUTHOR("Johnson Chen ");
+MODULE_LICENSE("GPL");
--
2.20.1
[PATCH 0/3] Use MFD for Dallas/Maxim DS1374 driver series
Hello all, This patch set uses MFD structure for DS1374 so that RTC and Watchdog functions can be separately. Therefore, we can add more Watchdog subfunctions here. A DS1374 MFD core driver supports the I2C communication to RTC and Watchdog devices. 1. Add DS1374 MFD core driver with I2C bus. 2. Let DS1374 RTC driver has RTC and Alarm functions only. 3. Add DS1374 Watchdog driver. Thanks, Johnson Johnson Chen (3): mfd: ds1374: Introduce Dallas/Maxim DS1374 MFD core driver rtc: rtc-ds1374: Move out Watchdog function and I2C client watchdog: ds1374_wdt: Introduce Dallas/Maxim DS1374 Watchdog driver drivers/mfd/Kconfig | 11 + drivers/mfd/Makefile | 2 + drivers/mfd/ds1374.c | 101 drivers/rtc/Kconfig | 9 +- drivers/rtc/rtc-ds1374.c | 458 ++ drivers/watchdog/Kconfig | 11 + drivers/watchdog/Makefile | 1 + drivers/watchdog/ds1374_wdt.c | 330 8 files changed, 532 insertions(+), 391 deletions(-) create mode 100644 drivers/mfd/ds1374.c create mode 100644 drivers/watchdog/ds1374_wdt.c -- 2.20.1
