| Message ID | 20231202075514.44474-1-tony@atomide.com |
|---|---|
| Headers | show |
| Series | Improvments for tc358775 with support for tc358765 | expand |
On Sat, 2 Dec 2023 at 10:01, Tony Lindgren <tony@atomide.com> wrote: > > From: Michael Walle <mwalle@kernel.org> > > The stby pin is optional. It is only needed for power-up and down > sequencing. It is not needed, if the power rails cannot by dynamically > enabled. > > Because the GPIO is not optional, remove the error message. > > Signed-off-by: Michael Walle <mwalle@kernel.org> > Signed-off-by: Tony Lindgren <tony@atomide.com> > --- > drivers/gpu/drm/bridge/tc358775.c | 9 +++------ > 1 file changed, 3 insertions(+), 6 deletions(-) Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
On Sat, 2 Dec 2023 at 10:01, Tony Lindgren <tony@atomide.com> wrote: > > The current code assumes the data-lanes property is configured on the > DSI host side instead of the bridge side, and assumes DSI host endpoint 1. > > Let's standardize on what the other bridge drivers are doing and parse the > data-lanes property for the bridge. Only if data-lanes property is not found, > let's be nice and also check the DSI host for old dtb in use and warn. It might be worth adding that lanes configuration for the host and for the bridge might be different (e.g. the lanes might be swapped on the host side). Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > Signed-off-by: Tony Lindgren <tony@atomide.com> > --- > drivers/gpu/drm/bridge/tc358775.c | 25 +++++++++++-------------- > 1 file changed, 11 insertions(+), 14 deletions(-)
On Sat, 2 Dec 2023 at 10:03, Tony Lindgren <tony@atomide.com> wrote: > > Set pre_enable_prev_first to ensure the previous bridge is enabled > first. > > Signed-off-by: Tony Lindgren <tony@atomide.com> > --- > drivers/gpu/drm/bridge/tc358775.c | 1 + > 1 file changed, 1 insertion(+) > Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
On Sat, 2 Dec 2023 at 10:06, Tony Lindgren <tony@atomide.com> wrote: > > The hs_rate and lp_rate may be used by the dsi host for timing > calculations. The tc358775 has a maximum bit rate of 1 Gbps/lane, > tc358765 has maximurate of 800 Mbps per lane. > > Signed-off-by: Tony Lindgren <tony@atomide.com> > --- > drivers/gpu/drm/bridge/tc358775.c | 5 +++++ > 1 file changed, 5 insertions(+) Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
On Sat, 2 Dec 2023 at 10:04, Tony Lindgren <tony@atomide.com> wrote: > > The tc358775 bridge is pin compatible with earlier tc358765 according to > the tc358774xbg_datasheet_en_20190118.pdf documentation. Compared to the > tc358765, the tc358775 supports a STBY GPIO and higher data rates. > > The tc358765 has a register bit for video event mode vs video pulse mode. > We must set it to video event mode for the LCD output to work, and on the > tc358775, this bit no longer exists. > > Looks like the registers seem to match otherwise based on a quick glance > comparing the defines to the earlier Android kernel tc358765 driver. > > Signed-off-by: Tony Lindgren <tony@atomide.com> > --- > drivers/gpu/drm/bridge/tc358775.c | 26 ++++++++++++++++++++++---- > 1 file changed, 22 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/tc358775.c b/drivers/gpu/drm/bridge/tc358775.c > --- a/drivers/gpu/drm/bridge/tc358775.c > +++ b/drivers/gpu/drm/bridge/tc358775.c > @@ -15,6 +15,7 @@ > #include <linux/kernel.h> > #include <linux/media-bus-format.h> > #include <linux/module.h> > +#include <linux/of_device.h> > #include <linux/regulator/consumer.h> > #include <linux/slab.h> > > @@ -107,6 +108,7 @@ > #define RDPKTLN 0x0404 /* Command Read Packet Length */ > > #define VPCTRL 0x0450 /* Video Path Control */ > +#define EVTMODE BIT(5) /* Video event mode enable, tc35876x only */ > #define HTIM1 0x0454 /* Horizontal Timing Control 1 */ > #define HTIM2 0x0458 /* Horizontal Timing Control 2 */ > #define VTIM1 0x045C /* Vertical Timing Control 1 */ > @@ -254,6 +256,11 @@ enum tc358775_ports { > TC358775_LVDS_OUT1, > }; > > +enum tc3587x5_type { > + TC358765, I'd suggest adding = 1 or =0x65 so that the specified type differs from 'no data' = 0 / NULL. > + TC358775, > +}; > + > struct tc_data { > struct i2c_client *i2c; > struct device *dev; > @@ -271,6 +278,8 @@ struct tc_data { > struct gpio_desc *stby_gpio; > u8 lvds_link; /* single-link or dual-link */ > u8 bpc; > + > + enum tc3587x5_type type; > }; > > static inline struct tc_data *bridge_to_tc(struct drm_bridge *b) > @@ -424,10 +433,16 @@ static void tc_bridge_enable(struct drm_bridge *bridge) > d2l_write(tc->i2c, PPI_STARTPPI, PPI_START_FUNCTION); > d2l_write(tc->i2c, DSI_STARTDSI, DSI_RX_START); > > + /* Video event mode vs pulse mode bit, does not exist for tc358775 */ > + if (tc->type == TC358765) > + val = EVTMODE; > + else > + val = 0; > + > if (tc->bpc == 8) > - val = TC358775_VPCTRL_OPXLFMT(1); > + val |= TC358775_VPCTRL_OPXLFMT(1); > else /* bpc = 6; */ > - val = TC358775_VPCTRL_MSF(1); > + val |= TC358775_VPCTRL_MSF(1); > > dsiclk = mode->crtc_clock * 3 * tc->bpc / tc->num_dsi_lanes / 1000; > clkdiv = dsiclk / (tc->lvds_link == DUAL_LINK ? DIVIDE_BY_6 : DIVIDE_BY_3); > @@ -643,6 +658,7 @@ static int tc_probe(struct i2c_client *client) > > tc->dev = dev; > tc->i2c = client; > + tc->type = (enum tc3587x5_type)of_device_get_match_data(dev); Would it make sense to use i2c_get_match_data() instead? > > tc->panel_bridge = devm_drm_of_get_bridge(dev, dev->of_node, > TC358775_LVDS_OUT0, 0); > @@ -704,13 +720,15 @@ static void tc_remove(struct i2c_client *client) > } > > static const struct i2c_device_id tc358775_i2c_ids[] = { > - { "tc358775", 0 }, > + { "tc358765", TC358765, }, > + { "tc358775", TC358775, }, > { } > }; > MODULE_DEVICE_TABLE(i2c, tc358775_i2c_ids); > > static const struct of_device_id tc358775_of_ids[] = { > - { .compatible = "toshiba,tc358775", }, > + { .compatible = "toshiba,tc358765", .data = (void *)TC358765, }, > + { .compatible = "toshiba,tc358775", .data = (void *)TC358775, }, > { } > }; > MODULE_DEVICE_TABLE(of, tc358775_of_ids); > -- > 2.43.0
> The stby pin is optional. It is only needed for power-up and down > sequencing. It is not needed, if the power rails cannot by dynamically > enabled. > > Because the GPIO is not optional, remove the error message. I just noticed a typo: "is *now* optional. -michael
>> The tc358775 bridge is pin compatible with earlier tc358765 according to >> the tc358774xbg_datasheet_en_20190118.pdf documentation. Compared to the >> tc358765, the tc358775 supports a STBY GPIO and higher data rates. >> >> The tc358765 has a register bit for video event mode vs video pulse mode. >> We must set it to video event mode for the LCD output to work, and on the >> tc358775, this bit no longer exists. >> >> Looks like the registers seem to match otherwise based on a quick glance >> comparing the defines to the earlier Android kernel tc358765 driver. >> >> Signed-off-by: Tony Lindgren <tony@atomide.com> >> --- >> drivers/gpu/drm/bridge/tc358775.c | 26 ++++++++++++++++++++++---- >> 1 file changed, 22 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/gpu/drm/bridge/tc358775.c b/drivers/gpu/drm/bridge/tc358775.c >> --- a/drivers/gpu/drm/bridge/tc358775.c >> +++ b/drivers/gpu/drm/bridge/tc358775.c >> @@ -15,6 +15,7 @@ >> #include <linux/kernel.h> >> #include <linux/media-bus-format.h> >> #include <linux/module.h> >> +#include <linux/of_device.h> >> #include <linux/regulator/consumer.h> >> #include <linux/slab.h> >> >> @@ -107,6 +108,7 @@ >> #define RDPKTLN 0x0404 /* Command Read Packet Length */ >> >> #define VPCTRL 0x0450 /* Video Path Control */ >> +#define EVTMODE BIT(5) /* Video event mode enable, tc35876x only */ >> #define HTIM1 0x0454 /* Horizontal Timing Control 1 */ >> #define HTIM2 0x0458 /* Horizontal Timing Control 2 */ >> #define VTIM1 0x045C /* Vertical Timing Control 1 */ >> @@ -254,6 +256,11 @@ enum tc358775_ports { >> TC358775_LVDS_OUT1, >> }; >> >> +enum tc3587x5_type { >> + TC358765, > > I'd suggest adding = 1 or =0x65 so that the specified type differs > from 'no data' = 0 / NULL. > >> + TC358775, >> +}; >> + >> struct tc_data { >> struct i2c_client *i2c; >> struct device *dev; >> @@ -271,6 +278,8 @@ struct tc_data { >> struct gpio_desc *stby_gpio; >> u8 lvds_link; /* single-link or dual-link */ >> u8 bpc; >> + >> + enum tc3587x5_type type; >> }; >> >> static inline struct tc_data *bridge_to_tc(struct drm_bridge *b) >> @@ -424,10 +433,16 @@ static void tc_bridge_enable(struct drm_bridge *bridge) >> d2l_write(tc->i2c, PPI_STARTPPI, PPI_START_FUNCTION); >> d2l_write(tc->i2c, DSI_STARTDSI, DSI_RX_START); >> >> + /* Video event mode vs pulse mode bit, does not exist for tc358775 */ >> + if (tc->type == TC358765) >> + val = EVTMODE; >> + else >> + val = 0; >> + >> if (tc->bpc == 8) >> - val = TC358775_VPCTRL_OPXLFMT(1); >> + val |= TC358775_VPCTRL_OPXLFMT(1); >> else /* bpc = 6; */ >> - val = TC358775_VPCTRL_MSF(1); >> + val |= TC358775_VPCTRL_MSF(1); >> >> dsiclk = mode->crtc_clock * 3 * tc->bpc / tc->num_dsi_lanes / 1000; >> clkdiv = dsiclk / (tc->lvds_link == DUAL_LINK ? DIVIDE_BY_6 : DIVIDE_BY_3); >> @@ -643,6 +658,7 @@ static int tc_probe(struct i2c_client *client) >> >> tc->dev = dev; >> tc->i2c = client; >> + tc->type = (enum tc3587x5_type)of_device_get_match_data(dev); > > Would it make sense to use i2c_get_match_data() instead? FWIW, I' planning to add a dsi binding for this driver. So I'd suggest either the of_ or the device_ variant. Not sure though, if the new device supports the DSI commands. Otherwise, the patch looks good: Reviewed-by: Michael Walle <mwalle@kernel.org> -michael > >> >> tc->panel_bridge = devm_drm_of_get_bridge(dev, dev->of_node, >> TC358775_LVDS_OUT0, 0); >> @@ -704,13 +720,15 @@ static void tc_remove(struct i2c_client *client) >> } >> >> static const struct i2c_device_id tc358775_i2c_ids[] = { >> - { "tc358775", 0 }, >> + { "tc358765", TC358765, }, >> + { "tc358775", TC358775, }, >> { } >> }; >> MODULE_DEVICE_TABLE(i2c, tc358775_i2c_ids); >> >> static const struct of_device_id tc358775_of_ids[] = { >> - { .compatible = "toshiba,tc358775", }, >> + { .compatible = "toshiba,tc358765", .data = (void *)TC358765, }, >> + { .compatible = "toshiba,tc358775", .data = (void *)TC358775, }, >> { } >> }; >> MODULE_DEVICE_TABLE(of, tc358775_of_ids); >> -- >> 2.43.0
* Michael Walle <mwalle@kernel.org> [231204 09:52]: > >> @@ -643,6 +658,7 @@ static int tc_probe(struct i2c_client *client) > >> > >> tc->dev = dev; > >> tc->i2c = client; > >> + tc->type = (enum tc3587x5_type)of_device_get_match_data(dev); > > > > Would it make sense to use i2c_get_match_data() instead? > > FWIW, I' planning to add a dsi binding for this driver. So I'd > suggest either the of_ or the device_ variant. Not sure though, > if the new device supports the DSI commands. Yeah good point as some hardware may not have i2c wired at all. Let's keep this as of_device_get_match_data() for now as the driver is currently completely dependant on devicetree. I'll update the enumeration to use the hardware id numbering like Dmitry suggested though. Regards, Tony
Here's v2 of these changes merged with patches from Michael. Regards, Tony Changes since v1: - After a brief offline discussion with Michael, merge series with Michael's patch series to make stby gpio and supplies optional as they may be hardwired - Use Michael's better patch for the jeida timings change - Parse lanes on the bridge side like other bridge devices do, and if not found, also parse on the DSI host side and warn Michael Walle (3): dt-bindings: display: bridge: tc358775: make stby gpio and vdd supplies optional drm/bridge: tc358775: fix support for jeida-18 and jeida-24 drm/bridge: tc358775: make standby GPIO optional Tony Lindgren (7): dt-bindings: display: bridge: tc358775: Add data-lanes dt-bindings: display: bridge: tc358775: Add support for tc358765 drm/bridge: tc358775: Get bridge data lanes instead of the DSI host lanes drm/bridge: tc358775: Add burst and low-power modes drm/bridge: tc358775: Enable pre_enable_prev_first flag drm/bridge: tc358775: Add support for tc358765 drm/bridge: tc358775: Configure hs_rate and lp_rate .../display/bridge/toshiba,tc358775.yaml | 30 +++++-- drivers/gpu/drm/bridge/tc358775.c | 90 +++++++++++-------- 2 files changed, 75 insertions(+), 45 deletions(-)