| Message ID | 1402464739-19044-4-git-send-email-tushar.b@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Tushar, 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. :( > Signed-off-by: Tushar Behera <tushar.b@samsung.com> > --- > arch/arm/boot/dts/exynos5800-peach-pi.dts | 31 +++++++++++++++++++++++++++++ > 1 file changed, 31 insertions(+) > > diff --git a/arch/arm/boot/dts/exynos5800-peach-pi.dts b/arch/arm/boot/dts/exynos5800-peach-pi.dts > index f3af207..76f5966 100644 > --- a/arch/arm/boot/dts/exynos5800-peach-pi.dts > +++ b/arch/arm/boot/dts/exynos5800-peach-pi.dts > @@ -78,9 +78,27 @@ > pinctrl-0 = <&usb301_vbus_en>; > enable-active-high; > }; > + > + sound { > + compatible = "google,snow-audio-max98090"; > + > + samsung,i2s-controller = <&i2s0>; > + samsung,audio-codec = <&max98090>; > + }; > +}; > + > +&i2s0 { > + status = "okay"; It would be awfully nice to keep diffs between exynos5420-peach-pit and exynos5800-peach-pi clean. They're 99% the same. I know this has already gotten messed up with DP/HDMI were added, but there's no need to make it worse. Could you add these nodes in the same place within the dts they were added in exynos5420-peach-pit?
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...
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
Mark, On Fri, Jun 13, 2014 at 10:13 AM, Doug Anderson <dianders@google.com> wrote: > 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... 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
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.
Mark, On Fri, Jun 13, 2014 at 3:04 PM, Mark Brown <broonie@kernel.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
On 06/13/2014 10:33 PM, Doug Anderson wrote: > Tushar, > > 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. :( > Hi Doug, You are right. I checked the boot logs, the detected codec type is MAX98091. Since both these CODECs are supported through a single driver and the detection of chip is done during runtime, I would suggest we go ahead with "max98090" compatible string. I will update the commit message accordingly. Does that sound okay to you? > >> Signed-off-by: Tushar Behera <tushar.b@samsung.com> >> --- >> arch/arm/boot/dts/exynos5800-peach-pi.dts | 31 +++++++++++++++++++++++++++++ >> 1 file changed, 31 insertions(+) >> >> diff --git a/arch/arm/boot/dts/exynos5800-peach-pi.dts b/arch/arm/boot/dts/exynos5800-peach-pi.dts >> index f3af207..76f5966 100644 >> --- a/arch/arm/boot/dts/exynos5800-peach-pi.dts >> +++ b/arch/arm/boot/dts/exynos5800-peach-pi.dts >> @@ -78,9 +78,27 @@ >> pinctrl-0 = <&usb301_vbus_en>; >> enable-active-high; >> }; >> + >> + sound { >> + compatible = "google,snow-audio-max98090"; >> + >> + samsung,i2s-controller = <&i2s0>; >> + samsung,audio-codec = <&max98090>; >> + }; >> +}; >> + >> +&i2s0 { >> + status = "okay"; > > It would be awfully nice to keep diffs between exynos5420-peach-pit > and exynos5800-peach-pi clean. They're 99% the same. I know this has > already gotten messed up with DP/HDMI were added, but there's no need > to make it worse. > If you so desire, I will submit a patch to sort peach-pi device-tree nodes (w.r.t. peach-pit dts file). > Could you add these nodes in the same place within the dts they were > added in exynos5420-peach-pit? > Okay, I will add it after watchdog node.
Tushar, On Mon, Jun 16, 2014 at 4:19 AM, Tushar Behera <trblinux@gmail.com> wrote: > On 06/13/2014 10:33 PM, Doug Anderson wrote: >> Tushar, >> >> 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. :( >> > Hi Doug, > > You are right. I checked the boot logs, the detected codec type is > MAX98091. Since both these CODECs are supported through a single driver > and the detection of chip is done during runtime, I would suggest we go > ahead with "max98090" compatible string. I will update the commit > message accordingly. > > Does that sound okay to you? As per my understanding you shouldn't do this. You should have two patches: 1. Add "max98091". You could simply post Wonjoon's patch from <https://chromium-review.googlesource.com/184091> 2. Change the device tree to refer to "max98091" The argument that the "current kernel driver has a single driver" is an argument that you're not supposed to make for device tree. The same device tree is supposed to work for U-Boot, BSD, or any other platform. On those platforms it might not be a shared driver. > If you so desire, I will submit a patch to sort peach-pi device-tree > nodes (w.r.t. peach-pit dts file). Yes please. I think there's supposed to be some official ordering of things. If anyone reading this has a pointer to the official sort order of things in the device tree I'd love to see it! ;) -Doug
On Mon, Jun 16, 2014 at 09:49:26AM -0700, Doug Anderson wrote: > Yes please. I think there's supposed to be some official ordering of > things. If anyone reading this has a pointer to the official sort > order of things in the device tree I'd love to see it! ;) Most exact first I believe?
Mark, On Mon, Jun 16, 2014 at 9:51 AM, Mark Brown <broonie@kernel.org> wrote: > On Mon, Jun 16, 2014 at 09:49:26AM -0700, Doug Anderson wrote: > >> Yes please. I think there's supposed to be some official ordering of >> things. If anyone reading this has a pointer to the official sort >> order of things in the device tree I'd love to see it! ;) > > Most exact first I believe? More specifically I'm looking for the ordering between nodes and between properties in a node. For instance: 1. It appears to be convention to sort children of the "pinctrl" nodes by the first pin number in that group. That is: ec_spi_cs: ec-spi-cs { samsung,pins = "gpb1-2"; ... }; ...comes before: usb300_vbus_en: usb300-vbus-en { samsung,pins = "gph0-0"; ... }; ...that's one really good and well-defined ordering. 2. I have no idea how general properties should be sorted. I tend to see "compatible" first but that's above the only rule I've seen. Sometimes I've seen "status" first, sometimes last, sometimes alphabetically sorted, and sometimes in a random place. Examples: usb301_vbus_reg: regulator-usb301 { compatible = "regulator-fixed"; regulator-name = "P5.0V_USB3CON1"; regulator-min-microvolt = <5000000>; regulator-max-microvolt = <5000000>; gpio = <&gph0 1 0>; pinctrl-names = "default"; pinctrl-0 = <&usb301_vbus_en>; enable-active-high; }; &hdmi { status = "okay"; hpd-gpio = <&gpx3 7 GPIO_ACTIVE_HIGH>; pinctrl-names = "default"; pinctrl-0 = <&hdmi_hpd_irq>; ddc = <&i2c_2>; }; 3. I have no idea how to sort nodes. In theory you could say that they should be sorted by base address: i2s0: i2s@03830000 { ... }; hsi2c_7: i2c@12CD0000 { ... }; i2s1: i2s@12D60000 { ... }; ...that works until someone argues that all of the "i2s" nodes should be together. It also doesn't work so well with the board convention of using aliases to refer to things in the SoC, like: &i2s0 { status = "okay"; }; &hsi2c_7 { status = "okay"; }; ...it's not at all obvious in the board file what the base address in the SoC was. --- Anyway, none of this is earth shattering and it doesn't matter all that much. It's just nice to have an official order to make diffing easier and also to avoid merge conflicts (unlikely someone changing different properties will both add them in the same place in the ordering). -Doug
On Mon, Jun 16, 2014 at 10:19 PM, Doug Anderson <dianders@google.com> wrote: > Tushar, > > On Mon, Jun 16, 2014 at 4:19 AM, Tushar Behera <trblinux@gmail.com> wrote: >> On 06/13/2014 10:33 PM, Doug Anderson wrote: >>> Tushar, >>> >>> 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. :( >>> >> Hi Doug, >> >> You are right. I checked the boot logs, the detected codec type is >> MAX98091. Since both these CODECs are supported through a single driver >> and the detection of chip is done during runtime, I would suggest we go >> ahead with "max98090" compatible string. I will update the commit >> message accordingly. >> >> Does that sound okay to you? > > As per my understanding you shouldn't do this. You should have two patches: > > 1. Add "max98091". You could simply post Wonjoon's patch from > <https://chromium-review.googlesource.com/184091> > > 2. Change the device tree to refer to "max98091" > > The argument that the "current kernel driver has a single driver" is > an argument that you're not supposed to make for device tree. The > same device tree is supposed to work for U-Boot, BSD, or any other > platform. On those platforms it might not be a shared driver. > My argument is that the device type is getting detected during runtime, hence there is no need to differentiate between these two. But if you prefer that way, I will repost. > >> If you so desire, I will submit a patch to sort peach-pi device-tree >> nodes (w.r.t. peach-pit dts file). > > Yes please. I think there's supposed to be some official ordering of > things. If anyone reading this has a pointer to the official sort > order of things in the device tree I'd love to see it! ;) > > -Doug
On Mon, Jun 16, 2014 at 10:32 PM, Doug Anderson <dianders@google.com> wrote: > Mark, > > On Mon, Jun 16, 2014 at 9:51 AM, Mark Brown <broonie@kernel.org> wrote: >> On Mon, Jun 16, 2014 at 09:49:26AM -0700, Doug Anderson wrote: >> >>> Yes please. I think there's supposed to be some official ordering of >>> things. If anyone reading this has a pointer to the official sort >>> order of things in the device tree I'd love to see it! ;) >> >> Most exact first I believe? > > More specifically I'm looking for the ordering between nodes and > between properties in a node. For instance: > > 1. It appears to be convention to sort children of the "pinctrl" nodes > by the first pin number in that group. That is: > > ec_spi_cs: ec-spi-cs { > samsung,pins = "gpb1-2"; > ... > }; > > ...comes before: > usb300_vbus_en: usb300-vbus-en { > samsung,pins = "gph0-0"; > ... > }; > > ...that's one really good and well-defined ordering. > > > 2. I have no idea how general properties should be sorted. I tend to > see "compatible" first but that's above the only rule I've seen. > Sometimes I've seen "status" first, sometimes last, sometimes > alphabetically sorted, and sometimes in a random place. Examples: > > usb301_vbus_reg: regulator-usb301 { > compatible = "regulator-fixed"; > regulator-name = "P5.0V_USB3CON1"; > regulator-min-microvolt = <5000000>; > regulator-max-microvolt = <5000000>; > gpio = <&gph0 1 0>; > pinctrl-names = "default"; > pinctrl-0 = <&usb301_vbus_en>; > enable-active-high; > }; > > &hdmi { > status = "okay"; > hpd-gpio = <&gpx3 7 GPIO_ACTIVE_HIGH>; > pinctrl-names = "default"; > pinctrl-0 = <&hdmi_hpd_irq>; > ddc = <&i2c_2>; > }; > > > 3. I have no idea how to sort nodes. In theory you could say that > they should be sorted by base address: > > i2s0: i2s@03830000 { > ... > }; > > hsi2c_7: i2c@12CD0000 { > ... > }; > > i2s1: i2s@12D60000 { > ... > }; > > ...that works until someone argues that all of the "i2s" nodes should > be together. It also doesn't work so well with the board convention > of using aliases to refer to things in the SoC, like: > > &i2s0 { > status = "okay"; > }; > > &hsi2c_7 { > status = "okay"; > }; > > ...it's not at all obvious in the board file what the base address in > the SoC was. > In case where we are using only aliases in board file, sorting them alphabetically would be reasonable. This rule would be easy to reinforce. > --- > > Anyway, none of this is earth shattering and it doesn't matter all > that much. It's just nice to have an official order to make diffing > easier and also to avoid merge conflicts (unlikely someone changing > different properties will both add them in the same place in the > ordering). > > > -Doug
Tushar, On Mon, Jun 16, 2014 at 8:36 PM, Tushar Behera <trblinux@gmail.com> wrote: > On Mon, Jun 16, 2014 at 10:19 PM, Doug Anderson <dianders@google.com> wrote: >> Tushar, >> >> On Mon, Jun 16, 2014 at 4:19 AM, Tushar Behera <trblinux@gmail.com> wrote: >>> On 06/13/2014 10:33 PM, Doug Anderson wrote: >>>> Tushar, >>>> >>>> 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. :( >>>> >>> Hi Doug, >>> >>> You are right. I checked the boot logs, the detected codec type is >>> MAX98091. Since both these CODECs are supported through a single driver >>> and the detection of chip is done during runtime, I would suggest we go >>> ahead with "max98090" compatible string. I will update the commit >>> message accordingly. >>> >>> Does that sound okay to you? >> >> As per my understanding you shouldn't do this. You should have two patches: >> >> 1. Add "max98091". You could simply post Wonjoon's patch from >> <https://chromium-review.googlesource.com/184091> >> >> 2. Change the device tree to refer to "max98091" >> >> The argument that the "current kernel driver has a single driver" is >> an argument that you're not supposed to make for device tree. The >> same device tree is supposed to work for U-Boot, BSD, or any other >> platform. On those platforms it might not be a shared driver. >> > > My argument is that the device type is getting detected during > runtime, hence there is no need to differentiate between these two. > > But if you prefer that way, I will repost. Yes please. True that it is possible to detect 98090 vs. 98091. ...but it's also possible to detect exynos5250 vs. exynos5420 vs. exynos5800. ...yet they have different compatible strings.
diff --git a/arch/arm/boot/dts/exynos5800-peach-pi.dts b/arch/arm/boot/dts/exynos5800-peach-pi.dts index f3af207..76f5966 100644 --- a/arch/arm/boot/dts/exynos5800-peach-pi.dts +++ b/arch/arm/boot/dts/exynos5800-peach-pi.dts @@ -78,9 +78,27 @@ pinctrl-0 = <&usb301_vbus_en>; enable-active-high; }; + + sound { + compatible = "google,snow-audio-max98090"; + + samsung,i2s-controller = <&i2s0>; + samsung,audio-codec = <&max98090>; + }; +}; + +&i2s0 { + status = "okay"; }; &pinctrl_0 { + max98090_irq: max98090-irq { + samsung,pins = "gpx0-2"; + samsung,pin-function = <0>; + samsung,pin-pud = <0>; + samsung,pin-drv = <0>; + }; + tpm_irq: tpm-irq { samsung,pins = "gpx1-0"; samsung,pin-function = <0>; @@ -207,6 +225,19 @@ samsung,invert-vclk; }; +&hsi2c_7 { + status = "okay"; + + max98090: codec@10 { + compatible = "maxim,max98090"; + reg = <0x10>; + interrupts = <2 0>; + interrupt-parent = <&gpx0>; + pinctrl-names = "default"; + pinctrl-0 = <&max98090_irq>; + }; +}; + &hsi2c_9 { status = "okay"; clock-frequency = <400000>;
Peach-pi board has MAX98090 audio codec connected on HSI2C-7 bus. Signed-off-by: Tushar Behera <tushar.b@samsung.com> --- arch/arm/boot/dts/exynos5800-peach-pi.dts | 31 +++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+)