Re: [RFC] dm: uclass: add functions to get device by platdata
Hi Walter, On Fri, 8 May 2020 at 04:08, Walter Lozano wrote: > > Hi Simon, > > On 7/5/20 22:36, Simon Glass wrote: > > Hi Walter, > > > > On Wed, 6 May 2020 at 06:57, Walter Lozano > > wrote: > >> Hi Simon, > >> > >> On 23/4/20 00:38, Walter Lozano wrote: > >>> Hi Simon, > >>> > >>> On 21/4/20 14:36, Simon Glass wrote: > Hi Walter, > > On Fri, 17 Apr 2020 at 15:18, Walter Lozano > wrote: > > Hi Simon, > > > > On 9/4/20 16:36, Simon Glass wrote: > >> HI Walter, > >> > >> On Thu, 9 Apr 2020 at 12:57, Walter Lozano > >> wrote: > >>> Hi Simon, > >>> > >>> On 8/4/20 00:14, Simon Glass wrote: > Hi Walter, > > On Tue, 7 Apr 2020 at 14:05, Walter > Lozano wrote: > > Hi Simon, > > > > On 6/4/20 00:43, Simon Glass wrote: > >> Hi Walter, > >> > >> On Mon, 9 Mar 2020 at 12:27, Walter > >> Lozano wrote: > >>> Hi Simon > >>> > >>> On 6/3/20 17:32, Simon Glass wrote: > Hi Walter, > > On Fri, 6 Mar 2020 at 09:10, Walter > Lozano wrote: > > Hi Simon, > > > > Thanks again for taking the time to check my comments. > > > > On 6/3/20 10:17, Simon Glass wrote: > >> Hi Walter, > >> > >> On Thu, 5 Mar 2020 at 06:54, Walter > >> Lozano wrote: > >>> Hi Simon, > >>> > >>> Thanks for taking the time to check for my comments > >>> > >>> On 4/3/20 20:11, Simon Glass wrote: > >>> > Hi Walter, > > On Wed, 4 Mar 2020 at 12:40, Walter > Lozano wrote: > > When OF_PLATDATA is enabled DT information is parsed and > > platdata > > structures are populated. In this context the links > > between DT nodes are > > represented as pointers to platdata structures, and > > there is no clear way > > to access to the device which owns the structure. > > > > This patch implements a set of functions: > > > > - device_find_by_platdata > > - uclass_find_device_by_platdata > > > > to access to the device. > > > > Signed-off-by: Walter Lozano > > --- > > drivers/core/device.c| 19 > > +++ > > drivers/core/uclass.c| 34 > > ++ > > include/dm/device.h | 2 ++ > > include/dm/uclass-internal.h | 3 +++ > > include/dm/uclass.h | 2 ++ > > 5 files changed, 60 insertions(+) > This is interesting. Could you also add the motivation > for this? It's > not clear to me who would call this function. > >>> I have been reviewing the OF_PLATDATA support as an R&D > >>> project, in this context, in order to have > >>> a better understanding on the possibilities and > >>> limitations I decided to add its support to iMX6, > >>> more particularly to the MMC drivers. The link issue > >>> arises when I tried to setup the GPIO for > >>> Card Detection, which is trivial when DT is available. > >>> However, when OF_PLATDATA is enabled > >>> this seems, at least for me, not straightforward. > >>> > >>> In order to overcome this limitation I think that having a > >>> set of functions to find/get devices > >>> based on platdata could be useful. Of course, there might > >>> be a better approach/idea, so that is > >>> the motivation for this RFC. > >>> > >>> An example of the usage could be > >>> > >>> #if CONFIG_IS_ENABLED(DM_GPIO) > >>> > >>> struct udevice *gpiodev; > >>> > >>> ret = > >>> uclass_get_device_by_platdata(UCLASS_GPIO, (void > >>> *)dtplat->cd_gpios->node, &gpiodev); > >>> > >>> if (ret) > >>> return ret; > >>> > >>> ret = gpio_dev_request_index(gpiodev, > >>> gpiodev->name, "cd-gpios", > >>> dtplat->cd_gpios->arg[0], GPIOD_IS_IN, > >>> dtplat->cd_gpios->arg[1], &priv->cd_gpio); > >>> > >>> if (ret) > >>>
Re: [RFC] dm: uclass: add functions to get device by platdata
Hi Simon, On 7/5/20 22:36, Simon Glass wrote: Hi Walter, On Wed, 6 May 2020 at 06:57, Walter Lozano wrote: Hi Simon, On 23/4/20 00:38, Walter Lozano wrote: Hi Simon, On 21/4/20 14:36, Simon Glass wrote: Hi Walter, On Fri, 17 Apr 2020 at 15:18, Walter Lozano wrote: Hi Simon, On 9/4/20 16:36, Simon Glass wrote: HI Walter, On Thu, 9 Apr 2020 at 12:57, Walter Lozano wrote: Hi Simon, On 8/4/20 00:14, Simon Glass wrote: Hi Walter, On Tue, 7 Apr 2020 at 14:05, Walter Lozano wrote: Hi Simon, On 6/4/20 00:43, Simon Glass wrote: Hi Walter, On Mon, 9 Mar 2020 at 12:27, Walter Lozano wrote: Hi Simon On 6/3/20 17:32, Simon Glass wrote: Hi Walter, On Fri, 6 Mar 2020 at 09:10, Walter Lozano wrote: Hi Simon, Thanks again for taking the time to check my comments. On 6/3/20 10:17, Simon Glass wrote: Hi Walter, On Thu, 5 Mar 2020 at 06:54, Walter Lozano wrote: Hi Simon, Thanks for taking the time to check for my comments On 4/3/20 20:11, Simon Glass wrote: Hi Walter, On Wed, 4 Mar 2020 at 12:40, Walter Lozano wrote: When OF_PLATDATA is enabled DT information is parsed and platdata structures are populated. In this context the links between DT nodes are represented as pointers to platdata structures, and there is no clear way to access to the device which owns the structure. This patch implements a set of functions: - device_find_by_platdata - uclass_find_device_by_platdata to access to the device. Signed-off-by: Walter Lozano --- drivers/core/device.c| 19 +++ drivers/core/uclass.c| 34 ++ include/dm/device.h | 2 ++ include/dm/uclass-internal.h | 3 +++ include/dm/uclass.h | 2 ++ 5 files changed, 60 insertions(+) This is interesting. Could you also add the motivation for this? It's not clear to me who would call this function. I have been reviewing the OF_PLATDATA support as an R&D project, in this context, in order to have a better understanding on the possibilities and limitations I decided to add its support to iMX6, more particularly to the MMC drivers. The link issue arises when I tried to setup the GPIO for Card Detection, which is trivial when DT is available. However, when OF_PLATDATA is enabled this seems, at least for me, not straightforward. In order to overcome this limitation I think that having a set of functions to find/get devices based on platdata could be useful. Of course, there might be a better approach/idea, so that is the motivation for this RFC. An example of the usage could be #if CONFIG_IS_ENABLED(DM_GPIO) struct udevice *gpiodev; ret = uclass_get_device_by_platdata(UCLASS_GPIO, (void *)dtplat->cd_gpios->node, &gpiodev); if (ret) return ret; ret = gpio_dev_request_index(gpiodev, gpiodev->name, "cd-gpios", dtplat->cd_gpios->arg[0], GPIOD_IS_IN, dtplat->cd_gpios->arg[1], &priv->cd_gpio); if (ret) return ret; #endif This is part of my current work, a series of patches to add OF_PLATDATA support as explained. Does this make sense to you? Not yet :-) What is the context of this call? Typically dtplat is only available in the driver that includes it. Sorry for not being clear enough. I'm working in a patchset that needs some clean up, that is the reason I didn't send it yet. I'll try to clarify, but if you think it could be useful to share it, please let me know. What driver is the above code in? Is it for MMC that needs a GPIO to function? I'll assume it is for now. The driver on which I'm working in is drivers/mmc/fsl_esdhc_imx.c, I'm adding support for OF_PLATDATA to it, and in this sense trying to get the GPIOs used for CD to be requested. Then the weird thing is that we are accessing the dtplat of another device. It's a clever technique but I wonder if we can find another way. If you see drivers/mmc/rockchip_sdhci.c it has: ret = clk_get_by_index_platdata(dev, 0, dtplat->clocks, &clk); So I wonder if we need gpio_dev_request_by_platdata()? Thanks for pointing to this example, as I saw it before starting to work on these functions and had some doubts. I'll use it in the next paragraph to share my thoughts and the motivation of my work. From my understanding, clk_get_by_index_platdata in this context can only get a UCLASS_CLK device with id == 0. Is this correct? If it is so, this will only allow us to use this function if we know in advance that the UCLASS_CLK device has index 0. How can we get the correct UCLASS_CLK device in case of multiple instances? We actually can't support that at present. I think we would need to change the property to be an array, like: clocks = [ [&clk1, CLK_ID_SPI], [&clk1, CLK_ID_I2C, 23], ] which would need a fancier dtoc. Perhaps we should start by upstreaming that tool. In this case, are you sugges
Re: [RFC] dm: uclass: add functions to get device by platdata
Hi Walter, On Wed, 6 May 2020 at 06:57, Walter Lozano wrote: > > Hi Simon, > > On 23/4/20 00:38, Walter Lozano wrote: > > Hi Simon, > > > > On 21/4/20 14:36, Simon Glass wrote: > >> Hi Walter, > >> > >> On Fri, 17 Apr 2020 at 15:18, Walter Lozano > >> wrote: > >>> Hi Simon, > >>> > >>> On 9/4/20 16:36, Simon Glass wrote: > HI Walter, > > On Thu, 9 Apr 2020 at 12:57, Walter Lozano > wrote: > > Hi Simon, > > > > On 8/4/20 00:14, Simon Glass wrote: > >> Hi Walter, > >> > >> On Tue, 7 Apr 2020 at 14:05, Walter > >> Lozano wrote: > >>> Hi Simon, > >>> > >>> On 6/4/20 00:43, Simon Glass wrote: > Hi Walter, > > On Mon, 9 Mar 2020 at 12:27, Walter > Lozano wrote: > > Hi Simon > > > > On 6/3/20 17:32, Simon Glass wrote: > >> Hi Walter, > >> > >> On Fri, 6 Mar 2020 at 09:10, Walter > >> Lozano wrote: > >>> Hi Simon, > >>> > >>> Thanks again for taking the time to check my comments. > >>> > >>> On 6/3/20 10:17, Simon Glass wrote: > Hi Walter, > > On Thu, 5 Mar 2020 at 06:54, Walter > Lozano wrote: > > Hi Simon, > > > > Thanks for taking the time to check for my comments > > > > On 4/3/20 20:11, Simon Glass wrote: > > > >> Hi Walter, > >> > >> On Wed, 4 Mar 2020 at 12:40, Walter > >> Lozano wrote: > >>> When OF_PLATDATA is enabled DT information is parsed and > >>> platdata > >>> structures are populated. In this context the links > >>> between DT nodes are > >>> represented as pointers to platdata structures, and > >>> there is no clear way > >>> to access to the device which owns the structure. > >>> > >>> This patch implements a set of functions: > >>> > >>> - device_find_by_platdata > >>> - uclass_find_device_by_platdata > >>> > >>> to access to the device. > >>> > >>> Signed-off-by: Walter Lozano > >>> --- > >>> drivers/core/device.c| 19 > >>> +++ > >>> drivers/core/uclass.c| 34 > >>> ++ > >>> include/dm/device.h | 2 ++ > >>> include/dm/uclass-internal.h | 3 +++ > >>> include/dm/uclass.h | 2 ++ > >>> 5 files changed, 60 insertions(+) > >> This is interesting. Could you also add the motivation > >> for this? It's > >> not clear to me who would call this function. > > I have been reviewing the OF_PLATDATA support as an R&D > > project, in this context, in order to have > > a better understanding on the possibilities and > > limitations I decided to add its support to iMX6, > > more particularly to the MMC drivers. The link issue > > arises when I tried to setup the GPIO for > > Card Detection, which is trivial when DT is available. > > However, when OF_PLATDATA is enabled > > this seems, at least for me, not straightforward. > > > > In order to overcome this limitation I think that having a > > set of functions to find/get devices > > based on platdata could be useful. Of course, there might > > be a better approach/idea, so that is > > the motivation for this RFC. > > > > An example of the usage could be > > > > #if CONFIG_IS_ENABLED(DM_GPIO) > > > >struct udevice *gpiodev; > > > >ret = > > uclass_get_device_by_platdata(UCLASS_GPIO, (void > > *)dtplat->cd_gpios->node, &gpiodev); > > > >if (ret) > >return ret; > > > >ret = gpio_dev_request_index(gpiodev, > > gpiodev->name, "cd-gpios", > > dtplat->cd_gpios->arg[0], GPIOD_IS_IN, > > dtplat->cd_gpios->arg[1], &priv->cd_gpio); > > > >if (ret) > >return ret; > > > > #endif > > > > This is part of my current work, a series of patches to > > add OF_PLATDATA support as explained. > > > > Does this make sense to you? > Not yet :-) > > What is the context of this call? Typically dtplat is only > >>>
Re: [RFC] dm: uclass: add functions to get device by platdata
Hi Simon, On 23/4/20 00:38, Walter Lozano wrote: Hi Simon, On 21/4/20 14:36, Simon Glass wrote: Hi Walter, On Fri, 17 Apr 2020 at 15:18, Walter Lozano wrote: Hi Simon, On 9/4/20 16:36, Simon Glass wrote: HI Walter, On Thu, 9 Apr 2020 at 12:57, Walter Lozano wrote: Hi Simon, On 8/4/20 00:14, Simon Glass wrote: Hi Walter, On Tue, 7 Apr 2020 at 14:05, Walter Lozano wrote: Hi Simon, On 6/4/20 00:43, Simon Glass wrote: Hi Walter, On Mon, 9 Mar 2020 at 12:27, Walter Lozano wrote: Hi Simon On 6/3/20 17:32, Simon Glass wrote: Hi Walter, On Fri, 6 Mar 2020 at 09:10, Walter Lozano wrote: Hi Simon, Thanks again for taking the time to check my comments. On 6/3/20 10:17, Simon Glass wrote: Hi Walter, On Thu, 5 Mar 2020 at 06:54, Walter Lozano wrote: Hi Simon, Thanks for taking the time to check for my comments On 4/3/20 20:11, Simon Glass wrote: Hi Walter, On Wed, 4 Mar 2020 at 12:40, Walter Lozano wrote: When OF_PLATDATA is enabled DT information is parsed and platdata structures are populated. In this context the links between DT nodes are represented as pointers to platdata structures, and there is no clear way to access to the device which owns the structure. This patch implements a set of functions: - device_find_by_platdata - uclass_find_device_by_platdata to access to the device. Signed-off-by: Walter Lozano --- drivers/core/device.c | 19 +++ drivers/core/uclass.c | 34 ++ include/dm/device.h | 2 ++ include/dm/uclass-internal.h | 3 +++ include/dm/uclass.h | 2 ++ 5 files changed, 60 insertions(+) This is interesting. Could you also add the motivation for this? It's not clear to me who would call this function. I have been reviewing the OF_PLATDATA support as an R&D project, in this context, in order to have a better understanding on the possibilities and limitations I decided to add its support to iMX6, more particularly to the MMC drivers. The link issue arises when I tried to setup the GPIO for Card Detection, which is trivial when DT is available. However, when OF_PLATDATA is enabled this seems, at least for me, not straightforward. In order to overcome this limitation I think that having a set of functions to find/get devices based on platdata could be useful. Of course, there might be a better approach/idea, so that is the motivation for this RFC. An example of the usage could be #if CONFIG_IS_ENABLED(DM_GPIO) struct udevice *gpiodev; ret = uclass_get_device_by_platdata(UCLASS_GPIO, (void *)dtplat->cd_gpios->node, &gpiodev); if (ret) return ret; ret = gpio_dev_request_index(gpiodev, gpiodev->name, "cd-gpios", dtplat->cd_gpios->arg[0], GPIOD_IS_IN, dtplat->cd_gpios->arg[1], &priv->cd_gpio); if (ret) return ret; #endif This is part of my current work, a series of patches to add OF_PLATDATA support as explained. Does this make sense to you? Not yet :-) What is the context of this call? Typically dtplat is only available in the driver that includes it. Sorry for not being clear enough. I'm working in a patchset that needs some clean up, that is the reason I didn't send it yet. I'll try to clarify, but if you think it could be useful to share it, please let me know. What driver is the above code in? Is it for MMC that needs a GPIO to function? I'll assume it is for now. The driver on which I'm working in is drivers/mmc/fsl_esdhc_imx.c, I'm adding support for OF_PLATDATA to it, and in this sense trying to get the GPIOs used for CD to be requested. Then the weird thing is that we are accessing the dtplat of another device. It's a clever technique but I wonder if we can find another way. If you see drivers/mmc/rockchip_sdhci.c it has: ret = clk_get_by_index_platdata(dev, 0, dtplat->clocks, &clk); So I wonder if we need gpio_dev_request_by_platdata()? Thanks for pointing to this example, as I saw it before starting to work on these functions and had some doubts. I'll use it in the next paragraph to share my thoughts and the motivation of my work. From my understanding, clk_get_by_index_platdata in this context can only get a UCLASS_CLK device with id == 0. Is this correct? If it is so, this will only allow us to use this function if we know in advance that the UCLASS_CLK device has index 0. How can we get the correct UCLASS_CLK device in case of multiple instances? We actually can't support that at present. I think we would need to change the property to be an array, like: clocks = [ [&clk1, CLK_ID_SPI], [&clk1, CLK_ID_I2C, 23], ] which would need a fancier dtoc. Perhaps we should start by upstreaming that tool. In this case, are you suggesting to replace CLK_ID_SPI and CLK_ID_I2C with a integer calculated by dtoc based on
Re: [RFC] dm: uclass: add functions to get device by platdata
Hi Simon, On 21/4/20 14:36, Simon Glass wrote: Hi Walter, On Fri, 17 Apr 2020 at 15:18, Walter Lozano wrote: Hi Simon, On 9/4/20 16:36, Simon Glass wrote: HI Walter, On Thu, 9 Apr 2020 at 12:57, Walter Lozano wrote: Hi Simon, On 8/4/20 00:14, Simon Glass wrote: Hi Walter, On Tue, 7 Apr 2020 at 14:05, Walter Lozano wrote: Hi Simon, On 6/4/20 00:43, Simon Glass wrote: Hi Walter, On Mon, 9 Mar 2020 at 12:27, Walter Lozano wrote: Hi Simon On 6/3/20 17:32, Simon Glass wrote: Hi Walter, On Fri, 6 Mar 2020 at 09:10, Walter Lozano wrote: Hi Simon, Thanks again for taking the time to check my comments. On 6/3/20 10:17, Simon Glass wrote: Hi Walter, On Thu, 5 Mar 2020 at 06:54, Walter Lozano wrote: Hi Simon, Thanks for taking the time to check for my comments On 4/3/20 20:11, Simon Glass wrote: Hi Walter, On Wed, 4 Mar 2020 at 12:40, Walter Lozano wrote: When OF_PLATDATA is enabled DT information is parsed and platdata structures are populated. In this context the links between DT nodes are represented as pointers to platdata structures, and there is no clear way to access to the device which owns the structure. This patch implements a set of functions: - device_find_by_platdata - uclass_find_device_by_platdata to access to the device. Signed-off-by: Walter Lozano --- drivers/core/device.c| 19 +++ drivers/core/uclass.c| 34 ++ include/dm/device.h | 2 ++ include/dm/uclass-internal.h | 3 +++ include/dm/uclass.h | 2 ++ 5 files changed, 60 insertions(+) This is interesting. Could you also add the motivation for this? It's not clear to me who would call this function. I have been reviewing the OF_PLATDATA support as an R&D project, in this context, in order to have a better understanding on the possibilities and limitations I decided to add its support to iMX6, more particularly to the MMC drivers. The link issue arises when I tried to setup the GPIO for Card Detection, which is trivial when DT is available. However, when OF_PLATDATA is enabled this seems, at least for me, not straightforward. In order to overcome this limitation I think that having a set of functions to find/get devices based on platdata could be useful. Of course, there might be a better approach/idea, so that is the motivation for this RFC. An example of the usage could be #if CONFIG_IS_ENABLED(DM_GPIO) struct udevice *gpiodev; ret = uclass_get_device_by_platdata(UCLASS_GPIO, (void *)dtplat->cd_gpios->node, &gpiodev); if (ret) return ret; ret = gpio_dev_request_index(gpiodev, gpiodev->name, "cd-gpios", dtplat->cd_gpios->arg[0], GPIOD_IS_IN, dtplat->cd_gpios->arg[1], &priv->cd_gpio); if (ret) return ret; #endif This is part of my current work, a series of patches to add OF_PLATDATA support as explained. Does this make sense to you? Not yet :-) What is the context of this call? Typically dtplat is only available in the driver that includes it. Sorry for not being clear enough. I'm working in a patchset that needs some clean up, that is the reason I didn't send it yet. I'll try to clarify, but if you think it could be useful to share it, please let me know. What driver is the above code in? Is it for MMC that needs a GPIO to function? I'll assume it is for now. The driver on which I'm working in is drivers/mmc/fsl_esdhc_imx.c, I'm adding support for OF_PLATDATA to it, and in this sense trying to get the GPIOs used for CD to be requested. Then the weird thing is that we are accessing the dtplat of another device. It's a clever technique but I wonder if we can find another way. If you see drivers/mmc/rockchip_sdhci.c it has: ret = clk_get_by_index_platdata(dev, 0, dtplat->clocks, &clk); So I wonder if we need gpio_dev_request_by_platdata()? Thanks for pointing to this example, as I saw it before starting to work on these functions and had some doubts. I'll use it in the next paragraph to share my thoughts and the motivation of my work. From my understanding, clk_get_by_index_platdata in this context can only get a UCLASS_CLK device with id == 0. Is this correct? If it is so, this will only allow us to use this function if we know in advance that the UCLASS_CLK device has index 0. How can we get the correct UCLASS_CLK device in case of multiple instances? We actually can't support that at present. I think we would need to change the property to be an array, like: clocks = [ [&clk1, CLK_ID_SPI], [&clk1, CLK_ID_I2C, 23], ] which would need a fancier dtoc. Perhaps we should start by upstreaming that tool. In this case, are you suggesting to replace CLK_ID_SPI and CLK_ID_I2C with a integer ca
Re: [RFC] dm: uclass: add functions to get device by platdata
Hi Walter, On Fri, 17 Apr 2020 at 15:18, Walter Lozano wrote: > > Hi Simon, > > On 9/4/20 16:36, Simon Glass wrote: > > HI Walter, > > > > On Thu, 9 Apr 2020 at 12:57, Walter Lozano > > wrote: > >> Hi Simon, > >> > >> On 8/4/20 00:14, Simon Glass wrote: > >>> Hi Walter, > >>> > >>> On Tue, 7 Apr 2020 at 14:05, Walter Lozano > >>> wrote: > Hi Simon, > > On 6/4/20 00:43, Simon Glass wrote: > > Hi Walter, > > > > On Mon, 9 Mar 2020 at 12:27, Walter Lozano > > wrote: > >> Hi Simon > >> > >> On 6/3/20 17:32, Simon Glass wrote: > >>> Hi Walter, > >>> > >>> On Fri, 6 Mar 2020 at 09:10, Walter > >>> Lozano wrote: > Hi Simon, > > Thanks again for taking the time to check my comments. > > On 6/3/20 10:17, Simon Glass wrote: > > Hi Walter, > > > > On Thu, 5 Mar 2020 at 06:54, Walter > > Lozano wrote: > >> Hi Simon, > >> > >> Thanks for taking the time to check for my comments > >> > >> On 4/3/20 20:11, Simon Glass wrote: > >> > >>> Hi Walter, > >>> > >>> On Wed, 4 Mar 2020 at 12:40, Walter > >>> Lozano wrote: > When OF_PLATDATA is enabled DT information is parsed and platdata > structures are populated. In this context the links between DT > nodes are > represented as pointers to platdata structures, and there is no > clear way > to access to the device which owns the structure. > > This patch implements a set of functions: > > - device_find_by_platdata > - uclass_find_device_by_platdata > > to access to the device. > > Signed-off-by: Walter Lozano > --- > drivers/core/device.c| 19 +++ > drivers/core/uclass.c| 34 > ++ > include/dm/device.h | 2 ++ > include/dm/uclass-internal.h | 3 +++ > include/dm/uclass.h | 2 ++ > 5 files changed, 60 insertions(+) > >>> This is interesting. Could you also add the motivation for this? > >>> It's > >>> not clear to me who would call this function. > >> I have been reviewing the OF_PLATDATA support as an R&D project, > >> in this context, in order to have > >> a better understanding on the possibilities and limitations I > >> decided to add its support to iMX6, > >> more particularly to the MMC drivers. The link issue arises when I > >> tried to setup the GPIO for > >> Card Detection, which is trivial when DT is available. However, > >> when OF_PLATDATA is enabled > >> this seems, at least for me, not straightforward. > >> > >> In order to overcome this limitation I think that having a set of > >> functions to find/get devices > >> based on platdata could be useful. Of course, there might be a > >> better approach/idea, so that is > >> the motivation for this RFC. > >> > >> An example of the usage could be > >> > >> #if CONFIG_IS_ENABLED(DM_GPIO) > >> > >> struct udevice *gpiodev; > >> > >> ret = uclass_get_device_by_platdata(UCLASS_GPIO, > >> (void *)dtplat->cd_gpios->node, &gpiodev); > >> > >> if (ret) > >> return ret; > >> > >> ret = gpio_dev_request_index(gpiodev, gpiodev->name, > >> "cd-gpios", > >> > >> dtplat->cd_gpios->arg[0], GPIOD_IS_IN, > >> > >> dtplat->cd_gpios->arg[1], &priv->cd_gpio); > >> > >> if (ret) > >> return ret; > >> > >> #endif > >> > >> This is part of my current work, a series of patches to add > >> OF_PLATDATA support as explained. > >> > >> Does this make sense to you? > > Not yet :-) > > > > What is the context of this call? Typically dtplat is only available > > in the driver that includes it. > Sorry for not being clear enough. I'm working in a patchset that > needs > some clean up, that is the reason I didn't send it yet. I'll try to > clarify, but if you think it could be useful to share it, please let > me > know. > > > What driver is the above code in? Is it for MMC that needs a GPIO to > > function? I'll assume i
Re: [RFC] dm: uclass: add functions to get device by platdata
Hi Simon, On 9/4/20 16:36, Simon Glass wrote: HI Walter, On Thu, 9 Apr 2020 at 12:57, Walter Lozano wrote: Hi Simon, On 8/4/20 00:14, Simon Glass wrote: Hi Walter, On Tue, 7 Apr 2020 at 14:05, Walter Lozano wrote: Hi Simon, On 6/4/20 00:43, Simon Glass wrote: Hi Walter, On Mon, 9 Mar 2020 at 12:27, Walter Lozano wrote: Hi Simon On 6/3/20 17:32, Simon Glass wrote: Hi Walter, On Fri, 6 Mar 2020 at 09:10, Walter Lozano wrote: Hi Simon, Thanks again for taking the time to check my comments. On 6/3/20 10:17, Simon Glass wrote: Hi Walter, On Thu, 5 Mar 2020 at 06:54, Walter Lozano wrote: Hi Simon, Thanks for taking the time to check for my comments On 4/3/20 20:11, Simon Glass wrote: Hi Walter, On Wed, 4 Mar 2020 at 12:40, Walter Lozano wrote: When OF_PLATDATA is enabled DT information is parsed and platdata structures are populated. In this context the links between DT nodes are represented as pointers to platdata structures, and there is no clear way to access to the device which owns the structure. This patch implements a set of functions: - device_find_by_platdata - uclass_find_device_by_platdata to access to the device. Signed-off-by: Walter Lozano --- drivers/core/device.c| 19 +++ drivers/core/uclass.c| 34 ++ include/dm/device.h | 2 ++ include/dm/uclass-internal.h | 3 +++ include/dm/uclass.h | 2 ++ 5 files changed, 60 insertions(+) This is interesting. Could you also add the motivation for this? It's not clear to me who would call this function. I have been reviewing the OF_PLATDATA support as an R&D project, in this context, in order to have a better understanding on the possibilities and limitations I decided to add its support to iMX6, more particularly to the MMC drivers. The link issue arises when I tried to setup the GPIO for Card Detection, which is trivial when DT is available. However, when OF_PLATDATA is enabled this seems, at least for me, not straightforward. In order to overcome this limitation I think that having a set of functions to find/get devices based on platdata could be useful. Of course, there might be a better approach/idea, so that is the motivation for this RFC. An example of the usage could be #if CONFIG_IS_ENABLED(DM_GPIO) struct udevice *gpiodev; ret = uclass_get_device_by_platdata(UCLASS_GPIO, (void *)dtplat->cd_gpios->node, &gpiodev); if (ret) return ret; ret = gpio_dev_request_index(gpiodev, gpiodev->name, "cd-gpios", dtplat->cd_gpios->arg[0], GPIOD_IS_IN, dtplat->cd_gpios->arg[1], &priv->cd_gpio); if (ret) return ret; #endif This is part of my current work, a series of patches to add OF_PLATDATA support as explained. Does this make sense to you? Not yet :-) What is the context of this call? Typically dtplat is only available in the driver that includes it. Sorry for not being clear enough. I'm working in a patchset that needs some clean up, that is the reason I didn't send it yet. I'll try to clarify, but if you think it could be useful to share it, please let me know. What driver is the above code in? Is it for MMC that needs a GPIO to function? I'll assume it is for now. The driver on which I'm working in is drivers/mmc/fsl_esdhc_imx.c, I'm adding support for OF_PLATDATA to it, and in this sense trying to get the GPIOs used for CD to be requested. Then the weird thing is that we are accessing the dtplat of another device. It's a clever technique but I wonder if we can find another way. If you see drivers/mmc/rockchip_sdhci.c it has: ret = clk_get_by_index_platdata(dev, 0, dtplat->clocks, &clk); So I wonder if we need gpio_dev_request_by_platdata()? Thanks for pointing to this example, as I saw it before starting to work on these functions and had some doubts. I'll use it in the next paragraph to share my thoughts and the motivation of my work. From my understanding, clk_get_by_index_platdata in this context can only get a UCLASS_CLK device with id == 0. Is this correct? If it is so, this will only allow us to use this function if we know in advance that the UCLASS_CLK device has index 0. How can we get the correct UCLASS_CLK device in case of multiple instances? We actually can't support that at present. I think we would need to change the property to be an array, like: clocks = [ [&clk1, CLK_ID_SPI], [&clk1, CLK_ID_I2C, 23], ] which would need a fancier dtoc. Perhaps we should start by upstreaming that tool. In this case, are you suggesting to replace CLK_ID_SPI and CLK_ID_I2C with a integer calculated by dtoc based on the clocks entries available? If I understand correctly, what we need is to get the index parameter to feed
Re: [RFC] dm: uclass: add functions to get device by platdata
On 9/4/20 16:36, Simon Glass wrote: HI Walter, On Thu, 9 Apr 2020 at 12:57, Walter Lozano wrote: Hi Simon, On 8/4/20 00:14, Simon Glass wrote: Hi Walter, On Tue, 7 Apr 2020 at 14:05, Walter Lozano wrote: Hi Simon, On 6/4/20 00:43, Simon Glass wrote: Hi Walter, On Mon, 9 Mar 2020 at 12:27, Walter Lozano wrote: Hi Simon On 6/3/20 17:32, Simon Glass wrote: Hi Walter, On Fri, 6 Mar 2020 at 09:10, Walter Lozano wrote: Hi Simon, Thanks again for taking the time to check my comments. On 6/3/20 10:17, Simon Glass wrote: Hi Walter, On Thu, 5 Mar 2020 at 06:54, Walter Lozano wrote: Hi Simon, Thanks for taking the time to check for my comments On 4/3/20 20:11, Simon Glass wrote: Hi Walter, On Wed, 4 Mar 2020 at 12:40, Walter Lozano wrote: When OF_PLATDATA is enabled DT information is parsed and platdata structures are populated. In this context the links between DT nodes are represented as pointers to platdata structures, and there is no clear way to access to the device which owns the structure. This patch implements a set of functions: - device_find_by_platdata - uclass_find_device_by_platdata to access to the device. Signed-off-by: Walter Lozano --- drivers/core/device.c| 19 +++ drivers/core/uclass.c| 34 ++ include/dm/device.h | 2 ++ include/dm/uclass-internal.h | 3 +++ include/dm/uclass.h | 2 ++ 5 files changed, 60 insertions(+) This is interesting. Could you also add the motivation for this? It's not clear to me who would call this function. I have been reviewing the OF_PLATDATA support as an R&D project, in this context, in order to have a better understanding on the possibilities and limitations I decided to add its support to iMX6, more particularly to the MMC drivers. The link issue arises when I tried to setup the GPIO for Card Detection, which is trivial when DT is available. However, when OF_PLATDATA is enabled this seems, at least for me, not straightforward. In order to overcome this limitation I think that having a set of functions to find/get devices based on platdata could be useful. Of course, there might be a better approach/idea, so that is the motivation for this RFC. An example of the usage could be #if CONFIG_IS_ENABLED(DM_GPIO) struct udevice *gpiodev; ret = uclass_get_device_by_platdata(UCLASS_GPIO, (void *)dtplat->cd_gpios->node, &gpiodev); if (ret) return ret; ret = gpio_dev_request_index(gpiodev, gpiodev->name, "cd-gpios", dtplat->cd_gpios->arg[0], GPIOD_IS_IN, dtplat->cd_gpios->arg[1], &priv->cd_gpio); if (ret) return ret; #endif This is part of my current work, a series of patches to add OF_PLATDATA support as explained. Does this make sense to you? Not yet :-) What is the context of this call? Typically dtplat is only available in the driver that includes it. Sorry for not being clear enough. I'm working in a patchset that needs some clean up, that is the reason I didn't send it yet. I'll try to clarify, but if you think it could be useful to share it, please let me know. What driver is the above code in? Is it for MMC that needs a GPIO to function? I'll assume it is for now. The driver on which I'm working in is drivers/mmc/fsl_esdhc_imx.c, I'm adding support for OF_PLATDATA to it, and in this sense trying to get the GPIOs used for CD to be requested. Then the weird thing is that we are accessing the dtplat of another device. It's a clever technique but I wonder if we can find another way. If you see drivers/mmc/rockchip_sdhci.c it has: ret = clk_get_by_index_platdata(dev, 0, dtplat->clocks, &clk); So I wonder if we need gpio_dev_request_by_platdata()? Thanks for pointing to this example, as I saw it before starting to work on these functions and had some doubts. I'll use it in the next paragraph to share my thoughts and the motivation of my work. From my understanding, clk_get_by_index_platdata in this context can only get a UCLASS_CLK device with id == 0. Is this correct? If it is so, this will only allow us to use this function if we know in advance that the UCLASS_CLK device has index 0. How can we get the correct UCLASS_CLK device in case of multiple instances? We actually can't support that at present. I think we would need to change the property to be an array, like: clocks = [ [&clk1, CLK_ID_SPI], [&clk1, CLK_ID_I2C, 23], ] which would need a fancier dtoc. Perhaps we should start by upstreaming that tool. In this case, are you suggesting to replace CLK_ID_SPI and CLK_ID_I2C with a integer calculated by dtoc based on the clocks entries available? If I understand correctly, what we need is to get the index parameter to feed the funct
Re: [RFC] dm: uclass: add functions to get device by platdata
HI Walter, On Thu, 9 Apr 2020 at 12:57, Walter Lozano wrote: > > Hi Simon, > > On 8/4/20 00:14, Simon Glass wrote: > > Hi Walter, > > > > On Tue, 7 Apr 2020 at 14:05, Walter Lozano > > wrote: > >> Hi Simon, > >> > >> On 6/4/20 00:43, Simon Glass wrote: > >>> Hi Walter, > >>> > >>> On Mon, 9 Mar 2020 at 12:27, Walter Lozano > >>> wrote: > Hi Simon > > On 6/3/20 17:32, Simon Glass wrote: > > Hi Walter, > > > > On Fri, 6 Mar 2020 at 09:10, Walter Lozano > > wrote: > >> Hi Simon, > >> > >> Thanks again for taking the time to check my comments. > >> > >> On 6/3/20 10:17, Simon Glass wrote: > >>> Hi Walter, > >>> > >>> On Thu, 5 Mar 2020 at 06:54, Walter > >>> Lozano wrote: > Hi Simon, > > Thanks for taking the time to check for my comments > > On 4/3/20 20:11, Simon Glass wrote: > > > Hi Walter, > > > > On Wed, 4 Mar 2020 at 12:40, Walter > > Lozano wrote: > >> When OF_PLATDATA is enabled DT information is parsed and platdata > >> structures are populated. In this context the links between DT > >> nodes are > >> represented as pointers to platdata structures, and there is no > >> clear way > >> to access to the device which owns the structure. > >> > >> This patch implements a set of functions: > >> > >> - device_find_by_platdata > >> - uclass_find_device_by_platdata > >> > >> to access to the device. > >> > >> Signed-off-by: Walter Lozano > >> --- > >> drivers/core/device.c| 19 +++ > >> drivers/core/uclass.c| 34 > >> ++ > >> include/dm/device.h | 2 ++ > >> include/dm/uclass-internal.h | 3 +++ > >> include/dm/uclass.h | 2 ++ > >> 5 files changed, 60 insertions(+) > > This is interesting. Could you also add the motivation for this? > > It's > > not clear to me who would call this function. > I have been reviewing the OF_PLATDATA support as an R&D project, in > this context, in order to have > a better understanding on the possibilities and limitations I > decided to add its support to iMX6, > more particularly to the MMC drivers. The link issue arises when I > tried to setup the GPIO for > Card Detection, which is trivial when DT is available. However, when > OF_PLATDATA is enabled > this seems, at least for me, not straightforward. > > In order to overcome this limitation I think that having a set of > functions to find/get devices > based on platdata could be useful. Of course, there might be a > better approach/idea, so that is > the motivation for this RFC. > > An example of the usage could be > > #if CONFIG_IS_ENABLED(DM_GPIO) > > struct udevice *gpiodev; > > ret = uclass_get_device_by_platdata(UCLASS_GPIO, (void > *)dtplat->cd_gpios->node, &gpiodev); > > if (ret) > return ret; > > ret = gpio_dev_request_index(gpiodev, gpiodev->name, > "cd-gpios", > dtplat->cd_gpios->arg[0], > GPIOD_IS_IN, > dtplat->cd_gpios->arg[1], > &priv->cd_gpio); > > if (ret) > return ret; > > #endif > > This is part of my current work, a series of patches to add > OF_PLATDATA support as explained. > > Does this make sense to you? > >>> Not yet :-) > >>> > >>> What is the context of this call? Typically dtplat is only available > >>> in the driver that includes it. > >> Sorry for not being clear enough. I'm working in a patchset that needs > >> some clean up, that is the reason I didn't send it yet. I'll try to > >> clarify, but if you think it could be useful to share it, please let me > >> know. > >> > >>> What driver is the above code in? Is it for MMC that needs a GPIO to > >>> function? I'll assume it is for now. > >> The driver on which I'm working in is drivers/mmc/fsl_esdhc_imx.c, I'm > >> adding support for OF_PLATDATA to it, and in this sense trying to get > >> the GPIOs used for CD to be requested. > >> > >>> Then the weird thing is that we are accessing the dtplat of another > >>> device. It's a clever technique but I wonder if we can find another > >>> way. > >>> > >>>
Re: [RFC] dm: uclass: add functions to get device by platdata
Hi Simon, On 8/4/20 00:14, Simon Glass wrote: Hi Walter, On Tue, 7 Apr 2020 at 14:05, Walter Lozano wrote: Hi Simon, On 6/4/20 00:43, Simon Glass wrote: Hi Walter, On Mon, 9 Mar 2020 at 12:27, Walter Lozano wrote: Hi Simon On 6/3/20 17:32, Simon Glass wrote: Hi Walter, On Fri, 6 Mar 2020 at 09:10, Walter Lozano wrote: Hi Simon, Thanks again for taking the time to check my comments. On 6/3/20 10:17, Simon Glass wrote: Hi Walter, On Thu, 5 Mar 2020 at 06:54, Walter Lozano wrote: Hi Simon, Thanks for taking the time to check for my comments On 4/3/20 20:11, Simon Glass wrote: Hi Walter, On Wed, 4 Mar 2020 at 12:40, Walter Lozano wrote: When OF_PLATDATA is enabled DT information is parsed and platdata structures are populated. In this context the links between DT nodes are represented as pointers to platdata structures, and there is no clear way to access to the device which owns the structure. This patch implements a set of functions: - device_find_by_platdata - uclass_find_device_by_platdata to access to the device. Signed-off-by: Walter Lozano --- drivers/core/device.c| 19 +++ drivers/core/uclass.c| 34 ++ include/dm/device.h | 2 ++ include/dm/uclass-internal.h | 3 +++ include/dm/uclass.h | 2 ++ 5 files changed, 60 insertions(+) This is interesting. Could you also add the motivation for this? It's not clear to me who would call this function. I have been reviewing the OF_PLATDATA support as an R&D project, in this context, in order to have a better understanding on the possibilities and limitations I decided to add its support to iMX6, more particularly to the MMC drivers. The link issue arises when I tried to setup the GPIO for Card Detection, which is trivial when DT is available. However, when OF_PLATDATA is enabled this seems, at least for me, not straightforward. In order to overcome this limitation I think that having a set of functions to find/get devices based on platdata could be useful. Of course, there might be a better approach/idea, so that is the motivation for this RFC. An example of the usage could be #if CONFIG_IS_ENABLED(DM_GPIO) struct udevice *gpiodev; ret = uclass_get_device_by_platdata(UCLASS_GPIO, (void *)dtplat->cd_gpios->node, &gpiodev); if (ret) return ret; ret = gpio_dev_request_index(gpiodev, gpiodev->name, "cd-gpios", dtplat->cd_gpios->arg[0], GPIOD_IS_IN, dtplat->cd_gpios->arg[1], &priv->cd_gpio); if (ret) return ret; #endif This is part of my current work, a series of patches to add OF_PLATDATA support as explained. Does this make sense to you? Not yet :-) What is the context of this call? Typically dtplat is only available in the driver that includes it. Sorry for not being clear enough. I'm working in a patchset that needs some clean up, that is the reason I didn't send it yet. I'll try to clarify, but if you think it could be useful to share it, please let me know. What driver is the above code in? Is it for MMC that needs a GPIO to function? I'll assume it is for now. The driver on which I'm working in is drivers/mmc/fsl_esdhc_imx.c, I'm adding support for OF_PLATDATA to it, and in this sense trying to get the GPIOs used for CD to be requested. Then the weird thing is that we are accessing the dtplat of another device. It's a clever technique but I wonder if we can find another way. If you see drivers/mmc/rockchip_sdhci.c it has: ret = clk_get_by_index_platdata(dev, 0, dtplat->clocks, &clk); So I wonder if we need gpio_dev_request_by_platdata()? Thanks for pointing to this example, as I saw it before starting to work on these functions and had some doubts. I'll use it in the next paragraph to share my thoughts and the motivation of my work. From my understanding, clk_get_by_index_platdata in this context can only get a UCLASS_CLK device with id == 0. Is this correct? If it is so, this will only allow us to use this function if we know in advance that the UCLASS_CLK device has index 0. How can we get the correct UCLASS_CLK device in case of multiple instances? We actually can't support that at present. I think we would need to change the property to be an array, like: clocks = [ [&clk1, CLK_ID_SPI], [&clk1, CLK_ID_I2C, 23], ] which would need a fancier dtoc. Perhaps we should start by upstreaming that tool. In this case, are you suggesting to replace CLK_ID_SPI and CLK_ID_I2C with a integer calculated by dtoc based on the clocks entries available? If I understand correctly, what we need is to get the index parameter to feed the function uclass_find_device. Is this correct? No, I was thinking that it would be the same CLK_ID_SPI value from the binding. W
Re: [RFC] dm: uclass: add functions to get device by platdata
Hi Walter, On Tue, 7 Apr 2020 at 14:05, Walter Lozano wrote: > > Hi Simon, > > On 6/4/20 00:43, Simon Glass wrote: > > Hi Walter, > > > > On Mon, 9 Mar 2020 at 12:27, Walter Lozano > > wrote: > >> Hi Simon > >> > >> On 6/3/20 17:32, Simon Glass wrote: > >>> Hi Walter, > >>> > >>> On Fri, 6 Mar 2020 at 09:10, Walter Lozano > >>> wrote: > Hi Simon, > > Thanks again for taking the time to check my comments. > > On 6/3/20 10:17, Simon Glass wrote: > > Hi Walter, > > > > On Thu, 5 Mar 2020 at 06:54, Walter Lozano > > wrote: > >> Hi Simon, > >> > >> Thanks for taking the time to check for my comments > >> > >> On 4/3/20 20:11, Simon Glass wrote: > >> > >>> Hi Walter, > >>> > >>> On Wed, 4 Mar 2020 at 12:40, Walter > >>> Lozano wrote: > When OF_PLATDATA is enabled DT information is parsed and platdata > structures are populated. In this context the links between DT nodes > are > represented as pointers to platdata structures, and there is no > clear way > to access to the device which owns the structure. > > This patch implements a set of functions: > > - device_find_by_platdata > - uclass_find_device_by_platdata > > to access to the device. > > Signed-off-by: Walter Lozano > --- > drivers/core/device.c| 19 +++ > drivers/core/uclass.c| 34 > ++ > include/dm/device.h | 2 ++ > include/dm/uclass-internal.h | 3 +++ > include/dm/uclass.h | 2 ++ > 5 files changed, 60 insertions(+) > >>> This is interesting. Could you also add the motivation for this? It's > >>> not clear to me who would call this function. > >> I have been reviewing the OF_PLATDATA support as an R&D project, in > >> this context, in order to have > >> a better understanding on the possibilities and limitations I decided > >> to add its support to iMX6, > >> more particularly to the MMC drivers. The link issue arises when I > >> tried to setup the GPIO for > >> Card Detection, which is trivial when DT is available. However, when > >> OF_PLATDATA is enabled > >> this seems, at least for me, not straightforward. > >> > >> In order to overcome this limitation I think that having a set of > >> functions to find/get devices > >> based on platdata could be useful. Of course, there might be a better > >> approach/idea, so that is > >> the motivation for this RFC. > >> > >> An example of the usage could be > >> > >> #if CONFIG_IS_ENABLED(DM_GPIO) > >> > >> struct udevice *gpiodev; > >> > >> ret = uclass_get_device_by_platdata(UCLASS_GPIO, (void > >> *)dtplat->cd_gpios->node, &gpiodev); > >> > >> if (ret) > >> return ret; > >> > >> ret = gpio_dev_request_index(gpiodev, gpiodev->name, > >> "cd-gpios", > >> dtplat->cd_gpios->arg[0], > >> GPIOD_IS_IN, > >> dtplat->cd_gpios->arg[1], > >> &priv->cd_gpio); > >> > >> if (ret) > >> return ret; > >> > >> #endif > >> > >> This is part of my current work, a series of patches to add > >> OF_PLATDATA support as explained. > >> > >> Does this make sense to you? > > Not yet :-) > > > > What is the context of this call? Typically dtplat is only available > > in the driver that includes it. > Sorry for not being clear enough. I'm working in a patchset that needs > some clean up, that is the reason I didn't send it yet. I'll try to > clarify, but if you think it could be useful to share it, please let me > know. > > > What driver is the above code in? Is it for MMC that needs a GPIO to > > function? I'll assume it is for now. > The driver on which I'm working in is drivers/mmc/fsl_esdhc_imx.c, I'm > adding support for OF_PLATDATA to it, and in this sense trying to get > the GPIOs used for CD to be requested. > > > Then the weird thing is that we are accessing the dtplat of another > > device. It's a clever technique but I wonder if we can find another > > way. > > > > If you see drivers/mmc/rockchip_sdhci.c it has: > > > > ret = clk_get_by_index_platdata(dev, 0, dtplat->clocks, &clk); > > > > So I wonder if we need gpio_dev_request_by_platdata()? > Thanks for pointing to this example, as I saw it before starting to work > on these functions and had some doubts. I'll use it in the next > paragraph to share my thoughts and the motivation
Re: [RFC] dm: uclass: add functions to get device by platdata
Hi Simon,
On 6/4/20 00:43, Simon Glass wrote:
Hi Walter,
On Mon, 9 Mar 2020 at 12:27, Walter Lozano wrote:
Hi Simon
On 6/3/20 17:32, Simon Glass wrote:
Hi Walter,
On Fri, 6 Mar 2020 at 09:10, Walter Lozano wrote:
Hi Simon,
Thanks again for taking the time to check my comments.
On 6/3/20 10:17, Simon Glass wrote:
Hi Walter,
On Thu, 5 Mar 2020 at 06:54, Walter Lozano wrote:
Hi Simon,
Thanks for taking the time to check for my comments
On 4/3/20 20:11, Simon Glass wrote:
Hi Walter,
On Wed, 4 Mar 2020 at 12:40, Walter Lozano wrote:
When OF_PLATDATA is enabled DT information is parsed and platdata
structures are populated. In this context the links between DT nodes are
represented as pointers to platdata structures, and there is no clear way
to access to the device which owns the structure.
This patch implements a set of functions:
- device_find_by_platdata
- uclass_find_device_by_platdata
to access to the device.
Signed-off-by: Walter Lozano
---
drivers/core/device.c| 19 +++
drivers/core/uclass.c| 34 ++
include/dm/device.h | 2 ++
include/dm/uclass-internal.h | 3 +++
include/dm/uclass.h | 2 ++
5 files changed, 60 insertions(+)
This is interesting. Could you also add the motivation for this? It's
not clear to me who would call this function.
I have been reviewing the OF_PLATDATA support as an R&D project, in this
context, in order to have
a better understanding on the possibilities and limitations I decided to add
its support to iMX6,
more particularly to the MMC drivers. The link issue arises when I tried to
setup the GPIO for
Card Detection, which is trivial when DT is available. However, when
OF_PLATDATA is enabled
this seems, at least for me, not straightforward.
In order to overcome this limitation I think that having a set of functions to
find/get devices
based on platdata could be useful. Of course, there might be a better
approach/idea, so that is
the motivation for this RFC.
An example of the usage could be
#if CONFIG_IS_ENABLED(DM_GPIO)
struct udevice *gpiodev;
ret = uclass_get_device_by_platdata(UCLASS_GPIO, (void
*)dtplat->cd_gpios->node, &gpiodev);
if (ret)
return ret;
ret = gpio_dev_request_index(gpiodev, gpiodev->name, "cd-gpios",
dtplat->cd_gpios->arg[0], GPIOD_IS_IN,
dtplat->cd_gpios->arg[1],
&priv->cd_gpio);
if (ret)
return ret;
#endif
This is part of my current work, a series of patches to add OF_PLATDATA support
as explained.
Does this make sense to you?
Not yet :-)
What is the context of this call? Typically dtplat is only available
in the driver that includes it.
Sorry for not being clear enough. I'm working in a patchset that needs
some clean up, that is the reason I didn't send it yet. I'll try to
clarify, but if you think it could be useful to share it, please let me
know.
What driver is the above code in? Is it for MMC that needs a GPIO to
function? I'll assume it is for now.
The driver on which I'm working in is drivers/mmc/fsl_esdhc_imx.c, I'm
adding support for OF_PLATDATA to it, and in this sense trying to get
the GPIOs used for CD to be requested.
Then the weird thing is that we are accessing the dtplat of another
device. It's a clever technique but I wonder if we can find another
way.
If you see drivers/mmc/rockchip_sdhci.c it has:
ret = clk_get_by_index_platdata(dev, 0, dtplat->clocks, &clk);
So I wonder if we need gpio_dev_request_by_platdata()?
Thanks for pointing to this example, as I saw it before starting to work
on these functions and had some doubts. I'll use it in the next
paragraph to share my thoughts and the motivation of my work.
From my understanding, clk_get_by_index_platdata in this context can
only get a UCLASS_CLK device with id == 0. Is this correct?
If it is so, this will only allow us to use this function if we know in
advance that the UCLASS_CLK device has index 0.
How can we get the correct UCLASS_CLK device in case of multiple instances?
We actually can't support that at present. I think we would need to
change the property to be an array, like:
clocks = [
[&clk1, CLK_ID_SPI],
[&clk1, CLK_ID_I2C, 23],
]
which would need a fancier dtoc. Perhaps we should start by
upstreaming that tool.
In this case, are you suggesting to replace CLK_ID_SPI and CLK_ID_I2C
with a integer calculated by dtoc based on the clocks entries available?
If I understand correctly, what we need is to get the index parameter to
feed the function uclass_find_device. Is this correct?
No, I was thinking that it would be the same CLK_ID_SPI value from the binding.
We currently have (see the 'rock' board):
struct dtd_rockchip_rk3188_uart {
fdt32_t clock_frequency;
struct phandle_1_arg clocks[2];
fdt
Re: [RFC] dm: uclass: add functions to get device by platdata
Hi Walter, On Mon, 9 Mar 2020 at 12:27, Walter Lozano wrote: > > Hi Simon > > On 6/3/20 17:32, Simon Glass wrote: > > Hi Walter, > > > > On Fri, 6 Mar 2020 at 09:10, Walter Lozano > > wrote: > >> Hi Simon, > >> > >> Thanks again for taking the time to check my comments. > >> > >> On 6/3/20 10:17, Simon Glass wrote: > >>> Hi Walter, > >>> > >>> On Thu, 5 Mar 2020 at 06:54, Walter Lozano > >>> wrote: > Hi Simon, > > Thanks for taking the time to check for my comments > > On 4/3/20 20:11, Simon Glass wrote: > > > Hi Walter, > > > > On Wed, 4 Mar 2020 at 12:40, Walter Lozano > > wrote: > >> When OF_PLATDATA is enabled DT information is parsed and platdata > >> structures are populated. In this context the links between DT nodes > >> are > >> represented as pointers to platdata structures, and there is no clear > >> way > >> to access to the device which owns the structure. > >> > >> This patch implements a set of functions: > >> > >> - device_find_by_platdata > >> - uclass_find_device_by_platdata > >> > >> to access to the device. > >> > >> Signed-off-by: Walter Lozano > >> --- > >> drivers/core/device.c| 19 +++ > >> drivers/core/uclass.c| 34 > >> ++ > >> include/dm/device.h | 2 ++ > >> include/dm/uclass-internal.h | 3 +++ > >> include/dm/uclass.h | 2 ++ > >> 5 files changed, 60 insertions(+) > > This is interesting. Could you also add the motivation for this? It's > > not clear to me who would call this function. > I have been reviewing the OF_PLATDATA support as an R&D project, in this > context, in order to have > a better understanding on the possibilities and limitations I decided to > add its support to iMX6, > more particularly to the MMC drivers. The link issue arises when I tried > to setup the GPIO for > Card Detection, which is trivial when DT is available. However, when > OF_PLATDATA is enabled > this seems, at least for me, not straightforward. > > In order to overcome this limitation I think that having a set of > functions to find/get devices > based on platdata could be useful. Of course, there might be a better > approach/idea, so that is > the motivation for this RFC. > > An example of the usage could be > > #if CONFIG_IS_ENABLED(DM_GPIO) > > struct udevice *gpiodev; > > ret = uclass_get_device_by_platdata(UCLASS_GPIO, (void > *)dtplat->cd_gpios->node, &gpiodev); > > if (ret) > return ret; > > ret = gpio_dev_request_index(gpiodev, gpiodev->name, > "cd-gpios", > dtplat->cd_gpios->arg[0], > GPIOD_IS_IN, > dtplat->cd_gpios->arg[1], > &priv->cd_gpio); > > if (ret) > return ret; > > #endif > > This is part of my current work, a series of patches to add OF_PLATDATA > support as explained. > > Does this make sense to you? > >>> Not yet :-) > >>> > >>> What is the context of this call? Typically dtplat is only available > >>> in the driver that includes it. > >> Sorry for not being clear enough. I'm working in a patchset that needs > >> some clean up, that is the reason I didn't send it yet. I'll try to > >> clarify, but if you think it could be useful to share it, please let me > >> know. > >> > >>> What driver is the above code in? Is it for MMC that needs a GPIO to > >>> function? I'll assume it is for now. > >> The driver on which I'm working in is drivers/mmc/fsl_esdhc_imx.c, I'm > >> adding support for OF_PLATDATA to it, and in this sense trying to get > >> the GPIOs used for CD to be requested. > >> > >>> Then the weird thing is that we are accessing the dtplat of another > >>> device. It's a clever technique but I wonder if we can find another > >>> way. > >>> > >>> If you see drivers/mmc/rockchip_sdhci.c it has: > >>> > >>> ret = clk_get_by_index_platdata(dev, 0, dtplat->clocks, &clk); > >>> > >>> So I wonder if we need gpio_dev_request_by_platdata()? > >> Thanks for pointing to this example, as I saw it before starting to work > >> on these functions and had some doubts. I'll use it in the next > >> paragraph to share my thoughts and the motivation of my work. > >> > >> From my understanding, clk_get_by_index_platdata in this context can > >> only get a UCLASS_CLK device with id == 0. Is this correct? > >> > >> If it is so, this will only allow us to use this function if we know in > >> advance that the UCLASS_CLK device has index 0. > >> > >> How can we get the correct UCLASS_CLK device in case of multiple instances? > > We actua
Re: [RFC] dm: uclass: add functions to get device by platdata
Hi Simon On 6/3/20 17:32, Simon Glass wrote: Hi Walter, On Fri, 6 Mar 2020 at 09:10, Walter Lozano wrote: Hi Simon, Thanks again for taking the time to check my comments. On 6/3/20 10:17, Simon Glass wrote: Hi Walter, On Thu, 5 Mar 2020 at 06:54, Walter Lozano wrote: Hi Simon, Thanks for taking the time to check for my comments On 4/3/20 20:11, Simon Glass wrote: Hi Walter, On Wed, 4 Mar 2020 at 12:40, Walter Lozano wrote: When OF_PLATDATA is enabled DT information is parsed and platdata structures are populated. In this context the links between DT nodes are represented as pointers to platdata structures, and there is no clear way to access to the device which owns the structure. This patch implements a set of functions: - device_find_by_platdata - uclass_find_device_by_platdata to access to the device. Signed-off-by: Walter Lozano --- drivers/core/device.c| 19 +++ drivers/core/uclass.c| 34 ++ include/dm/device.h | 2 ++ include/dm/uclass-internal.h | 3 +++ include/dm/uclass.h | 2 ++ 5 files changed, 60 insertions(+) This is interesting. Could you also add the motivation for this? It's not clear to me who would call this function. I have been reviewing the OF_PLATDATA support as an R&D project, in this context, in order to have a better understanding on the possibilities and limitations I decided to add its support to iMX6, more particularly to the MMC drivers. The link issue arises when I tried to setup the GPIO for Card Detection, which is trivial when DT is available. However, when OF_PLATDATA is enabled this seems, at least for me, not straightforward. In order to overcome this limitation I think that having a set of functions to find/get devices based on platdata could be useful. Of course, there might be a better approach/idea, so that is the motivation for this RFC. An example of the usage could be #if CONFIG_IS_ENABLED(DM_GPIO) struct udevice *gpiodev; ret = uclass_get_device_by_platdata(UCLASS_GPIO, (void *)dtplat->cd_gpios->node, &gpiodev); if (ret) return ret; ret = gpio_dev_request_index(gpiodev, gpiodev->name, "cd-gpios", dtplat->cd_gpios->arg[0], GPIOD_IS_IN, dtplat->cd_gpios->arg[1], &priv->cd_gpio); if (ret) return ret; #endif This is part of my current work, a series of patches to add OF_PLATDATA support as explained. Does this make sense to you? Not yet :-) What is the context of this call? Typically dtplat is only available in the driver that includes it. Sorry for not being clear enough. I'm working in a patchset that needs some clean up, that is the reason I didn't send it yet. I'll try to clarify, but if you think it could be useful to share it, please let me know. What driver is the above code in? Is it for MMC that needs a GPIO to function? I'll assume it is for now. The driver on which I'm working in is drivers/mmc/fsl_esdhc_imx.c, I'm adding support for OF_PLATDATA to it, and in this sense trying to get the GPIOs used for CD to be requested. Then the weird thing is that we are accessing the dtplat of another device. It's a clever technique but I wonder if we can find another way. If you see drivers/mmc/rockchip_sdhci.c it has: ret = clk_get_by_index_platdata(dev, 0, dtplat->clocks, &clk); So I wonder if we need gpio_dev_request_by_platdata()? Thanks for pointing to this example, as I saw it before starting to work on these functions and had some doubts. I'll use it in the next paragraph to share my thoughts and the motivation of my work. From my understanding, clk_get_by_index_platdata in this context can only get a UCLASS_CLK device with id == 0. Is this correct? If it is so, this will only allow us to use this function if we know in advance that the UCLASS_CLK device has index 0. How can we get the correct UCLASS_CLK device in case of multiple instances? We actually can't support that at present. I think we would need to change the property to be an array, like: clocks = [ [&clk1, CLK_ID_SPI], [&clk1, CLK_ID_I2C, 23], ] which would need a fancier dtoc. Perhaps we should start by upstreaming that tool. In this case, are you suggesting to replace CLK_ID_SPI and CLK_ID_I2C with a integer calculated by dtoc based on the clocks entries available? If I understand correctly, what we need is to get the index parameter to feed the function uclass_find_device. Is this correct? I understand that we need a way to use the link information present in platdata. However I could not find a way to get the actual index of the UCLASS_CLK device. In this context, I thought that the simplest but still valid approach could be to find the right device based on the struct platdata pointer it owns. The index should be the first
Re: [RFC] dm: uclass: add functions to get device by platdata
Hi Walter, On Fri, 6 Mar 2020 at 09:10, Walter Lozano wrote: > > Hi Simon, > > Thanks again for taking the time to check my comments. > > On 6/3/20 10:17, Simon Glass wrote: > > Hi Walter, > > > > On Thu, 5 Mar 2020 at 06:54, Walter Lozano > > wrote: > >> Hi Simon, > >> > >> Thanks for taking the time to check for my comments > >> > >> On 4/3/20 20:11, Simon Glass wrote: > >> > >>> Hi Walter, > >>> > >>> On Wed, 4 Mar 2020 at 12:40, Walter Lozano > >>> wrote: > When OF_PLATDATA is enabled DT information is parsed and platdata > structures are populated. In this context the links between DT nodes are > represented as pointers to platdata structures, and there is no clear way > to access to the device which owns the structure. > > This patch implements a set of functions: > > - device_find_by_platdata > - uclass_find_device_by_platdata > > to access to the device. > > Signed-off-by: Walter Lozano > --- > drivers/core/device.c| 19 +++ > drivers/core/uclass.c| 34 ++ > include/dm/device.h | 2 ++ > include/dm/uclass-internal.h | 3 +++ > include/dm/uclass.h | 2 ++ > 5 files changed, 60 insertions(+) > >>> This is interesting. Could you also add the motivation for this? It's > >>> not clear to me who would call this function. > >> I have been reviewing the OF_PLATDATA support as an R&D project, in this > >> context, in order to have > >> a better understanding on the possibilities and limitations I decided to > >> add its support to iMX6, > >> more particularly to the MMC drivers. The link issue arises when I tried > >> to setup the GPIO for > >> Card Detection, which is trivial when DT is available. However, when > >> OF_PLATDATA is enabled > >> this seems, at least for me, not straightforward. > >> > >> In order to overcome this limitation I think that having a set of > >> functions to find/get devices > >> based on platdata could be useful. Of course, there might be a better > >> approach/idea, so that is > >> the motivation for this RFC. > >> > >> An example of the usage could be > >> > >> #if CONFIG_IS_ENABLED(DM_GPIO) > >> > >> struct udevice *gpiodev; > >> > >> ret = uclass_get_device_by_platdata(UCLASS_GPIO, (void > >> *)dtplat->cd_gpios->node, &gpiodev); > >> > >> if (ret) > >> return ret; > >> > >> ret = gpio_dev_request_index(gpiodev, gpiodev->name, "cd-gpios", > >>dtplat->cd_gpios->arg[0], > >> GPIOD_IS_IN, > >>dtplat->cd_gpios->arg[1], > >> &priv->cd_gpio); > >> > >> if (ret) > >> return ret; > >> > >> #endif > >> > >> This is part of my current work, a series of patches to add OF_PLATDATA > >> support as explained. > >> > >> Does this make sense to you? > > Not yet :-) > > > > What is the context of this call? Typically dtplat is only available > > in the driver that includes it. > > Sorry for not being clear enough. I'm working in a patchset that needs > some clean up, that is the reason I didn't send it yet. I'll try to > clarify, but if you think it could be useful to share it, please let me > know. > > > What driver is the above code in? Is it for MMC that needs a GPIO to > > function? I'll assume it is for now. > > The driver on which I'm working in is drivers/mmc/fsl_esdhc_imx.c, I'm > adding support for OF_PLATDATA to it, and in this sense trying to get > the GPIOs used for CD to be requested. > > > Then the weird thing is that we are accessing the dtplat of another > > device. It's a clever technique but I wonder if we can find another > > way. > > > > If you see drivers/mmc/rockchip_sdhci.c it has: > > > > ret = clk_get_by_index_platdata(dev, 0, dtplat->clocks, &clk); > > > > So I wonder if we need gpio_dev_request_by_platdata()? > > Thanks for pointing to this example, as I saw it before starting to work > on these functions and had some doubts. I'll use it in the next > paragraph to share my thoughts and the motivation of my work. > > From my understanding, clk_get_by_index_platdata in this context can > only get a UCLASS_CLK device with id == 0. Is this correct? > > If it is so, this will only allow us to use this function if we know in > advance that the UCLASS_CLK device has index 0. > > How can we get the correct UCLASS_CLK device in case of multiple instances? We actually can't support that at present. I think we would need to change the property to be an array, like: clocks = [ [&clk1, CLK_ID_SPI], [&clk1, CLK_ID_I2C, 23], ] which would need a fancier dtoc. Perhaps we should start by upstreaming that tool. > > I understand that we need a way to use the link information present in > platdata. However I could not find a way to get the actual index of the > UCLASS_CLK device. In th
Re: [RFC] dm: uclass: add functions to get device by platdata
Hi Simon,
Thanks again for taking the time to check my comments.
On 6/3/20 10:17, Simon Glass wrote:
Hi Walter,
On Thu, 5 Mar 2020 at 06:54, Walter Lozano wrote:
Hi Simon,
Thanks for taking the time to check for my comments
On 4/3/20 20:11, Simon Glass wrote:
Hi Walter,
On Wed, 4 Mar 2020 at 12:40, Walter Lozano wrote:
When OF_PLATDATA is enabled DT information is parsed and platdata
structures are populated. In this context the links between DT nodes are
represented as pointers to platdata structures, and there is no clear way
to access to the device which owns the structure.
This patch implements a set of functions:
- device_find_by_platdata
- uclass_find_device_by_platdata
to access to the device.
Signed-off-by: Walter Lozano
---
drivers/core/device.c| 19 +++
drivers/core/uclass.c| 34 ++
include/dm/device.h | 2 ++
include/dm/uclass-internal.h | 3 +++
include/dm/uclass.h | 2 ++
5 files changed, 60 insertions(+)
This is interesting. Could you also add the motivation for this? It's
not clear to me who would call this function.
I have been reviewing the OF_PLATDATA support as an R&D project, in this
context, in order to have
a better understanding on the possibilities and limitations I decided to add
its support to iMX6,
more particularly to the MMC drivers. The link issue arises when I tried to
setup the GPIO for
Card Detection, which is trivial when DT is available. However, when
OF_PLATDATA is enabled
this seems, at least for me, not straightforward.
In order to overcome this limitation I think that having a set of functions to
find/get devices
based on platdata could be useful. Of course, there might be a better
approach/idea, so that is
the motivation for this RFC.
An example of the usage could be
#if CONFIG_IS_ENABLED(DM_GPIO)
struct udevice *gpiodev;
ret = uclass_get_device_by_platdata(UCLASS_GPIO, (void
*)dtplat->cd_gpios->node, &gpiodev);
if (ret)
return ret;
ret = gpio_dev_request_index(gpiodev, gpiodev->name, "cd-gpios",
dtplat->cd_gpios->arg[0], GPIOD_IS_IN,
dtplat->cd_gpios->arg[1],
&priv->cd_gpio);
if (ret)
return ret;
#endif
This is part of my current work, a series of patches to add OF_PLATDATA support
as explained.
Does this make sense to you?
Not yet :-)
What is the context of this call? Typically dtplat is only available
in the driver that includes it.
Sorry for not being clear enough. I'm working in a patchset that needs
some clean up, that is the reason I didn't send it yet. I'll try to
clarify, but if you think it could be useful to share it, please let me
know.
What driver is the above code in? Is it for MMC that needs a GPIO to
function? I'll assume it is for now.
The driver on which I'm working in is drivers/mmc/fsl_esdhc_imx.c, I'm
adding support for OF_PLATDATA to it, and in this sense trying to get
the GPIOs used for CD to be requested.
Then the weird thing is that we are accessing the dtplat of another
device. It's a clever technique but I wonder if we can find another
way.
If you see drivers/mmc/rockchip_sdhci.c it has:
ret = clk_get_by_index_platdata(dev, 0, dtplat->clocks, &clk);
So I wonder if we need gpio_dev_request_by_platdata()?
Thanks for pointing to this example, as I saw it before starting to work
on these functions and had some doubts. I'll use it in the next
paragraph to share my thoughts and the motivation of my work.
From my understanding, clk_get_by_index_platdata in this context can
only get a UCLASS_CLK device with id == 0. Is this correct?
If it is so, this will only allow us to use this function if we know in
advance that the UCLASS_CLK device has index 0.
How can we get the correct UCLASS_CLK device in case of multiple instances?
I understand that we need a way to use the link information present in
platdata. However I could not find a way to get the actual index of the
UCLASS_CLK device. In this context, I thought that the simplest but
still valid approach could be to find the right device based on the
struct platdata pointer it owns.
So in my understanding, your code could be more generic in this way
diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c
index 71878474eb..61041bb3b8 100644
--- a/drivers/clk/clk-uclass.c
+++ b/drivers/clk/clk-uclass.c
@@ -25,14 +25,12 @@ static inline const struct clk_ops *clk_dev_ops(struct
udevice *dev)
#if CONFIG_IS_ENABLED(OF_CONTROL)
# if CONFIG_IS_ENABLED(OF_PLATDATA)
-int clk_get_by_index_platdata(struct udevice *dev, int index,
- struct phandle_1_arg *cells, struct clk *clk)
+int clk_get_by_platdata(struct udevice *dev, struct phandle_1_arg *cells,
+ struct clk *clk)
{
int ret;
- if (index !=
Re: [RFC] dm: uclass: add functions to get device by platdata
Hi Walter,
On Thu, 5 Mar 2020 at 06:54, Walter Lozano wrote:
>
> Hi Simon,
>
> Thanks for taking the time to check for my comments
>
> On 4/3/20 20:11, Simon Glass wrote:
>
> > Hi Walter,
> >
> > On Wed, 4 Mar 2020 at 12:40, Walter Lozano
> > wrote:
> >> When OF_PLATDATA is enabled DT information is parsed and platdata
> >> structures are populated. In this context the links between DT nodes are
> >> represented as pointers to platdata structures, and there is no clear way
> >> to access to the device which owns the structure.
> >>
> >> This patch implements a set of functions:
> >>
> >> - device_find_by_platdata
> >> - uclass_find_device_by_platdata
> >>
> >> to access to the device.
> >>
> >> Signed-off-by: Walter Lozano
> >> ---
> >> drivers/core/device.c| 19 +++
> >> drivers/core/uclass.c| 34 ++
> >> include/dm/device.h | 2 ++
> >> include/dm/uclass-internal.h | 3 +++
> >> include/dm/uclass.h | 2 ++
> >> 5 files changed, 60 insertions(+)
> > This is interesting. Could you also add the motivation for this? It's
> > not clear to me who would call this function.
>
> I have been reviewing the OF_PLATDATA support as an R&D project, in this
> context, in order to have
> a better understanding on the possibilities and limitations I decided to add
> its support to iMX6,
> more particularly to the MMC drivers. The link issue arises when I tried to
> setup the GPIO for
> Card Detection, which is trivial when DT is available. However, when
> OF_PLATDATA is enabled
> this seems, at least for me, not straightforward.
>
> In order to overcome this limitation I think that having a set of functions
> to find/get devices
> based on platdata could be useful. Of course, there might be a better
> approach/idea, so that is
> the motivation for this RFC.
>
> An example of the usage could be
>
> #if CONFIG_IS_ENABLED(DM_GPIO)
>
> struct udevice *gpiodev;
>
> ret = uclass_get_device_by_platdata(UCLASS_GPIO, (void
> *)dtplat->cd_gpios->node, &gpiodev);
>
> if (ret)
> return ret;
>
> ret = gpio_dev_request_index(gpiodev, gpiodev->name, "cd-gpios",
> dtplat->cd_gpios->arg[0], GPIOD_IS_IN,
> dtplat->cd_gpios->arg[1],
> &priv->cd_gpio);
>
> if (ret)
> return ret;
>
> #endif
>
> This is part of my current work, a series of patches to add OF_PLATDATA
> support as explained.
>
> Does this make sense to you?
Not yet :-)
What is the context of this call? Typically dtplat is only available
in the driver that includes it.
What driver is the above code in? Is it for MMC that needs a GPIO to
function? I'll assume it is for now.
Then the weird thing is that we are accessing the dtplat of another
device. It's a clever technique but I wonder if we can find another
way.
If you see drivers/mmc/rockchip_sdhci.c it has:
ret = clk_get_by_index_platdata(dev, 0, dtplat->clocks, &clk);
So I wonder if we need gpio_dev_request_by_platdata()?
>
> > Also it relates to another thing I've been thinking about for a while,
> > which is to validate that all the structs pointed to are correct.
> >
> > E.g. if every struct had a magic number like:
> >
> > struct tpm_platdata {
> > DM_STRUCT(UCLASS_TPM, DM_STRUCT_PLATDATA, ...)
> > fields here
> > };
> >
> > then we could check the structure pointers are correct.
> >
> > DM_STRUCT() would define to nothing if we were not building with
> > CONFIG_DM_DEBUG or similar.
>
> Interesting, I think it could be useful and save us from headaches while
> debugging.
>
> Thanks for sharing this idea.
>
> > Anyway, I wonder whether you could expand your definition a bit so you
> > have an enum for the different types of struct you can request:
> >
> > enum dm_struct_t {
> > DM_STRUCT_PLATDATA,
> > ...
> >
> > DM_STRUCT_COUNT,
> > };
> >
> > and modify the function so it can request it via the enum?
>
> Let me check if I understand correctly, your suggestion is to do
> something like diff --git a/include/dm/uclass.h b/include/dm/uclass.h
> index 92c07f8426..bf09dadf3f 100644 --- a/include/dm/uclass.h +++
> b/include/dm/uclass.h
>
> @@ -167,8 +167,8 @@ int uclass_get_device(enum uclass_id id, int index,
> struct udevice **devp);
>
> int uclass_get_device_by_name(enum uclass_id id, const char *name,
>struct udevice **devp); -int
> uclass_get_device_by_platdata(enum uclass_id id, void *platdata,
> - struct udevice **devp);
>
> +int uclass_get_device_by_struct(enum uclass_id id, enum dm_struct_t
> struct_id, + void *struct_pointer, struct
> udevice **devp); /** * uclass_get_device_by_seq() - Get a uclass
> device based on an ID and sequence *
>
> If that is the case, I would be happy to help.
>
> Also, if my understanding is correct, could you elab
Re: [RFC] dm: uclass: add functions to get device by platdata
Hi Simon, On 5/3/20 10:54, Walter Lozano wrote: Let me check if I understand correctly, your suggestion is to do something like diff --git a/include/dm/uclass.h b/include/dm/uclass.h index 92c07f8426..bf09dadf3f 100644 --- a/include/dm/uclass.h +++ b/include/dm/uclass.h @@ -167,8 +167,8 @@ int uclass_get_device(enum uclass_id id, int index, struct udevice **devp); int uclass_get_device_by_name(enum uclass_id id, const char *name, struct udevice **devp); -int uclass_get_device_by_platdata(enum uclass_id id, void *platdata, - struct udevice **devp); +int uclass_get_device_by_struct(enum uclass_id id, enum dm_struct_t struct_id, + void *struct_pointer, struct udevice **devp); /** * uclass_get_device_by_seq() - Get a uclass device based on an ID and sequence * If that is the case, I would be happy to help. Also, if my understanding is correct, could you elaborate which cases are you trying to cover with this approach? Sorry, it looks like the last part of the email test got screw. If I understand correctly, your suggestion is to change diff --git a/include/dm/uclass.h b/include/dm/uclass.h index 92c07f8426..bf09dadf3f 100644 --- a/include/dm/uclass.h +++ b/include/dm/uclass.h @@ -167,8 +167,8 @@ int uclass_get_device(enum uclass_id id, int index, struct udevice **devp); int uclass_get_device_by_name(enum uclass_id id, const char *name, struct udevice **devp); -int uclass_get_device_by_platdata(enum uclass_id id, void *platdata, - struct udevice **devp); +int uclass_get_device_by_struct(enum uclass_id id, enum dm_struct_t , struct_id, + void *struct_pointer, struct udevice **devp); /** * uclass_get_device_by_seq() - Get a uclass device based on an ID and sequence * If that is the case, I would be happy to help. Also, if my understanding is correct, could you elaborate which cases are trying to cover with this approach? Regards, Walter
Re: [RFC] dm: uclass: add functions to get device by platdata
Hi Simon,
Thanks for taking the time to check for my comments
On 4/3/20 20:11, Simon Glass wrote:
Hi Walter,
On Wed, 4 Mar 2020 at 12:40, Walter Lozano wrote:
When OF_PLATDATA is enabled DT information is parsed and platdata
structures are populated. In this context the links between DT nodes are
represented as pointers to platdata structures, and there is no clear way
to access to the device which owns the structure.
This patch implements a set of functions:
- device_find_by_platdata
- uclass_find_device_by_platdata
to access to the device.
Signed-off-by: Walter Lozano
---
drivers/core/device.c| 19 +++
drivers/core/uclass.c| 34 ++
include/dm/device.h | 2 ++
include/dm/uclass-internal.h | 3 +++
include/dm/uclass.h | 2 ++
5 files changed, 60 insertions(+)
This is interesting. Could you also add the motivation for this? It's
not clear to me who would call this function.
I have been reviewing the OF_PLATDATA support as an R&D project, in this
context, in order to have
a better understanding on the possibilities and limitations I decided to add
its support to iMX6,
more particularly to the MMC drivers. The link issue arises when I tried to
setup the GPIO for
Card Detection, which is trivial when DT is available. However, when
OF_PLATDATA is enabled
this seems, at least for me, not straightforward.
In order to overcome this limitation I think that having a set of functions to
find/get devices
based on platdata could be useful. Of course, there might be a better
approach/idea, so that is
the motivation for this RFC.
An example of the usage could be
#if CONFIG_IS_ENABLED(DM_GPIO)
struct udevice *gpiodev;
ret = uclass_get_device_by_platdata(UCLASS_GPIO, (void
*)dtplat->cd_gpios->node, &gpiodev);
if (ret)
return ret;
ret = gpio_dev_request_index(gpiodev, gpiodev->name, "cd-gpios",
dtplat->cd_gpios->arg[0], GPIOD_IS_IN,
dtplat->cd_gpios->arg[1], &priv->cd_gpio);
if (ret)
return ret;
#endif
This is part of my current work, a series of patches to add OF_PLATDATA support
as explained.
Does this make sense to you?
Also it relates to another thing I've been thinking about for a while,
which is to validate that all the structs pointed to are correct.
E.g. if every struct had a magic number like:
struct tpm_platdata {
DM_STRUCT(UCLASS_TPM, DM_STRUCT_PLATDATA, ...)
fields here
};
then we could check the structure pointers are correct.
DM_STRUCT() would define to nothing if we were not building with
CONFIG_DM_DEBUG or similar.
Interesting, I think it could be useful and save us from headaches while
debugging.
Thanks for sharing this idea.
Anyway, I wonder whether you could expand your definition a bit so you
have an enum for the different types of struct you can request:
enum dm_struct_t {
DM_STRUCT_PLATDATA,
...
DM_STRUCT_COUNT,
};
and modify the function so it can request it via the enum?
Let me check if I understand correctly, your suggestion is to do
something like diff --git a/include/dm/uclass.h b/include/dm/uclass.h
index 92c07f8426..bf09dadf3f 100644 --- a/include/dm/uclass.h +++
b/include/dm/uclass.h
@@ -167,8 +167,8 @@ int uclass_get_device(enum uclass_id id, int index,
struct udevice **devp);
int uclass_get_device_by_name(enum uclass_id id, const char *name,
struct udevice **devp); -int
uclass_get_device_by_platdata(enum uclass_id id, void *platdata,
- struct udevice **devp);
+int uclass_get_device_by_struct(enum uclass_id id, enum dm_struct_t
struct_id, + void *struct_pointer, struct
udevice **devp); /** * uclass_get_device_by_seq() - Get a uclass
device based on an ID and sequence *
If that is the case, I would be happy to help.
Also, if my understanding is correct, could you elaborate which cases
are you trying to cover with this approach? Regards,
Walter
Re: [RFC] dm: uclass: add functions to get device by platdata
Hi Walter,
On Wed, 4 Mar 2020 at 12:40, Walter Lozano wrote:
>
> When OF_PLATDATA is enabled DT information is parsed and platdata
> structures are populated. In this context the links between DT nodes are
> represented as pointers to platdata structures, and there is no clear way
> to access to the device which owns the structure.
>
> This patch implements a set of functions:
>
> - device_find_by_platdata
> - uclass_find_device_by_platdata
>
> to access to the device.
>
> Signed-off-by: Walter Lozano
> ---
> drivers/core/device.c| 19 +++
> drivers/core/uclass.c| 34 ++
> include/dm/device.h | 2 ++
> include/dm/uclass-internal.h | 3 +++
> include/dm/uclass.h | 2 ++
> 5 files changed, 60 insertions(+)
This is interesting. Could you also add the motivation for this? It's
not clear to me who would call this function.
Also it relates to another thing I've been thinking about for a while,
which is to validate that all the structs pointed to are correct.
E.g. if every struct had a magic number like:
struct tpm_platdata {
DM_STRUCT(UCLASS_TPM, DM_STRUCT_PLATDATA, ...)
fields here
};
then we could check the structure pointers are correct.
DM_STRUCT() would define to nothing if we were not building with
CONFIG_DM_DEBUG or similar.
Anyway, I wonder whether you could expand your definition a bit so you
have an enum for the different types of struct you can request:
enum dm_struct_t {
DM_STRUCT_PLATDATA,
...
DM_STRUCT_COUNT,
};
and modify the function so it can request it via the enum?
Regards,
Simon
[RFC] dm: uclass: add functions to get device by platdata
When OF_PLATDATA is enabled DT information is parsed and platdata
structures are populated. In this context the links between DT nodes are
represented as pointers to platdata structures, and there is no clear way
to access to the device which owns the structure.
This patch implements a set of functions:
- device_find_by_platdata
- uclass_find_device_by_platdata
to access to the device.
Signed-off-by: Walter Lozano
---
drivers/core/device.c| 19 +++
drivers/core/uclass.c| 34 ++
include/dm/device.h | 2 ++
include/dm/uclass-internal.h | 3 +++
include/dm/uclass.h | 2 ++
5 files changed, 60 insertions(+)
diff --git a/drivers/core/device.c b/drivers/core/device.c
index 89ea820d48..54a3a8d870 100644
--- a/drivers/core/device.c
+++ b/drivers/core/device.c
@@ -591,6 +591,25 @@ static int device_find_by_ofnode(ofnode node, struct
udevice **devp)
}
#endif
+
+int device_find_by_platdata(void *platdata, struct udevice **devp)
+{
+ struct uclass *uc;
+ struct udevice *dev;
+ int ret;
+
+ list_for_each_entry(uc, &gd->uclass_root, sibling_node) {
+ ret = uclass_find_device_by_platdata(uc->uc_drv->id, platdata,
+ &dev);
+ if (!ret || dev) {
+ *devp = dev;
+ return 0;
+ }
+ }
+
+ return -ENODEV;
+}
+
int device_get_child(const struct udevice *parent, int index,
struct udevice **devp)
{
diff --git a/drivers/core/uclass.c b/drivers/core/uclass.c
index 58b19a4210..7b0ae5b122 100644
--- a/drivers/core/uclass.c
+++ b/drivers/core/uclass.c
@@ -271,6 +271,29 @@ int uclass_find_device_by_name(enum uclass_id id, const
char *name,
return -ENODEV;
}
+int uclass_find_device_by_platdata(enum uclass_id id, void * platdata, struct
udevice **devp)
+{
+ struct uclass *uc;
+ struct udevice *dev;
+ int ret;
+
+ *devp = NULL;
+ ret = uclass_get(id, &uc);
+ if (ret)
+ return ret;
+ if (list_empty(&uc->dev_head))
+ return -ENODEV;
+
+ uclass_foreach_dev(dev, uc) {
+ if (dev->platdata == platdata) {
+ *devp = dev;
+ return 0;
+ }
+ }
+
+ return -ENODEV;
+}
+
int uclass_find_next_free_req_seq(enum uclass_id id)
{
struct uclass *uc;
@@ -466,6 +489,17 @@ int uclass_get_device_by_name(enum uclass_id id, const
char *name,
return uclass_get_device_tail(dev, ret, devp);
}
+int uclass_get_device_by_platdata(enum uclass_id id, void * platdata,
+ struct udevice **devp)
+{
+ struct udevice *dev;
+ int ret;
+
+ *devp = NULL;
+ ret = uclass_find_device_by_platdata(id, platdata, &dev);
+ return uclass_get_device_tail(dev, ret, devp);
+}
+
int uclass_get_device_by_seq(enum uclass_id id, int seq, struct udevice **devp)
{
struct udevice *dev;
diff --git a/include/dm/device.h b/include/dm/device.h
index ab806d0b7e..3c043a3127 100644
--- a/include/dm/device.h
+++ b/include/dm/device.h
@@ -396,6 +396,8 @@ enum uclass_id device_get_uclass_id(const struct udevice
*dev);
*/
const char *dev_get_uclass_name(const struct udevice *dev);
+int device_find_by_platdata(void *platdata, struct udevice **devp);
+
/**
* device_get_child() - Get the child of a device by index
*
diff --git a/include/dm/uclass-internal.h b/include/dm/uclass-internal.h
index 6e3f15c2b0..f643fc95da 100644
--- a/include/dm/uclass-internal.h
+++ b/include/dm/uclass-internal.h
@@ -100,6 +100,9 @@ int uclass_find_next_device(struct udevice **devp);
int uclass_find_device_by_name(enum uclass_id id, const char *name,
struct udevice **devp);
+int uclass_find_device_by_platdata(enum uclass_id id, void *platdata,
+ struct udevice **devp);
+
/**
* uclass_find_device_by_seq() - Find uclass device based on ID and sequence
*
diff --git a/include/dm/uclass.h b/include/dm/uclass.h
index 70fca79b44..92c07f8426 100644
--- a/include/dm/uclass.h
+++ b/include/dm/uclass.h
@@ -167,6 +167,8 @@ int uclass_get_device(enum uclass_id id, int index, struct
udevice **devp);
int uclass_get_device_by_name(enum uclass_id id, const char *name,
struct udevice **devp);
+int uclass_get_device_by_platdata(enum uclass_id id, void *platdata,
+ struct udevice **devp);
/**
* uclass_get_device_by_seq() - Get a uclass device based on an ID and sequence
*
--
2.20.1
