On Wed, Feb 28, 2018 at 8:14 PM, Manivannan Sadhasivam
[off-list ref] 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().

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

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

+}

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

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

Redundant assignment.

+}

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

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

+       return ret;

return 0; ?

+}

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

+       clk_prepare_enable(pctrl->clk);

This can fail.

+}

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

+       { },

No comma needed.

+};

-- 
With Best Regards,
Andy Shevchenko