Re: [U-Boot] [RFC v3 PATCH 3/4] pinctrl: add simple pinctrl implementation

2015-08-25 Thread Masahiro Yamada 
Hi Simon,


2015-08-12 23:16 GMT+09:00 Simon Glass s...@chromium.org:

 diff --git a/include/dm/pinctrl.h b/include/dm/pinctrl.h
 index 42008da..b2ba0b4 100644
 --- a/include/dm/pinctrl.h
 +++ b/include/dm/pinctrl.h
 @@ -85,6 +85,8 @@ struct pinctrl_ops {
 int (*pinconf_group_set)(struct udevice *dev, unsigned 
 group_selector,
  unsigned param, unsigned argument);
 int (*set_state)(struct udevice *dev, struct udevice *config);
 +   /* for pinctrl-simple */
 +   int (*set_state_simple)(struct udevice *dev, struct udevice *periph);

 So should the other members be #idef'd out? Also, comments on this function?


After my careful consideration, I did not do this.

If we do this,the corresponding members in all drivers must be also
#ifdef'd out,
including full-pinctrl drivers that do not care about memory footprint.

I do not like adding #ifdefs around to fix build errors found with
make allyesconfig, make randconfig.

I think it is a general strategy to not #ifdef out struct members.



-- 
Best Regards
Masahiro Yamada
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [RFC v3 PATCH 3/4] pinctrl: add simple pinctrl implementation

2015-08-12 Thread Simon Glass 
Hi Masahiro,

On 10 August 2015 at 10:05, Masahiro Yamada
yamada.masah...@socionext.com wrote:
 The full pinctrl implementation added by the previous commit can
 support the same DT bindings as Linux, but it typically needs about
 1.5 KB footprint to include the uclass support (CONFIG_PINCTRL +
 CONFIG_PINCTRL_GENERIC + CONFIG_PINMUX on ARM architecture).

 We are often limited on code size for SPL.  Besides, we still have
 many boards that do not support device tree configuration.  The full
 pinctrl, which requires OF_CONTROL, does not make sense for those
 boards.

 So, we need much more ad-hoc, smaller implementation, providing one
 operation, set_state_simple.  This callback takes two arguments,
 a pinctrl device and a peripheral device on which the operation
 should be done.  The uclass provides no mechanism to identify the
 target peripheral device, so set_state_simple must do it on its own.
 Probably, base addresses, interrupt numbers, etc. would be used for
 that purpose, but it is totally up to the implementation of each
 low-level driver.

 There are some more limitations worth mentioning.  The pinctrl
 device should generally be specified by a phandle in DTS, but this
 uclass itself does not parse device tree at all, i.e. there is no
 way to know the pinctrl device that takes care of the peripheral
 devices.  This simple uclass assumes the first pinctrl device is the
 correct one.  This is practically no problem because almost all
 systems have only one pinctrl device.  Another limitation is that
 it can not handle multiple states.  This simplification should be
 also OK since most of systems only uses default state during the
 boot stage.

 Signed-off-by: Simon Glass s...@chromium.org
 Signed-off-by: Masahiro Yamada yamada.masah...@socionext.com
 Suggested-by: Simon Glass s...@chromium.org
 ---

  drivers/pinctrl/Kconfig  | 12 ++--
  drivers/pinctrl/pinctrl-uclass.c | 29 +
  include/dm/pinctrl.h |  2 ++
  3 files changed, 41 insertions(+), 2 deletions(-)

 diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
 index 3f4f4b3..eca83fe 100644
 --- a/drivers/pinctrl/Kconfig
 +++ b/drivers/pinctrl/Kconfig
 @@ -7,9 +7,13 @@ menu Pin controllers
  config PINCTRL
 bool Support pin controllers

 +config PINCTRL_SIMPLE
 +   bool Support simple pin controllers
 +   depends on PINCTRL
 +
  config PINCTRL_GENERIC
 bool Support generic pin controllers
 -   depends on PINCTRL
 +   depends on PINCTRL  !PINCTRL_SIMPLE

  config PINMUX
 bool Support pin multiplexing controllers
 @@ -23,9 +27,13 @@ config SPL_PINCTRL
 bool Support pin controlloers in SPL
 depends on SPL

 +config SPL_PINCTRL_SIMPLE
 +   bool Support simple pin controlloers in SPL
 +   depends on SPL_PINCTRL
 +
  config SPL_PINCTRL_GENERIC
 bool Support generic pin controllers in SPL
 -   depends on SPL_PINCTRL
 +   depends on SPL_PINCTRL  !SPL_PINCTRL_SIMPLE

  config SPL_PINMUX
 bool Support pin multiplexing controllers in SPL
 diff --git a/drivers/pinctrl/pinctrl-uclass.c 
 b/drivers/pinctrl/pinctrl-uclass.c
 index ab3c146..853d779 100644
 --- a/drivers/pinctrl/pinctrl-uclass.c
 +++ b/drivers/pinctrl/pinctrl-uclass.c
 @@ -15,6 +15,34 @@

  DECLARE_GLOBAL_DATA_PTR;

 +#if CONFIG_IS_ENABLED(PINCTRL_SIMPLE)
 +
 +static int pinctrl_select_state(struct udevice *dev, const char *ignored)
 +{
 +   struct udevice *pctldev;
 +   struct pinconf_ops *ops;
 +   int ret;
 +
 +   ret = uclass_get_device(UCLASS_PINCTRL, 0, pctldev);
 +   if (ret)
 +   return ret;
 +
 +   ops = pctldev-driver-ops;
 +   if (!ops || !ops-set_state_simple) {
 +   dev_err(dev, set_state_simple op missing\n);
 +   return -EINVAL;
 +   }
 +
 +   return ops-set_state_simple(pctldev, dev);
 +}
 +
 +UCLASS_DRIVER(pinctrl) = {
 +   .id = UCLASS_PINCTRL,
 +   .name = pinctrl,
 +};
 +
 +#else
 +
  static int pinctrl_config_one(struct udevice *config)
  {
 struct udevice *pctldev;
 @@ -149,3 +177,4 @@ U_BOOT_DRIVER(pinconfig_generic) = {
 .name = pinconfig,
 .id = UCLASS_PINCONFIG,
  };
 +#endif
 diff --git a/include/dm/pinctrl.h b/include/dm/pinctrl.h
 index 42008da..b2ba0b4 100644
 --- a/include/dm/pinctrl.h
 +++ b/include/dm/pinctrl.h
 @@ -85,6 +85,8 @@ struct pinctrl_ops {
 int (*pinconf_group_set)(struct udevice *dev, unsigned group_selector,
  unsigned param, unsigned argument);
 int (*set_state)(struct udevice *dev, struct udevice *config);
 +   /* for pinctrl-simple */
 +   int (*set_state_simple)(struct udevice *dev, struct udevice *periph);

So should the other members be #idef'd out? Also, comments on this function?

  };

  /**
 --
 1.9.1


Regards,
Simon
___
U-Boot mailing list
U-Boot@lists.denx.de

[U-Boot] [RFC v3 PATCH 3/4] pinctrl: add simple pinctrl implementation

2015-08-10 Thread Masahiro Yamada 
The full pinctrl implementation added by the previous commit can
support the same DT bindings as Linux, but it typically needs about
1.5 KB footprint to include the uclass support (CONFIG_PINCTRL +
CONFIG_PINCTRL_GENERIC + CONFIG_PINMUX on ARM architecture).

We are often limited on code size for SPL.  Besides, we still have
many boards that do not support device tree configuration.  The full
pinctrl, which requires OF_CONTROL, does not make sense for those
boards.

So, we need much more ad-hoc, smaller implementation, providing one
operation, set_state_simple.  This callback takes two arguments,
a pinctrl device and a peripheral device on which the operation
should be done.  The uclass provides no mechanism to identify the
target peripheral device, so set_state_simple must do it on its own.
Probably, base addresses, interrupt numbers, etc. would be used for
that purpose, but it is totally up to the implementation of each
low-level driver.

There are some more limitations worth mentioning.  The pinctrl
device should generally be specified by a phandle in DTS, but this
uclass itself does not parse device tree at all, i.e. there is no
way to know the pinctrl device that takes care of the peripheral
devices.  This simple uclass assumes the first pinctrl device is the
correct one.  This is practically no problem because almost all
systems have only one pinctrl device.  Another limitation is that
it can not handle multiple states.  This simplification should be
also OK since most of systems only uses default state during the
boot stage.

Signed-off-by: Simon Glass s...@chromium.org
Signed-off-by: Masahiro Yamada yamada.masah...@socionext.com
Suggested-by: Simon Glass s...@chromium.org
---

 drivers/pinctrl/Kconfig  | 12 ++--
 drivers/pinctrl/pinctrl-uclass.c | 29 +
 include/dm/pinctrl.h |  2 ++
 3 files changed, 41 insertions(+), 2 deletions(-)

diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
index 3f4f4b3..eca83fe 100644
--- a/drivers/pinctrl/Kconfig
+++ b/drivers/pinctrl/Kconfig
@@ -7,9 +7,13 @@ menu Pin controllers
 config PINCTRL
bool Support pin controllers
 
+config PINCTRL_SIMPLE
+   bool Support simple pin controllers
+   depends on PINCTRL
+
 config PINCTRL_GENERIC
bool Support generic pin controllers
-   depends on PINCTRL
+   depends on PINCTRL  !PINCTRL_SIMPLE
 
 config PINMUX
bool Support pin multiplexing controllers
@@ -23,9 +27,13 @@ config SPL_PINCTRL
bool Support pin controlloers in SPL
depends on SPL
 
+config SPL_PINCTRL_SIMPLE
+   bool Support simple pin controlloers in SPL
+   depends on SPL_PINCTRL
+
 config SPL_PINCTRL_GENERIC
bool Support generic pin controllers in SPL
-   depends on SPL_PINCTRL
+   depends on SPL_PINCTRL  !SPL_PINCTRL_SIMPLE
 
 config SPL_PINMUX
bool Support pin multiplexing controllers in SPL
diff --git a/drivers/pinctrl/pinctrl-uclass.c b/drivers/pinctrl/pinctrl-uclass.c
index ab3c146..853d779 100644
--- a/drivers/pinctrl/pinctrl-uclass.c
+++ b/drivers/pinctrl/pinctrl-uclass.c
@@ -15,6 +15,34 @@
 
 DECLARE_GLOBAL_DATA_PTR;
 
+#if CONFIG_IS_ENABLED(PINCTRL_SIMPLE)
+
+static int pinctrl_select_state(struct udevice *dev, const char *ignored)
+{
+   struct udevice *pctldev;
+   struct pinconf_ops *ops;
+   int ret;
+
+   ret = uclass_get_device(UCLASS_PINCTRL, 0, pctldev);
+   if (ret)
+   return ret;
+
+   ops = pctldev-driver-ops;
+   if (!ops || !ops-set_state_simple) {
+   dev_err(dev, set_state_simple op missing\n);
+   return -EINVAL;
+   }
+
+   return ops-set_state_simple(pctldev, dev);
+}
+
+UCLASS_DRIVER(pinctrl) = {
+   .id = UCLASS_PINCTRL,
+   .name = pinctrl,
+};
+
+#else
+
 static int pinctrl_config_one(struct udevice *config)
 {
struct udevice *pctldev;
@@ -149,3 +177,4 @@ U_BOOT_DRIVER(pinconfig_generic) = {
.name = pinconfig,
.id = UCLASS_PINCONFIG,
 };
+#endif
diff --git a/include/dm/pinctrl.h b/include/dm/pinctrl.h
index 42008da..b2ba0b4 100644
--- a/include/dm/pinctrl.h
+++ b/include/dm/pinctrl.h
@@ -85,6 +85,8 @@ struct pinctrl_ops {
int (*pinconf_group_set)(struct udevice *dev, unsigned group_selector,
 unsigned param, unsigned argument);
int (*set_state)(struct udevice *dev, struct udevice *config);
+   /* for pinctrl-simple */
+   int (*set_state_simple)(struct udevice *dev, struct udevice *periph);
 };
 
 /**
-- 
1.9.1

___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot