Re: [PATCH v2] firewire-core: remove cast of function callback

[Takashi Sakamoto]

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