Re: [alsa-devel] [PATCH] ASoC: codecs: da9055: Update driver name to fix breakage due to pmic driver with same name

Re: [alsa-devel] [PATCH] ASoC: codecs: da9055: Update driver name to fix breakage due to pmic driver with same name

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 562 bytes --]

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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [alsa-devel] [PATCH] ASoC: codecs: da9055: Update driver name to fix breakage due to pmic driver with same name

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 673 bytes --]

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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [alsa-devel] [PATCH] ASoC: codecs: da9055: Update driver name to fix breakage due to pmic driver with same name

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1227 bytes --]

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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [alsa-devel] [PATCH] ASoC: codecs: da9055: Update driver name to fix breakage due to pmic driver with same name

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 521 bytes --]

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...

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [alsa-devel] [PATCH] ASoC: codecs: da9055: Update driver name to fix breakage due to pmic driver with same name

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 299 bytes --]

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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [alsa-devel] [PATCH] ASoC: codecs: da9055: Update driver name to fix breakage due to pmic driver with same name

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 602 bytes --]

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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [alsa-devel] [PATCH] ASoC: codecs: da9055: Update driver name to fix breakage due to pmic driver with same name

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1007 bytes --]

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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [alsa-devel] [PATCH] ASoC: codecs: da9055: Update driver name to fix breakage due to pmic driver with same name

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1073 bytes --]

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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [alsa-devel] [PATCH] ASoC: codecs: da9055: Update driver name to fix breakage due to pmic driver with same name

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 2131 bytes --]

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).

> 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.

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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [alsa-devel] [PATCH] ASoC: codecs: da9055: Update driver name to fix breakage due to pmic driver with same name

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 2854 bytes --]

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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [alsa-devel] [PATCH] ASoC: codecs: da9055: Update driver name to fix breakage due to pmic driver with same name

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 964 bytes --]

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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [alsa-devel] [PATCH] ASoC: codecs: da9055: Update driver name to fix breakage due to pmic driver with same name

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 633 bytes --]

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...

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [alsa-devel] [PATCH] ASoC: codecs: da9055: Update driver name to fix breakage due to pmic driver with same name

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 394 bytes --]

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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [alsa-devel] [PATCH] ASoC: codecs: da9055: Update driver name to fix breakage due to pmic driver with same name

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 949 bytes --]

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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]