| Message ID | 1424898082-1522-1-git-send-email-arun.ramamurthy@broadcom.com |
|---|---|
| State | Needs Review / ACK, archived |
| Headers | show |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | warning | total: 1 errors, 0 warnings, 0 lines checked |
| robh/patch-applied | success |
On Wed, 2015-02-25 at 21:01 +0000, Arun Ramamurthy wrote: > Added code based on linaro tree: > http://git.linaro.org/kernel/linux-linaro-stable.git > with commit id:6846e7822c4cab5a84672baace3b768c2d0db142 > at drivers/video/amba-clcd.c. This lets the driver set > certain tim2 register bits after reading them from > device tree. > > Reviewed-by: Ray Jui <rjui@broadcom.com> > Reviewed-by: Scott Branden <sbranden@broadcom.com> > Signed-off-by: Arun Ramamurthy <arun.ramamurthy@broadcom.com> > --- > .../devicetree/bindings/video/arm,pl11x.txt | 17 ++++++++- > drivers/video/fbdev/amba-clcd.c | 41 ++++++++++++++++++++++ > 2 files changed, 57 insertions(+), 1 deletion(-) > > diff --git a/Documentation/devicetree/bindings/video/arm,pl11x.txt b/Documentation/devicetree/bindings/video/arm,pl11x.txt > index 3e3039a..14d6f87 100644 > --- a/Documentation/devicetree/bindings/video/arm,pl11x.txt > +++ b/Documentation/devicetree/bindings/video/arm,pl11x.txt > @@ -35,6 +35,21 @@ Optional properties: > cell's memory interface can handle; if not present, the memory > interface is fast enough to handle all possible video modes > > +- tim2: Used to set certain bits in LCDTiming2 register. > + It can be TIM2_CLKSEL or TIM2_IOE or both > + > + TIM2_CLKSEL: This bit drives the CLCDCLKSEL signal. It is the select > + signal for the external LCD clock multiplexor. > + > + TIM2_IOE: Invert output enable: > + 0 = CLAC output pin is active HIGH in TFT mode > + 1 = CLAC output pin is active LOW in TFT mode. > + This bit selects the active polarity of the output enable signal in > + TFT mode. In this mode, the CLAC pin is an enable that indicates to > + the LCD panel when valid display data is available. In active > + display mode, data is driven onto the LCD data lines at the > + programmed edge of CLCP when CLAC is in its active state. > + The existing bindings intentionally avoided quoting internal registers - they are supposed to describe how the hardware is wired up... So how about something like "arm,pl11x,tft-invert-clac"? Then the driver sets the bit or not, depending on the property existance? Pawel -- 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
On 15-03-02 08:11 AM, Pawel Moll wrote: > On Wed, 2015-02-25 at 21:01 +0000, Arun Ramamurthy wrote: >> Added code based on linaro tree: >> http://git.linaro.org/kernel/linux-linaro-stable.git >> with commit id:6846e7822c4cab5a84672baace3b768c2d0db142 >> at drivers/video/amba-clcd.c. This lets the driver set >> certain tim2 register bits after reading them from >> device tree. >> >> Reviewed-by: Ray Jui <rjui@broadcom.com> >> Reviewed-by: Scott Branden <sbranden@broadcom.com> >> Signed-off-by: Arun Ramamurthy <arun.ramamurthy@broadcom.com> >> --- >> .../devicetree/bindings/video/arm,pl11x.txt | 17 ++++++++- >> drivers/video/fbdev/amba-clcd.c | 41 ++++++++++++++++++++++ >> 2 files changed, 57 insertions(+), 1 deletion(-) >> >> diff --git a/Documentation/devicetree/bindings/video/arm,pl11x.txt b/Documentation/devicetree/bindings/video/arm,pl11x.txt >> index 3e3039a..14d6f87 100644 >> --- a/Documentation/devicetree/bindings/video/arm,pl11x.txt >> +++ b/Documentation/devicetree/bindings/video/arm,pl11x.txt >> @@ -35,6 +35,21 @@ Optional properties: >> cell's memory interface can handle; if not present, the memory >> interface is fast enough to handle all possible video modes >> >> +- tim2: Used to set certain bits in LCDTiming2 register. >> + It can be TIM2_CLKSEL or TIM2_IOE or both >> + >> + TIM2_CLKSEL: This bit drives the CLCDCLKSEL signal. It is the select >> + signal for the external LCD clock multiplexor. >> + >> + TIM2_IOE: Invert output enable: >> + 0 = CLAC output pin is active HIGH in TFT mode >> + 1 = CLAC output pin is active LOW in TFT mode. >> + This bit selects the active polarity of the output enable signal in >> + TFT mode. In this mode, the CLAC pin is an enable that indicates to >> + the LCD panel when valid display data is available. In active >> + display mode, data is driven onto the LCD data lines at the >> + programmed edge of CLCP when CLAC is in its active state. >> + > > The existing bindings intentionally avoided quoting internal registers - > they are supposed to describe how the hardware is wired up... > > So how about something like "arm,pl11x,tft-invert-clac"? Then the driver > sets the bit or not, depending on the property existance? > Sure, I can change it to two properties called arm,pl11x,tft-invert-clac and arm,pl11x,tft-clksel. Would that be acceptable? > Pawel > -- 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
On Mon, 2015-03-02 at 19:09 +0000, Arun Ramamurthy wrote: > > The existing bindings intentionally avoided quoting internal registers - > > they are supposed to describe how the hardware is wired up... > > > > So how about something like "arm,pl11x,tft-invert-clac"? Then the driver > > sets the bit or not, depending on the property existance? > > > Sure, I can change it to two properties called arm,pl11x,tft-invert-clac > and arm,pl11x,tft-clksel. Would that be acceptable? That would be fine by me :-) Pawel -- 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
On Tue, 2015-03-03 at 10:02 +0000, Pawel Moll wrote: > On Mon, 2015-03-02 at 19:09 +0000, Arun Ramamurthy wrote: > > > The existing bindings intentionally avoided quoting internal registers - > > > they are supposed to describe how the hardware is wired up... > > > > > > So how about something like "arm,pl11x,tft-invert-clac"? Then the driver > > > sets the bit or not, depending on the property existance? > > > > > Sure, I can change it to two properties called arm,pl11x,tft-invert-clac > > and arm,pl11x,tft-clksel. Would that be acceptable? > > That would be fine by me :-) Or (after having a look at the TRM) I should rather say: the invert-clac is fine by me :-) but the tft-clksel doesn't work, I afraid. If I'm not mistaken, there are two problems with it. Number one: it's not TFT-specific, is it? So it certainly should not have the "tft-" bit. Number two: setting this bit says "do not use CLCDCLK for the logic; use HCLK instead", correct? If so, have a look at the clock properties. They say: - clock-names: should contain "clcdclk" and "apb_pclk" - clocks: contains phandle and clock specifier pairs for the entries in the clock-names property. See So if your hardware has the reference clock wired to HCLK, and you defining the clocks as "clcdclk", you are (no offence meant ;-) lying :-) So how about solving the problem by extending the clock-names definition like this (feel free to use own wording): - clock-names: should contain two clocks, either "clcdclk" or "hclk" (depending on which input is to be used as a reference clock by the controller logic) and "apb_pclk" That way you're precisely describing the way the hardware is wired up. And the driver simply tries to get clcdclk first, if it's defined - cool, set clksel to 1, if not - try hclk and set clksel to 0. If neither of them is present - bail out. Does this make any sense? Pawel -- 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
On 15-03-03 02:22 AM, Pawel Moll wrote: > On Tue, 2015-03-03 at 10:02 +0000, Pawel Moll wrote: >> On Mon, 2015-03-02 at 19:09 +0000, Arun Ramamurthy wrote: >>>> The existing bindings intentionally avoided quoting internal registers - >>>> they are supposed to describe how the hardware is wired up... >>>> >>>> So how about something like "arm,pl11x,tft-invert-clac"? Then the driver >>>> sets the bit or not, depending on the property existance? >>>> >>> Sure, I can change it to two properties called arm,pl11x,tft-invert-clac >>> and arm,pl11x,tft-clksel. Would that be acceptable? >> >> That would be fine by me :-) > > Or (after having a look at the TRM) I should rather say: the invert-clac > is fine by me :-) but the tft-clksel doesn't work, I afraid. > > If I'm not mistaken, there are two problems with it. > > Number one: it's not TFT-specific, is it? So it certainly should not > have the "tft-" bit. > > Number two: setting this bit says "do not use CLCDCLK for the logic; use > HCLK instead", correct? If so, have a look at the clock properties. They > say: > > - clock-names: should contain "clcdclk" and "apb_pclk" > > - clocks: contains phandle and clock specifier pairs for the entries > in the clock-names property. See > > So if your hardware has the reference clock wired to HCLK, and you > defining the clocks as "clcdclk", you are (no offence meant ;-) > lying :-) > No offense taken :) > So how about solving the problem by extending the clock-names definition > like this (feel free to use own wording): > > - clock-names: should contain two clocks, either "clcdclk" or "hclk" > (depending on which input is to be used as a reference > clock by the controller logic) and "apb_pclk" > > That way you're precisely describing the way the hardware is wired up. > And the driver simply tries to get clcdclk first, if it's defined - > cool, set clksel to 1, if not - try hclk and set clksel to 0. If neither > of them is present - bail out. > > Does this make any sense? > This makes sense to me, thank you for the suggestions. I will fix it all up in V2 > Pawel > -- 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
On Wed, 2015-03-04 at 00:37 +0000, Arun Ramamurthy wrote: > > That way you're precisely describing the way the hardware is wired up. > > And the driver simply tries to get clcdclk first, if it's defined - > > cool, set clksel to 1, if not - try hclk and set clksel to 0. If neither > > of them is present - bail out. > > > > Does this make any sense? > > > This makes sense to me, thank you for the suggestions. I will fix it all > up in V2 Cool. Just a word of comment to my own words ;-) The "bail out" case was a bad idea - the non-DT use cases more likely than not will have no clock name defined. So, to maintain backward compatibility, the driver will still have to work when no named clock is available. I think the simplest way to do that is to check if "hclk" is available, and use it if it is (setting the clksel accordingly). Otherwise - proceed as it was the case previously. Pawel -- 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
On Tue, Mar 03, 2015 at 10:22:07AM +0000, Pawel Moll wrote: > On Tue, 2015-03-03 at 10:02 +0000, Pawel Moll wrote: > > On Mon, 2015-03-02 at 19:09 +0000, Arun Ramamurthy wrote: > > > > The existing bindings intentionally avoided quoting internal registers - > > > > they are supposed to describe how the hardware is wired up... > > > > > > > > So how about something like "arm,pl11x,tft-invert-clac"? Then the driver > > > > sets the bit or not, depending on the property existance? > > > > > > > Sure, I can change it to two properties called arm,pl11x,tft-invert-clac > > > and arm,pl11x,tft-clksel. Would that be acceptable? > > > > That would be fine by me :-) > > Or (after having a look at the TRM) I should rather say: the invert-clac > is fine by me :-) but the tft-clksel doesn't work, I afraid. > > If I'm not mistaken, there are two problems with it. > > Number one: it's not TFT-specific, is it? So it certainly should not > have the "tft-" bit. > > Number two: setting this bit says "do not use CLCDCLK for the logic; use > HCLK instead", correct? If so, have a look at the clock properties. They > say: > > - clock-names: should contain "clcdclk" and "apb_pclk" > > - clocks: contains phandle and clock specifier pairs for the entries > in the clock-names property. See > > So if your hardware has the reference clock wired to HCLK, and you > defining the clocks as "clcdclk", you are (no offence meant ;-) > lying :-) No. The CLCD block always takes two clock signals - the AHB bus clock (HCLK) for the slave interface, and a CLCD clock. The CLCDCLKSEL is a bit which affects a signal sent to the world outside of the CLCD block, which is used to drive an _external_ multiplexer to select the CLCD clock source. (See the description for bit 5 of the LCDTiming2 register.) So, the clock is still input to the CLCDCLK input, even if it is ultimately derived from HCLK. Remember, the clock API does not deal with names describing the source of the clock, but the consumer of the clock. The consumer in this case is the PL11x CLCD block, which takes a CLCDCLK input.
On Wed, Feb 25, 2015 at 10:01 PM, Arun Ramamurthy <arun.ramamurthy@broadcom.com> wrote: > Added code based on linaro tree: > http://git.linaro.org/kernel/linux-linaro-stable.git > with commit id:6846e7822c4cab5a84672baace3b768c2d0db142 > at drivers/video/amba-clcd.c. This lets the driver set > certain tim2 register bits after reading them from > device tree. > > Reviewed-by: Ray Jui <rjui@broadcom.com> > Reviewed-by: Scott Branden <sbranden@broadcom.com> > Signed-off-by: Arun Ramamurthy <arun.ramamurthy@broadcom.com> I have now implemented this properly in this patch: http://marc.info/?l=linux-fbdev&m=145459469913983&w=2 Please provide your Tested-by/Reviewed-by/Ack if you're still working on this. Yours, Linus Walleij -- 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 Linus, On 2/10/2016 5:58 AM, Linus Walleij wrote: > On Wed, Feb 25, 2015 at 10:01 PM, Arun Ramamurthy > <arun.ramamurthy@broadcom.com> wrote: > >> Added code based on linaro tree: >> http://git.linaro.org/kernel/linux-linaro-stable.git >> with commit id:6846e7822c4cab5a84672baace3b768c2d0db142 >> at drivers/video/amba-clcd.c. This lets the driver set >> certain tim2 register bits after reading them from >> device tree. >> >> Reviewed-by: Ray Jui <rjui@broadcom.com> >> Reviewed-by: Scott Branden <sbranden@broadcom.com> >> Signed-off-by: Arun Ramamurthy <arun.ramamurthy@broadcom.com> > > I have now implemented this properly in this patch: > http://marc.info/?l=linux-fbdev&m=145459469913983&w=2 > > Please provide your Tested-by/Reviewed-by/Ack if you're > still working on this. Could you please add me to the email thread and I can review it there (I won't have time to test, but I can help to review the code and find time to test later)? This may be a dumb question, is there any way for me to directly reply to the thread here? http://marc.info/?l=linux-fbdev&m=145459469913983&w=2 > > Yours, > Linus Walleij > -- 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
On Wed, Feb 10, 2016 at 6:48 PM, Ray Jui <ray.jui@broadcom.com> wrote: > Could you please add me to the email thread and I can review it there (I > won't have time to test, but I can help to review the code and find time to > test later)? OK I will add you to subsequent postings, if any. > This may be a dumb question, is there any way for me to directly reply to > the thread here? Not easily, I guess it is possible to conjure an SMTP mail with the right in-reply-to message ID but that is so complex hacking that I have no clue how to do it, just ever reached the limit of "a little knowledge is dangerous". Yours, Linus Walleij -- 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
On 2/16/2016 11:32 AM, Dmitry Torokhov wrote: > > > On Mon, Feb 15, 2016 at 5:25 AM, Linus Walleij <linus.walleij@linaro.org > <mailto:linus.walleij@linaro.org>> wrote: > > On Wed, Feb 10, 2016 at 6:48 PM, Ray Jui <ray.jui@broadcom.com > <mailto:ray.jui@broadcom.com>> wrote: > > > Could you please add me to the email thread and I can review it there (I > > won't have time to test, but I can help to review the code and find time to > > test later)? > > OK I will add you to subsequent postings, if any. > > > This may be a dumb question, is there any way for me to directly reply to > > the thread here? > > Not easily, I guess it is possible to conjure an SMTP mail > with the right in-reply-to message ID but that is so complex > hacking that I have no clue how to do it, just ever reached > the limit of "a little knowledge is dangerous". > > > To reply to a patch find it in patchwork, download it as mbox, open > downloaded file with mutt and reply as usual. > > Thanks, > Dmitry Got it, thanks! Ray -- 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
diff --git a/Documentation/devicetree/bindings/video/arm,pl11x.txt b/Documentation/devicetree/bindings/video/arm,pl11x.txt index 3e3039a..14d6f87 100644 --- a/Documentation/devicetree/bindings/video/arm,pl11x.txt +++ b/Documentation/devicetree/bindings/video/arm,pl11x.txt @@ -35,6 +35,21 @@ Optional properties: cell's memory interface can handle; if not present, the memory interface is fast enough to handle all possible video modes +- tim2: Used to set certain bits in LCDTiming2 register. + It can be TIM2_CLKSEL or TIM2_IOE or both + + TIM2_CLKSEL: This bit drives the CLCDCLKSEL signal. It is the select + signal for the external LCD clock multiplexor. + + TIM2_IOE: Invert output enable: + 0 = CLAC output pin is active HIGH in TFT mode + 1 = CLAC output pin is active LOW in TFT mode. + This bit selects the active polarity of the output enable signal in + TFT mode. In this mode, the CLAC pin is an enable that indicates to + the LCD panel when valid display data is available. In active + display mode, data is driven onto the LCD data lines at the + programmed edge of CLCP when CLAC is in its active state. + Required sub-nodes: - port: describes LCD panel signals, following the common binding @@ -76,7 +91,7 @@ Example: clocks = <&oscclk1>, <&oscclk2>; clock-names = "clcdclk", "apb_pclk"; max-memory-bandwidth = <94371840>; /* Bps, 1024x768@60 16bpp */ - + tim2 = "TIM2_CLKSEL"; port { clcd_pads: endpoint { remote-endpoint = <&clcd_panel>; diff --git a/drivers/video/fbdev/amba-clcd.c b/drivers/video/fbdev/amba-clcd.c index 32c0b6b..4e4e50f 100644 --- a/drivers/video/fbdev/amba-clcd.c +++ b/drivers/video/fbdev/amba-clcd.c @@ -41,6 +41,44 @@ /* This is limited to 16 characters when displayed by X startup */ static const char *clcd_name = "CLCD FB"; +struct string_lookup { + const char *string; + const u32 val; +}; + +static const struct string_lookup tim2_lookups[] = { + { "TIM2_CLKSEL", TIM2_CLKSEL}, + { "TIM2_IOE", TIM2_IOE}, + { NULL, 0}, +}; + +static u32 parse_setting(const struct string_lookup *lookup, const char *name) +{ + int i = 0; + + while (lookup[i].string != NULL) { + if (strcmp(lookup[i].string, name) == 0) + return lookup[i].val; + ++i; + } + return 0; +} + +static u32 get_string_lookup(struct device_node *node, const char *name, + const struct string_lookup *lookup) +{ + const char *string; + int count, i; + u32 ret = 0; + + count = of_property_count_strings(node, name); + if (count >= 0) + for (i = 0; i < count; i++) + if (of_property_read_string_index(node, name, i, + &string) == 0) + ret |= parse_setting(lookup, string); + return ret; +} /* * Unfortunately, the enable/disable functions may be called either from * process or IRQ context, and we _need_ to delay. This is _not_ good. @@ -626,6 +664,9 @@ static int clcdfb_of_init_tft_panel(struct clcd_fb *fb, u32 r0, u32 g0, u32 b0) /* Bypass pixel clock divider, data output on the falling edge */ fb->panel->tim2 = TIM2_BCD | TIM2_IPC; + fb->panel->tim2 |= get_string_lookup(fb->dev->dev.of_node, + "tim2", tim2_lookups); + /* TFT display, vert. comp. interrupt at the start of the back porch */ fb->panel->cntl |= CNTL_LCDTFT | CNTL_LCDVCOMP(1);