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
