[alsa-devel] [PATCH] ASoC: codecs: da9055: Update driver name to fix breakage due to pmic driver with same name
This patch updates i2c driver name and device id of da9055 codec driver. DA9055 is a PMIC + CODEC and currently, the corresponding PMIC driver also registers itself with the same name as codec, i.e. "da9055". Because of this the codec driver was broken. Now codec driver uses "da9055-codec" as driver name instead of "da9055".
Signed-off-by: Ashish Chavan ashish.chavan@kpitcummins.com Signed-off-by: David Dajun Chen david.chen@diasemi.com --- sound/soc/codecs/da9055.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/sound/soc/codecs/da9055.c b/sound/soc/codecs/da9055.c index 2e1bf5d..e146f68 100644 --- a/sound/soc/codecs/da9055.c +++ b/sound/soc/codecs/da9055.c @@ -1544,7 +1544,7 @@ static int da9055_remove(struct i2c_client *client) }
static const struct i2c_device_id da9055_i2c_id[] = { - { "da9055", 0 }, + { "da9055-codec", 0 }, { } }; MODULE_DEVICE_TABLE(i2c, da9055_i2c_id); @@ -1552,7 +1552,7 @@ MODULE_DEVICE_TABLE(i2c, da9055_i2c_id); /* I2C codec control layer */ static struct i2c_driver da9055_i2c_driver = { .driver = { - .name = "da9055", + .name = "da9055-codec", .owner = THIS_MODULE, }, .probe = da9055_i2c_probe,
On Fri, Jul 05, 2013 at 05:17:42PM +0530, Ashish Chavan wrote:
This patch updates i2c driver name and device id of da9055 codec driver. DA9055 is a PMIC + CODEC and currently, the corresponding PMIC driver also registers itself with the same name as codec, i.e. "da9055". Because of this the codec driver was broken. Now codec driver uses "da9055-codec" as driver name instead of "da9055".
static const struct i2c_device_id da9055_i2c_id[] = {
- { "da9055", 0 },
- { "da9055-codec", 0 }, { }
};
I can't believe that you've tested this.
On Fri, 2013-07-05 at 12:44 +0100, Mark Brown wrote:
On Fri, Jul 05, 2013 at 05:17:42PM +0530, Ashish Chavan wrote:
This patch updates i2c driver name and device id of da9055 codec driver. DA9055 is a PMIC + CODEC and currently, the corresponding PMIC driver also registers itself with the same name as codec, i.e. "da9055". Because of this the codec driver was broken. Now codec driver uses "da9055-codec" as driver name instead of "da9055".
static const struct i2c_device_id da9055_i2c_id[] = {
- { "da9055", 0 },
- { "da9055-codec", 0 }, { }
};
I can't believe that you've tested this.
Yes, I have tested this today only. But with 3.6.0-rc4 kernel. That is the newest kernel for which I have board and other required support available.
On Fri, Jul 05, 2013 at 07:05:06PM +0530, Ashish Chavan wrote:
On Fri, 2013-07-05 at 12:44 +0100, Mark Brown wrote:
static const struct i2c_device_id da9055_i2c_id[] = {
- { "da9055", 0 },
- { "da9055-codec", 0 }, { }
};
I can't believe that you've tested this.
Yes, I have tested this today only. But with 3.6.0-rc4 kernel. That is the newest kernel for which I have board and other required support available.
Your testing seems like it's not very good then - you do undersand that the problem that this is supposed to be fixing is that this is a multi function device? Please look at how other MFDs in the kernel work.
On Fri, 2013-07-05 at 14:37 +0100, Mark Brown wrote:
On Fri, Jul 05, 2013 at 07:05:06PM +0530, Ashish Chavan wrote:
On Fri, 2013-07-05 at 12:44 +0100, Mark Brown wrote:
static const struct i2c_device_id da9055_i2c_id[] = {
- { "da9055", 0 },
- { "da9055-codec", 0 }, { }
};
I can't believe that you've tested this.
Yes, I have tested this today only. But with 3.6.0-rc4 kernel. That is the newest kernel for which I have board and other required support available.
Your testing seems like it's not very good then - you do undersand that the problem that this is supposed to be fixing is that this is a multi function device? Please look at how other MFDs in the kernel work.
I feel that some info is missing from your view. DA9055 is CODEC + PMIC but with two different I2C addresses. Actually it is a case of two different chips enclosed in a single die. There is NO interconnection between CODEC and PMIC inside DA9055. To me, this seems enough reason to make two drivers independent from each other and not let one part know about the existence of other. Actually in near future, there may be three variants of this chip,
DA9055 - PMIC + CODEC DA9055 - PMIC only DA9055 - CODEC only
In my opinion, keeping the drivers independent will also help in re-using existing drivers "as it is" for any of the above combination.
Let me know if you have a different opinion.
On Mon, Jul 08, 2013 at 01:24:51PM +0530, Ashish Chavan wrote:
On Fri, 2013-07-05 at 14:37 +0100, Mark Brown wrote:
Your testing seems like it's not very good then - you do undersand that the problem that this is supposed to be fixing is that this is a multi function device? Please look at how other MFDs in the kernel work.
I feel that some info is missing from your view. DA9055 is CODEC + PMIC but with two different I2C addresses. Actually it is a case of two different chips enclosed in a single die. There is NO interconnection between CODEC and PMIC inside DA9055. To me, this seems enough reason to make two drivers independent from each other and not let one part know about the existence of other. Actually in near future, there may be three variants of this chip,
This is very similar to things like the TI palmas chips - they have multiple functions on different I2C addresses. The chip still gets instantiated a single time and then the subdevices are instantiated like a MFD by the core device.
In my opinion, keeping the drivers independent will also help in re-using existing drivers "as it is" for any of the above combination.
It shouldn't make a huge difference here.
I feel that some info is missing from your view. DA9055 is CODEC + PMIC but with two different I2C addresses. Actually it is a case of two different chips enclosed in a single die. There is NO interconnection between CODEC and PMIC inside DA9055. To me, this seems enough reason to make two drivers independent from each other and not let one part know about the existence of other. Actually in near future, there may be three variants of this chip,
This is very similar to things like the TI palmas chips - they have multiple functions on different I2C addresses. The chip still gets instantiated a single time and then the subdevices are instantiated like a MFD by the core device.
I have had a look at palmas implementation. It seems to me that a significant change to both PMIC and CODEC drivers is required to make them inline with palmas structure. It will also need a thorough testing cycle for both. I am afraid that we may not have immediate bandwidth to accommodate this level of change. Is it possible that in first go we just fix the breakage due to name collision so that both drivers remain usable. And in second iteration, we restructure both of them as you suggested?
On Thu, Jul 11, 2013 at 05:06:16PM +0530, Ashish Chavan wrote:
accommodate this level of change. Is it possible that in first go we just fix the breakage due to name collision so that both drivers remain usable. And in second iteration, we restructure both of them as you suggested?
To be honest I'm really not terribly happy about this - I don't have all that much confidence that the second step will happen, the fact that this has been broken for a considerable time isn't inspiring here. However I guess...
On Wed, 2013-07-17 at 11:36 +0100, Mark Brown wrote:
On Thu, Jul 11, 2013 at 05:06:16PM +0530, Ashish Chavan wrote:
accommodate this level of change. Is it possible that in first go we just fix the breakage due to name collision so that both drivers remain usable. And in second iteration, we restructure both of them as you suggested?
To be honest I'm really not terribly happy about this - I don't have all that much confidence that the second step will happen, the fact that this has been broken for a considerable time isn't inspiring here. However I guess...
Thanks for sharing your view. This makes me believe that second step is very important and urgent. Keeping this in mind, we have already started working on it and hope to submit updated drivers in next 5-6 weeks. In the meanwhile you may apply quick fix (one which I already posted), if you find it worth.
This message contains information that may be privileged or confidential and is the property of the KPIT Cummins Infosystems Ltd. It is intended only for the person to whom it is addressed. If you are not the intended recipient, you are not authorized to read, print, retain copy, disseminate, distribute, or use this message or any part thereof. If you receive this message in error, please notify the sender immediately and delete all copies of this message. KPIT Cummins Infosystems Ltd. does not accept any liability for virus infected mails.
On Mon, Jul 22, 2013 at 02:13:14PM +0530, Ashish Chavan wrote:
working on it and hope to submit updated drivers in next 5-6 weeks. In the meanwhile you may apply quick fix (one which I already posted), if you find it worth.
You would need to resend, as I indicated orginally I discarded it.
On Mon, 2013-07-22 at 11:02 +0100, Mark Brown wrote:
On Mon, Jul 22, 2013 at 02:13:14PM +0530, Ashish Chavan wrote:
working on it and hope to submit updated drivers in next 5-6 weeks. In the meanwhile you may apply quick fix (one which I already posted), if you find it worth.
You would need to resend, as I indicated orginally I discarded it.
We had started working on restructuring of both PMIC and CODEC drivers. But soon after starting, we realized that the kind of abstraction that we were heading towards will not be logically correct and good from maintainability point of view. Basically tying up two COMPLETELY INDEPENDENT (they really don't share anything other than the name!) drivers via a virtual MFD entity will not make much sense in our opinion. Also as mentioned earlier, the codec design(chip) will be produced as a standalone codec device soon, and we hope the driver can be re-used directly without changes. Is this the reason why we see only a handful examples of suggested implementation in kernel?
This message contains information that may be privileged or confidential and is the property of the KPIT Cummins Infosystems Ltd. It is intended only for the person to whom it is addressed. If you are not the intended recipient, you are not authorized to read, print, retain copy, disseminate, distribute, or use this message or any part thereof. If you receive this message in error, please notify the sender immediately and delete all copies of this message. KPIT Cummins Infosystems Ltd. does not accept any liability for virus infected mails.
On Mon, Jul 29, 2013 at 08:36:26PM +0530, Ashish Chavan wrote:
produced as a standalone codec device soon, and we hope the driver can be re-used directly without changes. Is this the reason why we see only a handful examples of suggested implementation in kernel?
Well, it's a very unusual hardware design choice to have multiple I2C endpoints in a single physical chip.
With regmap it should be very straightforward to reuse the same driver for both standalone and non-standalone versions, just a small amount of glue code in the CODEC driver I'd expect. Usually the bus level code is tiny.
On Mon, 2013-07-29 at 17:01 +0100, Mark Brown wrote:
On Mon, Jul 29, 2013 at 08:36:26PM +0530, Ashish Chavan wrote:
produced as a standalone codec device soon, and we hope the driver can be re-used directly without changes. Is this the reason why we see only a handful examples of suggested implementation in kernel?
Well, it's a very unusual hardware design choice to have multiple I2C endpoints in a single physical chip.
I hope to see more of such devices in near future.
With regmap it should be very straightforward to reuse the same driver for both standalone and non-standalone versions, just a small amount of glue code in the CODEC driver I'd expect. Usually the bus level code is tiny.
The glue code that you are talking about is for the same virtual MFD component that you proposed initially, right? I mean the glue code in CODEC will help it to get attached to the MFD. In this case, in addition to the glue code inside CODEC we will also need additional MFD component. Or I am completely misinterpreting you here?
This message contains information that may be privileged or confidential and is the property of the KPIT Cummins Infosystems Ltd. It is intended only for the person to whom it is addressed. If you are not the intended recipient, you are not authorized to read, print, retain copy, disseminate, distribute, or use this message or any part thereof. If you receive this message in error, please notify the sender immediately and delete all copies of this message. KPIT Cummins Infosystems Ltd. does not accept any liability for virus infected mails.
On Mon, Aug 05, 2013 at 01:25:31PM +0530, Ashish Chavan wrote:
On Mon, 2013-07-29 at 17:01 +0100, Mark Brown wrote:
Well, it's a very unusual hardware design choice to have multiple I2C endpoints in a single physical chip.
I hope to see more of such devices in near future.
There's probably a reason why it's not a common hardware design...
With regmap it should be very straightforward to reuse the same driver for both standalone and non-standalone versions, just a small amount of glue code in the CODEC driver I'd expect. Usually the bus level code is tiny.
The glue code that you are talking about is for the same virtual MFD component that you proposed initially, right? I mean the glue code in CODEC will help it to get attached to the MFD. In this case, in addition to the glue code inside CODEC we will also need additional MFD component. Or I am completely misinterpreting you here?
No, I'm talking about the same thing I was talking about originally.
On Mon, 2013-08-05 at 15:42 +0100, Mark Brown wrote:
On Mon, Aug 05, 2013 at 01:25:31PM +0530, Ashish Chavan wrote:
On Mon, 2013-07-29 at 17:01 +0100, Mark Brown wrote:
Well, it's a very unusual hardware design choice to have multiple I2C endpoints in a single physical chip.
I hope to see more of such devices in near future.
There's probably a reason why it's not a common hardware design...
With regmap it should be very straightforward to reuse the same driver for both standalone and non-standalone versions, just a small amount of glue code in the CODEC driver I'd expect. Usually the bus level code is tiny.
The glue code that you are talking about is for the same virtual MFD component that you proposed initially, right? I mean the glue code in CODEC will help it to get attached to the MFD. In this case, in addition to the glue code inside CODEC we will also need additional MFD component. Or I am completely misinterpreting you here?
No, I'm talking about the same thing I was talking about originally.
Thanks for confirming it. From our view point, we still feel that it's not a good design which requires an additional MFD component even to support a stand alone CODEC chip. The way we look at it is, there are so many stand alone CODEC drivers in kernel and most of them are fine without the MFD stub. We wish that our DA9055 CODEC driver should also be treated in the same way. Just placing it in a different hardware package (together with PMIC, in this case) shouldn't necessitate any changes in software. e.g. whether any chip is produced as a BGA component or through hole component, has no effect on it's software.
If you still feel that having additional MFD component is THE correct way to move forward, then I would like to propose another way which seems more logical to us. i.e. changing name of the CODEC driver. We will rename the codec to "da9055c" or something similar to resolve the name collision with PMIC. BTW this is not our preferred way and should be considered as last option.
This message contains information that may be privileged or confidential and is the property of the KPIT Cummins Infosystems Ltd. It is intended only for the person to whom it is addressed. If you are not the intended recipient, you are not authorized to read, print, retain copy, disseminate, distribute, or use this message or any part thereof. If you receive this message in error, please notify the sender immediately and delete all copies of this message. KPIT Cummins Infosystems Ltd. does not accept any liability for virus infected mails.
On Mon, Aug 05, 2013 at 09:21:37PM +0530, Ashish Chavan wrote:
On Mon, 2013-08-05 at 15:42 +0100, Mark Brown wrote:
No, I'm talking about the same thing I was talking about originally.
Thanks for confirming it. From our view point, we still feel that it's not a good design which requires an additional MFD component even to support a stand alone CODEC chip. The way we look at it is, there are so
What makes you say that a MFD is required for a standalone CODEC?
many stand alone CODEC drivers in kernel and most of them are fine without the MFD stub. We wish that our DA9055 CODEC driver should also be treated in the same way. Just placing it in a different hardware package (together with PMIC, in this case) shouldn't necessitate any changes in software. e.g. whether any chip is produced as a BGA component or through hole component, has no effect on it's software.
You only need to write the glue once, it'd probably take you less time than writing these e-mails... Once you've handed the regmap to the ASoC core the code is identical.
On Mon, 2013-08-05 at 17:23 +0100, Mark Brown wrote:
On Mon, Aug 05, 2013 at 09:21:37PM +0530, Ashish Chavan wrote:
many stand alone CODEC drivers in kernel and most of them are fine without the MFD stub. We wish that our DA9055 CODEC driver should also be treated in the same way. Just placing it in a different hardware package (together with PMIC, in this case) shouldn't necessitate any changes in software. e.g. whether any chip is produced as a BGA component or through hole component, has no effect on it's software.
You only need to write the glue once, it'd probably take you less time than writing these e-mails... Once you've handed the regmap to the ASoC core the code is identical.
Apologies for my delayed response. Unfortunately I need to come out of this activity abruptly. One of my colleague, Adam Thomson will take this thread forward and will help in doing all required updates to the driver. He will also serve as future maintainer for DA9055 codec driver.
Thanks for all your support till date.
This message contains information that may be privileged or confidential and is the property of the KPIT Cummins Infosystems Ltd. It is intended only for the person to whom it is addressed. If you are not the intended recipient, you are not authorized to read, print, retain copy, disseminate, distribute, or use this message or any part thereof. If you receive this message in error, please notify the sender immediately and delete all copies of this message. KPIT Cummins Infosystems Ltd. does not accept any liability for virus infected mails.
On Thu, Aug 29, 2013 at 13:08, Ashish Chavan wrote:
Apologies for my delayed response. Unfortunately I need to come out of this activity abruptly. One of my colleague, Adam Thomson will take this thread forward and will help in doing all required updates to the driver. He will also serve as future maintainer for DA9055 codec driver.
Thanks for all your support till date.
Hi Mark,
As Ashish mentioned, I'm now taking responsibility for this. Would be good to get a quick resolution as this has been dragging along for some time now.
At present I believe your suggestion is to instantiate the codec regmap in the MFD core for the PMIC, and then pass this in as part initialisation of the codec driver, from the PMIC. Please correct me if I'm wrong.
If I'm correct then to me this doesn't make sense. The devices are separate, and have completely independent register maps. As such I believe the instantiation and ownership of the Codec register map should be in the Codec driver itself and not performed outside by other code. If you were to use the Codec driver as a standalone device, using the above approach, you would need to write some additional code to create the regmap structure before you could use the driver. Seems logically wrong to me and provides an unnecessary step for anyone wanting to use the driver.
I currently see two options on how to proceed with this:
1) Simply modify the Codec driver to use a different I2C id string and leave the rest of the functionality as is (as was previously suggested, and my preferred option).
2) Including the above change, add some optional code to the PMIC MFD core which uses 'i2c_new_device()' to instantiate the codec, if it's required (I guess indicated by platform data to the PMIC). Means the Codec can still be used as is, but the PMIC core code can, if required, instantiate the codec.
I personally believe option 2 seems unnecessary and it would be simple enough just to instantiate the codec driver from machine code, as is done for many standalone codecs. Am interested though in understanding the reasoning behind your suggestion, for devices like this which are completely independent but can share the same HW package. I currently don't see a good reason to make PMIC MFD core instantiate the codec, for this type of scenario, but maybe you see something I don't?
On Mon, Sep 02, 2013 at 09:49:20AM +0000, Opensource [Adam Thomson] wrote:
Please fix your mailer to wrap within 80 columns, it makes your mails very hard to read if you don't do this.
At present I believe your suggestion is to instantiate the codec regmap in the MFD core for the PMIC, and then pass this in as part initialisation of the codec driver, from the PMIC. Please correct me if I'm wrong.
That's correct.
If I'm correct then to me this doesn't make sense. The devices are separate, and have completely independent register maps. As such I believe the instantiation
They are not separate, they are soldered to the board as part of the same package - quite a few other devices use a similar scheme and are also handled in this fashion (the TI TWL devices are one example).
- Including the above change, add some optional code to the PMIC MFD core which
uses 'i2c_new_device()' to instantiate the codec, if it's required (I guess indicated by platform data to the PMIC). Means the Codec can still be used as is, but the PMIC core code can, if required, instantiate the codec.
This is roughly what ends up happening, you do need to instantiate another I2C client no matter what. The important thing here is that the CODEC does not need to be separately registered by the user, if it really is only the I2C client that needs creating that's probably OK so long as the user doesn't need to worry about that implementation detail.
I personally believe option 2 seems unnecessary and it would be simple enough just to instantiate the codec driver from machine code, as is done for many standalone codecs. Am interested though in understanding the reasoning behind your suggestion, for devices like this which are completely independent but can share the same HW package. I currently don't see a good reason to make PMIC MFD core instantiate the codec, for this type of scenario, but maybe you see something I don't?
The reasoning is simply that if the chip design solders a single device to the board then the software system integration should register a single device with the system.
On Mon, Sep 02, 2013 at 11:39, Mark Brown wrote:
Please fix your mailer to wrap within 80 columns, it makes your mails very hard to read if you don't do this.
Yeah, sorry for that. Having to use Outlook and of course it doesn't seem to have that feature (at least not that I could find), so trying to do it manually.
They are not separate, they are soldered to the board as part of the same package - quite a few other devices use a similar scheme and are also handled in this fashion (the TI TWL devices are one example).
The difference here is that our combined devices can also be separate chips, not just one HW package containing logically separate devices. I don't believe that's the case with say the TI devices.
This is roughly what ends up happening, you do need to instantiate another I2C client no matter what. The important thing here is that the CODEC does not need to be separately registered by the user, if it really is only the I2C client that needs creating that's probably OK so long as the user doesn't need to worry about that implementation detail.
The only thing that would need populating is some small platform data for the codec (MIC bias voltages, and such). You'd still have to do this, combined or separate, as this is platform specific. Other than this, the I2C client initialisation for the codec is simple, which is a reason why I don't think the PMIC needs to initialise it, and you can just as simply do it from machine code.
The reasoning is simply that if the chip design solders a single device to the board then the software system integration should register a single device with the system.
Ok, but what about the scenario where the devices start life as separate chips and are then later also packaged together as one chip but still with no internal connection, like DA9055. The drivers were already written and accepted as separate entities in the kernel, without chained initialisation. What would be the approach there? To me, logically it makes sense to leave them separate.
On Mon, Sep 02, 2013 at 03:38:18PM +0000, Opensource [Adam Thomson] wrote:
On Mon, Sep 02, 2013 at 11:39, Mark Brown wrote:
Please fix your mailer to wrap within 80 columns, it makes your mails very hard to read if you don't do this.
Yeah, sorry for that. Having to use Outlook and of course it doesn't seem to have that feature (at least not that I could find), so trying to do it manually.
It's always been in there, can't remember where exactly and I don't have access to Outlook any more.
They are not separate, they are soldered to the board as part of the same package - quite a few other devices use a similar scheme and are also handled in this fashion (the TI TWL devices are one example).
The difference here is that our combined devices can also be separate chips, not just one HW package containing logically separate devices. I don't believe that's the case with say the TI devices.
That doesn't seem like a unique feature.
This is roughly what ends up happening, you do need to instantiate another I2C client no matter what. The important thing here is that the CODEC does not need to be separately registered by the user, if it really is only the I2C client that needs creating that's probably OK so long as the user doesn't need to worry about that implementation detail.
The only thing that would need populating is some small platform data for the codec (MIC bias voltages, and such). You'd still have to do this, combined or separate, as this is platform specific. Other than this, the I2C client initialisation for the codec is simple, which is a reason why I don't think the PMIC needs to initialise it, and you can just as simply do it from machine code.
It's the bit where the board has to register the CODEC separately at all that's the thing. Think about it from the point of view of people writing and reviewing the machine bindings - they end up with this odd chip that appears twice with two names and registration schemas.
The reasoning is simply that if the chip design solders a single device to the board then the software system integration should register a single device with the system.
Ok, but what about the scenario where the devices start life as separate chips and are then later also packaged together as one chip but still with no internal connection, like DA9055. The drivers were already written and accepted as separate entities in the kernel, without chained initialisation. What would be the approach there? To me, logically it makes sense to leave them separate.
It doesn't seem to make much difference what order the drivers are added in here? You're going to need to add a new device IDs for the SIP anyway since it'd presumably be badged as something new - if it wasn't then that's a bit different.
On Mon, Sep 02, 2013 at 18:14, Mark Brown wrote:
It's always been in there, can't remember where exactly and I don't have access to Outlook any more.
Wish I was that lucky :-) There is an option for word wrap at a certain number of characters but that just seems to do nothing, at least for plain text e-mails like this. Will have to find another solution when I have some spare time.
The difference here is that our combined devices can also be separate chips, not just one HW package containing logically separate devices. I don't believe that's the case with say the TI devices.
That doesn't seem like a unique feature.
It may not be unique. To be honest I don't know. However, in terms of existing drivers in the kernel for MFD and Codec, I don't believe there's an example of something exactly like this in place already.
It's the bit where the board has to register the CODEC separately at all that's the thing. Think about it from the point of view of people writing and reviewing the machine bindings - they end up with this odd chip that appears twice with two names and registration schemas.
Personally I don't see the issue in having two I2C clients defined for the one chip, in the machine bindings. Logically it's absolutely correct for the chip, and as long as the I2C IDs for the devices make sense (which wasn't the case for DA9055, and is something I want to rectify), then it should be obvious to anyone who comes across that code. Actually having them as two separate entries in machine code helps to highlight that they are individual I2C devices in one package rather than somehow linked internally and requiring ordered initialisation. That seems right to me, and would tally with the associated datasheet for the device.
Ok, but what about the scenario where the devices start life as separate chips and are then later also packaged together as one chip but still with no internal connection, like DA9055. The drivers were already written and accepted as separate entities in the kernel, without chained initialisation. What would be the approach there? To me, logically it makes sense to leave them separate.
It doesn't seem to make much difference what order the drivers are added in here? You're going to need to add a new device IDs for the SIP anyway since it'd presumably be badged as something new - if it wasn't then that's a bit different.
Yes you would add new device IDs to the drivers in question, to align with a new device number for the combined chip, but I wouldn't expect anything more than that as the functionality remains identical. It would simply be to document which devices each driver supports.
On Wed, Sep 04, 2013 at 04:13:25PM +0000, Opensource [Adam Thomson] wrote:
Personally I don't see the issue in having two I2C clients defined for the one chip, in the machine bindings. Logically it's absolutely correct for the chip, and as long as the I2C IDs for the devices make sense (which wasn't the case for DA9055, and is something I want to rectify), then it should be obvious to anyone who comes across that code. Actually having them as two separate entries in machine code helps to highlight that they are individual I2C devices in one package rather than somehow linked internally and requiring ordered initialisation. That seems right to me, and would tally with the associated datasheet for the device.
The goal is that users shouldn't even need to have to think about how the chip is constructed, they should just be able to use it with minimal thought or effort. Anything that can be encapsulated within the drivers should be.
On Wed, Sep 04, 2013 at 19:34, Mark Brown wrote:
Personally I don't see the issue in having two I2C clients defined for the one chip, in the machine bindings. Logically it's absolutely correct for the chip, and as long as the I2C IDs for the devices make sense (which wasn't the case for DA9055, and is something I want to rectify), then it should be obvious to anyone who comes across that code. Actually having them as two separate entries in machine code helps to highlight that they are individual I2C devices in one package rather than somehow linked internally and requiring ordered initialisation. That seems right to me, and would tally with the associated datasheet for the device.
The goal is that users shouldn't even need to have to think about how the chip is constructed, they should just be able to use it with minimal thought or effort. Anything that can be encapsulated within the drivers should be.
I agree with your ideas and it's something that should be enforced, but only if it makes sense to the scenario. Taking away flexibility, functionality and logical correctness just to make things 'easier' seems a bit wrong. Also, there are many chip drivers which 'encapsulate' as you say, but they're not necessarily effortless to work with. I have spotted examples which require chunks of platform data to configure the device correctly, thus needing at least some understanding of the chip itself. But I wouldn't say that's wrong because it's needed to allow for complete usage of the device in question.
Bringing this back to DA9055 though, the current method of initialisation, for PMIC and Codec, isn't complicated or confusing nor does it require a lot of effort. As well as this the implementation is logically sound. It should remain that way in my opinion. The only thing that should be updated is the I2C Id of the Codec to make it more meaningful, and preferably sooner rather than later so the issue can be finally put to bed.
On Fri, Sep 06, 2013 at 02:17:48PM +0000, Opensource [Adam Thomson] wrote:
Bringing this back to DA9055 though, the current method of initialisation, for PMIC and Codec, isn't complicated or confusing nor does it require a lot of effort. As well as this the implementation is logically sound. It should remain that way in my opinion. The only thing that should be updated is the I2C Id of the Codec to make it more meaningful, and preferably sooner rather than later so the issue can be finally put to bed.
Well, the obvious approach would seem to be to just do the registration of the CODEC I2C device from the MFD...
On Mon, Sep 09, 2013 at 12:27, Mark Brown wrote:
Bringing this back to DA9055 though, the current method of initialisation, for PMIC and Codec, isn't complicated or confusing nor does it require a lot of effort. As well as this the implementation is logically sound. It should remain that way in my opinion. The only thing that should be updated is the I2C Id of the Codec to make it more meaningful, and preferably sooner rather than later so the issue can be finally put to bed.
Well, the obvious approach would seem to be to just do the registration of the CODEC I2C device from the MFD...
Had we felt that was the obvious approach then we'd have taken it, as it's much easier to 'fall in line'. However I believe the approach that we adopted was the obvious and correct one for our chip. It is just a shame that you don't see things the same way.
As it stands we have fully functional drivers integrated in the kernel, which are tested and proven. What remains as far as I'm concerned is a simple ID name change to make things cleaner, but you do not agree with this approach. Your suggestion is to move initialisation of the Codec to the MFD core of the PMIC driver which to me is unnecessary and limiting and is something I cannot agree with. Basically we're at a bit of an impasse here...
On Tue, Sep 10, 2013 at 01:05:54PM +0000, Opensource [Adam Thomson] wrote:
Your suggestion is to move initialisation of the Codec to the MFD core of the PMIC driver which to me is unnecessary and limiting and is something I cannot agree with. Basically we're at a bit of an impasse here...
What limitations do you see? I don't see any substantial issues and you haven't mentioned any.
On Tue, Sep 10, 2013 at 08:07, Mark Brown wrote:
Your suggestion is to move initialisation of the Codec to the MFD core of the PMIC driver which to me is unnecessary and limiting and is something I cannot agree with. Basically we're at a bit of an impasse here...
What limitations do you see? I don't see any substantial issues and you haven't mentioned any.
It's limiting in as much as it's insisting on a required order for initialisation which shouldn't be there. As said previously they're 2 separate devices in one package, with no internal connection, so either could be instantiated first. It should be open to the user to decide on this based on their platform and needs.
With your approach, it is more work for no gain here, and holds us to a logical representation which doesn't fit with the device in question (which is not really an MFD, it's two devices, one of which is an MFD, the PMIC).
On Thu, Sep 12, 2013 at 05:11:06PM +0000, Opensource [Adam Thomson] wrote:
It's limiting in as much as it's insisting on a required order for initialisation which shouldn't be there. As said previously they're 2 separate devices in one package, with no internal connection, so either could be instantiated first. It should be open to the user to decide on this based on their platform and needs.
With your approach, it is more work for no gain here, and holds us to a logical representation which doesn't fit with the device in question (which is not really an MFD, it's two devices, one of which is an MFD, the PMIC).
I'm having a hard time understanding this as a practical limitation, can you be more specific about the cases where this would present a noticable problem? It'd at least ensure that the configuration where the whole device is present gets tested to some extent, though that doesn't seem likely to break again.
On Thu, Sep 12, 2013 at 22:58, Mark Brown wrote:
It's limiting in as much as it's insisting on a required order for initialisation which shouldn't be there. As said previously they're 2 separate devices in one package, with no internal connection, so either could be instantiated first. It should be open to the user to decide on this based on their platform and needs.
With your approach, it is more work for no gain here, and holds us to a logical representation which doesn't fit with the device in question (which is not really an MFD, it's two devices, one of which is an MFD, the PMIC).
I'm having a hard time understanding this as a practical limitation, can you be more specific about the cases where this would present a noticable problem? It'd at least ensure that the configuration where the whole device is present gets tested to some extent, though that doesn't seem likely to break again.
If I'm honest I didn't have anything specific in mind here. There is the possibility that the Codec needn't be powered from the packaged PMIC (unlikely but technically possible in HW), in the same way as if it were a standalone chip, so I didn't want to unnecessarily imply or force any kind of link or required ordering in Software, to leave it flexible for the future should some need arise.
I understand your concern over testing, given the issue that was found with these drivers, and I can only apologise that it wasn't caught. This is not something that will occur again as we certainly don't want a repeat scenario. It should be noted though that the drivers were thoroughly tested individually for each device and proven fully functional, and I maintain the approach taken for driver development was the correct one here. It will make it easier in the future with standalone versions, and where these devices themselves are paired with a different devices, in the same manor.
participants (3)
-
Ashish Chavan -
Mark Brown -
Opensource [Adam Thomson]