Messages in this thread | | | Date | Tue, 01 Apr 2014 17:32:42 -0700 | From | Guenter Roeck <> | Subject | Re: [PATCH 1/2] watchdog: Add Cadence WDT driver |
| |
On 04/01/2014 04:20 AM, Michal Simek wrote: > Hi Guenter, > >>> +/** >>> + * struct cdns_wdt - Watchdog device structure >>> + * @regs: baseaddress of device >>> + * @rst: reset flag >>> + * @clk: struct clk * of a clock source >>> + * @prescaler: for saving prescaler value >>> + * @ctrl_clksel: counter clock prescaler selection >>> + * @io_lock: spinlock for IO register access >>> + * @cdns_wdt_device: watchdog device structure >>> + * @cdns_wdt_notifier: notifier structure >>> + * >>> + * Structure containing parameters specific to cadence watchdog. >>> + */ >>> +struct cdns_wdt { >>> + void __iomem *regs; >>> + u32 rst; >>> + struct clk *clk; >>> + u32 prescaler; >>> + u32 ctrl_clksel; >>> + spinlock_t io_lock; >>> + struct watchdog_device cdns_wdt_device; >>> + struct notifier_block cdns_wdt_notifier; >>> +}; >>> + >>> +/* Write access to Registers */ >>> +static inline void cdns_wdt_writereg(void __iomem *offset, u32 val) >>> +{ >>> + writel_relaxed(val, offset); >>> +} >>> + >> >> Not really sure if this function provides any value. > > I can't see any problem to use this helper IO function > but maybe we could do it a little bit differently. > Currently implementation is just passing values to writel_relaxed() > > What about to do it like this? > > static inline void cdns_wdt_writereg(struct cdns_wdt *wdt, u32 offset, u32 val) > { > writel_relaxed(val, wdt->regs + offset); > } > Yes, that would make more sense.
Guenter
> This solution was suggested by Mark here too. > https://lkml.org/lkml/2014/3/17/234 > > The reason for having one IO helper function is > 1. Simper debugging when you add printks to one location and > you get full list for accesses > 2. This driver can be also used by BE Linux on Microblaze in PL > that's why there could be a need to autodetect endianess directly > on the IP itself. Then using helper function is necessary. > 3. We have met with secure monitor implementation where all these > io accesses should be done via smc calls and having > IO helper function is easier for change. > > Thanks, > Michal >
| |