[U-Boot] [PATCH v2] Add single register pin controller driver

[Felix Brack]

Hello Masahiro,

This is my understanding of the parameters 'dev' and 'perihp' passed to
the function implementing set_state_simple().

On 08.02.2017 04:02, Masahiro Yamada wrote:
> Hi.
> 
> 
> 
> 2017-02-07 22:30 GMT+09:00 Felix Brack <fb at ltec.ch>:
> 
>> +
>> +static int single_set_state_simple(struct udevice *dev,
>> +                                  struct udevice *periph)
>> +{
>> +       const void *fdt = gd->fdt_blob;
>> +       const struct single_fdt_pin_cfg *prop;
>> +       int len;
>> +
>> +       prop = fdt_getprop(fdt, periph->of_offset, "pinctrl-single,pins", &len);
> 
> 
> This seems wrong to me.
> 
> 
> The "periph" is a peripheral device (like UART, eMMC, USB, etc.).
> 

In the case above 'dev' represents the pin controller node of the DT and
'periph' represents the DT node holding the pin configuration
information for a specific peripheral like i2c0, not the peripheral
itself. I know that neither 'dev' nor 'periph' _are_ device tree nodes
objects. This is why I use the word 'represent'.

> So, you are parsing the "pinctrl-single,pins" property
> in the peripheral device, like
> 
>     uart: uart {
> 
>                   pinctrl-single,pins = <
>                           .....
>                   >;
> 
>     };
> 
> 

I don't think so (see below).

> 
> 
> In pinctrl, peripheral nodes should have a phandle to a child of the
> pinctrl device.
> As you see Linux, the DT should look like this:
> 
> 
>          uart: uart {
> 
>                pinctrl-0 = <&uart_pins>;
> 
>         };
> 
> 
>     pinctrl {
> 
>                                uart_pins: uart_pins {
>                                                   pinctrl-single,pins = <
>                                                                ....
>                                                   >;
>                                };
> 
>          };
> 
> 

This is how it works. single_set_state_simple() would be called with
'dev' representing 'pinctrl' node and 'periph' representing 'uart_pins'
node. I would have to parse 'periph' for 'pinctrl-single,pins'.

> 
> 

I use this pin controller on a AM335x based board. Here are the relevant
parts from the device tree.

>From 'am33xx.dtsi', representing the pin controller itself:

am33xx_pinmux: pinmux at 800 {
	compatible = "pinctrl-single";
	reg = <0x800 0x238>;
	...
}

>From a a device tree source file, representing the configuration of the
pins for the peripheral i2c0:

&am33xx_pinmux {
	pinctrl-names = "default";
	pinctrl-0 = <&i2c0_pins>;

	i2c0_pins: pinmux_i2c0_pins {
		pinctrl-single,pins = <
			AM33XX_IOPAD(0x988, PIN_INPUT_PULLUP | MUX_MODE0)	/* i2c0_sda.i2c0_sda */
			AM33XX_IOPAD(0x98c, PIN_INPUT_PULLUP | MUX_MODE0)	/* i2c0_scl.i2c0_scl */
		>;
	};
};

With this, single_set_state_simple() gets called with 'dev' representing
'pinmux at 800' and 'periph' representing 'pinmux_i2c0_pins' which makes
perfect sense to me.
Again I would have to parse 'periph' for 'pinctrl-single,pins'.


regards Felix