RE: [PATCH 2/4] pinctrl/lantiq: introduce new dedicated devicetree bindings

[Martin Schiller <mschiller@tdt.de>]

On 11/21/2015 at 3:51 PM, Hauke Mehrtens wrote:
> On 11/20/2015 05:52 AM, Martin Schiller wrote:
> > This patch introduces new dedicated "lantiq,pinctrl-<chip>"
> devicetree
> > bindings, where <chip> is one of "ase", "danube", "xrx100", "xrx200"
> or
> > "xrx300" and marks the "lantiq,pinctrl-xway" and "lantiq,pinctrl-xr9"
> bindings as
> > DEPRECATED.

(snip)

> > diff --git a/drivers/pinctrl/pinctrl-lantiq.h
> b/drivers/pinctrl/pinctrl-lantiq.h
> > index eb89ba0..e137d13 100644
> > --- a/drivers/pinctrl/pinctrl-lantiq.h
> > +++ b/drivers/pinctrl/pinctrl-lantiq.h
> > @@ -162,6 +162,14 @@ enum ltq_pin {
> >  GPIO53,
> >  GPIO54,
> >  GPIO55,
> > +GPIO56,
> > +GPIO57,
> > +GPIO58,
> > +GPIO59,
> > +GPIO60, /* 60 */
> > +GPIO61,
> > +GPIO62,
> > +GPIO63,
> >
> >  GPIO64,
> >  GPIO65,
>
> What is the advantage of using this enum compared to just use integers
> directly? Was it intentionally that GPIO64 has the number 56 before?

Maybe this GPIO56 - GPIO63 gap has some relation to the xRX100 (aka ar9) SoC,
which has 3 1/2 GPIO Ports of 16bits (GPIO0 - GPIO55).


>
> > diff --git a/drivers/pinctrl/pinctrl-xway.c
> b/drivers/pinctrl/pinctrl-xway.c
> > index ae724bd..234d3f4 100644
> > --- a/drivers/pinctrl/pinctrl-xway.c
> > +++ b/drivers/pinctrl/pinctrl-xway.c
> > @@ -7,6 +7,7 @@
> >   *  publishhed by the Free Software Foundation.
> >   *
> >   *  Copyright (C) 2012 John Crispin <blogic@openwrt.org>
> > + *  Copyright (C) 2015 Martin Schiller <mschiller@tdt.de>
> >   */
> >
> >  #include <linux/err.h>
> > @@ -24,7 +25,7 @@
> >
> >  #include <lantiq_soc.h>
> >
> > -/* we have 3 1/2 banks of 16 bit each */
> > +/* we have up to 4 banks of 16 bit each */
> >  #define PINS16
> >  #define PORT33
> >  #define PORT(x)(x / PINS)
> > @@ -35,7 +36,7 @@
> >  #define MUX_ALT10x2
> >
> >  /*
> > - * each bank has this offset apart from the 1/2 bank that is mixed
> into the
> > + * each bank has this offset apart from the 4th bank that is mixed
> into the
> >   * other 3 ranges
> >   */
> >  #define REG_OFF0x30
> > @@ -51,7 +52,7 @@
> >  #define GPIO_PUDSEL(p)(GPIO_BASE(p) + 0x1c)
> >  #define GPIO_PUDEN(p)(GPIO_BASE(p) + 0x20)
> >
> > -/* the 1/2 port needs special offsets for some registers */
> > +/* the 4th port needs special offsets for some registers */
> >  #define GPIO3_OD(GPIO_BASE(0) + 0x24)
> >  #define GPIO3_PUDSEL(GPIO_BASE(0) + 0x28)
> >  #define GPIO3_PUDEN(GPIO_BASE(0) + 0x2C)
> > @@ -80,17 +81,18 @@
> >  #define FUNC_MUX(f, m)\
> >  { .func = f, .mux = XWAY_MUX_##m, }
> >

(snip)

> >  /* ---------  pinconf related code --------- */
> >  static int xway_pinconf_get(struct pinctrl_dev *pctldev,
> >  unsigned pin,
> > @@ -695,9 +1593,6 @@ static struct gpio_chip xway_chip = {
> >
> >
> >  /* --------- register the pinctrl layer --------- */
> > -static const unsigned xway_exin_pin_map[] = {GPIO0, GPIO1, GPIO2,
> GPIO39, GPIO46, GPIO9};
> > -static const unsigned ase_exin_pins_map[] = {GPIO6, GPIO29, GPIO0};
> > -
> >  static struct pinctrl_xway_soc {
> >  int pin_count;
> >  const struct ltq_mfp_pin *mfp;
> > @@ -708,21 +1603,41 @@ static struct pinctrl_xway_soc {
> >  const unsigned *exin;
> >  unsigned int num_exin;
> >  } soc_cfg[] = {
> > -/* legacy xway */
> > +/* legacy xway (DEPRECATED: Use XWAY DANUBE Family) */
> >  {XWAY_MAX_PIN, xway_mfp,
> >  xway_grps, ARRAY_SIZE(xway_grps),
> > -danube_funcs, ARRAY_SIZE(danube_funcs),
> > +legacy_danube_funcs, ARRAY_SIZE(legacy_danube_funcs),
> >  xway_exin_pin_map, 3},
>
> What is the difference between this entry and the new danube entry? Is
> this legay entry needed here our can we map that to the danube entry?
> If
> it is not needed you should probably change it directly in the
> xway_match table.

Yes, you are right. We could simply match the old "lantiq,pinctrl-xway"
compatible string to the new danube entry, when we also add the old "spi"
mux group (DEPRECATED in the ase code) here too.

>
> > -/* xway xr9 series */
> > +/* xway xr9 series (DEPRECATED: Use XWAY xRX100/xRX200 Family) */
> >  {XR9_MAX_PIN, xway_mfp,
> >  xway_grps, ARRAY_SIZE(xway_grps),
> >  xrx_funcs, ARRAY_SIZE(xrx_funcs),
> >  xway_exin_pin_map, 6},
> > -/* xway ase series */
> > -{XWAY_MAX_PIN, ase_mfp,
> > +/* XWAY AMAZON Family */
> > +{ASE_MAX_PIN, ase_mfp,
> >  ase_grps, ARRAY_SIZE(ase_grps),
> >  ase_funcs, ARRAY_SIZE(ase_funcs),
> > -ase_exin_pins_map, 3},
> > +ase_exin_pin_map, 3},
> > +/* XWAY DANUBE Family */
> > +{DANUBE_MAX_PIN, danube_mfp,
> > +danube_grps, ARRAY_SIZE(danube_grps),
> > +danube_funcs, ARRAY_SIZE(danube_funcs),
> > +danube_exin_pin_map, 3},
> > +/* XWAY xRX100 Family */
> > +{XRX100_MAX_PIN, xrx100_mfp,
> > +xrx100_grps, ARRAY_SIZE(xrx100_grps),
> > +xrx100_funcs, ARRAY_SIZE(xrx100_funcs),
> > +xrx100_exin_pin_map, 6},
> > +/* XWAY xRX200 Family */
> > +{XRX200_MAX_PIN, xrx200_mfp,
> > +xrx200_grps, ARRAY_SIZE(xrx200_grps),
> > +xrx200_funcs, ARRAY_SIZE(xrx200_funcs),
> > +xrx200_exin_pin_map, 6},
> > +/* XWAY xRX300 Family */
> > +{XRX300_MAX_PIN, xrx300_mfp,
> > +xrx300_grps, ARRAY_SIZE(xrx300_grps),
> > +xrx300_funcs, ARRAY_SIZE(xrx300_funcs),
> > +xrx300_exin_pin_map, 5},
> >  };
> >
> >  static struct pinctrl_gpio_range xway_gpio_range = {
> > @@ -734,6 +1649,10 @@ static const struct of_device_id xway_match[] =
> {
> >  { .compatible = "lantiq,pinctrl-xway", .data = &soc_cfg[0]},
> >  { .compatible = "lantiq,pinctrl-xr9", .data = &soc_cfg[1]},
> >  { .compatible = "lantiq,pinctrl-ase", .data = &soc_cfg[2]},
> > +{ .compatible = "lantiq,pinctrl-danube", .data = &soc_cfg[3]},
> > +{ .compatible = "lantiq,pinctrl-xrx100", .data = &soc_cfg[4]},
> > +{ .compatible = "lantiq,pinctrl-xrx200", .data = &soc_cfg[5]},
> > +{ .compatible = "lantiq,pinctrl-xrx300", .data = &soc_cfg[6]},
>
> Using pointers to an array elemnt here looks error prone to me. Why not
> use one static entry for each SoC and reference it directly.
>
> Something like this:
>
> static struct pinctrl_xway_soc pinctrl_xrx300 = {
> XRX300_MAX_PIN, xrx300_mfp,
> xrx300_grps, ARRAY_SIZE(xrx300_grps),
> xrx300_funcs, ARRAY_SIZE(xrx300_funcs),
> xrx300_exin_pin_map, 5
> };
>
> Then you can just do this:
> { .compatible = "lantiq,pinctrl-xrx300", .data = &pinctrl_xrx300},
>

Good point, I will change that.

Martin