[PATCH v2 1/4] pinctrl: add samsung pinctrl and gpiolib driver

On Tue, Aug 21, 2012 at 9:22 PM, Thomas Abraham
[off-list ref] wrote:

quoted

quoted

+  - samsung,pin-pud: Pull up/down configuration.
+  - samsung,pin-drv: Drive strength configuration.
+  - samsung,pin-pud-pdn: Pull up/down configuration in power down mode.
+  - samsung,pin-drv-pdn: Drive strength configuration in power down mode.

This looks a bit scary, as it seems to be orthogonal to the pin config
interface. I.e. this will be programmed "behind the back" of the
pin config system. However as long as the pin config implementation
reads back these things from the registers it will work, too.

These properties are converted to a PIN_MAP_TYPE_CONFIGS_GROUP map
type and stored in a instance of 'struct pinctrl_map'. These can be
read back from the registers and reverse-mapped as well.

OK

All the dt bindings defined and used in the Samsung pinctrl driver are
first translated into pinctrl subystem defined data structures and
then used. Hence, there are no register configurations done that skip
over the pinctrl subsystem (except for the gpio/wakeup interrupts).

OK fine.

quoted

In the U300 and Ux500 I explicitly use pin config hogs to set up
the pin configuration, and when we enter a state such as
"default" the mux setting and config settings are set from the
framework separately.

See for example:
arch/arm/mach-ux500/board-mop500-pins.c

This example is using platform data but it should be trivial to do with
device tree.

I think the Tegra also works this way. Can you elaborate on
why you need this static setup from the device tree instead
of using default states?

Sorry, I did not understand this question.

You answered above, no problem.

quoted

quoted

+       pinctrl_0: pinctrl at 11400000 {
+               compatible = "samsung,pinctrl-exynos4210";
+               reg = <0x11400000 0x1000>;
+               interrupts = <0 47 0>;
+
+               uart0_data: uart0-data {
+                       samsung,pins = "gpa0-0", "gpa0-1";
+                       samsung,pin-function = <2>;
+                       samsung,pin-pud = <0>;
+                       samsung,pin-drv = <0>;
+               };

This setup needs to be associated with a certain state, it's possible to
do in the code or directly in the device tree.

I.e. these settings for pin-pud and pin-drv needs to belong to a
certain pin config state, typically the state named "default"

Yes, I agree. So, for example, the uart device node would have

                    uart at 13800000 {
                               compatilble = " .... ";
                               <rest of the properties here>

                               pinctrl-names = "default";
                               pinctrl-0 - <&uart0_data>;
                    };

The uart driver during probe can then call

                   devm_pinctrl_get_select_default(&pdev->dev);

For the example above, this call will set the 'mux', 'pud' and 'drv'
values to gpa-0 and gpa-1 pins.

OK perfect, that's how it should work.

quoted

quoted

+/* list of all possible config options supported */
+struct pin_config {
+       char            *prop_cfg;
+       unsigned int    cfg_type;
+} pcfgs[] = {
+       { "samsung,pin-pud", PINCFG_TYPE_PUD },
+       { "samsung,pin-drv", PINCFG_TYPE_DRV },
+       { "samsung,pin-pud-pdn", PINCFG_TYPE_CON_PND },
+       { "samsung,pin-drv-pdn", PINCFG_TYPE_PUD_PND },
+};

Hmmmmm it looks very much like this controller could make use of
the generic pinconf library, but it's not mandatory so just a suggestion.

Ok. The last two entries in the above table are Samsung specific and
not covered by generic-pinconf. So, I am not sure if it can be added
to generic-pinconf.

What is so Samsung-specific about them?

If you tell us the electrical property of setting them we can figure out
if they should be generic or not...

For now, since you are not enforcing the use of
generic-pinconf, I will keep it the way it is now.

Sure that's OK.

quoted

quoted

+       /* Allocate memory for pin group name. The pin group name is derived
+        * from the node name from which these map entries are be created.
+        */
+       gname = kzalloc(strlen(np->name) + 4, GFP_KERNEL);

Why +4? I would have suspected +1 for the null terminator...

The name of the pin group is derived from the node name and hence
strlen(np->name). To this name, "-grp" is appended to imply that this
is a group. Hence +4 is used. I will replace +4 with probably
strlen(PINGRP_SUFFIX) where PINGRP_SUFFIX is defined as "-grp".

Aha OK I get it.

quoted

quoted

+/* reading pin pull up/down and driver strength settings not implemented */

OK why not? It seems very simple and straight-forward.
Just read the same registers and switch() then return...

Ok, I will do that. I did not see how those would be used and hence skipped it.

One good usecase is to look at the state of pins in debugfs.

quoted

quoted

+static int __devinit samsung_pinctrl_probe(struct platform_device *pdev)

(...)

quoted

+       if (ctrl->eint_gpio_init)
+               ctrl->eint_gpio_init(drvdata);
+       if (ctrl->eint_wkup_init)
+               ctrl->eint_wkup_init(drvdata);

So this stuff I'm doing in the default states instead.

These callbacks setup the irq domain and irq_chip for gpio and wakeup
interrupts. These are Samsung specific and are dealt with outside of
the pinctrl subsystem framework.

Aha I get it, OK.

quoted

quoted

+       unsigned int            eint_type;

Shouldn't this be some kund of enum if it denotes a type?

It was done to reduce adding new data types.

Oh I like new data types if they make the code easier to read
and reduce the risk for errors so just go ahead :-)

quoted

quoted

+/**
+ * struct samsung_pin_ctrl: represent a pin controller.
+ * @pin_banks: list of pin banks included in this controller.
+ * @nr_banks: number of pin banks.
+ * @base: starting system wide pin number.
+ * @nr_pins: number of pins supported by the controller.
+ * @nr_gint: number of external gpio interrupts supported.
+ * @nr_wint: number of external wakeup interrupts supported.
+ * @geint_con: offset of the ext-gpio controller registers.

If it's an offset why not name it geint_con_offset?

I wanted to keep the lines within 80 columns. Splitting a line into
two lines started making the code look unreadable.

OK no big deal.

quoted

quoted

+       int                     (*eint_gpio_init)(struct samsung_pinctrl_drv_data *);
+       int                     (*eint_wkup_init)(struct samsung_pinctrl_drv_data *);

I guess you need to set up these using auxdata?

No, these are populated by the platform (SoC) specific data that the
Samsung pinctrl driver gets during probe. Due to the differences in
the gpio and wakeup interrupt controllers on Samsung SoC's, the setup
and implementations of these  interrupts have been made SoC specific.
The pinctrl driver is responsible only for initiating the setup of the
gpio/wakeup interrupts.

I see, OK.

quoted

quoted

+/**
+ * struct samsung_pinctrl_drv_data: wrapper for holding driver data together.
+ * @virt_base: register base address of the controller.
+ * @dev: device instance representing the controller.
+ * @irq: interrpt number used by the controller to notify gpio interrupts.
+ * @ctrl: pin controller instance managed by the driver.
+ * @pctl: pin controller descriptor registered with the pinctrl subsystem.

Maybe name this pctl_desc then?

This name is used to in multiple places in the code and the longer the
name, there is always the case of the line spilling over 80
characters.

OK whatever... looking formward to next iteration!

Yours,
Linus Walleij