| Message ID | 20180809192944.7371-1-kieran.bingham@ideasonboard.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] dt-bindings: media: adv748x: Document re-mappable addresses | expand |
On Thu, Aug 09, 2018 at 08:29:44PM +0100, Kieran Bingham wrote: > The ADV748x supports configurable slave addresses for its I2C pages. > Document the page names, and provide an example for setting each of the > pages explicitly. It would be good to say why you need this. The only use I can think of is if there are other devices on the bus and you need to make sure the addresses don't conflict. Arguably, that information could be figured out without this in DT. Regardless, Reviewed-by: Rob Herring <robh@kernel.org> > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > --- > v2: > - Fix commit message > - Extend documentation for the "required property" reg: > > v3 > - Fix missing comment from Laurent. > - correct the reg descrption > --- > .../devicetree/bindings/media/i2c/adv748x.txt | 16 ++++++++++++++-- > 1 file changed, 14 insertions(+), 2 deletions(-) > > diff --git a/Documentation/devicetree/bindings/media/i2c/adv748x.txt b/Documentation/devicetree/bindings/media/i2c/adv748x.txt > index 21ffb5ed8183..25a02496f4ba 100644 > --- a/Documentation/devicetree/bindings/media/i2c/adv748x.txt > +++ b/Documentation/devicetree/bindings/media/i2c/adv748x.txt > @@ -10,7 +10,11 @@ Required Properties: > - "adi,adv7481" for the ADV7481 > - "adi,adv7482" for the ADV7482 > > - - reg: I2C slave address > + - reg: I2C slave addresses > + The ADV748x has up to twelve 256-byte maps that can be accessed via the > + main I2C ports. Each map has it own I2C address and acts as a standard > + slave device on the I2C bus. The main address is mandatory, others are > + optional and remain at default values if not specified. > > Optional Properties: > > @@ -18,6 +22,11 @@ Optional Properties: > "intrq3". All interrupts are optional. The "intrq3" interrupt > is only available on the adv7481 > - interrupts: Specify the interrupt lines for the ADV748x > + - reg-names : Names of maps with programmable addresses. > + It shall contain all maps needing a non-default address. > + Possible map names are: > + "main", "dpll", "cp", "hdmi", "edid", "repeater", > + "infoframe", "cbus", "cec", "sdp", "txa", "txb" > > The device node must contain one 'port' child node per device input and output > port, in accordance with the video interface bindings defined in > @@ -47,7 +56,10 @@ Example: > > video-receiver@70 { > compatible = "adi,adv7482"; > - reg = <0x70>; > + reg = <0x70 0x71 0x72 0x73 0x74 0x75 > + 0x60 0x61 0x62 0x63 0x64 0x65>; > + reg-names = "main", "dpll", "cp", "hdmi", "edid", "repeater", > + "infoframe", "cbus", "cec", "sdp", "txa", "txb"; > > #address-cells = <1>; > #size-cells = <0>; > -- > 2.17.1 > > -- > To unsubscribe from this list: send the line "unsubscribe devicetree" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Rob, On 13/08/18 18:45, Rob Herring wrote: > On Thu, Aug 09, 2018 at 08:29:44PM +0100, Kieran Bingham wrote: >> The ADV748x supports configurable slave addresses for its I2C pages. >> Document the page names, and provide an example for setting each of the >> pages explicitly. > > It would be good to say why you need this. In fact - I should probably have added a fixes tag here, which would have added more context: Fixes: 67537fe960e5 ("media: i2c: adv748x: Add support for i2c_new_secondary_device") Should I repost with this fixes tag? Or can it be collected with the RB tag? > The only use I can think of > is if there are other devices on the bus and you need to make sure the > addresses don't conflict. Yes, precisely. This driver has 'slave pages' which are created and mapped by the driver. The device has default addresses which are used by the driver - but it's very easy for these to conflict with other devices on the same I2C bus. Because the mappings are simply a software construct, we have a means to specify the desired mappings through DT at the board level - which allows the boards to ensure that conflicts do not appear. > Arguably, that information could be figured out without this in DT. How so ? Scanning the bus is error prone, and dependant upon driver state (and presence), and we have no means currently of requesting 'free/unused' addresses from the I2C core framework. > Regardless, > > Reviewed-by: Rob Herring <robh@kernel.org> Thank you. -- Regards Kieran > >> >> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >> >> --- >> v2: >> - Fix commit message >> - Extend documentation for the "required property" reg: >> >> v3 >> - Fix missing comment from Laurent. >> - correct the reg descrption >> --- >> .../devicetree/bindings/media/i2c/adv748x.txt | 16 ++++++++++++++-- >> 1 file changed, 14 insertions(+), 2 deletions(-) >> >> diff --git a/Documentation/devicetree/bindings/media/i2c/adv748x.txt b/Documentation/devicetree/bindings/media/i2c/adv748x.txt >> index 21ffb5ed8183..25a02496f4ba 100644 >> --- a/Documentation/devicetree/bindings/media/i2c/adv748x.txt >> +++ b/Documentation/devicetree/bindings/media/i2c/adv748x.txt >> @@ -10,7 +10,11 @@ Required Properties: >> - "adi,adv7481" for the ADV7481 >> - "adi,adv7482" for the ADV7482 >> >> - - reg: I2C slave address >> + - reg: I2C slave addresses >> + The ADV748x has up to twelve 256-byte maps that can be accessed via the >> + main I2C ports. Each map has it own I2C address and acts as a standard >> + slave device on the I2C bus. The main address is mandatory, others are >> + optional and remain at default values if not specified. >> >> Optional Properties: >> >> @@ -18,6 +22,11 @@ Optional Properties: >> "intrq3". All interrupts are optional. The "intrq3" interrupt >> is only available on the adv7481 >> - interrupts: Specify the interrupt lines for the ADV748x >> + - reg-names : Names of maps with programmable addresses. >> + It shall contain all maps needing a non-default address. >> + Possible map names are: >> + "main", "dpll", "cp", "hdmi", "edid", "repeater", >> + "infoframe", "cbus", "cec", "sdp", "txa", "txb" >> >> The device node must contain one 'port' child node per device input and output >> port, in accordance with the video interface bindings defined in >> @@ -47,7 +56,10 @@ Example: >> >> video-receiver@70 { >> compatible = "adi,adv7482"; >> - reg = <0x70>; >> + reg = <0x70 0x71 0x72 0x73 0x74 0x75 >> + 0x60 0x61 0x62 0x63 0x64 0x65>; >> + reg-names = "main", "dpll", "cp", "hdmi", "edid", "repeater", >> + "infoframe", "cbus", "cec", "sdp", "txa", "txb"; >> >> #address-cells = <1>; >> #size-cells = <0>; >> -- >> 2.17.1 >> >> -- >> To unsubscribe from this list: send the line "unsubscribe devicetree" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Kieran, Thank you for the patch. On Thursday, 9 August 2018 22:29:44 EEST Kieran Bingham wrote: > The ADV748x supports configurable slave addresses for its I2C pages. > Document the page names, and provide an example for setting each of the > pages explicitly. > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > --- > v2: > - Fix commit message > - Extend documentation for the "required property" reg: > > v3 > - Fix missing comment from Laurent. > - correct the reg descrption > --- > .../devicetree/bindings/media/i2c/adv748x.txt | 16 ++++++++++++++-- > 1 file changed, 14 insertions(+), 2 deletions(-) > > diff --git a/Documentation/devicetree/bindings/media/i2c/adv748x.txt > b/Documentation/devicetree/bindings/media/i2c/adv748x.txt index > 21ffb5ed8183..25a02496f4ba 100644 > --- a/Documentation/devicetree/bindings/media/i2c/adv748x.txt > +++ b/Documentation/devicetree/bindings/media/i2c/adv748x.txt > @@ -10,7 +10,11 @@ Required Properties: > - "adi,adv7481" for the ADV7481 > - "adi,adv7482" for the ADV7482 > > - - reg: I2C slave address > + - reg: I2C slave addresses > + The ADV748x has up to twelve 256-byte maps that can be accessed via the > + main I2C ports. Each map has it own I2C address and acts as a standard > + slave device on the I2C bus. The main address is mandatory, others are > + optional and remain at default values if not specified. > > Optional Properties: > > @@ -18,6 +22,11 @@ Optional Properties: > "intrq3". All interrupts are optional. The "intrq3" interrupt > is only available on the adv7481 > - interrupts: Specify the interrupt lines for the ADV748x > + - reg-names : Names of maps with programmable addresses. > + It shall contain all maps needing a non-default address. > + Possible map names are: > + "main", "dpll", "cp", "hdmi", "edid", "repeater", > + "infoframe", "cbus", "cec", "sdp", "txa", "txb" Reading this again, the text doesn't document the "main" map very well. The main I2C address is always required, but if no other I2C address needs to be specified, the reg-names property can be omitted. Otherwise the "main" reg- names entry is required. This being said, this problem isn't specific to the adv748x bindings, so I don't want to block this patch until we figure out a proper wording that can be applied globally. For that reason, Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > The device node must contain one 'port' child node per device input and > output port, in accordance with the video interface bindings defined in > @@ -47,7 +56,10 @@ Example: > > video-receiver@70 { > compatible = "adi,adv7482"; > - reg = <0x70>; > + reg = <0x70 0x71 0x72 0x73 0x74 0x75 > + 0x60 0x61 0x62 0x63 0x64 0x65>; > + reg-names = "main", "dpll", "cp", "hdmi", "edid", "repeater", > + "infoframe", "cbus", "cec", "sdp", "txa", "txb"; > > #address-cells = <1>; > #size-cells = <0>;
On Mon, Aug 13, 2018 at 1:17 PM Kieran Bingham <kieran.bingham@ideasonboard.com> wrote: > > Hi Rob, > > On 13/08/18 18:45, Rob Herring wrote: > > On Thu, Aug 09, 2018 at 08:29:44PM +0100, Kieran Bingham wrote: > >> The ADV748x supports configurable slave addresses for its I2C pages. > >> Document the page names, and provide an example for setting each of the > >> pages explicitly. > > > > It would be good to say why you need this. > > In fact - I should probably have added a fixes tag here, which would > have added more context: > > Fixes: 67537fe960e5 ("media: i2c: adv748x: Add support for > i2c_new_secondary_device") That doesn't really explain things from a DT perspective. > Should I repost with this fixes tag? > Or can it be collected with the RB tag? I'll leave that to Hans. > > The only use I can think of > > is if there are other devices on the bus and you need to make sure the > > addresses don't conflict. > > Yes, precisely. This driver has 'slave pages' which are created and > mapped by the driver. The device has default addresses which are used by > the driver - but it's very easy for these to conflict with other devices > on the same I2C bus. > > Because the mappings are simply a software construct, we have a means to > specify the desired mappings through DT at the board level - which > allows the boards to ensure that conflicts do not appear. > > > > Arguably, that information could be figured out without this in DT. > > How so ? > > Scanning the bus is error prone, and dependant upon driver state (and > presence), and we have no means currently of requesting 'free/unused' > addresses from the I2C core framework. True. But assuming all devices are in DT, then you just need to scan the child nodes of the bus and get a map of the used addresses. Though if you had 2 or more devices like this, then you'd need to maintain s/w allocated addresses too. It could all be maintained with a bitmap which you initialize with addresses in DT. Rob
Hi Rob, On Tuesday, 14 August 2018 01:48:05 EEST Rob Herring wrote: > On Mon, Aug 13, 2018 at 1:17 PM Kieran Bingham wrote: > > On 13/08/18 18:45, Rob Herring wrote: > > > On Thu, Aug 09, 2018 at 08:29:44PM +0100, Kieran Bingham wrote: > > >> The ADV748x supports configurable slave addresses for its I2C pages. > > >> Document the page names, and provide an example for setting each of the > > >> pages explicitly. > > > > > > It would be good to say why you need this. > > > > In fact - I should probably have added a fixes tag here, which would > > have added more context: > > > > Fixes: 67537fe960e5 ("media: i2c: adv748x: Add support for > > i2c_new_secondary_device") > > That doesn't really explain things from a DT perspective. > > > Should I repost with this fixes tag? > > Or can it be collected with the RB tag? > > I'll leave that to Hans. > > > > The only use I can think of > > > is if there are other devices on the bus and you need to make sure the > > > addresses don't conflict. > > > > Yes, precisely. This driver has 'slave pages' which are created and > > mapped by the driver. The device has default addresses which are used by > > the driver - but it's very easy for these to conflict with other devices > > on the same I2C bus. > > > > Because the mappings are simply a software construct, we have a means to > > specify the desired mappings through DT at the board level - which > > allows the boards to ensure that conflicts do not appear. > > > > > Arguably, that information could be figured out without this in DT. > > > > How so ? > > > > Scanning the bus is error prone, and dependant upon driver state (and > > presence), and we have no means currently of requesting 'free/unused' > > addresses from the I2C core framework. > > True. But assuming all devices are in DT, then you just need to scan > the child nodes of the bus and get a map of the used addresses. Though > if you had 2 or more devices like this, then you'd need to maintain > s/w allocated addresses too. It could all be maintained with a bitmap > which you initialize with addresses in DT. We've discussed this topic with Wolfram before, and unfortunately the base assumption of assuming all devices are in DT often doesn't hold :-( For all kind of reasons vendors often don't provide a full description of I2C buses, especially when slave devices don't need to be accessed from Linux. Sometimes they lack DT bindings for some I2C slaves that are not essential and still want to provide a partial but functional DT. Sometimes devices are hooked up to an I2C bus for debugging purposes only and end up never being used, so nobody bothers describing them. Sometimes an I2C slave has multiple slave addresses which DT writes are not aware of. Those are just a few examples. One alternative would be to add a DT property to the I2C master node to list the known to be free addresses. I'm not sure that's better though. We should also keep in mind that some I2C slaves could have restrictions on which secondary addresses can be used, so we would need to pass constraints to the I2C address allocator, which would quickly become a mess.
Hi Rob, On 13/08/18 23:48, Rob Herring wrote: > On Mon, Aug 13, 2018 at 1:17 PM Kieran Bingham > <kieran.bingham@ideasonboard.com> wrote: >> >> On 13/08/18 18:45, Rob Herring wrote: >>> On Thu, Aug 09, 2018 at 08:29:44PM +0100, Kieran Bingham wrote: >>>> The ADV748x supports configurable slave addresses for its I2C pages. >>>> Document the page names, and provide an example for setting each of the >>>> pages explicitly. >>> >>> It would be good to say why you need this. >> >> In fact - I should probably have added a fixes tag here, which would >> have added more context: >> >> Fixes: 67537fe960e5 ("media: i2c: adv748x: Add support for >> i2c_new_secondary_device") > > That doesn't really explain things from a DT perspective. > >> Should I repost with this fixes tag? >> Or can it be collected with the RB tag? > > I'll leave that to Hans. > >>> The only use I can think of >>> is if there are other devices on the bus and you need to make sure the >>> addresses don't conflict. >> >> Yes, precisely. This driver has 'slave pages' which are created and >> mapped by the driver. The device has default addresses which are used by >> the driver - but it's very easy for these to conflict with other devices >> on the same I2C bus. >> >> Because the mappings are simply a software construct, we have a means to >> specify the desired mappings through DT at the board level - which >> allows the boards to ensure that conflicts do not appear. >> >> >>> Arguably, that information could be figured out without this in DT. >> >> How so ? >> >> Scanning the bus is error prone, and dependant upon driver state (and >> presence), and we have no means currently of requesting 'free/unused' >> addresses from the I2C core framework. > > True. But assuming all devices are in DT, then you just need to scan > the child nodes of the bus and get a map of the used addresses. Though > if you had 2 or more devices like this, then you'd need to maintain > s/w allocated addresses too. It could all be maintained with a bitmap > which you initialize with addresses in DT. We do indeed have cases with platforms which use the same device populated twice: See 1d26a5217187 ("ARM: dts: wheat: Fix ADV7513 address usage") In that instance, a hardware bug means that if one of the two instances of the ADV7513 goes into standby, it will respond to it's default hardware slave map addresses. So because of this we must map *both* instances to be non defaults. (and ideally, we'd want to mark the actual defaults as unused - but - unusable; alas we don't have a means to do that yet) In the instance of this device (the adv748x) it has a default mapping at address 0x30 for the CBUS page. We have an expansion board, which adds GMSL cameras to the same board - of which reside on the same I2C bus - and are fixed to use 0x30 as the base address for one of the components. (Which even that then gets re-mapped - but it must be available to perform the remap in the first place) So we even have this concept of an address which is 'mostly' unused, but must be available for use at certain stages of the probe sequences. (There are 8 cameras attached, all with that same 0x30 address) The GMSL expansion is an 'overlay' (it's actually just an include file currently, but it /should/ be a DTO), and so I use this functionality to remap the ADV748x maps to a 'free' block of address space. Adding that patch [0] ([PATCH] arm64: dts: renesas: salvator-common: adv748x: Override secondary addresses) was what led me to notice that I had not updated the DT documentation for the feature, leading to this patch :) [0] https://www.spinics.net/lists/linux-renesas-soc/msg31194.html
Hi Kieran, Thanks for your patch. On 2018-08-09 20:29:44 +0100, Kieran Bingham wrote: > The ADV748x supports configurable slave addresses for its I2C pages. > Document the page names, and provide an example for setting each of the > pages explicitly. > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > > --- > v2: > - Fix commit message > - Extend documentation for the "required property" reg: > > v3 > - Fix missing comment from Laurent. > - correct the reg descrption > --- > .../devicetree/bindings/media/i2c/adv748x.txt | 16 ++++++++++++++-- > 1 file changed, 14 insertions(+), 2 deletions(-) > > diff --git a/Documentation/devicetree/bindings/media/i2c/adv748x.txt b/Documentation/devicetree/bindings/media/i2c/adv748x.txt > index 21ffb5ed8183..25a02496f4ba 100644 > --- a/Documentation/devicetree/bindings/media/i2c/adv748x.txt > +++ b/Documentation/devicetree/bindings/media/i2c/adv748x.txt > @@ -10,7 +10,11 @@ Required Properties: > - "adi,adv7481" for the ADV7481 > - "adi,adv7482" for the ADV7482 > > - - reg: I2C slave address > + - reg: I2C slave addresses > + The ADV748x has up to twelve 256-byte maps that can be accessed via the > + main I2C ports. Each map has it own I2C address and acts as a standard > + slave device on the I2C bus. The main address is mandatory, others are > + optional and remain at default values if not specified. > > Optional Properties: > > @@ -18,6 +22,11 @@ Optional Properties: > "intrq3". All interrupts are optional. The "intrq3" interrupt > is only available on the adv7481 > - interrupts: Specify the interrupt lines for the ADV748x > + - reg-names : Names of maps with programmable addresses. > + It shall contain all maps needing a non-default address. > + Possible map names are: > + "main", "dpll", "cp", "hdmi", "edid", "repeater", > + "infoframe", "cbus", "cec", "sdp", "txa", "txb" > > The device node must contain one 'port' child node per device input and output > port, in accordance with the video interface bindings defined in > @@ -47,7 +56,10 @@ Example: > > video-receiver@70 { > compatible = "adi,adv7482"; > - reg = <0x70>; > + reg = <0x70 0x71 0x72 0x73 0x74 0x75 > + 0x60 0x61 0x62 0x63 0x64 0x65>; > + reg-names = "main", "dpll", "cp", "hdmi", "edid", "repeater", > + "infoframe", "cbus", "cec", "sdp", "txa", "txb"; > > #address-cells = <1>; > #size-cells = <0>; > -- > 2.17.1 >
diff --git a/Documentation/devicetree/bindings/media/i2c/adv748x.txt b/Documentation/devicetree/bindings/media/i2c/adv748x.txt index 21ffb5ed8183..25a02496f4ba 100644 --- a/Documentation/devicetree/bindings/media/i2c/adv748x.txt +++ b/Documentation/devicetree/bindings/media/i2c/adv748x.txt @@ -10,7 +10,11 @@ Required Properties: - "adi,adv7481" for the ADV7481 - "adi,adv7482" for the ADV7482 - - reg: I2C slave address + - reg: I2C slave addresses + The ADV748x has up to twelve 256-byte maps that can be accessed via the + main I2C ports. Each map has it own I2C address and acts as a standard + slave device on the I2C bus. The main address is mandatory, others are + optional and remain at default values if not specified. Optional Properties: @@ -18,6 +22,11 @@ Optional Properties: "intrq3". All interrupts are optional. The "intrq3" interrupt is only available on the adv7481 - interrupts: Specify the interrupt lines for the ADV748x + - reg-names : Names of maps with programmable addresses. + It shall contain all maps needing a non-default address. + Possible map names are: + "main", "dpll", "cp", "hdmi", "edid", "repeater", + "infoframe", "cbus", "cec", "sdp", "txa", "txb" The device node must contain one 'port' child node per device input and output port, in accordance with the video interface bindings defined in @@ -47,7 +56,10 @@ Example: video-receiver@70 { compatible = "adi,adv7482"; - reg = <0x70>; + reg = <0x70 0x71 0x72 0x73 0x74 0x75 + 0x60 0x61 0x62 0x63 0x64 0x65>; + reg-names = "main", "dpll", "cp", "hdmi", "edid", "repeater", + "infoframe", "cbus", "cec", "sdp", "txa", "txb"; #address-cells = <1>; #size-cells = <0>;
The ADV748x supports configurable slave addresses for its I2C pages. Document the page names, and provide an example for setting each of the pages explicitly. Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> --- v2: - Fix commit message - Extend documentation for the "required property" reg: v3 - Fix missing comment from Laurent. - correct the reg descrption --- .../devicetree/bindings/media/i2c/adv748x.txt | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-)