Re: [PATCH v7 04/13] slimbus: core: Add slim controllers support

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

 



thanks for the comments.


On 16/11/17 16:42, Vinod Koul wrote:
On Wed, Nov 15, 2017 at 02:10:34PM +0000, srinivas.kandagatla@xxxxxxxxxx wrote:

+static void slim_dev_release(struct device *dev)
+{
+	struct slim_device *sbdev = to_slim_device(dev);
+
+	put_device(sbdev->ctrl->dev);

which device would that be?
This is controller device


+static int slim_add_device(struct slim_controller *ctrl,
+			   struct slim_device *sbdev,
+			   struct device_node *node)
+{
+	sbdev->dev.bus = &slimbus_bus;
+	sbdev->dev.parent = ctrl->dev;
+	sbdev->dev.release = slim_dev_release;
+	sbdev->dev.driver = NULL;
+	sbdev->ctrl = ctrl;
+
+	dev_set_name(&sbdev->dev, "%x:%x:%x:%x",
+				  sbdev->e_addr.manf_id,
+				  sbdev->e_addr.prod_code,
+				  sbdev->e_addr.dev_index,
+				  sbdev->e_addr.instance);
+
+	get_device(ctrl->dev);

is this controller device and you ensuring it doesnt go away while you have
slaves on it?

Yes.


+static struct slim_device *slim_alloc_device(struct slim_controller *ctrl,
+					     struct slim_eaddr *eaddr,
+					     struct device_node *node)
+{
+	struct slim_device *sbdev;
+	int ret;
+
+	sbdev = kzalloc(sizeof(struct slim_device), GFP_KERNEL);

Usual kernel way of doing is kzalloc(*sbdev)

I agree will fix it in next version.

+void slim_report_absent(struct slim_device *sbdev)
+{
+	struct slim_controller *ctrl = sbdev->ctrl;
+
+	if (!ctrl)
+		return;
+
+	/* invalidate logical addresses */
+	mutex_lock(&ctrl->lock);
+	sbdev->is_laddr_valid = false;
+	mutex_unlock(&ctrl->lock);
+
+	ida_simple_remove(&ctrl->laddr_ida, sbdev->laddr);
+	slim_device_update_status(sbdev, SLIM_DEVICE_STATUS_DOWN);
+}
+EXPORT_SYMBOL_GPL(slim_report_absent);

Do you have APIs for report present too, if so why not add te status in
argument as you may have common handling
Yes, We do have api for reporting too, I will give it a try to combine both.


+static int slim_device_alloc_laddr(struct slim_device *sbdev,
+				   u8 *laddr, bool report_present)
+{
+	struct slim_controller *ctrl = sbdev->ctrl;
+	int ret;
+
+	mutex_lock(&ctrl->lock);
+	if (ctrl->get_laddr) {
+		ret = ctrl->get_laddr(ctrl, &sbdev->e_addr, laddr);
+		if (ret < 0)
+			goto err;
+	} else if (report_present) {
+		ret = ida_simple_get(&ctrl->laddr_ida,
+				     0, SLIM_LA_MANAGER - 1, GFP_KERNEL);
+		if (ret < 0)
+			goto err;
+
+		*laddr = ret;
+	} else {
+		ret = -EINVAL;
+		goto err;
+	}
+
+	if (ctrl->set_laddr) {
+		ret = ctrl->set_laddr(ctrl, &sbdev->e_addr, *laddr);
+		if (ret) {
+			ret = -EINVAL;
+			goto err;
+		}
+	}
+
+	sbdev->laddr = *laddr;

if you have this in sbdev, then why have this as an arg also?
Yes makes sens, laddr argument in this function is redundant, it can be removed totally.

+	sbdev->is_laddr_valid = true;

shouldn't non-zero value signify that?
0 is also a valid laddr.


--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [Linux for Sparc]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux