From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S935390AbdKPQ7M (ORCPT ); Thu, 16 Nov 2017 11:59:12 -0500 Received: from mga09.intel.com ([134.134.136.24]:45040 "EHLO mga09.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932459AbdKPQ7D (ORCPT ); Thu, 16 Nov 2017 11:59:03 -0500 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.44,404,1505804400"; d="scan'208";a="1244945442" Date: Thu, 16 Nov 2017 22:32:21 +0530 From: Vinod Koul To: Srinivas Kandagatla Cc: Greg Kroah-Hartman , LKML , ALSA , Mark , Takashi , Pierre , Sanyog Kale , Shreyas NC , patches.audio@intel.com, alan@linux.intel.com, Charles Keepax , Sagar Dharia , plai@codeaurora.org, Sudheer Papothi Subject: Re: [PATCH v2 02/14] soundwire: Add SoundWire bus type Message-ID: <20171116170221.GD3187@localhost> References: <1510314556-13002-1-git-send-email-vinod.koul@intel.com> <1510314556-13002-3-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, Nov 16, 2017 at 04:05:17PM +0000, Srinivas Kandagatla wrote: > > > On 10/11/17 11:49, Vinod Koul wrote: > >index 000000000000..9b3dca95a098 > >--- /dev/null > >+++ b/drivers/soundwire/bus.h > >+ > >+#ifndef __SDW_BUS_H > >+#define __SDW_BUS_H > >+ > >+#include > >+#include > >+#include > >+#include > >+#include > > Do you need all these headers as part of this patch? I think so. One thing we need to keep in mind that bus is compiled on different archs where we may not have things availble explictly as they are on x86 or others. I think couple of them complained last time. I will check once more though.. > > >+#include > >+ > >+int sdw_slave_modalias(const struct sdw_slave *slave, char *buf, size_t size); > >+ > >+#endif /* __SDW_BUS_H */ > >diff --git a/drivers/soundwire/bus_type.c b/drivers/soundwire/bus_type.c > >new file mode 100644 > >index 000000000000..3e97a8284871 > >--- /dev/null > >+++ b/drivers/soundwire/bus_type.c > >@@ -0,0 +1,227 @@ > ... > > >+static const struct sdw_device_id * > >+sdw_get_device_id(struct sdw_slave *slave, struct sdw_driver *drv) > Indentation looks Odd here, not really, the return type in preceding line is a perfect way to define things... > >+static int sdw_drv_probe(struct device *dev) > >+{ > >+ struct sdw_slave *slave = dev_to_sdw_dev(dev); > >+ struct sdw_driver *drv = drv_to_sdw_driver(dev->driver); > >+ const struct sdw_device_id *id; > >+ int ret; > ... > >+ /* > >+ * attach to power domain but don't turn on (last arg) > >+ */ > >+ ret = dev_pm_domain_attach(dev, false); > >+ if (ret) { > > I think we discussed this in v1, but erring out here means that all the > devices need to have pm domain attached, which might not be true all the > time. oh yes, sorry i missed this one. will fix > >+ > >+/** > >+ * struct sdw_slave_id: Slave ID > >+ * > Do we need an empty line Here?? same thing for all the kernel doc comments. > > Also looking at examples in Documentation/doc-guide/kernel-doc.rst > > struct should follow with - instead of : > same for functions.. Kernel seems to have both. But yes - is more predominant by an order of magnitude and Documented so will fix it up. -- ~Vinod