Re: [PATCH v2] hwspinlock/msm: Add support for Qualcomm MSM HW Mutex block

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 8/29/2014 5:14 PM, Bjorn Andersson wrote:
From: Kumar Gala <galak@xxxxxxxxxxxxxx>

Add driver for Qualcomm MSM Hardware Mutex block that exists on
newer Qualcomm SoCs.

Cc: Jeffrey Hugo <jhugo@xxxxxxxxxxxxxx>
Cc: Eric Holmberg <eholmber@xxxxxxxxxxxxxx>
Cc: Courtney Cavin <courtney.cavin@xxxxxxxxxxxxxx>
Signed-off-by: Kumar Gala <galak@xxxxxxxxxxxxxx>
[bjorn: added pm_runtime calls, from Courtney,
	added sfpb-mutex compatible,
	updated DT binding documentation formatting]
Signed-off-by: Bjorn Andersson <bjorn.andersson@xxxxxxxxxxxxxx>
---
[...]
diff --git a/drivers/hwspinlock/msm_hwspinlock.c b/drivers/hwspinlock/msm_hwspinlock.c
new file mode 100644
index 0000000..9ddd020
--- /dev/null
+++ b/drivers/hwspinlock/msm_hwspinlock.c
@@ -0,0 +1,155 @@
+/*
+ * Copyright (c) 2013, The Linux Foundation. All rights reserved.

Should the copyright range be updated to include your changes which I presume were authored in 2014?

+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/err.h>
+#include <linux/kernel.h>
+#include <linux/slab.h>
+#include <linux/device.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_device.h>
+#include <linux/hwspinlock.h>
+#include <linux/io.h>

Could these be put in alphabetical order? I vaguely recall a few maintainers expressing this preference to avoid merge issues.

+
+#include "hwspinlock_internal.h"
+
+#define SPINLOCK_ID_APPS_PROC	1
+#define BASE_ID			0
[...]
+static int msm_hwspinlock_probe(struct platform_device *pdev)
+{
+	int ret, i, stride;
+	size_t array_size;
+	u32 num_locks;
+	struct hwspinlock_device *bank;
+	struct hwspinlock *hwlock;
+	struct resource *res;
+	void __iomem *iobase;
+	struct device_node *node = pdev->dev.of_node;
+	const struct of_device_id *match;
+
+	match = of_match_device(msm_hwspinlock_of_match, &pdev->dev);
+	if (!match)
+		return -EINVAL;

This seems redundant. It is my understanding that probe will only be called for a matching device. What are we attempting to accomplish here?

+
+	ret = of_property_read_u32(node, "qcom,num-locks", &num_locks);
+	if (ret || num_locks == 0)
+		return -ENODEV;
+
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "mutex-base");
+	iobase = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(iobase))
+		return PTR_ERR(iobase);
+
+	array_size = num_locks * sizeof(*hwlock);
+	bank = devm_kzalloc(&pdev->dev, sizeof(*bank) + array_size, GFP_KERNEL);
+	if (!bank)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, bank);
+
+	stride = (int)match->data;
+	for (i = 0, hwlock = &bank->lock[0]; i < num_locks; i++, hwlock++)
+		hwlock->priv = iobase + i * stride;

I am not a fan of this method for determining the stride. We already have 0x4 and 0x80 in this driver, and will soon need 0x1000 for some of the current chips (still listed as TCSR too). The stride is completely up to our hardware designers, and it seems like encoding stride in this manner will require constant updates and maintenance. I prefer calculating stride by dividing the reg size by num_locks since that will automatically adjust for whatever the hardware designers decide to use next month. If you wanted to, with the reg size calculation, you could remove the "qcom,sfpb-mutex" since there is no functional difference other than stride. What are your thoughts?

+
+	pm_runtime_enable(&pdev->dev);
+
+	ret = hwspin_lock_register(bank, &pdev->dev, &msm_hwspinlock_ops,
+						BASE_ID, num_locks);
+	if (ret)
+		pm_runtime_disable(&pdev->dev);
+
+	return ret;
+}
[...]


Jeffrey Hugo
--
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]
  Powered by Linux