On Thu, Oct 8, 2015 at 6:54 PM, Luis R. Rodriguez <mcgrof@xxxxxxxx> wrote: > On Thu, Oct 08, 2015 at 01:36:53PM -0400, Josh Boyer wrote: >> On Thu, Oct 1, 2015 at 1:44 PM, Luis R. Rodriguez >> <mcgrof@xxxxxxxxxxxxxxxx> wrote: >> > From: David Howells <dhowells@xxxxxxxxxx> >> > >> > We'll want to reuse this same code later in order to >> > read two separate types of file contents. This generalizes >> > fw_read_file() for reading a file rebrands it as fw_read_file(). >> >> Er, maybe that should read "...fw_read_file_contents() for reading a >> file and rebrands it as fw_read_file()." ? > > Thanks, corrected. > >> > This caller lets us pegs arbitrary data onto the target >> > buffer and size if the file is found. >> >> This sentence is somewhat confusing. The data isn't arbitrary. It is >> what the caller wants you to read from path. What is arbitrary, at >> least in the context of this function, is the path passed to it. >> Maybe rewrite this as: >> >> "The new function allows us to read file contents from arbitrary paths >> and return the data and size of the files read." > > The path is arbitrary but what I meant by arbitrary data is that > the data need no longer be firmware, whereas fw_read_file_contents() > *did* require passing firmware_class data structures. What this does > is it make the possibility of eventually making a more core system > data file reader more obvious, so for instance the goal is to later > share a reader with: > > - firmware_class: fw_read_file() > - module: kernel_read() > - kexec: copy_file_fd() > > I will clarify this in the commit log and also clarify the path is > arbitrary as well as you note. > >> > While at it this cleans up the exit paths on fw_read_file(). >> > >> > Signed-off-by: David Howells <dhowells@xxxxxxxxxx> >> > Signed-off-by: Luis R. Rodriguez <mcgrof@xxxxxxxx> >> >> The code changes themselves look fine. > > Thank you for the review. Can I peg your Acked-by or Reviewed-by? > How about this for a change in the commit log: > > firmware: generalize reading file contents as a helper > > We'll want to reuse this same code later in order to read > two separate types of file contents. This generalizes > fw_read_file_contents() for reading a file and rebrands it > as fw_read_file(). This new caller is now generic and that > path can be arbitrary, the caller is also agnostic to the > firmware_class code now, which begs the possibility of code > re-use with other similar callers in the kernel. For instance > in the future we may want to share a solution with: > > - firmware_class: fw_read_file() > - module: kernel_read() > - kexec: copy_file_fd() > > While at it this also cleans up the exit paths on fw_read_file(). That reads much clearer to me. Thanks. With that changed: Reviewed-by: Josh Boyer <jwboyer@xxxxxxxxxxxxxxxxx> josh -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
