On Tue, Aug 21, 2012 at 9:22 PM, Thomas Abraham
<thomas.abraham@xxxxxxxxxx> wrote:
>>> + - 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.
>> 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.
>>> + pinctrl_0: pinctrl@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@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.
>>> +/* 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.
>>> + /* 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.
>>> +/* 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.
>>> +static int __devinit samsung_pinctrl_probe(struct platform_device *pdev)
>> (...)
>>> + 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.
>>> + 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 :-)
>>> +/**
>>> + * 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.
>>> + 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.
>>> +/**
>>> + * 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
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
