Messages in this thread |  | | Date | Sun, 27 Aug 2017 21:36:07 -0700 | From | Sukadev Bhattiprolu <> | Subject | Re: [PATCH v7 04/12] powerpc/vas: Define helpers to access MMIO regions |
| |
Michael Ellerman [mpe@ellerman.id.au] wrote: > Hi Suka, > > Comments inline. > > Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com> writes: > > diff --git a/arch/powerpc/platforms/powernv/vas-window.c b/arch/powerpc/platforms/powernv/vas-window.c > > index 6156fbe..a3a705a 100644 > > --- a/arch/powerpc/platforms/powernv/vas-window.c > > +++ b/arch/powerpc/platforms/powernv/vas-window.c > > @@ -9,9 +9,182 @@ > > > > #include <linux/types.h> > > #include <linux/mutex.h> > > +#include <linux/slab.h> > > +#include <linux/io.h> > > > > #include "vas.h" > > > > +/* > > + * Compute the paste address region for the window @window using the > > + * ->paste_base_addr and ->paste_win_id_shift we got from device tree. > > + */ > > +void compute_paste_address(struct vas_window *window, uint64_t *addr, int *len) > > +{ > > + uint64_t base, shift; > > Please use the kernel types, so u64 here.
Ok.
> > > + int winid; > > + > > + base = window->vinst->paste_base_addr; > > + shift = window->vinst->paste_win_id_shift; > > + winid = window->winid; > > + > > + *addr = base + (winid << shift); > > + if (len) > > + *len = PAGE_SIZE; > > Having multiple output parameters makes for a pretty awkward API. Is it > really necesssary given len is a constant PAGE_SIZE anyway. > > If you didn't return len, then you could just make the function return > the addr, and you wouldn't need any output parameters.
I agree, I went back and forth on it. I was trying to avoid callers making assumptions on the size. But since there are just a couple of places, I guess we could have them assume PAGE_SIZE.
> > One of the callers that passes len is unmap_paste_region(), but that > is a bit odd. It would be more natural I think if once a window is > mapped it knows its size. Or if the mapping will always just be one page > then we can just know that.
Agree, since the len values are constant I was trying to avoid saving them in each of the 64K windows - so the compute during unmap. Will change to assume PAGE_SIZE.
Also agree with other comments here.
|  |