Hi,
On Mon, May 25, 2020 at 01:55:32AM +0200, Stefan Richter wrote:
> > Why is this in a .h file? What's wrong with just putting it in the .c
> > file as a static function? There's no need to make this an inline,
> > right?
>
> There is no need for this in a header.
> Furthermore, I prefer the original switch statement over the nested if/else.
>
> Here is another proposal; I will resend it later as a proper patch.
>
> diff --git a/drivers/firewire/core-cdev.c b/drivers/firewire/core-cdev.c
> index 719791819c24..bece1b69b43f 100644
> --- a/drivers/firewire/core-cdev.c
> +++ b/drivers/firewire/core-cdev.c
> @@ -957,7 +957,6 @@ static int ioctl_create_iso_context(struct client
> *client, union ioctl_arg *arg)
> {
> struct fw_cdev_create_iso_context *a = &arg->create_iso_context;
> struct fw_iso_context *context;
> - fw_iso_callback_t cb;
> int ret;
>
> BUILD_BUG_ON(FW_CDEV_ISO_CONTEXT_TRANSMIT != FW_ISO_CONTEXT_TRANSMIT ||
> @@ -969,20 +968,15 @@ static int ioctl_create_iso_context(struct client
> *client, union ioctl_arg *arg)
> case FW_ISO_CONTEXT_TRANSMIT:
> if (a->speed > SCODE_3200 || a->channel > 63)
> return -EINVAL;
> -
> - cb = iso_callback;
> break;
>
> case FW_ISO_CONTEXT_RECEIVE:
> if (a->header_size < 4 || (a->header_size & 3) ||
> a->channel > 63)
> return -EINVAL;
> -
> - cb = iso_callback;
> break;
>
> case FW_ISO_CONTEXT_RECEIVE_MULTICHANNEL:
> - cb = (fw_iso_callback_t)iso_mc_callback;
> break;
>
> default:
> @@ -990,9 +984,15 @@ static int ioctl_create_iso_context(struct client
> *client, union ioctl_arg *arg)
> }
>
> context = fw_iso_context_create(client->device->card, a->type,
> - a->channel, a->speed, a->header_size, cb, client);
> + a->channel, a->speed, a->header_size, NULL, client);
> if (IS_ERR(context))
> return PTR_ERR(context);
> +
> + if (a->type == FW_ISO_CONTEXT_RECEIVE_MULTICHANNEL)
> + context->callback.mc = iso_mc_callback;
> + else
> + context->callback.sc = iso_callback;
> +
> if (client->version < FW_CDEV_VERSION_AUTO_FLUSH_ISO_OVERFLOW)
> context->drop_overflow_headers = true;
At first place, I wrote the similar patch but judged it's a bit ad-hoc
way that callback functions are assigned after the call of
fw_iso_context_create() in raw code. For explicitness in a point of things
being declarative, I put the inline function into header, and avoid
someone's misfortune for future even if IEEE 1394 is enough legacy.
Anyway, I don't mind Stefan's proposal since it works well. It depends
on developers' fashion to choose policy to write codes.
Thanks
Takashi Sakamoto
