On Tue Oct 6, 2026 at 4:25 PM CEST, Tzung-Bi Shih wrote: > On Sun, Oct 04, 2026 at 02:12:52PM +0200, Paul Louvel wrote: >> static int wdt_probe(struct platform_device *pdev) >> { >> const struct platform_device_id *id = platform_get_device_id(pdev); >> + const struct wdt_pdata *pdata = pdev->dev.platform_data; > > Use dev_get_platdata(). > >> @@ -522,12 +533,16 @@ static int wdt_probe(struct platform_device *pdev) >> */ >> static int __init wdt_use_alt_key(const struct dmi_system_id *d) >> { >> - wdt_cfg_enter = 0x88; >> - wdt_cfg_leave = 0xBB; >> + struct wdt_pdata *pdata = d->driver_data; >> + >> + pdata->siocfg_enter = 0x88; >> + pdata->siocfg_leave = 0xBB; >> >> return 0; >> } >> >> +static struct wdt_pdata pdata; >> + >> static const struct dmi_system_id wdt_dmi_table[] __initconst = { >> { >> .matches = { >> @@ -537,6 +552,7 @@ static const struct dmi_system_id wdt_dmi_table[] __initconst = { >> DMI_EXACT_MATCH(DMI_BOARD_NAME, "SHARKBAY"), >> }, >> .callback = wdt_use_alt_key, >> + .driver_data = &pdata, >> }, >> {} >> }; >> @@ -581,27 +597,33 @@ static struct platform_device *wdt_pdev; >> >> static int __init wdt_init(void) >> { >> + int sioaddr; >> int ret; >> int chip; >> >> + pdata.siocfg_enter = 0x87; >> + pdata.siocfg_leave = 0xAA; >> + >> /* Apply system-specific quirks */ >> dmi_check_system(wdt_dmi_table); >> >> - wdt_io = SIO_REG_CONF_ADDR0; >> - chip = wdt_find(SIO_REG_CONF_ADDR0); >> + sioaddr = SIO_REG_CONF_ADDR0; >> + chip = wdt_find(sioaddr, pdata.siocfg_enter, pdata.siocfg_leave); >> if (chip < 0) { >> - wdt_io = SIO_REG_CONF_ADDR1; >> - chip = wdt_find(SIO_REG_CONF_ADDR1); >> + sioaddr = SIO_REG_CONF_ADDR1; >> + chip = wdt_find(sioaddr, pdata.siocfg_enter, pdata.siocfg_leave); >> if (chip < 0) >> return chip; >> } >> >> + pdata.sioaddr = sioaddr; >> + > > `pdata` is still global. Given that there are only a few settings, maybe > we can use the same method I proposed in [3/6] patch, and use > `dmi_first_match()` to avoid the global variable. E.g.: > > struct w83627hf_sio_pdata { > u16 sioaddr; > u8 siocfg_enter; > u8 siocfg_leave; > }; > > static const struct w83627hf_sio_pdata default_pdata = { > .siocfg_enter = 0x87, > .siocfg_leave = 0xaa, > }; > > static const struct w83627hf_sio_pdata alt_pdata = { > .siocfg_enter = 0x88, > .siocfg_leave = 0xbb, > }; > > static const struct dmi_system_id wdt_dmi_table[] __initconst = { > { > .matches = { > DMI_EXACT_MATCH(DMI_SYS_VENDOR, "INVENTEC"), > DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "Symphony"), > DMI_EXACT_MATCH(DMI_BOARD_NAME, "SHARKBAY"), > }, > .driver_data = (void *)&alt_pdata, > }, > {} > }; > > Then, in `wdt_init()`, it can define a local `pdata` and initialize it > based on the DMI match: > > const struct dmi_system_id *match; > struct w83627hf_sio_pdata pdata; > > match = dmi_first_match(wdt_dmi_table); > if (match) > pdata = *(const struct w83627hf_sio_pdata *)match->driver_data; > else > pdata = default_pdata; > > pdata.sioaddr = SIO_REG_CONF_ADDR0; > chip = wdt_find(pdata.sioaddr, pdata.siocfg_enter, pdata.siocfg_leave); Yes, pdata was still global, but the scope is limited to the init function. Your proposition looks better tho. -- Paul Louvel, Bootlin Embedded Linux and Kernel engineering https://bootlin.com text: Return-Path: Delivered-To: paul.louvel@bootlin.com Received: from mail.galae.net ([172.22.1.253]) by 9e7fea892943 with LMTP id oH2THD72yGpQfCkAtaWaaQ (envelope-from ) for ; Fri, 09 Oct 2026 16:12:14 +0200 X-Original-To: paul.louvel@bootlin.com Received: from [127.0.0.1] (localhost [127.0.0.1]) by localhost (Mailerdaemon) with ESMTPSA id 1C92A11D70841; Fri, 9 Oct 2026 16:12:07 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bootlin.com; s=dkim; t91555130; h=from:subject:date:message-id:to:cc:mime-version:content-type: content-transfer-encoding:in-reply-to:references; bhÍZYN8ntqvipg3FYRWOYxSDKsm2OOZGI6Qs5B7jTHyg=; b=OaV7x4LGSW6b2FnAKVdiks6LA9di0h6IEKNQVLYWnT0py3eLzLeIzaoC7AoA+fTFRaL/Xj 4wWvkXky+Z5f3v295TFVfUFFM5B/QGXhLcwWFHT8e0sttThRh2jox1FXUrYz+mOUvGMzWQ IX9sSHcM08okc7CvmcRSP2LL+XSQsZIKXadnkCyBZZUYC6iglTZv1FT+GBnFhKSdWsr8mN Wy/XjjpTmUzHioiFShyML8FQ+hrgMjp2c+E8hkxv06qlu/+kQlpVsFzygTVU32jzs4rnho k1o0ohkJw8L3ZnpPckzPPlafRA5hkmeK/MkTpEMlqA8aaP+7lDQHVx9tgGkRiw=Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=UTF-8 Date: Fri, 09 Oct 2026 16:12:06 +0200 Message-Id: From: "Paul Louvel" To: "Tzung-Bi Shih" , "Paul Louvel" Cc: "Wim Van Sebroeck" , "Guenter Roeck" , , , "Thomas Petazzoni" Subject: Re: [PATCH v3 4/6] watchdog: w83627hf_wdt: Store Super I/O configurations in platform data X-Mailer: aerc 0.21.0-0-g5549850facc2 References: <20261004-w83627hf_wdt-improvements-v3-0-8e27b518595e@bootlin.com> <20261004-w83627hf_wdt-improvements-v3-4-8e27b518595e@bootlin.com> In-Reply-To: X-Last-TLS-Session-Version: TLSv1.3 On Tue Oct 6, 2026 at 4:25 PM CEST, Tzung-Bi Shih wrote: > On Sun, Oct 04, 2026 at 02:12:52PM +0200, Paul Louvel wrote: >> static int wdt_probe(struct platform_device *pdev) >> { >> const struct platform_device_id *id = platform_get_device_id(pdev); >> + const struct wdt_pdata *pdata = pdev->dev.platform_data; > > Use dev_get_platdata(). > >> @@ -522,12 +533,16 @@ static int wdt_probe(struct platform_device *pdev) >> */ >> static int __init wdt_use_alt_key(const struct dmi_system_id *d) >> { >> - wdt_cfg_enter = 0x88; >> - wdt_cfg_leave = 0xBB; >> + struct wdt_pdata *pdata = d->driver_data; >> + >> + pdata->siocfg_enter = 0x88; >> + pdata->siocfg_leave = 0xBB; >> >> return 0; >> } >> >> +static struct wdt_pdata pdata; >> + >> static const struct dmi_system_id wdt_dmi_table[] __initconst = { >> { >> .matches = { >> @@ -537,6 +552,7 @@ static const struct dmi_system_id wdt_dmi_table[] __initconst = { >> DMI_EXACT_MATCH(DMI_BOARD_NAME, "SHARKBAY"), >> }, >> .callback = wdt_use_alt_key, >> + .driver_data = &pdata, >> }, >> {} >> }; >> @@ -581,27 +597,33 @@ static struct platform_device *wdt_pdev; >> >> static int __init wdt_init(void) >> { >> + int sioaddr; >> int ret; >> int chip; >> >> + pdata.siocfg_enter = 0x87; >> + pdata.siocfg_leave = 0xAA; >> + >> /* Apply system-specific quirks */ >> dmi_check_system(wdt_dmi_table); >> >> - wdt_io = SIO_REG_CONF_ADDR0; >> - chip = wdt_find(SIO_REG_CONF_ADDR0); >> + sioaddr = SIO_REG_CONF_ADDR0; >> + chip = wdt_find(sioaddr, pdata.siocfg_enter, pdata.siocfg_leave); >> if (chip < 0) { >> - wdt_io = SIO_REG_CONF_ADDR1; >> - chip = wdt_find(SIO_REG_CONF_ADDR1); >> + sioaddr = SIO_REG_CONF_ADDR1; >> + chip = wdt_find(sioaddr, pdata.siocfg_enter, pdata.siocfg_leave); >> if (chip < 0) >> return chip; >> } >> >> + pdata.sioaddr = sioaddr; >> + > > `pdata` is still global. Given that there are only a few settings, maybe > we can use the same method I proposed in [3/6] patch, and use > `dmi_first_match()` to avoid the global variable. E.g.: > > struct w83627hf_sio_pdata { > u16 sioaddr; > u8 siocfg_enter; > u8 siocfg_leave; > }; > > static const struct w83627hf_sio_pdata default_pdata = { > .siocfg_enter = 0x87, > .siocfg_leave = 0xaa, > }; > > static const struct w83627hf_sio_pdata alt_pdata = { > .siocfg_enter = 0x88, > .siocfg_leave = 0xbb, > }; > > static const struct dmi_system_id wdt_dmi_table[] __initconst = { > { > .matches = { > DMI_EXACT_MATCH(DMI_SYS_VENDOR, "INVENTEC"), > DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "Symphony"), > DMI_EXACT_MATCH(DMI_BOARD_NAME, "SHARKBAY"), > }, > .driver_data = (void *)&alt_pdata, > }, > {} > }; > > Then, in `wdt_init()`, it can define a local `pdata` and initialize it > based on the DMI match: > > const struct dmi_system_id *match; > struct w83627hf_sio_pdata pdata; > > match = dmi_first_match(wdt_dmi_table); > if (match) > pdata = *(const struct w83627hf_sio_pdata *)match->driver_data; > else > pdata = default_pdata; > > pdata.sioaddr = SIO_REG_CONF_ADDR0; > chip = wdt_find(pdata.sioaddr, pdata.siocfg_enter, pdata.siocfg_leave); Yes, pdata was still global, but the scope is limited to the init function. Your proposition looks better tho. -- Paul Louvel, Bootlin Embedded Linux and Kernel engineering https://bootlin.com