Re: [PATCH 3/3] ARM: dts: Enable audio support for Peach-pi board

Re: [PATCH 3/3] ARM: dts: Enable audio support for Peach-pi board

[Doug Anderson]

Mark,

On Fri, Jun 13, 2014 at 10:05 AM, Mark Brown <broonie@kernel.org> wrote:
> On Fri, Jun 13, 2014 at 10:03:50AM -0700, Doug Anderson wrote:
>> On Tue, Jun 10, 2014 at 10:32 PM, Tushar Behera <tushar.b@samsung.com> wrote:
>
>> > Peach-pi board has MAX98090 audio codec connected on HSI2C-7 bus.
>
>> If you want to be a stickler about it, peach-pi actually has a
>> max98091.  That requires code changes to the i2c driver, though.
>> ...and unfortunately listing two compatible strings for i2c devices is
>> broken.  :(
>
> It is?  We should fix that if it's the case...

Yah, I mentioned it to Mark Rutland at the last ELC and he said he
might take a look at it, but I probably should have posted something
up to the i2c list.

I made a half-assed attempt to fix it locally in the ChromeOS but
quickly found that it was going to be a much bigger job than I had
time for...

https://chromium-review.googlesource.com/#/c/184406/

IIRC i2c_new_device didn't return an error like I thought it would,
probably trying to deal with the fact that devices might show up at a
later point in time.


Hrm, now that I think about it I wonder if the right answer is just to
call i2c_new_device for all the compatible strings even if it doesn't
return an error.  I'd have to go back and try that and re-explore this
code...

-Doug

Re: [PATCH 3/3] ARM: dts: Enable audio support for Peach-pi board

[Doug Anderson]

Mark,

On Fri, Jun 13, 2014 at 10:13 AM, Doug Anderson <dianders-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> wrote:
> Mark,
>
> On Fri, Jun 13, 2014 at 10:05 AM, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
>> On Fri, Jun 13, 2014 at 10:03:50AM -0700, Doug Anderson wrote:
>>> On Tue, Jun 10, 2014 at 10:32 PM, Tushar Behera <tushar.b-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org> wrote:
>>
>>> > Peach-pi board has MAX98090 audio codec connected on HSI2C-7 bus.
>>
>>> If you want to be a stickler about it, peach-pi actually has a
>>> max98091.  That requires code changes to the i2c driver, though.
>>> ...and unfortunately listing two compatible strings for i2c devices is
>>> broken.  :(
>>
>> It is?  We should fix that if it's the case...
>
> Yah, I mentioned it to Mark Rutland at the last ELC and he said he
> might take a look at it, but I probably should have posted something
> up to the i2c list.
>
> I made a half-assed attempt to fix it locally in the ChromeOS but
> quickly found that it was going to be a much bigger job than I had
> time for...
>
> https://chromium-review.googlesource.com/#/c/184406/
>
> IIRC i2c_new_device didn't return an error like I thought it would,
> probably trying to deal with the fact that devices might show up at a
> later point in time.
>
>
> Hrm, now that I think about it I wonder if the right answer is just to
> call i2c_new_device for all the compatible strings even if it doesn't
> return an error.  I'd have to go back and try that and re-explore this
> code...

Nope, that didn't work either.  Now I remember trying that before,
too.  It doesn't like you registering two different devices with the
same address:

[    2.582539] DOUG: /i2c@12CD0000/codec@10 (0) max98091
[    2.587360] DOUG: /i2c@12CD0000/codec@10 (0) max98091
[    2.591160] DOUG: /i2c@12CD0000/codec@10 (1) max98090
[    2.596686] i2c i2c-7: Failed to register i2c client max98090 at 0x10 (-16)

If you hack out the check for address business:

sysfs: cannot create duplicate filename '/devices/12cd0000.i2c/i2c-7/7-0010'

Anyway, suffice to say that the i2c core needs to be extended to
handle the idea that a single device has more than one "compatible"
string.  I'll leave it to an eager reader of this thread to implement
this since we can also fix our own problem by just listing "max98091"
in "sound/soc/codecs/max98090.c" like has always been done in the
past.


-Doug

Re: [PATCH 3/3] ARM: dts: Enable audio support for Peach-pi board

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 585 bytes --]

On Fri, Jun 13, 2014 at 02:58:26PM -0700, Doug Anderson wrote:

> Anyway, suffice to say that the i2c core needs to be extended to
> handle the idea that a single device has more than one "compatible"
> string.  I'll leave it to an eager reader of this thread to implement
> this since we can also fix our own problem by just listing "max98091"
> in "sound/soc/codecs/max98090.c" like has always been done in the
> past.

Why do you need to register multiple compatible strings (I guess for
fallback purposes?).  A quick fix that is about as good is to take the
first compatible only.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/3] ARM: dts: Enable audio support for Peach-pi board

[Doug Anderson]

Mark,

On Fri, Jun 13, 2014 at 3:04 PM, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> On Fri, Jun 13, 2014 at 02:58:26PM -0700, Doug Anderson wrote:
>
>> Anyway, suffice to say that the i2c core needs to be extended to
>> handle the idea that a single device has more than one "compatible"
>> string.  I'll leave it to an eager reader of this thread to implement
>> this since we can also fix our own problem by just listing "max98091"
>> in "sound/soc/codecs/max98090.c" like has always been done in the
>> past.
>
> Why do you need to register multiple compatible strings (I guess for
> fallback purposes?).

I'm no expert, but I think that's part of device tree isn't it?

In the case of max98090 and max98091, they are incredibly similar
pieces of hardware (I think the max98091 simply has more microphones).
If you've got a driver for a max98090 it will work just fine for a
max98091 but you just won't get the extra microphones.

In cases like this then device tree theory says that you should list
both compatible strings: max98091 and max98090, right?  If your OS has
a driver for max98091 it will use it.  ...if it doesn't but it has a
max98090 driver it will try that one.

As far as I understand we _shouldn't_ lie and just say that we have a
max98090 when we really have a max98091.  The device tree is supposed
to describe the hardware and isn't support to care that the OS has a
driver for max98090 but not max98091.

Ironically in our case we have a driver that supports both the 98090
and the 98091 via autodetect.  However, it doesn't know about the
98091 compatible string so if you list yourself as compatible with
98091 then it won't find the driver.

> A quick fix that is about as good is to take the
> first compatible only.

That's how the code works today, actually.  ...but as per above the
current 98090 driver doesn't know about the 98091 compatible string,
so:

compatible = "maxim,max98091", "maxim,max98090";

...won't find the right driver.

--

The quick fix is to add max98091 to the max98090 driver and is what
I'd suggest in this case.  ...but I still think that the above logic
is valid and eventually the i2c core should be fixed.  Please correct
me if I'm wrong.

-Doug