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
