Re: [PATCH v3 5/7] venus: Add debugfs interface to set firmware log level

From: Stanimir Varbanov <stanimir.varbanov@linaro.org>
To: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Stanimir Varbanov <stanimir.varbanov@linaro.org>
Cc: linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-media@vger.kernel.org, linux-arm-msm@vger.kernel.org,
	linux-btrfs@vger.kernel.org, linux-acpi@vger.kernel.org,
	netdev@vger.kernel.org, Joe Perches <joe@perches.com>,
	Jason Baron <jbaron@akamai.com>
Subject: Re: [PATCH v3 5/7] venus: Add debugfs interface to set firmware log level
Date: Thu, 11 Jun 2020 14:51:00 +0300	[thread overview]
Message-ID: <32eeb0ce-49fd-34ed-d5c8-0406ea174525@linaro.org> (raw)
In-Reply-To: <20200609111224.GA780233@kroah.com>

Hi Greg,

Thanks for the comments!

On 6/9/20 2:12 PM, Greg Kroah-Hartman wrote:
> On Tue, Jun 09, 2020 at 01:46:02PM +0300, Stanimir Varbanov wrote:
>> +int venus_dbgfs_init(struct venus_core *core)
>> +{
>> +	core->root = debugfs_create_dir("venus", NULL);
>> +	if (IS_ERR(core->root))
>> +		return IS_ERR(core->root);
> 
> You really do not care, and obviously did not test this on a system with
> CONFIG_DEBUG_FS disabled :)

Probably not :(

> 
> Just make the call to debugfs, and move on, feed it into other debugfs
> calls, all is good.
> 
> This function should just return 'void', no need to care about this at
> all.
> 
>> +	ret = venus_sys_set_debug(hdev, venus_fw_debug);
>> +	if (ret)
>> +		dev_warn(dev, "setting fw debug msg ON failed (%d)\n", ret);
> 
> Why do you care about this "error"?

I don't care so much, that's why it is dev_warn. But if I enable debug
messages from the firmware and I don't see them this warn will give me
an idea why.

-- 
regards,
Stan