Re: [PATCH 5/9] ARM: at91/dt: add mmc0 slot0 support to at91rm9200ek board

[boris brezillon <b.brezillon@overkiz.com>]

Hello Linus,

On 29/11/2013 11:03, Linus Walleij wrote:
> På tisdag, 26 Nov, 2013 vid 6:11 PM, skrev boris brezillon
> <b.brezillon@overkiz.com>:
>> Le 26/11/2013 14:46, Linus Walleij a écrit :
>>> But in this case it is a mechanical switch rather than a jumper?
>> Not exactly.
>>
>> The functionnaly selection (spi device or mmc slot) is done by the software
>> using to the
>> PB22 pin:
>>   - set PB22 pin to 1 if you want to enable the mmc slot
>>   - set PB22 pin to 0 if you want to enable the spi device
> OK I got it wrong, I thought it was a mechanical switch. So this is a GPIO
> line somekindof, you control it like this, and it will have effects on the
> electronics.
>
> So to get the device running you need to both:
>
> - Switch the value of this GPIO line.
> - Switch pin control state.
>
> We are certain that the gpio_set_value() shall be used to set that GPIO
> line, because it does not control any pin control logic, it controls
> some electronics outside of the SoC, and that is outside the pin
> controller domain.
>
> I guess one way is to obtain this GPIO in board code and just
> flick it depending on which device you register.
>
> You can have GPIOs tied to the machine/board itself, see this
> fragment from arch/arm/boot/dts/ste-nomadik-s8815.dts:
>
>          /* Custom board node with GPIO pins to active etc */
>          usb-s8815 {
>                  /* This will bias the MMC/SD card detect line */
>                  mmcsd-gpio {
>                          gpios = <&gpio3 16 0x1>;
>                  };
>          };
>
> This GPIO needs to be driven high to bias the MMC/SD card.
> I solved it like this in the board code in
> arch/arm/mach-nomadik/cpu-8815.c:
>
> /*
>   * This GPIO pin turns on a line that is used to detect card insertion
>   * on this board.
>   */
> static int __init cpu8815_mmcsd_init(void)
> {
>          struct device_node *cdbias;
>          int gpio, err;
>
>          cdbias = of_find_node_by_path("/usb-s8815/mmcsd-gpio");
>          if (!cdbias) {
>                  pr_info("could not find MMC/SD card detect bias node\n");
>                  return 0;
>          }
>          gpio = of_get_gpio(cdbias, 0);
>          if (gpio < 0) {
>                  pr_info("could not obtain MMC/SD card detect bias GPIO\n");
>                  return 0;
>          }
>          err = gpio_request(gpio, "card detect bias");
>          if (err) {
>                  pr_info("failed to request card detect bias GPIO %d\n", gpio);
>                  return -ENODEV;
>          }
>          err = gpio_direction_output(gpio, 0);
>          if (err){
>                  pr_info("failed to set GPIO %d as output, low\n", gpio);
>                  return err;
>          }
>          pr_info("enabled USB-S8815 CD bias GPIO %d, low\n", gpio);
>          return 0;
> }
> device_initcall(cpu8815_mmcsd_init);
>
> This is maybe not a perfect approach :-/
>
> But you get the idea. You could set this up one way or another
> depending on whether this board is registering a device for
> SPI or MMC.

The whole goal of moving from board files to dt is to drop all board
specific processing or initialization and only keep a common description
with generic drivers capable of handling common use cases.

I'm not sure providing new board specific drivers is a good solution
(even if it is the simplest way to achieve our goal).

Could we have something similar to pinctrl but with gpios :
when the device is probed the device/driver core code request the gpio
configure it appropriately and set it to the requested value (if configured
as output).

Or even better, provide an external switch subsystem (with a gpio-switch 
driver)
and automate switch request/config in device/driver core code (as done 
for the
pinctrl config).

These are just thoughts, and I guess introducing new code in the 
device/driver core
code is not that easy, especially when this code is here to handle 
specific case
like ours.

Thanks for sharing your thoughts.

Best Regards,

Boris

>
> Probably Jean-Christophe has opinions on this so let's see what
> he says.
>
>> If I understand correctly, you're suggesting to retrieve the PB22 pin value
>> to decide
>> wether the mmc slot or the spi device is enabled. Is that right ?
> Forget about this, I didn't understand the real problem.
>
>> In the board version this was configured in the init_machine function (or
>> board init
>> function) depending on the MTD_AT91_DATAFLASH_CARD
>> (
>> http://lxr.free-electrons.com/source/arch/arm/mach-at91/board-rm9200ek.c#L173).
>> As a result it was not reconfigurable at runtime.
>>
>> But Jean-Christophe suggested to make it configurable at runtime (using dt
>> fragments).
> It should definately be set up at runtime, just a matter where and
> how.
>
> Yours,
> Linus Walleij