From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
To: Cezary Rojewski <cezary.rojewski@intel.com>
Cc: alsa-devel@alsa-project.org, linux-kernel@vger.kernel.org,
tiwai@suse.de, broonie@kernel.org, vkoul@kernel.org,
gregkh@linuxfoundation.org, jank@cadence.com,
srinivas.kandagatla@linaro.org, slawomir.blauciak@intel.com,
Sanyog Kale <sanyog.r.kale@intel.com>
Subject: Re: [alsa-devel] [RFC PATCH 01/40] soundwire: add debugfs support
Date: Fri, 26 Jul 2019 08:57:29 -0500 [thread overview]
Message-ID: <1e0c8b9c-8e02-2735-df2d-7f82bdbb74a8@linux.intel.com> (raw)
In-Reply-To: <35a3936d-12d5-d301-2c8e-9e90163dd86e@intel.com>
Thanks for the feedback Cezary.
>> +static ssize_t sdw_slave_reg_read(struct file *file, char __user
>> *user_buf,
>> + size_t count, loff_t *ppos)
>> +{
>> + struct sdw_slave *slave = file->private_data;
>> + unsigned int reg;
>> + char *buf;
>> + ssize_t ret;
>> + int i, j;
>> +
>> + buf = kzalloc(RD_BUF, GFP_KERNEL);
>> + if (!buf)
>> + return -ENOMEM;
>> +
>> + ret = scnprintf(buf, RD_BUF, "Register Value\n");
>> + ret += scnprintf(buf + ret, RD_BUF - ret, "\nDP0\n");
>> +
>> + for (i = 0; i < 6; i++)
>> + ret += sdw_sprintf(slave, buf, ret, i);
>
> In most cases explicit reg macro is used, here it's implicit. Align with
> the rest?
I don't see what you are referring to, or I need more coffee.
we use this function sdw_printf in a number of places. Or are you
referring to the magic value 6? That should indeed be a macro.
>> +
>> + ret += scnprintf(buf + ret, RD_BUF - ret, "Bank0\n");
>> + ret += sdw_sprintf(slave, buf, ret, SDW_DP0_CHANNELEN);
>> + for (i = SDW_DP0_SAMPLECTRL1; i <= SDW_DP0_LANECTRL; i++)
>> + ret += sdw_sprintf(slave, buf, ret, i);
>> +
>> + ret += scnprintf(buf + ret, RD_BUF - ret, "Bank1\n");
>> + ret += sdw_sprintf(slave, buf, ret,
>> + SDW_DP0_CHANNELEN + SDW_BANK1_OFFSET);
>> + for (i = SDW_DP0_SAMPLECTRL1 + SDW_BANK1_OFFSET;
>> + i <= SDW_DP0_LANECTRL + SDW_BANK1_OFFSET; i++)
>> + ret += sdw_sprintf(slave, buf, ret, i);
>
> I'd advice to revisit macros declarations first.
> There should be SDW_DP0_SAMPLECTRL1_B(bank) declared. In general all
> macros for SDW should be "bank-less" (name wise). Additionally,
> SDW_BANK_OFFSET(bank) could be provided for convenience i.e.: return 0
> for bank0.
> Yeah, there might be some speed loss in terms of operation count but in
> most cases it is negligible.
>
> Would simplify this entire reg dump greatly.
> const array on top with {0, 1} elements and replacing explicit "bank0/1"
> strings with "bank%d" gets code size reduced while not losing on
> readability.
This could require a lot of changes in other parts of the code, and I
don't want to do this just for debugfs. It's valid point that maybe the
code can be simplified, but the changes are an across-the-board change
to be done when we don't add new functionality. I'll keep this on the
todo list.
>
>> +
>> + ret += scnprintf(buf + ret, RD_BUF - ret, "\nSCP\n");
>> + for (i = SDW_SCP_INT1; i <= SDW_SCP_BANKDELAY; i++)
>> + ret += sdw_sprintf(slave, buf, ret, i);
>> + for (i = SDW_SCP_DEVID_0; i <= SDW_SCP_DEVID_5; i++)
>> + ret += sdw_sprintf(slave, buf, ret, i);
>> +
>> + ret += scnprintf(buf + ret, RD_BUF - ret, "Bank0\n");
>> + ret += sdw_sprintf(slave, buf, ret, SDW_SCP_FRAMECTRL_B0);
>> + ret += sdw_sprintf(slave, buf, ret, SDW_SCP_NEXTFRAME_B0);
>> +
>> + ret += scnprintf(buf + ret, RD_BUF - ret, "Bank1\n");
>> + ret += sdw_sprintf(slave, buf, ret, SDW_SCP_FRAMECTRL_B1);
>> + ret += sdw_sprintf(slave, buf, ret, SDW_SCP_NEXTFRAME_B1);
>> +
>> + for (i = 1; i < 14; i++) {
>
> Explicit valid slave addresses would be preferred.
no, these are ports. we should use a macro instead of the magic 14 but
it's fine to try and read all ports. As I explained it's a good way to
figure out how many ports the Slave device supports even in the absence
of documentation. This also helps figure out if the DisCo properties
make sense.
>
>> + ret += scnprintf(buf + ret, RD_BUF - ret, "\nDP%d\n", i);
>> + reg = SDW_DPN_INT(i);
>> + for (j = 0; j < 6; j++)
>> + ret += sdw_sprintf(slave, buf, ret, reg + j);
>> +
>> + ret += scnprintf(buf + ret, RD_BUF - ret, "Bank0\n");
>> + reg = SDW_DPN_CHANNELEN_B0(i);
>> + for (j = 0; j < 9; j++)
>> + ret += sdw_sprintf(slave, buf, ret, reg + j);
>> +
>> + ret += scnprintf(buf + ret, RD_BUF - ret, "Bank1\n");
>> + reg = SDW_DPN_CHANNELEN_B1(i);
>> + for (j = 0; j < 9; j++)
>> + ret += sdw_sprintf(slave, buf, ret, reg + j);
>
> Some sort of MAX_CHANNELS would be nice here too.
Yes, need to use macros indeed.
>> +struct dentry *sdw_slave_debugfs_init(struct sdw_slave *slave)
>> +{
>> + struct dentry *master;
>> + struct dentry *d;
>> + char name[32];
>> +
>> + master = slave->bus->debugfs;
>> +
>> + /* create the debugfs slave-name */
>> + snprintf(name, sizeof(name), "%s", dev_name(&slave->dev));
>> + d = debugfs_create_dir(name, master);
>> +
>> + debugfs_create_file("registers", 0400, d, slave,
>> &sdw_slave_reg_fops);
>
> Pointer returned by _create_file gets completely ignored here. At least
> dbg msg would be nice if it fails.
>
>> + return d;
I understood that Greg KH doesn't want us to depend on the result of
debugfs calls, but a dev_dbg is likely ok.
next prev parent reply other threads:[~2019-07-26 13:57 UTC|newest]
Thread overview: 183+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-07-25 23:39 [RFC PATCH 00/40] soundwire: updates for 5.4 Pierre-Louis Bossart
2019-07-25 23:39 ` [RFC PATCH 01/40] soundwire: add debugfs support Pierre-Louis Bossart
2019-07-25 22:15 ` [alsa-devel] " Guennadi Liakhovetski
2019-07-26 13:43 ` Pierre-Louis Bossart
2019-07-26 9:22 ` Cezary Rojewski
2019-07-26 13:57 ` Pierre-Louis Bossart [this message]
2019-07-26 14:04 ` Greg KH
2019-07-26 15:29 ` Pierre-Louis Bossart
2019-07-25 23:39 ` [RFC PATCH 02/40] soundwire: cadence_master: add debugfs register dump Pierre-Louis Bossart
2019-07-26 14:09 ` Greg KH
2019-07-26 15:32 ` Pierre-Louis Bossart
2019-08-05 7:55 ` Sanyog Kale
2019-08-05 15:20 ` [alsa-devel] " Pierre-Louis Bossart
2019-07-25 23:39 ` [RFC PATCH 03/40] soundwire: cadence_master: align debugfs to 8 digits Pierre-Louis Bossart
2019-07-26 9:38 ` Cezary Rojewski
2019-07-26 13:58 ` [alsa-devel] " Pierre-Louis Bossart
2019-08-02 11:50 ` Vinod Koul
2019-07-25 23:39 ` [RFC PATCH 04/40] soundwire: intel: add debugfs register dump Pierre-Louis Bossart
2019-07-26 9:35 ` Cezary Rojewski
2019-07-26 14:00 ` [alsa-devel] " Pierre-Louis Bossart
2019-07-26 14:11 ` Greg KH
2019-07-26 14:06 ` Greg KH
2019-07-26 14:09 ` Greg KH
2019-07-26 15:34 ` Pierre-Louis Bossart
2019-07-25 23:39 ` [RFC PATCH 05/40] soundwire: intel: move interrupt enable after interrupt handler registration Pierre-Louis Bossart
2019-08-02 11:53 ` Vinod Koul
2019-08-02 15:08 ` Pierre-Louis Bossart
2019-07-25 23:39 ` [RFC PATCH 06/40] soundwire: intel: prevent possible dereference in hw_params Pierre-Louis Bossart
2019-07-26 9:45 ` Cezary Rojewski
2019-07-26 14:02 ` [alsa-devel] " Pierre-Louis Bossart
2019-08-02 11:55 ` Vinod Koul
2019-08-02 15:16 ` [alsa-devel] " Pierre-Louis Bossart
2019-08-02 15:57 ` Vinod Koul
2019-08-02 16:52 ` Pierre-Louis Bossart
2019-08-02 17:37 ` Vinod Koul
2019-07-25 23:39 ` [RFC PATCH 07/40] soundwire: intel: fix channel number reported by hardware Pierre-Louis Bossart
2019-08-02 11:57 ` Vinod Koul
2019-08-02 15:18 ` Pierre-Louis Bossart
2019-07-25 23:40 ` [RFC PATCH 08/40] soundwire: intel: remove BIOS work-arounds Pierre-Louis Bossart
2019-08-02 11:58 ` Vinod Koul
2019-07-25 23:40 ` [RFC PATCH 09/40] soundwire: cadence_master: fix usage of CONFIG_UPDATE Pierre-Louis Bossart
2019-07-25 22:23 ` [alsa-devel] " Guennadi Liakhovetski
2019-07-26 14:05 ` Pierre-Louis Bossart
2019-07-26 2:11 ` Bard liao
2019-07-26 13:33 ` Pierre-Louis Bossart
2019-07-26 9:53 ` Cezary Rojewski
2019-07-26 9:54 ` Cezary Rojewski
2019-08-02 12:03 ` Vinod Koul
2019-08-02 15:18 ` Pierre-Louis Bossart
2019-08-05 8:51 ` Sanyog Kale
2019-07-25 23:40 ` [RFC PATCH 10/40] soundwire: cadence_master: remove useless wrapper Pierre-Louis Bossart
2019-07-25 23:40 ` [RFC PATCH 11/40] soundwire: cadence_master: simplify bus clash interrupt clear Pierre-Louis Bossart
2019-08-02 12:07 ` Vinod Koul
2019-07-25 23:40 ` [RFC PATCH 12/40] soundwire: cadence_master: revisit interrupt settings Pierre-Louis Bossart
2019-08-02 12:10 ` Vinod Koul
2019-08-02 15:23 ` Pierre-Louis Bossart
2019-07-25 23:40 ` [RFC PATCH 13/40] soundwire: cadence_master: fix register definition for SLAVE_STATE Pierre-Louis Bossart
2019-08-02 12:12 ` Vinod Koul
2019-07-25 23:40 ` [RFC PATCH 14/40] soundwire: cadence_master: fix definitions for INTSTAT0/1 Pierre-Louis Bossart
2019-08-02 12:13 ` Vinod Koul
2019-07-25 23:40 ` [RFC PATCH 15/40] soundwire: cadence_master: handle multiple status reports per Slave Pierre-Louis Bossart
2019-07-25 22:31 ` [alsa-devel] " Guennadi Liakhovetski
2019-07-26 14:09 ` Pierre-Louis Bossart
2019-08-02 12:20 ` Vinod Koul
2019-08-02 15:29 ` Pierre-Louis Bossart
2019-08-02 16:01 ` Vinod Koul
2019-08-02 16:41 ` Pierre-Louis Bossart
2019-07-25 23:40 ` [RFC PATCH 16/40] soundwire: cadence_master: improve startup sequence with link hw_reset Pierre-Louis Bossart
2019-07-26 7:22 ` [alsa-devel] " Guennadi Liakhovetski
2019-07-26 14:11 ` Pierre-Louis Bossart
2019-07-25 23:40 ` [RFC PATCH 17/40] soundwire: bus: use runtime_pm_get_sync/pm when enabled Pierre-Louis Bossart
2019-07-26 7:39 ` [alsa-devel] " Guennadi Liakhovetski
2019-07-26 7:47 ` Jan Kotas
2019-07-26 8:22 ` Guennadi Liakhovetski
2019-07-26 8:33 ` Jan Kotas
2019-07-26 8:42 ` Guennadi Liakhovetski
2019-07-26 18:08 ` Pierre-Louis Bossart
2019-07-26 18:25 ` [alsa-devel] " Guennadi Liakhovetski
2019-07-26 19:11 ` Andy Shevchenko
2019-07-26 19:08 ` Andy Shevchenko
2019-07-29 22:07 ` [alsa-devel] " Pierre-Louis Bossart
2019-07-30 11:21 ` Andy Shevchenko
2019-07-30 12:57 ` Pierre-Louis Bossart
2019-07-30 15:58 ` Andy Shevchenko
2019-07-30 15:59 ` Andy Shevchenko
2019-08-02 16:58 ` Vinod Koul
2019-08-02 17:20 ` Pierre-Louis Bossart
2019-07-25 23:40 ` [RFC PATCH 18/40] soundwire: bus: split handling of Device0 events Pierre-Louis Bossart
2019-08-02 16:59 ` Vinod Koul
2019-07-25 23:40 ` [RFC PATCH 19/40] soundwire: bus: improve dynamic debug comments for enumeration Pierre-Louis Bossart
2019-08-02 17:00 ` Vinod Koul
2019-08-02 17:20 ` Pierre-Louis Bossart
2019-07-25 23:40 ` [RFC PATCH 20/40] soundwire: prototypes for suspend/resume Pierre-Louis Bossart
2019-07-26 10:04 ` Cezary Rojewski
2019-07-26 14:15 ` [alsa-devel] " Pierre-Louis Bossart
2019-08-02 17:03 ` Vinod Koul
2019-08-02 17:21 ` Pierre-Louis Bossart
2019-07-25 23:40 ` [RFC PATCH 21/40] soundwire: export helpers to find row and column values Pierre-Louis Bossart
2019-07-26 14:43 ` [alsa-devel] " Guennadi Liakhovetski
2019-07-26 15:26 ` Pierre-Louis Bossart
2019-08-02 17:04 ` Vinod Koul
2019-08-05 9:39 ` Sanyog Kale
2019-08-05 15:27 ` [alsa-devel] " Pierre-Louis Bossart
2019-07-25 23:40 ` [RFC PATCH 22/40] soundwire: include mod_devicetable.h to avoid compiling warnings Pierre-Louis Bossart
2019-07-25 23:40 ` [RFC PATCH 23/40] soundwire: stream: fix disable sequence Pierre-Louis Bossart
2019-07-26 10:14 ` Cezary Rojewski
2019-07-26 14:17 ` [alsa-devel] " Pierre-Louis Bossart
2019-07-26 14:51 ` Guennadi Liakhovetski
2019-07-26 15:05 ` Pierre-Louis Bossart
2019-08-05 9:56 ` Sanyog Kale
2019-08-05 15:33 ` [alsa-devel] " Pierre-Louis Bossart
2019-08-05 16:32 ` Sanyog Kale
2019-08-05 19:12 ` Pierre-Louis Bossart
2019-07-25 23:40 ` [RFC PATCH 24/40] soundwire: cadence_master: use BIOS defaults for frame shape Pierre-Louis Bossart
2019-07-26 10:20 ` Cezary Rojewski
2019-07-26 14:22 ` [alsa-devel] " Pierre-Louis Bossart
2019-08-02 17:10 ` Vinod Koul
2019-08-02 17:24 ` Pierre-Louis Bossart
2019-08-05 10:01 ` Sanyog Kale
2019-07-25 23:40 ` [RFC PATCH 25/40] soundwire: intel: use BIOS information to set clock dividers Pierre-Louis Bossart
2019-08-02 17:17 ` Vinod Koul
2019-08-02 17:29 ` Pierre-Louis Bossart
2019-08-05 10:28 ` Sanyog Kale
2019-08-05 15:40 ` [alsa-devel] " Pierre-Louis Bossart
2019-07-25 23:40 ` [RFC PATCH 26/40] soundwire: cadence_master: fix divider setting in clock register Pierre-Louis Bossart
2019-07-26 5:19 ` [alsa-devel] " Bard liao
2019-07-26 5:56 ` rander.wang
2019-07-26 14:24 ` Pierre-Louis Bossart
2019-08-02 17:19 ` Vinod Koul
2019-08-02 17:30 ` Pierre-Louis Bossart
2019-08-05 10:40 ` Sanyog Kale
2019-08-05 15:41 ` [alsa-devel] " Pierre-Louis Bossart
2019-07-25 23:40 ` [RFC PATCH 27/40] soundwire: Add Intel resource management algorithm Pierre-Louis Bossart
2019-07-26 11:07 ` Cezary Rojewski
2019-07-26 14:41 ` [alsa-devel] " Pierre-Louis Bossart
2019-07-26 15:43 ` Guennadi Liakhovetski
2019-07-26 17:55 ` Pierre-Louis Bossart
2019-08-05 16:54 ` Sanyog Kale
2019-08-05 19:08 ` [alsa-devel] " Pierre-Louis Bossart
2019-07-25 23:40 ` [RFC PATCH 28/40] soundwire: intel: handle disabled links Pierre-Louis Bossart
2019-08-05 16:57 ` Sanyog Kale
2019-08-05 19:18 ` [alsa-devel] " Pierre-Louis Bossart
2019-07-25 23:40 ` [RFC PATCH 29/40] soundwire: intel_init: add kernel module parameter to filter out links Pierre-Louis Bossart
2019-07-26 10:30 ` Cezary Rojewski
2019-07-26 14:43 ` [alsa-devel] " Pierre-Louis Bossart
2019-07-25 23:40 ` [RFC PATCH 30/40] soundwire: cadence_master: add kernel parameter to override interrupt mask Pierre-Louis Bossart
2019-07-25 23:40 ` [RFC PATCH 31/40] soundwire: intel: move shutdown() callback and don't export symbol Pierre-Louis Bossart
2019-07-26 10:38 ` Cezary Rojewski
2019-07-26 14:46 ` [alsa-devel] " Pierre-Louis Bossart
2019-08-02 17:28 ` Vinod Koul
2019-08-02 17:42 ` Pierre-Louis Bossart
2019-08-14 19:31 ` Pierre-Louis Bossart
2019-08-23 7:34 ` Vinod Koul
2019-08-23 15:57 ` Pierre-Louis Bossart
2019-07-25 23:40 ` [RFC PATCH 32/40] soundwire: intel: add helper for initialization Pierre-Louis Bossart
2019-07-26 10:42 ` Cezary Rojewski
2019-07-26 14:55 ` [alsa-devel] " Pierre-Louis Bossart
2019-07-25 23:40 ` [RFC PATCH 33/40] soundwire: intel: Add basic power management support Pierre-Louis Bossart
2019-07-26 10:50 ` Cezary Rojewski
2019-07-26 14:57 ` [alsa-devel] " Pierre-Louis Bossart
2019-07-25 23:40 ` [RFC PATCH 34/40] soundwire: intel: ignore disabled links for suspend/resume Pierre-Louis Bossart
2019-08-02 17:30 ` Vinod Koul
2019-08-02 17:44 ` Pierre-Louis Bossart
2019-07-25 23:40 ` [RFC PATCH 35/40] soundwire: intel: export helper to exit reset Pierre-Louis Bossart
2019-07-26 15:52 ` [alsa-devel] " Guennadi Liakhovetski
2019-07-26 17:22 ` Pierre-Louis Bossart
2019-08-02 17:31 ` Vinod Koul
2019-08-02 17:44 ` Pierre-Louis Bossart
2019-07-25 23:40 ` [RFC PATCH 36/40] soundwire: intel: disable interrupts on suspend Pierre-Louis Bossart
2019-07-26 15:55 ` [alsa-devel] " Guennadi Liakhovetski
2019-07-26 17:26 ` Pierre-Louis Bossart
2019-07-25 23:40 ` [RFC PATCH 37/40] soundwire: cadence_master: add hw_reset capability in debugfs Pierre-Louis Bossart
2019-07-26 14:07 ` Greg KH
2019-07-26 15:01 ` [alsa-devel] " Pierre-Louis Bossart
2019-07-26 15:57 ` Guennadi Liakhovetski
2019-07-26 17:31 ` Pierre-Louis Bossart
2019-07-25 23:40 ` [RFC PATCH 38/40] soundwire: cadence_master: make clock stop exit configurable on init Pierre-Louis Bossart
2019-07-26 16:02 ` [alsa-devel] " Guennadi Liakhovetski
2019-07-26 17:35 ` Pierre-Louis Bossart
2019-07-25 23:40 ` [RFC PATCH 39/40] soundwire: intel: add pm_runtime support Pierre-Louis Bossart
2019-07-25 23:40 ` [RFC PATCH 40/40] soundwire: intel: add delay on restart for enumeration Pierre-Louis Bossart
2019-07-26 11:14 ` [RFC PATCH 00/40] soundwire: updates for 5.4 Cezary Rojewski
2019-07-26 15:23 ` [alsa-devel] " Pierre-Louis Bossart
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1e0c8b9c-8e02-2735-df2d-7f82bdbb74a8@linux.intel.com \
--to=pierre-louis.bossart@linux.intel.com \
--cc=alsa-devel@alsa-project.org \
--cc=broonie@kernel.org \
--cc=cezary.rojewski@intel.com \
--cc=gregkh@linuxfoundation.org \
--cc=jank@cadence.com \
--cc=linux-kernel@vger.kernel.org \
--cc=sanyog.r.kale@intel.com \
--cc=slawomir.blauciak@intel.com \
--cc=srinivas.kandagatla@linaro.org \
--cc=tiwai@suse.de \
--cc=vkoul@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).