Re: [PATCH 2/3] pinctrl: ralink: add a pinctrl driver for the rt2880 family

[Dan Carpenter <dan.carpenter@xxxxxxxxxx>]

On Mon, Dec 07, 2020 at 08:21:03PM +0100, Sergio Paracuellos wrote:
> +static struct pinctrl_desc rt2880_pctrl_desc = {
> +	.owner		= THIS_MODULE,
> +	.name		= "rt2880-pinmux",
> +	.pctlops	= &rt2880_pctrl_ops,
> +	.pmxops		= &rt2880_pmx_group_ops,
> +};
> +
> +static struct rt2880_pmx_func gpio_func = {
> +	.name = "gpio",
> +};
> +
> +static int rt2880_pinmux_index(struct rt2880_priv *p)


This function name is not great.  I assumed that it would return the
index.

> +{
> +	struct rt2880_pmx_func **f;

Get rid of this "f" variable and use "p->func" instead.

> +	struct rt2880_pmx_group *mux = p->groups;
> +	int i, j, c = 0;
> +
> +	/* count the mux functions */
> +	while (mux->name) {
> +		p->group_count++;
> +		mux++;
> +	}
> +
> +	/* allocate the group names array needed by the gpio function */
> +	p->group_names = devm_kcalloc(p->dev, p->group_count,
> +				      sizeof(char *), GFP_KERNEL);
> +	if (!p->group_names)
> +		return -1;

Return proper error codes in this function.  s/-1/-ENOMEM/

> +
> +	for (i = 0; i < p->group_count; i++) {
> +		p->group_names[i] = p->groups[i].name;
> +		p->func_count += p->groups[i].func_count;
> +	}
> +
> +	/* we have a dummy function[0] for gpio */
> +	p->func_count++;
> +
> +	/* allocate our function and group mapping index buffers */
> +	f = p->func = devm_kcalloc(p->dev,
> +				   p->func_count,
> +				   sizeof(*p->func),
> +				   GFP_KERNEL);
> +	gpio_func.groups = devm_kcalloc(p->dev, p->group_count, sizeof(int),
> +					GFP_KERNEL);
> +	if (!f || !gpio_func.groups)
> +		return -1;
> +
> +	/* add a backpointer to the function so it knows its group */
> +	gpio_func.group_count = p->group_count;
> +	for (i = 0; i < gpio_func.group_count; i++)
> +		gpio_func.groups[i] = i;
> +
> +	f[c] = &gpio_func;
> +	c++;
> +
> +	/* add remaining functions */
> +	for (i = 0; i < p->group_count; i++) {
> +		for (j = 0; j < p->groups[i].func_count; j++) {
> +			f[c] = &p->groups[i].func[j];
> +			f[c]->groups = devm_kzalloc(p->dev, sizeof(int),
> +						    GFP_KERNEL);

Add a NULL check.

> +			f[c]->groups[0] = i;
> +			f[c]->group_count = 1;
> +			c++;
> +		}
> +	}
> +	return 0;
> +}
> +
> +static int rt2880_pinmux_pins(struct rt2880_priv *p)
> +{
> +	int i, j;
> +
> +	/*
> +	 * loop over the functions and initialize the pins array.
> +	 * also work out the highest pin used.
> +	 */
> +	for (i = 0; i < p->func_count; i++) {
> +		int pin;
> +
> +		if (!p->func[i]->pin_count)
> +			continue;
> +
> +		p->func[i]->pins = devm_kcalloc(p->dev,
> +						p->func[i]->pin_count,
> +						sizeof(int),
> +						GFP_KERNEL);

This can fit on two lines.

		p->func[i]->pins = devm_kcalloc(p->dev, p->func[i]->pin_count,
						sizeof(int), GFP_KERNEL);

> +		for (j = 0; j < p->func[i]->pin_count; j++)
> +			p->func[i]->pins[j] = p->func[i]->pin_first + j;
> +
> +		pin = p->func[i]->pin_first + p->func[i]->pin_count;
> +		if (pin > p->max_pins)
> +			p->max_pins = pin;
> +	}
> +
> +	/* the buffer that tells us which pins are gpio */
> +	p->gpio = devm_kcalloc(p->dev, p->max_pins, sizeof(u8), GFP_KERNEL);
> +	/* the pads needed to tell pinctrl about our pins */
> +	p->pads = devm_kcalloc(p->dev, p->max_pins,
> +			       sizeof(struct pinctrl_pin_desc), GFP_KERNEL);
> +	if (!p->pads || !p->gpio) {
> +		dev_err(p->dev, "Failed to allocate gpio data\n");

Delete this error message.  #checkpatch.pl

> +		return -ENOMEM;
> +	}
> +
> +	memset(p->gpio, 1, sizeof(u8) * p->max_pins);
> +	for (i = 0; i < p->func_count; i++) {
> +		if (!p->func[i]->pin_count)
> +			continue;
> +
> +		for (j = 0; j < p->func[i]->pin_count; j++)
> +			p->gpio[p->func[i]->pins[j]] = 0;
> +	}
> +
> +	/* pin 0 is always a gpio */
> +	p->gpio[0] = 1;
> +
> +	/* set the pads */
> +	for (i = 0; i < p->max_pins; i++) {
> +		/* strlen("ioXY") + 1 = 5 */
> +		char *name = devm_kzalloc(p->dev, 5, GFP_KERNEL);
> +

		char *name;

		name = kasprintf(GFP_KERNEL, "io%d", i);


> +		if (!name)
> +			return -ENOMEM;
> +		snprintf(name, 5, "io%d", i);
> +		p->pads[i].number = i;
> +		p->pads[i].name = name;
> +	}
> +	p->desc->pins = p->pads;
> +	p->desc->npins = p->max_pins;
> +
> +	return 0;
> +}
> +
> +static int rt2880_pinmux_probe(struct platform_device *pdev)
> +{
> +	struct rt2880_priv *p;
> +	struct pinctrl_dev *dev;
> +
> +	if (!rt2880_pinmux_data)
> +		return -ENOTSUPP;
> +
> +	/* setup the private data */
> +	p = devm_kzalloc(&pdev->dev, sizeof(struct rt2880_priv), GFP_KERNEL);
> +	if (!p)
> +		return -ENOMEM;
> +
> +	p->dev = &pdev->dev;
> +	p->desc = &rt2880_pctrl_desc;
> +	p->groups = rt2880_pinmux_data;
> +	platform_set_drvdata(pdev, p);
> +
> +	/* init the device */
> +	if (rt2880_pinmux_index(p)) {
> +		dev_err(&pdev->dev, "failed to load index\n");
> +		return -EINVAL;

Preserve the error code:

	err = rt2880_pinmux_index(p);
	if (err) {
		dev_err(&pdev->dev, "failed to load index\n");
		return err;
	}


> +	}
> +	if (rt2880_pinmux_pins(p)) {
> +		dev_err(&pdev->dev, "failed to load pins\n");
> +		return -EINVAL;

Same.

> +	}
> +	dev = pinctrl_register(p->desc, &pdev->dev, p);
> +	if (IS_ERR(dev))
> +		return PTR_ERR(dev);
> +
> +	return 0;
> +}

regards,
dan carpenter