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
