On Fri, Oct 09, 2015 at 08:46:42AM -0400, Josh Boyer wrote: > 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> Thanks, amended. Luis -- 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
