Re: [alsa-devel] [PATCH] ASoC: da9055: Partially fix device
On Wed, 8 Jan 2014 20:38:55 +0000, Mark Brown wrote:
It has never been possible to even load the da9055 driver since both the MFD and CODEC drivers register the I2C device "da9055". Fix this by using the usual pattern for MFDs with multiple I2C addresses and having the CODEC driver reference an I2C dummy registered by the MFD.
Since I don't know which I2C address to use for the CODEC a FIXME has been left in the MFD, this doesn't make anything any worse since the device has never been able to load in the first place.
What you're saying here is incorrect, as previously discussed at length in this thread:
http://mailman.alsa-project.org/pipermail/alsa-devel/2013-September/066027.h...
The two drivers did work together fine until someone in 3.12 changed the PMIC I2C Id back to just 'da9055' and broke it again. Also, I wanted to add a simple patch to tidy up the I2C Id names and avoid this but you would not agree. The codec part can be standalone and by doing what you've done here means that customers would need to now add additional, unnecessary code to be able to use just the codec driver. This is not right to me.
On Thu, Jan 09, 2014 at 11:30:42AM +0000, Opensource [Adam Thomson] wrote:
On Wed, 8 Jan 2014 20:38:55 +0000, Mark Brown wrote:
Please fix the word wrapping in your mailer to something less than 80 columns, I've reflowed for legibility.
Since I don't know which I2C address to use for the CODEC a FIXME has been left in the MFD, this doesn't make anything any worse since the device has never been able to load in the first place.
What you're saying here is incorrect, as previously discussed at length in this thread:
http://mailman.alsa-project.org/pipermail/alsa-devel/2013-September/066027.h...
The two drivers did work together fine until someone in 3.12 changed the PMIC I2C Id back to just 'da9055' and broke it again. Also, I wanted to add a simple patch to tidy up
Hrm, you hadn't mentioned the rename in that thread. I do note that the change was from KPIT Cummins who submitted the drivers for this chip for you guys and appear to be doing updates too...
the I2C Id names and avoid this but you would not agree. The codec part can be standalone and by doing what you've done here means that customers would need to now add additional, unnecessary code to be able to use just the codec driver. This is not right to me.
No, users will not have to do any extra work - if anything they have to do marginally less with only registering one device. As discussed this is the way Linux handles things (which is also the rationale in the changelog for the PMIC rename). Looking at the changelogs I'd not be surprised if the rename was done due to the author getting caught out by the non-standard way the driver was done.
On Wed, 9 Jan 2014 12:14:53 +0000, Mark Brown wrote:
Hrm, you hadn't mentioned the rename in that thread. I do note that the change was from KPIT Cummins who submitted the drivers for this chip for you guys and appear to be doing updates too...
I said at the beginning of that thread that we would be taking responsibility of the driver and KPIT Cummins are no longer involved with this work. At that point the change to the PMIC I2C id name had already been made (by KPIT) and was in the kernel. I refer you to this mail in the thread, and to the last paragraph where I make the proposal of changing the codec I2C Id name to tidy things up:
http://mailman.alsa-project.org/pipermail/alsa-devel/2013-September/066138.h...
the I2C Id names and avoid this but you would not agree. The codec part can be standalone and by doing what you've done here means that customers would need to now add additional, unnecessary code to be able to use just the codec driver. This is not right to me.
No, users will not have to do any extra work - if anything they have to do marginally less with only registering one device. As discussed this is the way Linux handles things (which is also the rationale in the changelog for the PMIC rename). Looking at the changelogs I'd not be surprised if the rename was done due to the author getting caught out by the non-standard way the driver was done.
Use JUST the codec driver. No PMIC. For some chips, 'the way Linux handles things' makes sense, but not here. I don't want to re-cover old ground though as we spent long enough over this last time.
What I would like to highlight, and it's something that concerns me more, is that someone went into the kernel and changed the I2C Id name for no good reason, and didn't test if their submission worked correctly (I am of course well aware in the original submission from KPIT there was a conflict in the I2C Id name, but that was rectified, and would've been tidied up further had it been permitted). On top of that you are also leaving it in an unusable state with your change. If we direct customers to the drivers in the mainline kernel, the codec cannot be simply instantiated as before, and certainly won't be instantiated from the MFD code. How is this acceptable?
On Thu, Jan 09, 2014 at 02:22:48PM +0000, Opensource [Adam Thomson] wrote:
On Wed, 9 Jan 2014 12:14:53 +0000, Mark Brown wrote:
Hrm, you hadn't mentioned the rename in that thread. I do note that the change was from KPIT Cummins who submitted the drivers for this chip for you guys and appear to be doing updates too...
I said at the beginning of that thread that we would be taking responsibility of the driver and KPIT Cummins are no longer involved with this work. At that point the change to the PMIC I2C id name had already been made (by KPIT) and was in the kernel. I refer you to this mail in the thread, and to the last paragraph
Right, but at the time...
No, users will not have to do any extra work - if anything they have to do marginally less with only registering one device. As discussed this is the way Linux handles things (which is also the rationale in the changelog for the PMIC rename). Looking at the changelogs I'd not be surprised if the rename was done due to the author getting caught out by the non-standard way the driver was done.
Use JUST the codec driver. No PMIC. For some chips, 'the way Linux handles things' makes sense, but not here. I don't want to re-cover old ground though as we spent long enough over this last time.
If you want to support any additional CODEC only devices with compatible register maps that exist you just need to add something like less than a hundred lines of mostly boilerplate code to do so in the ASoC driver. It's hard to see any difference to devices that have both I2C and SPI or both PCI and platform buses here. Those devices should have separate device ID entries anyway.
What I would like to highlight, and it's something that concerns me more, is that someone went into the kernel and changed the I2C Id name for no good reason, and didn't test if their submission worked correctly (I am of course well aware in the original submission from KPIT there was a conflict in the I2C Id name, but that was rectified, and would've been tidied up further had it been permitted). On top of that you are also leaving it in an unusable state with your change. If we direct customers to the drivers in the mainline kernel, the codec cannot be simply instantiated as before, and certainly won't be instantiated from the MFD code. How is this acceptable?
I do agree that the current situation is not great - this is intended to try to move things forwards since mainline is currently broken for this device and has been for a couple of releases now. One of the things my patch is trying to do is to get someone to tell us what we need to plug into i2c_add_dummy() to make this work. I had hoped that if I wrote the code we could get this fixed but without documentation or something that last bit can't be done, would that be possible?
On Thu, 9 Jan 2014 20:24:46 +0000, Mark Brown wrote:
If you want to support any additional CODEC only devices with compatible register maps that exist you just need to add something like less than a hundred lines of mostly boilerplate code to do so in the ASoC driver. It's hard to see any difference to devices that have both I2C and SPI or both PCI and platform buses here. Those devices should have separate device ID entries anyway.
The point is here is that it's unnecessary what you're suggesting. You're basically adding more code to instantiate the Codec from the PMIC, removing the valid I2C code from the Codec driver, and then if a user/customer wants to use the Codec standalone then they need to add that code again. Agreed that new device Ids should be added in the future for new standalone variants, but that should not mean having to add all of the I2C boilerplate code again. It think it makes things unnecessarily messy, on top of the additional effort required.
I do agree that the current situation is not great - this is intended to try to move things forwards since mainline is currently broken for this device and has been for a couple of releases now. One of the things my patch is trying to do is to get someone to tell us what we need to plug into i2c_add_dummy() to make this work. I had hoped that if I wrote the code we could get this fixed but without documentation or something that last bit can't be done, would that be possible?
Well, the truth is that there are default I2C addresses for both PMIC and Codec but as they are standalone devices, albeit in one package, both addresses can be changed. This is one more reason why I don't believe we should be taking your method of implementation here. If you just make sure PMIC and Codec drivers have unique Ids then the problem is solved. If you are worried about people changing this again in the future (although for the life of me I don't know why they would), then we can add a glaring comment to dissuade them, or point them at the codec driver.
On Fri, Jan 10, 2014 at 06:32:38PM +0000, Opensource [Adam Thomson] wrote:
On Thu, 9 Jan 2014 20:24:46 +0000, Mark Brown wrote:
If you want to support any additional CODEC only devices with compatible register maps that exist you just need to add something like less than a hundred lines of mostly boilerplate code to do so in the ASoC driver. It's hard to see any difference to devices that have both I2C and SPI or both PCI and platform buses here. Those devices should have separate device ID entries anyway.
The point is here is that it's unnecessary what you're suggesting. You're basically adding more code to instantiate the Codec from the PMIC, removing the valid I2C code from the Codec driver, and then if a user/customer wants to use the Codec standalone then they need to add that code again. Agreed that new device Ids should be added in the future for new standalone variants, but that should not mean having to add all of the I2C boilerplate code again. It think it makes things unnecessarily messy, on top of the additional effort required.
We also get people complaining about other stylistic things that they get asked to fix and people going round fixing style issues - the whole reason this broke is that people noticed and fixed the fact that the drivers looked wrong to inspection without realising that there were multiple drivers for the device because it's not done in the manner we usually do this (that's definitely why the CODEC driver was changed, I picked up on it during review on submission). It's really useful to have things follow common patterns, it makes maintianing the system easier and it makes factoring out shareable code easier. Things like interfaces to the rest of the system tend to get particular focus.
Please, this isn't helping...
Well, the truth is that there are default I2C addresses for both PMIC and Codec but as they are standalone devices, albeit in one package, both addresses can be changed. This is one more reason why I don't believe we should be taking your method of implementation here. If you just make sure PMIC and Codec drivers
...but on the other hand this sounds like an actual technical issue which could be a good reason to do something different - it's a bit surprising that it didn't come up before. What exactly is the flexibility the devices have here? It's a pretty unusual hardware design.
have unique Ids then the problem is solved. If you are worried about people changing this again in the future (although for the life of me I don't know why they would), then we can add a glaring comment to dissuade them, or point them at the codec driver.
That's definitely required.
On Mon, 13 Jan 2014 21:19:06 +0000, Mark Brown wrote:
We also get people complaining about other stylistic things that they get asked to fix and people going round fixing style issues - the whole reason this broke is that people noticed and fixed the fact that the drivers looked wrong to inspection without realising that there were multiple drivers for the device because it's not done in the manner we usually do this (that's definitely why the CODEC driver was changed, I picked up on it during review on submission). It's really useful to have things follow common patterns, it makes maintianing the system easier and it makes factoring out shareable code easier. Things like interfaces to the rest of the system tend to get particular focus.
Please, this isn't helping...
I don't disagree with you on common patterns, and I have been an advocate of following these when working in the kernel as it generally makes things simpler and easier. Here I'm not trying to be awkward but am trying to get a simple, flexible, solution which fits best with our devices, whilst still using common kernel frameworks.
Well, the truth is that there are default I2C addresses for both PMIC and Codec but as they are standalone devices, albeit in one package, both addresses can be changed. This is one more reason why I don't believe we should be taking your method of implementation here. If you just make sure PMIC and Codec drivers
...but on the other hand this sounds like an actual technical issue which could be a good reason to do something different - it's a bit surprising that it didn't come up before. What exactly is the flexibility the devices have here? It's a pretty unusual hardware design.
The PMIC is configurable via a register in its register map, and the Codec is configurable via an OTP register, which a customer is able to set themselves through a series of register writes, in the Codec register map. When you mentioned the i2c_add_dummy() approach again in your last mail it caused me to go over your patch once more, and in doing so I then spotted this. I do wish I had picked this out earlier though as it may have saved some time in e-mail discussions.
On Tue, Jan 14, 2014 at 11:04:22AM +0000, Opensource [Adam Thomson] wrote:
I don't disagree with you on common patterns, and I have been an advocate of following these when working in the kernel as it generally makes things simpler and easier. Here I'm not trying to be awkward but am trying to get a simple, flexible, solution which fits best with our devices, whilst still using common kernel frameworks.
That's not what's been coming over.
The PMIC is configurable via a register in its register map, and the Codec is configurable via an OTP register, which a customer is able to set themselves through a series of register writes, in the Codec register map. When you
Are you sure that it's not the other way around? It's much more common to have OTP for PMICs... In any case, for the device configured via the register map is there any real scenario where someone might use that feature - devices do have this facility but it's generally never used since by the time you can do register writes to configure the address register I/O is already up and running so it's just making work.
On Tue, 14 Jan 2014 19:52:19 +0000, Mark Brown wrote:
I don't disagree with you on common patterns, and I have been an advocate of following these when working in the kernel as it generally makes things simpler and easier. Here I'm not trying to be awkward but am trying to get a simple, flexible, solution which fits best with our devices, whilst still using common kernel frameworks.
That's not what's been coming over.
The drivers follow common patterns. The only thing slightly different is that both the Codec and PMIC are initialised separately, but this reflects the nature of the chip. I don't believe wanting to add a simple solution which fits with that as being awkward or against common methods and am not overly happy you're trying to suggest otherwise, especially when previous kernel submissions I've been involved with have very much followed common kernel practices. I just want the correct solution for our drivers here.
Are you sure that it's not the other way around? It's much more common to have OTP for PMICs... In any case, for the device configured via the register map is there any real scenario where someone might use that feature - devices do have this facility but it's generally never used since by the time you can do register writes to configure the address register I/O is already up and running so it's just making work.
Yes I'm sure, and there are OTP settings for the PMIC as well. For the PMIC address configurability, there isn't really an issue anyway as you need to provide the address for this I2C device at board initialisation in the kernel. The point of note here is that the Codec I2C address is OTP configurable and therefore not fixed.
On Thu, Jan 16, 2014 at 02:11:03AM +0000, Opensource [Adam Thomson] wrote:
On Tue, 14 Jan 2014 19:52:19 +0000, Mark Brown wrote:
That's not what's been coming over.
The drivers follow common patterns. The only thing slightly different is that both the Codec and PMIC are initialised separately, but this reflects the nature of the chip. I don't believe wanting to add a simple solution which fits with that as being awkward or against common methods and am not overly happy you're trying to suggest otherwise, especially when previous kernel submissions I've been involved with have very much followed common kernel practices. I just want the correct solution for our drivers here.
Sure, like I say this is about the impression that is created rather than your actual intention. One particular thing I'd highlight is that it's really common for device vendors to say that their device or code is special in some way and can't do the standard things but this rarely turns out to be true. It is therefore really important to do things like highlight specific technical things that mean new approaches are needed (like the fact that the addresses are independently controllable for the functions on this device) otherwise it is very easy for it to look like a common pattern is being repeated.
Yes I'm sure, and there are OTP settings for the PMIC as well. For the PMIC address configurability, there isn't really an issue anyway as you need to provide the address for this I2C device at board initialisation in the kernel. The point of note here is that the Codec I2C address is OTP configurable and therefore not fixed.
OK, that's fine - I was just really surprised since the normal thing would be that if only one function was going to have the address configured via OTP it'd be the PMIC.
What I would suggest doing is writing a binding document for the device (which seems to be missing anyway) and sending that along with patches changing both the MFD and CODEC compatible strings and IDs and adding comments saying that this is intentional to ensure that nobody cleans this up again.
On Fri, 17 Jan 2014 17:20:25 +0000, Mark Brown wrote:
it's really common for device vendors to say that their device or code is special in some way and can't do the standard things but this rarely turns out to be true. It is therefore really important to do things like highlight specific technical things that mean new approaches are needed (like the fact that the addresses are independently controllable for the functions on this device) otherwise it is very easy for it to look like a common pattern is being repeated.
Certainly wasn't trying to con anyone here. Do wish I'd brought that feature up a lot earlier though as it would've saved us both time.
What I would suggest doing is writing a binding document for the device (which seems to be missing anyway) and sending that along with patches changing both the MFD and CODEC compatible strings and IDs and adding comments saying that this is intentional to ensure that nobody cleans this up again.
Ok, I'll look into that. I guess it makes sense to write one document for the PMIC and one for the CODEC as well, given their individual nature?
On Tue, Jan 21, 2014 at 12:35:11AM +0000, Opensource [Adam Thomson] wrote:
On Fri, 17 Jan 2014 17:20:25 +0000, Mark Brown wrote:
What I would suggest doing is writing a binding document for the device (which seems to be missing anyway) and sending that along with patches changing both the MFD and CODEC compatible strings and IDs and adding comments saying that this is intentional to ensure that nobody cleans this up again.
Ok, I'll look into that. I guess it makes sense to write one document for the PMIC and one for the CODEC as well, given their individual nature?
I don't really mind either way but if they are split then the documents should reference each other to avoid confusion.
participants (2)
-
Mark Brown -
Opensource [Adam Thomson]