| Message ID | 20181011165123.32198-1-dmurphy@ti.com |
|---|---|
| Headers | show |
| Series | TI LMU common Framework | expand |
Pavel On 10/11/2018 01:27 PM, Pavel Machek wrote: > On Thu 2018-10-11 11:51:16, Dan Murphy wrote: >> Remove support for the LM3697 LED device >> from the ti-lmu. The LM3697 will be supported >> via a stand alone LED driver. >> >> Signed-off-by: Dan Murphy <dmurphy@ti.com> > > NAK. Thanks for the NAK. This NAK was NAK'd by other maintainer in the V2 RFC patchset https://lore.kernel.org/patchwork/patch/993171/ > > Pavel > >> index c885cf89b8ce..920f910be4e9 100644 >> --- a/Documentation/devicetree/bindings/mfd/ti-lmu.txt >> +++ b/Documentation/devicetree/bindings/mfd/ti-lmu.txt >> @@ -9,7 +9,6 @@ TI LMU driver supports lighting devices below. >> LM3632 Backlight and regulator >> LM3633 Backlight, LED and fault monitor >> LM3695 Backlight >> - LM3697 Backlight and fault monitor >> >> Required properties: >> - compatible: Should be one of: >> @@ -18,11 +17,10 @@ Required properties: >> "ti,lm3632" >> "ti,lm3633" >> "ti,lm3695" >> - "ti,lm3697" >> - reg: I2C slave address. >> 0x11 for LM3632 >> 0x29 for LM3631 >> - 0x36 for LM3633, LM3697 >> + 0x36 for LM3633 >> 0x38 for LM3532 >> 0x63 for LM3695 >> >> @@ -38,7 +36,6 @@ Optional nodes: >> Required properties: >> - compatible: Should be one of: >> "ti,lm3633-fault-monitor" >> - "ti,lm3697-fault-monitor" >> - leds: LED properties for LM3633. Please refer to [2]. >> - regulators: Regulator properties for LM3631 and LM3632. >> Please refer to [3]. >> @@ -220,24 +217,3 @@ lm3695@63 { >> }; >> }; >> }; >> - >> -lm3697@36 { >> - compatible = "ti,lm3697"; >> - reg = <0x36>; >> - >> - enable-gpios = <&pioC 2 GPIO_ACTIVE_HIGH>; >> - >> - backlight { >> - compatible = "ti,lm3697-backlight"; >> - >> - lcd { >> - led-sources = <0 1 2>; >> - ramp-up-msec = <200>; >> - ramp-down-msec = <200>; >> - }; >> - }; >> - >> - fault-monitor { >> - compatible = "ti,lm3697-fault-monitor"; >> - }; >> -}; > -- ------------------ Dan Murphy
On Thu, Oct 11, 2018 at 09:38:53PM +0200, Jacek Anaszewski wrote: > On 10/11/2018 08:34 PM, Dan Murphy wrote: > > Pavel > > > > On 10/11/2018 01:27 PM, Pavel Machek wrote: > >> On Thu 2018-10-11 11:51:16, Dan Murphy wrote: > >>> Remove support for the LM3697 LED device > >>> from the ti-lmu. The LM3697 will be supported > >>> via a stand alone LED driver. > >>> > >>> Signed-off-by: Dan Murphy <dmurphy@ti.com> > >> > >> NAK. > > > > Thanks for the NAK. > > > > This NAK was NAK'd by other maintainer in the V2 RFC patchset > > > > https://lore.kernel.org/patchwork/patch/993171/ > > I confirm. LM3697 is a standalone device and not a cell of any > MFD device. > > Waiting for DT maintainer's ack. You all sort out what you want... I can't follow it all, and I'm not going to spend the time trying to figure out what is going on here. As this is worded, changing the driver is a Linux problem and irrelevant to the binding. Now if you want to move documentation to a location that makes more sense, then fine. But structure patches that way and make it clear that from an binding ABI perspective, nothing is changing. Rob
Hi! > > >>> Signed-off-by: Dan Murphy <dmurphy@ti.com> > > >> > > >> NAK. > > > > > > Thanks for the NAK. > > > > > > This NAK was NAK'd by other maintainer in the V2 RFC patchset > > > > > > https://lore.kernel.org/patchwork/patch/993171/ > > > > I confirm. LM3697 is a standalone device and not a cell of any > > MFD device. > > > > Waiting for DT maintainer's ack. > > You all sort out what you want... I can't follow it all, and I'm not > going to spend the time trying to figure out what is going on here. This is what I want: > As this is worded, changing the driver is a Linux problem and irrelevant > to the binding. Now if you want to move documentation to a location that > makes more sense, then fine. But structure patches that way and make it > clear that from an binding ABI perspective, nothing is changing. ...but apparently I did not have enough authority to get it. (I'm ok with move, and it is possible that binding does need some fixups besides the move; still it should be done as fixup not as a new binding). Thanks, Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On Sat, Oct 13, 2018 at 1:46 PM Jacek Anaszewski <jacek.anaszewski@gmail.com> wrote: > > On 10/12/2018 08:03 PM, Pavel Machek wrote: > > Hi! > > > >>>>>> Signed-off-by: Dan Murphy <dmurphy@ti.com> > >>>>> > >>>>> NAK. > >>>> > >>>> Thanks for the NAK. > >>>> > >>>> This NAK was NAK'd by other maintainer in the V2 RFC patchset > >>>> > >>>> https://lore.kernel.org/patchwork/patch/993171/ > >>> > >>> I confirm. LM3697 is a standalone device and not a cell of any > >>> MFD device. > >>> > >>> Waiting for DT maintainer's ack. > >> > >> You all sort out what you want... I can't follow it all, and I'm not > >> going to spend the time trying to figure out what is going on here. > > > > This is what I want: > > > >> As this is worded, changing the driver is a Linux problem and irrelevant > >> to the binding. Now if you want to move documentation to a location that > >> makes more sense, then fine. But structure patches that way and make it > >> clear that from an binding ABI perspective, nothing is changing. > > > > ...but apparently I did not have enough authority to get it. > > > > (I'm ok with move, and it is possible that binding does need some > > fixups besides the move; still it should be done as fixup not as a new > > binding). > > There is a fundamental question - should the bindings be considered > an ABI, even though there is no mainline "*.dts" implementation basing > on these bindings? In theory there could be out of tree users. Having a dts file using it in tree shouldn't be a requirement to maintain the ABI IMO. However, a lack of dts is one piece of determining whether this is in use or not. > This patch fixes the issues of bindings in a way that would change > the ABI, if only it existed. But it apparently doesn't exist in > mainline. Unless a DT documentation itself constitutes an ABI. > > I'd like to have it clarified at this occasion, and that's why > I kindly ask for DT maintainer's ack or NACK for this modification > of bindings. > > For a reference we have a nice summary of the MFD driver and related > bindings' flaws in [0] and [1]. > > [0] https://lkml.org/lkml/2018/9/7/774 > [1] https://lkml.org/lkml/2018/9/11/984 Given this one seems to have not really been finished, it's probably okay to make changes in this case. Still, it would be good to see patches structured so that it's obvious we're breaking things. But how the patches are structured doesn't matter until there's some agreement on the end result. Rob
On 10/15/2018 02:56 AM, Rob Herring wrote: > On Sat, Oct 13, 2018 at 1:46 PM Jacek Anaszewski > <jacek.anaszewski@gmail.com> wrote: >> >> On 10/12/2018 08:03 PM, Pavel Machek wrote: >>> Hi! >>> >>>>>>>> Signed-off-by: Dan Murphy <dmurphy@ti.com> >>>>>>> >>>>>>> NAK. >>>>>> >>>>>> Thanks for the NAK. >>>>>> >>>>>> This NAK was NAK'd by other maintainer in the V2 RFC patchset >>>>>> >>>>>> https://lore.kernel.org/patchwork/patch/993171/ >>>>> >>>>> I confirm. LM3697 is a standalone device and not a cell of any >>>>> MFD device. >>>>> >>>>> Waiting for DT maintainer's ack. >>>> >>>> You all sort out what you want... I can't follow it all, and I'm not >>>> going to spend the time trying to figure out what is going on here. >>> >>> This is what I want: >>> >>>> As this is worded, changing the driver is a Linux problem and irrelevant >>>> to the binding. Now if you want to move documentation to a location that >>>> makes more sense, then fine. But structure patches that way and make it >>>> clear that from an binding ABI perspective, nothing is changing. >>> >>> ...but apparently I did not have enough authority to get it. >>> >>> (I'm ok with move, and it is possible that binding does need some >>> fixups besides the move; still it should be done as fixup not as a new >>> binding). >> >> There is a fundamental question - should the bindings be considered >> an ABI, even though there is no mainline "*.dts" implementation basing >> on these bindings? > > In theory there could be out of tree users. Having a dts file using it > in tree shouldn't be a requirement to maintain the ABI IMO. However, a > lack of dts is one piece of determining whether this is in use or not. > >> This patch fixes the issues of bindings in a way that would change >> the ABI, if only it existed. But it apparently doesn't exist in >> mainline. Unless a DT documentation itself constitutes an ABI. >> >> I'd like to have it clarified at this occasion, and that's why >> I kindly ask for DT maintainer's ack or NACK for this modification >> of bindings. >> >> For a reference we have a nice summary of the MFD driver and related >> bindings' flaws in [0] and [1]. >> >> [0] https://lkml.org/lkml/2018/9/7/774 >> [1] https://lkml.org/lkml/2018/9/11/984 > > Given this one seems to have not really been finished, it's probably > okay to make changes in this case. Still, it would be good to see > patches structured so that it's obvious we're breaking things. But how > the patches are structured doesn't matter until there's some agreement > on the end result. OK, so if I'm getting it right, the correct patch structure in this case means that changes removing bindings from MFD and moving them along with the modifications to the LED subsystem should be placed in a single patch. Dan, could you please arrange the next patch set version accordingly? -- Best regards, Jacek Anaszewski
Jacek On 10/15/2018 02:13 PM, Jacek Anaszewski wrote: > On 10/15/2018 02:56 AM, Rob Herring wrote: >> On Sat, Oct 13, 2018 at 1:46 PM Jacek Anaszewski >> <jacek.anaszewski@gmail.com> wrote: >>> >>> On 10/12/2018 08:03 PM, Pavel Machek wrote: >>>> Hi! >>>> >>>>>>>>> Signed-off-by: Dan Murphy <dmurphy@ti.com> >>>>>>>> >>>>>>>> NAK. >>>>>>> >>>>>>> Thanks for the NAK. >>>>>>> >>>>>>> This NAK was NAK'd by other maintainer in the V2 RFC patchset >>>>>>> >>>>>>> https://lore.kernel.org/patchwork/patch/993171/ >>>>>> >>>>>> I confirm. LM3697 is a standalone device and not a cell of any >>>>>> MFD device. >>>>>> >>>>>> Waiting for DT maintainer's ack. >>>>> >>>>> You all sort out what you want... I can't follow it all, and I'm not >>>>> going to spend the time trying to figure out what is going on here. >>>> >>>> This is what I want: >>>> >>>>> As this is worded, changing the driver is a Linux problem and irrelevant >>>>> to the binding. Now if you want to move documentation to a location that >>>>> makes more sense, then fine. But structure patches that way and make it >>>>> clear that from an binding ABI perspective, nothing is changing. >>>> >>>> ...but apparently I did not have enough authority to get it. >>>> >>>> (I'm ok with move, and it is possible that binding does need some >>>> fixups besides the move; still it should be done as fixup not as a new >>>> binding). >>> >>> There is a fundamental question - should the bindings be considered >>> an ABI, even though there is no mainline "*.dts" implementation basing >>> on these bindings? >> >> In theory there could be out of tree users. Having a dts file using it >> in tree shouldn't be a requirement to maintain the ABI IMO. However, a >> lack of dts is one piece of determining whether this is in use or not. >> >>> This patch fixes the issues of bindings in a way that would change >>> the ABI, if only it existed. But it apparently doesn't exist in >>> mainline. Unless a DT documentation itself constitutes an ABI. >>> >>> I'd like to have it clarified at this occasion, and that's why >>> I kindly ask for DT maintainer's ack or NACK for this modification >>> of bindings. >>> >>> For a reference we have a nice summary of the MFD driver and related >>> bindings' flaws in [0] and [1]. >>> >>> [0] https://lkml.org/lkml/2018/9/7/774 >>> [1] https://lkml.org/lkml/2018/9/11/984 >> >> Given this one seems to have not really been finished, it's probably >> okay to make changes in this case. Still, it would be good to see >> patches structured so that it's obvious we're breaking things. But how >> the patches are structured doesn't matter until there's some agreement >> on the end result. > > OK, so if I'm getting it right, the correct patch structure in this case > means that changes removing bindings from MFD and moving them along > with the modifications to the LED subsystem should be placed in a single > patch. > > Dan, could you please arrange the next patch set version accordingly? Yes I can squash the DT bindings patches and call it a "move and modify" Dan > -- ------------------ Dan Murphy
Hi! > >> Given this one seems to have not really been finished, it's probably > >> okay to make changes in this case. Still, it would be good to see > >> patches structured so that it's obvious we're breaking things. But how > >> the patches are structured doesn't matter until there's some agreement > >> on the end result. > > > > OK, so if I'm getting it right, the correct patch structure in this case > > means that changes removing bindings from MFD and moving them along > > with the modifications to the LED subsystem should be placed in a single > > patch. > > > > Dan, could you please arrange the next patch set version accordingly? > > Yes I can squash the DT bindings patches and call it a "move and modify" You can do move without problems. But if you plan to modify them, please try to change as little as possible, make it separate patch and explain why new version is better than old one. Thanks, Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Thanks On 10/15/2018 04:45 PM, Pavel Machek wrote: > Hi! > >>>> Given this one seems to have not really been finished, it's probably >>>> okay to make changes in this case. Still, it would be good to see >>>> patches structured so that it's obvious we're breaking things. But how >>>> the patches are structured doesn't matter until there's some agreement >>>> on the end result. >>> >>> OK, so if I'm getting it right, the correct patch structure in this case >>> means that changes removing bindings from MFD and moving them along >>> with the modifications to the LED subsystem should be placed in a single >>> patch. >>> >>> Dan, could you please arrange the next patch set version accordingly? >> >> Yes I can squash the DT bindings patches and call it a "move and modify" > > You can do move without problems. But if you plan to modify them, > please try to change as little as possible, make it separate patch and > explain why new version is better than old one. > I have been thinking of how to do this. Since the 2 devices are part of the current binding there really is not a way to move them. Since there are still MFD capable devices available. I would still need to remove the current active binding and create a new binding in the LED bindings directory. I would have to remove and create in the same patch. Dan > Thanks, > Pavel > -- ------------------ Dan Murphy
Hi! > >>>> Given this one seems to have not really been finished, it's probably > >>>> okay to make changes in this case. Still, it would be good to see > >>>> patches structured so that it's obvious we're breaking things. But how > >>>> the patches are structured doesn't matter until there's some agreement > >>>> on the end result. > >>> > >>> OK, so if I'm getting it right, the correct patch structure in this case > >>> means that changes removing bindings from MFD and moving them along > >>> with the modifications to the LED subsystem should be placed in a single > >>> patch. > >>> > >>> Dan, could you please arrange the next patch set version accordingly? > >> > >> Yes I can squash the DT bindings patches and call it a "move and modify" > > > > You can do move without problems. But if you plan to modify them, > > please try to change as little as possible, make it separate patch and > > explain why new version is better than old one. > > > > I have been thinking of how to do this. Since the 2 devices are part of the current > binding there really is not a way to move them. Since there are still MFD capable > devices available. > > I would still need to remove the current active binding and create a new binding in the LED > bindings directory. > > I would have to remove and create in the same patch. Yeah, and this all is a sign that you should just keep the current binding, and make your code use it, see? You are free to use Sebastian's updated binding. It has "suggested by: Rob" or something like that on it, so it should be fine. You can add note to bindings/leds pointing to mfd binding. Now... this is what I've suggested before. If you don't agree, you may want to contact Tony Lindgren, IIRC he works for TI, too, and might be willing to help. Thank you, Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On 10/18/2018 05:10 PM, Pavel Machek wrote: > Hi! > >>>>>> Given this one seems to have not really been finished, it's probably >>>>>> okay to make changes in this case. Still, it would be good to see >>>>>> patches structured so that it's obvious we're breaking things. But how >>>>>> the patches are structured doesn't matter until there's some agreement >>>>>> on the end result. >>>>> >>>>> OK, so if I'm getting it right, the correct patch structure in this case >>>>> means that changes removing bindings from MFD and moving them along >>>>> with the modifications to the LED subsystem should be placed in a single >>>>> patch. >>>>> >>>>> Dan, could you please arrange the next patch set version accordingly? >>>> >>>> Yes I can squash the DT bindings patches and call it a "move and modify" >>> >>> You can do move without problems. But if you plan to modify them, >>> please try to change as little as possible, make it separate patch and >>> explain why new version is better than old one. >>> >> >> I have been thinking of how to do this. Since the 2 devices are part of the current >> binding there really is not a way to move them. Since there are still MFD capable >> devices available. >> >> I would still need to remove the current active binding and create a new binding in the LED >> bindings directory. >> >> I would have to remove and create in the same patch. > > Yeah, and this all is a sign that you should just keep the current > binding, and make your code use it, see? No I don't actually see this as a sign. This is just operational issues nothing to do with actually correcting the incomplete unused bindings. And actually moving and creating the bindings in the same patch makes sense as the change is self contained in a single patch and is easier to track. > > You are free to use Sebastian's updated binding. It has "suggested by: > Rob" or something like that on it, so it should be fine. > > You can add note to bindings/leds pointing to mfd binding. > > Now... this is what I've suggested before. If you don't agree, you may > want to contact Tony Lindgren, IIRC he works for TI, too, and might be > willing to help. I will ping Tony just to close the loop. I will be posting v4 today after making the changes. I was hoping to have some code review prior to posting v4 but have not received any comments so v4 will just be a patch rearrangement. Dan > > Thank you, > Pavel > -- ------------------ Dan Murphy
Rob Thanks for the review. On 10/12/2018 11:27 AM, Rob Herring wrote: > On Thu, Oct 11, 2018 at 11:51:18AM -0500, Dan Murphy wrote: >> Add the device tree bindings for the lm3697 >> LED driver for backlighting and display. > > Bindings are for h/w, not drivers... > ACK Dan >> >> Signed-off-by: Dan Murphy <dmurphy@ti.com> >> --- >> .../devicetree/bindings/leds/leds-lm3697.txt | 98 +++++++++++++++++++ >> 1 file changed, 98 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/leds/leds-lm3697.txt >> >> diff --git a/Documentation/devicetree/bindings/leds/leds-lm3697.txt b/Documentation/devicetree/bindings/leds/leds-lm3697.txt >> new file mode 100644 >> index 000000000000..4bb2ed51025b >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/leds/leds-lm3697.txt >> @@ -0,0 +1,98 @@ >> +* Texas Instruments - LM3697 Highly Efficient White LED Driver >> + >> +The LM3697 11-bit LED driver provides high- >> +performance backlight dimming for 1, 2, or 3 series >> +LED strings while delivering up to 90% efficiency. >> + >> +This device is suitable for display and keypad Lighting >> + >> +Required properties: >> + - compatible: >> + "ti,lm3697" >> + - reg : I2C slave address >> + - #address-cells : 1 >> + - #size-cells : 0 >> + >> +Optional properties: >> + - enable-gpios : GPIO pin to enable/disable the device >> + - vled-supply : LED supply >> + >> +Required child properties: >> + - reg : 0 - LED is Controlled by bank A >> + 1 - LED is Controlled by bank B >> + - led-sources : Indicates which HVLED string is associated to which >> + control bank. Each element in the array is associated >> + with a specific HVLED string. Element 0 is HVLED1, >> + element 1 is HVLED2 and element 2 HVLED3. >> + Additional information is contained >> + in Documentation/devicetree/bindings/leds/common.txt >> + 0 - HVLED is not active in this control bank >> + 1 - HVLED string is controlled by this control bank >> + >> +Optional child properties: >> + - runtime-ramp-up-msec: Current ramping from one brightness level to >> + the a higher brightness level. >> + Range from 2048 us - 117.44 s >> + - runtime-ramp-down-msec: Current ramping from one brightness level to >> + the a lower brightness level. >> + Range from 2048 us - 117.44 s >> + - label : see Documentation/devicetree/bindings/leds/common.txt >> + - linux,default-trigger : >> + see Documentation/devicetree/bindings/leds/common.txt >> + >> +Example: >> + >> +HVLED string 1 and 3 are controlled by control bank A and HVLED 2 string is >> +controlled by control bank B. >> + >> +led-controller@36 { >> + compatible = "ti,lm3697"; >> + reg = <0x36>; >> + #address-cells = <1>; >> + #size-cells = <0>; >> + >> + enable-gpios = <&gpio1 28 GPIO_ACTIVE_HIGH>; >> + vled-supply = <&vbatt>; >> + >> + led@0 { >> + reg = <0>; >> + led-sources = <1 0 1>; >> + runtime-ramp-up-msec = <5000>; >> + runtime-ramp-down-msec = <1000>; >> + label = "white:first_backlight_cluster"; >> + linux,default-trigger = "backlight"; >> + }; >> + >> + led@1 { >> + reg = <1>; >> + led-sources = <0 1 0>; >> + runtime-ramp-up-msec = <500>; >> + runtime-ramp-down-msec = <1000>; >> + label = "white:second_backlight_cluster"; >> + linux,default-trigger = "backlight"; >> + }; >> +} >> + >> +All HVLED strings controlled by control bank A >> + >> +led-controller@36 { >> + compatible = "ti,lm3697"; >> + reg = <0x36>; >> + #address-cells = <1>; >> + #size-cells = <0>; >> + >> + enable-gpios = <&gpio1 28 GPIO_ACTIVE_HIGH>; >> + vled-supply = <&vbatt>; >> + >> + led@0 { >> + reg = <0>; >> + led-sources = <1 1 1>; >> + runtime-ramp-up-msec = <500>; >> + runtime-ramp-down-msec = <1000>; >> + label = "white:backlight_cluster"; >> + linux,default-trigger = "backlight"; >> + }; >> +} >> + >> +For more product information please see the link below: >> +http://www.ti.com/lit/ds/symlink/lm3697.pdf >> -- >> 2.19.0 >> -- ------------------ Dan Murphy
* Dan Murphy <dmurphy@ti.com> [181019 11:42]: > On 10/18/2018 05:10 PM, Pavel Machek wrote: > > > > Now... this is what I've suggested before. If you don't agree, you may > > want to contact Tony Lindgren, IIRC he works for TI, too, and might be > > willing to help. > > I will ping Tony just to close the loop. I will be posting v4 today after making the changes. > I was hoping to have some code review prior to posting v4 but have not received any comments so > v4 will just be a patch rearrangement. I guess not much to ping here though as I know little about these chips :) As long as Rob is happy with the binding changes I'll be happy too. Regards, Tony