Re: [PATCH v2 09/10] drm/bridge: tc358775: Add support for tc358765
* Michael Walle [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
Re: [PATCH v2 09/10] drm/bridge: tc358775: Add support for tc358765
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-...@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-...@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
| Closes:
https://lore.kernel.org/oe-kbuild-all/202312060419.b8qmgrsh-...@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 = >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,
664TC358775_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 = _bridge_funcs;
698 tc->bridge.of_node = dev->of_node;
699 tc->bridge.pre_enable_prev_first = true;
700 drm_bridge_add(>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(>bridge);
712 return ret;
713 }
714
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Re: [PATCH v2 09/10] drm/bridge: tc358775: Add support for tc358765
>> 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
>> ---
>> 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
>> #include
>> #include
>> +#include
>> #include
>> #include
>>
>> @@ -107,6 +108,7 @@
>> #define RDPKTLN 0x0404 /* Command Read Packet Length */
>>
>> #define VPCTRL 0x0450 /* Video Path Control */
>> +#define EVTMODEBIT(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
-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
--
With best wishes
Dmitry
Re: [PATCH v2 09/10] drm/bridge: tc358775: Add support for tc358765
On Sat, 2 Dec 2023 at 10:04, Tony Lindgren 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
> ---
> 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
> #include
> #include
> +#include
> #include
> #include
>
> @@ -107,6 +108,7 @@
> #define RDPKTLN 0x0404 /* Command Read Packet Length */
>
> #define VPCTRL 0x0450 /* Video Path Control */
> +#define EVTMODEBIT(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
--
With best wishes
Dmitry
