Re: [PATCH v3 03/14] ASoC: SOF: Add driver debug support.

Pierre-Louis Bossart
2018-12-11T23:29:12Z

>> + if (count > size - pos)
>> + count = size - pos;
> max() / min()
ok
>
>> + /* intermediate buffer size must be u32 multiple */
>> + size = (count + 3) & ~3;
> Hmm, don't we have something like round_up()?
yes, round_up (count, 4) would work.
>
>> + dfse->dfsentry = debugfs_create_file(name, 0444, sdev->debugfs_root,
>> + dfse, &sof_dfs_fops);
>> + if (!dfse->dfsentry) {
>> + dev_err(sdev->dev, "error: cannot create debugfs entry.\n");
>> + return -ENODEV;
>> + }
> We shouldn't rely on debugfs files.
> Thus, error checking needs to be removed.
>
> Something like
>
> void ()
> {
> struct dentry *d;
>
> d = debugfs_...();
> if (!d)
> dev_err();
> }

You lost me on this one. The main purpose of this return -ENODEV is to
avoid adding more entries if one item failed. It's not different from
what our estimeed colleagues did in skl-debug.c

>
>
>> + dfse->dfsentry = debugfs_create_file(name, 0444, sdev->debugfs_root,
>> + dfse, &sof_dfs_fops);
>> + if (!dfse->dfsentry) {
>> + dev_err(sdev->dev, "error: cannot create debugfs entry.\n");
>> + return -ENODEV;
>> + }
> Ditto.
>
>> + sdev->debugfs_root = debugfs_create_dir("sof", NULL);
>> + if (IS_ERR_OR_NULL(sdev->debugfs_root)) {
>> + dev_err(sdev->dev, "error: failed to create debugfs directory\n");
>> + return -EINVAL;
>> + }
>> +
>> + /* create debugFS files for platform specific MMIO/DSP memories */
>> + for (i = 0; i < ops->debug_map_count; i++) {
>> + map = &ops->debug_map[i];
>> +
>> + err = snd_sof_debugfs_io_create_item(sdev, sdev->bar[map->bar] +
>> + map->offset, map->size,
>> + map->name);
>> + if (err < 0)
>> + dev_err(sdev->dev, "error: cannot create debugfs for %s\n",
>> + map->name);
> Ditto (error message dups above).
>
>> + }
>> +
>> + return err;

or maybe that's the issue, we shouldn't return an error on the init
proper, but I see nothing wrong in stopping when things go south.

>> +}
>> +EXPORT_SYMBOL(snd_sof_dbg_init);