Messages in this thread | | | Date | Tue, 29 Sep 2015 19:30:24 +0100 | From | Mark Brown <> | Subject | Re: [PATCH 2/2] regmap: debugfs: improve regmap_reg_ranges_read_file() |
| |
On Tue, Sep 29, 2015 at 12:29:02AM +0200, Rasmus Villemoes wrote:
This patch is an example of why SubmittingPatches recommends splitting things up into one change per patch, it would be much easier to read and review as a series (especially given that there's very few collisions).
> * A page is a bit much for two integers and a bit of punctuation. 64 > bytes should suffice.
Right, the reason PAGE_SIZE was chosen is that it's a natural unit for the allocator and is clearly absurdly large for the data. For 64 bytes I have to think for a moment if it's suitably large. Please leave it at PAGE_SIZE, it's not like this hangs around for any length of time.
> * Calling strlen() on entry no less than three times is silly, > especially when snprintf() has returned that value (which was just > thrown away).
I think we were expecting the compiler would figure out that strlen() is a pure function and do the right thing here (though I do see it's missing an annotation).
> * Transferring entry to the output buffer using snprintf is silly, > when we know the length. Use memcpy instead.
Right, that's a legacy of a previous version transferring the maximum amount of data possible (which got abandoned due to complexity). However with the changes to use the return value of snprinf() it seems like the best thing to do here is to go back to this and just fill up as much of the buffer as we can by using snprintf() to do the transfer.
That said I think memcpy() is going to be the best way of getting to that since one of the issues there (which currently doesn't work) is slicing things off the front and memcpy() handles that nicely. [unhandled content-type:application/pgp-signature] | |