Re: [RFC net-next 01/15] net: cpc: add base skeleton driver

[Andrew Lunn <andrew@lunn.ch>]

> +/**
> + * cpc_interface_register() - Register CPC interface.
> + * @intf: CPC device to register.
> + *
> + * Context: Process context.
> + *
> + * Return: 0 if successful, otherwise a negative error code.
> + */
> +int cpc_interface_register(struct cpc_interface *intf)
> +{
> +	int err;
> +
> +	err = device_add(&intf->dev);
> +	if (err)
> +		return err;
> +
> +	return 0;

I guess this will change in a later patch, but maybe not. This should
just be

	return device_add(&intf->dev);

> +}
> +
> +/**
> + * cpc_interface_unregister() - Unregister a CPC interface.
> + * @intf: CPC device to unregister.
> + *
> + * Context: Process context.
> + */
> +void cpc_interface_unregister(struct cpc_interface *intf)
> +{
> +	device_del(&intf->dev);
> +	cpc_interface_put(intf);
> +}

It seems odd that unregister is not a mirror of register?

> +/**
> + * cpc_interface_get() - Get a reference to interface and return its pointer.
> + * @intf: Interface to get.
> + *
> + * Return: Interface pointer with its reference counter incremented, or %NULL.
> + */
> +static inline struct cpc_interface *cpc_interface_get(struct cpc_interface *intf)
> +{
> +	if (!intf || !get_device(&intf->dev))
> +		return NULL;
> +	return intf;
> +}

What is the use case for passing in NULL?

> +
> +/**
> + * cpc_interface_put() - Release reference to an interface.
> + * @intf: CPC interface
> + *
> + * Context: Process context.
> + */
> +static inline void cpc_interface_put(struct cpc_interface *intf)
> +{
> +	if (intf)
> +		put_device(&intf->dev);
> +}
> +
> +/**
> + * cpc_interface_get_priv() - Get driver data associated with this interface.
> + * @intf: Interface pointer.
> + *
> + * Return: Driver data, set at allocation via cpc_interface_alloc().
> + */
> +static inline void *cpc_interface_get_priv(struct cpc_interface *intf)
> +{
> +	if (!intf)
> +		return NULL;
> +	return dev_get_drvdata(&intf->dev);
> +}

What is the use case for passing in NULL?

To me, this is hiding bugs. It seems better to let the kernel opp so
you find out where you lost your intf.

	Andrew