[PATCH v2 5/9] misc: qcom_geni: Add minicore support

[Varadarajan Narayanan]

Hi Simon,

Thanks for the feedback, need one clarification...

> > +     ...
> > +     if (elf) {
> > +             hdr = info;
> > +             ...
> > +     } else if (IS_ENABLED(CONFIG_QCOM_GENI_MINICORE)) {
>
> If elf is false and CONFIG_QCOM_GENI_MINICORE is disabled some
> variables are used uninitialised. Please add an else clause that
> returns an error.

Ok.

> > diff --git a/drivers/misc/qcom_geni.c b/drivers/misc/qcom_geni.c
> > @@ -377,15 +413,22 @@ int qcom_geni_load_firmware(...)
> > +     if (IS_ELF(*(Elf32_Ehdr *)fw)) {
> > +             ...
> > +             elf = 1;
> > +             info = hdr;
> > +     } else {
> > +             elf = 0;
> > +             info = fw;
> > +     }
>
> Using 1/0 for a bool is a bit odd. Please use true/false.

Ok.

> > diff --git a/drivers/misc/qcom_geni.c b/drivers/misc/qcom_geni.c
> > @@ -492,7 +540,11 @@ static int probe_children_load_firmware(...)
> > +#if CONFIG_XPL_BUILD
> > +int qcom_geni_fw_initialise(struct udevice *dev)
> > +#else
> > +int qcom_geni_fw_initialise(void)
> > +#endif
>
> Having two different function signatures based on a preprocessor
> conditional makes future maintenance harder. Consider always passing
> dev and ignoring it when unused.

The signature difference is because for one case it is

	int (*probe)(struct udevice *dev);

and

	typedef int (*event_handler_simple_t)(void);

for the other. Not sure how to reconcile between these two.
Please advice.

Thanks
Varada