Re: [RFC 1/5] pinctrl: rz-pfc: Add Renesas RZ pinctrl core module

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

 



Hi Laurent,

On 01/02/2017 16:21, Laurent Pinchart wrote:
Hi Jacopo,

[snip]

+}
+
+/**
+ * rz_dt_node_to_map() - Parse device tree nodes and collect pins, groups
and
+ *			 functions

I don't think we have groups and functions, do we ?


Sort of.. The hardware does not have that, but we create them from pin configuration sub-nodes, as the pinmux core API is sort of group/function centric (see the pinmux_ops.set_mux function prototype)

[snip]

+	mutex_lock(&rz_pinctrl->mutex);

This seems weird. The pinctrl generic functions indeed state that the caller
must take care of locking, but there doesn't seem to be any lock protecting
races between .dt_node_to_map() calling the generic functions below, and the
generic .get_group_*() handlers.

+	ret = pinctrl_generic_add_group(pctldev, grpname, grpins, npins,
NULL);
+	if (ret) {
+		mutex_unlock(&rz_pinctrl->mutex);
+		goto free_map;
+	}
+
+	ret = pinmux_generic_add_function(pctldev, grpname, fngrps,
+					  1, mux_modes);
+	if (ret) {
+		mutex_unlock(&rz_pinctrl->mutex);
+		goto free_map;
+	}
+	mutex_unlock(&rz_pinctrl->mutex);

radix trees are RCU protected structures, so no locking should be required when accessing them from different places, so I guess lock here is just to protect the num_groups and num_functions variables in core pin controller driver...

[snip]

+/**
+ * rz_dt_free_map()
+ */
+static void rz_dt_free_map(struct pinctrl_dev *pctldev,
+			   struct pinctrl_map *map, unsigned int num_maps)
+{
+	struct rz_pinctrl_dev *rz_pinctrl = pinctrl_dev_get_drvdata(pctldev);
+
+	devm_kfree(rz_pinctrl->dev, map);

Shouldn't you also free the other allocated pieces of memory (fngrps,
mux_modes and grpins) ?


That's an interesting one. If I have to free all data associated with that map (and I assume so) I shall be able to retrieve groups and functions that map represents and delete them and

Currently there is no generic pinctrl and pinmux function to retrieve a group or function by name, but only by their id (selector). It would take 10minutes to add them, but I wonder if that's desirable or there are other ways to do so I haven't found out yet.

Linus, Tony and gpio people: do you have opinions on this? I can add those functions to core/pinmux in my next patch series if I get your ack here.

Thanks
   j

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



[Index of Archives]     [Linux SPI]     [Linux Kernel]     [Linux ARM (vger)]     [Linux ARM MSM]     [Linux Omap]     [Linux Arm]     [Linux Tegra]     [Fedora ARM]     [Linux for Samsung SOC]     [eCos]     [Linux Fastboot]     [Gcc Help]     [Git]     [DCCP]     [IETF Announce]     [Security]     [Linux MIPS]     [Yosemite Campsites]

  Powered by Linux