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
[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,
+ 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);
--
2.43.0
