On Thu, Nov 07, 2019 at 03:36:44PM +0800, Rahul Tanwar wrote:

Intel Lightning Mountain SoC has a pinmux controller & GPIO controller IP which
controls pin multiplexing & configuration including GPIO functions selection &
GPIO attributes configuration.

This IP is not based on & does not have anything in common with Chassis
specification. The pinctrl drivers under pinctrl/intel/* are all based upon
Chassis spec compliant pinctrl IPs. So this driver doesn't fit & can not use
pinctrl framework under pinctrl/intel/* and it requires a separate new driver.

Add a new GPIO & pin control framework based driver for this IP.

+static void eqbr_gpio_mask_ack_irq(struct irq_data *d)
+{
+	eqbr_gpio_disable_irq(d);
+	eqbr_gpio_ack_irq(d);

Potential race?

+}

+static int eqbr_pinmux_set_mux(struct pinctrl_dev *pctldev,
+			       unsigned int selector, unsigned int group)
+{
+	struct eqbr_pinctrl_drv_data *pctl = pinctrl_dev_get_drvdata(pctldev);
+	struct function_desc *func;
+	struct group_desc *grp;
+	unsigned int *pinmux;
+	int i;
+
+	func = pinmux_generic_get_function(pctldev, selector);
+	if (!func)
+		return -EINVAL;
+
+	grp = pinctrl_generic_get_group(pctldev, group);
+	if (!grp)
+		return -EINVAL;
+
+	pinmux = grp->data;

+	for (i = 0; i < grp->num_pins; i++)
+		eqbr_set_pin_mux(pctl, pinmux[i], grp->pins[i]);

What if in the middle of the loop mux of one of the pins be changed by parallel
thread?

+	return 0;
+}

+static int eqbr_pinmux_gpio_request(struct pinctrl_dev *pctldev,
+				    struct pinctrl_gpio_range *range,
+				    unsigned int pin)
+{
+	struct eqbr_pinctrl_drv_data *pctl = pinctrl_dev_get_drvdata(pctldev);
+

+	/* 0 mux is reserved for GPIO */

Perhaps

#define EQBR_GPIO_MODE	0

?

+	return eqbr_set_pin_mux(pctl, 0, pin);
+}

+	for (i = 0; i < npins; i++) {
+		ret = eqbr_pinconf_set(pctldev, pins[i], configs, num_configs);
+		if (ret)
+			return ret;

What if in the middle of the loop settings of one of the pins be changed by
parallel thread?

+	}

+static int eqbr_build_groups(struct eqbr_pinctrl_drv_data *drvdata)
+{
+	struct device *dev = drvdata->dev;
+	struct device_node *node = dev->of_node;
+	unsigned int *pinmux, pin_id, pinmux_id;
+	struct group_desc group;
+	struct device_node *np;
+	struct property *prop;
+	int j, err;
+
+	for_each_child_of_node(node, np) {
+		prop = of_find_property(np, "groups", NULL);
+		if (!prop)
+			continue;
+
+		group.num_pins = of_property_count_u32_elems(np, "pins");
+		if (group.num_pins < 0) {

+			dev_err(dev, "No pins in the group: %s\n",
+				prop->name);

It is perfectly one line. The idea of if (!foo) pattern in the loop to make
code occupy less LOCs along with increased readability.

+			return -EINVAL;
+		}
+		group.name = prop->value;

+		group.pins = devm_kcalloc(dev, group.num_pins,
+					  sizeof(*(group.pins)), GFP_KERNEL);
+		pinmux = devm_kcalloc(dev, group.num_pins,
+				      sizeof(*pinmux), GFP_KERNEL);

These can be rearranged.

+
+		if (!group.pins || !pinmux)
+			return -ENOMEM;
+		for (j = 0; j < group.num_pins; j++) {

+			if (of_property_read_u32_index(np, "pins",
+						       j, &pin_id)) {

One line.

And so on...

+				dev_err(dev, "Group %s: Read intel pins id failed\n",
+					group.name);
+				return -EINVAL;
+			}
+			if (pin_id >= drvdata->pctl_desc.npins) {
+				dev_err(dev, "Group %s: Invalid pin ID, idx: %d, pin %u\n",
+					group.name, j, pin_id);
+				return -EINVAL;
+			}
+			group.pins[j] = pin_id;
+			if (of_property_read_u32_index(np, "pinmux",
+						       j, &pinmux_id)) {
+				dev_err(dev, "Group %s: Read intel pinmux id failed\n",
+					group.name);
+				return -EINVAL;
+			}
+			pinmux[j] = pinmux_id;
+		}
+
+		err = pinctrl_generic_add_group(drvdata->pctl_dev, group.name,
+						group.pins, group.num_pins,
+						pinmux);
+		if (err < 0) {
+			dev_err(dev, "Failed to register group %s\n",
+				group.name);
+			return err;
+		}
+		memset(&group, 0, sizeof(group));
+		pinmux = NULL;
+	}
+
+	return 0;
+}

-- 
With Best Regards,
Andy Shevchenko