Re: [Xenomai] Hilscher driver for cifX boards

[Jan Kiszka <jan.kiszka@siemens.com>]

On 2013-03-05 11:45, Jerome Poncin wrote:
> The code with #ifdef DEBUG is also questionable, it will tend to bitrot
> since nobody will think to enable it.
> 
> => I disagree with you, it could be helpful if somebody have a problem
> and want to debug quickly.
> But I can remove it if it's not standard to xenomai kernel.

#ifdef DEBUG is widely considered obsolete. If you have debug relevant
information, tracepoints are much better as they are runtime switchable.

But to look at a concrete example:

> +static ssize_t cifx_pci_read(struct rtdm_dev_context *context,
> +                 rtdm_user_info_t *user_info, void *buf,
> +                 size_t nbyte)
> +{
> +    struct rtdm_device *info = (struct rtdm_device *)context->device;
> +
> +    if (nbyte > sizeof(struct io_info)) {
> +#ifdef DEBUG
> +        rtdm_printk
> +            ("cifx rtdm driver error : data user size too big\n");
> +#endif /* DEBUG */
> +
> +        return 0;

If it is an error, return the proper code (-EINVAL? Or -E2BIG?). That
will be visible without any driver instrumentation.

> +    }
> +
> +    /* Copy data information for userland */
> +    if (rtdm_safe_copy_to_user(user_info, buf,
> +                   ((struct io_info *)info->device_data),
> +                   nbyte)) {
> +#ifdef DEBUG
> +        rtdm_printk
> +            ("cifx rtdm driver error : can't copy data from driver\n");
> +#endif /* DEBUG */

And this is a bug: return -EFAULT. Again, no need for driver
instrumentation.

> +    }
> +
> +    return nbyte;
> +}

...

> 
> I think you are not convinced yet that "code is the best documentation"
> 
> => Yes, you right it's because I worked long time on safety project (for
> me it's a habit) .
> Moreover I thunk it could be helpful in an open source project, if
> somebody want to more easily understand code.

Non-trivial code is better augmented with comments. However, commenting
on the obvious is like meaningless chatting. ;)

> 
> The coding style is a bit indigest
> 
> => I agree with you but that the price to be compliant with
> checkpatch.pl... limitation to 80 characters per line is difficult for me.
> If you saw some better thing to do, don't hesitate ;-) ! I tried to my
> best !

If you overrun the 80-char limit, it is usually a good sign that you
should introduce a function.

Jan

PS: Your mail client does funky marking of cited text. The web standard
is ">".

-- 
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux