Re: [PATCH v4 1/2] pinctrl: Add pinmux & GPIO controller driver for a new SoC

[Tanwar, Rahul]

Hi Andy,

On 7/11/2019 5:07 PM, Andy Shevchenko wrote:
> On Thu, Nov 07, 2019 at 03:36:44PM +0800, Rahul Tanwar wrote:
> +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?

These are all ops called back from the core pinctrl framework.
My understanding is that multi-threading serialization is provided by the
pinctrl framework using mutex's. Drivers don't have to worry about that.
Drivers only have to worry about multi-core serialization. I checked
many other existing pinctrl drivers & all of them seem to use spin lock
only for register accesses (not for serializing the ops itself).

Is this understanding incorrect ?


>
>> +	/* 0 mux is reserved for GPIO */
> Perhaps
>
> #define EQBR_GPIO_MODE	0
>
> ?

I had first used #define for this but removed it based on Rob Herring
review feedback. I will add it back here but keep it as 0 in dt bindings..

>> +	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?

Same comments as above..

>
>> +		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.

Lost you here. Please elaborate more on what you mean by rearranging. Thanks.

Regards,
Rahul