Re: [RFC PATCH v2 9/9] tools: spkgimage: add Renesas SPKG format

[Ralph Siemsen <ralph.siemsen@linaro.org>]

On Mon, Aug 22, 2022 at 11:42:54PM -0400, Sean Anderson wrote:
>>>>>+static int spkgimage_check_image_types(uint8_t type)
>>>>>+{
>>>>>+     return type == IH_TYPE_RENESAS_SPKG ? 0 : 1;
>>>>
>>>>This function is not necessary if you only support one type.
>>>
>>>Without this function, mkimage kept telling me that my format
>>>(spkgimage) was not supported, and none of my callbacks got invoked.
>>>It only complained when trying to generate a header. When listing the
>>>supported formats, spkgimage showed up correctly.
>>>
>>>I'll take another look on Monday, maybe I missed something obvious.
>>
>>I have re-checked this:
>>- without the function, mkimage complains that spkgimage is unknown
>>- with a function that unconditionally returns 0, it works fine
>>
>>If it really is meant to work without the function, then a bug must
>>have crept in elsewhere...
>
>Huh. I did a quick grep so maybe I missed something. IMO this *should*
>work without a function, because we have tons of drivers which just
>have an equality check. In any case, you can just do
>
>return type == IH_TYPE_RENESAS_SPKG ? 0 : -EINVAL;

It works fine when I use the following for the function:

static int spkgimage_check_image_types(uint8_t type)
{
	return 0;
}

However if no function is provided, i.e. U_BOOT_IMAGE_TYPE has
NULL for check_image_type field, then mkimage fails with the error:

tools/mkimage: unsupported type Renesas SPKG Image

Looking at this a bit more, it seems to be due to:

struct image_type_params *imagetool_get_type(int type)
{
	...snip...

	for (curr = start; curr != end; curr++) {
		if ((*curr)->check_image_type) {
			if (!(*curr)->check_image_type(type))
				return *curr;
		}
	}
	return NULL;
}

So the only way to get non-NULL from imagetool_get_type is for
there to be a callback function, and it must return zero. And
this in turn causes mkimage to bail out quite early in main():

	/* set tparams as per input type_id */
	tparams = imagetool_get_type(params.type);
	if (tparams == NULL && !params.lflag) {
		fprintf (stderr, "%s: unsupported type %s\n",
			params.cmdname, genimg_get_type_name(params.type));
		exit (EXIT_FAILURE);
	}

Unless I am missing something, it seems I must provide a function.

-Ralph

PS I will post an updated series (v3) eventually. I'm working on making 
changes to the clock driver on the kernel side, to keep it in sync with 
the changes you requested in the u-boot side.