Messages in this thread | | | Date | Fri, 7 Jan 2022 12:05:45 +0100 | From | Robert Richter <> | Subject | Re: [PATCH v2 3/4] Watchdog: sp5100_tco: Add EFCH SMBus controller initialization using MMIO |
| |
On 06.01.22 13:07:11, Terry Bowman wrote: > On 1/6/22 12:18 PM, Guenter Roeck wrote: > > On Wed, Nov 03, 2021 at 11:15:20AM -0500, Terry Bowman wrote:
> >> diff --git a/drivers/watchdog/sp5100_tco.c b/drivers/watchdog/sp5100_tco.c > >> index 80ae42ae7aaa..4777e672a8ad 100644 > >> --- a/drivers/watchdog/sp5100_tco.c > >> +++ b/drivers/watchdog/sp5100_tco.c > >> @@ -48,12 +48,14 @@ > >> /* internal variables */ > >> > >> enum tco_reg_layout { > >> - sp5100, sb800, efch > >> + sp5100, sb800, efch, efch_mmio > >> }; > >> > >> struct sp5100_tco { > >> struct watchdog_device wdd; > >> void __iomem *tcobase; > >> + void __iomem *addr; > >> + struct resource *res; > > > > I must admit that I really don't like this code. Both res and > > addr are only used during initialization, yet their presence suggests > > runtime usage. Any chance to reqork this to not require those variables ?
We did that in an earlier version, see struct efch_cfg of:
https://patchwork.kernel.org/project/linux-watchdog/patch/20210813213216.54780-1-Terry.Bowman@amd.com/
The motivation of it was the same as you suggested to only use it during init.
Having it in struct sp5100_tco made things simpler esp. in the definition of the function interfaces where those new members are used.
If that init vars are no longer in struct sp5100_tco then callers of efch_read_pm_reg8() and efch_update_pm_reg8() will need to carry a pointer to them. To avoid this I see those options:
1) Implement them as global (or a single global struct) and possibly protect it by a mutex. There is only a single device anyway and we wouldn't need a protection.
2) Have an own mmio implementation of tco_timer_enable() and/or sp5100_tco_timer_init().
> Yes, v3 will include refactoring to remove 'res' and 'addr'. I will also > correct the trailing newline you mentioned in an earlier email. > > Regards, > Terry > > >> enum tco_reg_layout tco_reg_layout;
While at it, tco_reg_layout is also only used during initialization and can be moved there too. This would raise option 3:
3) Add a pointer of struct sp5100_tco to struct efch_cfg and use that struct instead in init funtions only. But that causes the most rework (which would be ok to me).
Going with 3) looks the cleanest way, I would try that. But all options have its advantages.
-Robert
> >> };
| |