| Message ID | 1425915402-10012-1-git-send-email-eliad@wizery.com |
|---|---|
| State | Needs Review / ACK, archived |
| Headers | show |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | warning | total: 1 errors, 0 warnings, 0 lines checked |
| robh/patch-applied | success |
On Monday 09 March 2015 17:36:41 Eliad Peller wrote: > @@ -323,11 +388,14 @@ out: > static void wl1271_remove(struct sdio_func *func) > { > struct wl12xx_sdio_glue *glue = sdio_get_drvdata(func); > + struct wlcore_platdev_data *pdev_data = glue->core->dev.platform_data; > + struct wl12xx_platform_data *pdata = pdev_data->pdata; > > /* Undo decrement done above in wl1271_probe */ > pm_runtime_get_noresume(&func->dev); > > platform_device_unregister(glue->core); > + wlcore_del_platform_data(pdata); > kfree(glue); > } > > The third patch looks ok, but now you should remove the wl12xx_platform_data from the wlcore code, since it's not used any more, it was broken to start with (as it supports only one instance) and we want to prevent others from adding new users of that. Since the only thing you need is the irq number, you can directly add the irq number to struct wlcore_platdev_data and remove the pdata pointer there. Arnd -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Monday 09 March 2015 17:36:42 Eliad Peller wrote: > --- a/arch/arm/boot/dts/omap3-igep0030-rev-g.dts > +++ b/arch/arm/boot/dts/omap3-igep0030-rev-g.dts > @@ -64,4 +64,13 @@ > vmmc-supply = <&lbep5clwmc_wlen>; > bus-width = <4>; > non-removable; > + > + #address-cells = <1>; > + #size-cells = <0>; > + wlcore: wlcore@2 { > + compatible = "ti,wl1835"; > + reg = <2>; > + interrupt-parent = <&gpio5>; > + interrupts = <8 IRQ_TYPE_NONE>; > + }; > Why IRQ_TYPE_NONE? I was expecting you to remove all calls to legacy_init_wl12xx from this file, including the ones for wl12xx aside from the wl18xx ones you removed, but if that's enough to clean out the platform_data handling from the wlcore driver, it's good enough as a start. Arnd -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Mar 9, 2015 at 9:50 PM, Arnd Bergmann <arnd@arndb.de> wrote: > On Monday 09 March 2015 17:36:42 Eliad Peller wrote: >> --- a/arch/arm/boot/dts/omap3-igep0030-rev-g.dts >> +++ b/arch/arm/boot/dts/omap3-igep0030-rev-g.dts >> @@ -64,4 +64,13 @@ >> vmmc-supply = <&lbep5clwmc_wlen>; >> bus-width = <4>; >> non-removable; >> + >> + #address-cells = <1>; >> + #size-cells = <0>; >> + wlcore: wlcore@2 { >> + compatible = "ti,wl1835"; >> + reg = <2>; >> + interrupt-parent = <&gpio5>; >> + interrupts = <8 IRQ_TYPE_NONE>; >> + }; >> > > Why IRQ_TYPE_NONE? > i simply mirrored the current board file (which only sets the irq number). > I was expecting you to remove all calls to legacy_init_wl12xx from this file, > including the ones for wl12xx aside from the wl18xx ones you removed, but > if that's enough to clean out the platform_data handling from the wlcore > driver, it's good enough as a start. not sure i'm following - can you elaborate? i'll summarize the way i see it. please correct me if i'm wrong. both wl18xx and wl12xx use the platform data to get the irq number. wl12xx (only) also needs some additional clock definitions to be passed. there's currently some issue with specifying some the of clock sources, so i preferred starting only with (the simpler) wl18xx bindings. for platforms with wl18xx, we can remove the pdata-quirk, as all the data (i.e. irq) can be passed by the new DT bindings. however, for platforms with wl12xx, we still need to pass the clock definitions (along with the irq), so we have to keep legacy_init_wl12xx for the time being (and that's also why we have to currently keep the platform_data handling in the wlcore driver) do you have something else in mind? Eliad. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* Eliad Peller <eliad@wizery.com> [150309 14:03]: > On Mon, Mar 9, 2015 at 9:50 PM, Arnd Bergmann <arnd@arndb.de> wrote: > > On Monday 09 March 2015 17:36:42 Eliad Peller wrote: > >> --- a/arch/arm/boot/dts/omap3-igep0030-rev-g.dts > >> +++ b/arch/arm/boot/dts/omap3-igep0030-rev-g.dts > >> @@ -64,4 +64,13 @@ > >> vmmc-supply = <&lbep5clwmc_wlen>; > >> bus-width = <4>; > >> non-removable; > >> + > >> + #address-cells = <1>; > >> + #size-cells = <0>; > >> + wlcore: wlcore@2 { > >> + compatible = "ti,wl1835"; > >> + reg = <2>; > >> + interrupt-parent = <&gpio5>; > >> + interrupts = <8 IRQ_TYPE_NONE>; > >> + }; > >> > > > > Why IRQ_TYPE_NONE? > > > i simply mirrored the current board file (which only sets the irq number). > > > I was expecting you to remove all calls to legacy_init_wl12xx from this file, > > including the ones for wl12xx aside from the wl18xx ones you removed, but > > if that's enough to clean out the platform_data handling from the wlcore > > driver, it's good enough as a start. > not sure i'm following - can you elaborate? > > i'll summarize the way i see it. please correct me if i'm wrong. > > both wl18xx and wl12xx use the platform data to get the irq number. > wl12xx (only) also needs some additional clock definitions to be > passed. there's currently some issue with specifying some the of clock > sources, so i preferred starting only with (the simpler) wl18xx > bindings. > > for platforms with wl18xx, we can remove the pdata-quirk, as all the > data (i.e. irq) can be passed by the new DT bindings. > however, for platforms with wl12xx, we still need to pass the clock > definitions (along with the irq), so we have to keep > legacy_init_wl12xx for the time being (and that's also why we have to > currently keep the platform_data handling in the wlcore driver) > > do you have something else in mind? I think what Arnd is saying is we've now removed all the wl12xx using legacy platforms, so all of them can boot with just data from dts. Regards, Tony -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Mar 10, 2015 at 12:49 AM, Tony Lindgren <tony@atomide.com> wrote: > * Eliad Peller <eliad@wizery.com> [150309 14:03]: >> On Mon, Mar 9, 2015 at 9:50 PM, Arnd Bergmann <arnd@arndb.de> wrote: >> > On Monday 09 March 2015 17:36:42 Eliad Peller wrote: >> >> --- a/arch/arm/boot/dts/omap3-igep0030-rev-g.dts >> >> +++ b/arch/arm/boot/dts/omap3-igep0030-rev-g.dts >> >> @@ -64,4 +64,13 @@ >> >> vmmc-supply = <&lbep5clwmc_wlen>; >> >> bus-width = <4>; >> >> non-removable; >> >> + >> >> + #address-cells = <1>; >> >> + #size-cells = <0>; >> >> + wlcore: wlcore@2 { >> >> + compatible = "ti,wl1835"; >> >> + reg = <2>; >> >> + interrupt-parent = <&gpio5>; >> >> + interrupts = <8 IRQ_TYPE_NONE>; >> >> + }; >> >> >> > >> > Why IRQ_TYPE_NONE? >> > >> i simply mirrored the current board file (which only sets the irq number). >> >> > I was expecting you to remove all calls to legacy_init_wl12xx from this file, >> > including the ones for wl12xx aside from the wl18xx ones you removed, but >> > if that's enough to clean out the platform_data handling from the wlcore >> > driver, it's good enough as a start. >> not sure i'm following - can you elaborate? >> >> i'll summarize the way i see it. please correct me if i'm wrong. >> >> both wl18xx and wl12xx use the platform data to get the irq number. >> wl12xx (only) also needs some additional clock definitions to be >> passed. there's currently some issue with specifying some the of clock >> sources, so i preferred starting only with (the simpler) wl18xx >> bindings. >> >> for platforms with wl18xx, we can remove the pdata-quirk, as all the >> data (i.e. irq) can be passed by the new DT bindings. >> however, for platforms with wl12xx, we still need to pass the clock >> definitions (along with the irq), so we have to keep >> legacy_init_wl12xx for the time being (and that's also why we have to >> currently keep the platform_data handling in the wlcore driver) >> >> do you have something else in mind? > > I think what Arnd is saying is we've now removed all the wl12xx using > legacy platforms, so all of them can boot with just data from dts. > I don't think that's the case (unless i'm missing something). e.g. there's still pdata-quirk for "compulab,omap3-sbc-t3730" which initializes wl12xx device. Eliad. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday 10 March 2015 13:00:19 Eliad Peller wrote: > On Tue, Mar 10, 2015 at 12:49 AM, Tony Lindgren <tony@atomide.com> wrote: > > * Eliad Peller <eliad@wizery.com> [150309 14:03]: > >> On Mon, Mar 9, 2015 at 9:50 PM, Arnd Bergmann <arnd@arndb.de> wrote: > >> > On Monday 09 March 2015 17:36:42 Eliad Peller wrote: > >> >> --- a/arch/arm/boot/dts/omap3-igep0030-rev-g.dts > >> >> +++ b/arch/arm/boot/dts/omap3-igep0030-rev-g.dts > >> >> @@ -64,4 +64,13 @@ > >> >> vmmc-supply = <&lbep5clwmc_wlen>; > >> >> bus-width = <4>; > >> >> non-removable; > >> >> + > >> >> + #address-cells = <1>; > >> >> + #size-cells = <0>; > >> >> + wlcore: wlcore@2 { > >> >> + compatible = "ti,wl1835"; > >> >> + reg = <2>; > >> >> + interrupt-parent = <&gpio5>; > >> >> + interrupts = <8 IRQ_TYPE_NONE>; > >> >> + }; > >> >> > >> > > >> > Why IRQ_TYPE_NONE? > >> > > >> i simply mirrored the current board file (which only sets the irq number). > >> > >> > I was expecting you to remove all calls to legacy_init_wl12xx from this file, > >> > including the ones for wl12xx aside from the wl18xx ones you removed, but > >> > if that's enough to clean out the platform_data handling from the wlcore > >> > driver, it's good enough as a start. > >> not sure i'm following - can you elaborate? > >> > >> i'll summarize the way i see it. please correct me if i'm wrong. > >> > >> both wl18xx and wl12xx use the platform data to get the irq number. > >> wl12xx (only) also needs some additional clock definitions to be > >> passed. there's currently some issue with specifying some the of clock > >> sources, so i preferred starting only with (the simpler) wl18xx > >> bindings. > >> > >> for platforms with wl18xx, we can remove the pdata-quirk, as all the > >> data (i.e. irq) can be passed by the new DT bindings. > >> however, for platforms with wl12xx, we still need to pass the clock > >> definitions (along with the irq), so we have to keep > >> legacy_init_wl12xx for the time being (and that's also why we have to > >> currently keep the platform_data handling in the wlcore driver) > >> > >> do you have something else in mind? > > > > I think what Arnd is saying is we've now removed all the wl12xx using > > legacy platforms, so all of them can boot with just data from dts. Right, that was my idea. > I don't think that's the case (unless i'm missing something). > e.g. there's still pdata-quirk for "compulab,omap3-sbc-t3730" which > initializes wl12xx device. This one is just like the igep0030, as Tony was saying: the board boots from device tree already, so now that we have a binding for it, we can remove the wl12xx_set_platform_data() for it. Unfortunately, there is still one machine that uses a board file in combination with this wl12xx: da850-evm calls wl12xx_set_platform_data() from a legacy board file without DT. We have DT support for this file as well, but if I remember correctly, it is somewhat incomplete and we don't want to remove the file yet. Adding Sekhar and Kevin to Cc here for confirmation. Arnd -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* Arnd Bergmann <arnd@arndb.de> [150310 07:11]: > On Tuesday 10 March 2015 13:00:19 Eliad Peller wrote: > > On Tue, Mar 10, 2015 at 12:49 AM, Tony Lindgren <tony@atomide.com> wrote: > > > * Eliad Peller <eliad@wizery.com> [150309 14:03]: > > >> On Mon, Mar 9, 2015 at 9:50 PM, Arnd Bergmann <arnd@arndb.de> wrote: > > >> > On Monday 09 March 2015 17:36:42 Eliad Peller wrote: > > >> >> --- a/arch/arm/boot/dts/omap3-igep0030-rev-g.dts > > >> >> +++ b/arch/arm/boot/dts/omap3-igep0030-rev-g.dts > > >> >> @@ -64,4 +64,13 @@ > > >> >> vmmc-supply = <&lbep5clwmc_wlen>; > > >> >> bus-width = <4>; > > >> >> non-removable; > > >> >> + > > >> >> + #address-cells = <1>; > > >> >> + #size-cells = <0>; > > >> >> + wlcore: wlcore@2 { > > >> >> + compatible = "ti,wl1835"; > > >> >> + reg = <2>; > > >> >> + interrupt-parent = <&gpio5>; > > >> >> + interrupts = <8 IRQ_TYPE_NONE>; > > >> >> + }; > > >> >> > > >> > > > >> > Why IRQ_TYPE_NONE? > > >> > > > >> i simply mirrored the current board file (which only sets the irq number). > > >> > > >> > I was expecting you to remove all calls to legacy_init_wl12xx from this file, > > >> > including the ones for wl12xx aside from the wl18xx ones you removed, but > > >> > if that's enough to clean out the platform_data handling from the wlcore > > >> > driver, it's good enough as a start. > > >> not sure i'm following - can you elaborate? > > >> > > >> i'll summarize the way i see it. please correct me if i'm wrong. > > >> > > >> both wl18xx and wl12xx use the platform data to get the irq number. > > >> wl12xx (only) also needs some additional clock definitions to be > > >> passed. there's currently some issue with specifying some the of clock > > >> sources, so i preferred starting only with (the simpler) wl18xx > > >> bindings. > > >> > > >> for platforms with wl18xx, we can remove the pdata-quirk, as all the > > >> data (i.e. irq) can be passed by the new DT bindings. > > >> however, for platforms with wl12xx, we still need to pass the clock > > >> definitions (along with the irq), so we have to keep > > >> legacy_init_wl12xx for the time being (and that's also why we have to > > >> currently keep the platform_data handling in the wlcore driver) > > >> > > >> do you have something else in mind? > > > > > > I think what Arnd is saying is we've now removed all the wl12xx using > > > legacy platforms, so all of them can boot with just data from dts. > > Right, that was my idea. > > > I don't think that's the case (unless i'm missing something). > > e.g. there's still pdata-quirk for "compulab,omap3-sbc-t3730" which > > initializes wl12xx device. > > This one is just like the igep0030, as Tony was saying: the board > boots from device tree already, so now that we have a binding for > it, we can remove the wl12xx_set_platform_data() for it. > > Unfortunately, there is still one machine that uses a board file > in combination with this wl12xx: da850-evm calls > wl12xx_set_platform_data() from a legacy board file without DT. > > We have DT support for this file as well, but if I remember correctly, > it is somewhat incomplete and we don't want to remove the file > yet. Adding Sekhar and Kevin to Cc here for confirmation. Oops I forgot about the omap3-sbc-t3730, so yes we have to keep the platform data a little bit longer. But nothing stopping us moving all the other ones to use a proper device tree based configuration. Regards, Tony -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Mar 10, 2015 at 4:11 PM, Arnd Bergmann <arnd@arndb.de> wrote: > On Tuesday 10 March 2015 13:00:19 Eliad Peller wrote: >> On Tue, Mar 10, 2015 at 12:49 AM, Tony Lindgren <tony@atomide.com> wrote: >> >> > I was expecting you to remove all calls to legacy_init_wl12xx from this file, >> >> > including the ones for wl12xx aside from the wl18xx ones you removed, but >> >> > if that's enough to clean out the platform_data handling from the wlcore >> >> > driver, it's good enough as a start. >> >> not sure i'm following - can you elaborate? >> >> >> >> i'll summarize the way i see it. please correct me if i'm wrong. >> >> >> >> both wl18xx and wl12xx use the platform data to get the irq number. >> >> wl12xx (only) also needs some additional clock definitions to be >> >> passed. there's currently some issue with specifying some the of clock >> >> sources, so i preferred starting only with (the simpler) wl18xx >> >> bindings. >> >> >> >> for platforms with wl18xx, we can remove the pdata-quirk, as all the >> >> data (i.e. irq) can be passed by the new DT bindings. >> >> however, for platforms with wl12xx, we still need to pass the clock >> >> definitions (along with the irq), so we have to keep >> >> legacy_init_wl12xx for the time being (and that's also why we have to >> >> currently keep the platform_data handling in the wlcore driver) >> >> >> >> do you have something else in mind? >> > >> > I think what Arnd is saying is we've now removed all the wl12xx using >> > legacy platforms, so all of them can boot with just data from dts. > > Right, that was my idea. > >> I don't think that's the case (unless i'm missing something). >> e.g. there's still pdata-quirk for "compulab,omap3-sbc-t3730" which >> initializes wl12xx device. > > This one is just like the igep0030, as Tony was saying: the board > boots from device tree already, so now that we have a binding for > it, we can remove the wl12xx_set_platform_data() for it. > i think the wl12xx_set_platform_data() name created some confusion - it is used to pass platform data for both wl12xx and wl18xx devices. (this confusion is all around the wlcore driver as well, due to the code evolution) the binding i added is for wl18xx only (there is no wl12xx binding yet). the remaining boards, AFAICT, have wl12xx (rather than wl18xx) cards. so i don't see how we can remove these wl12xx_set_platform_data() calls before we have wl12xx bindings in-place as well. > Unfortunately, there is still one machine that uses a board file > in combination with this wl12xx: da850-evm calls > wl12xx_set_platform_data() from a legacy board file without DT. > ditto. Eliad. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday 10 March 2015 07:41 PM, Arnd Bergmann wrote: > On Tuesday 10 March 2015 13:00:19 Eliad Peller wrote: >> On Tue, Mar 10, 2015 at 12:49 AM, Tony Lindgren <tony@atomide.com> wrote: >>> * Eliad Peller <eliad@wizery.com> [150309 14:03]: >>>> On Mon, Mar 9, 2015 at 9:50 PM, Arnd Bergmann <arnd@arndb.de> wrote: >>>>> On Monday 09 March 2015 17:36:42 Eliad Peller wrote: >>>>>> --- a/arch/arm/boot/dts/omap3-igep0030-rev-g.dts >>>>>> +++ b/arch/arm/boot/dts/omap3-igep0030-rev-g.dts >>>>>> @@ -64,4 +64,13 @@ >>>>>> vmmc-supply = <&lbep5clwmc_wlen>; >>>>>> bus-width = <4>; >>>>>> non-removable; >>>>>> + >>>>>> + #address-cells = <1>; >>>>>> + #size-cells = <0>; >>>>>> + wlcore: wlcore@2 { >>>>>> + compatible = "ti,wl1835"; >>>>>> + reg = <2>; >>>>>> + interrupt-parent = <&gpio5>; >>>>>> + interrupts = <8 IRQ_TYPE_NONE>; >>>>>> + }; >>>>>> >>>>> >>>>> Why IRQ_TYPE_NONE? >>>>> >>>> i simply mirrored the current board file (which only sets the irq number). >>>> >>>>> I was expecting you to remove all calls to legacy_init_wl12xx from this file, >>>>> including the ones for wl12xx aside from the wl18xx ones you removed, but >>>>> if that's enough to clean out the platform_data handling from the wlcore >>>>> driver, it's good enough as a start. >>>> not sure i'm following - can you elaborate? >>>> >>>> i'll summarize the way i see it. please correct me if i'm wrong. >>>> >>>> both wl18xx and wl12xx use the platform data to get the irq number. >>>> wl12xx (only) also needs some additional clock definitions to be >>>> passed. there's currently some issue with specifying some the of clock >>>> sources, so i preferred starting only with (the simpler) wl18xx >>>> bindings. >>>> >>>> for platforms with wl18xx, we can remove the pdata-quirk, as all the >>>> data (i.e. irq) can be passed by the new DT bindings. >>>> however, for platforms with wl12xx, we still need to pass the clock >>>> definitions (along with the irq), so we have to keep >>>> legacy_init_wl12xx for the time being (and that's also why we have to >>>> currently keep the platform_data handling in the wlcore driver) >>>> >>>> do you have something else in mind? >>> >>> I think what Arnd is saying is we've now removed all the wl12xx using >>> legacy platforms, so all of them can boot with just data from dts. > > Right, that was my idea. > >> I don't think that's the case (unless i'm missing something). >> e.g. there's still pdata-quirk for "compulab,omap3-sbc-t3730" which >> initializes wl12xx device. > > This one is just like the igep0030, as Tony was saying: the board > boots from device tree already, so now that we have a binding for > it, we can remove the wl12xx_set_platform_data() for it. > > Unfortunately, there is still one machine that uses a board file > in combination with this wl12xx: da850-evm calls > wl12xx_set_platform_data() from a legacy board file without DT. > > We have DT support for this file as well, but if I remember correctly, > it is somewhat incomplete and we don't want to remove the file > yet. Adding Sekhar and Kevin to Cc here for confirmation. Thats true, we do not want to drop legacy booting for DA850 EVM yet. But if its coming in the way of code consolidation, I do not mind dropping WLAN support for that board. Anyone who really needs it can add support to the DA850 EVM DT file. Thanks, Sekhar -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday 10 March 2015 07:28:05 Tony Lindgren wrote: > > Oops I forgot about the omap3-sbc-t3730, so yes we have to keep the > platform data a little bit longer. But nothing stopping us moving > all the other ones to use a proper device tree based configuration. > For all I can tell, the t3730 board file does not support wl12xx at the moment, only the dts file does. If we do what Sekhar suggested and drop wl12xx support from the DA850 EVM board file, we can fix all wlcore users. Arnd -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday 10 March 2015 16:31:33 Eliad Peller wrote: > On Tue, Mar 10, 2015 at 4:11 PM, Arnd Bergmann <arnd@arndb.de> wrote: > > On Tuesday 10 March 2015 13:00:19 Eliad Peller wrote: > >> On Tue, Mar 10, 2015 at 12:49 AM, Tony Lindgren <tony@atomide.com> wrote: > >> >> > I was expecting you to remove all calls to legacy_init_wl12xx from this file, > >> >> > including the ones for wl12xx aside from the wl18xx ones you removed, but > >> >> > if that's enough to clean out the platform_data handling from the wlcore > >> >> > driver, it's good enough as a start. > >> >> not sure i'm following - can you elaborate? > >> >> > >> >> i'll summarize the way i see it. please correct me if i'm wrong. > >> >> > >> >> both wl18xx and wl12xx use the platform data to get the irq number. > >> >> wl12xx (only) also needs some additional clock definitions to be > >> >> passed. there's currently some issue with specifying some the of clock > >> >> sources, so i preferred starting only with (the simpler) wl18xx > >> >> bindings. > >> >> > >> >> for platforms with wl18xx, we can remove the pdata-quirk, as all the > >> >> data (i.e. irq) can be passed by the new DT bindings. > >> >> however, for platforms with wl12xx, we still need to pass the clock > >> >> definitions (along with the irq), so we have to keep > >> >> legacy_init_wl12xx for the time being (and that's also why we have to > >> >> currently keep the platform_data handling in the wlcore driver) > >> >> > >> >> do you have something else in mind? > >> > > >> > I think what Arnd is saying is we've now removed all the wl12xx using > >> > legacy platforms, so all of them can boot with just data from dts. > > > > Right, that was my idea. > > > >> I don't think that's the case (unless i'm missing something). > >> e.g. there's still pdata-quirk for "compulab,omap3-sbc-t3730" which > >> initializes wl12xx device. > > > > This one is just like the igep0030, as Tony was saying: the board > > boots from device tree already, so now that we have a binding for > > it, we can remove the wl12xx_set_platform_data() for it. > > > i think the wl12xx_set_platform_data() name created some confusion - > it is used to pass platform data for both wl12xx and wl18xx devices. > (this confusion is all around the wlcore driver as well, due to the > code evolution) > > the binding i added is for wl18xx only (there is no wl12xx binding yet). > the remaining boards, AFAICT, have wl12xx (rather than wl18xx) cards. > so i don't see how we can remove these wl12xx_set_platform_data() > calls before we have wl12xx bindings in-place as well. What is missing for that binding then? I keep getting confused here, but I thought that they share the implementation that looks at the platform data. Arnd -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Monday 09 March 2015 23:03:30 Eliad Peller wrote: > On Mon, Mar 9, 2015 at 9:50 PM, Arnd Bergmann <arnd@arndb.de> wrote: > > On Monday 09 March 2015 17:36:42 Eliad Peller wrote: > >> --- a/arch/arm/boot/dts/omap3-igep0030-rev-g.dts > >> +++ b/arch/arm/boot/dts/omap3-igep0030-rev-g.dts > >> @@ -64,4 +64,13 @@ > >> vmmc-supply = <&lbep5clwmc_wlen>; > >> bus-width = <4>; > >> non-removable; > >> + > >> + #address-cells = <1>; > >> + #size-cells = <0>; > >> + wlcore: wlcore@2 { > >> + compatible = "ti,wl1835"; > >> + reg = <2>; > >> + interrupt-parent = <&gpio5>; > >> + interrupts = <8 IRQ_TYPE_NONE>; > >> + }; > >> > > > > Why IRQ_TYPE_NONE? > > > i simply mirrored the current board file (which only sets the irq number). The irq type is set in this chunk of code from wlcore_nvs_cb: if (wl->platform_quirks & WL12XX_PLATFORM_QUIRK_EDGE_IRQ) { irqflags = IRQF_TRIGGER_RISING; hardirq_fn = wlcore_hardirq; } else { irqflags = IRQF_TRIGGER_HIGH | IRQF_ONESHOT; } This means you would replace the platform_quirks with setting the correct irq type. Arnd -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* Arnd Bergmann <arnd@arndb.de> [150310 08:48]: > On Tuesday 10 March 2015 07:28:05 Tony Lindgren wrote: > > > > Oops I forgot about the omap3-sbc-t3730, so yes we have to keep the > > platform data a little bit longer. But nothing stopping us moving > > all the other ones to use a proper device tree based configuration. > > > > For all I can tell, the t3730 board file does not support wl12xx > at the moment, only the dts file does. Hmm strange, it seems to configure the wlan_rst pin. Maybe there are variants with different WLAN module. But yeah, seems to be unused. That still leaves configuring all the dts users of the pdata-quirks.c legacy_init_wl12xx() with proper dts before removing it: $ git grep legacy_init_wl12xx pdata-quirks.c pdata-quirks.c:static void __init __used legacy_init_wl12xx(unsigned ref_clock, pdata-quirks.c:static inline void legacy_init_wl12xx(unsigned ref_clock, pdata-quirks.c: legacy_init_wl12xx(WL12XX_REFCLOCK_38, 0, 136); pdata-quirks.c: legacy_init_wl12xx(0, 0, 177); pdata-quirks.c: legacy_init_wl12xx(0, 0, 136); pdata-quirks.c: legacy_init_wl12xx(WL12XX_REFCLOCK_38, 0, 149); pdata-quirks.c: legacy_init_wl12xx(WL12XX_REFCLOCK_26, 0, 162); pdata-quirks.c: legacy_init_wl12xx(WL12XX_REFCLOCK_38, 0, 145); pdata-quirks.c: legacy_init_wl12xx(WL12XX_REFCLOCK_26, pdata-quirks.c: legacy_init_wl12xx(WL12XX_REFCLOCK_38, 0, 53); pdata-quirks.c: legacy_init_wl12xx(WL12XX_REFCLOCK_38, 0, 41); pdata-quirks.c: legacy_init_wl12xx(WL12XX_REFCLOCK_38, 0, 31); > If we do what Sekhar suggested and drop wl12xx support from > the DA850 EVM board file, we can fix all wlcore users. I guess Sekhar knows the da850 evm status the best. So up to you guys if wl12xx is not being used on legacy da850. Regards, Tony -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Mar 10, 2015 at 5:52 PM, Arnd Bergmann <arnd@arndb.de> wrote: > On Tuesday 10 March 2015 16:31:33 Eliad Peller wrote: >> On Tue, Mar 10, 2015 at 4:11 PM, Arnd Bergmann <arnd@arndb.de> wrote: >> > On Tuesday 10 March 2015 13:00:19 Eliad Peller wrote: >> >> On Tue, Mar 10, 2015 at 12:49 AM, Tony Lindgren <tony@atomide.com> wrote: >> >> >> > I was expecting you to remove all calls to legacy_init_wl12xx from this file, >> >> >> > including the ones for wl12xx aside from the wl18xx ones you removed, but >> >> >> > if that's enough to clean out the platform_data handling from the wlcore >> >> >> > driver, it's good enough as a start. >> >> >> not sure i'm following - can you elaborate? >> >> >> >> >> >> i'll summarize the way i see it. please correct me if i'm wrong. >> >> >> >> >> >> both wl18xx and wl12xx use the platform data to get the irq number. >> >> >> wl12xx (only) also needs some additional clock definitions to be >> >> >> passed. there's currently some issue with specifying some the of clock >> >> >> sources, so i preferred starting only with (the simpler) wl18xx >> >> >> bindings. >> >> >> >> >> >> for platforms with wl18xx, we can remove the pdata-quirk, as all the >> >> >> data (i.e. irq) can be passed by the new DT bindings. >> >> >> however, for platforms with wl12xx, we still need to pass the clock >> >> >> definitions (along with the irq), so we have to keep >> >> >> legacy_init_wl12xx for the time being (and that's also why we have to >> >> >> currently keep the platform_data handling in the wlcore driver) >> >> >> >> >> >> do you have something else in mind? >> >> > >> >> > I think what Arnd is saying is we've now removed all the wl12xx using >> >> > legacy platforms, so all of them can boot with just data from dts. >> > >> > Right, that was my idea. >> > >> >> I don't think that's the case (unless i'm missing something). >> >> e.g. there's still pdata-quirk for "compulab,omap3-sbc-t3730" which >> >> initializes wl12xx device. >> > >> > This one is just like the igep0030, as Tony was saying: the board >> > boots from device tree already, so now that we have a binding for >> > it, we can remove the wl12xx_set_platform_data() for it. >> > >> i think the wl12xx_set_platform_data() name created some confusion - >> it is used to pass platform data for both wl12xx and wl18xx devices. >> (this confusion is all around the wlcore driver as well, due to the >> code evolution) >> >> the binding i added is for wl18xx only (there is no wl12xx binding yet). >> the remaining boards, AFAICT, have wl12xx (rather than wl18xx) cards. >> so i don't see how we can remove these wl12xx_set_platform_data() >> calls before we have wl12xx bindings in-place as well. > > What is missing for that binding then? I keep getting confused here, > but I thought that they share the implementation that looks at the > platform data. > they both get the same wl12xx_platform_data struct, but only wl12xx cares about the clocks-related fields. the bindings i added parses only the irq. (Luca tried previously to upstream wl12xx DT support along with the required clock DT changes, but got some rejections, mainly wrt. clock stuff. e.g. http://thread.gmane.org/gmane.linux.kernel/1520752 that's why i preferred starting with "easier" wl18xx bindings only) Eliad. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* Eliad Peller <eliad@wizery.com> [150310 09:11]: > On Tue, Mar 10, 2015 at 5:52 PM, Arnd Bergmann <arnd@arndb.de> wrote: > > On Tuesday 10 March 2015 16:31:33 Eliad Peller wrote: > >> On Tue, Mar 10, 2015 at 4:11 PM, Arnd Bergmann <arnd@arndb.de> wrote: > >> > On Tuesday 10 March 2015 13:00:19 Eliad Peller wrote: > >> >> On Tue, Mar 10, 2015 at 12:49 AM, Tony Lindgren <tony@atomide.com> wrote: > >> >> >> > I was expecting you to remove all calls to legacy_init_wl12xx from this file, > >> >> >> > including the ones for wl12xx aside from the wl18xx ones you removed, but > >> >> >> > if that's enough to clean out the platform_data handling from the wlcore > >> >> >> > driver, it's good enough as a start. > >> >> >> not sure i'm following - can you elaborate? > >> >> >> > >> >> >> i'll summarize the way i see it. please correct me if i'm wrong. > >> >> >> > >> >> >> both wl18xx and wl12xx use the platform data to get the irq number. > >> >> >> wl12xx (only) also needs some additional clock definitions to be > >> >> >> passed. there's currently some issue with specifying some the of clock > >> >> >> sources, so i preferred starting only with (the simpler) wl18xx > >> >> >> bindings. > >> >> >> > >> >> >> for platforms with wl18xx, we can remove the pdata-quirk, as all the > >> >> >> data (i.e. irq) can be passed by the new DT bindings. > >> >> >> however, for platforms with wl12xx, we still need to pass the clock > >> >> >> definitions (along with the irq), so we have to keep > >> >> >> legacy_init_wl12xx for the time being (and that's also why we have to > >> >> >> currently keep the platform_data handling in the wlcore driver) > >> >> >> > >> >> >> do you have something else in mind? > >> >> > > >> >> > I think what Arnd is saying is we've now removed all the wl12xx using > >> >> > legacy platforms, so all of them can boot with just data from dts. > >> > > >> > Right, that was my idea. > >> > > >> >> I don't think that's the case (unless i'm missing something). > >> >> e.g. there's still pdata-quirk for "compulab,omap3-sbc-t3730" which > >> >> initializes wl12xx device. > >> > > >> > This one is just like the igep0030, as Tony was saying: the board > >> > boots from device tree already, so now that we have a binding for > >> > it, we can remove the wl12xx_set_platform_data() for it. > >> > > >> i think the wl12xx_set_platform_data() name created some confusion - > >> it is used to pass platform data for both wl12xx and wl18xx devices. > >> (this confusion is all around the wlcore driver as well, due to the > >> code evolution) > >> > >> the binding i added is for wl18xx only (there is no wl12xx binding yet). > >> the remaining boards, AFAICT, have wl12xx (rather than wl18xx) cards. > >> so i don't see how we can remove these wl12xx_set_platform_data() > >> calls before we have wl12xx bindings in-place as well. > > > > What is missing for that binding then? I keep getting confused here, > > but I thought that they share the implementation that looks at the > > platform data. > > > they both get the same wl12xx_platform_data struct, but only wl12xx > cares about the clocks-related fields. > the bindings i added parses only the irq. > > (Luca tried previously to upstream wl12xx DT support along with the > required clock DT changes, but got some rejections, mainly wrt. clock > stuff. > e.g. http://thread.gmane.org/gmane.linux.kernel/1520752 > that's why i preferred starting with "easier" wl18xx bindings only) I believe we did not have clock bindings back then, now it's simple to get the clock. If it's some internal clock to the wl12xx, then that's a different story, it should be just hidden behind a compatible flag then. Regards, Tony -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Mar 10, 2015 at 6:18 PM, Tony Lindgren <tony@atomide.com> wrote: > * Eliad Peller <eliad@wizery.com> [150310 09:11]: >> On Tue, Mar 10, 2015 at 5:52 PM, Arnd Bergmann <arnd@arndb.de> wrote: >> > On Tuesday 10 March 2015 16:31:33 Eliad Peller wrote: >> >> On Tue, Mar 10, 2015 at 4:11 PM, Arnd Bergmann <arnd@arndb.de> wrote: >> >> > On Tuesday 10 March 2015 13:00:19 Eliad Peller wrote: >> >> >> On Tue, Mar 10, 2015 at 12:49 AM, Tony Lindgren <tony@atomide.com> wrote: >> >> >> >> > I was expecting you to remove all calls to legacy_init_wl12xx from this file, >> >> >> >> > including the ones for wl12xx aside from the wl18xx ones you removed, but >> >> >> >> > if that's enough to clean out the platform_data handling from the wlcore >> >> >> >> > driver, it's good enough as a start. >> >> >> >> not sure i'm following - can you elaborate? >> >> >> >> >> >> >> >> i'll summarize the way i see it. please correct me if i'm wrong. >> >> >> >> >> >> >> >> both wl18xx and wl12xx use the platform data to get the irq number. >> >> >> >> wl12xx (only) also needs some additional clock definitions to be >> >> >> >> passed. there's currently some issue with specifying some the of clock >> >> >> >> sources, so i preferred starting only with (the simpler) wl18xx >> >> >> >> bindings. >> >> >> >> >> >> >> >> for platforms with wl18xx, we can remove the pdata-quirk, as all the >> >> >> >> data (i.e. irq) can be passed by the new DT bindings. >> >> >> >> however, for platforms with wl12xx, we still need to pass the clock >> >> >> >> definitions (along with the irq), so we have to keep >> >> >> >> legacy_init_wl12xx for the time being (and that's also why we have to >> >> >> >> currently keep the platform_data handling in the wlcore driver) >> >> >> >> >> >> >> >> do you have something else in mind? >> >> >> > >> >> >> > I think what Arnd is saying is we've now removed all the wl12xx using >> >> >> > legacy platforms, so all of them can boot with just data from dts. >> >> > >> >> > Right, that was my idea. >> >> > >> >> >> I don't think that's the case (unless i'm missing something). >> >> >> e.g. there's still pdata-quirk for "compulab,omap3-sbc-t3730" which >> >> >> initializes wl12xx device. >> >> > >> >> > This one is just like the igep0030, as Tony was saying: the board >> >> > boots from device tree already, so now that we have a binding for >> >> > it, we can remove the wl12xx_set_platform_data() for it. >> >> > >> >> i think the wl12xx_set_platform_data() name created some confusion - >> >> it is used to pass platform data for both wl12xx and wl18xx devices. >> >> (this confusion is all around the wlcore driver as well, due to the >> >> code evolution) >> >> >> >> the binding i added is for wl18xx only (there is no wl12xx binding yet). >> >> the remaining boards, AFAICT, have wl12xx (rather than wl18xx) cards. >> >> so i don't see how we can remove these wl12xx_set_platform_data() >> >> calls before we have wl12xx bindings in-place as well. >> > >> > What is missing for that binding then? I keep getting confused here, >> > but I thought that they share the implementation that looks at the >> > platform data. >> > >> they both get the same wl12xx_platform_data struct, but only wl12xx >> cares about the clocks-related fields. >> the bindings i added parses only the irq. >> >> (Luca tried previously to upstream wl12xx DT support along with the >> required clock DT changes, but got some rejections, mainly wrt. clock >> stuff. >> e.g. http://thread.gmane.org/gmane.linux.kernel/1520752 >> that's why i preferred starting with "easier" wl18xx bindings only) > > I believe we did not have clock bindings back then, now it's simple > to get the clock. If it's some internal clock to the wl12xx, then > that's a different story, it should be just hidden behind a compatible > flag then. > i'm really not familiar with the common clock framework, but there still doesn't seem to be a way to determine whether a clock is XTAL or not (which is what Luca's patch was about). should we use compatible flag in such case? i'm not sure it sounds right. anyway, i'd really prefer, if possible, starting with the wl18xx bindings, and defer the wl12xx complications to later on. (i also need to find some wl12xx card in order to actually test the patches once i'll have them...) we do have to make sure these wl18xx bindings are future-compatible with the wl12xx ones, but i think the current bindings are pretty much standard (and are actually a subset of the bindings needed by wl12xx), so it should be fine. Eliad. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* Eliad Peller <eliad@wizery.com> [150310 10:01]: > On Tue, Mar 10, 2015 at 6:18 PM, Tony Lindgren <tony@atomide.com> wrote: > > * Eliad Peller <eliad@wizery.com> [150310 09:11]: > >> On Tue, Mar 10, 2015 at 5:52 PM, Arnd Bergmann <arnd@arndb.de> wrote: > >> > On Tuesday 10 March 2015 16:31:33 Eliad Peller wrote: > >> >> On Tue, Mar 10, 2015 at 4:11 PM, Arnd Bergmann <arnd@arndb.de> wrote: > >> >> > On Tuesday 10 March 2015 13:00:19 Eliad Peller wrote: > >> >> >> On Tue, Mar 10, 2015 at 12:49 AM, Tony Lindgren <tony@atomide.com> wrote: > >> >> >> >> > I was expecting you to remove all calls to legacy_init_wl12xx from this file, > >> >> >> >> > including the ones for wl12xx aside from the wl18xx ones you removed, but > >> >> >> >> > if that's enough to clean out the platform_data handling from the wlcore > >> >> >> >> > driver, it's good enough as a start. > >> >> >> >> not sure i'm following - can you elaborate? > >> >> >> >> > >> >> >> >> i'll summarize the way i see it. please correct me if i'm wrong. > >> >> >> >> > >> >> >> >> both wl18xx and wl12xx use the platform data to get the irq number. > >> >> >> >> wl12xx (only) also needs some additional clock definitions to be > >> >> >> >> passed. there's currently some issue with specifying some the of clock > >> >> >> >> sources, so i preferred starting only with (the simpler) wl18xx > >> >> >> >> bindings. > >> >> >> >> > >> >> >> >> for platforms with wl18xx, we can remove the pdata-quirk, as all the > >> >> >> >> data (i.e. irq) can be passed by the new DT bindings. > >> >> >> >> however, for platforms with wl12xx, we still need to pass the clock > >> >> >> >> definitions (along with the irq), so we have to keep > >> >> >> >> legacy_init_wl12xx for the time being (and that's also why we have to > >> >> >> >> currently keep the platform_data handling in the wlcore driver) > >> >> >> >> > >> >> >> >> do you have something else in mind? > >> >> >> > > >> >> >> > I think what Arnd is saying is we've now removed all the wl12xx using > >> >> >> > legacy platforms, so all of them can boot with just data from dts. > >> >> > > >> >> > Right, that was my idea. > >> >> > > >> >> >> I don't think that's the case (unless i'm missing something). > >> >> >> e.g. there's still pdata-quirk for "compulab,omap3-sbc-t3730" which > >> >> >> initializes wl12xx device. > >> >> > > >> >> > This one is just like the igep0030, as Tony was saying: the board > >> >> > boots from device tree already, so now that we have a binding for > >> >> > it, we can remove the wl12xx_set_platform_data() for it. > >> >> > > >> >> i think the wl12xx_set_platform_data() name created some confusion - > >> >> it is used to pass platform data for both wl12xx and wl18xx devices. > >> >> (this confusion is all around the wlcore driver as well, due to the > >> >> code evolution) > >> >> > >> >> the binding i added is for wl18xx only (there is no wl12xx binding yet). > >> >> the remaining boards, AFAICT, have wl12xx (rather than wl18xx) cards. > >> >> so i don't see how we can remove these wl12xx_set_platform_data() > >> >> calls before we have wl12xx bindings in-place as well. > >> > > >> > What is missing for that binding then? I keep getting confused here, > >> > but I thought that they share the implementation that looks at the > >> > platform data. > >> > > >> they both get the same wl12xx_platform_data struct, but only wl12xx > >> cares about the clocks-related fields. > >> the bindings i added parses only the irq. > >> > >> (Luca tried previously to upstream wl12xx DT support along with the > >> required clock DT changes, but got some rejections, mainly wrt. clock > >> stuff. > >> e.g. http://thread.gmane.org/gmane.linux.kernel/1520752 > >> that's why i preferred starting with "easier" wl18xx bindings only) > > > > I believe we did not have clock bindings back then, now it's simple > > to get the clock. If it's some internal clock to the wl12xx, then > > that's a different story, it should be just hidden behind a compatible > > flag then. > > > i'm really not familiar with the common clock framework, but there > still doesn't seem to be a way to determine whether a clock is XTAL or > not (which is what Luca's patch was about). should we use compatible > flag in such case? i'm not sure it sounds right. > > anyway, i'd really prefer, if possible, starting with the wl18xx > bindings, and defer the wl12xx complications to later on. > (i also need to find some wl12xx card in order to actually test the > patches once i'll have them...) > > we do have to make sure these wl18xx bindings are future-compatible > with the wl12xx ones, but i think the current bindings are pretty much > standard (and are actually a subset of the bindings needed by wl12xx), > so it should be fine. Well how about add just the parsing of the clock and assume it's always WL12XX_REFCLOCK_38 for now as that's the only thing we're currently using. Then we can add a property or compatible value if using something else as needed. Regards, Tony -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday 10 March 2015 10:35:50 Tony Lindgren wrote: > * Eliad Peller <eliad@wizery.com> [150310 10:01]: > > i'm really not familiar with the common clock framework, but there > > still doesn't seem to be a way to determine whether a clock is XTAL or > > not (which is what Luca's patch was about). should we use compatible > > flag in such case? i'm not sure it sounds right. > > > > anyway, i'd really prefer, if possible, starting with the wl18xx > > bindings, and defer the wl12xx complications to later on. > > (i also need to find some wl12xx card in order to actually test the > > patches once i'll have them...) > > > > we do have to make sure these wl18xx bindings are future-compatible > > with the wl12xx ones, but i think the current bindings are pretty much > > standard (and are actually a subset of the bindings needed by wl12xx), > > so it should be fine. > > Well how about add just the parsing of the clock and assume it's always > WL12XX_REFCLOCK_38 for now as that's the only thing we're currently > using. Then we can add a property or compatible value if using something > else as needed. > We have two exceptions: static void __init omap3_zoom_legacy_init(void) { legacy_init_wl12xx(WL12XX_REFCLOCK_26, 0, 162); } static void __init omap4_sdp_legacy_init(void) { legacy_init_wl12xx(WL12XX_REFCLOCK_26, WL12XX_TCXOCLOCK_26, 53); } Where do those clocks come from? If these are always fixed-rate clocks coming from an external clock provider, using a separate compatible string in the clock provider would seem reasonable to me, but we can do that once we have to: none of the machines we support use an XTAL clock input. Arnd -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello Eliad, On Mon, Mar 9, 2015 at 4:36 PM, Eliad Peller <eliad@wizery.com> wrote: > Add wl18xx (wilink8) bindings to omap3-igep0030-rev-g and > omap3-igep0020-rev-f dts files, instead of defining the > platform data through the pdata-quirks. > > The patch was compile-tested only. > You should look at MAINTAINERS or use ./scripts/get_maintainer.pl to know who should be cc'ed. Specially for this kind of patches where you are not able to test on the actual platform. Added Enric to cc as well since he also maintains the IGEP boards an has access to the hw too. > Signed-off-by: Eliad Peller <eliad@wizery.com> > --- > arch/arm/boot/dts/omap3-igep0020-rev-f.dts | 9 +++++++++ > arch/arm/boot/dts/omap3-igep0030-rev-g.dts | 9 +++++++++ > arch/arm/mach-omap2/pdata-quirks.c | 2 -- > 3 files changed, 18 insertions(+), 2 deletions(-) > > diff --git a/arch/arm/boot/dts/omap3-igep0020-rev-f.dts b/arch/arm/boot/dts/omap3-igep0020-rev-f.dts > index cc8bd0c..8e5b44e 100644 > --- a/arch/arm/boot/dts/omap3-igep0020-rev-f.dts > +++ b/arch/arm/boot/dts/omap3-igep0020-rev-f.dts > @@ -42,4 +42,13 @@ > vmmc-supply = <&lbep5clwmc_wlen>; > bus-width = <4>; > non-removable; > + > + #address-cells = <1>; > + #size-cells = <0>; > + wlcore: wlcore@2 { > + compatible = "ti,wl1835"; > + reg = <2>; > + interrupt-parent = <&gpio6>; > + interrupts = <17 IRQ_TYPE_NONE>; As Arnd said, it seems this should be IRQ_TYPE_LEVEL_HIGH to match what the platform code is currently doing. Best regards, Javier -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello Eliad, On Mon, Mar 9, 2015 at 4:36 PM, Eliad Peller <eliad@wizery.com> wrote: > When running with device-tree, we no longer have a board file > that can set up the platform data for wlcore. > Allow this data to be passed from DT. > > For now, parse only the irq used. Other (optional) properties > can be added later on. > > Signed-off-by: Ido Yariv <ido@wizery.com> > Signed-off-by: Eliad Peller <eliad@wizery.com> > --- I see this is a v5 but I don't know what was changed from prior revisions. It would be good if the patches had a versions history. > drivers/net/wireless/ti/wlcore/sdio.c | 80 ++++++++++++++++++++++++++++++++--- > 1 file changed, 74 insertions(+), 6 deletions(-) > > diff --git a/drivers/net/wireless/ti/wlcore/sdio.c b/drivers/net/wireless/ti/wlcore/sdio.c > index d3dd7bf..ee556ac 100644 > --- a/drivers/net/wireless/ti/wlcore/sdio.c > +++ b/drivers/net/wireless/ti/wlcore/sdio.c > @@ -34,6 +34,8 @@ > #include <linux/wl12xx.h> > #include <linux/pm_runtime.h> > #include <linux/printk.h> > +#include <linux/of.h> > +#include <linux/of_irq.h> > > #include "wlcore.h" > #include "wl12xx_80211.h" > @@ -214,6 +216,69 @@ static struct wl1271_if_operations sdio_ops = { > .set_block_size = wl1271_sdio_set_block_size, > }; > > +#ifdef CONFIG_OF > +static const struct of_device_id wlcore_sdio_of_match_table[] = { > + { .compatible = "ti,wl1801" }, > + { .compatible = "ti,wl1805" }, > + { .compatible = "ti,wl1807" }, > + { .compatible = "ti,wl1831" }, > + { .compatible = "ti,wl1835" }, > + { .compatible = "ti,wl1837" }, > + { } > +}; > + > +static struct wl12xx_platform_data *wlcore_probe_of(struct device *dev) > +{ > + struct device_node *np = dev->of_node; > + struct wl12xx_platform_data *pdata; > + > + if (!np || !of_match_node(wlcore_sdio_of_match_table, np)) > + return NULL; > + > + pdata = kzalloc(sizeof(*pdata), GFP_KERNEL); > + if (!pdata) > + return NULL; > + > + pdata->irq = irq_of_parse_and_map(np, 0); > + if (!pdata->irq) { > + dev_err(dev, "No irq in platform data\n"); > + kfree(pdata); > + return NULL; > + } > + > + return pdata; > +} > +#else > +static struct wl12xx_platform_data *wlcore_probe_of(struct device *dev) > +{ > + return NULL; > +} > +#endif > + > +static struct wl12xx_platform_data * > +wlcore_get_platform_data(struct device *dev) > +{ > + struct wl12xx_platform_data *pdata; > + > + /* first, look for DT data */ I thought it was the opposite, that platform data should over-rule DT. That way you can still use the data filled in arch/arm/mach-omap2/pdata-quirks.c even after the driver supports your new DT binding. > + pdata = wlcore_probe_of(dev); > + if (pdata) > + return pdata; > + > + /* if not found - fallback to static platform data */ > + pdata = wl12xx_get_platform_data(); > + if (!IS_ERR(pdata)) > + return kmemdup(pdata, sizeof(*pdata), GFP_KERNEL); > + > + dev_err(dev, "No platform data set\n"); > + return NULL; > +} > + > +static void wlcore_del_platform_data(struct wl12xx_platform_data *pdata) > +{ > + kfree(pdata); > +} > + This function seems to be an unnecessary, why not just call kfree() directly? Or better, maybe the resource-managed devm_*() functions can be used so the data doesn't have to be explicitly freed? Best regards, Javier -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello Tony, On Tue, Mar 10, 2015 at 6:35 PM, Tony Lindgren <tony@atomide.com> wrote: >> >> we do have to make sure these wl18xx bindings are future-compatible >> with the wl12xx ones, but i think the current bindings are pretty much >> standard (and are actually a subset of the bindings needed by wl12xx), >> so it should be fine. > > Well how about add just the parsing of the clock and assume it's always > WL12XX_REFCLOCK_38 for now as that's the only thing we're currently > using. Then we can add a property or compatible value if using something > else as needed. > What do you mean by parsing here? IIUC there isn't a clock driver for these clocks and are setup directly in the drivers/net/wireless/ti/wl12xx/main.c driver. So you can't make the WLAN chip dev node a consumer of these clocks by adding a phandle to a clock provider and clock specifiers since there isn't a provider to be referenced in the DT. Or did I misunderstand? > Regards, > > Tony Best regards, Javier -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello Eliad, On Wed, Mar 11, 2015 at 1:28 AM, Javier Martinez Canillas <javier@dowhile0.org> wrote: > On Mon, Mar 9, 2015 at 4:36 PM, Eliad Peller <eliad@wizery.com> wrote: >> --- a/arch/arm/boot/dts/omap3-igep0020-rev-f.dts >> +++ b/arch/arm/boot/dts/omap3-igep0020-rev-f.dts >> @@ -42,4 +42,13 @@ >> vmmc-supply = <&lbep5clwmc_wlen>; Something I forgot to mention is that this vmmc-supply is a DT hack since the WLAN SDIO chip needs a WL_EN signal to be asserted as a part of the chip power sequencing. But there wasn't a DT binding to describe that so most boards use the same hack which is to define a fake fixed-regulator with an enable GPIO just to toggle that pin. But now the MMC subsystem has support for power sequence providers and the pwrseq-simple driver (Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.txt) has support for reset GPIOs so you may want to change that as well. In fact, the pwrseq-simple also has support for an external clock (and can be extended to support more than one) so if the clocks are not internal to the wl12xx, maybe these should be defined in the pwrseq-simple dev node assuming that there is a clock driver for them. Best regards, Javier -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday 11 March 2015 01:34:19 Javier Martinez Canillas wrote: > > + > > +static struct wl12xx_platform_data * > > +wlcore_get_platform_data(struct device *dev) > > +{ > > + struct wl12xx_platform_data *pdata; > > + > > + /* first, look for DT data */ > > I thought it was the opposite, that platform data should over-rule DT. > That way you can still use the data filled in > arch/arm/mach-omap2/pdata-quirks.c even after the driver supports your > new DT binding. No, the pdata-quirks stuff for this driver must die, it was a hack that only exists because we previously could not attach data to an sdio function. > > + pdata = wlcore_probe_of(dev); > > + if (pdata) > > + return pdata; > > + > > + /* if not found - fallback to static platform data */ > > + pdata = wl12xx_get_platform_data(); > > + if (!IS_ERR(pdata)) > > + return kmemdup(pdata, sizeof(*pdata), GFP_KERNEL); > > + > > + dev_err(dev, "No platform data set\n"); > > + return NULL; > > +} > > + > > +static void wlcore_del_platform_data(struct wl12xx_platform_data *pdata) > > +{ > > + kfree(pdata); > > +} > > + > > This function seems to be an unnecessary, why not just call kfree() directly? > > Or better, maybe the resource-managed devm_*() functions can be used > so the data doesn't have to be explicitly freed? As I said earlier, I think it would be best not to dynamically allocate anything here at all. As Eliad explained, the data is used by two different drivers: wl12xx and wl18xx, and only the latter is converted for now, but after the conversion, it should not need the platform data structure any more, only the irq number that gets passed in from DT. Arnd -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday 11 March 2015 02:00:59 Javier Martinez Canillas wrote: > Hello Tony, > > On Tue, Mar 10, 2015 at 6:35 PM, Tony Lindgren <tony@atomide.com> wrote: > >> > >> we do have to make sure these wl18xx bindings are future-compatible > >> with the wl12xx ones, but i think the current bindings are pretty much > >> standard (and are actually a subset of the bindings needed by wl12xx), > >> so it should be fine. > > > > Well how about add just the parsing of the clock and assume it's always > > WL12XX_REFCLOCK_38 for now as that's the only thing we're currently > > using. Then we can add a property or compatible value if using something > > else as needed. > > > > What do you mean by parsing here? IIUC there isn't a clock driver for > these clocks and are setup directly in the > drivers/net/wireless/ti/wl12xx/main.c driver. > > So you can't make the WLAN chip dev node a consumer of these clocks by > adding a phandle to a clock provider and clock specifiers since there > isn't a provider to be referenced in the DT. Or did I misunderstand? As I understand it, the clock signal is provided by an external oscillator, which we can easily model in DT, and then you call clk_get_rate on that. If there is no external clock provider for this chip and the clocks are provided by the device itself, then all we need is a clock-frequency property in the device node. Arnd -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello Arnd, On Wed, Mar 11, 2015 at 10:51 AM, Arnd Bergmann <arnd@arndb.de> wrote: > On Wednesday 11 March 2015 01:34:19 Javier Martinez Canillas wrote: >> > + >> > +static struct wl12xx_platform_data * >> > +wlcore_get_platform_data(struct device *dev) >> > +{ >> > + struct wl12xx_platform_data *pdata; >> > + >> > + /* first, look for DT data */ >> >> I thought it was the opposite, that platform data should over-rule DT. >> That way you can still use the data filled in >> arch/arm/mach-omap2/pdata-quirks.c even after the driver supports your >> new DT binding. > > No, the pdata-quirks stuff for this driver must die, it was a hack > that only exists because we previously could not attach data to an > sdio function. > Ok sorry, I misunderstood and thought that the output from the discussion in patch 3/3 was that the pdata could not still be removed due not having a way to configure the clocks for wl12xx. I totally agree with removing the pdata on this driver, in fact I would go an just remove all the remaining OMAP2+ board files and pdata-quirks completely. Most OMAP2+ boards have been converted to DT years ago and if someone really cares about mainline support for these boards, they can add a DTS and DT bindings for the drivers that still needed like this one. Best regards, Javier -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday 11 March 2015 11:05:47 Javier Martinez Canillas wrote: > > On Wed, Mar 11, 2015 at 10:51 AM, Arnd Bergmann <arnd@arndb.de> wrote: > > On Wednesday 11 March 2015 01:34:19 Javier Martinez Canillas wrote: > >> > + > >> > +static struct wl12xx_platform_data * > >> > +wlcore_get_platform_data(struct device *dev) > >> > +{ > >> > + struct wl12xx_platform_data *pdata; > >> > + > >> > + /* first, look for DT data */ > >> > >> I thought it was the opposite, that platform data should over-rule DT. > >> That way you can still use the data filled in > >> arch/arm/mach-omap2/pdata-quirks.c even after the driver supports your > >> new DT binding. > > > > No, the pdata-quirks stuff for this driver must die, it was a hack > > that only exists because we previously could not attach data to an > > sdio function. > > > > Ok sorry, I misunderstood and thought that the output from the > discussion in patch 3/3 was that the pdata could not still be removed > due not having a way to configure the clocks for wl12xx. I think that is still the case, but we should never have both pdata and DT, and if we ever did, I think the DT should take precedence so we can be sure that the pdata is not used and can be removed. Arnd -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello Arnd, On Wed, Mar 11, 2015 at 10:53 AM, Arnd Bergmann <arnd@arndb.de> wrote: > On Wednesday 11 March 2015 02:00:59 Javier Martinez Canillas wrote: >> >> What do you mean by parsing here? IIUC there isn't a clock driver for >> these clocks and are setup directly in the >> drivers/net/wireless/ti/wl12xx/main.c driver. >> >> So you can't make the WLAN chip dev node a consumer of these clocks by >> adding a phandle to a clock provider and clock specifiers since there >> isn't a provider to be referenced in the DT. Or did I misunderstand? > > As I understand it, the clock signal is provided by an external oscillator, According to [0], it seems the chip can be connected to both external oscillators or internal clocks provided by the chip itself. > which we can easily model in DT, and then you call clk_get_rate on that. > Right, my point wast that this can be done only if the external oscillator have a proper clock driver / provider which I don't think is the case here. Most of this stuff predates the common clock framework. Or at least Luciano Coelho had a patch on his series to make the wilink driver a clock provider itself by registering the refclock and tcxoclock clocks [0]. Luciano also had patches for: * Adding the clock provider dev node in the DTS [1] * Have a table to map the clock rate with the FW configuration values [2] * Getting the clock from DT and the rate as you said to configure the firmware accordingly [3] I think that patch [0] should not be needed since for external clocks, the IP providing the clocks should have its own clock driver and for internal clocks, a property should be used instead as you said. > If there is no external clock provider for this chip and the clocks > are provided by the device itself, then all we need is a clock-frequency > property in the device node. > Agreed, IIUC Luciano wanted to expose the internal clocks by registering in the common clock framework but if those clocks are not really accessible from outside the wlan chip, then I also think that a device node property should be used instead. > Arnd > -- Best regards, Javier [0]: https://lists.ozlabs.org/pipermail/devicetree-discuss/2013-July/037139.html [1]: http://lists.infradead.org/pipermail/linux-arm-kernel/2013-July/187794.html [2]: http://lists.infradead.org/pipermail/linux-arm-kernel/2013-July/181594.html [3]: http://lists.infradead.org/pipermail/linux-arm-kernel/2013-July/181591.html -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Mar 11, 2015 at 11:51 AM, Arnd Bergmann <arnd@arndb.de> wrote: >> > +static void wlcore_del_platform_data(struct wl12xx_platform_data *pdata) >> > +{ >> > + kfree(pdata); >> > +} >> > + >> >> This function seems to be an unnecessary, why not just call kfree() directly? >> >> Or better, maybe the resource-managed devm_*() functions can be used >> so the data doesn't have to be explicitly freed? > > As I said earlier, I think it would be best not to dynamically allocate anything > here at all. As Eliad explained, the data is used by two different drivers: > wl12xx and wl18xx, and only the latter is converted for now, but after the > conversion, it should not need the platform data structure any more, only > the irq number that gets passed in from DT. > sure, i'll try taking care of it (probably with additional patch after the conversion) Eliad. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
hi Javier, On Wed, Mar 11, 2015 at 2:28 AM, Javier Martinez Canillas <javier@dowhile0.org> wrote: > Hello Eliad, > > On Mon, Mar 9, 2015 at 4:36 PM, Eliad Peller <eliad@wizery.com> wrote: >> Add wl18xx (wilink8) bindings to omap3-igep0030-rev-g and >> omap3-igep0020-rev-f dts files, instead of defining the >> platform data through the pdata-quirks. >> >> The patch was compile-tested only. >> > > You should look at MAINTAINERS or use ./scripts/get_maintainer.pl to > know who should be cc'ed. Specially for this kind of patches where you > are not able to test on the actual platform. Added Enric to cc as well > since he also maintains the IGEP boards an has access to the hw too. > right. sorry about that. i assumed adding the relevant mailing lists should be enough, but i guess i should have added the relevant maintainers as well. >> arch/arm/boot/dts/omap3-igep0020-rev-f.dts | 9 +++++++++ >> arch/arm/boot/dts/omap3-igep0030-rev-g.dts | 9 +++++++++ >> arch/arm/mach-omap2/pdata-quirks.c | 2 -- >> 3 files changed, 18 insertions(+), 2 deletions(-) >> >> diff --git a/arch/arm/boot/dts/omap3-igep0020-rev-f.dts b/arch/arm/boot/dts/omap3-igep0020-rev-f.dts >> index cc8bd0c..8e5b44e 100644 >> --- a/arch/arm/boot/dts/omap3-igep0020-rev-f.dts >> +++ b/arch/arm/boot/dts/omap3-igep0020-rev-f.dts >> @@ -42,4 +42,13 @@ >> vmmc-supply = <&lbep5clwmc_wlen>; >> bus-width = <4>; >> non-removable; >> + >> + #address-cells = <1>; >> + #size-cells = <0>; >> + wlcore: wlcore@2 { >> + compatible = "ti,wl1835"; >> + reg = <2>; >> + interrupt-parent = <&gpio6>; >> + interrupts = <17 IRQ_TYPE_NONE>; > > As Arnd said, it seems this should be IRQ_TYPE_LEVEL_HIGH to match > what the platform code is currently doing. > will be fixed. thanks, Eliad. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
hi Javier, On Wed, Mar 11, 2015 at 3:19 AM, Javier Martinez Canillas <javier@dowhile0.org> wrote: > Hello Eliad, > > On Wed, Mar 11, 2015 at 1:28 AM, Javier Martinez Canillas > <javier@dowhile0.org> wrote: >> On Mon, Mar 9, 2015 at 4:36 PM, Eliad Peller <eliad@wizery.com> wrote: >>> --- a/arch/arm/boot/dts/omap3-igep0020-rev-f.dts >>> +++ b/arch/arm/boot/dts/omap3-igep0020-rev-f.dts >>> @@ -42,4 +42,13 @@ >>> vmmc-supply = <&lbep5clwmc_wlen>; > > Something I forgot to mention is that this vmmc-supply is a DT hack > since the WLAN SDIO chip needs a WL_EN signal to be asserted as a part > of the chip power sequencing. > > But there wasn't a DT binding to describe that so most boards use the > same hack which is to define a fake fixed-regulator with an enable > GPIO just to toggle that pin. > > But now the MMC subsystem has support for power sequence providers and > the pwrseq-simple driver > (Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.txt) has > support for reset GPIOs so you may want to change that as well. > interesting. i wasn't aware of it. but that's for a different time i guess :) thanks, Eliad. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello Eliad, On Wed, Mar 11, 2015 at 12:59 PM, Eliad Peller <eliad@wizery.com> wrote: > On Wed, Mar 11, 2015 at 3:19 AM, Javier Martinez Canillas > <javier@dowhile0.org> wrote: >> >> But there wasn't a DT binding to describe that so most boards use the >> same hack which is to define a fake fixed-regulator with an enable >> GPIO just to toggle that pin. >> >> But now the MMC subsystem has support for power sequence providers and >> the pwrseq-simple driver >> (Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.txt) has >> support for reset GPIOs so you may want to change that as well. >> > interesting. i wasn't aware of it. > but that's for a different time i guess :) > Indeed, I've on my TODO anyways so I can post a patch on top of your series :-) > thanks, > Eliad. Best regards, Javier -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Mar 11, 2015 at 1:34 PM, Javier Martinez Canillas <javier@dowhile0.org> wrote: > I think that patch [0] should not be needed since for external clocks, > the IP providing the clocks should have its own clock driver and for > internal clocks, a property should be used instead as you said. > >> If there is no external clock provider for this chip and the clocks >> are provided by the device itself, then all we need is a clock-frequency >> property in the device node. >> > > Agreed, IIUC Luciano wanted to expose the internal clocks by > registering in the common clock framework but if those clocks are not > really accessible from outside the wlan chip, then I also think that a > device node property should be used instead. > how should i describe multiple clock-frequency properties (there are 2 relevant clocks) in this case? does something like the following makes sense? wlcore: wlcore@2 { ... refclock: refclock { compatible = "fixed-clock"; #clock-cells = <0>; clock-frequency = <38400000>; }; } Eliad. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday 11 March 2015 12:34:03 Javier Martinez Canillas wrote: > Hello Arnd, > > On Wed, Mar 11, 2015 at 10:53 AM, Arnd Bergmann <arnd@arndb.de> wrote: > > On Wednesday 11 March 2015 02:00:59 Javier Martinez Canillas wrote: > >> > >> What do you mean by parsing here? IIUC there isn't a clock driver for > >> these clocks and are setup directly in the > >> drivers/net/wireless/ti/wl12xx/main.c driver. > >> > >> So you can't make the WLAN chip dev node a consumer of these clocks by > >> adding a phandle to a clock provider and clock specifiers since there > >> isn't a provider to be referenced in the DT. Or did I misunderstand? > > > > As I understand it, the clock signal is provided by an external oscillator, > > According to [0], it seems the chip can be connected to both external > oscillators or internal clocks provided by the chip itself. I see, that part wasn't clear to me. > > which we can easily model in DT, and then you call clk_get_rate on that. > > > > Right, my point wast that this can be done only if the external > oscillator have a proper clock driver / provider which I don't think > is the case here. Most of this stuff predates the common clock > framework. > > Or at least Luciano Coelho had a patch on his series to make the > wilink driver a clock provider itself by registering the refclock and > tcxoclock clocks [0]. I guess we should only do this if the clocks from the wilink device might be used by some other device. > Luciano also had patches for: > > * Adding the clock provider dev node in the DTS [1] > * Have a table to map the clock rate with the FW configuration values [2] > * Getting the clock from DT and the rate as you said to configure the > firmware accordingly [3] > > I think that patch [0] should not be needed since for external clocks, > the IP providing the clocks should have its own clock driver and for > internal clocks, a property should be used instead as you said. Right. > > If there is no external clock provider for this chip and the clocks > > are provided by the device itself, then all we need is a clock-frequency > > property in the device node. > > > > Agreed, IIUC Luciano wanted to expose the internal clocks by > registering in the common clock framework but if those clocks are not > really accessible from outside the wlan chip, then I also think that a > device node property should be used instead. If I understand this right, that measn we can easily distinguish between an external XTAL clock and the internal clock by they way they are described in the DT: for the internal clock, we just provide a clock-frequency property, while the external clock would be referenced through a clocks property. Arnd -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello Arnd, On Wed, Mar 11, 2015 at 1:40 PM, Arnd Bergmann <arnd@arndb.de> wrote: > On Wednesday 11 March 2015 12:34:03 Javier Martinez Canillas wrote: >> Hello Arnd, >> >> On Wed, Mar 11, 2015 at 10:53 AM, Arnd Bergmann <arnd@arndb.de> wrote: >> > On Wednesday 11 March 2015 02:00:59 Javier Martinez Canillas wrote: >> >> >> >> What do you mean by parsing here? IIUC there isn't a clock driver for >> >> these clocks and are setup directly in the >> >> drivers/net/wireless/ti/wl12xx/main.c driver. >> >> >> >> So you can't make the WLAN chip dev node a consumer of these clocks by >> >> adding a phandle to a clock provider and clock specifiers since there >> >> isn't a provider to be referenced in the DT. Or did I misunderstand? >> > >> > As I understand it, the clock signal is provided by an external oscillator, >> >> According to [0], it seems the chip can be connected to both external >> oscillators or internal clocks provided by the chip itself. > > I see, that part wasn't clear to me. > Yeah, it was not clear to me either before reading Luciano's commit message. [snip] >> > If there is no external clock provider for this chip and the clocks >> > are provided by the device itself, then all we need is a clock-frequency >> > property in the device node. >> > >> >> Agreed, IIUC Luciano wanted to expose the internal clocks by >> registering in the common clock framework but if those clocks are not >> really accessible from outside the wlan chip, then I also think that a >> device node property should be used instead. > > If I understand this right, that measn we can easily distinguish between > an external XTAL clock and the internal clock by they way they are > described in the DT: for the internal clock, we just provide a clock-frequency > property, while the external clock would be referenced through a clocks > property. > That's my understanding as well. Right now it seems that all boards in mainline with a WiLink6 part are using internal clocks. So as a first step I think that adding an optional refclock-frequency and tcxoclock-frequency properties should be enough. It would be good if the driver supports getting the refclock and tcxoclock from an external provider in case a board gets these from external clocks but that can be done as a followup if there are boards in the future using that design. But please bear in mind that I'm not familiar with the clock handling in WiLink6 since the WiLink8 part used in the IGEP boards does not need these clocks and I only looked at Luciano's previous patches and the WiLink today driver today. So it would be good if Eliad can double check my assumptions to see if those are correct. Best regards, Javier -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday 11 March 2015 14:12:54 Eliad Peller wrote: > On Wed, Mar 11, 2015 at 1:34 PM, Javier Martinez Canillas > <javier@dowhile0.org> wrote: > > I think that patch [0] should not be needed since for external clocks, > > the IP providing the clocks should have its own clock driver and for > > internal clocks, a property should be used instead as you said. > > > >> If there is no external clock provider for this chip and the clocks > >> are provided by the device itself, then all we need is a clock-frequency > >> property in the device node. > >> > > > > Agreed, IIUC Luciano wanted to expose the internal clocks by > > registering in the common clock framework but if those clocks are not > > really accessible from outside the wlan chip, then I also think that a > > device node property should be used instead. > > > how should i describe multiple clock-frequency properties (there are 2 > relevant clocks) in this case? > > does something like the following makes sense? > > wlcore: wlcore@2 { > ... > refclock: refclock { > compatible = "fixed-clock"; > #clock-cells = <0>; > clock-frequency = <38400000>; > }; > } I would put that clock node on the top level of the DT, as you are describing an external clock input here, but other than that, it looks good. I would do the binding in a way that mandates either a "clocks" reference to an external clock provider in case of a XTAL or a "clock-frequency" property. In the first case, you can use the "clock-names" property to identify the "ref" and "txco" clock inputs, in the second case we could either decide to have a single property with two cells, or have named properties like wlcore@2 { interrupts = < ... >; tcxo-clock-frequency = <38400000>; ref-clock-frequency = <19200000>; } I don't know which combinations are possible here. I did notice that most of the references in the board hacks use '0' as the tcxo clock value, which happens to be the same as WL12XX_TCXOCLOCK_19_2, but could also meant that no tcxo clock is used. Arnd -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday 11 March 2015 14:07:11 Javier Martinez Canillas wrote: > > Right now it seems that all boards in mainline with a WiLink6 part are > using internal clocks. So as a first step I think that adding an > optional refclock-frequency and tcxoclock-frequency properties should > be enough. > > It would be good if the driver supports getting the refclock and > tcxoclock from an external provider in case a board gets these from > external clocks but that can be done as a followup if there are boards > in the future using that design. > > But please bear in mind that I'm not familiar with the clock handling > in WiLink6 since the WiLink8 part used in the IGEP boards does not > need these clocks and I only looked at Luciano's previous patches and > the WiLink today driver today. So it would be good if Eliad can double > check my assumptions to see if those are correct. Sounds good. I'd also be fine with not implementing the case for external clocks in the code until we need (and can test) it, but I think it should be specified in the binding from the start. Arnd -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Mar 11, 2015 at 2:17 PM, Arnd Bergmann <arnd@arndb.de> wrote: > On Wednesday 11 March 2015 14:07:11 Javier Martinez Canillas wrote: >> >> Right now it seems that all boards in mainline with a WiLink6 part are >> using internal clocks. So as a first step I think that adding an >> optional refclock-frequency and tcxoclock-frequency properties should >> be enough. >> >> It would be good if the driver supports getting the refclock and >> tcxoclock from an external provider in case a board gets these from >> external clocks but that can be done as a followup if there are boards >> in the future using that design. >> >> But please bear in mind that I'm not familiar with the clock handling >> in WiLink6 since the WiLink8 part used in the IGEP boards does not >> need these clocks and I only looked at Luciano's previous patches and >> the WiLink today driver today. So it would be good if Eliad can double >> check my assumptions to see if those are correct. > > Sounds good. I'd also be fine with not implementing the case for > external clocks in the code until we need (and can test) it, but > I think it should be specified in the binding from the start. > Agreed. Best regards, Javier -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Mar 11, 2015 at 3:21 PM, Javier Martinez Canillas <javier@dowhile0.org> wrote: > On Wed, Mar 11, 2015 at 2:17 PM, Arnd Bergmann <arnd@arndb.de> wrote: >> On Wednesday 11 March 2015 14:07:11 Javier Martinez Canillas wrote: >>> >>> Right now it seems that all boards in mainline with a WiLink6 part are >>> using internal clocks. So as a first step I think that adding an >>> optional refclock-frequency and tcxoclock-frequency properties should >>> be enough. >>> >>> It would be good if the driver supports getting the refclock and >>> tcxoclock from an external provider in case a board gets these from >>> external clocks but that can be done as a followup if there are boards >>> in the future using that design. >>> >>> But please bear in mind that I'm not familiar with the clock handling >>> in WiLink6 since the WiLink8 part used in the IGEP boards does not >>> need these clocks and I only looked at Luciano's previous patches and >>> the WiLink today driver today. So it would be good if Eliad can double >>> check my assumptions to see if those are correct. >> sounds right. that's what i know as well. >> Sounds good. I'd also be fine with not implementing the case for >> external clocks in the code until we need (and can test) it, but >> I think it should be specified in the binding from the start. >> > Agreed. > great. so i'll implement the internal clocks case only. thanks, Eliad. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* Eliad Peller <eliad@wizery.com> [150311 06:39]: > On Wed, Mar 11, 2015 at 3:21 PM, Javier Martinez Canillas > <javier@dowhile0.org> wrote: > > On Wed, Mar 11, 2015 at 2:17 PM, Arnd Bergmann <arnd@arndb.de> wrote: > >> On Wednesday 11 March 2015 14:07:11 Javier Martinez Canillas wrote: > >>> > >>> Right now it seems that all boards in mainline with a WiLink6 part are > >>> using internal clocks. So as a first step I think that adding an > >>> optional refclock-frequency and tcxoclock-frequency properties should > >>> be enough. > >>> > >>> It would be good if the driver supports getting the refclock and > >>> tcxoclock from an external provider in case a board gets these from > >>> external clocks but that can be done as a followup if there are boards > >>> in the future using that design. > >>> > >>> But please bear in mind that I'm not familiar with the clock handling > >>> in WiLink6 since the WiLink8 part used in the IGEP boards does not > >>> need these clocks and I only looked at Luciano's previous patches and > >>> the WiLink today driver today. So it would be good if Eliad can double > >>> check my assumptions to see if those are correct. > >> > sounds right. that's what i know as well. > > >> Sounds good. I'd also be fine with not implementing the case for > >> external clocks in the code until we need (and can test) it, but > >> I think it should be specified in the binding from the start. > >> > > Agreed. > > > great. so i'll implement the internal clocks case only. OK great sounds good to me also. Regards, Tony -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/devicetree/bindings/net/wireless/ti,wl18xx.txt b/Documentation/devicetree/bindings/net/wireless/ti,wl18xx.txt new file mode 100644 index 0000000..9dcf535 --- /dev/null +++ b/Documentation/devicetree/bindings/net/wireless/ti,wl18xx.txt @@ -0,0 +1,39 @@ +TI Wilink8 (wl18xx) SDIO devices + +This node provides properties for controlling the wilink8 wireless device. The +node is expected to be specified as a child node to the SDIO controller that +connects the device to the system. + +Required properties: + - compatible : Should be "ti,wl18xx". + - compatible: should be one of the following: + * "ti,wl1801" + * "ti,wl1805" + * "ti,wl1807" + * "ti,wl1831" + * "ti,wl1835" + * "ti,wl1837" + - interrupts : specifies attributes for the out-of-band interrupt. + +Optional properties: + - interrupt-parent : the phandle for the interrupt controller to which the + device interrupts are connected. + +Example: + +&mmc3 { + status = "okay"; + vmmc-supply = <&wlan_en_reg>; + bus-width = <4>; + cap-power-off-card; + keep-power-in-suspend; + + #address-cells = <1>; + #size-cells = <0>; + wlcore: wlcore@2 { + compatible = "ti,wl1835"; + reg = <2>; + interrupt-parent = <&gpio0>; + interrupts = <19 IRQ_TYPE_NONE>; + }; +};
Add device tree binding documentation for TI's wl18xx wlan chip. Signed-off-by: Eliad Peller <eliad@wizery.com> --- .../devicetree/bindings/net/wireless/ti,wl18xx.txt | 39 ++++++++++++++++++++++ 1 file changed, 39 insertions(+) create mode 100644 Documentation/devicetree/bindings/net/wireless/ti,wl18xx.txt