From: Vinod Koul <vinod.koul@intel.com> To: Takashi Iwai <tiwai@suse.de> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>, ALSA <alsa-devel@alsa-project.org>, Charles Keepax <ckeepax@opensource.cirrus.com>, Sudheer Papothi <spapothi@codeaurora.org>, plai@codeaurora.org, LKML <linux-kernel@vger.kernel.org>, Pierre <pierre-louis.bossart@linux.intel.com>, patches.audio@intel.com, Mark <broonie@kernel.org>, srinivas.kandagatla@linaro.org, Shreyas NC <shreyas.nc@intel.com>, Sanyog Kale <sanyog.r.kale@intel.com>, Sagar Dharia <sdharia@codeaurora.org>, alan@linux.intel.com Subject: Re: [alsa-devel] [PATCH 03/14] soundwire: Add Master registration Date: Fri, 20 Oct 2017 10:49:59 +0530 [thread overview] Message-ID: <20171020051959.GH30097@localhost> (raw) In-Reply-To: <s5hh8uvtt45.wl-tiwai@suse.de> On Thu, Oct 19, 2017 at 10:54:50AM +0200, Takashi Iwai wrote: > On Thu, 19 Oct 2017 05:03:19 +0200, > Vinod Koul wrote: > > +int sdw_add_bus_master(struct sdw_bus *bus) > > +{ > > + int ret; > > + > > + if (!bus->dev) { > > + pr_err("SoundWire bus has no device"); > > + return -ENODEV; > > + } > > + > > + mutex_init(&bus->bus_lock); > > + INIT_LIST_HEAD(&bus->slaves); > > + > > + /* > > + * SDW is an enumerable bus, but devices can be powered off. So, > > + * they won't be able to report as present. > > + * > > + * Create Slave devices based on Slaves described in > > + * the respective firmware (ACPI/DT) > > + */ > > + > > + if (IS_ENABLED(CONFIG_ACPI) && bus->dev && ACPI_HANDLE(bus->dev)) > > + ret = sdw_acpi_find_slaves(bus); > > + else if (IS_ENABLED(CONFIG_OF) && bus->dev && bus->dev->of_node) > > The bus->dev NULL check is already done at the beginning of the > function, so here are superfluous. right > > +static int sdw_delete_slave(struct device *dev, void *data) > > +{ > > + struct sdw_slave *slave = dev_to_sdw_dev(dev); > > + struct sdw_bus *bus = slave->bus; > > + > > + mutex_lock(&bus->bus_lock); > > + if (!list_empty(&bus->slaves)) > > + list_del(&slave->node); > > You can perform list_del_init() without empty check. Better :) > > > +void sdw_extract_slave_id(struct sdw_bus *bus, > > + unsigned long long addr, struct sdw_slave_id *id) > > Use u64 instead. okay > > +{ > > + dev_dbg(bus->dev, "SDW Slave Addr: %llx", addr); > > + > > + /* > > + * Spec definition > > + * Register Bit Contents > > + * DevId_0 [7:4] 47:44 sdw_version > > + * DevId_0 [3:0] 43:40 unique_id > > + * DevId_1 39:32 mfg_id [15:8] > > + * DevId_2 31:24 mfg_id [7:0] > > + * DevId_3 23:16 part_id [15:8] > > + * DevId_4 15:08 part_id [7:0] > > + * DevId_5 07:00 class_id > > + */ > > + id->sdw_version = (addr >> 44) & GENMASK(3, 0); > > + id->unique_id = (addr >> 40) & GENMASK(3, 0); > > + id->mfg_id = (addr >> 24) & GENMASK(15, 0); > > + id->part_id = (addr >> 8) & GENMASK(15, 0); > > + id->class_id = addr & GENMASK(7, 0); > > + > > + dev_info(bus->dev, > > + "SDW Slave class_id %x, part_id %x, mfg_id %x, unique_id %x, version %x", > > + id->class_id, id->part_id, id->mfg_id, > > + id->unique_id, id->sdw_version); > > + > > Do we want to print a message always at each invocation? Not really, lets make it debug > > +static int sdw_slave_add(struct sdw_bus *bus, > > + struct sdw_slave_id *id, struct fwnode_handle *fwnode) > > +{ > > + struct sdw_slave *slave; > > + char name[32]; > > + int ret; > > + > > + slave = kzalloc(sizeof(*slave), GFP_KERNEL); > > + if (!slave) > > + return -ENOMEM; > > + > > + /* Initialize data structure */ > > + memcpy(&slave->id, id, sizeof(*id)); > > + > > + /* name shall be sdw:link:mfg:part:class:unique */ > > + snprintf(name, sizeof(name), "sdw:%x:%x:%x:%x:%x", > > + bus->link_id, id->mfg_id, id->part_id, > > + id->class_id, id->unique_id); > > You can set the name directly via dev_set_name(). It's printf format, > after all. right, am using it but with this string :D > > + slave->dev.parent = bus->dev; > > + slave->dev.fwnode = fwnode; > > + dev_set_name(&slave->dev, "%s", name); > > + slave->dev.release = sdw_slave_release; > > + slave->dev.bus = &sdw_bus_type; > > + slave->bus = bus; > > + slave->status = SDW_SLAVE_UNATTACHED; > > + slave->dev_num = 0; > > + > > + mutex_lock(&bus->bus_lock); > > + list_add_tail(&slave->node, &bus->slaves); > > + mutex_unlock(&bus->bus_lock); > > + > > + ret = device_register(&slave->dev); > > + if (ret) { > > + dev_err(bus->dev, "Failed to add slave: ret %d\n", ret); > > + > > + /* > > + * On err, don't free but drop ref as this will be freed > > + * when release method is invoked. > > + */ > > + put_device(&slave->dev); > > Wouldn't it leave a stale link to bus? yes that needs to be removed too, thanks for pointing -- ~Vinod
WARNING: multiple messages have this Message-ID (diff)
From: Vinod Koul <vinod.koul@intel.com> To: Takashi Iwai <tiwai@suse.de> Cc: ALSA <alsa-devel@alsa-project.org>, Charles Keepax <ckeepax@opensource.cirrus.com>, patches.audio@intel.com, Greg Kroah-Hartman <gregkh@linuxfoundation.org>, plai@codeaurora.org, LKML <linux-kernel@vger.kernel.org>, Pierre <pierre-louis.bossart@linux.intel.com>, Sagar Dharia <sdharia@codeaurora.org>, Mark <broonie@kernel.org>, srinivas.kandagatla@linaro.org, Shreyas NC <shreyas.nc@intel.com>, Sanyog Kale <sanyog.r.kale@intel.com>, Sudheer Papothi <spapothi@codeaurora.org>, alan@linux.intel.com Subject: Re: [PATCH 03/14] soundwire: Add Master registration Date: Fri, 20 Oct 2017 10:49:59 +0530 [thread overview] Message-ID: <20171020051959.GH30097@localhost> (raw) In-Reply-To: <s5hh8uvtt45.wl-tiwai@suse.de> On Thu, Oct 19, 2017 at 10:54:50AM +0200, Takashi Iwai wrote: > On Thu, 19 Oct 2017 05:03:19 +0200, > Vinod Koul wrote: > > +int sdw_add_bus_master(struct sdw_bus *bus) > > +{ > > + int ret; > > + > > + if (!bus->dev) { > > + pr_err("SoundWire bus has no device"); > > + return -ENODEV; > > + } > > + > > + mutex_init(&bus->bus_lock); > > + INIT_LIST_HEAD(&bus->slaves); > > + > > + /* > > + * SDW is an enumerable bus, but devices can be powered off. So, > > + * they won't be able to report as present. > > + * > > + * Create Slave devices based on Slaves described in > > + * the respective firmware (ACPI/DT) > > + */ > > + > > + if (IS_ENABLED(CONFIG_ACPI) && bus->dev && ACPI_HANDLE(bus->dev)) > > + ret = sdw_acpi_find_slaves(bus); > > + else if (IS_ENABLED(CONFIG_OF) && bus->dev && bus->dev->of_node) > > The bus->dev NULL check is already done at the beginning of the > function, so here are superfluous. right > > +static int sdw_delete_slave(struct device *dev, void *data) > > +{ > > + struct sdw_slave *slave = dev_to_sdw_dev(dev); > > + struct sdw_bus *bus = slave->bus; > > + > > + mutex_lock(&bus->bus_lock); > > + if (!list_empty(&bus->slaves)) > > + list_del(&slave->node); > > You can perform list_del_init() without empty check. Better :) > > > +void sdw_extract_slave_id(struct sdw_bus *bus, > > + unsigned long long addr, struct sdw_slave_id *id) > > Use u64 instead. okay > > +{ > > + dev_dbg(bus->dev, "SDW Slave Addr: %llx", addr); > > + > > + /* > > + * Spec definition > > + * Register Bit Contents > > + * DevId_0 [7:4] 47:44 sdw_version > > + * DevId_0 [3:0] 43:40 unique_id > > + * DevId_1 39:32 mfg_id [15:8] > > + * DevId_2 31:24 mfg_id [7:0] > > + * DevId_3 23:16 part_id [15:8] > > + * DevId_4 15:08 part_id [7:0] > > + * DevId_5 07:00 class_id > > + */ > > + id->sdw_version = (addr >> 44) & GENMASK(3, 0); > > + id->unique_id = (addr >> 40) & GENMASK(3, 0); > > + id->mfg_id = (addr >> 24) & GENMASK(15, 0); > > + id->part_id = (addr >> 8) & GENMASK(15, 0); > > + id->class_id = addr & GENMASK(7, 0); > > + > > + dev_info(bus->dev, > > + "SDW Slave class_id %x, part_id %x, mfg_id %x, unique_id %x, version %x", > > + id->class_id, id->part_id, id->mfg_id, > > + id->unique_id, id->sdw_version); > > + > > Do we want to print a message always at each invocation? Not really, lets make it debug > > +static int sdw_slave_add(struct sdw_bus *bus, > > + struct sdw_slave_id *id, struct fwnode_handle *fwnode) > > +{ > > + struct sdw_slave *slave; > > + char name[32]; > > + int ret; > > + > > + slave = kzalloc(sizeof(*slave), GFP_KERNEL); > > + if (!slave) > > + return -ENOMEM; > > + > > + /* Initialize data structure */ > > + memcpy(&slave->id, id, sizeof(*id)); > > + > > + /* name shall be sdw:link:mfg:part:class:unique */ > > + snprintf(name, sizeof(name), "sdw:%x:%x:%x:%x:%x", > > + bus->link_id, id->mfg_id, id->part_id, > > + id->class_id, id->unique_id); > > You can set the name directly via dev_set_name(). It's printf format, > after all. right, am using it but with this string :D > > + slave->dev.parent = bus->dev; > > + slave->dev.fwnode = fwnode; > > + dev_set_name(&slave->dev, "%s", name); > > + slave->dev.release = sdw_slave_release; > > + slave->dev.bus = &sdw_bus_type; > > + slave->bus = bus; > > + slave->status = SDW_SLAVE_UNATTACHED; > > + slave->dev_num = 0; > > + > > + mutex_lock(&bus->bus_lock); > > + list_add_tail(&slave->node, &bus->slaves); > > + mutex_unlock(&bus->bus_lock); > > + > > + ret = device_register(&slave->dev); > > + if (ret) { > > + dev_err(bus->dev, "Failed to add slave: ret %d\n", ret); > > + > > + /* > > + * On err, don't free but drop ref as this will be freed > > + * when release method is invoked. > > + */ > > + put_device(&slave->dev); > > Wouldn't it leave a stale link to bus? yes that needs to be removed too, thanks for pointing -- ~Vinod
next prev parent reply other threads:[~2017-10-20 5:15 UTC|newest] Thread overview: 139+ messages / expand[flat|nested] mbox.gz Atom feed top 2017-10-19 3:03 [PATCH 00/14] soundwire: Add a new SoundWire subsystem Vinod Koul 2017-10-19 3:03 ` Vinod Koul 2017-10-19 3:03 ` [PATCH 01/14] Documentation: Add SoundWire summary Vinod Koul 2017-10-19 3:33 ` Randy Dunlap 2017-10-19 3:33 ` Randy Dunlap 2017-10-19 4:44 ` Vinod Koul 2017-10-19 4:44 ` Vinod Koul 2017-10-20 10:39 ` Greg Kroah-Hartman 2017-10-20 10:39 ` Greg Kroah-Hartman 2017-10-20 15:49 ` Vinod Koul 2017-10-20 15:49 ` Vinod Koul 2017-10-20 16:22 ` Greg Kroah-Hartman 2017-10-20 16:22 ` Greg Kroah-Hartman 2017-10-20 17:09 ` Vinod Koul 2017-10-20 17:09 ` Vinod Koul 2017-10-21 8:57 ` Mark Brown 2017-10-21 8:57 ` Mark Brown 2017-10-21 11:28 ` Vinod Koul 2017-10-21 11:28 ` Vinod Koul 2017-10-22 10:06 ` [alsa-devel] " Pierre-Louis Bossart 2017-10-23 8:21 ` Mark Brown 2017-10-23 7:50 ` Mark Brown 2017-10-23 7:50 ` Mark Brown 2017-10-23 11:18 ` [alsa-devel] " Vinod Koul 2017-10-19 3:03 ` [PATCH 02/14] soundwire: Add SoundWire bus type Vinod Koul 2017-10-19 7:40 ` Takashi Iwai 2017-10-19 8:32 ` [alsa-devel] " Takashi Iwai 2017-10-19 8:32 ` Takashi Iwai 2017-10-20 5:11 ` Vinod Koul 2017-10-20 5:11 ` Vinod Koul 2017-10-20 6:59 ` Takashi Iwai 2017-10-20 15:46 ` Vinod Koul 2017-10-20 15:50 ` Takashi Iwai 2017-10-20 15:50 ` Takashi Iwai 2017-10-20 16:11 ` Vinod Koul 2017-10-20 10:41 ` Greg Kroah-Hartman 2017-10-20 10:41 ` Greg Kroah-Hartman 2017-10-20 15:52 ` Vinod Koul 2017-10-20 10:45 ` Greg Kroah-Hartman 2017-10-20 16:01 ` Vinod Koul 2017-10-20 16:21 ` Greg Kroah-Hartman 2017-10-20 16:21 ` Greg Kroah-Hartman 2017-10-20 17:10 ` Vinod Koul 2017-10-23 11:46 ` Alan Cox 2017-10-23 11:46 ` Alan Cox 2017-10-26 8:33 ` Vinod Koul 2017-10-26 8:33 ` Vinod Koul 2017-10-27 8:57 ` Greg Kroah-Hartman 2017-10-30 13:11 ` Vinod Koul 2017-10-20 16:03 ` Philippe Ombredanne 2017-10-20 16:20 ` Vinod Koul 2017-10-20 16:20 ` Vinod Koul 2017-10-20 16:27 ` Greg Kroah-Hartman 2017-10-20 17:13 ` Vinod Koul 2017-10-20 17:13 ` Vinod Koul 2017-10-23 11:52 ` Alan Cox 2017-10-23 11:52 ` Alan Cox 2017-10-21 9:03 ` Mark Brown 2017-10-21 11:29 ` Vinod Koul 2017-10-21 11:29 ` Vinod Koul 2017-11-09 21:14 ` Srinivas Kandagatla 2017-11-09 21:14 ` Srinivas Kandagatla 2017-11-10 4:59 ` Vinod Koul 2017-11-10 4:59 ` Vinod Koul 2017-11-10 8:55 ` Vinod Koul 2017-11-10 10:50 ` Srinivas Kandagatla 2017-11-10 10:42 ` Srinivas Kandagatla 2017-11-10 10:42 ` Srinivas Kandagatla 2017-11-10 10:58 ` Vinod Koul 2017-10-19 3:03 ` [PATCH 03/14] soundwire: Add Master registration Vinod Koul 2017-10-19 3:03 ` Vinod Koul 2017-10-19 8:54 ` [alsa-devel] " Takashi Iwai 2017-10-19 8:54 ` Takashi Iwai 2017-10-20 5:19 ` Vinod Koul [this message] 2017-10-20 5:19 ` Vinod Koul 2017-10-20 10:47 ` Greg Kroah-Hartman 2017-10-20 16:05 ` Vinod Koul 2017-10-21 9:12 ` Mark Brown 2017-10-21 9:12 ` Mark Brown 2017-10-21 11:35 ` Vinod Koul 2017-10-21 11:35 ` Vinod Koul 2017-10-23 8:24 ` Mark Brown 2017-10-23 8:24 ` Mark Brown 2017-10-23 11:19 ` Vinod Koul 2017-11-09 21:14 ` Srinivas Kandagatla 2017-11-10 5:02 ` Vinod Koul 2017-11-10 5:02 ` Vinod Koul 2017-10-19 3:03 ` [PATCH 04/14] soundwire: Add MIPI DisCo property helpers Vinod Koul 2017-10-19 9:02 ` [alsa-devel] " Takashi Iwai 2017-10-20 5:25 ` Vinod Koul 2017-10-21 9:20 ` Mark Brown 2017-10-21 9:20 ` Mark Brown 2017-10-21 11:37 ` Vinod Koul 2017-10-22 10:14 ` [alsa-devel] " Pierre-Louis Bossart 2017-10-22 10:14 ` Pierre-Louis Bossart 2017-10-19 3:03 ` [PATCH 05/14] soundwire: Add SoundWire MIPI defined registers Vinod Koul 2017-10-19 3:03 ` [PATCH 06/14] soundwire: Add IO transfer Vinod Koul 2017-10-19 3:03 ` Vinod Koul 2017-10-19 9:13 ` [alsa-devel] " Takashi Iwai 2017-10-20 5:30 ` Vinod Koul 2017-10-20 5:30 ` Vinod Koul 2017-10-20 7:06 ` [alsa-devel] " Takashi Iwai 2017-10-20 7:06 ` Takashi Iwai 2017-10-20 15:48 ` [alsa-devel] " Vinod Koul 2017-10-21 9:29 ` Mark Brown 2017-10-21 9:29 ` Mark Brown 2017-10-21 11:40 ` Vinod Koul 2017-10-19 3:03 ` [PATCH 07/14] regmap: Add SoundWire bus support Vinod Koul 2017-10-21 9:34 ` Mark Brown 2017-10-21 9:34 ` Mark Brown 2017-10-21 11:44 ` Vinod Koul 2017-10-23 11:56 ` Alan Cox 2017-10-23 13:16 ` Mark Brown 2017-10-19 3:03 ` [PATCH 08/14] soundwire: Add Slave status handling helpers Vinod Koul 2017-10-19 13:44 ` [alsa-devel] " Takashi Iwai 2017-10-31 13:04 ` Vinod Koul 2017-10-31 13:04 ` Vinod Koul 2017-10-31 21:19 ` [alsa-devel] " Pierre-Louis Bossart 2017-10-31 21:19 ` Pierre-Louis Bossart 2017-11-01 9:08 ` [alsa-devel] " Vinod Koul 2017-11-01 9:08 ` Vinod Koul 2017-11-01 21:10 ` [alsa-devel] " Pierre-Louis Bossart 2017-11-02 3:28 ` Vinod Koul 2017-10-19 3:03 ` [PATCH 09/14] soundwire: Add slave status handling Vinod Koul 2017-10-19 3:03 ` [PATCH 10/14] soundwire: Add sysfs for SoundWire DisCo properties Vinod Koul 2017-10-21 9:42 ` Mark Brown 2017-10-21 9:42 ` Mark Brown 2017-10-21 11:53 ` Vinod Koul 2017-10-21 11:53 ` Vinod Koul 2017-11-09 21:14 ` Srinivas Kandagatla 2017-11-10 4:52 ` Vinod Koul 2017-10-19 3:03 ` [PATCH 11/14] soundwire: cdns: Add cadence module Vinod Koul 2017-10-21 9:52 ` Mark Brown 2017-10-21 9:52 ` Mark Brown 2017-10-21 11:54 ` Vinod Koul 2017-10-21 11:54 ` Vinod Koul 2017-10-19 3:03 ` [PATCH 12/14] soundwire: cdns: Add sdw_master_ops and IO transfer support Vinod Koul 2017-10-19 3:03 ` [PATCH 13/14] soundwire: intel: Add Intel Master driver Vinod Koul 2017-10-19 3:03 ` [PATCH 14/14] soundwire: intel: Add Intel init module 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=20171020051959.GH30097@localhost \ --to=vinod.koul@intel.com \ --cc=alan@linux.intel.com \ --cc=alsa-devel@alsa-project.org \ --cc=broonie@kernel.org \ --cc=ckeepax@opensource.cirrus.com \ --cc=gregkh@linuxfoundation.org \ --cc=linux-kernel@vger.kernel.org \ --cc=patches.audio@intel.com \ --cc=pierre-louis.bossart@linux.intel.com \ --cc=plai@codeaurora.org \ --cc=sanyog.r.kale@intel.com \ --cc=sdharia@codeaurora.org \ --cc=shreyas.nc@intel.com \ --cc=spapothi@codeaurora.org \ --cc=srinivas.kandagatla@linaro.org \ --cc=tiwai@suse.de \ /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: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.