Re: [PATCH 3/3] pinctrl: zx: Add ZTE ZX SoC pinctrl driver

[Linus Walleij <linus.walleij@xxxxxxxxxx>]

On Fri, Aug 26, 2016 at 2:19 PM, Jun Nie <jun.nie@xxxxxxxxxx> wrote:

> This adds pinctrl driver for ZTE ZX platform. The bit width
> of pinmux registers are not unified, so this dedicated driver
> is needed and detail bit width data is store in private data.
> The parent pin information is also stored in private data so
> that parent pin can be requested automatically.
>
> Signed-off-by: Jun Nie <jun.nie@xxxxxxxxxx>

(...)
> +static int zx_pctrl_dt_node_to_map(struct pinctrl_dev *pctldev,
> +                                  struct device_node *np_config,
> +                                  struct pinctrl_map **map,
> +                                  u32 *num_maps)
> +{
> +       return pinconf_generic_dt_node_to_map(pctldev, np_config, map,
> +                                             num_maps, PIN_MAP_TYPE_INVALID);
> +}

Uhm...

> +static const struct pinctrl_ops zx_pctrl_ops = {
> +       .dt_node_to_map         = zx_pctrl_dt_node_to_map,

Can't you just put pinconf_generic_dt_node_to_map here?

> +       .dt_free_map            = pinctrl_utils_free_map,
> +       .get_groups_count       = zx_pctrl_get_groups_count,
> +       .get_group_name         = zx_pctrl_get_group_name,
> +       .get_group_pins         = zx_pctrl_get_group_pins,
> +};

(...)

> +#define NONAON_MVAL 2
> +int zx_pmx_request(struct pinctrl_dev *pctldev, unsigned offset)
> +{
> +       struct zx_pinctrl *pctl = pinctrl_dev_get_drvdata(pctldev);
> +       int ret;
> +       const struct pinctrl_pin_desc *pins = pctldev->desc->pins;
> +       struct zx_pin_desc_data *zx_pin_dat;
> +
> +       pins += offset;
> +       zx_pin_dat = (struct zx_pin_desc_data *)pins->drv_data;
> +       if (!zx_pin_dat->parent)
> +               return 0;
> +
> +       ret = pinctrl_request_pin(pctldev, zx_pin_dat->parent, "nonAON");
> +       if (ret) {
> +               dev_err(pctl->dev, "Failed to request parent pin %d\n",
> +                       zx_pin_dat->parent);
> +               return ret;
> +       }

This is the real trick isn't it?

I don't like exposing that function directly like this. It's
shortcutting through
the abstractions.

What we need is either:

- A way to describe that a pin controller is fully a child of another
  pin controller, a "strict hierarchy" if all pins go through both pin
  controllers.

OR

- If only *some* pins go routed through the hierarchy: something akin
  to the GPIO pin ranges. That makes it possible to specify in the
  device tree which lines are hierarchical and how that plays out.

Apart from this the driver looks all right, but we need to figure out
how to represent the hierarchy.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html