From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751616AbdJTFPq (ORCPT ); Fri, 20 Oct 2017 01:15:46 -0400 Received: from mga14.intel.com ([192.55.52.115]:13859 "EHLO mga14.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751351AbdJTFPm (ORCPT ); Fri, 20 Oct 2017 01:15:42 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.43,405,1503385200"; d="scan'208";a="911801132" Date: Fri, 20 Oct 2017 10:49:59 +0530 From: Vinod Koul To: Takashi Iwai 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 Message-ID: <20171020051959.GH30097@localhost> 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 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 From mboxrd@z Thu Jan 1 00:00:00 1970 From: Vinod Koul Subject: Re: [PATCH 03/14] soundwire: Add Master registration Date: Fri, 20 Oct 2017 10:49:59 +0530 Message-ID: <20171020051959.GH30097@localhost> 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 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by alsa0.perex.cz (Postfix) with ESMTP id 75AF9266A51 for ; Fri, 20 Oct 2017 07:15:43 +0200 (CEST) Content-Disposition: inline In-Reply-To: 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: Takashi Iwai 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, 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