| Message ID | 1480399662-8858-2-git-send-email-architt@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Archit, Thank you for the patch. On Tuesday 29 Nov 2016 11:37:41 Archit Taneja wrote: > Add the regulator supply properties needed by ADV7511 and ADV7533. > > The regulators are specified as optional properties since there can > be boards which have a fixed supply directly routed to the pins, and > these may not be modelled as regulator supplies. That's why we have support for dummy supplies in the kernel, isn't it ? Isn't it better to make the supplies mandatory in the bindings (and obviously handling them as optional in the driver for backward-compatibility) ? Apart from that, Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Cc: devicetree@vger.kernel.org > Acked-by: Rob Herring <robh@kernel.org> > Signed-off-by: Archit Taneja <architt@codeaurora.org> > --- > v3: > - Revert back to having a common avdd-supply property for the 1.8V > supplies > > Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt | 9 ++++++ > 1 file changed, 9 insertions(+) > > diff --git > a/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt > b/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt index > 6532a59..13d53bc 100644 > --- a/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt > +++ b/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt > @@ -56,6 +56,15 @@ Optional properties: > - adi,disable-timing-generator: Only for ADV7533. Disables the internal > timing generator. The chip will rely on the sync signals in the DSI data > lanes, rather than generate its own timings for HDMI output. > +- avdd-supply: A common 1.8V supply that powers up the AVDD, DVDD and PVDD > + pins. On ADV7511, it also feeds to the BGVDD pin. On ADV7533, it also > powers > + up the A2VDD pin. > +- v3p3-supply: A 3.3V supply that powers up the pin called DVDD_3V on > + ADV7511 and V3P3 on ADV7533. > + > +ADV7533 specific supplies: > +- v1p2-supply: A supply that powers up the V1P2 pin on the chip. It can be > + either 1.2V or 1.8V. > > Required nodes:
On 11/29/2016 12:03 PM, Laurent Pinchart wrote: > Hi Archit, > > Thank you for the patch. > > On Tuesday 29 Nov 2016 11:37:41 Archit Taneja wrote: >> Add the regulator supply properties needed by ADV7511 and ADV7533. >> >> The regulators are specified as optional properties since there can >> be boards which have a fixed supply directly routed to the pins, and >> these may not be modelled as regulator supplies. > > That's why we have support for dummy supplies in the kernel, isn't it ? Isn't > it better to make the supplies mandatory in the bindings (and obviously > handling them as optional in the driver for backward-compatibility) ? I'm a bit unclear on this. I thought we couldn't add mandatory properties once the device is already present in DT for one or more platforms. Say, if we do make it mandatory for future additions, we would need to have DT property for the supplies for the new platforms. If the regulators on these boards are fixed supplies, they would be need to be modeled using "regulator-fixed", possibly without any input supply. Is that what you're suggesting? Thanks, Archit > > Apart from that, > > Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >> Cc: devicetree@vger.kernel.org >> Acked-by: Rob Herring <robh@kernel.org> >> Signed-off-by: Archit Taneja <architt@codeaurora.org> >> --- >> v3: >> - Revert back to having a common avdd-supply property for the 1.8V >> supplies >> >> Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt | 9 ++++++ >> 1 file changed, 9 insertions(+) >> >> diff --git >> a/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt >> b/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt index >> 6532a59..13d53bc 100644 >> --- a/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt >> +++ b/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt >> @@ -56,6 +56,15 @@ Optional properties: >> - adi,disable-timing-generator: Only for ADV7533. Disables the internal >> timing generator. The chip will rely on the sync signals in the DSI data >> lanes, rather than generate its own timings for HDMI output. >> +- avdd-supply: A common 1.8V supply that powers up the AVDD, DVDD and PVDD >> + pins. On ADV7511, it also feeds to the BGVDD pin. On ADV7533, it also >> powers >> + up the A2VDD pin. >> +- v3p3-supply: A 3.3V supply that powers up the pin called DVDD_3V on >> + ADV7511 and V3P3 on ADV7533. >> + >> +ADV7533 specific supplies: >> +- v1p2-supply: A supply that powers up the V1P2 pin on the chip. It can be >> + either 1.2V or 1.8V. >> >> Required nodes: >
Hi Archit, (CC'ing Mark Brown) On Tuesday 29 Nov 2016 13:41:33 Archit Taneja wrote: > On 11/29/2016 12:03 PM, Laurent Pinchart wrote: > > On Tuesday 29 Nov 2016 11:37:41 Archit Taneja wrote: > >> Add the regulator supply properties needed by ADV7511 and ADV7533. > >> > >> The regulators are specified as optional properties since there can > >> be boards which have a fixed supply directly routed to the pins, and > >> these may not be modelled as regulator supplies. > > > > That's why we have support for dummy supplies in the kernel, isn't it ? > > Isn't it better to make the supplies mandatory in the bindings (and > > obviously handling them as optional in the driver for > > backward-compatibility) ? > > I'm a bit unclear on this. > > I thought we couldn't add mandatory properties once the device is already > present in DT for one or more platforms. You can, as long as you treat them as optional in the driver to retain backward compatibility. The DT bindings should document the properties expected from a new platform (older versions of the bindings will always be available in the git history). > Say, if we do make it mandatory for future additions, we would need to have > DT property for the supplies for the new platforms. If the regulators on > these boards are fixed supplies, they would be need to be modeled > using "regulator-fixed", possibly without any input supply. Is that > what you're suggesting? That's the idea, yes. Clock maintainers have a similar opinion regarding the clock bindings, where a clock that is not optional at the hardware level should be specified in DT even if it's always present. Mark, any opinion ? > > Apart from that, > > > > Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > >> Cc: devicetree@vger.kernel.org > >> Acked-by: Rob Herring <robh@kernel.org> > >> Signed-off-by: Archit Taneja <architt@codeaurora.org> > >> --- > >> v3: > >> - Revert back to having a common avdd-supply property for the 1.8V > >> supplies > >> > >> Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt | 9 ++++ > >> 1 file changed, 9 insertions(+) > >> > >> diff --git > >> a/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt > >> b/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt index > >> 6532a59..13d53bc 100644 > >> --- a/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt > >> +++ b/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt > >> > >> @@ -56,6 +56,15 @@ Optional properties: > >> - adi,disable-timing-generator: Only for ADV7533. Disables the internal > >> > >> timing generator. The chip will rely on the sync signals in the DSI data > >> lanes, rather than generate its own timings for HDMI output. > >> +- avdd-supply: A common 1.8V supply that powers up the AVDD, DVDD and > >> PVDD > >> + pins. On ADV7511, it also feeds to the BGVDD pin. On ADV7533, it also > >> powers > >> + up the A2VDD pin. > >> +- v3p3-supply: A 3.3V supply that powers up the pin called DVDD_3V on > >> + ADV7511 and V3P3 on ADV7533. > >> + > >> +ADV7533 specific supplies: > >> +- v1p2-supply: A supply that powers up the V1P2 pin on the chip. It can > >> be > >> + either 1.2V or 1.8V. > >> > >> Required nodes:
On Tue, Nov 29, 2016 at 11:11:03AM +0200, Laurent Pinchart wrote: > On Tuesday 29 Nov 2016 13:41:33 Archit Taneja wrote: > > I thought we couldn't add mandatory properties once the device is already > > present in DT for one or more platforms. > You can, as long as you treat them as optional in the driver to retain > backward compatibility. The DT bindings should document the properties > expected from a new platform (older versions of the bindings will always be > available in the git history). The device probably never worked without power... note that the kernel will substitute in dummy regulators for anything that isn't explicitly mapped so it won't actually break anything. > > Say, if we do make it mandatory for future additions, we would need to have > > DT property for the supplies for the new platforms. If the regulators on > > these boards are fixed supplies, they would be need to be modeled > > using "regulator-fixed", possibly without any input supply. Is that > > what you're suggesting? > That's the idea, yes. Clock maintainers have a similar opinion regarding the > clock bindings, where a clock that is not optional at the hardware level > should be specified in DT even if it's always present. > Mark, any opinion ? It's best practice to always describe the power. The kernel will cope if people don't but it's not unknown for drivers to discover a reason for wanting information about their power and hard to retrofit that if it's not been in there from the get go. Please note that if you're going to CC me into a graphics thread there's a good chance I will miss it, I get copied on quite a lot of graphics related mail that's not really relevant so I often skip it. Changing the subject line would help with that.
Hi Mark, On Tuesday 29 Nov 2016 11:01:25 Mark Brown wrote: > On Tue, Nov 29, 2016 at 11:11:03AM +0200, Laurent Pinchart wrote: > > On Tuesday 29 Nov 2016 13:41:33 Archit Taneja wrote: > >> I thought we couldn't add mandatory properties once the device is > >> already present in DT for one or more platforms. > > > > You can, as long as you treat them as optional in the driver to retain > > backward compatibility. The DT bindings should document the properties > > expected from a new platform (older versions of the bindings will always > > be available in the git history). > > The device probably never worked without power... note that the kernel > will substitute in dummy regulators for anything that isn't explicitly > mapped so it won't actually break anything. > > >> Say, if we do make it mandatory for future additions, we would need to > >> have DT property for the supplies for the new platforms. If the > >> regulators on these boards are fixed supplies, they would be need to be > >> modeled using "regulator-fixed", possibly without any input supply. Is > >> that what you're suggesting? > > > > That's the idea, yes. Clock maintainers have a similar opinion regarding > > the clock bindings, where a clock that is not optional at the hardware > > level should be specified in DT even if it's always present. > > > > Mark, any opinion ? > > It's best practice to always describe the power. The kernel will cope > if people don't but it's not unknown for drivers to discover a reason > for wanting information about their power and hard to retrofit that if > it's not been in there from the get go. Sounds good to me, thanks. > Please note that if you're going to CC me into a graphics thread there's > a good chance I will miss it, I get copied on quite a lot of graphics > related mail that's not really relevant so I often skip it. Changing > the subject line would help with that. I try to add a (CC'ing xxx) at the beginning of the e-mail to draw attention when a question is targetted at a particular person. If that's not enough I can change the subject, but might forget to do so from time to time. Or you could whitelist me, unless I'm already blacklisted ;-)
On Tue, Nov 29, 2016 at 09:37:31PM +0200, Laurent Pinchart wrote: > On Tuesday 29 Nov 2016 11:01:25 Mark Brown wrote: > > Please note that if you're going to CC me into a graphics thread there's > > a good chance I will miss it, I get copied on quite a lot of graphics > > related mail that's not really relevant so I often skip it. Changing > > the subject line would help with that. > I try to add a (CC'ing xxx) at the beginning of the e-mail to draw attention > when a question is targetted at a particular person. If that's not enough I > can change the subject, but might forget to do so from time to time. Or you > could whitelist me, unless I'm already blacklisted ;-) That helps if I open the mail (especially with large e-mails, I do sometimes get bored looking for something relevant) but I also filter just on subject lines - once you've been involved with a thread about a topic people often seem to end up copying you for anything even vaguely related unfortunately.
On Tue 29 Nov 01:11 PST 2016, Laurent Pinchart wrote: > Hi Archit, > > (CC'ing Mark Brown) > > On Tuesday 29 Nov 2016 13:41:33 Archit Taneja wrote: > > On 11/29/2016 12:03 PM, Laurent Pinchart wrote: > > > On Tuesday 29 Nov 2016 11:37:41 Archit Taneja wrote: > > >> Add the regulator supply properties needed by ADV7511 and ADV7533. > > >> > > >> The regulators are specified as optional properties since there can > > >> be boards which have a fixed supply directly routed to the pins, and > > >> these may not be modelled as regulator supplies. > > > > > > That's why we have support for dummy supplies in the kernel, isn't it ? > > > Isn't it better to make the supplies mandatory in the bindings (and > > > obviously handling them as optional in the driver for > > > backward-compatibility) ? > > > > I'm a bit unclear on this. > > > > I thought we couldn't add mandatory properties once the device is already > > present in DT for one or more platforms. > > You can, as long as you treat them as optional in the driver to retain > backward compatibility. The DT bindings should document the properties > expected from a new platform (older versions of the bindings will always be > available in the git history). If you document them as required and don't do anything special in the implementation (i.e. just call devm_regulator_get() as usual) it will just work, in the absence of the property you will get a dummy regulator from the framework. And then add the fixed-voltage regulators to the new DT to make that properly describe the hardware. > > > Say, if we do make it mandatory for future additions, we would need to have > > DT property for the supplies for the new platforms. If the regulators on > > these boards are fixed supplies, they would be need to be modeled > > using "regulator-fixed", possibly without any input supply. Is that > > what you're suggesting? > > That's the idea, yes. Clock maintainers have a similar opinion regarding the > clock bindings, where a clock that is not optional at the hardware level > should be specified in DT even if it's always present. > Further more, a DT binding for a particular block should describe that block; so if we have three different 1.8V pins then the DT binding should reflect this - even if our current platform have them wired to the same regulator. (And the supply names would preferably be based on the pin names in the component data sheet) Regards, Bjorn
Hi Bjorn, On Monday 05 Dec 2016 13:11:51 Bjorn Andersson wrote: > On Tue 29 Nov 01:11 PST 2016, Laurent Pinchart wrote: > > On Tuesday 29 Nov 2016 13:41:33 Archit Taneja wrote: > >> On 11/29/2016 12:03 PM, Laurent Pinchart wrote: > >>> On Tuesday 29 Nov 2016 11:37:41 Archit Taneja wrote: > >>>> Add the regulator supply properties needed by ADV7511 and ADV7533. > >>>> > >>>> The regulators are specified as optional properties since there can > >>>> be boards which have a fixed supply directly routed to the pins, and > >>>> these may not be modelled as regulator supplies. > >>> > >>> That's why we have support for dummy supplies in the kernel, isn't it > >>> ? Isn't it better to make the supplies mandatory in the bindings (and > >>> obviously handling them as optional in the driver for > >>> backward-compatibility) ? > >> > >> I'm a bit unclear on this. > >> > >> I thought we couldn't add mandatory properties once the device is > >> already present in DT for one or more platforms. > > > > You can, as long as you treat them as optional in the driver to retain > > backward compatibility. The DT bindings should document the properties > > expected from a new platform (older versions of the bindings will always > > be available in the git history). > > If you document them as required and don't do anything special in the > implementation (i.e. just call devm_regulator_get() as usual) it will > just work, in the absence of the property you will get a dummy regulator > from the framework. > > And then add the fixed-voltage regulators to the new DT to make that > properly describe the hardware. > > >> Say, if we do make it mandatory for future additions, we would need to > >> have DT property for the supplies for the new platforms. If the > >> regulators on these boards are fixed supplies, they would be need to be > >> modeled using "regulator-fixed", possibly without any input supply. Is > >> that what you're suggesting? > > > > That's the idea, yes. Clock maintainers have a similar opinion regarding > > the clock bindings, where a clock that is not optional at the hardware > > level should be specified in DT even if it's always present. > > Further more, a DT binding for a particular block should describe that > block; so if we have three different 1.8V pins then the DT binding > should reflect this - even if our current platform have them wired to > the same regulator. This has been discussed previously, and Rob agreed that if the datasheet recommends to power all supplies from the same regulator we can take that as a good hint that a single supply should be enough. In the very unlikely event that a board would require control of more regulators we can always extend the DT bindings later without breaking backward compatibility. > (And the supply names would preferably be based on the pin names in the > component data sheet)
On Mon, Dec 05, 2016 at 11:16:22PM +0200, Laurent Pinchart wrote: > On Monday 05 Dec 2016 13:11:51 Bjorn Andersson wrote: > > Further more, a DT binding for a particular block should describe that > > block; so if we have three different 1.8V pins then the DT binding > > should reflect this - even if our current platform have them wired to > > the same regulator. > This has been discussed previously, and Rob agreed that if the datasheet > recommends to power all supplies from the same regulator we can take that as a > good hint that a single supply should be enough. In the very unlikely event > that a board would require control of more regulators we can always extend the > DT bindings later without breaking backward compatibility. No, don't do this - introducing special snowflake bindings just makes things more complex at the system level and tells everyone else that they too can have special snowflake bindings. Someone should be able to connect up the regulators based purely on a schematic. Just describe the hardware, it's just one extra line in the DT per regulator.
Hi Mark, On Tuesday 06 Dec 2016 10:05:17 Mark Brown wrote: > On Mon, Dec 05, 2016 at 11:16:22PM +0200, Laurent Pinchart wrote: > > On Monday 05 Dec 2016 13:11:51 Bjorn Andersson wrote: > >> Further more, a DT binding for a particular block should describe that > >> block; so if we have three different 1.8V pins then the DT binding > >> should reflect this - even if our current platform have them wired to > >> the same regulator. > > > > This has been discussed previously, and Rob agreed that if the datasheet > > recommends to power all supplies from the same regulator we can take that > > as a good hint that a single supply should be enough. In the very > > unlikely event that a board would require control of more regulators we > > can always extend the DT bindings later without breaking backward > > compatibility. > > No, don't do this - introducing special snowflake bindings just makes > things more complex at the system level and tells everyone else that > they too can have special snowflake bindings. Someone should be able to > connect up the regulators based purely on a schematic. Just describe > the hardware, it's just one extra line in the DT per regulator. There are power supply pin that have different names but documented as having to be connected to the same supply. I really see no point in having multiple regulators for them.
On Tue, Dec 06, 2016 at 02:46:55PM +0200, Laurent Pinchart wrote: > On Tuesday 06 Dec 2016 10:05:17 Mark Brown wrote: > > On Mon, Dec 05, 2016 at 11:16:22PM +0200, Laurent Pinchart wrote: > > > This has been discussed previously, and Rob agreed that if the datasheet > > > recommends to power all supplies from the same regulator we can take that > > > as a good hint that a single supply should be enough. In the very > > No, don't do this - introducing special snowflake bindings just makes > > things more complex at the system level and tells everyone else that > > they too can have special snowflake bindings. Someone should be able to > > connect up the regulators based purely on a schematic. Just describe > > the hardware, it's just one extra line in the DT per regulator. > There are power supply pin that have different names but documented as having > to be connected to the same supply. I really see no point in having multiple > regulators for them. The tiny amount of extra typing involved doesn't seem like much of a cost for keeping things consistent with every other regulator user out there.
Hi Mark, On Tuesday 06 Dec 2016 13:20:20 Mark Brown wrote: > On Tue, Dec 06, 2016 at 02:46:55PM +0200, Laurent Pinchart wrote: > > On Tuesday 06 Dec 2016 10:05:17 Mark Brown wrote: > > > On Mon, Dec 05, 2016 at 11:16:22PM +0200, Laurent Pinchart wrote: > > > > This has been discussed previously, and Rob agreed that if the > > > > datasheet recommends to power all supplies from the same regulator we > > > > can take that as a good hint that a single supply should be enough. In > > > > the very > > > > > > No, don't do this - introducing special snowflake bindings just makes > > > things more complex at the system level and tells everyone else that > > > they too can have special snowflake bindings. Someone should be able to > > > connect up the regulators based purely on a schematic. Just describe > > > the hardware, it's just one extra line in the DT per regulator. > > > > There are power supply pin that have different names but documented as > > having to be connected to the same supply. I really see no point in > > having multiple regulators for them. > > The tiny amount of extra typing involved doesn't seem like much of a > cost for keeping things consistent with every other regulator user out > there. I'm not concerned by that at all, but by the additional runtime complexity :-/
On Tue, Dec 06, 2016 at 06:08:55PM +0200, Laurent Pinchart wrote: > On Tuesday 06 Dec 2016 13:20:20 Mark Brown wrote: > > The tiny amount of extra typing involved doesn't seem like much of a > > cost for keeping things consistent with every other regulator user out > > there. > I'm not concerned by that at all, but by the additional runtime complexity :-/ regulator_bulk_()! :)
diff --git a/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt b/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt index 6532a59..13d53bc 100644 --- a/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt +++ b/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt @@ -56,6 +56,15 @@ Optional properties: - adi,disable-timing-generator: Only for ADV7533. Disables the internal timing generator. The chip will rely on the sync signals in the DSI data lanes, rather than generate its own timings for HDMI output. +- avdd-supply: A common 1.8V supply that powers up the AVDD, DVDD and PVDD + pins. On ADV7511, it also feeds to the BGVDD pin. On ADV7533, it also powers + up the A2VDD pin. +- v3p3-supply: A 3.3V supply that powers up the pin called DVDD_3V on + ADV7511 and V3P3 on ADV7533. + +ADV7533 specific supplies: +- v1p2-supply: A supply that powers up the V1P2 pin on the chip. It can be + either 1.2V or 1.8V. Required nodes: