Re: [alsa-devel] [PATCH v4 03/15] soundwire: Add Master registration

[Vinod Koul <vinod.koul@intel.com>]

On Fri, Dec 01, 2017 at 04:10:55PM -0600, Pierre-Louis Bossart wrote:
> On 12/1/17 3:56 AM, Vinod Koul wrote:
> >A Master registers with SoundWire bus and scans the firmware provided
> 
> nitpick: is the 'register' correct? You create a bus instance for each
> hardware master interface. Or is my brain fried?

naah, thankfully it is not :)

I will reword this, I see it can cause ambguity :)

> >+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);
> >+
> >+	/*
> >+	 * Device numbers in SoundWire are 0 thru 15 with 0 being
> >+	 * Enumeration device number and 15 broadcast device number. So
> >+	 * they are not used for assignment so mask these and other
> >+	 * higher bits
> 
> device 14 is used for the master and is not supported by these patches.
> While allowed, if you use device 12 and 13 (groups) you can't get the status
> information in a PING command so the recommendation is to avoid them.
> Those device numbers should never be used really (on top of 0 and 15 as
> explained above)

Yeah we should also exclude 12 thru 14 here. The group alllocation when done
need to do differently that this one, so will mask those too.

> >+void sdw_extract_slave_id(struct sdw_bus *bus,
> >+			u64 addr, struct sdw_slave_id *id)
> >+{
> >+	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]
> 
> Add a reference to mid.mipi.org (here or in the summary) ?

Will add in summary, makes sense on that page

> >+int sdw_acpi_find_slaves(struct sdw_bus *bus)
> >+{
> >+	struct acpi_device *adev, *parent;
> >+
> >+	parent = ACPI_COMPANION(bus->dev);
> >+	if (!parent) {
> >+		dev_err(bus->dev, "Can't find parent for acpi bind\n");
> >+		return -ENODEV;
> >+	}
> >+
> >+	list_for_each_entry(adev, &parent->children, node) {
> >+		unsigned long long addr;
> >+		struct sdw_slave_id id;
> >+		unsigned int link_id;
> >+		acpi_status status;
> >+
> >+		status = acpi_evaluate_integer(adev->handle,
> >+					METHOD_NAME__ADR, NULL, &addr);
> >+
> >+		if (ACPI_FAILURE(status)) {
> >+			dev_err(bus->dev, "_ADR resolution failed: %x\n",
> >+							status);
> >+			return status;
> >+		}
> >+
> >+		/* Extract link id from ADR, it is from 48 to 51 bits */
> 
> nitpick: in bits 51..48 as defined in the MIPI SoundWire DisCo spec.

ok

> >+/* SDW Enumeration Device Number */
> >+#define SDW_ENUM_DEV_NUM		0
> 
> add
> SDW_GROUP12_DEV_NUM		12
> SDW_GROUP12_DEV_NUM		13

you mean SDW_GROUP13_DEV_NUM :D

> SDW_MASTER_DEV_NUM		14 /* not supported in these patches */

These dont help now as we dont support, so kind of dead code.
Lets add them when we really support it

-- 
~Vinod