| Message ID | 1404219871-18419-5-git-send-email-sebastian.hesselbarth@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Jul 01, 2014 at 03:04:31PM +0200, Sebastian Hesselbarth wrote: > + pdev = platform_device_register_full(&armada_drm_dev_info); > + /* assign last found lcd node to drm device for clk lookup */ > + pdev->dev.of_node = clknp; NAK. This really isn't a good way to deal with this, even in a temporary basis. While assigning a DT node to a manually created platform device does solve that problem, it also introduces the problem that this platform device will now match any platform driver which recognises the "marvell,dove-lcd" compatible type, which may occur _before_ we find the driver to match using the legacy strings. There really isn't an easy solution to this other than doing the thing properly. The other problem in this series is that while you introduce some bindings which may work today, they're not going to work tomorrow, and that's a problem. Don't do DT piecemeal like this and end up having to break the bindings (which we will have to do to add the endpoints.) If you want to do this then you need to add the endpoints from the start even though the driver doesn't yet make use of them - or don't add the DT bits at all.
On 07/01/2014 03:10 PM, Russell King - ARM Linux wrote: > On Tue, Jul 01, 2014 at 03:04:31PM +0200, Sebastian Hesselbarth wrote: >> + pdev = platform_device_register_full(&armada_drm_dev_info); >> + /* assign last found lcd node to drm device for clk lookup */ >> + pdev->dev.of_node = clknp; > > NAK. This really isn't a good way to deal with this, even in a > temporary basis. While assigning a DT node to a manually created > platform device does solve that problem, it also introduces the > problem that this platform device will now match any platform driver > which recognises the "marvell,dove-lcd" compatible type, which may > occur _before_ we find the driver to match using the legacy strings. Right, I never said it is a good solution but there is no driver for "marvell,dove-lcd" *and* there is no way to assign clock aliases for clocks not yet registered. > There really isn't an easy solution to this other than doing the thing > properly. Well, you may have noticed that three moving subsystems plus new bindings plus non-DT/DT drivers quickly create some kind of patch deadlock. This is a dirty but tiny step to resolve one of those deadlocks. > The other problem in this series is that while you introduce some > bindings which may work today, they're not going to work tomorrow, and > that's a problem. Don't do DT piecemeal like this and end up having to > break the bindings (which we will have to do to add the endpoints.) Adding new properties/subnodes never has been a problem for us at all. New generic bindings were introduced *often* in the past and added to existing bindings, e.g. clocks, gpio, pinctrl. The proposed binding for dove-lcd simply reflects the tiny part that is mandatory for identifying the lcd controllers. It only contains reg and interrupts which would also be in the corresponding platform_device. > If you want to do this then you need to add the endpoints from the start > even though the driver doesn't yet make use of them - or don't add the > DT bits at all. If you really think that way, I definitely give up on mainline Dove and SolidRun Cubox. You are /really/ proposing to wait for *all* related subsystem bindings to settle before even starting to add DT support? Sebastian
On Tue, Jul 01, 2014 at 03:21:41PM +0200, Sebastian Hesselbarth wrote: > On 07/01/2014 03:10 PM, Russell King - ARM Linux wrote: >> On Tue, Jul 01, 2014 at 03:04:31PM +0200, Sebastian Hesselbarth wrote: >>> + pdev = platform_device_register_full(&armada_drm_dev_info); >>> + /* assign last found lcd node to drm device for clk lookup */ >>> + pdev->dev.of_node = clknp; >> >> NAK. This really isn't a good way to deal with this, even in a >> temporary basis. While assigning a DT node to a manually created >> platform device does solve that problem, it also introduces the >> problem that this platform device will now match any platform driver >> which recognises the "marvell,dove-lcd" compatible type, which may >> occur _before_ we find the driver to match using the legacy strings. > > Right, I never said it is a good solution but there is no driver for > "marvell,dove-lcd" *and* there is no way to assign clock aliases for > clocks not yet registered. Now consider what happens when I get to push the updates which do add that to David Airlie. > Well, you may have noticed that three moving subsystems plus new > bindings plus non-DT/DT drivers quickly create some kind of patch > deadlock. This is a dirty but tiny step to resolve one of those > deadlocks. The deadlock only exists because it is impossible to get anyone to look outside their own little world and review patches. This is a growing problem, and there's a growing number of articles and blogs on the Internet about how kernel development now sucks rocks because of it. Where do you get three moving subsystems from anyway? There's the component stuff and the DRM stuff - that's two subsystems. The component stuff is something which I maintain, but needs to go through Greg. I'm seriously thinking about just pushing those three patches to Greg in the next few days. Then I'll start talking to David about the rest of it. The Armada DRM stuff shouldn't be a problem, but the TDA998x may end up being so /if/ Jean-Francois decides to object to my approach there to allow tilcdc to continue working with the driver... but then as I now maintain that as well, that's not much of an issue because I can make the decision to overrule that point. >> The other problem in this series is that while you introduce some >> bindings which may work today, they're not going to work tomorrow, and >> that's a problem. Don't do DT piecemeal like this and end up having to >> break the bindings (which we will have to do to add the endpoints.) > > Adding new properties/subnodes never has been a problem for us at all. > New generic bindings were introduced *often* in the past and added to > existing bindings, e.g. clocks, gpio, pinctrl. > > The proposed binding for dove-lcd simply reflects the tiny part that > is mandatory for identifying the lcd controllers. It only contains > reg and interrupts which would also be in the corresponding > platform_device. There is a difference between augmenting an existing driver binding for new features (such as adding a GPIO to allow a new platform to work) and adding new bindings which are required becaues we've updated the driver. The former adds a new feature but allows existing users to continue working. The latter adds a new feature which breaks existing users. That's a problem. In this case, it can be trivially avoided by adding all the properties that we need right from the start. We know what we need, and the of_graph stuff is now established and in use by other DRM drivers. Just because the existing driver may not make use of it is no excuse to omit it - it's part of the description of how the hardware is connected, it conforms to agreed methods to describe this connectivity, we know that it works, and we know that the driver will need it in the very near future. Omitting it now just results in additional pain later on that we can avoid. >> If you want to do this then you need to add the endpoints from the start >> even though the driver doesn't yet make use of them - or don't add the >> DT bits at all. > > If you really think that way, I definitely give up on mainline Dove and > SolidRun Cubox. You are /really/ proposing to wait for *all* related > subsystem bindings to settle before even starting to add DT support? *Sigh*.
On 07/01/2014 03:36 PM, Russell King - ARM Linux wrote: > On Tue, Jul 01, 2014 at 03:21:41PM +0200, Sebastian Hesselbarth wrote: >> On 07/01/2014 03:10 PM, Russell King - ARM Linux wrote: >>> On Tue, Jul 01, 2014 at 03:04:31PM +0200, Sebastian Hesselbarth wrote: >>> If you want to do this then you need to add the endpoints from the start >>> even though the driver doesn't yet make use of them - or don't add the >>> DT bits at all. >> >> If you really think that way, I definitely give up on mainline Dove and >> SolidRun Cubox. You are /really/ proposing to wait for *all* related >> subsystem bindings to settle before even starting to add DT support? > > *Sigh*. Russell, honestly, I am out. Feel free to take over anything you want. I understand you are in a bad mood because you feel ignored on your patches, but for me your attitude is unbearable and *the* reason to step back now. Sebastian
On Tue, Jul 01, 2014 at 03:40:14PM +0200, Sebastian Hesselbarth wrote: > On 07/01/2014 03:36 PM, Russell King - ARM Linux wrote: >> On Tue, Jul 01, 2014 at 03:21:41PM +0200, Sebastian Hesselbarth wrote: >>> On 07/01/2014 03:10 PM, Russell King - ARM Linux wrote: >>>> On Tue, Jul 01, 2014 at 03:04:31PM +0200, Sebastian Hesselbarth wrote: >>>> If you want to do this then you need to add the endpoints from the start >>>> even though the driver doesn't yet make use of them - or don't add the >>>> DT bits at all. >>> >>> If you really think that way, I definitely give up on mainline Dove and >>> SolidRun Cubox. You are /really/ proposing to wait for *all* related >>> subsystem bindings to settle before even starting to add DT support? >> >> *Sigh*. > > Russell, > > honestly, I am out. Feel free to take over anything you want. I > understand you are in a bad mood because you feel ignored on your > patches, but for me your attitude is unbearable and *the* reason > to step back now. That's fine, and probably for the best, since you appear not to be able to take part in the completely reasonable technical discussion in this thread. I wish you all the best in your future endeavours.
On Tue, 1 Jul 2014 14:36:27 +0100 Russell King - ARM Linux <linux@arm.linux.org.uk> wrote: > The Armada DRM stuff shouldn't be a problem, but the TDA998x may end up > being so /if/ Jean-Francois decides to object to my approach there to > allow tilcdc to continue working with the driver... but then as I now > maintain that as well, that's not much of an issue because I can make > the decision to overrule that point. I proposed a patch to move the tda998x from a slave_encoder to a simple encoder/connector. This patch included the associated changes to the tilcdc. It has been violently refused. So, I will keep out of tree my tda998x and Dove DRM drivers. Now, the last thing for me is to put the TDA998x codec in the kernel (it is also working in an other machine with 2 tda998x's). So, you may do what you want...
On Tue, Jul 01, 2014 at 06:04:26PM +0200, Jean-Francois Moine wrote: > On Tue, 1 Jul 2014 14:36:27 +0100 > Russell King - ARM Linux <linux@arm.linux.org.uk> wrote: > > > The Armada DRM stuff shouldn't be a problem, but the TDA998x may end up > > being so /if/ Jean-Francois decides to object to my approach there to > > allow tilcdc to continue working with the driver... but then as I now > > maintain that as well, that's not much of an issue because I can make > > the decision to overrule that point. > > I proposed a patch to move the tda998x from a slave_encoder to a simple > encoder/connector. This patch included the associated changes to the > tilcdc. It has been violently refused. So, I will keep out of tree my > tda998x and Dove DRM drivers. Let's tell the full story rather than just presenting half of it. You indeed wanted to do what you said above, but you also wanted to completely change the component helpers in a way that I was not happy with. You wanted to add all sorts of DT specific gunk into the helpers, which would have tied it to DT. While DT is the current thing on ARM, it is /not/ the current thing everywhere - a point which you failed to grasp. You wanted to make the component operations optional, which I pointed out makes no sense (because then components have no way to know what happens to their master device - which is /really/ important for DRM.) You refused to listen to those concerns, and refused to look at the patch which I proposed, which did exactly the same as your patch, while keeping the DRM slave interfaces for tilcdc to use, until they have a chance to convert over. You kept telling me that I had "opened the door" to your changes. I claimed that your changes abused the code which I had written - a point which I still maintain to this day. You also claimed that deferred probing didn't work. Since the component helpers were designed with the deferred probing problem in mind at the time, and include the solution to that problem - which has been well tested hundreds if not thousands of times by now - and you did not provide the technical details as to why you thought it didn't work, there was nothing that could be done to progress that point. Moreover, your abuse of the component layer would have made it more difficult to maintain it into the future - which is a fundamental point which has to be considered when accepting any patch into the kernel. If a patch makes some code unable to be maintained, then an alternative approach has to be found. Since you were not willing to compromise on finding or considering alternative approaches such as the one I presented. Since the component layer had had various comments which were in progress, and your abuse of the component layer would have also prevented those changes taking place (which are - in part - the set of component patches which are on the list now) there is no way that your uncompromising set of patches would be merged - at least not until you start accepting some of the comments being given to you. > Now, the last thing for me is to put the TDA998x codec in the kernel > (it is also working in an other machine with 2 tda998x's). Yes, supporting the I2S connection is something that is need, but we /also/ need to support SPDIF, and SPDIF is the preferred method on the Cubox. SPDIF should be used to talk to the TDA998x whenever possible because it opens up the possibility for sending out compressed MPEG2 and AC-3 audio streams, thus offloading the decode of these formats to external hardware. This works today, and is a feature that people have been using with platforms such as xbmc and various other installations on the Cubox. Limiting to I2S means that you can't send out these compressed audio streams. In fact, the Dove manual tells you that you must disable the I2S playback stream if you're sending non-PCM - non-PCM is only supported via SPDIF.
Russell, On Tue, Jul 01, 2014 at 02:10:26PM +0100, Russell King - ARM Linux wrote: > On Tue, Jul 01, 2014 at 03:04:31PM +0200, Sebastian Hesselbarth wrote: > > + pdev = platform_device_register_full(&armada_drm_dev_info); > > + /* assign last found lcd node to drm device for clk lookup */ > > + pdev->dev.of_node = clknp; > > NAK. This really isn't a good way to deal with this, even in a > temporary basis. While assigning a DT node to a manually created > platform device does solve that problem, it also introduces the > problem that this platform device will now match any platform driver > which recognises the "marvell,dove-lcd" compatible type, which may > occur _before_ we find the driver to match using the legacy strings. > > There really isn't an easy solution to this other than doing the thing > properly. And this creates our current deadlock. > The other problem in this series is that while you introduce some > bindings which may work today, they're not going to work tomorrow, and > that's a problem. Don't do DT piecemeal like this and end up having to > break the bindings (which we will have to do to add the endpoints.) Seriously, we've got to do something to get off of the X. Why not just declare the binding Sebastian just sent as unstable? Then we change it as needed until we get all the pieces in place. At KS2013, we discussed, and agreed upon the fact that we _can_ have unstable bindings to keep the bus moving. Perfection being the enemy of getting it done and all. Once we reach the goal of how we want the bindings to look, then we declare it stable. I know it's not ideal, but "plan it out perfectly before pushing anything" isn't realistic either. thx, Jason.
On Tue, Jul 01, 2014 at 12:53:19PM -0400, Jason Cooper wrote: > Russell, > > On Tue, Jul 01, 2014 at 02:10:26PM +0100, Russell King - ARM Linux wrote: > > On Tue, Jul 01, 2014 at 03:04:31PM +0200, Sebastian Hesselbarth wrote: > > > + pdev = platform_device_register_full(&armada_drm_dev_info); > > > + /* assign last found lcd node to drm device for clk lookup */ > > > + pdev->dev.of_node = clknp; > > > > NAK. This really isn't a good way to deal with this, even in a > > temporary basis. While assigning a DT node to a manually created > > platform device does solve that problem, it also introduces the > > problem that this platform device will now match any platform driver > > which recognises the "marvell,dove-lcd" compatible type, which may > > occur _before_ we find the driver to match using the legacy strings. > > > > There really isn't an easy solution to this other than doing the thing > > properly. > > And this creates our current deadlock. Right, and what it does is it creates /another/ deadlock. Let's say that we get those changes in place in the mvebu tree. I'm pushing to get the component stuff in place right now. Once it's in place, I'll be pushing the Armada DRM stuff forward. Let's say that gets in. We now have a problem. The mvebu stuff now creates a platform device which will bind with the CRTC part of Armada DRM, rather than the master part and everything now breaks. So, rather than having a component / DRM problem, it ends up becoming a component / DRM / mvebu problem - rather than just two subsystems needing coordination, we end up with /another/ subsystem to contend with as well. > > The other problem in this series is that while you introduce some > > bindings which may work today, they're not going to work tomorrow, and > > that's a problem. Don't do DT piecemeal like this and end up having to > > break the bindings (which we will have to do to add the endpoints.) > > Seriously, we've got to do something to get off of the X. Why not just > declare the binding Sebastian just sent as unstable? Then we change it > as needed until we get all the pieces in place. As I've already pointed out, why not add the complete binding right now and be done with it? That would sort out my concern about DT, and - as I've already said (again) - we know exactly how this should look because we have the of_graph stuff in drivers/of as standard helpers, and we have imx-drm and v4l2 media stuff using it already. It's well established, and I've demonstrated that Armada DRM _will_ also conform. In fact, I have plans to consolidate the code between Armada DRM and imx-drm into some standard DRM helpers (if you read through the patches I linked to on cgit, you will notice that has already started.) But... the DT issue is relatively minor compared to the one which I raise above, which adds to the problems of moving forward. It effectively means I need to then carry a patch which undoes that, and that patch would need to go through the DRM tree along with the rest of the Armada DRM updates, which means more people need to be involved, which then makes the process even slower than it already is.
On Tue, 1 Jul 2014 17:45:27 +0100 Russell King - ARM Linux <linux@arm.linux.org.uk> wrote: > Let's tell the full story rather than just presenting half of it. > > You indeed wanted to do what you said above, but you also wanted to > completely change the component helpers in a way that I was not happy > with. You wanted to add all sorts of DT specific gunk into the > helpers, which would have tied it to DT. > > While DT is the current thing on ARM, it is /not/ the current thing > everywhere - a point which you failed to grasp. I don't think that there will ever be a x86 DT. Yes, I did not know what was the DT. For me, when entering in the ARM world, it was a marvellous tool which could have ended in a unique ARM kernel. I'm a dreamer! > You wanted to make the component operations optional, which I pointed > out makes no sense (because then components have no way to know what > happens to their master device - which is /really/ important for DRM.) You did not explain too much that you wanted to keep it for DRM only. > You refused to listen to those concerns, and refused to look at the > patch which I proposed, which did exactly the same as your patch, > while keeping the DRM slave interfaces for tilcdc to use, until they > have a chance to convert over. The change in tilcdc was easy and the code was greatly simplified. > You kept telling me that I had "opened the door" to your changes. I > claimed that your changes abused the code which I had written - a point > which I still maintain to this day. Your code was offering a simple way to synchronize the system initialization without this lot of 'probe defer's. It could have been used so. > You also claimed that deferred probing didn't work. Since the component > helpers were designed with the deferred probing problem in mind at the > time, and include the solution to that problem - which has been well > tested hundreds if not thousands of times by now - and you did not > provide the technical details as to why you thought it didn't work, > there was nothing that could be done to progress that point. AFAIR, you also said that the probe defer was not working. But, thanks again for your component layer: all my init problems are gone, even if there are still some prove defer's... > Moreover, your abuse of the component layer would have made it more > difficult to maintain it into the future - which is a fundamental > point which has to be considered when accepting any patch into the > kernel. If a patch makes some code unable to be maintained, then an > alternative approach has to be found. Since you were not willing to > compromise on finding or considering alternative approaches such as > the one I presented. I don't see what you are talking about. > Since the component layer had had various comments which were in > progress, and your abuse of the component layer would have also > prevented those changes taking place (which are - in part - the set > of component patches which are on the list now) there is no way that > your uncompromising set of patches would be merged - at least not > until you start accepting some of the comments being given to you. > > > Now, the last thing for me is to put the TDA998x codec in the kernel > > (it is also working in an other machine with 2 tda998x's). > > Yes, supporting the I2S connection is something that is need, but we > /also/ need to support SPDIF, and SPDIF is the preferred method on > the Cubox. SPDIF should be used to talk to the TDA998x whenever > possible because it opens up the possibility for sending out > compressed MPEG2 and AC-3 audio streams, thus offloading the decode > of these formats to external hardware. > > This works today, and is a feature that people have been using with > platforms such as xbmc and various other installations on the Cubox. > > Limiting to I2S means that you can't send out these compressed audio > streams. In fact, the Dove manual tells you that you must disable > the I2S playback stream if you're sending non-PCM - non-PCM is only > supported via SPDIF. I don't understand: both I2S and S/PDIF may work in the kirkwood audio subsystem as it is (yes, actually not at the same time, and this is a choice because DPCM does not work for the Cubox). I have the 3 ways in my system: $ cat /proc/asound/pcm 00-00: i2s-i2s-hifi i2s-hifi-0 : : playback 1 00-01: spdif-spdif-hifi spdif-hifi-1 : : playback 1 00-02: spdif-dit-hifi dit-hifi-2 : : playback 1 So, S/PDIF should work, but I don't know it: I have no optical connector.
On Tue, Jul 01, 2014 at 07:49:53PM +0200, Jean-Francois Moine wrote: > On Tue, 1 Jul 2014 17:45:27 +0100 > Russell King - ARM Linux <linux@arm.linux.org.uk> wrote: > > Let's tell the full story rather than just presenting half of it. > > > > You indeed wanted to do what you said above, but you also wanted to > > completely change the component helpers in a way that I was not happy > > with. You wanted to add all sorts of DT specific gunk into the > > helpers, which would have tied it to DT. > > > > While DT is the current thing on ARM, it is /not/ the current thing > > everywhere - a point which you failed to grasp. > > I don't think that there will ever be a x86 DT. x86 is ACPI, which is a completely different firmware interface. Now, we are starting to see ARM peripherals (even some rather old ones) being used on some x86 platforms. You can't assume that just because something is used on ARM, it's never going to be used on any other architecture. IP which gets used with one host CPU, even where it's integrated into a SoC, can find its way to other CPU architectures very easily, and this really does happen. So don't be surprised one day to find (eg) that the Dove/Kirkwood I2S audio interface ends up being connected to an x86 CPU at some point. Or the TDA998x ends up being used on x86 in a set top box. You really can't think that just because something seems to be ARM specific that it's absolutely fine to assume that it will always be specific to ARM - /especially/ when adding core kernel facilities. Core kernel facilities have to be completely independent of any CPU architecture. If they aren't, by definition they aren't core kernel facilities. > > You wanted to make the component operations optional, which I pointed > > out makes no sense (because then components have no way to know what > > happens to their master device - which is /really/ important for DRM.) > > You did not explain too much that you wanted to keep it for DRM only. It /isn't/ just for DRM, I've no idea what gave you that idea. I've said that I want to keep DT out of the core component helpers (see above why.) I also want to keep subsystem specifics out of it as well. > > You refused to listen to those concerns, and refused to look at the > > patch which I proposed, which did exactly the same as your patch, > > while keeping the DRM slave interfaces for tilcdc to use, until they > > have a chance to convert over. > > The change in tilcdc was easy and the code was greatly simplified. The same applies to mine once tilcdc is converted to the component helpers - provided there are no other users of TDA998x, then the non-component code can all be dropped and the driver reduced down - and at that point it becomes purely a drm encoder with an associated drm connector with just one setup methodology. > > You kept telling me that I had "opened the door" to your changes. I > > claimed that your changes abused the code which I had written - a point > > which I still maintain to this day. > > Your code was offering a simple way to synchronize the system > initialization without this lot of 'probe defer's. It could have been > used so. My code does /today/ offer a simple way to sort out the initialisation without lots of deferred probes. This is what happens: 1. There are zero deferred probes until all components for the master are present. 2. You will only get a deferred probe once all components are present _unless_ the master bind function returns a deferred probe, which _may_ come from a component driver. So, the deferred probes are reduced to an absolute minimum. Again, this is by intention and design. When resources are not available, we have to rely on the deferred probe mechanism, because the deferred probe mechanism knows when other non- component drivers are probed - and thus when resources may become available. This is not really any different from a single driver trying to claim resources, finding one that isn't available, and returning -EPROBE_DEFER - in fact, only the last driver to be probed in a set of components which satisfies the master will suffer deferred probes. I'm now pushing changes to the component helper which change the way the matching works, so that master doesn't have to keep scanning whatever it needs to in order to work out which components are required. Masters will be required to gather that information before registering themselves and supply it when registering into the component helper. This is beneficial for two reasons - not only does it avoid wasting time rescanning (eg) the device tree multiple times, but it also means that we can eventually allow partial subsystem bring-up which is something the v4l2 people would like. So, there's quite some work planned here which will require a number of changes to the visible API. So, it's really important that all users make use of the component layer as it is intended to be used, so that changes can be made. Right now, that hasn't happened with Exynos, and that is right now giving problems pushing this work forward - and that's the /exact/ problem when stuff starts getting abused. It means that it can't be maintained and it blocks other work. As the person who created this problem isn't responding to my emails, it's pretty hard to see how to retire the old interfaces - except maybe by forcing the issue which I really don't want to do. > > You also claimed that deferred probing didn't work. Since the component > > helpers were designed with the deferred probing problem in mind at the > > time, and include the solution to that problem - which has been well > > tested hundreds if not thousands of times by now - and you did not > > provide the technical details as to why you thought it didn't work, > > there was nothing that could be done to progress that point. > > AFAIR, you also said that the probe defer was not working. But, thanks > again for your component layer: all my init problems are gone, even if > there are still some prove defer's... That's really strange, because imx-drm relies upon it since day one, and I even sent you the kernel boot log which provided evidence of it working correctly. Yes, there have been a few corner cases where the devm resources were not handled quite correctly, but those have been corrected. > > Moreover, your abuse of the component layer would have made it more > > difficult to maintain it into the future - which is a fundamental > > point which has to be considered when accepting any patch into the > > kernel. If a patch makes some code unable to be maintained, then an > > alternative approach has to be found. Since you were not willing to > > compromise on finding or considering alternative approaches such as > > the one I presented. > > I don't see what you are talking about. See my explanation of the upcoming changes above - some of those changes are required for Armada DRM (required in the sense that I want Armada DRM to use the new way right now rather than have to rework yet another DRM driver for those changes, pushing the component changes even later - allowing more users to come along potentially blocking the work.) > > Since the component layer had had various comments which were in > > progress, and your abuse of the component layer would have also > > prevented those changes taking place (which are - in part - the set > > of component patches which are on the list now) there is no way that > > your uncompromising set of patches would be merged - at least not > > until you start accepting some of the comments being given to you. > > > > > Now, the last thing for me is to put the TDA998x codec in the kernel > > > (it is also working in an other machine with 2 tda998x's). > > > > Yes, supporting the I2S connection is something that is need, but we > > /also/ need to support SPDIF, and SPDIF is the preferred method on > > the Cubox. SPDIF should be used to talk to the TDA998x whenever > > possible because it opens up the possibility for sending out > > compressed MPEG2 and AC-3 audio streams, thus offloading the decode > > of these formats to external hardware. > > > > This works today, and is a feature that people have been using with > > platforms such as xbmc and various other installations on the Cubox. > > > > Limiting to I2S means that you can't send out these compressed audio > > streams. In fact, the Dove manual tells you that you must disable > > the I2S playback stream if you're sending non-PCM - non-PCM is only > > supported via SPDIF. > > I don't understand: both I2S and S/PDIF may work in the kirkwood audio > subsystem as it is (yes, actually not at the same time, and this is a > choice because DPCM does not work for the Cubox). I have the 3 ways in > my system: > > $ cat /proc/asound/pcm > 00-00: i2s-i2s-hifi i2s-hifi-0 : : playback 1 > 00-01: spdif-spdif-hifi spdif-hifi-1 : : playback 1 > 00-02: spdif-dit-hifi dit-hifi-2 : : playback 1 > > So, S/PDIF should work, but I don't know it: I have no optical connector. How does that even work - I mean, pulseaudio will try and open all three at startup and it sends samples through each... Have you tested this with pulseaudio? Have you tested it with other programs such as an audio player going direct to the ALSA interfaces (as would happen with a compressed audio stream) - and checked what happens if pulse wants to use it? You don't need an actual compressed audio stream to test that - you can just send audio direct to one of your other devices. Second point - if you have the TDA998x only using i2s, then (as you say) you can't use spdif, so simultaneous output via both HDMI and optical is impossible with your driver on the Cubox.
diff --git a/arch/arm/mach-mvebu/dove.c b/arch/arm/mach-mvebu/dove.c index b50464e..c8c2284 100644 --- a/arch/arm/mach-mvebu/dove.c +++ b/arch/arm/mach-mvebu/dove.c @@ -11,11 +11,75 @@ #include <linux/init.h> #include <linux/mbus.h> #include <linux/of.h> +#include <linux/of_address.h> #include <linux/of_platform.h> #include <asm/hardware/cache-tauros2.h> #include <asm/mach/arch.h> +#include <asm/memblock.h> #include "common.h" +/* + * Armada DRM framebuffer + */ +static struct resource armada_drm_resources[3] = { + DEFINE_RES_MEM(0, 0), +}; + +static const char *armada_drm_devices[4]; + +static const struct platform_device_info armada_drm_dev_info __initconst = { + .name = "armada-510-drm", + .id = -1, + .res = armada_drm_resources, + .num_res = ARRAY_SIZE(armada_drm_resources), + .data = armada_drm_devices, + .size_data = sizeof(armada_drm_devices), +}; + +static void __init armada_drm_reserve(void) +{ + phys_addr_t phys; + + /* Steal 32MB for the drm framebuffers */ + phys = arm_memblock_steal(SZ_32M, SZ_2M); + armada_drm_resources[0].start = phys; + armada_drm_resources[0].end = phys + SZ_32M - 1; +} + +static void __init armada_drm_init(void) +{ + struct device_node *np, *clknp = NULL; + struct platform_device *pdev; + int i = 0; + + for_each_compatible_node(np, NULL, "marvell,dove-lcd") { + if (!of_device_is_available(np)) + continue; + + /* add lcd controller mmio resources */ + if (of_address_to_resource(np, 0, &armada_drm_resources[i+1])) + continue; + + /* get device name for component framework */ + pdev = of_find_device_by_node(np); + if (pdev) + armada_drm_devices[i] = dev_name(&pdev->dev); + clknp = np; + i++; + } + armada_drm_devices[i++] = NULL; + + pdev = platform_device_register_full(&armada_drm_dev_info); + /* assign last found lcd node to drm device for clk lookup */ + pdev->dev.of_node = clknp; +} + +static void __init dove_reserve(void) +{ + if (IS_ENABLED(CONFIG_DRM_ARMADA)) + armada_drm_reserve(); +} + static void __init dove_init(void) { pr_info("Dove 88AP510 SoC\n"); @@ -25,6 +89,9 @@ static void __init dove_init(void) #endif BUG_ON(mvebu_mbus_dt_init(false)); of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL); + + if (IS_ENABLED(CONFIG_DRM_ARMADA)) + armada_drm_init(); } static const char * const dove_dt_compat[] = { @@ -33,7 +100,8 @@ static const char * const dove_dt_compat[] = { }; DT_MACHINE_START(DOVE_DT, "Marvell Dove") + .dt_compat = dove_dt_compat, .init_machine = dove_init, + .reserve = dove_reserve, .restart = mvebu_restart, - .dt_compat = dove_dt_compat, MACHINE_END