Messages in this thread | | | Date | Thu, 1 Mar 2018 22:59:44 +0530 | From | Manivannan Sadhasivam <> | Subject | Re: [PATCH v3 04/10] pinctrl: actions: Add Actions S900 pinctrl driver |
| |
Hi Andy,
On Wed, Feb 28, 2018 at 08:36:53PM +0200, Andy Shevchenko wrote: > On Wed, Feb 28, 2018 at 8:14 PM, Manivannan Sadhasivam > <manivannan.sadhasivam@linaro.org> wrote: > > Add pinctrl driver for Actions Semi S900 SoC. The driver supports > > pinctrl, pinmux and pinconf functionalities through a range of registers > > common to both gpio driver and pinctrl driver. > > > > Pinmux functionality is available only for the pin groups while the > > pinconf functionality is available for both pin groups and individual > > pins. > > > +static int owl_set_mux(struct pinctrl_dev *pctrldev, > > + unsigned int function, > > + unsigned int group) > > +{ > > > + mfpval = readl(pctrl->base + g->mfpctl_reg); > > + mfpval &= ~mask; > > + mfpval |= val; > > + writel(mfpval, pctrl->base + g->mfpctl_reg); > > This is called owl_update_bits(). >
Okay. Will add a helper.
> > +static int owl_pin_config_set(struct pinctrl_dev *pctrldev, > > + unsigned int pin, > > + unsigned long *configs, > > + unsigned int num_configs) > > +{ > > > + int ret = 0; > > Redundant assignment? >
Ack.
> > + mask = (1 << width) - 1; > > + mask = mask << bit; > > + tmp = readl(pctrl->base + reg); > > + tmp &= ~mask; > > + tmp |= arg << bit; > > + writel(tmp, pctrl->base + reg); > > This is called owl_update_bits(). >
Ack.
> > +} > > > +static int owl_group_pinconf_val2arg(const struct owl_pingroup *g, > > + unsigned int param, > > + u32 *arg) > > +{ > > > + case PIN_CONFIG_SLEW_RATE: > > + if (*arg) > > + *arg = 1; > > + else > > + *arg = 0; > > Doesn't slew rate allow a non-binary value? >
As stated in the binding doc, valid values for the slew rate parameter are:
0 - Slow 1 - Fast
> > + return 0; > > +} > > + > > +static int owl_group_config_get(struct pinctrl_dev *pctrldev, > > + unsigned int group, > > + unsigned long *config) > > +{ > > + int ret = 0; > > Redundant assignment. >
Ack.
> > +} > > > +static int owl_group_config_set(struct pinctrl_dev *pctrldev, > > + unsigned int group, > > + unsigned long *configs, > > + unsigned int num_configs) > > +{ > > + int ret = 0; > > Redundant assignment, see below. >
Ack. Will return 0 directly.
> > + mask = (1 << width) - 1; > > + mask = mask << bit; > > + tmp = readl(pctrl->base + reg); > > + tmp &= ~mask; > > + tmp |= arg << bit; > > + writel(tmp, pctrl->base + reg); > > This is called owl_update_bits(). >
Ack.
> > + return ret; > > return 0; ? >
Okay.
> > +} > > > +int owl_pinctrl_probe(struct platform_device *pdev, > > + struct owl_pinctrl_soc_data *soc_data) > > +{ > > > + clk_prepare_enable(pctrl->clk); > > This can fail. >
Okay. Will add a check.
> > +} > > > +static const struct of_device_id s900_pinctrl_of_match[] = { > > + { .compatible = "actions,s900-pinctrl", }, > > > + { }, > > No comma needed. >
Okay.
Thanks for the review.
Regards, Mani
> > +}; > > -- > With Best Regards, > Andy Shevchenko
| |