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

[Jun Nie <jun.nie@xxxxxxxxxx>]

2016-09-06 22:31 GMT+08:00 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?

The callback function argument number is different with
pinconf_generic_dt_node_to_map. So a wrapper is needed.

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

As my reply in 2/3 patches, the shared pin config registers make me to
merge the two pin controllers driver together to ease pin config code.
GPIO pin ranges like logic surely provide clearer hierarchical. I am thinking
whether pin config can share the pinmux hierarchical path to get the
data to configure the required register.

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