From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
To: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>,
vkoul@kernel.org, broonie@kernel.org
Cc: bgoswami@codeaurora.org, plai@codeaurora.org, robh+dt@kernel.org,
devicetree@vger.kernel.org, lgirdwood@gmail.com,
alsa-devel@alsa-project.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 2/4] soundwire: core: add device tree support for slave devices
Date: Thu, 8 Aug 2019 16:17:32 +0100 [thread overview]
Message-ID: <564f5fa4-59ec-b4e5-a7a5-29dee99039b3@linaro.org> (raw)
In-Reply-To: <42ca4170-0fa0-6951-f568-89a05c095d5a@linux.intel.com>
Thanks for taking time to review.
On 08/08/2019 16:00, Pierre-Louis Bossart wrote:
>
>> @@ -35,6 +36,7 @@ static int sdw_slave_add(struct sdw_bus *bus,
>> slave->dev.release = sdw_slave_release;
>> slave->dev.bus = &sdw_bus_type;
>> + slave->dev.of_node = of_node_get(to_of_node(fwnode));
>
> shouldn't this protected by
> #if IS_ENABLED(CONFIG_OF) ?
>
These macros and functions have dummy entries, so it should not be an issue.
I did build soundwire with i386_defconfig with no issues.
>> slave->bus = bus;
>> slave->status = SDW_SLAVE_UNATTACHED;
>> slave->dev_num = 0;
>> @@ -112,3 +114,48 @@ int sdw_acpi_find_slaves(struct sdw_bus *bus)
>> }
>> #endif
>> +
>> +/*
>> + * sdw_of_find_slaves() - Find Slave devices in master device tree node
>> + * @bus: SDW bus instance
>> + *
>> + * Scans Master DT node for SDW child Slave devices and registers it.
>> + */
>> +int sdw_of_find_slaves(struct sdw_bus *bus)
>> +{
>> + struct device *dev = bus->dev;
>> + struct device_node *node;
>> +
>> + for_each_child_of_node(bus->dev->of_node, node) {
>> + struct sdw_slave_id id;
>> + const char *compat = NULL;
>> + int unique_id, ret;
>> + int ver, mfg_id, part_id, class_id;
>> +
>> + compat = of_get_property(node, "compatible", NULL);
>> + if (!compat)
>> + continue;
>> +
>> + ret = sscanf(compat, "sdw%x,%x,%x,%x",
>> + &ver, &mfg_id, &part_id, &class_id);
>> + if (ret != 4) {
>> + dev_err(dev, "Manf ID & Product code not found %s\n",
>> + compat);
>> + continue;
>> + }
>> +
>> + ret = of_property_read_u32(node, "sdw-instance-id", &unique_id);
>> + if (ret) {
>> + dev_err(dev, "Instance id not found:%d\n", ret);
>> + continue;
>
> I am confused here.
> If you have two identical devices on the same link, isn't this property
> required and that should be a real error instead of a continue?
Yes, I agree it will be mandatory in such cases.
Am okay either way, I dont mind changing it to returning EINVAL in all
the cases.
>
>> + }
>> +
>> + id.sdw_version = ver - 0xF;
>
> maybe a comment in the code would help to make the encoding
> self-explanatory, as you did in the DT bindings
>
> Version number '0x10' represents SoundWire 1.0
> Version number '0x11' represents SoundWire 1.1
Makes sense, will fix this in next version.
This info is also available in bindings.
--srini
>
>> + id.unique_id = unique_id;
>> + id.mfg_id = mfg_id;
>> + id.part_id = part_id;
>> + id.class_id = class_id;
>> + sdw_slave_add(bus, &id, of_fwnode_handle(node));
>> + }
>> + return 0;
>> +}
>>
next prev parent reply other threads:[~2019-08-08 15:17 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-08-08 14:45 [PATCH v2 0/4] ASoC: codecs: Add WSA881x Smart Speaker amplifier support Srinivas Kandagatla
2019-08-08 14:45 ` [PATCH v2 1/4] dt-bindings: soundwire: add slave bindings Srinivas Kandagatla
2019-08-08 15:58 ` Pierre-Louis Bossart
2019-08-08 16:48 ` Srinivas Kandagatla
2019-08-08 19:52 ` Mark Brown
2019-08-09 4:54 ` Vinod Koul
2019-08-09 8:25 ` Srinivas Kandagatla
2019-08-09 5:00 ` Vinod Koul
2019-08-08 14:45 ` [PATCH v2 2/4] soundwire: core: add device tree support for slave devices Srinivas Kandagatla
2019-08-08 15:00 ` Pierre-Louis Bossart
2019-08-08 15:17 ` Srinivas Kandagatla [this message]
2019-08-09 5:46 ` Vinod Koul
2019-08-09 8:24 ` Srinivas Kandagatla
2019-08-09 5:07 ` Vinod Koul
2019-08-09 8:24 ` Srinivas Kandagatla
2019-08-08 14:45 ` [PATCH v2 3/4] dt-bindings: ASoC: Add WSA881x bindings Srinivas Kandagatla
2019-08-08 14:45 ` [PATCH v2 4/4] ASoC: codecs: add wsa881x amplifier support Srinivas Kandagatla
2019-08-08 15:18 ` [alsa-devel] " Pierre-Louis Bossart
2019-08-08 16:20 ` Srinivas Kandagatla
2019-08-08 16:29 ` Pierre-Louis Bossart
2019-08-09 4:56 ` [PATCH v2 0/4] ASoC: codecs: Add WSA881x Smart Speaker " Vinod Koul
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=564f5fa4-59ec-b4e5-a7a5-29dee99039b3@linaro.org \
--to=srinivas.kandagatla@linaro.org \
--cc=alsa-devel@alsa-project.org \
--cc=bgoswami@codeaurora.org \
--cc=broonie@kernel.org \
--cc=devicetree@vger.kernel.org \
--cc=lgirdwood@gmail.com \
--cc=linux-kernel@vger.kernel.org \
--cc=pierre-louis.bossart@linux.intel.com \
--cc=plai@codeaurora.org \
--cc=robh+dt@kernel.org \
--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).