From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752252AbdJSIyy (ORCPT ); Thu, 19 Oct 2017 04:54:54 -0400 Received: from mx2.suse.de ([195.135.220.15]:56779 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751921AbdJSIyw (ORCPT ); Thu, 19 Oct 2017 04:54:52 -0400 Date: Thu, 19 Oct 2017 10:54:50 +0200 Message-ID: From: Takashi Iwai To: Vinod Koul Cc: Greg Kroah-Hartman , ALSA , Charles Keepax , Sudheer Papothi , plai@codeaurora.org, LKML , Pierre , patches.audio@intel.com, Mark , srinivas.kandagatla@linaro.org, Shreyas NC , Sanyog Kale , Sagar Dharia , alan@linux.intel.com Subject: Re: [alsa-devel] [PATCH 03/14] soundwire: Add Master registration In-Reply-To: <1508382211-3154-4-git-send-email-vinod.koul@intel.com> References: <1508382211-3154-1-git-send-email-vinod.koul@intel.com> <1508382211-3154-4-git-send-email-vinod.koul@intel.com> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI/1.14.6 (Maruoka) FLIM/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL/10.8 Emacs/25.3 (x86_64-suse-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka") Content-Type: text/plain; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, 19 Oct 2017 05:03:19 +0200, Vinod Koul wrote: > > +/** > + * sdw_add_bus_master: add a bus Master instance > + * > + * @bus: bus instance > + * > + * Initializes the bus instance, read properties and create child > + * devices. > + */ > +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. > +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. > +void sdw_extract_slave_id(struct sdw_bus *bus, > + unsigned long long addr, struct sdw_slave_id *id) Use u64 instead. > +{ > + 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? > +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. > + 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? thanks, Takashi From mboxrd@z Thu Jan 1 00:00:00 1970 From: Takashi Iwai Subject: Re: [PATCH 03/14] soundwire: Add Master registration Date: Thu, 19 Oct 2017 10:54:50 +0200 Message-ID: References: <1508382211-3154-1-git-send-email-vinod.koul@intel.com> <1508382211-3154-4-git-send-email-vinod.koul@intel.com> Mime-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka") Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mx2.suse.de (mx2.suse.de [195.135.220.15]) by alsa0.perex.cz (Postfix) with ESMTP id B84962675B6 for ; Thu, 19 Oct 2017 10:54:51 +0200 (CEST) In-Reply-To: <1508382211-3154-4-git-send-email-vinod.koul@intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: alsa-devel-bounces@alsa-project.org Sender: alsa-devel-bounces@alsa-project.org To: Vinod Koul Cc: ALSA , Charles Keepax , patches.audio@intel.com, Greg Kroah-Hartman , plai@codeaurora.org, LKML , Pierre , Sagar Dharia , Mark , srinivas.kandagatla@linaro.org, Shreyas NC , Sanyog Kale , Sudheer Papothi , alan@linux.intel.com List-Id: alsa-devel@alsa-project.org On Thu, 19 Oct 2017 05:03:19 +0200, Vinod Koul wrote: > > +/** > + * sdw_add_bus_master: add a bus Master instance > + * > + * @bus: bus instance > + * > + * Initializes the bus instance, read properties and create child > + * devices. > + */ > +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. > +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. > +void sdw_extract_slave_id(struct sdw_bus *bus, > + unsigned long long addr, struct sdw_slave_id *id) Use u64 instead. > +{ > + 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? > +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. > + 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? thanks, Takashi