| Message ID | 1436348838-22671-6-git-send-email-rnayak@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 07/08, Rajendra Nayak wrote: > diff --git a/drivers/clk/qcom/gcc-msm8960.c b/drivers/clk/qcom/gcc-msm8960.c > index eb6a4f9..2c80d03 100644 > --- a/drivers/clk/qcom/gcc-msm8960.c > +++ b/drivers/clk/qcom/gcc-msm8960.c > @@ -15,6 +15,7 @@ > #include <linux/bitops.h> > #include <linux/err.h> > #include <linux/platform_device.h> > +#include <linux/of_platform.h> > #include <linux/module.h> > #include <linux/of.h> > #include <linux/of_device.h> > @@ -3520,7 +3521,8 @@ static int gcc_msm8960_probe(struct platform_device *pdev) > if (IS_ERR(clk)) > return PTR_ERR(clk); > > - return qcom_cc_probe(pdev, match->data); > + qcom_cc_probe(pdev, match->data); > + return of_platform_populate(pdev->dev.of_node, NULL, NULL, &pdev->dev); We just lost the error code from qcom_cc_probe()... Also, I don't like having a subnode in DT. Why can't we use the same node as the GCC node and create a virtual child device here for tsens? We can assign the same of_node that this platform device has so that DT keeps working correctly.
On 11/08/15 23:49, Stephen Boyd wrote: > On 07/08, Rajendra Nayak wrote: >> diff --git a/drivers/clk/qcom/gcc-msm8960.c b/drivers/clk/qcom/gcc-msm8960.c >> index eb6a4f9..2c80d03 100644 >> --- a/drivers/clk/qcom/gcc-msm8960.c >> +++ b/drivers/clk/qcom/gcc-msm8960.c >> @@ -15,6 +15,7 @@ >> #include <linux/bitops.h> >> #include <linux/err.h> >> #include <linux/platform_device.h> >> +#include <linux/of_platform.h> >> #include <linux/module.h> >> #include <linux/of.h> >> #include <linux/of_device.h> >> @@ -3520,7 +3521,8 @@ static int gcc_msm8960_probe(struct platform_device *pdev) >> if (IS_ERR(clk)) >> return PTR_ERR(clk); >> >> - return qcom_cc_probe(pdev, match->data); >> + qcom_cc_probe(pdev, match->data); >> + return of_platform_populate(pdev->dev.of_node, NULL, NULL, &pdev->dev); > > We just lost the error code from qcom_cc_probe()... > I think Rajendra picked up the wrong patch for this series, I did submit a v2 (https://patches.linaro.org/44033/) with the above fixed. > Also, I don't like having a subnode in DT. Why can't we use the > same node as the GCC node and create a virtual child device here > for tsens? We can assign the same of_node that this platform > device has so that DT keeps working correctly. I don't mind either way, if that makes things moving :-) --srini >
On Tue 11 Aug 15:49 PDT 2015, Stephen Boyd wrote: > On 07/08, Rajendra Nayak wrote: > > diff --git a/drivers/clk/qcom/gcc-msm8960.c b/drivers/clk/qcom/gcc-msm8960.c > > index eb6a4f9..2c80d03 100644 > > --- a/drivers/clk/qcom/gcc-msm8960.c > > +++ b/drivers/clk/qcom/gcc-msm8960.c > > @@ -15,6 +15,7 @@ > > #include <linux/bitops.h> > > #include <linux/err.h> > > #include <linux/platform_device.h> > > +#include <linux/of_platform.h> > > #include <linux/module.h> > > #include <linux/of.h> > > #include <linux/of_device.h> > > @@ -3520,7 +3521,8 @@ static int gcc_msm8960_probe(struct platform_device *pdev) > > if (IS_ERR(clk)) > > return PTR_ERR(clk); > > > > - return qcom_cc_probe(pdev, match->data); > > + qcom_cc_probe(pdev, match->data); > > + return of_platform_populate(pdev->dev.of_node, NULL, NULL, &pdev->dev); > > We just lost the error code from qcom_cc_probe()... > > Also, I don't like having a subnode in DT. Why can't we use the > same node as the GCC node and create a virtual child device here > for tsens? We can assign the same of_node that this platform > device has so that DT keeps working correctly. > Can't we make the gcc driver support being a child of a simple-mfd by having it attempting to acquire the regmap of the parent and falling back to creating its own mmio regmap? That way we don't need to make tsense a child of the clock device and we're still backwards compatible. Regards, Bjorn
> On 11/08/15 23:49, Stephen Boyd wrote: >> On 07/08, Rajendra Nayak wrote: >>> diff --git a/drivers/clk/qcom/gcc-msm8960.c >>> b/drivers/clk/qcom/gcc-msm8960.c >>> index eb6a4f9..2c80d03 100644 >>> --- a/drivers/clk/qcom/gcc-msm8960.c >>> +++ b/drivers/clk/qcom/gcc-msm8960.c >>> @@ -15,6 +15,7 @@ >>> #include <linux/bitops.h> >>> #include <linux/err.h> >>> #include <linux/platform_device.h> >>> +#include <linux/of_platform.h> >>> #include <linux/module.h> >>> #include <linux/of.h> >>> #include <linux/of_device.h> >>> @@ -3520,7 +3521,8 @@ static int gcc_msm8960_probe(struct >>> platform_device *pdev) >>> if (IS_ERR(clk)) >>> return PTR_ERR(clk); >>> >>> - return qcom_cc_probe(pdev, match->data); >>> + qcom_cc_probe(pdev, match->data); >>> + return of_platform_populate(pdev->dev.of_node, NULL, NULL, >>> &pdev->dev); >> >> We just lost the error code from qcom_cc_probe()... >> > I think Rajendra picked up the wrong patch for this series, I did submit > a v2 (https://patches.linaro.org/44033/) with the above fixed. ah, sorry about that. >> Also, I don't like having a subnode in DT. Why can't we use the >> same node as the GCC node and create a virtual child device here >> for tsens? We can assign the same of_node that this platform >> device has so that DT keeps working correctly. So the current driver looks up data based on compatible strings. So you suggesting to create a virtual child device for gcc and associate the gcc DT node to it? (And have the tsens compatible mentioned as part of the gcc DT node?) How is this any different from creating a subnode in DT anyway?
On 08/13, Rajendra Nayak wrote: > >On 11/08/15 23:49, Stephen Boyd wrote: > >>On 07/08, Rajendra Nayak wrote: > >>>diff --git a/drivers/clk/qcom/gcc-msm8960.c > >>>b/drivers/clk/qcom/gcc-msm8960.c > >>>index eb6a4f9..2c80d03 100644 > >>>--- a/drivers/clk/qcom/gcc-msm8960.c > >>>+++ b/drivers/clk/qcom/gcc-msm8960.c > >>>@@ -15,6 +15,7 @@ > >>> #include <linux/bitops.h> > >>> #include <linux/err.h> > >>> #include <linux/platform_device.h> > >>>+#include <linux/of_platform.h> > >>> #include <linux/module.h> > >>> #include <linux/of.h> > >>> #include <linux/of_device.h> > >>>@@ -3520,7 +3521,8 @@ static int gcc_msm8960_probe(struct > >>>platform_device *pdev) > >>> if (IS_ERR(clk)) > >>> return PTR_ERR(clk); > >>> > >>>- return qcom_cc_probe(pdev, match->data); > >>>+ qcom_cc_probe(pdev, match->data); > >>>+ return of_platform_populate(pdev->dev.of_node, NULL, NULL, > >>>&pdev->dev); > >> > >>We just lost the error code from qcom_cc_probe()... > >> > >I think Rajendra picked up the wrong patch for this series, I did submit > >a v2 (https://patches.linaro.org/44033/) with the above fixed. > > ah, sorry about that. > > >>Also, I don't like having a subnode in DT. Why can't we use the > >>same node as the GCC node and create a virtual child device here > >>for tsens? We can assign the same of_node that this platform > >>device has so that DT keeps working correctly. > > So the current driver looks up data based on compatible strings. The tsens device is always the same piece of hardware. The only thing that's changing is the qfprom data and the number of sensors. So we should be looking at the qfprom compatible string to figure out how to interpret the qfprom data which would include the number of sensors and how the data is encoded. > So you suggesting to create a virtual child device for gcc and > associate the gcc DT node to it? (And have the tsens compatible > mentioned as part of the gcc DT node?) No. The driver should work just fine without having to interrogate the device's compatible string. If we still need the compatible check for some reason, then we can always match based on qcom,gcc-msm8960, qcom,gcc-apq8064, etc. But I don't see why we need to do that when we should be looking at what type of qfprom is connected so we can correctly parse the data. > How is this any different from creating a subnode in DT anyway? The difference is we don't make up nodes to satisfy linux device driver design. I suspect the hardware engineers put tsens inside gcc because both blocks were written by the same person/team and they just needed some place to jam some registers and call it a day. That doesn't constitute an MFD or bus, which is what we would be expressing in DT if we made a child node, it constitutes a horrible software interface design that we have to live with.
[].. >>>> Also, I don't like having a subnode in DT. Why can't we use the >>>> same node as the GCC node and create a virtual child device here >>>> for tsens? We can assign the same of_node that this platform >>>> device has so that DT keeps working correctly. >> >> So the current driver looks up data based on compatible strings. > > The tsens device is always the same piece of hardware. The only Well, not always. The one in 8960 does need additional initializations, requires you to save/restore context as it can be powered off not being in an always powered on domain etc. > thing that's changing is the qfprom data and the number of > sensors. So we should be looking at the qfprom compatible string How? Tsens uses nvmem framework apis to read the qfprom atleast in this series. > to figure out how to interpret the qfprom data which would > include the number of sensors and how the data is encoded. > >> So you suggesting to create a virtual child device for gcc and >> associate the gcc DT node to it? (And have the tsens compatible >> mentioned as part of the gcc DT node?) > > No. The driver should work just fine without having to > interrogate the device's compatible string. If we still need the > compatible check for some reason, then we can always match based > on qcom,gcc-msm8960, qcom,gcc-apq8064, etc. But I don't see why Thats not quite possible I guess. 2 drivers (gcc and tsens) matching the same compatibles? Will it not just depend on which ends up being the first match? > we need to do that when we should be looking at what type of > qfprom is connected so we can correctly parse the data.
Stephen, >>>>> Also, I don't like having a subnode in DT. Why can't we use the >>>>> same node as the GCC node and create a virtual child device here >>>>> for tsens? We can assign the same of_node that this platform >>>>> device has so that DT keeps working correctly. >>> >>> So the current driver looks up data based on compatible strings. >> >> The tsens device is always the same piece of hardware. The only > > Well, not always. The one in 8960 does need additional initializations, > requires you to save/restore context as it can be powered off > not being in an always powered on domain etc. > >> thing that's changing is the qfprom data and the number of >> sensors. So we should be looking at the qfprom compatible string > > How? Tsens uses nvmem framework apis to read the qfprom atleast > in this series. > >> to figure out how to interpret the qfprom data which would >> include the number of sensors and how the data is encoded. >> >>> So you suggesting to create a virtual child device for gcc and >>> associate the gcc DT node to it? (And have the tsens compatible >>> mentioned as part of the gcc DT node?) >> >> No. The driver should work just fine without having to >> interrogate the device's compatible string. If we still need the >> compatible check for some reason, then we can always match based >> on qcom,gcc-msm8960, qcom,gcc-apq8064, etc. But I don't see why > > Thats not quite possible I guess. 2 drivers (gcc and tsens) matching > the same compatibles? Will it not just depend on which ends up being > the first match? Any thoughts on how to move forward with this? I tried what you were suggesting, and here's what I had to do to get things working * Created a gcc node in DT with gcc and tsens compatibles, with gcc and tsens properties * gcc driver probes the device/node first given gcc is registered with a core_initcall() * creates a virtual child device attaching the same of_node (having both gcc and tsens compatibles) * gcc ends up probing the virtual device/node _again_ (due to the gcc compatible match), fails * At a later point, tsens driver gets registered (using module_initcall) ends up probing the virtual child node and succeeds Is this what you had in mind, or am I at the wrong end of the stick? regards, Rajendra
On 09/02, Rajendra Nayak wrote: > Stephen, > > >>>>>Also, I don't like having a subnode in DT. Why can't we use the > >>>>>same node as the GCC node and create a virtual child device here > >>>>>for tsens? We can assign the same of_node that this platform > >>>>>device has so that DT keeps working correctly. > >>> > >>>So the current driver looks up data based on compatible strings. > >> > >>The tsens device is always the same piece of hardware. The only > > > >Well, not always. The one in 8960 does need additional initializations, > >requires you to save/restore context as it can be powered off > >not being in an always powered on domain etc. > > > >>thing that's changing is the qfprom data and the number of > >>sensors. So we should be looking at the qfprom compatible string > > > >How? Tsens uses nvmem framework apis to read the qfprom atleast > >in this series. > > > >>to figure out how to interpret the qfprom data which would > >>include the number of sensors and how the data is encoded. > >> > >>>So you suggesting to create a virtual child device for gcc and > >>>associate the gcc DT node to it? (And have the tsens compatible > >>>mentioned as part of the gcc DT node?) > >> > >>No. The driver should work just fine without having to > >>interrogate the device's compatible string. If we still need the > >>compatible check for some reason, then we can always match based > >>on qcom,gcc-msm8960, qcom,gcc-apq8064, etc. But I don't see why > > > >Thats not quite possible I guess. 2 drivers (gcc and tsens) matching > >the same compatibles? Will it not just depend on which ends up being > >the first match? > > Any thoughts on how to move forward with this? > > I tried what you were suggesting, and here's what I had to do to get > things working > > * Created a gcc node in DT with gcc and tsens compatibles, with gcc and > tsens properties This is not what I had in mind. This is what's should be in DT clock-controller@f000 { compatible = "qcom,gcc-msm8916"; reg = <0xf000 ...>; .... }; > * gcc driver probes the device/node first given gcc is registered with > a core_initcall() > * creates a virtual child device attaching the same of_node (having > both gcc and tsens compatibles) > * gcc ends up probing the virtual device/node _again_ (due to the gcc > compatible match), fails > * At a later point, tsens driver gets registered (using module_initcall) > ends up probing the virtual child node and succeeds Yeah this might happen though because we've assigned the of_node pointer to the tsens device before we register it on the platform bus. The other way to pass that data down from gcc to tsens would be to not have an of_node assigned to the tsens device, and check for that case in the tsens driver. If there isn't an of_node, then we look at the parent device's of_node to figure out which gcc it is (if this even matters) and parse DT properties.
[].. >>>> No. The driver should work just fine without having to >>>> interrogate the device's compatible string. If we still need the >>>> compatible check for some reason, then we can always match based >>>> on qcom,gcc-msm8960, qcom,gcc-apq8064, etc. But I don't see why >>> >>> Thats not quite possible I guess. 2 drivers (gcc and tsens) matching >>> the same compatibles? Will it not just depend on which ends up being >>> the first match? >> >> Any thoughts on how to move forward with this? >> >> I tried what you were suggesting, and here's what I had to do to get >> things working >> >> * Created a gcc node in DT with gcc and tsens compatibles, with gcc and >> tsens properties > > This is not what I had in mind. This is what's should be in DT > > clock-controller@f000 { > compatible = "qcom,gcc-msm8916"; > reg = <0xf000 ...>; > .... > }; > >> * gcc driver probes the device/node first given gcc is registered with >> a core_initcall() >> * creates a virtual child device attaching the same of_node (having >> both gcc and tsens compatibles) >> * gcc ends up probing the virtual device/node _again_ (due to the gcc >> compatible match), fails >> * At a later point, tsens driver gets registered (using module_initcall) >> ends up probing the virtual child node and succeeds > > Yeah this might happen though because we've assigned the of_node > pointer to the tsens device before we register it on the platform > bus. The other way to pass that data down from gcc to tsens would > be to not have an of_node assigned to the tsens device, and check > for that case in the tsens driver. If there isn't an of_node, > then we look at the parent device's of_node to figure out which > gcc it is (if this even matters) and parse DT properties. Parsing DT properties from parent (in the tsens driver) is fine, but the nvmem apis still expect an of_node for the tsens device and hence fail. Associating the of_node of the parent to the tsens device while being probed ends up with the same issues of gcc ending up probing the device and failing because tsens defers probe a couple times before a successful probe.
On 09/07, Rajendra Nayak wrote: > > > >Yeah this might happen though because we've assigned the of_node > >pointer to the tsens device before we register it on the platform > >bus. The other way to pass that data down from gcc to tsens would > >be to not have an of_node assigned to the tsens device, and check > >for that case in the tsens driver. If there isn't an of_node, > >then we look at the parent device's of_node to figure out which > >gcc it is (if this even matters) and parse DT properties. > > Parsing DT properties from parent (in the tsens driver) is fine, but > the nvmem apis still expect an of_node for the tsens device and hence > fail. So pass the parent device to the nvmem APIs? Or adjust the nvmem APIs to look for a parent of_node if there isn't an of_node for the device being passed? Or make the nvmem APIs work without using DT, and copy over the nvmem information from the gcc node to the virtual tsens child device? > > Associating the of_node of the parent to the tsens device while being > probed ends up with the same issues of gcc ending up probing the device > and failing because tsens defers probe a couple times before a > successful probe. Yeah sounds like we shouldn't do it that way.
On 09/09/2015 12:51 AM, Stephen Boyd wrote: > On 09/07, Rajendra Nayak wrote: >>> >>> Yeah this might happen though because we've assigned the of_node >>> pointer to the tsens device before we register it on the platform >>> bus. The other way to pass that data down from gcc to tsens would >>> be to not have an of_node assigned to the tsens device, and check >>> for that case in the tsens driver. If there isn't an of_node, >>> then we look at the parent device's of_node to figure out which >>> gcc it is (if this even matters) and parse DT properties. >> >> Parsing DT properties from parent (in the tsens driver) is fine, but >> the nvmem apis still expect an of_node for the tsens device and hence >> fail. > > So pass the parent device to the nvmem APIs? Or adjust the nvmem > APIs to look for a parent of_node if there isn't an of_node for > the device being passed? Or make the nvmem APIs work without > using DT, and copy over the nvmem information from the gcc node > to the virtual tsens child device? Srini, you being the nvmem maintainer, any thoughts?
On 09/09/2015 09:03 AM, Rajendra Nayak wrote: > > On 09/09/2015 12:51 AM, Stephen Boyd wrote: >> On 09/07, Rajendra Nayak wrote: >>>> >>>> Yeah this might happen though because we've assigned the of_node >>>> pointer to the tsens device before we register it on the platform >>>> bus. The other way to pass that data down from gcc to tsens would >>>> be to not have an of_node assigned to the tsens device, and check >>>> for that case in the tsens driver. If there isn't an of_node, >>>> then we look at the parent device's of_node to figure out which >>>> gcc it is (if this even matters) and parse DT properties. >>> >>> Parsing DT properties from parent (in the tsens driver) is fine, but >>> the nvmem apis still expect an of_node for the tsens device and hence >>> fail. >> >> So pass the parent device to the nvmem APIs? Or adjust the nvmem >> APIs to look for a parent of_node if there isn't an of_node for >> the device being passed? Or make the nvmem APIs work without >> using DT, and copy over the nvmem information from the gcc node >> to the virtual tsens child device? > > Srini, you being the nvmem maintainer, any thoughts? passing the parent device to nvmem APIs seems the cleanest to me, without having to change much with the nvmem APIs itself.
On 09/09/15 04:33, Rajendra Nayak wrote: > > On 09/09/2015 12:51 AM, Stephen Boyd wrote: >> On 09/07, Rajendra Nayak wrote: >>>> >>>> Yeah this might happen though because we've assigned the of_node >>>> pointer to the tsens device before we register it on the platform >>>> bus. The other way to pass that data down from gcc to tsens would >>>> be to not have an of_node assigned to the tsens device, and check >>>> for that case in the tsens driver. If there isn't an of_node, >>>> then we look at the parent device's of_node to figure out which >>>> gcc it is (if this even matters) and parse DT properties. >>> >>> Parsing DT properties from parent (in the tsens driver) is fine, but >>> the nvmem apis still expect an of_node for the tsens device and hence >>> fail. >> >> So pass the parent device to the nvmem APIs? Or adjust the nvmem >> APIs to look for a parent of_node if there isn't an of_node for >> the device being passed? Or make the nvmem APIs work without >> using DT, and copy over the nvmem information from the gcc node >> to the virtual tsens child device? > > Srini, you being the nvmem maintainer, any thoughts? We could do either one of the below. 1> pass parent device for nvmem_get_cell as suggested by Stephen 2> use of_nvmem_get_cell() api to pass correct of_node Both of them would work. --srini >
diff --git a/drivers/clk/qcom/gcc-msm8960.c b/drivers/clk/qcom/gcc-msm8960.c index eb6a4f9..2c80d03 100644 --- a/drivers/clk/qcom/gcc-msm8960.c +++ b/drivers/clk/qcom/gcc-msm8960.c @@ -15,6 +15,7 @@ #include <linux/bitops.h> #include <linux/err.h> #include <linux/platform_device.h> +#include <linux/of_platform.h> #include <linux/module.h> #include <linux/of.h> #include <linux/of_device.h> @@ -3520,7 +3521,8 @@ static int gcc_msm8960_probe(struct platform_device *pdev) if (IS_ERR(clk)) return PTR_ERR(clk); - return qcom_cc_probe(pdev, match->data); + qcom_cc_probe(pdev, match->data); + return of_platform_populate(pdev->dev.of_node, NULL, NULL, &pdev->dev); } static int gcc_msm8960_remove(struct platform_device *pdev)