[PATCH v3 2/3] net: fm: Add firmware name parameter

[Simon Glass]

Hi Sean,

On Fri, 30 Dec 2022 at 09:32, Sean Anderson <sean.anderson at seco.com> wrote:
>
> On 12/29/22 17:38, Simon Glass wrote:
> > Hi Sean,
> >
> > On Thu, 29 Dec 2022 at 10:53, Sean Anderson <sean.anderson at seco.com> wrote:
> >>
> >> In order to read the firmware from the filesystem, we need a file name.
> >> Read the firmware name from the device tree, using the firmware-name
> >> property. This property is commonly used in Linux to determine the
> >> correct name to use (and can be seen in several device trees in U-Boot).
> >>
> >> Signed-off-by: Sean Anderson <sean.anderson at seco.com>
> >> Reviewed-by: Ramon Fried <rfried.dev at gmail.com>
> >> ---
> >>
> >> (no changes since v1)
> >>
> >>  drivers/net/fm/fm.c | 15 ++++++++++++---
> >>  drivers/net/fm/fm.h |  2 +-
> >>  2 files changed, 13 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/net/fm/fm.c b/drivers/net/fm/fm.c
> >> index 055dd61fbe..457200e766 100644
> >> --- a/drivers/net/fm/fm.c
> >> +++ b/drivers/net/fm/fm.c
> >> @@ -8,6 +8,7 @@
> >>  #include <image.h>
> >>  #include <malloc.h>
> >>  #include <asm/io.h>
> >> +#include <dm/device_compat.h>
> >>  #include <linux/errno.h>
> >>  #include <u-boot/crc.h>
> >>  #include <dm.h>
> >> @@ -353,7 +354,7 @@ static void fm_init_qmi(struct fm_qmi_common *qmi)
> >>
> >>  /* Init common part of FM, index is fm num# like fm as above */
> >>  #ifdef CONFIG_TFABOOT
> >> -int fm_init_common(int index, struct ccsr_fman *reg)
> >> +int fm_init_common(int index, struct ccsr_fman *reg, const char *firmware_name)
> >>  {
> >>         int rc;
> >>         void *addr = NULL;
> >> @@ -448,7 +449,7 @@ int fm_init_common(int index, struct ccsr_fman *reg)
> >>         return fm_init_bmi(index, &reg->fm_bmi_common);
> >>  }
> >>  #else
> >> -int fm_init_common(int index, struct ccsr_fman *reg)
> >> +int fm_init_common(int index, struct ccsr_fman *reg, const char *firmware_name)
> >>  {
> >>         int rc;
> >>  #if defined(CONFIG_SYS_QE_FMAN_FW_IN_NOR)
> >> @@ -561,6 +562,8 @@ static const struct udevice_id fman_ids[] = {
> >>
> >>  static int fman_probe(struct udevice *dev)
> >>  {
> >> +       const char *firmware_name = NULL;
> >> +       int ret;
> >>         struct fman_priv *priv = dev_get_priv(dev);
> >>
> >>         priv->reg = (struct ccsr_fman *)(uintptr_t)dev_read_addr(dev);
> >> @@ -570,7 +573,13 @@ static int fman_probe(struct udevice *dev)
> >>                 return -EINVAL;
> >>         }
> >>
> >> -       return fm_init_common(priv->fman_id, priv->reg);
> >> +       ret = dev_read_string_index(dev, "firmware-name", 0, &firmware_name);
> >> +       if (ret && ret != -EINVAL) {
> >> +               dev_dbg(dev, "Could not read firmware-name\n");
> >> +               return ret;
> >> +       }
> >> +
> >> +       return fm_init_common(priv->fman_id, priv->reg, firmware_name);
> >>  }
> >>
> >>  static int fman_remove(struct udevice *dev)
> >> diff --git a/drivers/net/fm/fm.h b/drivers/net/fm/fm.h
> >> index ba858cc24b..a2d5b03429 100644
> >> --- a/drivers/net/fm/fm.h
> >> +++ b/drivers/net/fm/fm.h
> >> @@ -106,7 +106,7 @@ struct fm_port_global_pram {
> >>
> >>  void *fm_muram_alloc(int fm_idx, size_t size, ulong align);
> >>  void *fm_muram_base(int fm_idx);
> >> -int fm_init_common(int index, struct ccsr_fman *reg);
> >> +int fm_init_common(int index, struct ccsr_fman *reg, const char *firmware_name);
> >
> > Please add a full function comment.
>
> I don't think that's really necessary. This is called from one place, and serves
> mostly to organize the code (now that non-DM net is gone).
>

As a matter of good practice, all exported functions should be
documented in their header files. May as well start now, can perhaps
include the header docs in an rST file.

Regards,
Simon