| Message ID | 20240511193055.1686149-1-a-bhatia1@ti.com |
|---|---|
| Headers | show |
| Series | drm/tidss: Add OLDI bridge support | expand |
Hello Aradhya, thanks for you patch, I should be able to test your patch on my hardware in the coming days. On Sun, May 12, 2024 at 01:00:55AM +0530, Aradhya Bhatia wrote: > Up till now, the OLDI support in tidss was integrated within the tidss dispc. > This was fine till the OLDI was one-to-mapped with the DSS video-port (VP). > The AM62 and AM62P SoCs have 2 OLDI TXes that can support dual-lvds / lvds-clone > modes. > > Add OLDI TXes as separate DRM bridge entities to better support the new LVDS > configurations. > > Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com> > --- > drivers/gpu/drm/tidss/Makefile | 3 +- > drivers/gpu/drm/tidss/tidss_dispc.c | 11 +- > drivers/gpu/drm/tidss/tidss_dispc.h | 4 + > drivers/gpu/drm/tidss/tidss_drv.c | 13 +- > drivers/gpu/drm/tidss/tidss_drv.h | 4 + > drivers/gpu/drm/tidss/tidss_oldi.c | 568 ++++++++++++++++++++++++++++ > drivers/gpu/drm/tidss/tidss_oldi.h | 73 ++++ > 7 files changed, 673 insertions(+), 3 deletions(-) > create mode 100644 drivers/gpu/drm/tidss/tidss_oldi.c > create mode 100644 drivers/gpu/drm/tidss/tidss_oldi.h > > diff --git a/drivers/gpu/drm/tidss/tidss_drv.c b/drivers/gpu/drm/tidss/tidss_drv.c > index d15f836dca95..fd90e8498cc2 100644 > --- a/drivers/gpu/drm/tidss/tidss_drv.c > +++ b/drivers/gpu/drm/tidss/tidss_drv.c > @@ -23,6 +23,7 @@ > #include "tidss_drv.h" > #include "tidss_kms.h" > #include "tidss_irq.h" > +#include "tidss_oldi.h" > > /* Power management */ > > @@ -140,10 +141,17 @@ static int tidss_probe(struct platform_device *pdev) > > spin_lock_init(&tidss->wait_lock); > > + ret = tidss_oldi_init(tidss); > + if (ret) { > + if (ret != -EPROBE_DEFER) > + dev_err(dev, "failed to init OLDI (%d)\n", ret); > + return ret; > + } return dev_err_probe() > diff --git a/drivers/gpu/drm/tidss/tidss_oldi.c b/drivers/gpu/drm/tidss/tidss_oldi.c > new file mode 100644 > index 000000000000..fd96ca815542 > --- /dev/null > +++ b/drivers/gpu/drm/tidss/tidss_oldi.c > @@ -0,0 +1,568 @@ ... > + ret = drm_of_find_panel_or_bridge(child, OLDI_OURPUT_PORT, -1, > + &panel, &bridge); > + if (ret) { > + /* > + * Either there was no OLDI sink in the devicetree, or > + * the OLDI sink has not been added yet. In any case, > + * return. > + * We don't want to have an OLDI node connected to DSS > + * but not to any sink. > + */ > + if (ret != -EPROBE_DEFER) > + dev_err(tidss->dev, > + "no panel/bridge for OLDI%d. Error %d\n", > + oldi_instance, ret); just dev_err_probe > + goto err_put_node; > + } ... > + if (IS_ERR(oldi->io_ctrl)) { > + dev_err(oldi->dev, > + "%s: oldi%d syscon_regmap_lookup_by_phandle failed %ld\n", > + __func__, oldi_instance, PTR_ERR(oldi->io_ctrl)); > + ret = PTR_ERR(oldi->io_ctrl); dev_err_probe > + goto err_put_node; > + } > + > + oldi->s_clk = of_clk_get_by_name(child, "s_clk"); > + if (IS_ERR(oldi->s_clk)) { > + dev_err(oldi->dev, > + "%s: oldi%d Failed to get s_clk: %ld\n", > + __func__, oldi_instance, PTR_ERR(oldi->s_clk)); > + ret = PTR_ERR(oldi->s_clk); dev_err_probe In general, in this function, sometime you print an error and goto err_put_node, sometime you just goto err_put_node. Not sure what's the rationale on this. > + goto err_put_node; > + } > + > + /* Register the bridge. */ > + oldi->bridge.of_node = child; > + oldi->bridge.driver_private = oldi; > + oldi->bridge.funcs = &tidss_oldi_bridge_funcs; > + oldi->bridge.timings = &default_tidss_oldi_timings; > + > + tidss->oldis[tidss->num_oldis++] = oldi; > + oldi->tidss = tidss; > + > + drm_bridge_add(&oldi->bridge); > + } > + > +err_put_node: > + of_node_put(child); > + of_node_put(oldi_parent); > + return ret; > +} > diff --git a/drivers/gpu/drm/tidss/tidss_oldi.h b/drivers/gpu/drm/tidss/tidss_oldi.h > new file mode 100644 > index 000000000000..5ad02ddea11a > --- /dev/null > +++ b/drivers/gpu/drm/tidss/tidss_oldi.h > @@ -0,0 +1,73 @@ > +/* SPDX-License-Identifier: GPL-2.0-or-later */ > +/* > + * Copyright (C) 2023 - Texas Instruments Incorporated > + * > + * Aradhya Bhatia <a-bhati1@ti.com> > + */ > + > +#ifndef __TIDSS_OLDI_H__ > +#define __TIDSS_OLDI_H__ > + > +#include <linux/media-bus-format.h> > + > +#include "tidss_drv.h" > +#include "tidss_dispc.h" > + > +struct tidss_oldi; why do you need this here? > + > +/* OLDI Instances */ > +#define OLDI(n) n > + > +/* OLDI PORTS */ > +#define OLDI_INPUT_PORT 0 > +#define OLDI_OURPUT_PORT 1 > + > +/* OLDI Config Bits */ > +#define OLDI_ENABLE BIT(0) > +#define OLDI_MAP (BIT(1) | BIT(2) | BIT(3)) > +#define OLDI_SRC BIT(4) > +#define OLDI_CLONE_MODE BIT(5) > +#define OLDI_MASTERSLAVE BIT(6) > +#define OLDI_DEPOL BIT(7) > +#define OLDI_MSB BIT(8) > +#define OLDI_LBEN BIT(9) > +#define OLDI_LBDATA BIT(10) > +#define OLDI_DUALMODESYNC BIT(11) > +#define OLDI_SOFTRST BIT(12) > +#define OLDI_TPATCFG BIT(13) > + > +/* Control MMR Register */ > + > +/* Register offsets */ > +#define OLDI_PD_CTRL 0x100 > +#define OLDI_LB_CTRL 0x104 > + > +/* Power control bits */ > +#define OLDI_PWRDN_TX(n) BIT(n) > + > +/* LVDS Bandgap reference Enable/Disable */ > +#define OLDI_PWRDN_BG BIT(8) > + > +#define OLDI_IDLE_CLK_HZ 25000000 /*25 MHz */ this is used only on a single C files, move it there? I would consider this comment in general for this header file, from a quick check most of this is used only in tidss_oldi.c. Francesco
Hi Francesco, On 12/05/24 17:18, Francesco Dolcini wrote: > Hello Aradhya, thanks for you patch, I should be able to test your patch on my > hardware in the coming days. That's appreciated. Thank you! =) > > On Sun, May 12, 2024 at 01:00:55AM +0530, Aradhya Bhatia wrote: >> Up till now, the OLDI support in tidss was integrated within the tidss dispc. >> This was fine till the OLDI was one-to-mapped with the DSS video-port (VP). >> The AM62 and AM62P SoCs have 2 OLDI TXes that can support dual-lvds / lvds-clone >> modes. >> >> Add OLDI TXes as separate DRM bridge entities to better support the new LVDS >> configurations. >> >> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com> >> --- >> drivers/gpu/drm/tidss/Makefile | 3 +- >> drivers/gpu/drm/tidss/tidss_dispc.c | 11 +- >> drivers/gpu/drm/tidss/tidss_dispc.h | 4 + >> drivers/gpu/drm/tidss/tidss_drv.c | 13 +- >> drivers/gpu/drm/tidss/tidss_drv.h | 4 + >> drivers/gpu/drm/tidss/tidss_oldi.c | 568 ++++++++++++++++++++++++++++ >> drivers/gpu/drm/tidss/tidss_oldi.h | 73 ++++ >> 7 files changed, 673 insertions(+), 3 deletions(-) >> create mode 100644 drivers/gpu/drm/tidss/tidss_oldi.c >> create mode 100644 drivers/gpu/drm/tidss/tidss_oldi.h >> >> diff --git a/drivers/gpu/drm/tidss/tidss_drv.c b/drivers/gpu/drm/tidss/tidss_drv.c >> index d15f836dca95..fd90e8498cc2 100644 >> --- a/drivers/gpu/drm/tidss/tidss_drv.c >> +++ b/drivers/gpu/drm/tidss/tidss_drv.c >> @@ -23,6 +23,7 @@ >> #include "tidss_drv.h" >> #include "tidss_kms.h" >> #include "tidss_irq.h" >> +#include "tidss_oldi.h" >> >> /* Power management */ >> >> @@ -140,10 +141,17 @@ static int tidss_probe(struct platform_device *pdev) >> >> spin_lock_init(&tidss->wait_lock); >> >> + ret = tidss_oldi_init(tidss); >> + if (ret) { >> + if (ret != -EPROBE_DEFER) >> + dev_err(dev, "failed to init OLDI (%d)\n", ret); >> + return ret; >> + } > > return dev_err_probe() > >> diff --git a/drivers/gpu/drm/tidss/tidss_oldi.c b/drivers/gpu/drm/tidss/tidss_oldi.c >> new file mode 100644 >> index 000000000000..fd96ca815542 >> --- /dev/null >> +++ b/drivers/gpu/drm/tidss/tidss_oldi.c >> @@ -0,0 +1,568 @@ > > ... > >> + ret = drm_of_find_panel_or_bridge(child, OLDI_OURPUT_PORT, -1, >> + &panel, &bridge); >> + if (ret) { >> + /* >> + * Either there was no OLDI sink in the devicetree, or >> + * the OLDI sink has not been added yet. In any case, >> + * return. >> + * We don't want to have an OLDI node connected to DSS >> + * but not to any sink. >> + */ >> + if (ret != -EPROBE_DEFER) >> + dev_err(tidss->dev, >> + "no panel/bridge for OLDI%d. Error %d\n", >> + oldi_instance, ret); > > just dev_err_probe > >> + goto err_put_node; >> + } > > ... > >> + if (IS_ERR(oldi->io_ctrl)) { >> + dev_err(oldi->dev, >> + "%s: oldi%d syscon_regmap_lookup_by_phandle failed %ld\n", >> + __func__, oldi_instance, PTR_ERR(oldi->io_ctrl)); >> + ret = PTR_ERR(oldi->io_ctrl); > > dev_err_probe > >> + goto err_put_node; >> + } >> + >> + oldi->s_clk = of_clk_get_by_name(child, "s_clk"); >> + if (IS_ERR(oldi->s_clk)) { >> + dev_err(oldi->dev, >> + "%s: oldi%d Failed to get s_clk: %ld\n", >> + __func__, oldi_instance, PTR_ERR(oldi->s_clk)); >> + ret = PTR_ERR(oldi->s_clk); > > dev_err_probe Got it. Will update in all the 4 places. > > In general, in this function, sometime you print an error and goto > err_put_node, sometime you just goto err_put_node. Not sure what's the > rationale on this. There hasn't been any real logic behind the prints, except that I have added them whenever there was something (specifc) to be explained. Other times, for example, if the error is -ENOMEM, or any other systemic API failure, there isn't any print required. If this function does exit in an error, however, tidss_probe will always throw a print (except in the case of -EPROBE_DEFER). > >> + goto err_put_node; >> + } >> + >> + /* Register the bridge. */ >> + oldi->bridge.of_node = child; >> + oldi->bridge.driver_private = oldi; >> + oldi->bridge.funcs = &tidss_oldi_bridge_funcs; >> + oldi->bridge.timings = &default_tidss_oldi_timings; >> + >> + tidss->oldis[tidss->num_oldis++] = oldi; >> + oldi->tidss = tidss; >> + >> + drm_bridge_add(&oldi->bridge); >> + } >> + >> +err_put_node: >> + of_node_put(child); >> + of_node_put(oldi_parent); >> + return ret; >> +} >> diff --git a/drivers/gpu/drm/tidss/tidss_oldi.h b/drivers/gpu/drm/tidss/tidss_oldi.h >> new file mode 100644 >> index 000000000000..5ad02ddea11a >> --- /dev/null >> +++ b/drivers/gpu/drm/tidss/tidss_oldi.h >> @@ -0,0 +1,73 @@ >> +/* SPDX-License-Identifier: GPL-2.0-or-later */ >> +/* >> + * Copyright (C) 2023 - Texas Instruments Incorporated >> + * >> + * Aradhya Bhatia <a-bhati1@ti.com> >> + */ >> + >> +#ifndef __TIDSS_OLDI_H__ >> +#define __TIDSS_OLDI_H__ >> + >> +#include <linux/media-bus-format.h> >> + >> +#include "tidss_drv.h" >> +#include "tidss_dispc.h" >> + >> +struct tidss_oldi; > > why do you need this here? So that struct tidss_device can store pointers to struct tidss_oldi instances. > >> + >> +/* OLDI Instances */ >> +#define OLDI(n) n >> + >> +/* OLDI PORTS */ >> +#define OLDI_INPUT_PORT 0 >> +#define OLDI_OURPUT_PORT 1 >> + >> +/* OLDI Config Bits */ >> +#define OLDI_ENABLE BIT(0) >> +#define OLDI_MAP (BIT(1) | BIT(2) | BIT(3)) >> +#define OLDI_SRC BIT(4) >> +#define OLDI_CLONE_MODE BIT(5) >> +#define OLDI_MASTERSLAVE BIT(6) >> +#define OLDI_DEPOL BIT(7) >> +#define OLDI_MSB BIT(8) >> +#define OLDI_LBEN BIT(9) >> +#define OLDI_LBDATA BIT(10) >> +#define OLDI_DUALMODESYNC BIT(11) >> +#define OLDI_SOFTRST BIT(12) >> +#define OLDI_TPATCFG BIT(13) >> + >> +/* Control MMR Register */ >> + >> +/* Register offsets */ >> +#define OLDI_PD_CTRL 0x100 >> +#define OLDI_LB_CTRL 0x104 >> + >> +/* Power control bits */ >> +#define OLDI_PWRDN_TX(n) BIT(n) >> + >> +/* LVDS Bandgap reference Enable/Disable */ >> +#define OLDI_PWRDN_BG BIT(8) >> + >> +#define OLDI_IDLE_CLK_HZ 25000000 /*25 MHz */ > this is used only on a single C files, move it there? > > I would consider this comment in general for this header file, > from a quick check most of this is used only in tidss_oldi.c. Apart from struct tidss_device being able to access struct tidss_oldi, there is no direct access to any of the above. Perhaps I can move the idle clock definition into the C file. However, before tidss_oldi.h, all the register definitions have been stored in tidss_dispc_regs.h. It just seemed right to keep them out in the header file and maintain the status quo. Regards Aradhya
Hello Aradhya, On Sun, May 12, 2024 at 08:53:12PM +0530, Aradhya Bhatia wrote: > On 12/05/24 17:18, Francesco Dolcini wrote: > > On Sun, May 12, 2024 at 01:00:55AM +0530, Aradhya Bhatia wrote: > >> Up till now, the OLDI support in tidss was integrated within the tidss dispc. > >> This was fine till the OLDI was one-to-mapped with the DSS video-port (VP). > >> The AM62 and AM62P SoCs have 2 OLDI TXes that can support dual-lvds / lvds-clone > >> modes. > >> > >> Add OLDI TXes as separate DRM bridge entities to better support the new LVDS > >> configurations. > >> > >> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com> > >> --- > >> drivers/gpu/drm/tidss/Makefile | 3 +- > >> drivers/gpu/drm/tidss/tidss_dispc.c | 11 +- > >> drivers/gpu/drm/tidss/tidss_dispc.h | 4 + > >> drivers/gpu/drm/tidss/tidss_drv.c | 13 +- > >> drivers/gpu/drm/tidss/tidss_drv.h | 4 + > >> drivers/gpu/drm/tidss/tidss_oldi.c | 568 ++++++++++++++++++++++++++++ > >> drivers/gpu/drm/tidss/tidss_oldi.h | 73 ++++ > >> 7 files changed, 673 insertions(+), 3 deletions(-) > >> create mode 100644 drivers/gpu/drm/tidss/tidss_oldi.c > >> create mode 100644 drivers/gpu/drm/tidss/tidss_oldi.h > >> > >> diff --git a/drivers/gpu/drm/tidss/tidss_oldi.h b/drivers/gpu/drm/tidss/tidss_oldi.h > >> new file mode 100644 > >> index 000000000000..5ad02ddea11a > >> --- /dev/null > >> +++ b/drivers/gpu/drm/tidss/tidss_oldi.h > >> @@ -0,0 +1,73 @@ > >> +/* SPDX-License-Identifier: GPL-2.0-or-later */ > >> +/* > >> + * Copyright (C) 2023 - Texas Instruments Incorporated > >> + * > >> + * Aradhya Bhatia <a-bhati1@ti.com> > >> + */ > >> + > >> +#ifndef __TIDSS_OLDI_H__ > >> +#define __TIDSS_OLDI_H__ > >> + > >> +#include <linux/media-bus-format.h> > >> + > >> +#include "tidss_drv.h" > >> +#include "tidss_dispc.h" > >> + > >> +struct tidss_oldi; > > > > why do you need this here? > > So that struct tidss_device can store pointers to struct tidss_oldi > instances. on this and ... > >> +#define OLDI_IDLE_CLK_HZ 25000000 /*25 MHz */ > > this is used only on a single C files, move it there? > > > > I would consider this comment in general for this header file, > > from a quick check most of this is used only in tidss_oldi.c. > > Apart from struct tidss_device being able to access struct tidss_oldi, > there is no direct access to any of the above. > > Perhaps I can move the idle clock definition into the C file. > > However, before tidss_oldi.h, all the register definitions have been > stored in tidss_dispc_regs.h. It just seemed right to keep them out in > the header file and maintain the status quo. ... this they are probably more of a personal taste topic, just go for whatever you and the actual maintainer (tomi?) prefer. Thanks, Francesco