From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
        id S935871AbdKPQFc (ORCPT <rfc822;w@1wt.eu>);
        Thu, 16 Nov 2017 11:05:32 -0500
Received: from mail-wr0-f193.google.com ([209.85.128.193]:46128 "EHLO
        mail-wr0-f193.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S935857AbdKPQF0 (ORCPT
        <rfc822;linux-kernel@vger.kernel.org>);
        Thu, 16 Nov 2017 11:05:26 -0500
X-Google-Smtp-Source: AGs4zMaywUC1sNkdL21ek7CkFJKGmtIwDDq0jWt3Bys052KKqacN03gFXrHlz9oZqyGiUVvf5mrw3w==
Subject: Re: [PATCH v2 02/14] soundwire: Add SoundWire bus type
To: Vinod Koul <vinod.koul@intel.com>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: LKML <linux-kernel@vger.kernel.org>,
        ALSA <alsa-devel@alsa-project.org>, Mark <broonie@kernel.org>,
        Takashi <tiwai@suse.de>, Pierre <pierre-louis.bossart@linux.intel.com>,
        Sanyog Kale <sanyog.r.kale@intel.com>,
        Shreyas NC <shreyas.nc@intel.com>, patches.audio@intel.com,
        alan@linux.intel.com, Charles Keepax <ckeepax@opensource.cirrus.com>,
        Sagar Dharia <sdharia@codeaurora.org>, plai@codeaurora.org,
        Sudheer Papothi <spapothi@codeaurora.org>
References: <1510314556-13002-1-git-send-email-vinod.koul@intel.com>
 <1510314556-13002-3-git-send-email-vinod.koul@intel.com>
From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
Message-ID: <cb4fbc87-7857-5ab3-a2cd-413ee57c463e@linaro.org>
Date: Thu, 16 Nov 2017 16:05:17 +0000
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.2.1
MIME-Version: 1.0
In-Reply-To: <1510314556-13002-3-git-send-email-vinod.koul@intel.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org



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 <linux/init.h>
> +#include <linux/device.h>
> +#include <linux/module.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/acpi.h>

Do you need all these headers as part of this patch?

> +#include <linux/soundwire/sdw.h>
> +
> +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,

> +{
> +	const struct sdw_device_id *id = drv->id_table;
> +
> +	while (id && id->mfg_id) {
> +		if (slave->id.mfg_id == id->mfg_id &&
> +		    slave->id.part_id == id->part_id)
> +			return id;
> +		id++;
> +	}
> +
> +	return NULL;
> +}
...
> +
> +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.

> +		dev_err(dev, "Failed to attach PM domain: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = drv->probe(slave, id);
> +	if (ret) {
> +		dev_err(dev, "Probe of %s failed: %d\n", drv->name, ret);
> +		dev_pm_domain_detach(dev, false);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +
>
...

> diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h
> +#ifndef __SOUNDWIRE_H
> +#define __SOUNDWIRE_H
> +
> +#include <linux/device.h>
> +#include <linux/mod_devicetable.h>
> +
> +struct sdw_bus;
> +struct sdw_slave;
> +
> +#define SDW_MAX_DEVICES			11
> +
> +/**
> + * enum sdw_slave_status: Slave status
> + *
> + * @SDW_SLAVE_UNATTACHED: Slave is not attached with the bus.
> + * @SDW_SLAVE_ATTACHED: Slave is attached with bus.
> + * @SDW_SLAVE_ALERT: Some alert condition on the Slave
> + * @SDW_SLAVE_RESERVED: Reserved for future use
> + */
> +enum sdw_slave_status {
> +	SDW_SLAVE_UNATTACHED = 0,
> +	SDW_SLAVE_ATTACHED = 1,
> +	SDW_SLAVE_ALERT = 2,
> +	SDW_SLAVE_RESERVED = 3,
> +};
> +
> +/*
> + * SDW Slave Structures and APIs
> + */
> +
> +/**
> + * 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..