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

[Andy Shevchenko]

On Tue, Nov 05, 2019 at 06:52:52PM +0800, Tanwar, Rahul wrote:
> On 5/11/2019 5:49 PM, Andy Shevchenko wrote:
> > On Tue, Nov 05, 2019 at 02:49:42PM +0800, Rahul Tanwar wrote:

> >> +static void eqbr_set_val(void __iomem *addr, u32 offset,
> >> +			 u32 mask, u32 set, raw_spinlock_t *lock)
> > This lock parameter is quite unusual. Can't you supply a pointer to a data
> > structure which has lock along with MMIO address?
> 
> On second thoughts, i realize that this function can be totally avoided
> since it just has two callers which can be further reduced to one caller.
> I will remove this function and instead do reg update in the caller function
> itself.

Good.

> >> +static int gpiochip_setup(struct device *dev, struct eqbr_gpio_desc *desc)
> >> +{
> >> +	struct gpio_irq_chip *girq;
> >> +	struct gpio_chip *gc;
> >> +#if defined(CONFIG_OF_GPIO)
> >> +	gc->of_node = desc->node;
> >> +#endif
> > Isn't it what GPIO library does for everybody?
> 
> We have 4 different of_node's for 4 different gpio_chips/banks. GPIO library
> handles like below:
> 
>         if (chip->parent) {
>                 gdev->dev.parent = chip->parent;
>                 gdev->dev.of_node = chip->parent->of_node;
>         }
> 
> #ifdef CONFIG_OF_GPIO
>         /* If the gpiochip has an assigned OF node this takes precedence */
>         if (chip->of_node)
>                 gdev->dev.of_node = chip->of_node;
>         else
>                 chip->of_node = gdev->dev.of_node;
> #endif
> 
> So i think we need to assign 4 of_node's to gpio_chip's in the driver.

OK!

> >> +	if (!of_property_read_bool(desc->node, "interrupt-controller")) {
> >> +		dev_info(dev, "gc %s: doesn't act as interrupt controller!\n",
> >> +			 desc->name);
> > Is it fatal or non-fatal?
> 
> It is not fatal. But i am totally missing your point. Is it about
> dev_info() ? Can you please elaborate more ?
> 
> 
> >> +		return 0;
> > Ditto.

> >> +	}

If it's fatal, you have to use dev_err() and return an appropriate error code,
if it's not fatal, switch to dev_warn() in case user has to know that behaviour
may be quite different, while seems to work in general or dev_dbg() if it's
only for the developer. dev_info() with return 0 is quite confusing.

> >> +}

> >> +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;
> >
> >> +	pinmux = grp->data;
> >> +	for (i = 0; i < grp->num_pins; i++)
> >> +		eqbr_set_pin_mux(pctl, pinmux[i], grp->pins[i]);
> > Shouldn't be this part serialized?
> >
> > Same Q to all similar places. I guess I already mentioned this in previous
> > review.
> 
> From serialization, you mean locking..rt ? Yes, there is one writel()
> statement inthis flow. I will add lock for that statement. Rechecked
> the code again, i do notfind any other similar places.

You need to clarify what exactly you are serializing.
When you figure this out, the locking should be done accordingly.

> >> +	return 0;
> >> +}

> >> +			if (of_property_read_string(np, "function",
> >> +						    &fn_name)) {

> > It's perfectly one line. Perhaps you may need to configure your text editor.
> 
> I am following strict 80 chars limit. This goes on to 81 chars. It's a bit
> confusing on when to adhere to 80 chars limit and when to bypass it :)

I have checked again, it's exactly 80 characters.

> >> +			}

-- 
With Best Regards,
Andy Shevchenko