Arnd Bergmann <arnd@xxxxxxxx> writes:
On Wed, Dec 26, 2018 at 1:43 AM Finn Thain <fthain@xxxxxxxxxxxxxxxxxxx> wrote:
+static ssize_t ppc_nvram_get_size(void)
+{
+ if (ppc_md.nvram_size)
+ return ppc_md.nvram_size();
+ return -ENODEV;
+}
+const struct nvram_ops arch_nvram_ops = {
+ .read = ppc_nvram_read,
+ .write = ppc_nvram_write,
+ .get_size = ppc_nvram_get_size,
+ .sync = ppc_nvram_sync,
+};
Coming back to this after my comment on the m68k side, I notice that
there is now a double indirection through function pointers. Have you
considered completely removing the operations from ppc_md instead
by having multiple copies of nvram_ops?
With the current method, it does seem odd to have a single
per-architecture instance of the exported structure containing
function pointers. This doesn't give us the flexibility of having
multiple copies in the kernel the way that ppc_md does, but it adds
overhead compared to simply exporting the functions directly.
Yeah TBH I'm not convinced the arch ops is the best solution.
Why can't each arch just implement the required ops functions? On ppc
we'd still use ppc_md but that would be a ppc detail.
Optional ops are fairly easy to support by providing a default
implementation, eg. instead of:
+ if (arch_nvram_ops.get_size == NULL)
+ return -ENODEV;
+
+ nvram_size = arch_nvram_ops.get_size();
+ if (nvram_size < 0)
+ return nvram_size;
We do in some header:
#ifndef arch_nvram_get_size
static inline int arch_nvram_get_size(void)
{
return -ENODEV;
}
#endif
And then:
nvram_size = arch_nvram_get_size();
if (nvram_size < 0)
return nvram_size;
But I haven't digested the whole series so maybe I'm missing something?
cheers
