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
[off-list ref] wrote:

quoted

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.

quoted

+static int owl_set_mux(struct pinctrl_dev *pctrldev,
+                               unsigned int function,
+                               unsigned int group)
+{

quoted

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

quoted

+static int owl_pin_config_set(struct pinctrl_dev *pctrldev,
+                               unsigned int pin,
+                               unsigned long *configs,
+                               unsigned int num_configs)
+{

quoted

+       int ret = 0;

Redundant assignment?

Ack.
 

quoted

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

quoted

+}

quoted

+static int owl_group_pinconf_val2arg(const struct owl_pingroup *g,
+                               unsigned int param,
+                               u32 *arg)
+{

quoted

+       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
 

quoted

+       return 0;
+}
+
+static int owl_group_config_get(struct pinctrl_dev *pctrldev,
+                               unsigned int group,
+                               unsigned long *config)
+{
+       int ret = 0;

Redundant assignment.

Ack.

quoted

+}

quoted

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

quoted

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

quoted

+       return ret;

return 0; ?

Okay.

quoted

+}

quoted

+int owl_pinctrl_probe(struct platform_device *pdev,
+                               struct owl_pinctrl_soc_data *soc_data)
+{

quoted

+       clk_prepare_enable(pctrl->clk);

This can fail.

Okay. Will add a check.
 

quoted

+}

quoted

+static const struct of_device_id s900_pinctrl_of_match[] = {
+       { .compatible = "actions,s900-pinctrl", },

quoted

+       { },

No comma needed.

Okay.

Thanks for the review.

Regards,
Mani
 

quoted

+};

-- 
With Best Regards,
Andy Shevchenko