| Message ID | 1573660292-10629-7-git-send-email-fabrizio.castro@bp.renesas.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series | Add LCD panel support to iwg20d | expand |
Hi Fabrizio, Thank you for the patch. On Wed, Nov 13, 2019 at 03:51:25PM +0000, Fabrizio Castro wrote: > Add support for transparent LVDS decoders by adding a new > compatible string ("lvds-decoder") to the driver. > This patch also adds member connector_type to struct lvds_codec, > and that's because LVDS decoders have a different connector type > from LVDS encoders. We fill this new member up with the data > matching the compatible string. > > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > v3->v4: > * New patch > --- > drivers/gpu/drm/bridge/lvds-codec.c | 19 ++++++++++++++++--- > 1 file changed, 16 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/lvds-codec.c b/drivers/gpu/drm/bridge/lvds-codec.c > index b5801a2..c32e125 100644 > --- a/drivers/gpu/drm/bridge/lvds-codec.c > +++ b/drivers/gpu/drm/bridge/lvds-codec.c > @@ -7,6 +7,7 @@ > #include <linux/gpio/consumer.h> > #include <linux/module.h> > #include <linux/of.h> > +#include <linux/of_device.h> > #include <linux/of_graph.h> > #include <linux/platform_device.h> > > @@ -17,6 +18,7 @@ struct lvds_codec { > struct drm_bridge bridge; > struct drm_bridge *panel_bridge; > struct gpio_desc *powerdown_gpio; > + u32 connector_type; > }; > > static int lvds_codec_attach(struct drm_bridge *bridge) > @@ -65,6 +67,7 @@ static int lvds_codec_probe(struct platform_device *pdev) > if (!lvds_codec) > return -ENOMEM; > > + lvds_codec->connector_type = (u32)of_device_get_match_data(&pdev->dev); > lvds_codec->powerdown_gpio = devm_gpiod_get_optional(dev, "powerdown", > GPIOD_OUT_HIGH); > if (IS_ERR(lvds_codec->powerdown_gpio)) { > @@ -105,7 +108,7 @@ static int lvds_codec_probe(struct platform_device *pdev) > > lvds_codec->panel_bridge = > devm_drm_panel_bridge_add_typed(dev, panel, > - DRM_MODE_CONNECTOR_LVDS); > + lvds_codec->connector_type); > if (IS_ERR(lvds_codec->panel_bridge)) > return PTR_ERR(lvds_codec->panel_bridge); > > @@ -133,8 +136,18 @@ static int lvds_codec_remove(struct platform_device *pdev) > } > > static const struct of_device_id lvds_codec_match[] = { > - { .compatible = "lvds-encoder" }, > - { .compatible = "thine,thc63lvdm83d" }, > + { > + .compatible = "lvds-decoder", > + .data = (void *)DRM_MODE_CONNECTOR_DPI, > + }, > + { > + .compatible = "lvds-encoder", > + .data = (void *)DRM_MODE_CONNECTOR_LVDS, > + }, > + { > + .compatible = "thine,thc63lvdm83d", > + .data = (void *)DRM_MODE_CONNECTOR_LVDS, > + }, > {}, > }; > MODULE_DEVICE_TABLE(of, lvds_codec_match);
Hi Fabrizio, On Wed, Nov 13, 2019 at 03:51:25PM +0000, Fabrizio Castro wrote: > Add support for transparent LVDS decoders by adding a new > compatible string ("lvds-decoder") to the driver. > This patch also adds member connector_type to struct lvds_codec, > and that's because LVDS decoders have a different connector type > from LVDS encoders. We fill this new member up with the data > matching the compatible string. > > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com> > > --- > v3->v4: > * New patch > --- > drivers/gpu/drm/bridge/lvds-codec.c | 19 ++++++++++++++++--- > 1 file changed, 16 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/lvds-codec.c b/drivers/gpu/drm/bridge/lvds-codec.c > index b5801a2..c32e125 100644 > --- a/drivers/gpu/drm/bridge/lvds-codec.c > +++ b/drivers/gpu/drm/bridge/lvds-codec.c > @@ -7,6 +7,7 @@ > #include <linux/gpio/consumer.h> > #include <linux/module.h> > #include <linux/of.h> > +#include <linux/of_device.h> > #include <linux/of_graph.h> > #include <linux/platform_device.h> > > @@ -17,6 +18,7 @@ struct lvds_codec { > struct drm_bridge bridge; > struct drm_bridge *panel_bridge; > struct gpio_desc *powerdown_gpio; > + u32 connector_type; > }; > > static int lvds_codec_attach(struct drm_bridge *bridge) > @@ -65,6 +67,7 @@ static int lvds_codec_probe(struct platform_device *pdev) > if (!lvds_codec) > return -ENOMEM; > > + lvds_codec->connector_type = (u32)of_device_get_match_data(&pdev->dev); I'm now getting a compilation failure here: drivers/gpu/drm/bridge/lvds-codec.c: In function ‘lvds_codec_probe’: drivers/gpu/drm/bridge/lvds-codec.c:68:31: error: cast from pointer to integer of different size [-Werror=pointer-to-int-cast] lvds_codec->connector_type = (u32)of_device_get_match_data(&pdev->dev); ^ The fix should be simple: lvds_codec->connector_type = (uintptr_t)of_device_get_match_data(dev); I'm bothered by the fact that I've compiled this before without any issue, so this really puzzles me. Do you get the same warning ? > lvds_codec->powerdown_gpio = devm_gpiod_get_optional(dev, "powerdown", > GPIOD_OUT_HIGH); > if (IS_ERR(lvds_codec->powerdown_gpio)) { > @@ -105,7 +108,7 @@ static int lvds_codec_probe(struct platform_device *pdev) > > lvds_codec->panel_bridge = > devm_drm_panel_bridge_add_typed(dev, panel, > - DRM_MODE_CONNECTOR_LVDS); > + lvds_codec->connector_type); > if (IS_ERR(lvds_codec->panel_bridge)) > return PTR_ERR(lvds_codec->panel_bridge); > > @@ -133,8 +136,18 @@ static int lvds_codec_remove(struct platform_device *pdev) > } > > static const struct of_device_id lvds_codec_match[] = { > - { .compatible = "lvds-encoder" }, > - { .compatible = "thine,thc63lvdm83d" }, > + { > + .compatible = "lvds-decoder", > + .data = (void *)DRM_MODE_CONNECTOR_DPI, > + }, > + { > + .compatible = "lvds-encoder", > + .data = (void *)DRM_MODE_CONNECTOR_LVDS, > + }, > + { > + .compatible = "thine,thc63lvdm83d", > + .data = (void *)DRM_MODE_CONNECTOR_LVDS, > + }, > {}, > }; > MODULE_DEVICE_TABLE(of, lvds_codec_match);
Hi Fabrizio, Ping ? On Fri, Dec 13, 2019 at 07:10:38PM +0200, Laurent Pinchart wrote: > On Wed, Nov 13, 2019 at 03:51:25PM +0000, Fabrizio Castro wrote: > > Add support for transparent LVDS decoders by adding a new > > compatible string ("lvds-decoder") to the driver. > > This patch also adds member connector_type to struct lvds_codec, > > and that's because LVDS decoders have a different connector type > > from LVDS encoders. We fill this new member up with the data > > matching the compatible string. > > > > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com> > > > > --- > > v3->v4: > > * New patch > > --- > > drivers/gpu/drm/bridge/lvds-codec.c | 19 ++++++++++++++++--- > > 1 file changed, 16 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/gpu/drm/bridge/lvds-codec.c b/drivers/gpu/drm/bridge/lvds-codec.c > > index b5801a2..c32e125 100644 > > --- a/drivers/gpu/drm/bridge/lvds-codec.c > > +++ b/drivers/gpu/drm/bridge/lvds-codec.c > > @@ -7,6 +7,7 @@ > > #include <linux/gpio/consumer.h> > > #include <linux/module.h> > > #include <linux/of.h> > > +#include <linux/of_device.h> > > #include <linux/of_graph.h> > > #include <linux/platform_device.h> > > > > @@ -17,6 +18,7 @@ struct lvds_codec { > > struct drm_bridge bridge; > > struct drm_bridge *panel_bridge; > > struct gpio_desc *powerdown_gpio; > > + u32 connector_type; > > }; > > > > static int lvds_codec_attach(struct drm_bridge *bridge) > > @@ -65,6 +67,7 @@ static int lvds_codec_probe(struct platform_device *pdev) > > if (!lvds_codec) > > return -ENOMEM; > > > > + lvds_codec->connector_type = (u32)of_device_get_match_data(&pdev->dev); > > I'm now getting a compilation failure here: > > drivers/gpu/drm/bridge/lvds-codec.c: In function ‘lvds_codec_probe’: > drivers/gpu/drm/bridge/lvds-codec.c:68:31: error: cast from pointer to integer of different size [-Werror=pointer-to-int-cast] > lvds_codec->connector_type = (u32)of_device_get_match_data(&pdev->dev); > ^ > > The fix should be simple: > > lvds_codec->connector_type = (uintptr_t)of_device_get_match_data(dev); > > I'm bothered by the fact that I've compiled this before without any > issue, so this really puzzles me. Do you get the same warning ? > > > lvds_codec->powerdown_gpio = devm_gpiod_get_optional(dev, "powerdown", > > GPIOD_OUT_HIGH); > > if (IS_ERR(lvds_codec->powerdown_gpio)) { > > @@ -105,7 +108,7 @@ static int lvds_codec_probe(struct platform_device *pdev) > > > > lvds_codec->panel_bridge = > > devm_drm_panel_bridge_add_typed(dev, panel, > > - DRM_MODE_CONNECTOR_LVDS); > > + lvds_codec->connector_type); > > if (IS_ERR(lvds_codec->panel_bridge)) > > return PTR_ERR(lvds_codec->panel_bridge); > > > > @@ -133,8 +136,18 @@ static int lvds_codec_remove(struct platform_device *pdev) > > } > > > > static const struct of_device_id lvds_codec_match[] = { > > - { .compatible = "lvds-encoder" }, > > - { .compatible = "thine,thc63lvdm83d" }, > > + { > > + .compatible = "lvds-decoder", > > + .data = (void *)DRM_MODE_CONNECTOR_DPI, > > + }, > > + { > > + .compatible = "lvds-encoder", > > + .data = (void *)DRM_MODE_CONNECTOR_LVDS, > > + }, > > + { > > + .compatible = "thine,thc63lvdm83d", > > + .data = (void *)DRM_MODE_CONNECTOR_LVDS, > > + }, > > {}, > > }; > > MODULE_DEVICE_TABLE(of, lvds_codec_match);
Hi Laurent, Thank you for your feedback! > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Sent: 13 December 2019 17:11 > Subject: Re: [PATCH v4 06/13] drm/bridge: lvds-codec: Add "lvds-decoder" support > > Hi Fabrizio, > > On Wed, Nov 13, 2019 at 03:51:25PM +0000, Fabrizio Castro wrote: > > Add support for transparent LVDS decoders by adding a new > > compatible string ("lvds-decoder") to the driver. > > This patch also adds member connector_type to struct lvds_codec, > > and that's because LVDS decoders have a different connector type > > from LVDS encoders. We fill this new member up with the data > > matching the compatible string. > > > > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com> > > > > --- > > v3->v4: > > * New patch > > --- > > drivers/gpu/drm/bridge/lvds-codec.c | 19 ++++++++++++++++--- > > 1 file changed, 16 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/gpu/drm/bridge/lvds-codec.c b/drivers/gpu/drm/bridge/lvds-codec.c > > index b5801a2..c32e125 100644 > > --- a/drivers/gpu/drm/bridge/lvds-codec.c > > +++ b/drivers/gpu/drm/bridge/lvds-codec.c > > @@ -7,6 +7,7 @@ > > #include <linux/gpio/consumer.h> > > #include <linux/module.h> > > #include <linux/of.h> > > +#include <linux/of_device.h> > > #include <linux/of_graph.h> > > #include <linux/platform_device.h> > > > > @@ -17,6 +18,7 @@ struct lvds_codec { > > struct drm_bridge bridge; > > struct drm_bridge *panel_bridge; > > struct gpio_desc *powerdown_gpio; > > + u32 connector_type; > > }; > > > > static int lvds_codec_attach(struct drm_bridge *bridge) > > @@ -65,6 +67,7 @@ static int lvds_codec_probe(struct platform_device *pdev) > > if (!lvds_codec) > > return -ENOMEM; > > > > + lvds_codec->connector_type = (u32)of_device_get_match_data(&pdev->dev); > > I'm now getting a compilation failure here: > > drivers/gpu/drm/bridge/lvds-codec.c: In function ‘lvds_codec_probe’: > drivers/gpu/drm/bridge/lvds-codec.c:68:31: error: cast from pointer to integer of different size [-Werror=pointer-to-int-cast] > lvds_codec->connector_type = (u32)of_device_get_match_data(&pdev->dev); > ^ > > The fix should be simple: > > lvds_codec->connector_type = (uintptr_t)of_device_get_match_data(dev); > > I'm bothered by the fact that I've compiled this before without any > issue, so this really puzzles me. Do you get the same warning ? The warning appears when compiling for arm64, understandably so. We must have compiled this for arm only the first time around. I think the right way to solve this is to either cast to (u32)(uintptr_t) or (u32)(unsigned long). What's your preference? Thanks, Fab > > > lvds_codec->powerdown_gpio = devm_gpiod_get_optional(dev, "powerdown", > > GPIOD_OUT_HIGH); > > if (IS_ERR(lvds_codec->powerdown_gpio)) { > > @@ -105,7 +108,7 @@ static int lvds_codec_probe(struct platform_device *pdev) > > > > lvds_codec->panel_bridge = > > devm_drm_panel_bridge_add_typed(dev, panel, > > - DRM_MODE_CONNECTOR_LVDS); > > + lvds_codec->connector_type); > > if (IS_ERR(lvds_codec->panel_bridge)) > > return PTR_ERR(lvds_codec->panel_bridge); > > > > @@ -133,8 +136,18 @@ static int lvds_codec_remove(struct platform_device *pdev) > > } > > > > static const struct of_device_id lvds_codec_match[] = { > > - { .compatible = "lvds-encoder" }, > > - { .compatible = "thine,thc63lvdm83d" }, > > + { > > + .compatible = "lvds-decoder", > > + .data = (void *)DRM_MODE_CONNECTOR_DPI, > > + }, > > + { > > + .compatible = "lvds-encoder", > > + .data = (void *)DRM_MODE_CONNECTOR_LVDS, > > + }, > > + { > > + .compatible = "thine,thc63lvdm83d", > > + .data = (void *)DRM_MODE_CONNECTOR_LVDS, > > + }, > > {}, > > }; > > MODULE_DEVICE_TABLE(of, lvds_codec_match); > > -- > Regards, > > Laurent Pinchart
Hi Fabrizio, On Tue, Dec 17, 2019 at 12:03 PM Fabrizio Castro <fabrizio.castro@bp.renesas.com> wrote: > > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Sent: 13 December 2019 17:11 > > Subject: Re: [PATCH v4 06/13] drm/bridge: lvds-codec: Add "lvds-decoder" support > > > > On Wed, Nov 13, 2019 at 03:51:25PM +0000, Fabrizio Castro wrote: > > > Add support for transparent LVDS decoders by adding a new > > > compatible string ("lvds-decoder") to the driver. > > > This patch also adds member connector_type to struct lvds_codec, > > > and that's because LVDS decoders have a different connector type > > > from LVDS encoders. We fill this new member up with the data > > > matching the compatible string. > > > > > > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com> > > > > > > --- > > > v3->v4: > > > * New patch > > > --- > > > drivers/gpu/drm/bridge/lvds-codec.c | 19 ++++++++++++++++--- > > > 1 file changed, 16 insertions(+), 3 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/bridge/lvds-codec.c b/drivers/gpu/drm/bridge/lvds-codec.c > > > index b5801a2..c32e125 100644 > > > --- a/drivers/gpu/drm/bridge/lvds-codec.c > > > +++ b/drivers/gpu/drm/bridge/lvds-codec.c > > > @@ -7,6 +7,7 @@ > > > #include <linux/gpio/consumer.h> > > > #include <linux/module.h> > > > #include <linux/of.h> > > > +#include <linux/of_device.h> > > > #include <linux/of_graph.h> > > > #include <linux/platform_device.h> > > > > > > @@ -17,6 +18,7 @@ struct lvds_codec { > > > struct drm_bridge bridge; > > > struct drm_bridge *panel_bridge; > > > struct gpio_desc *powerdown_gpio; > > > + u32 connector_type; > > > }; > > > > > > static int lvds_codec_attach(struct drm_bridge *bridge) > > > @@ -65,6 +67,7 @@ static int lvds_codec_probe(struct platform_device *pdev) > > > if (!lvds_codec) > > > return -ENOMEM; > > > > > > + lvds_codec->connector_type = (u32)of_device_get_match_data(&pdev->dev); > > > > I'm now getting a compilation failure here: > > > > drivers/gpu/drm/bridge/lvds-codec.c: In function ‘lvds_codec_probe’: > > drivers/gpu/drm/bridge/lvds-codec.c:68:31: error: cast from pointer to integer of different size [-Werror=pointer-to-int-cast] > > lvds_codec->connector_type = (u32)of_device_get_match_data(&pdev->dev); > > ^ > > > > The fix should be simple: > > > > lvds_codec->connector_type = (uintptr_t)of_device_get_match_data(dev); > > > > I'm bothered by the fact that I've compiled this before without any > > issue, so this really puzzles me. Do you get the same warning ? > > The warning appears when compiling for arm64, understandably so. > We must have compiled this for arm only the first time around. > > I think the right way to solve this is to either cast to (u32)(uintptr_t) or (u32)(unsigned long). Just casting to uintptr_t should be sufficient. Gr{oetje,eeting}s, Geert
Hi Geert, Thank you for your feedback! > From: linux-renesas-soc-owner@vger.kernel.org <linux-renesas-soc-owner@vger.kernel.org> On Behalf Of Geert Uytterhoeven > Sent: 17 December 2019 12:21 > Subject: Re: [PATCH v4 06/13] drm/bridge: lvds-codec: Add "lvds-decoder" support > > Hi Fabrizio, > > On Tue, Dec 17, 2019 at 12:03 PM Fabrizio Castro > <fabrizio.castro@bp.renesas.com> wrote: > > > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > Sent: 13 December 2019 17:11 > > > Subject: Re: [PATCH v4 06/13] drm/bridge: lvds-codec: Add "lvds-decoder" support > > > > > > On Wed, Nov 13, 2019 at 03:51:25PM +0000, Fabrizio Castro wrote: > > > > Add support for transparent LVDS decoders by adding a new > > > > compatible string ("lvds-decoder") to the driver. > > > > This patch also adds member connector_type to struct lvds_codec, > > > > and that's because LVDS decoders have a different connector type > > > > from LVDS encoders. We fill this new member up with the data > > > > matching the compatible string. > > > > > > > > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com> > > > > > > > > --- > > > > v3->v4: > > > > * New patch > > > > --- > > > > drivers/gpu/drm/bridge/lvds-codec.c | 19 ++++++++++++++++--- > > > > 1 file changed, 16 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/bridge/lvds-codec.c b/drivers/gpu/drm/bridge/lvds-codec.c > > > > index b5801a2..c32e125 100644 > > > > --- a/drivers/gpu/drm/bridge/lvds-codec.c > > > > +++ b/drivers/gpu/drm/bridge/lvds-codec.c > > > > @@ -7,6 +7,7 @@ > > > > #include <linux/gpio/consumer.h> > > > > #include <linux/module.h> > > > > #include <linux/of.h> > > > > +#include <linux/of_device.h> > > > > #include <linux/of_graph.h> > > > > #include <linux/platform_device.h> > > > > > > > > @@ -17,6 +18,7 @@ struct lvds_codec { > > > > struct drm_bridge bridge; > > > > struct drm_bridge *panel_bridge; > > > > struct gpio_desc *powerdown_gpio; > > > > + u32 connector_type; > > > > }; > > > > > > > > static int lvds_codec_attach(struct drm_bridge *bridge) > > > > @@ -65,6 +67,7 @@ static int lvds_codec_probe(struct platform_device *pdev) > > > > if (!lvds_codec) > > > > return -ENOMEM; > > > > > > > > + lvds_codec->connector_type = (u32)of_device_get_match_data(&pdev->dev); > > > > > > I'm now getting a compilation failure here: > > > > > > drivers/gpu/drm/bridge/lvds-codec.c: In function ‘lvds_codec_probe’: > > > drivers/gpu/drm/bridge/lvds-codec.c:68:31: error: cast from pointer to integer of different size [-Werror=pointer-to-int-cast] > > > lvds_codec->connector_type = (u32)of_device_get_match_data(&pdev->dev); > > > ^ > > > > > > The fix should be simple: > > > > > > lvds_codec->connector_type = (uintptr_t)of_device_get_match_data(dev); > > > > > > I'm bothered by the fact that I've compiled this before without any > > > issue, so this really puzzles me. Do you get the same warning ? > > > > The warning appears when compiling for arm64, understandably so. > > We must have compiled this for arm only the first time around. > > > > I think the right way to solve this is to either cast to (u32)(uintptr_t) or (u32)(unsigned long). > > Just casting to uintptr_t should be sufficient. It should be sufficient for the compiler, but I have seen examples where people preferred to be explicit, like in: drivers/mailbox/mtk-cmdq-mailbox.c drivers/leds/leds-pm8058.c Since the kernel is increasing its tightness with respect to warnings, I personally prefer (u32)(uintptr_t), even though not strictly necessary, but I am fine with (uintptr_t) if you don't like (u32)(uintptr_t). Cheers, Fab > > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org > > In personal conversations with technical people, I call myself a hacker. But > when I'm talking to journalists I just say "programmer" or something like that. > -- Linus Torvalds
Hi Fabrizio, On Tue, Dec 17, 2019 at 1:31 PM Fabrizio Castro <fabrizio.castro@bp.renesas.com> wrote: > > From: linux-renesas-soc-owner@vger.kernel.org <linux-renesas-soc-owner@vger.kernel.org> On Behalf Of Geert Uytterhoeven > > Sent: 17 December 2019 12:21 > > Subject: Re: [PATCH v4 06/13] drm/bridge: lvds-codec: Add "lvds-decoder" support > > > > On Tue, Dec 17, 2019 at 12:03 PM Fabrizio Castro > > <fabrizio.castro@bp.renesas.com> wrote: > > > > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > Sent: 13 December 2019 17:11 > > > > Subject: Re: [PATCH v4 06/13] drm/bridge: lvds-codec: Add "lvds-decoder" support > > > > > > > > On Wed, Nov 13, 2019 at 03:51:25PM +0000, Fabrizio Castro wrote: > > > > > Add support for transparent LVDS decoders by adding a new > > > > > compatible string ("lvds-decoder") to the driver. > > > > > This patch also adds member connector_type to struct lvds_codec, > > > > > and that's because LVDS decoders have a different connector type > > > > > from LVDS encoders. We fill this new member up with the data > > > > > matching the compatible string. > > > > > > > > > > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com> > > > > > --- a/drivers/gpu/drm/bridge/lvds-codec.c > > > > > +++ b/drivers/gpu/drm/bridge/lvds-codec.c > > > > > @@ -65,6 +67,7 @@ static int lvds_codec_probe(struct platform_device *pdev) > > > > > if (!lvds_codec) > > > > > return -ENOMEM; > > > > > > > > > > + lvds_codec->connector_type = (u32)of_device_get_match_data(&pdev->dev); > > > > > > > > I'm now getting a compilation failure here: > > > > > > > > drivers/gpu/drm/bridge/lvds-codec.c: In function ‘lvds_codec_probe’: > > > > drivers/gpu/drm/bridge/lvds-codec.c:68:31: error: cast from pointer to integer of different size [-Werror=pointer-to-int-cast] > > > > lvds_codec->connector_type = (u32)of_device_get_match_data(&pdev->dev); > > > > ^ > > > > > > > > The fix should be simple: > > > > > > > > lvds_codec->connector_type = (uintptr_t)of_device_get_match_data(dev); > > > > > > > > I'm bothered by the fact that I've compiled this before without any > > > > issue, so this really puzzles me. Do you get the same warning ? > > > > > > The warning appears when compiling for arm64, understandably so. > > > We must have compiled this for arm only the first time around. > > > > > > I think the right way to solve this is to either cast to (u32)(uintptr_t) or (u32)(unsigned long). > > > > Just casting to uintptr_t should be sufficient. > > It should be sufficient for the compiler, but I have seen examples where people > preferred to be explicit, like in: > drivers/mailbox/mtk-cmdq-mailbox.c > drivers/leds/leds-pm8058.c > > Since the kernel is increasing its tightness with respect to warnings, I personally prefer > (u32)(uintptr_t), even though not strictly necessary, but I am fine with (uintptr_t) if you > don't like (u32)(uintptr_t). It depends. I try to have as few casts as possible ("casts are evil"). While adding the extra (u32) cast makes it clearer that the intended result is a u32 (for now), it will cause silent truncation on 64-bit if connector_type is ever enlarged to unsigned long, and larger values are used. In this particular case this is unlikely, though, as unsigned long would still be 32-bit on 32-bit platforms, so the larger values cannot be used. Gr{oetje,eeting}s, Geert
Hello, On Tue, Dec 17, 2019 at 01:38:51PM +0100, Geert Uytterhoeven wrote: > On Tue, Dec 17, 2019 at 1:31 PM Fabrizio Castro wrote: > > On 17 December 2019 12:21, Geert Uytterhoeven wrote: > >> On Tue, Dec 17, 2019 at 12:03 PM Fabrizio Castro wrote: > >>> On 13 December 2019 17:11, Laurent Pinchart wrote: > >>>> On Wed, Nov 13, 2019 at 03:51:25PM +0000, Fabrizio Castro wrote: > >>>>> Add support for transparent LVDS decoders by adding a new > >>>>> compatible string ("lvds-decoder") to the driver. > >>>>> This patch also adds member connector_type to struct lvds_codec, > >>>>> and that's because LVDS decoders have a different connector type > >>>>> from LVDS encoders. We fill this new member up with the data > >>>>> matching the compatible string. > >>>>> > >>>>> Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com> > >>>>> > >>>>> --- a/drivers/gpu/drm/bridge/lvds-codec.c > >>>>> +++ b/drivers/gpu/drm/bridge/lvds-codec.c > >>>>> > >>>>> @@ -65,6 +67,7 @@ static int lvds_codec_probe(struct platform_device *pdev) > >>>>> if (!lvds_codec) > >>>>> return -ENOMEM; > >>>>> > >>>>> + lvds_codec->connector_type = (u32)of_device_get_match_data(&pdev->dev); > >>>> > >>>> I'm now getting a compilation failure here: > >>>> > >>>> drivers/gpu/drm/bridge/lvds-codec.c: In function ‘lvds_codec_probe’: > >>>> drivers/gpu/drm/bridge/lvds-codec.c:68:31: error: cast from pointer to integer of different size [-Werror=pointer-to-int-cast] > >>>> lvds_codec->connector_type = (u32)of_device_get_match_data(&pdev->dev); > >>>> ^ > >>>> > >>>> The fix should be simple: > >>>> > >>>> lvds_codec->connector_type = (uintptr_t)of_device_get_match_data(dev); > >>>> > >>>> I'm bothered by the fact that I've compiled this before without any > >>>> issue, so this really puzzles me. Do you get the same warning ? > >>> > >>> The warning appears when compiling for arm64, understandably so. > >>> We must have compiled this for arm only the first time around. > >>> > >>> I think the right way to solve this is to either cast to (u32)(uintptr_t) or (u32)(unsigned long). > >> > >> Just casting to uintptr_t should be sufficient. > > > > It should be sufficient for the compiler, but I have seen examples where people > > preferred to be explicit, like in: > > drivers/mailbox/mtk-cmdq-mailbox.c > > drivers/leds/leds-pm8058.c > > > > Since the kernel is increasing its tightness with respect to warnings, I personally prefer > > (u32)(uintptr_t), even though not strictly necessary, but I am fine with (uintptr_t) if you > > don't like (u32)(uintptr_t). > > It depends. I try to have as few casts as possible ("casts are evil"). > > While adding the extra (u32) cast makes it clearer that the intended > result is a u32 (for now), it will cause silent truncation on 64-bit if > connector_type is ever enlarged to unsigned long, and larger values are > used. > > In this particular case this is unlikely, though, as unsigned long would > still be 32-bit on 32-bit platforms, so the larger values cannot be > used. I also try to add as few casts as possible, so (uintptr_t) would be my preference. Fabrizio, could you submit a new version of this patch with the problem fixed (and with the casts you decide to use, but using uintptr_t instead of unsigned long in any case) ?
Hi guys, > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Sent: 17 December 2019 13:54 > Subject: Re: [PATCH v4 06/13] drm/bridge: lvds-codec: Add "lvds-decoder" support > > Hello, > > On Tue, Dec 17, 2019 at 01:38:51PM +0100, Geert Uytterhoeven wrote: > > On Tue, Dec 17, 2019 at 1:31 PM Fabrizio Castro wrote: > > > On 17 December 2019 12:21, Geert Uytterhoeven wrote: > > >> On Tue, Dec 17, 2019 at 12:03 PM Fabrizio Castro wrote: > > >>> On 13 December 2019 17:11, Laurent Pinchart wrote: > > >>>> On Wed, Nov 13, 2019 at 03:51:25PM +0000, Fabrizio Castro wrote: > > >>>>> Add support for transparent LVDS decoders by adding a new > > >>>>> compatible string ("lvds-decoder") to the driver. > > >>>>> This patch also adds member connector_type to struct lvds_codec, > > >>>>> and that's because LVDS decoders have a different connector type > > >>>>> from LVDS encoders. We fill this new member up with the data > > >>>>> matching the compatible string. > > >>>>> > > >>>>> Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com> > > >>>>> > > >>>>> --- a/drivers/gpu/drm/bridge/lvds-codec.c > > >>>>> +++ b/drivers/gpu/drm/bridge/lvds-codec.c > > >>>>> > > >>>>> @@ -65,6 +67,7 @@ static int lvds_codec_probe(struct platform_device *pdev) > > >>>>> if (!lvds_codec) > > >>>>> return -ENOMEM; > > >>>>> > > >>>>> + lvds_codec->connector_type = (u32)of_device_get_match_data(&pdev->dev); > > >>>> > > >>>> I'm now getting a compilation failure here: > > >>>> > > >>>> drivers/gpu/drm/bridge/lvds-codec.c: In function ‘lvds_codec_probe’: > > >>>> drivers/gpu/drm/bridge/lvds-codec.c:68:31: error: cast from pointer to integer of different size [-Werror=pointer-to-int-cast] > > >>>> lvds_codec->connector_type = (u32)of_device_get_match_data(&pdev->dev); > > >>>> ^ > > >>>> > > >>>> The fix should be simple: > > >>>> > > >>>> lvds_codec->connector_type = (uintptr_t)of_device_get_match_data(dev); > > >>>> > > >>>> I'm bothered by the fact that I've compiled this before without any > > >>>> issue, so this really puzzles me. Do you get the same warning ? > > >>> > > >>> The warning appears when compiling for arm64, understandably so. > > >>> We must have compiled this for arm only the first time around. > > >>> > > >>> I think the right way to solve this is to either cast to (u32)(uintptr_t) or (u32)(unsigned long). > > >> > > >> Just casting to uintptr_t should be sufficient. > > > > > > It should be sufficient for the compiler, but I have seen examples where people > > > preferred to be explicit, like in: > > > drivers/mailbox/mtk-cmdq-mailbox.c > > > drivers/leds/leds-pm8058.c > > > > > > Since the kernel is increasing its tightness with respect to warnings, I personally prefer > > > (u32)(uintptr_t), even though not strictly necessary, but I am fine with (uintptr_t) if you > > > don't like (u32)(uintptr_t). > > > > It depends. I try to have as few casts as possible ("casts are evil"). > > > > While adding the extra (u32) cast makes it clearer that the intended > > result is a u32 (for now), it will cause silent truncation on 64-bit if > > connector_type is ever enlarged to unsigned long, and larger values are > > used. > > > > In this particular case this is unlikely, though, as unsigned long would > > still be 32-bit on 32-bit platforms, so the larger values cannot be > > used. > > I also try to add as few casts as possible, so (uintptr_t) would be my > preference. > > Fabrizio, could you submit a new version of this patch with the problem > fixed (and with the casts you decide to use, but using uintptr_t instead > of unsigned long in any case) ? Will send a new version tomorrow with (uintptr_t) Thanks, Fab > > -- > Regards, > > Laurent Pinchart
On Tue, Dec 17, 2019 at 10:06:14PM +0000, Fabrizio Castro wrote: > On 17 December 2019 13:54, Laurent Pinchart wrote: > > On Tue, Dec 17, 2019 at 01:38:51PM +0100, Geert Uytterhoeven wrote: > >> On Tue, Dec 17, 2019 at 1:31 PM Fabrizio Castro wrote: > >>> On 17 December 2019 12:21, Geert Uytterhoeven wrote: > >>>> On Tue, Dec 17, 2019 at 12:03 PM Fabrizio Castro wrote: > >>>>> On 13 December 2019 17:11, Laurent Pinchart wrote: > >>>>>> On Wed, Nov 13, 2019 at 03:51:25PM +0000, Fabrizio Castro wrote: > >>>>>>> Add support for transparent LVDS decoders by adding a new > >>>>>>> compatible string ("lvds-decoder") to the driver. > >>>>>>> This patch also adds member connector_type to struct lvds_codec, > >>>>>>> and that's because LVDS decoders have a different connector type > >>>>>>> from LVDS encoders. We fill this new member up with the data > >>>>>>> matching the compatible string. > >>>>>>> > >>>>>>> Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com> > >>>>>>> > >>>>>>> --- a/drivers/gpu/drm/bridge/lvds-codec.c > >>>>>>> +++ b/drivers/gpu/drm/bridge/lvds-codec.c > >>>>>>> > >>>>>>> @@ -65,6 +67,7 @@ static int lvds_codec_probe(struct platform_device *pdev) > >>>>>>> if (!lvds_codec) > >>>>>>> return -ENOMEM; > >>>>>>> > >>>>>>> + lvds_codec->connector_type = (u32)of_device_get_match_data(&pdev->dev); > >>>>>> > >>>>>> I'm now getting a compilation failure here: > >>>>>> > >>>>>> drivers/gpu/drm/bridge/lvds-codec.c: In function ‘lvds_codec_probe’: > >>>>>> drivers/gpu/drm/bridge/lvds-codec.c:68:31: error: cast from pointer to integer of different size [-Werror=pointer-to-int-cast] > >>>>>> lvds_codec->connector_type = (u32)of_device_get_match_data(&pdev->dev); > >>>>>> ^ > >>>>>> > >>>>>> The fix should be simple: > >>>>>> > >>>>>> lvds_codec->connector_type = (uintptr_t)of_device_get_match_data(dev); > >>>>>> > >>>>>> I'm bothered by the fact that I've compiled this before without any > >>>>>> issue, so this really puzzles me. Do you get the same warning ? > >>>>> > >>>>> The warning appears when compiling for arm64, understandably so. > >>>>> We must have compiled this for arm only the first time around. > >>>>> > >>>>> I think the right way to solve this is to either cast to (u32)(uintptr_t) or (u32)(unsigned long). > >>>> > >>>> Just casting to uintptr_t should be sufficient. > >>> > >>> It should be sufficient for the compiler, but I have seen examples where people > >>> preferred to be explicit, like in: > >>> drivers/mailbox/mtk-cmdq-mailbox.c > >>> drivers/leds/leds-pm8058.c > >>> > >>> Since the kernel is increasing its tightness with respect to warnings, I personally prefer > >>> (u32)(uintptr_t), even though not strictly necessary, but I am fine with (uintptr_t) if you > >>> don't like (u32)(uintptr_t). > >> > >> It depends. I try to have as few casts as possible ("casts are evil"). > >> > >> While adding the extra (u32) cast makes it clearer that the intended > >> result is a u32 (for now), it will cause silent truncation on 64-bit if > >> connector_type is ever enlarged to unsigned long, and larger values are > >> used. > >> > >> In this particular case this is unlikely, though, as unsigned long would > >> still be 32-bit on 32-bit platforms, so the larger values cannot be > >> used. > > > > I also try to add as few casts as possible, so (uintptr_t) would be my > > preference. > > > > Fabrizio, could you submit a new version of this patch with the problem > > fixed (and with the casts you decide to use, but using uintptr_t instead > > of unsigned long in any case) ? > > Will send a new version tomorrow with (uintptr_t) Done as v4.1 :-)
> From: linux-renesas-soc-owner@vger.kernel.org <linux-renesas-soc-owner@vger.kernel.org> On Behalf Of Laurent Pinchart > Sent: 17 December 2019 23:09 > Subject: Re: [PATCH v4 06/13] drm/bridge: lvds-codec: Add "lvds-decoder" support > > On Tue, Dec 17, 2019 at 10:06:14PM +0000, Fabrizio Castro wrote: > > On 17 December 2019 13:54, Laurent Pinchart wrote: > > > On Tue, Dec 17, 2019 at 01:38:51PM +0100, Geert Uytterhoeven wrote: > > >> On Tue, Dec 17, 2019 at 1:31 PM Fabrizio Castro wrote: > > >>> On 17 December 2019 12:21, Geert Uytterhoeven wrote: > > >>>> On Tue, Dec 17, 2019 at 12:03 PM Fabrizio Castro wrote: > > >>>>> On 13 December 2019 17:11, Laurent Pinchart wrote: > > >>>>>> On Wed, Nov 13, 2019 at 03:51:25PM +0000, Fabrizio Castro wrote: > > >>>>>>> Add support for transparent LVDS decoders by adding a new > > >>>>>>> compatible string ("lvds-decoder") to the driver. > > >>>>>>> This patch also adds member connector_type to struct lvds_codec, > > >>>>>>> and that's because LVDS decoders have a different connector type > > >>>>>>> from LVDS encoders. We fill this new member up with the data > > >>>>>>> matching the compatible string. > > >>>>>>> > > >>>>>>> Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com> > > >>>>>>> > > >>>>>>> --- a/drivers/gpu/drm/bridge/lvds-codec.c > > >>>>>>> +++ b/drivers/gpu/drm/bridge/lvds-codec.c > > >>>>>>> > > >>>>>>> @@ -65,6 +67,7 @@ static int lvds_codec_probe(struct platform_device *pdev) > > >>>>>>> if (!lvds_codec) > > >>>>>>> return -ENOMEM; > > >>>>>>> > > >>>>>>> + lvds_codec->connector_type = (u32)of_device_get_match_data(&pdev->dev); > > >>>>>> > > >>>>>> I'm now getting a compilation failure here: > > >>>>>> > > >>>>>> drivers/gpu/drm/bridge/lvds-codec.c: In function ‘lvds_codec_probe’: > > >>>>>> drivers/gpu/drm/bridge/lvds-codec.c:68:31: error: cast from pointer to integer of different size [-Werror=pointer-to-int- > cast] > > >>>>>> lvds_codec->connector_type = (u32)of_device_get_match_data(&pdev->dev); > > >>>>>> ^ > > >>>>>> > > >>>>>> The fix should be simple: > > >>>>>> > > >>>>>> lvds_codec->connector_type = (uintptr_t)of_device_get_match_data(dev); > > >>>>>> > > >>>>>> I'm bothered by the fact that I've compiled this before without any > > >>>>>> issue, so this really puzzles me. Do you get the same warning ? > > >>>>> > > >>>>> The warning appears when compiling for arm64, understandably so. > > >>>>> We must have compiled this for arm only the first time around. > > >>>>> > > >>>>> I think the right way to solve this is to either cast to (u32)(uintptr_t) or (u32)(unsigned long). > > >>>> > > >>>> Just casting to uintptr_t should be sufficient. > > >>> > > >>> It should be sufficient for the compiler, but I have seen examples where people > > >>> preferred to be explicit, like in: > > >>> drivers/mailbox/mtk-cmdq-mailbox.c > > >>> drivers/leds/leds-pm8058.c > > >>> > > >>> Since the kernel is increasing its tightness with respect to warnings, I personally prefer > > >>> (u32)(uintptr_t), even though not strictly necessary, but I am fine with (uintptr_t) if you > > >>> don't like (u32)(uintptr_t). > > >> > > >> It depends. I try to have as few casts as possible ("casts are evil"). > > >> > > >> While adding the extra (u32) cast makes it clearer that the intended > > >> result is a u32 (for now), it will cause silent truncation on 64-bit if > > >> connector_type is ever enlarged to unsigned long, and larger values are > > >> used. > > >> > > >> In this particular case this is unlikely, though, as unsigned long would > > >> still be 32-bit on 32-bit platforms, so the larger values cannot be > > >> used. > > > > > > I also try to add as few casts as possible, so (uintptr_t) would be my > > > preference. > > > > > > Fabrizio, could you submit a new version of this patch with the problem > > > fixed (and with the casts you decide to use, but using uintptr_t instead > > > of unsigned long in any case) ? > > > > Will send a new version tomorrow with (uintptr_t) > > Done as v4.1 :-) Thank you! Fab > > -- > Regards, > > Laurent Pinchart
diff --git a/drivers/gpu/drm/bridge/lvds-codec.c b/drivers/gpu/drm/bridge/lvds-codec.c index b5801a2..c32e125 100644 --- a/drivers/gpu/drm/bridge/lvds-codec.c +++ b/drivers/gpu/drm/bridge/lvds-codec.c @@ -7,6 +7,7 @@ #include <linux/gpio/consumer.h> #include <linux/module.h> #include <linux/of.h> +#include <linux/of_device.h> #include <linux/of_graph.h> #include <linux/platform_device.h> @@ -17,6 +18,7 @@ struct lvds_codec { struct drm_bridge bridge; struct drm_bridge *panel_bridge; struct gpio_desc *powerdown_gpio; + u32 connector_type; }; static int lvds_codec_attach(struct drm_bridge *bridge) @@ -65,6 +67,7 @@ static int lvds_codec_probe(struct platform_device *pdev) if (!lvds_codec) return -ENOMEM; + lvds_codec->connector_type = (u32)of_device_get_match_data(&pdev->dev); lvds_codec->powerdown_gpio = devm_gpiod_get_optional(dev, "powerdown", GPIOD_OUT_HIGH); if (IS_ERR(lvds_codec->powerdown_gpio)) { @@ -105,7 +108,7 @@ static int lvds_codec_probe(struct platform_device *pdev) lvds_codec->panel_bridge = devm_drm_panel_bridge_add_typed(dev, panel, - DRM_MODE_CONNECTOR_LVDS); + lvds_codec->connector_type); if (IS_ERR(lvds_codec->panel_bridge)) return PTR_ERR(lvds_codec->panel_bridge); @@ -133,8 +136,18 @@ static int lvds_codec_remove(struct platform_device *pdev) } static const struct of_device_id lvds_codec_match[] = { - { .compatible = "lvds-encoder" }, - { .compatible = "thine,thc63lvdm83d" }, + { + .compatible = "lvds-decoder", + .data = (void *)DRM_MODE_CONNECTOR_DPI, + }, + { + .compatible = "lvds-encoder", + .data = (void *)DRM_MODE_CONNECTOR_LVDS, + }, + { + .compatible = "thine,thc63lvdm83d", + .data = (void *)DRM_MODE_CONNECTOR_LVDS, + }, {}, }; MODULE_DEVICE_TABLE(of, lvds_codec_match);
Add support for transparent LVDS decoders by adding a new compatible string ("lvds-decoder") to the driver. This patch also adds member connector_type to struct lvds_codec, and that's because LVDS decoders have a different connector type from LVDS encoders. We fill this new member up with the data matching the compatible string. Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com> --- v3->v4: * New patch --- drivers/gpu/drm/bridge/lvds-codec.c | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-)