Re: [PATCH v2 4/6] ARM: Samsung: Modify s3c64xx_spi{0|1|2}_set_platdata function
On 1 June 2012 18:09, Mark Brown broo...@opensource.wolfsonmicro.com wrote: On Fri, Jun 01, 2012 at 10:47:44AM +0800, Thomas Abraham wrote: 1. There is one instance of 'struct platform_device' for each of the spi controller instances (0/1/2) named s3c64xx-spi (in arch/arm/plat-samsung/devs.c). Right, which looks rather like it is specific to s3c64xx at least given the naming... there aren't any current in tree users, though I do have one out of tree user on s3c6410. If this is supposed to be used on the later SoCs too then I guess it ought to be renamed anyway since what's there looks wrong. The spi platform device is not really s3c64xx specific. It can be reused on all other Samsung SoC's as well, but there are no users of this except for s3c64xx SoC. The resources for this platform devices are statically bound for a SoC during compile time. I would not consider the existing spi platform device instance name (s3c64xx_device_spi0) or the .name (s3c64xx-spi) to be factor in reusing it on other Samsung SoC's. 5. If point 4 is not correct, the other option is to create a separate instances of 'struct platform_device' for each of the s3c, s5p and Exynos4/5 SoC's. Is this the correct way, and if yes, could you please help me understand the issues in setting the name of the platform device at runtime. That would seem less painful to me. The big problem I've found is with things like matching clocks up where if you look at the device that's being registered it has one name but at runtime it has another name, it makes everything much harder to follow. There was one of the devices where the .id also ended up getting changed which created even more confusion. Yes, I agree with you that changing device names at runtime can be confusing at times. Probably, additional comments to indicate possible runtime changes in device names could be useful. For now, I will avoid runtime setting of platform device name as you have suggested. Thank your for providing clarification for my questions. Regards, Thomas. -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 4/6] ARM: Samsung: Modify s3c64xx_spi{0|1|2}_set_platdata function
On Fri, Jun 01, 2012 at 10:47:44AM +0800, Thomas Abraham wrote: 1. There is one instance of 'struct platform_device' for each of the spi controller instances (0/1/2) named s3c64xx-spi (in arch/arm/plat-samsung/devs.c). Right, which looks rather like it is specific to s3c64xx at least given the naming... there aren't any current in tree users, though I do have one out of tree user on s3c6410. If this is supposed to be used on the later SoCs too then I guess it ought to be renamed anyway since what's there looks wrong. 5. If point 4 is not correct, the other option is to create a separate instances of 'struct platform_device' for each of the s3c, s5p and Exynos4/5 SoC's. Is this the correct way, and if yes, could you please help me understand the issues in setting the name of the platform device at runtime. That would seem less painful to me. The big problem I've found is with things like matching clocks up where if you look at the device that's being registered it has one name but at runtime it has another name, it makes everything much harder to follow. There was one of the devices where the .id also ended up getting changed which created even more confusion. signature.asc Description: Digital signature
Re: [PATCH v2 4/6] ARM: Samsung: Modify s3c64xx_spi{0|1|2}_set_platdata function
On Thu, May 31, 2012 at 10:05:42AM +0800, Thomas Abraham wrote: On 30 May 2012 18:13, Mark Brown broo...@opensource.wolfsonmicro.com wrote: No there isn't. You've got things like s3c64xx_device_spi0 in arch/arm/plat-samsung/devs.c (which you'd expect since the resources that are passed in for memory mapping, DMA and interrupt vary with the SoC). The bit of code I was querying just changes s3c64xx-spi to s3c6410-spi at runtime in that structure which seems like a waste of time. So is the concern only with the change of device name from s3c64xx-spi to s3c6410-spi? Is there any concern with changing the name of the static spi platform device (s3c64xx_device_spi0/1/2) at runtime which then is used to select a driver data? No, you're not getting it at all. The changing at runtime is the problem, it's achieving nothing except making the code more fragile and obscure. Those devices will always come out with exactly the same name so we should just assign that name statically. signature.asc Description: Digital signature
Re: [PATCH v2 4/6] ARM: Samsung: Modify s3c64xx_spi{0|1|2}_set_platdata function
On 31 May 2012 19:36, Mark Brown broo...@opensource.wolfsonmicro.com wrote: On Thu, May 31, 2012 at 10:05:42AM +0800, Thomas Abraham wrote: On 30 May 2012 18:13, Mark Brown broo...@opensource.wolfsonmicro.com wrote: No there isn't. You've got things like s3c64xx_device_spi0 in arch/arm/plat-samsung/devs.c (which you'd expect since the resources that are passed in for memory mapping, DMA and interrupt vary with the SoC). The bit of code I was querying just changes s3c64xx-spi to s3c6410-spi at runtime in that structure which seems like a waste of time. So is the concern only with the change of device name from s3c64xx-spi to s3c6410-spi? Is there any concern with changing the name of the static spi platform device (s3c64xx_device_spi0/1/2) at runtime which then is used to select a driver data? No, you're not getting it at all. The changing at runtime is the problem, it's achieving nothing except making the code more fragile and obscure. Those devices will always come out with exactly the same name so we should just assign that name statically. Yes, I am not able to understand your point. So I am listing out my understanding of the issue here. Please let me know which of these you think needs to be changed. 1. There is one instance of 'struct platform_device' for each of the spi controller instances (0/1/2) named s3c64xx-spi (in arch/arm/plat-samsung/devs.c). 2. These instances of platform device is then statically assigned the resources (mem/irq/dma) based on the SoC for which it is being compiled (IRQ_SPI0, S3C_PA_SPI0, DMACH_SPI0_TX and DMACH_SPI0_RX). These macros have different values for different SoC's. 3. Board files based on Samsung SoC's includes the static 'struct platform_device' for spi in the list of platform devices that are registered. Which means, the spi platform device is registered with the name s3c64xx-spi for all s3c, s5p and Exynos series SoC's. 4. So, in order to use a struct platform_device_id table in the spi driver (drivers/spi/spi-s3c64xx.c), the '.name' in spi's struct platform_device is changed at runtime, to indicate the SoC on which it is being used, and correspondingly, match with one of the entries in the struct platform_device_id table in the driver. 5. If point 4 is not correct, the other option is to create a separate instances of 'struct platform_device' for each of the s3c, s5p and Exynos4/5 SoC's. Is this the correct way, and if yes, could you please help me understand the issues in setting the name of the platform device at runtime. Sorry to prolong this for so long, but I want to understand your point on this. Thanks, Thomas. -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 4/6] ARM: Samsung: Modify s3c64xx_spi{0|1|2}_set_platdata function
On Sun, May 20, 2012 at 2:21 AM, Mark Brown broo...@opensource.wolfsonmicro.com wrote: On Fri, May 18, 2012 at 03:03:31PM +0530, Thomas Abraham wrote: - s3c64xx_spi0_set_platdata(s3c64xx_spi0_pdata, 0, 1); + s3c64xx_spi0_set_platdata(s3c6410-spi, NULL, 0, 1); ... + pd.src_clk_nr = src_clk_nr; + pd.cfg_gpio = (cfg_gpio) ? cfg_gpio : s3c64xx_spi0_cfg_gpio; + s3c64xx_device_spi0.name = dev_name; This looks *really* strange. Why do we need to pass in a name to set in s3c64xx_device_spi0's name, why would we want to use a different name? This dev_name also isn't equivalent to dev_name() which makes matters more confusing than they need to be. There was similar code for the I2C controllers which caused some really hard to debug brittleness. Looks like they use the name as the magic string to pick initialization data. I wonder if it makes more sense to keep the platform_data still around, and just fill it in with the table (from patch spi: s3c64xx: move controller information into driver data) based on the OF compatible field for the device-tree probe case, instead of trying to overload and having to change the name in this way. -Olof -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 4/6] ARM: Samsung: Modify s3c64xx_spi{0|1|2}_set_platdata function
On 30 May 2012 15:28, Olof Johansson o...@lixom.net wrote: On Sun, May 20, 2012 at 2:21 AM, Mark Brown broo...@opensource.wolfsonmicro.com wrote: On Fri, May 18, 2012 at 03:03:31PM +0530, Thomas Abraham wrote: - s3c64xx_spi0_set_platdata(s3c64xx_spi0_pdata, 0, 1); + s3c64xx_spi0_set_platdata(s3c6410-spi, NULL, 0, 1); ... + pd.src_clk_nr = src_clk_nr; + pd.cfg_gpio = (cfg_gpio) ? cfg_gpio : s3c64xx_spi0_cfg_gpio; + s3c64xx_device_spi0.name = dev_name; This looks *really* strange. Why do we need to pass in a name to set in s3c64xx_device_spi0's name, why would we want to use a different name? This dev_name also isn't equivalent to dev_name() which makes matters more confusing than they need to be. There was similar code for the I2C controllers which caused some really hard to debug brittleness. Looks like they use the name as the magic string to pick initialization data. I wonder if it makes more sense to keep the platform_data still around, and just fill it in with the table (from patch spi: s3c64xx: move controller information into driver data) based on the OF compatible field for the device-tree probe case, instead of trying to overload and having to change the name in this way. Ok. Thanks for your suggestion. So for now, the spi controller configuration data will be left in platform data for non-dt platforms and allow dt-platforms to have this data as part of the driver (and picked up based on the compatible value). Thanks, Thomas. -Olof -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 4/6] ARM: Samsung: Modify s3c64xx_spi{0|1|2}_set_platdata function
On Wed, May 30, 2012 at 12:28:55AM -0700, Olof Johansson wrote: On Sun, May 20, 2012 at 2:21 AM, Mark Brown This dev_name also isn't equivalent to dev_name() which makes matters more confusing than they need to be. Looks like they use the name as the magic string to pick initialization data. Right, and there's no problem at all with using the name. The thing is that there's no need to set the name at runtime since the struct device being configured is always going to end up with the same name and doing so is just causing confusion. The device being registered is specific to the SoC already so setting the SoC name at runtime isn't needed. I wonder if it makes more sense to keep the platform_data still around, and just fill it in with the table (from patch spi: s3c64xx: move controller information into driver data) based on the OF compatible field for the device-tree probe case, instead of trying to overload and having to change the name in this way. We could do that, though it does seem nice to have everything be consistent. signature.asc Description: Digital signature
Re: [PATCH v2 4/6] ARM: Samsung: Modify s3c64xx_spi{0|1|2}_set_platdata function
On 30 May 2012 17:34, Mark Brown broo...@opensource.wolfsonmicro.com wrote: On Wed, May 30, 2012 at 12:28:55AM -0700, Olof Johansson wrote: On Sun, May 20, 2012 at 2:21 AM, Mark Brown This dev_name also isn't equivalent to dev_name() which makes matters more confusing than they need to be. Looks like they use the name as the magic string to pick initialization data. Right, and there's no problem at all with using the name. The thing is that there's no need to set the name at runtime since the struct device being configured is always going to end up with the same name and doing so is just causing confusion. The device being registered is specific to the SoC already so setting the SoC name at runtime isn't needed. I think I did not understand your point. There is only one instance of spi platform device statically defined for all Samsung platforms. The name of this device is set as s3c64xx-spi. Now, to choose a particular driver data (for a platform) based on the device name, the device name has to be changed during boot to indicate the platform. If this not a good way, then the other way to choose driver data based on device name is to statically define platform device for each of the samsung platforms. This will add more lines of code into arch/arm and can be avoided. If all the Samsung platforms had device tree support enabled, this would not have been a problem. I wonder if it makes more sense to keep the platform_data still around, and just fill it in with the table (from patch spi: s3c64xx: move controller information into driver data) based on the OF compatible field for the device-tree probe case, instead of trying to overload and having to change the name in this way. We could do that, though it does seem nice to have everything be consistent. So, for now, I will not change the existing platform data. But add driver data (spi controller configuration) only for dt enabled platforms. And use the spi controller configuration found in the driver data instead of the platform data. Thanks Mark for your comments. Regards, Thomas. -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 4/6] ARM: Samsung: Modify s3c64xx_spi{0|1|2}_set_platdata function
On Wed, May 30, 2012 at 06:05:31PM +0800, Thomas Abraham wrote: On 30 May 2012 17:34, Mark Brown broo...@opensource.wolfsonmicro.com wrote: Right, and there's no problem at all with using the name. The thing is that there's no need to set the name at runtime since the struct device being configured is always going to end up with the same name and doing so is just causing confusion. The device being registered is specific to the SoC already so setting the SoC name at runtime isn't needed. I think I did not understand your point. There is only one instance of spi platform device statically defined for all Samsung platforms. The No there isn't. You've got things like s3c64xx_device_spi0 in arch/arm/plat-samsung/devs.c (which you'd expect since the resources that are passed in for memory mapping, DMA and interrupt vary with the SoC). The bit of code I was querying just changes s3c64xx-spi to s3c6410-spi at runtime in that structure which seems like a waste of time. signature.asc Description: Digital signature
Re: [PATCH v2 4/6] ARM: Samsung: Modify s3c64xx_spi{0|1|2}_set_platdata function
On 30 May 2012 18:13, Mark Brown broo...@opensource.wolfsonmicro.com wrote: On Wed, May 30, 2012 at 06:05:31PM +0800, Thomas Abraham wrote: On 30 May 2012 17:34, Mark Brown broo...@opensource.wolfsonmicro.com wrote: Right, and there's no problem at all with using the name. The thing is that there's no need to set the name at runtime since the struct device being configured is always going to end up with the same name and doing so is just causing confusion. The device being registered is specific to the SoC already so setting the SoC name at runtime isn't needed. I think I did not understand your point. There is only one instance of spi platform device statically defined for all Samsung platforms. The No there isn't. You've got things like s3c64xx_device_spi0 in arch/arm/plat-samsung/devs.c (which you'd expect since the resources that are passed in for memory mapping, DMA and interrupt vary with the SoC). The bit of code I was querying just changes s3c64xx-spi to s3c6410-spi at runtime in that structure which seems like a waste of time. So is the concern only with the change of device name from s3c64xx-spi to s3c6410-spi? Is there any concern with changing the name of the static spi platform device (s3c64xx_device_spi0/1/2) at runtime which then is used to select a driver data? Thanks, Thomas. -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
