| Message ID | 20231202075514.44474-10-tony@atomide.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Improvments for tc358775 with support for tc358765 | expand |
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 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
Hi Tony, kernel test robot noticed the following build warnings: [auto build test WARNING on drm-misc/drm-misc-next] [also build test WARNING on linus/master v6.7-rc4 next-20231205] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Tony-Lindgren/dt-bindings-display-bridge-tc358775-make-stby-gpio-and-vdd-supplies-optional/20231202-160622 base: git://anongit.freedesktop.org/drm/drm-misc drm-misc-next patch link: https://lore.kernel.org/r/20231202075514.44474-10-tony%40atomide.com patch subject: [PATCH v2 09/10] drm/bridge: tc358775: Add support for tc358765 config: x86_64-randconfig-121-20231203 (https://download.01.org/0day-ci/archive/20231206/202312060419.B8qmgrsh-lkp@intel.com/config) compiler: clang version 16.0.4 (https://github.com/llvm/llvm-project.git ae42196bc493ffe877a7e3dff8be32035dea4d07) reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231206/202312060419.B8qmgrsh-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202312060419.B8qmgrsh-lkp@intel.com/ All warnings (new ones prefixed by >>): >> drivers/gpu/drm/bridge/tc358775.c:661:13: warning: cast to smaller integer type 'enum tc3587x5_type' from 'const void *' [-Wvoid-pointer-to-enum-cast] tc->type = (enum tc3587x5_type)of_device_get_match_data(dev); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 1 warning generated. vim +661 drivers/gpu/drm/bridge/tc358775.c 648 649 static int tc_probe(struct i2c_client *client) 650 { 651 struct device *dev = &client->dev; 652 struct tc_data *tc; 653 int ret; 654 655 tc = devm_kzalloc(dev, sizeof(*tc), GFP_KERNEL); 656 if (!tc) 657 return -ENOMEM; 658 659 tc->dev = dev; 660 tc->i2c = client; > 661 tc->type = (enum tc3587x5_type)of_device_get_match_data(dev); 662 663 tc->panel_bridge = devm_drm_of_get_bridge(dev, dev->of_node, 664 TC358775_LVDS_OUT0, 0); 665 if (IS_ERR(tc->panel_bridge)) 666 return PTR_ERR(tc->panel_bridge); 667 668 ret = tc358775_parse_dt(dev->of_node, tc); 669 if (ret) 670 return ret; 671 672 tc->vddio = devm_regulator_get(dev, "vddio-supply"); 673 if (IS_ERR(tc->vddio)) { 674 ret = PTR_ERR(tc->vddio); 675 dev_err(dev, "vddio-supply not found\n"); 676 return ret; 677 } 678 679 tc->vdd = devm_regulator_get(dev, "vdd-supply"); 680 if (IS_ERR(tc->vdd)) { 681 ret = PTR_ERR(tc->vdd); 682 dev_err(dev, "vdd-supply not found\n"); 683 return ret; 684 } 685 686 tc->stby_gpio = devm_gpiod_get_optional(dev, "stby", GPIOD_OUT_HIGH); 687 if (IS_ERR(tc->stby_gpio)) 688 return PTR_ERR(tc->stby_gpio); 689 690 tc->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH); 691 if (IS_ERR(tc->reset_gpio)) { 692 ret = PTR_ERR(tc->reset_gpio); 693 dev_err(dev, "cannot get reset-gpios %d\n", ret); 694 return ret; 695 } 696 697 tc->bridge.funcs = &tc_bridge_funcs; 698 tc->bridge.of_node = dev->of_node; 699 tc->bridge.pre_enable_prev_first = true; 700 drm_bridge_add(&tc->bridge); 701 702 i2c_set_clientdata(client, tc); 703 704 ret = tc_attach_host(tc); 705 if (ret) 706 goto err_bridge_remove; 707 708 return 0; 709 710 err_bridge_remove: 711 drm_bridge_remove(&tc->bridge); 712 return ret; 713 } 714
* 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
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, + 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); 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);
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(-)