Re: EP93xx PIO IDE driver proposal

["João Ramos" <joao.ramos@inov.pt>]

Ryan Mallon escreveu:
> H Hartley Sweeten wrote:
>   
>> On Thursday, May 07, 2009 3:51 PM, Ryan Mallon wrote:
>>     
>>>>> You can (and should) use platform_driver_probe and still have
>>>>> the module be loadable. For the gpio settings, I think that
>>>>> probably the best approach is to use gpio_request for each of
>>>>> the gpio pins so that gpio_lib knows that they are in use and
>>>>> sysfs/debugfs will correctly show they are assigned to ide,
>>>>> and then call ep93xx_ide_on_gpio, so you will have something
>>>>> like in your init code:
>>>>>           

As for the platform_driver_probe, I will fix that then.

>>>> Hmmm...
>>>>
>>>> How about doing all the request/free's in the ep93xx_ide_on_gpio()
>>>> function?  That way the platform code can initially call it and set
>>>> all the pins into gpio mode (freeing them).  Then later when the
>>>> IDE driver is loaded it can call the function to try and put the
>>>> pins into IDE mode (requesting them).  If any of the pins are
>>>> unavailable the ep93xx_ide_on_gpio() function can return the
>>>> necessary error code preventing the IDE driver from loading.  When
>>>> the driver is unloaded it just needs to call the core to free
>>>> the gpio's.
>>>>
>>>> Something like:
>>>>
>>>> int ep93xx_ide_on_gpio(int enable)
>>>> {
>>>> 	u32 reg;
>>>> 	int err;
>>>> 	int i;
>>>>
>>>> 	reg = __raw_read(EP93XX_SYSCON_DEVICE_CONFIG);
>>>>
>>>> 	if (enable) {
>>>> 		for (i = 0; i < 8; i++) {
>>>> 			err = gpio_request(EP93XX_GPIO_LINE_E(i), "ep93xx_ide");
>>>> 			if (err)
>>>> 				goto fail_gpio_e;
>>>> 			err = gpio_request(EP93XX_GPIO_LINE_F(i), "ep93xx_ide");
>>>> 			if (err)
>>>> 				goto fail_gpio_f;
>>>> 			err = gpio_request(EP93XX_GPIO_LINE_G(i), "ep93xx_ide");
>>>> 			if (err)
>>>> 				goto fail_gpio_g;
>>>> 		}
>>>> 		reg &= ~(EP93XX_SYSCON_DEVICE_CONFIG_EONIDE |
>>>> 			EP93XX_SYSCON_DEVICE_CONFIG_GONIDE |
>>>> 			EP93XX_SYSCON_DEVICE_CONFIG_HONIDE);
>>>> 	} else {
>>>> 		for (i = 0; i < 8; i++) {
>>>> 			gpio_free(EP93XX_GPIO_LINE_E(i));
>>>> 			gpio_free(EP93XX_GPIO_LINE_F(i));
>>>> 			gpio_free(EP93XX_GPIO_LINE_G(i));
>>>> 		}
>>>> 		reg |= (EP93XX_SYSCON_DEVICE_CONFIG_EONIDE |
>>>> 			EP93XX_SYSCON_DEVICE_CONFIG_GONIDE |
>>>> 			EP93XX_SYSCON_DEVICE_CONFIG_HONIDE);
>>>> 	}
>>>> 	
>>>> 	__raw_writel(0xaa, EP93XX_SYSCON_SWLOCK);
>>>> 	__raw_writel(reg, EP93XX_SYSCON_DEVICE_CONFIG);	
>>>>         
>>> Is this going to be modified later to use the ep93xx_syscon_ functions
>>> once they are merged?
>>>       
>> Yes, hopefully that occurs sometime soon.  Still no responses on it.
>>     

I was hoping for that patch to be merged to use the functions provided 
to write Syscon locked registers.
In the meanwhile, as it isn't in the mainline kernel yet, I used this form.

>>     
>>>> 	return 0;
>>>>
>>>> fail_gpio_g:
>>>> 	free_gpio(EP93XX_GPIO_LINE_F(i);
>>>> fail_gpio_f:
>>>> 	free_gpio(EP93XX_GPIO_LINE_E(i);
>>>> fail_gpio_e:
>>>> 	for ( ; i >= 0; --i) {
>>>> 		free_gpio(EP93XX_GPIO_LINE_E(i);
>>>> 		free_gpio(EP93XX_GPIO_LINE_F(i);
>>>> 		free_gpio(EP93XX_GPIO_LINE_G(i);
>>>> 	}
>>>> 	return err;
>>>> }
>>>>         
>>> Yeah, that makes sense. Keeps the driver nice and clean that way too.
>>> The ep93xx_ide_on_gpio function for core.c should be posted as a
>>> separate patch though.
>>>
>>>       
>> I agree.  Actually that patch needs to go in now so that the port E/F/G
>> pins will work for normal GPIO.  The syscon register defaults at reset with
>> the bits cleared so the boot state has them set for IDE mode.  That drove
>> me nuts for quite a while...
>>
>> If wanted I will put together the necessary patch for this.
>>     
>
> That would be good. I think that we should default all of the pins to
> gpio mode on boot, and then drivers should call into core.c to put pins
> in alternative function mode where necessary.
>   

Hartley, will you handle this then, or do you want me to submit a 
separate patch for the renewed 'ep93xx_ide_on_gpio' function?
I can do this and test it already with the IDE patch to see if it's OK.

>   
>> This function will hopefully get a bit cleaner later.  I've been messing
>> with an addition to the GPIOLIB API to add support for "ports".  When/if
>> that goes in all the for() loops go away and you just need to do a
>> gpio_port_request(...) to grab the pins and gpio_port_free(...) to put
>> them back.
>>     
>
> It would be nice if gpiolib had some understanding of alternative
> function modes for gpios. However I'm not sure that it can be done in a
> generic way that will suit all architectures/chips.
>
>   
>> Ahh.. If only the days were longer....
>>     
>
> Agreed :-).
>
> ~Ryan
>
>   


-- 
************************************************************************

    João Ramos        <joao.ramos@inov.pt>

    INOV INESC Inovação - ESTG Leiria
    Escola Superior de Tecnologia e Gestão de Leiria
    Edíficio C1, Campus 2
    Morro do Lena, Alto do Vieiro
    Leiria
    2411-901 Leiria
    Portugal

    Tel: +351244843424
    Fax: +351244843424

************************************************************************


-------------------------------------------------------------------
List admin: http://lists.arm.linux.org.uk/mailman/listinfo/linux-arm-kernel
FAQ:        http://www.arm.linux.org.uk/mailinglists/faq.php
Etiquette:  http://www.arm.linux.org.uk/mailinglists/etiquette.php