| Message ID | 1573660292-10629-1-git-send-email-fabrizio.castro@bp.renesas.com |
|---|---|
| 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:24PM +0000, Fabrizio Castro wrote: > lvds-encoder.c implementation is also suitable for LVDS decoders, > not just LVDS encoders. > Instead of creating a new driver for addressing support for > transparent LVDS decoders, repurpose lvds-encoder.c for the greater > good with this patch. > > This patch only "rebrands" the lvds-encoder.c driver, to make it > suitable for hosting LVDS decoders support. The actual support for > LVDS decoders will come with a later patch. > > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > v3->v4: > * The patch now only renames the driver (with related data structures > and build options), changes the description of the module, and > refreshes the copyright > v2->v3: > * No change > v1->v2: > * No change > --- > drivers/gpu/drm/bridge/Kconfig | 8 +- > drivers/gpu/drm/bridge/Makefile | 2 +- > .../drm/bridge/{lvds-encoder.c => lvds-codec.c} | 97 +++++++++++----------- > 3 files changed, 53 insertions(+), 54 deletions(-) > rename drivers/gpu/drm/bridge/{lvds-encoder.c => lvds-codec.c} (45%) > > diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig > index 3436297..fbbea46 100644 > --- a/drivers/gpu/drm/bridge/Kconfig > +++ b/drivers/gpu/drm/bridge/Kconfig > @@ -45,14 +45,14 @@ config DRM_DUMB_VGA_DAC > Support for non-programmable RGB to VGA DAC bridges, such as ADI > ADV7123, TI THS8134 and THS8135 or passive resistor ladder DACs. > > -config DRM_LVDS_ENCODER > - tristate "Transparent parallel to LVDS encoder support" > +config DRM_LVDS_CODEC > + tristate "Transparent LVDS encoders and decoders support" > depends on OF > select DRM_KMS_HELPER > select DRM_PANEL_BRIDGE > help > - Support for transparent parallel to LVDS encoders that don't require > - any configuration. > + Support for transparent LVDS encoders and decoders that don't > + require any configuration. > > config DRM_MEGACHIPS_STDPXXXX_GE_B850V3_FW > tristate "MegaChips stdp4028-ge-b850v3-fw and stdp2690-ge-b850v3-fw" > diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile > index 4934fcf..8a9178a 100644 > --- a/drivers/gpu/drm/bridge/Makefile > +++ b/drivers/gpu/drm/bridge/Makefile > @@ -2,7 +2,7 @@ > obj-$(CONFIG_DRM_ANALOGIX_ANX78XX) += analogix-anx78xx.o > obj-$(CONFIG_DRM_CDNS_DSI) += cdns-dsi.o > obj-$(CONFIG_DRM_DUMB_VGA_DAC) += dumb-vga-dac.o > -obj-$(CONFIG_DRM_LVDS_ENCODER) += lvds-encoder.o > +obj-$(CONFIG_DRM_LVDS_CODEC) += lvds-codec.o > obj-$(CONFIG_DRM_MEGACHIPS_STDPXXXX_GE_B850V3_FW) += megachips-stdpxxxx-ge-b850v3-fw.o > obj-$(CONFIG_DRM_NXP_PTN3460) += nxp-ptn3460.o > obj-$(CONFIG_DRM_PARADE_PS8622) += parade-ps8622.o > diff --git a/drivers/gpu/drm/bridge/lvds-encoder.c b/drivers/gpu/drm/bridge/lvds-codec.c > similarity index 45% > rename from drivers/gpu/drm/bridge/lvds-encoder.c > rename to drivers/gpu/drm/bridge/lvds-codec.c > index e2132a8..b5801a2 100644 > --- a/drivers/gpu/drm/bridge/lvds-encoder.c > +++ b/drivers/gpu/drm/bridge/lvds-codec.c > @@ -1,5 +1,6 @@ > // SPDX-License-Identifier: GPL-2.0-or-later > /* > + * Copyright (C) 2019 Renesas Electronics Corporation > * Copyright (C) 2016 Laurent Pinchart <laurent.pinchart@ideasonboard.com> > */ > > @@ -12,65 +13,62 @@ > #include <drm/drm_bridge.h> > #include <drm/drm_panel.h> > > -struct lvds_encoder { > +struct lvds_codec { > struct drm_bridge bridge; > struct drm_bridge *panel_bridge; > struct gpio_desc *powerdown_gpio; > }; > > -static int lvds_encoder_attach(struct drm_bridge *bridge) > +static int lvds_codec_attach(struct drm_bridge *bridge) > { > - struct lvds_encoder *lvds_encoder = container_of(bridge, > - struct lvds_encoder, > - bridge); > + struct lvds_codec *lvds_codec = container_of(bridge, > + struct lvds_codec, bridge); > > - return drm_bridge_attach(bridge->encoder, lvds_encoder->panel_bridge, > + return drm_bridge_attach(bridge->encoder, lvds_codec->panel_bridge, > bridge); > } > > -static void lvds_encoder_enable(struct drm_bridge *bridge) > +static void lvds_codec_enable(struct drm_bridge *bridge) > { > - struct lvds_encoder *lvds_encoder = container_of(bridge, > - struct lvds_encoder, > - bridge); > + struct lvds_codec *lvds_codec = container_of(bridge, > + struct lvds_codec, bridge); > > - if (lvds_encoder->powerdown_gpio) > - gpiod_set_value_cansleep(lvds_encoder->powerdown_gpio, 0); > + if (lvds_codec->powerdown_gpio) > + gpiod_set_value_cansleep(lvds_codec->powerdown_gpio, 0); > } > > -static void lvds_encoder_disable(struct drm_bridge *bridge) > +static void lvds_codec_disable(struct drm_bridge *bridge) > { > - struct lvds_encoder *lvds_encoder = container_of(bridge, > - struct lvds_encoder, > - bridge); > + struct lvds_codec *lvds_codec = container_of(bridge, > + struct lvds_codec, bridge); > > - if (lvds_encoder->powerdown_gpio) > - gpiod_set_value_cansleep(lvds_encoder->powerdown_gpio, 1); > + if (lvds_codec->powerdown_gpio) > + gpiod_set_value_cansleep(lvds_codec->powerdown_gpio, 1); > } > > static struct drm_bridge_funcs funcs = { > - .attach = lvds_encoder_attach, > - .enable = lvds_encoder_enable, > - .disable = lvds_encoder_disable, > + .attach = lvds_codec_attach, > + .enable = lvds_codec_enable, > + .disable = lvds_codec_disable, > }; > > -static int lvds_encoder_probe(struct platform_device *pdev) > +static int lvds_codec_probe(struct platform_device *pdev) > { > struct device *dev = &pdev->dev; > struct device_node *port; > struct device_node *endpoint; > struct device_node *panel_node; > struct drm_panel *panel; > - struct lvds_encoder *lvds_encoder; > + struct lvds_codec *lvds_codec; > > - lvds_encoder = devm_kzalloc(dev, sizeof(*lvds_encoder), GFP_KERNEL); > - if (!lvds_encoder) > + lvds_codec = devm_kzalloc(dev, sizeof(*lvds_codec), GFP_KERNEL); > + if (!lvds_codec) > return -ENOMEM; > > - lvds_encoder->powerdown_gpio = devm_gpiod_get_optional(dev, "powerdown", > - GPIOD_OUT_HIGH); > - if (IS_ERR(lvds_encoder->powerdown_gpio)) { > - int err = PTR_ERR(lvds_encoder->powerdown_gpio); > + lvds_codec->powerdown_gpio = devm_gpiod_get_optional(dev, "powerdown", > + GPIOD_OUT_HIGH); > + if (IS_ERR(lvds_codec->powerdown_gpio)) { > + int err = PTR_ERR(lvds_codec->powerdown_gpio); > > if (err != -EPROBE_DEFER) > dev_err(dev, "powerdown GPIO failure: %d\n", err); > @@ -105,51 +103,52 @@ static int lvds_encoder_probe(struct platform_device *pdev) > return PTR_ERR(panel); > } > > - lvds_encoder->panel_bridge = > + lvds_codec->panel_bridge = > devm_drm_panel_bridge_add_typed(dev, panel, > DRM_MODE_CONNECTOR_LVDS); > - if (IS_ERR(lvds_encoder->panel_bridge)) > - return PTR_ERR(lvds_encoder->panel_bridge); > + if (IS_ERR(lvds_codec->panel_bridge)) > + return PTR_ERR(lvds_codec->panel_bridge); > > - /* The panel_bridge bridge is attached to the panel's of_node, > + /* > + * The panel_bridge bridge is attached to the panel's of_node, > * but we need a bridge attached to our of_node for our user > * to look up. > */ > - lvds_encoder->bridge.of_node = dev->of_node; > - lvds_encoder->bridge.funcs = &funcs; > - drm_bridge_add(&lvds_encoder->bridge); > + lvds_codec->bridge.of_node = dev->of_node; > + lvds_codec->bridge.funcs = &funcs; > + drm_bridge_add(&lvds_codec->bridge); > > - platform_set_drvdata(pdev, lvds_encoder); > + platform_set_drvdata(pdev, lvds_codec); > > return 0; > } > > -static int lvds_encoder_remove(struct platform_device *pdev) > +static int lvds_codec_remove(struct platform_device *pdev) > { > - struct lvds_encoder *lvds_encoder = platform_get_drvdata(pdev); > + struct lvds_codec *lvds_codec = platform_get_drvdata(pdev); > > - drm_bridge_remove(&lvds_encoder->bridge); > + drm_bridge_remove(&lvds_codec->bridge); > > return 0; > } > > -static const struct of_device_id lvds_encoder_match[] = { > +static const struct of_device_id lvds_codec_match[] = { > { .compatible = "lvds-encoder" }, > { .compatible = "thine,thc63lvdm83d" }, > {}, > }; > -MODULE_DEVICE_TABLE(of, lvds_encoder_match); > +MODULE_DEVICE_TABLE(of, lvds_codec_match); > > -static struct platform_driver lvds_encoder_driver = { > - .probe = lvds_encoder_probe, > - .remove = lvds_encoder_remove, > +static struct platform_driver lvds_codec_driver = { > + .probe = lvds_codec_probe, > + .remove = lvds_codec_remove, > .driver = { > - .name = "lvds-encoder", > - .of_match_table = lvds_encoder_match, > + .name = "lvds-codec", > + .of_match_table = lvds_codec_match, > }, > }; > -module_platform_driver(lvds_encoder_driver); > +module_platform_driver(lvds_codec_driver); > > MODULE_AUTHOR("Laurent Pinchart <laurent.pinchart@ideasonboard.com>"); > -MODULE_DESCRIPTION("Transparent parallel to LVDS encoder"); > +MODULE_DESCRIPTION("LVDS encoders and decoders"); > MODULE_LICENSE("GPL");
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, Thank you for the patch. On Wed, Nov 13, 2019 at 03:51:26PM +0000, Fabrizio Castro wrote: > The probe function needs to get ahold of the panel device tree > node, and it achieves that by using a combination of > of_graph_get_port_by_id, of_get_child_by_name, and > of_graph_get_remote_port_parent. We can achieve the same goal > by replacing those calls with a call to of_graph_get_remote_node > these days. > > 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 | 20 ++------------------ > 1 file changed, 2 insertions(+), 18 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/lvds-codec.c b/drivers/gpu/drm/bridge/lvds-codec.c > index c32e125..784bbd3 100644 > --- a/drivers/gpu/drm/bridge/lvds-codec.c > +++ b/drivers/gpu/drm/bridge/lvds-codec.c > @@ -57,8 +57,6 @@ static struct drm_bridge_funcs funcs = { > static int lvds_codec_probe(struct platform_device *pdev) > { > struct device *dev = &pdev->dev; > - struct device_node *port; > - struct device_node *endpoint; > struct device_node *panel_node; > struct drm_panel *panel; > struct lvds_codec *lvds_codec; > @@ -79,23 +77,9 @@ static int lvds_codec_probe(struct platform_device *pdev) > } > > /* Locate the panel DT node. */ > - port = of_graph_get_port_by_id(dev->of_node, 1); > - if (!port) { > - dev_dbg(dev, "port 1 not found\n"); > - return -ENXIO; > - } > - > - endpoint = of_get_child_by_name(port, "endpoint"); > - of_node_put(port); > - if (!endpoint) { > - dev_dbg(dev, "no endpoint for port 1\n"); > - return -ENXIO; > - } > - > - panel_node = of_graph_get_remote_port_parent(endpoint); > - of_node_put(endpoint); > + panel_node = of_graph_get_remote_node(dev->of_node, 1, 0); > if (!panel_node) { > - dev_dbg(dev, "no remote endpoint for port 1\n"); > + dev_dbg(dev, "panel DT node not found\n"); > return -ENXIO; > } >
Hi Fabrizio, Thank you for the patch. On Wed, Nov 13, 2019 at 03:51:29PM +0000, Fabrizio Castro wrote: > The iwg20d comes with a 7" capacitive touch screen, therefore > add support for it. > > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com> Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> I expect Geert to pick this up. > --- > v3->v4: > * Reworked compatibility of DT node lvds-receiver > * s/powerdown/powerdown-gpios/g in lvds-receiver > * Reworked compatibility of DT node panel > v2->v3: > * No change > v1->v2: > * No change > --- > arch/arm/boot/dts/iwg20d-q7-common.dtsi | 85 ++++++++++++++++++++++++++++++++ > arch/arm/boot/dts/iwg20d-q7-dbcm-ca.dtsi | 1 - > 2 files changed, 85 insertions(+), 1 deletion(-) > > diff --git a/arch/arm/boot/dts/iwg20d-q7-common.dtsi b/arch/arm/boot/dts/iwg20d-q7-common.dtsi > index ae75a1db..ab5ae93 100644 > --- a/arch/arm/boot/dts/iwg20d-q7-common.dtsi > +++ b/arch/arm/boot/dts/iwg20d-q7-common.dtsi > @@ -46,6 +46,49 @@ > clock-frequency = <26000000>; > }; > > + lcd_backlight: backlight { > + compatible = "pwm-backlight"; > + > + pwms = <&pwm3 0 5000000 0>; > + brightness-levels = <0 4 8 16 32 64 128 255>; > + default-brightness-level = <7>; > + enable-gpios = <&gpio5 14 GPIO_ACTIVE_HIGH>; > + }; > + > + lvds-receiver { > + compatible = "ti,ds90cf384a", "lvds-decoder"; > + powerdown-gpios = <&gpio7 25 GPIO_ACTIVE_LOW>; > + > + ports { > + #address-cells = <1>; > + #size-cells = <0>; > + > + port@0 { > + reg = <0>; > + lvds_receiver_in: endpoint { > + remote-endpoint = <&lvds0_out>; > + }; > + }; > + port@1 { > + reg = <1>; > + lvds_receiver_out: endpoint { > + remote-endpoint = <&panel_in>; > + }; > + }; > + }; > + }; > + > + panel { > + compatible = "edt,etm0700g0dh6"; > + backlight = <&lcd_backlight>; > + > + port { > + panel_in: endpoint { > + remote-endpoint = <&lvds_receiver_out>; > + }; > + }; > + }; > + > reg_1p5v: 1p5v { > compatible = "regulator-fixed"; > regulator-name = "1P5V"; > @@ -120,6 +163,18 @@ > status = "okay"; > }; > > +&du { > + status = "okay"; > +}; > + > +&gpio2 { > + touch-interrupt { > + gpio-hog; > + gpios = <12 GPIO_ACTIVE_LOW>; > + input; > + }; > +}; > + > &hsusb { > status = "okay"; > pinctrl-0 = <&usb0_pins>; > @@ -147,6 +202,25 @@ > VDDIO-supply = <®_3p3v>; > VDDD-supply = <®_1p5v>; > }; > + > + touch: touchpanel@38 { > + compatible = "edt,edt-ft5406"; > + reg = <0x38>; > + interrupt-parent = <&gpio2>; > + interrupts = <12 IRQ_TYPE_EDGE_FALLING>; > + }; > +}; > + > +&lvds0 { > + status = "okay"; > + > + ports { > + port@1 { > + lvds0_out: endpoint { > + remote-endpoint = <&lvds_receiver_in>; > + }; > + }; > + }; > }; > > &pci0 { > @@ -180,6 +254,11 @@ > function = "i2c2"; > }; > > + pwm3_pins: pwm3 { > + groups = "pwm3"; > + function = "pwm3"; > + }; > + > scif0_pins: scif0 { > groups = "scif0_data_d"; > function = "scif0"; > @@ -218,6 +297,12 @@ > }; > }; > > +&pwm3 { > + pinctrl-0 = <&pwm3_pins>; > + pinctrl-names = "default"; > + status = "okay"; > +}; > + > &rcar_sound { > pinctrl-0 = <&sound_pins>; > pinctrl-names = "default"; > diff --git a/arch/arm/boot/dts/iwg20d-q7-dbcm-ca.dtsi b/arch/arm/boot/dts/iwg20d-q7-dbcm-ca.dtsi > index 0e99df2..ede2e0c 100644 > --- a/arch/arm/boot/dts/iwg20d-q7-dbcm-ca.dtsi > +++ b/arch/arm/boot/dts/iwg20d-q7-dbcm-ca.dtsi > @@ -39,7 +39,6 @@ > &du { > pinctrl-0 = <&du_pins>; > pinctrl-names = "default"; > - status = "okay"; > > ports { > port@0 {
Hi Fabrizio, Thank you for the patch. On Wed, Nov 13, 2019 at 03:51:30PM +0000, Fabrizio Castro wrote: > The iwg20d comes with an LCD panel from Emerging Display > Technologies Corporation (EDT), therefore enable what's > required to support it. > > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com> Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> I expect Geert to pick this up. > --- > v3->v4: > * No change > v2->v3: > * No change > v1->v2: > * No change > --- > arch/arm/configs/shmobile_defconfig | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/arch/arm/configs/shmobile_defconfig b/arch/arm/configs/shmobile_defconfig > index c6c7035..ab416a5 100644 > --- a/arch/arm/configs/shmobile_defconfig > +++ b/arch/arm/configs/shmobile_defconfig > @@ -66,6 +66,7 @@ CONFIG_INPUT_EVDEV=y > CONFIG_KEYBOARD_GPIO=y > # CONFIG_INPUT_MOUSE is not set > CONFIG_INPUT_TOUCHSCREEN=y > +CONFIG_TOUCHSCREEN_EDT_FT5X06=y > CONFIG_TOUCHSCREEN_ST1232=y > CONFIG_INPUT_MISC=y > CONFIG_INPUT_ADXL34X=y > @@ -125,7 +126,9 @@ CONFIG_VIDEO_ADV7604=y > CONFIG_VIDEO_ML86V7667=y > CONFIG_DRM=y > CONFIG_DRM_RCAR_DU=y > +CONFIG_DRM_PANEL_SIMPLE=y > CONFIG_DRM_DUMB_VGA_DAC=y > +CONFIG_DRM_LVDS_CODEC=y > CONFIG_DRM_SII902X=y > CONFIG_DRM_I2C_ADV7511=y > CONFIG_DRM_I2C_ADV7511_AUDIO=y
Hi Fabrizio, Thank you for the patch. On Wed, Nov 13, 2019 at 03:51:31PM +0000, Fabrizio Castro wrote: > The lvds-codec driver is a generic stub for transparent LVDS > encoders and decoders. > It's good practice to list a device specific compatible string s/good practice/mandatory/ > before the generic fallback (if any) in the DT node for the relevant > LVDS encoder/decoder, and it's also required by the dt-bindings. > A notable exception to the generic fallback mechanism is the case > of "thine,thc63lvdm83d", as documented in: > Documentation/devicetree/bindings/display/bridge/thine,thc63lvdm83d.txt > This patch enforces the adoption of a device specific compatible > string (as fist string in the list), by using markers for the s/fist/first/ > compatible string we match against and the index of the matching > compatible string in the list. > > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com> > > --- > Hi Laurent, > > I don't think we need to do anything in the driver to address your > comment, as we can "enforce" this with the bindings (please see the > next patch, as it would help with the "enforcing" of the compatible > string for the thine device). > I am sending this patch only so that you can see what a possible > solution in the driver could look like. > > v3->v4: > * New patch addressing the below comment from Laurent: > "I think the lvds-decoder driver should error out at probe time if only > one compatible string is listed." > --- > drivers/gpu/drm/bridge/lvds-codec.c | 55 +++++++++++++++++++++++++++++++++---- > 1 file changed, 49 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/lvds-codec.c b/drivers/gpu/drm/bridge/lvds-codec.c > index 784bbd3..145c25d 100644 > --- a/drivers/gpu/drm/bridge/lvds-codec.c > +++ b/drivers/gpu/drm/bridge/lvds-codec.c > @@ -14,11 +14,16 @@ > #include <drm/drm_bridge.h> > #include <drm/drm_panel.h> > > +struct lvds_codec_data { > + u32 connector_type; > + bool device_specific; > +}; > + > struct lvds_codec { > struct drm_bridge bridge; > struct drm_bridge *panel_bridge; > struct gpio_desc *powerdown_gpio; > - u32 connector_type; > + const struct lvds_codec_data *data; > }; > > static int lvds_codec_attach(struct drm_bridge *bridge) > @@ -65,7 +70,30 @@ 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->data = of_device_get_match_data(&pdev->dev); > + if (!lvds_codec->data) > + return -EINVAL; > + > + /* > + * If we haven't matched a device specific compatible string, we need > + * to work out if the generic compatible string we matched against was > + * listed first in the compatible property. > + */ Can't we do this unconditionally, and thus drop the lvds_codec_data structure ? > + if (!lvds_codec->data->device_specific) { > + const struct of_device_id *match; > + int compatible_index; > + > + match = of_match_node(dev->driver->of_match_table, > + dev->of_node); > + compatible_index = of_property_match_string(dev->of_node, > + "compatible", > + match->compatible); > + if (compatible_index == 0) { > + dev_err(dev, "Device specific compatible needed\n"); > + return -EINVAL; > + } > + } > + > lvds_codec->powerdown_gpio = devm_gpiod_get_optional(dev, "powerdown", > GPIOD_OUT_HIGH); > if (IS_ERR(lvds_codec->powerdown_gpio)) { > @@ -92,7 +120,7 @@ static int lvds_codec_probe(struct platform_device *pdev) > > lvds_codec->panel_bridge = > devm_drm_panel_bridge_add_typed(dev, panel, > - lvds_codec->connector_type); > + lvds_codec->data->connector_type); > if (IS_ERR(lvds_codec->panel_bridge)) > return PTR_ERR(lvds_codec->panel_bridge); > > @@ -119,18 +147,33 @@ static int lvds_codec_remove(struct platform_device *pdev) > return 0; > } > > +static const struct lvds_codec_data lvds_codec_decoder_data = { > + .connector_type = DRM_MODE_CONNECTOR_DPI, > + .device_specific = false, > +}; > + > +static const struct lvds_codec_data lvds_codec_encoder_data = { > + .connector_type = DRM_MODE_CONNECTOR_LVDS, > + .device_specific = false, > +}; > + > +static const struct lvds_codec_data lvds_codec_thc63lvdm83d_data = { > + .connector_type = DRM_MODE_CONNECTOR_LVDS, > + .device_specific = true, > +}; > + > static const struct of_device_id lvds_codec_match[] = { > { > .compatible = "lvds-decoder", > - .data = (void *)DRM_MODE_CONNECTOR_DPI, > + .data = &lvds_codec_decoder_data, > }, > { > .compatible = "lvds-encoder", > - .data = (void *)DRM_MODE_CONNECTOR_LVDS, > + .data = &lvds_codec_encoder_data, > }, > { > .compatible = "thine,thc63lvdm83d", > - .data = (void *)DRM_MODE_CONNECTOR_LVDS, > + .data = &lvds_codec_thc63lvdm83d_data, > }, > {}, > };
Hi Laurent, Thank you for your feedback! > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Sent: 19 November 2019 00:16 > Subject: Re: [PATCH v4 12/13] [HACK] drm/bridge: lvds-codec: Enforce device specific compatible strings > > Hi Fabrizio, > > Thank you for the patch. > > On Wed, Nov 13, 2019 at 03:51:31PM +0000, Fabrizio Castro wrote: > > The lvds-codec driver is a generic stub for transparent LVDS > > encoders and decoders. > > It's good practice to list a device specific compatible string > > s/good practice/mandatory/ Will fix > > > before the generic fallback (if any) in the DT node for the relevant > > LVDS encoder/decoder, and it's also required by the dt-bindings. > > A notable exception to the generic fallback mechanism is the case > > of "thine,thc63lvdm83d", as documented in: > > Documentation/devicetree/bindings/display/bridge/thine,thc63lvdm83d.txt > > This patch enforces the adoption of a device specific compatible > > string (as fist string in the list), by using markers for the > > s/fist/first/ Well spotted > > > compatible string we match against and the index of the matching > > compatible string in the list. > > > > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com> > > > > --- > > Hi Laurent, > > > > I don't think we need to do anything in the driver to address your > > comment, as we can "enforce" this with the bindings (please see the > > next patch, as it would help with the "enforcing" of the compatible > > string for the thine device). > > I am sending this patch only so that you can see what a possible > > solution in the driver could look like. > > > > v3->v4: > > * New patch addressing the below comment from Laurent: > > "I think the lvds-decoder driver should error out at probe time if only > > one compatible string is listed." > > --- > > drivers/gpu/drm/bridge/lvds-codec.c | 55 +++++++++++++++++++++++++++++++++---- > > 1 file changed, 49 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/gpu/drm/bridge/lvds-codec.c b/drivers/gpu/drm/bridge/lvds-codec.c > > index 784bbd3..145c25d 100644 > > --- a/drivers/gpu/drm/bridge/lvds-codec.c > > +++ b/drivers/gpu/drm/bridge/lvds-codec.c > > @@ -14,11 +14,16 @@ > > #include <drm/drm_bridge.h> > > #include <drm/drm_panel.h> > > > > +struct lvds_codec_data { > > + u32 connector_type; > > + bool device_specific; > > +}; > > + > > struct lvds_codec { > > struct drm_bridge bridge; > > struct drm_bridge *panel_bridge; > > struct gpio_desc *powerdown_gpio; > > - u32 connector_type; > > + const struct lvds_codec_data *data; > > }; > > > > static int lvds_codec_attach(struct drm_bridge *bridge) > > @@ -65,7 +70,30 @@ 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->data = of_device_get_match_data(&pdev->dev); > > + if (!lvds_codec->data) > > + return -EINVAL; > > + > > + /* > > + * If we haven't matched a device specific compatible string, we need > > + * to work out if the generic compatible string we matched against was > > + * listed first in the compatible property. > > + */ > > Can't we do this unconditionally, and thus drop the lvds_codec_data > structure ? I don't think so, and the reason for this is that we have a corner case for thine,thc63lvdm83d. Here is what's allowed (according to the documentation) from what's supported upstream (+ this series): "ti,ds90c185", "lvds-encoder" "ti,ds90c187", "lvds-encoder" "ti,sn75lvds83", "lvds-encoder" "ti,ds90cf384a", "lvds-decoder" "thine,thc63lvdm83d" As you can see from the examples above, in most cases it's enough to say it's all good when we match a compatible string with index > 0, but for the thine device you _have_ to match the string with index 0 as that's what's currently documented (please see thine,thc63lvdm83d.txt) and that's what's supported by device trees already (please see arch/arm/boot/dts/r8a7779-marzen.dts). This patch "classifies" compatible strings, and it considers a good match device specific compatible strings, or generic compatible strings as long as they are not listed first. These days you can leverage the yaml files to validate the device trees, therefore we should be focusing on writing yaml files in such a way we only pass the checks we mean to, and by checks I mean: make dtbs_check or more specifically, for this series: make dtbs_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml and that's of course on top of make dt_binding_check. It's a very common requirement to have a part number specific compatible string first followed by a generic (fallback) compatible string in the device trees, most drivers for Renesas SoCs have similar requirements. If we start doing this here, we'll end up doing it elsewhere as well, and I really think we shouldn't, but others may see things differently, so I'll wait for others (and yourself with further comments) to jump in before doing any more work on this patch. Thanks, Fab > > > + if (!lvds_codec->data->device_specific) { > > + const struct of_device_id *match; > > + int compatible_index; > > + > > + match = of_match_node(dev->driver->of_match_table, > > + dev->of_node); > > + compatible_index = of_property_match_string(dev->of_node, > > + "compatible", > > + match->compatible); > > + if (compatible_index == 0) { > > + dev_err(dev, "Device specific compatible needed\n"); > > + return -EINVAL; > > + } > > + } > > + > > lvds_codec->powerdown_gpio = devm_gpiod_get_optional(dev, "powerdown", > > GPIOD_OUT_HIGH); > > if (IS_ERR(lvds_codec->powerdown_gpio)) { > > @@ -92,7 +120,7 @@ static int lvds_codec_probe(struct platform_device *pdev) > > > > lvds_codec->panel_bridge = > > devm_drm_panel_bridge_add_typed(dev, panel, > > - lvds_codec->connector_type); > > + lvds_codec->data->connector_type); > > if (IS_ERR(lvds_codec->panel_bridge)) > > return PTR_ERR(lvds_codec->panel_bridge); > > > > @@ -119,18 +147,33 @@ static int lvds_codec_remove(struct platform_device *pdev) > > return 0; > > } > > > > +static const struct lvds_codec_data lvds_codec_decoder_data = { > > + .connector_type = DRM_MODE_CONNECTOR_DPI, > > + .device_specific = false, > > +}; > > + > > +static const struct lvds_codec_data lvds_codec_encoder_data = { > > + .connector_type = DRM_MODE_CONNECTOR_LVDS, > > + .device_specific = false, > > +}; > > + > > +static const struct lvds_codec_data lvds_codec_thc63lvdm83d_data = { > > + .connector_type = DRM_MODE_CONNECTOR_LVDS, > > + .device_specific = true, > > +}; > > + > > static const struct of_device_id lvds_codec_match[] = { > > { > > .compatible = "lvds-decoder", > > - .data = (void *)DRM_MODE_CONNECTOR_DPI, > > + .data = &lvds_codec_decoder_data, > > }, > > { > > .compatible = "lvds-encoder", > > - .data = (void *)DRM_MODE_CONNECTOR_LVDS, > > + .data = &lvds_codec_encoder_data, > > }, > > { > > .compatible = "thine,thc63lvdm83d", > > - .data = (void *)DRM_MODE_CONNECTOR_LVDS, > > + .data = &lvds_codec_thc63lvdm83d_data, > > }, > > {}, > > }; > > -- > Regards, > > Laurent Pinchart
Hi Fabrizio, On Tue, Nov 19, 2019 at 11:17:34AM +0000, Fabrizio Castro wrote: > On 19 November 2019 00:16 Laurent Pinchart wrote: > > On Wed, Nov 13, 2019 at 03:51:31PM +0000, Fabrizio Castro wrote: > > > The lvds-codec driver is a generic stub for transparent LVDS > > > encoders and decoders. > > > It's good practice to list a device specific compatible string > > > > s/good practice/mandatory/ > > Will fix > > > > before the generic fallback (if any) in the DT node for the relevant > > > LVDS encoder/decoder, and it's also required by the dt-bindings. > > > A notable exception to the generic fallback mechanism is the case > > > of "thine,thc63lvdm83d", as documented in: > > > Documentation/devicetree/bindings/display/bridge/thine,thc63lvdm83d.txt > > > This patch enforces the adoption of a device specific compatible > > > string (as fist string in the list), by using markers for the > > > > s/fist/first/ > > Well spotted > > > > > > compatible string we match against and the index of the matching > > > compatible string in the list. > > > > > > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com> > > > > > > --- > > > Hi Laurent, > > > > > > I don't think we need to do anything in the driver to address your > > > comment, as we can "enforce" this with the bindings (please see the > > > next patch, as it would help with the "enforcing" of the compatible > > > string for the thine device). > > > I am sending this patch only so that you can see what a possible > > > solution in the driver could look like. > > > > > > v3->v4: > > > * New patch addressing the below comment from Laurent: > > > "I think the lvds-decoder driver should error out at probe time if only > > > one compatible string is listed." > > > --- > > > drivers/gpu/drm/bridge/lvds-codec.c | 55 +++++++++++++++++++++++++++++++++---- > > > 1 file changed, 49 insertions(+), 6 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/bridge/lvds-codec.c b/drivers/gpu/drm/bridge/lvds-codec.c > > > index 784bbd3..145c25d 100644 > > > --- a/drivers/gpu/drm/bridge/lvds-codec.c > > > +++ b/drivers/gpu/drm/bridge/lvds-codec.c > > > @@ -14,11 +14,16 @@ > > > #include <drm/drm_bridge.h> > > > #include <drm/drm_panel.h> > > > > > > +struct lvds_codec_data { > > > + u32 connector_type; > > > + bool device_specific; > > > +}; > > > + > > > struct lvds_codec { > > > struct drm_bridge bridge; > > > struct drm_bridge *panel_bridge; > > > struct gpio_desc *powerdown_gpio; > > > - u32 connector_type; > > > + const struct lvds_codec_data *data; > > > }; > > > > > > static int lvds_codec_attach(struct drm_bridge *bridge) > > > @@ -65,7 +70,30 @@ 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->data = of_device_get_match_data(&pdev->dev); > > > + if (!lvds_codec->data) > > > + return -EINVAL; > > > + > > > + /* > > > + * If we haven't matched a device specific compatible string, we need > > > + * to work out if the generic compatible string we matched against was > > > + * listed first in the compatible property. > > > + */ > > > > Can't we do this unconditionally, and thus drop the lvds_codec_data > > structure ? > > I don't think so, and the reason for this is that we have a corner case for > thine,thc63lvdm83d. Here is what's allowed (according to the documentation) > from what's supported upstream (+ this series): > "ti,ds90c185", "lvds-encoder" > "ti,ds90c187", "lvds-encoder" > "ti,sn75lvds83", "lvds-encoder" > "ti,ds90cf384a", "lvds-decoder" > "thine,thc63lvdm83d" > > As you can see from the examples above, in most cases it's enough to say it's > all good when we match a compatible string with index > 0, but for the thine > device you _have_ to match the string with index 0 as that's what's currently > documented (please see thine,thc63lvdm83d.txt) and that's what's supported > by device trees already (please see arch/arm/boot/dts/r8a7779-marzen.dts). How about the following logic ? if (match_index("lvds-encoder") == 0 || match_index("lvds-decoder") == 0) return -EINVAL; ? > This patch "classifies" compatible strings, and it considers a good match > device specific compatible strings, or generic compatible strings as long > as they are not listed first. > > These days you can leverage the yaml files to validate the device trees, > therefore we should be focusing on writing yaml files in such a way we only > pass the checks we mean to, and by checks I mean: > make dtbs_check > > or more specifically, for this series: > make dtbs_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml > > and that's of course on top of make dt_binding_check. Sure, but that doesn't prevent anyone ignoring the validation. > It's a very common requirement to have a part number specific compatible > string first followed by a generic (fallback) compatible string in the device trees, > most drivers for Renesas SoCs have similar requirements. > > If we start doing this here, we'll end up doing it elsewhere as well, and I really > think we shouldn't, but others may see things differently, so I'll wait for others > (and yourself with further comments) to jump in before doing any more work > on this patch. I agree with this argument, it would set a precedent, and is probably not worth duplicating similar code in all drivers. I wonder if this is something we could handle with core helpers, but maybe it's overkill. > > > + if (!lvds_codec->data->device_specific) { > > > + const struct of_device_id *match; > > > + int compatible_index; > > > + > > > + match = of_match_node(dev->driver->of_match_table, > > > + dev->of_node); > > > + compatible_index = of_property_match_string(dev->of_node, > > > + "compatible", > > > + match->compatible); > > > + if (compatible_index == 0) { > > > + dev_err(dev, "Device specific compatible needed\n"); > > > + return -EINVAL; > > > + } > > > + } > > > + > > > lvds_codec->powerdown_gpio = devm_gpiod_get_optional(dev, "powerdown", > > > GPIOD_OUT_HIGH); > > > if (IS_ERR(lvds_codec->powerdown_gpio)) { > > > @@ -92,7 +120,7 @@ static int lvds_codec_probe(struct platform_device *pdev) > > > > > > lvds_codec->panel_bridge = > > > devm_drm_panel_bridge_add_typed(dev, panel, > > > - lvds_codec->connector_type); > > > + lvds_codec->data->connector_type); > > > if (IS_ERR(lvds_codec->panel_bridge)) > > > return PTR_ERR(lvds_codec->panel_bridge); > > > > > > @@ -119,18 +147,33 @@ static int lvds_codec_remove(struct platform_device *pdev) > > > return 0; > > > } > > > > > > +static const struct lvds_codec_data lvds_codec_decoder_data = { > > > + .connector_type = DRM_MODE_CONNECTOR_DPI, > > > + .device_specific = false, > > > +}; > > > + > > > +static const struct lvds_codec_data lvds_codec_encoder_data = { > > > + .connector_type = DRM_MODE_CONNECTOR_LVDS, > > > + .device_specific = false, > > > +}; > > > + > > > +static const struct lvds_codec_data lvds_codec_thc63lvdm83d_data = { > > > + .connector_type = DRM_MODE_CONNECTOR_LVDS, > > > + .device_specific = true, > > > +}; > > > + > > > static const struct of_device_id lvds_codec_match[] = { > > > { > > > .compatible = "lvds-decoder", > > > - .data = (void *)DRM_MODE_CONNECTOR_DPI, > > > + .data = &lvds_codec_decoder_data, > > > }, > > > { > > > .compatible = "lvds-encoder", > > > - .data = (void *)DRM_MODE_CONNECTOR_LVDS, > > > + .data = &lvds_codec_encoder_data, > > > }, > > > { > > > .compatible = "thine,thc63lvdm83d", > > > - .data = (void *)DRM_MODE_CONNECTOR_LVDS, > > > + .data = &lvds_codec_thc63lvdm83d_data, > > > }, > > > {}, > > > };
Hi Laurent, Thank you for your feedback! > From: devicetree-owner@vger.kernel.org <devicetree-owner@vger.kernel.org> On Behalf Of Laurent Pinchart > Sent: 19 November 2019 21:52 > Subject: Re: [PATCH v4 12/13] [HACK] drm/bridge: lvds-codec: Enforce device specific compatible strings > > Hi Fabrizio, > > On Tue, Nov 19, 2019 at 11:17:34AM +0000, Fabrizio Castro wrote: > > On 19 November 2019 00:16 Laurent Pinchart wrote: > > > On Wed, Nov 13, 2019 at 03:51:31PM +0000, Fabrizio Castro wrote: > > > > The lvds-codec driver is a generic stub for transparent LVDS > > > > encoders and decoders. > > > > It's good practice to list a device specific compatible string > > > > > > s/good practice/mandatory/ > > > > Will fix > > > > > > before the generic fallback (if any) in the DT node for the relevant > > > > LVDS encoder/decoder, and it's also required by the dt-bindings. > > > > A notable exception to the generic fallback mechanism is the case > > > > of "thine,thc63lvdm83d", as documented in: > > > > Documentation/devicetree/bindings/display/bridge/thine,thc63lvdm83d.txt > > > > This patch enforces the adoption of a device specific compatible > > > > string (as fist string in the list), by using markers for the > > > > > > s/fist/first/ > > > > Well spotted > > > > > > > > > compatible string we match against and the index of the matching > > > > compatible string in the list. > > > > > > > > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com> > > > > > > > > --- > > > > Hi Laurent, > > > > > > > > I don't think we need to do anything in the driver to address your > > > > comment, as we can "enforce" this with the bindings (please see the > > > > next patch, as it would help with the "enforcing" of the compatible > > > > string for the thine device). > > > > I am sending this patch only so that you can see what a possible > > > > solution in the driver could look like. > > > > > > > > v3->v4: > > > > * New patch addressing the below comment from Laurent: > > > > "I think the lvds-decoder driver should error out at probe time if only > > > > one compatible string is listed." > > > > --- > > > > drivers/gpu/drm/bridge/lvds-codec.c | 55 +++++++++++++++++++++++++++++++++---- > > > > 1 file changed, 49 insertions(+), 6 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/bridge/lvds-codec.c b/drivers/gpu/drm/bridge/lvds-codec.c > > > > index 784bbd3..145c25d 100644 > > > > --- a/drivers/gpu/drm/bridge/lvds-codec.c > > > > +++ b/drivers/gpu/drm/bridge/lvds-codec.c > > > > @@ -14,11 +14,16 @@ > > > > #include <drm/drm_bridge.h> > > > > #include <drm/drm_panel.h> > > > > > > > > +struct lvds_codec_data { > > > > + u32 connector_type; > > > > + bool device_specific; > > > > +}; > > > > + > > > > struct lvds_codec { > > > > struct drm_bridge bridge; > > > > struct drm_bridge *panel_bridge; > > > > struct gpio_desc *powerdown_gpio; > > > > - u32 connector_type; > > > > + const struct lvds_codec_data *data; > > > > }; > > > > > > > > static int lvds_codec_attach(struct drm_bridge *bridge) > > > > @@ -65,7 +70,30 @@ 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->data = of_device_get_match_data(&pdev->dev); > > > > + if (!lvds_codec->data) > > > > + return -EINVAL; > > > > + > > > > + /* > > > > + * If we haven't matched a device specific compatible string, we need > > > > + * to work out if the generic compatible string we matched against was > > > > + * listed first in the compatible property. > > > > + */ > > > > > > Can't we do this unconditionally, and thus drop the lvds_codec_data > > > structure ? > > > > I don't think so, and the reason for this is that we have a corner case for > > thine,thc63lvdm83d. Here is what's allowed (according to the documentation) > > from what's supported upstream (+ this series): > > "ti,ds90c185", "lvds-encoder" > > "ti,ds90c187", "lvds-encoder" > > "ti,sn75lvds83", "lvds-encoder" > > "ti,ds90cf384a", "lvds-decoder" > > "thine,thc63lvdm83d" > > > > As you can see from the examples above, in most cases it's enough to say it's > > all good when we match a compatible string with index > 0, but for the thine > > device you _have_ to match the string with index 0 as that's what's currently > > documented (please see thine,thc63lvdm83d.txt) and that's what's supported > > by device trees already (please see arch/arm/boot/dts/r8a7779-marzen.dts). > > How about the following logic ? > > if (match_index("lvds-encoder") == 0 || > match_index("lvds-decoder") == 0) > return -EINVAL; > > Now I see what you mean > > > This patch "classifies" compatible strings, and it considers a good match > > device specific compatible strings, or generic compatible strings as long > > as they are not listed first. > > > > These days you can leverage the yaml files to validate the device trees, > > therefore we should be focusing on writing yaml files in such a way we only > > pass the checks we mean to, and by checks I mean: > > make dtbs_check > > > > or more specifically, for this series: > > make dtbs_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml > > > > and that's of course on top of make dt_binding_check. > > Sure, but that doesn't prevent anyone ignoring the validation. > > > It's a very common requirement to have a part number specific compatible > > string first followed by a generic (fallback) compatible string in the device trees, > > most drivers for Renesas SoCs have similar requirements. > > > > If we start doing this here, we'll end up doing it elsewhere as well, and I really > > think we shouldn't, but others may see things differently, so I'll wait for others > > (and yourself with further comments) to jump in before doing any more work > > on this patch. > > I agree with this argument, it would set a precedent, and is probably > not worth duplicating similar code in all drivers. I wonder if this is > something we could handle with core helpers, but maybe it's overkill. I was hoping others would comment as well, but perhaps this topic is not too exciting. Geert, what do you think about this? Is this something we should enforce in drivers? Thanks, Fab > > > > > + if (!lvds_codec->data->device_specific) { > > > > + const struct of_device_id *match; > > > > + int compatible_index; > > > > + > > > > + match = of_match_node(dev->driver->of_match_table, > > > > + dev->of_node); > > > > + compatible_index = of_property_match_string(dev->of_node, > > > > + "compatible", > > > > + match->compatible); > > > > + if (compatible_index == 0) { > > > > + dev_err(dev, "Device specific compatible needed\n"); > > > > + return -EINVAL; > > > > + } > > > > + } > > > > + > > > > lvds_codec->powerdown_gpio = devm_gpiod_get_optional(dev, "powerdown", > > > > GPIOD_OUT_HIGH); > > > > if (IS_ERR(lvds_codec->powerdown_gpio)) { > > > > @@ -92,7 +120,7 @@ static int lvds_codec_probe(struct platform_device *pdev) > > > > > > > > lvds_codec->panel_bridge = > > > > devm_drm_panel_bridge_add_typed(dev, panel, > > > > - lvds_codec->connector_type); > > > > + lvds_codec->data->connector_type); > > > > if (IS_ERR(lvds_codec->panel_bridge)) > > > > return PTR_ERR(lvds_codec->panel_bridge); > > > > > > > > @@ -119,18 +147,33 @@ static int lvds_codec_remove(struct platform_device *pdev) > > > > return 0; > > > > } > > > > > > > > +static const struct lvds_codec_data lvds_codec_decoder_data = { > > > > + .connector_type = DRM_MODE_CONNECTOR_DPI, > > > > + .device_specific = false, > > > > +}; > > > > + > > > > +static const struct lvds_codec_data lvds_codec_encoder_data = { > > > > + .connector_type = DRM_MODE_CONNECTOR_LVDS, > > > > + .device_specific = false, > > > > +}; > > > > + > > > > +static const struct lvds_codec_data lvds_codec_thc63lvdm83d_data = { > > > > + .connector_type = DRM_MODE_CONNECTOR_LVDS, > > > > + .device_specific = true, > > > > +}; > > > > + > > > > static const struct of_device_id lvds_codec_match[] = { > > > > { > > > > .compatible = "lvds-decoder", > > > > - .data = (void *)DRM_MODE_CONNECTOR_DPI, > > > > + .data = &lvds_codec_decoder_data, > > > > }, > > > > { > > > > .compatible = "lvds-encoder", > > > > - .data = (void *)DRM_MODE_CONNECTOR_LVDS, > > > > + .data = &lvds_codec_encoder_data, > > > > }, > > > > { > > > > .compatible = "thine,thc63lvdm83d", > > > > - .data = (void *)DRM_MODE_CONNECTOR_LVDS, > > > > + .data = &lvds_codec_thc63lvdm83d_data, > > > > }, > > > > {}, > > > > }; > > -- > Regards, > > Laurent Pinchart
Hi Fabrizio, On Thu, Nov 21, 2019 at 5:00 PM Fabrizio Castro <fabrizio.castro@bp.renesas.com> wrote: > > From: devicetree-owner@vger.kernel.org <devicetree-owner@vger.kernel.org> On Behalf Of Laurent Pinchart > > Sent: 19 November 2019 21:52 > > Subject: Re: [PATCH v4 12/13] [HACK] drm/bridge: lvds-codec: Enforce device specific compatible strings > > > > On Tue, Nov 19, 2019 at 11:17:34AM +0000, Fabrizio Castro wrote: > > > On 19 November 2019 00:16 Laurent Pinchart wrote: > > > > On Wed, Nov 13, 2019 at 03:51:31PM +0000, Fabrizio Castro wrote: > > > > > The lvds-codec driver is a generic stub for transparent LVDS > > > > > encoders and decoders. > > > > > It's good practice to list a device specific compatible string > > > > > before the generic fallback (if any) in the DT node for the relevant > > > > > LVDS encoder/decoder, and it's also required by the dt-bindings. > > > > > A notable exception to the generic fallback mechanism is the case > > > > > of "thine,thc63lvdm83d", as documented in: > > > > > Documentation/devicetree/bindings/display/bridge/thine,thc63lvdm83d.txt > > > > > This patch enforces the adoption of a device specific compatible > > > > > string (as fist string in the list), by using markers for the > > > > > compatible string we match against and the index of the matching > > > > > compatible string in the list. > > > > > > > > > > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com> > > > > > > > > > > --- > > > > > Hi Laurent, > > > > > > > > > > I don't think we need to do anything in the driver to address your > > > > > comment, as we can "enforce" this with the bindings (please see the > > > > > next patch, as it would help with the "enforcing" of the compatible > > > > > string for the thine device). > > > > > I am sending this patch only so that you can see what a possible > > > > > solution in the driver could look like. > > > > > > > > > > v3->v4: > > > > > * New patch addressing the below comment from Laurent: > > > > > "I think the lvds-decoder driver should error out at probe time if only > > > > > one compatible string is listed." > > > > > --- a/drivers/gpu/drm/bridge/lvds-codec.c > > > > > +++ b/drivers/gpu/drm/bridge/lvds-codec.c > > > > > @@ -65,7 +70,30 @@ 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->data = of_device_get_match_data(&pdev->dev); > > > > > + if (!lvds_codec->data) > > > > > + return -EINVAL; > > > > > + > > > > > + /* > > > > > + * If we haven't matched a device specific compatible string, we need > > > > > + * to work out if the generic compatible string we matched against was > > > > > + * listed first in the compatible property. > > > > > + */ > > > > > > > > Can't we do this unconditionally, and thus drop the lvds_codec_data > > > > structure ? > > > > > > I don't think so, and the reason for this is that we have a corner case for > > > thine,thc63lvdm83d. Here is what's allowed (according to the documentation) > > > from what's supported upstream (+ this series): > > > "ti,ds90c185", "lvds-encoder" > > > "ti,ds90c187", "lvds-encoder" > > > "ti,sn75lvds83", "lvds-encoder" > > > "ti,ds90cf384a", "lvds-decoder" > > > "thine,thc63lvdm83d" > > > > > > As you can see from the examples above, in most cases it's enough to say it's > > > all good when we match a compatible string with index > 0, but for the thine > > > device you _have_ to match the string with index 0 as that's what's currently > > > documented (please see thine,thc63lvdm83d.txt) and that's what's supported > > > by device trees already (please see arch/arm/boot/dts/r8a7779-marzen.dts). > > > > How about the following logic ? > > > > if (match_index("lvds-encoder") == 0 || > > match_index("lvds-decoder") == 0) > > return -EINVAL; > > > > > > Now I see what you mean > > > > This patch "classifies" compatible strings, and it considers a good match > > > device specific compatible strings, or generic compatible strings as long > > > as they are not listed first. > > > > > > These days you can leverage the yaml files to validate the device trees, > > > therefore we should be focusing on writing yaml files in such a way we only > > > pass the checks we mean to, and by checks I mean: > > > make dtbs_check > > > > > > or more specifically, for this series: > > > make dtbs_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml > > > > > > and that's of course on top of make dt_binding_check. > > > > Sure, but that doesn't prevent anyone ignoring the validation. > > > > > It's a very common requirement to have a part number specific compatible > > > string first followed by a generic (fallback) compatible string in the device trees, > > > most drivers for Renesas SoCs have similar requirements. > > > > > > If we start doing this here, we'll end up doing it elsewhere as well, and I really > > > think we shouldn't, but others may see things differently, so I'll wait for others > > > (and yourself with further comments) to jump in before doing any more work > > > on this patch. > > > > I agree with this argument, it would set a precedent, and is probably > > not worth duplicating similar code in all drivers. I wonder if this is > > something we could handle with core helpers, but maybe it's overkill. > > I was hoping others would comment as well, but perhaps this topic is not too exciting. > > Geert, what do you think about this? Is this something we should enforce > in drivers? So IIUIC, you want to enforce the presence of both specific and generic compatible values (in that order) in the driver (except for "thine,thc63lvdm83d", as that predates the introduction of the generic compatible value)? However, the driver would not really care about the actual hardware-specific value, as it would still match against the generic one, and the hardware-specific one may not even be listed in the driver's match table? By definition, you can have one or more compatible values listed in a device node, from most-specific to least-specific. Typically the driver cannot know if a more specific value is missing, but YAML DT binding validation can. In this case it is a bit special, as there is a generic one involved, so you can assume there should be a more specific one, too. If you want to handle this in the core, you probably need to add an "is_generic" flag to struct of_device_id. Rob/Mark? > > > > > + if (!lvds_codec->data->device_specific) { > > > > > + const struct of_device_id *match; > > > > > + int compatible_index; > > > > > + > > > > > + match = of_match_node(dev->driver->of_match_table, > > > > > + dev->of_node); > > > > > + compatible_index = of_property_match_string(dev->of_node, > > > > > + "compatible", > > > > > + match->compatible); > > > > > + if (compatible_index == 0) { > > > > > + dev_err(dev, "Device specific compatible needed\n"); > > > > > + return -EINVAL; -ENODEV? So a "more generic" driver can take over? > > > > > + } > > > > > + } > > > > > + > > > > > lvds_codec->powerdown_gpio = devm_gpiod_get_optional(dev, "powerdown", > > > > > GPIOD_OUT_HIGH); > > > > > if (IS_ERR(lvds_codec->powerdown_gpio)) { > > > > > @@ -92,7 +120,7 @@ static int lvds_codec_probe(struct platform_device *pdev) > > > > > > > > > > lvds_codec->panel_bridge = > > > > > devm_drm_panel_bridge_add_typed(dev, panel, > > > > > - lvds_codec->connector_type); > > > > > + lvds_codec->data->connector_type); > > > > > if (IS_ERR(lvds_codec->panel_bridge)) > > > > > return PTR_ERR(lvds_codec->panel_bridge); > > > > > > > > > > @@ -119,18 +147,33 @@ static int lvds_codec_remove(struct platform_device *pdev) > > > > > return 0; > > > > > } > > > > > > > > > > +static const struct lvds_codec_data lvds_codec_decoder_data = { > > > > > + .connector_type = DRM_MODE_CONNECTOR_DPI, > > > > > + .device_specific = false, > > > > > +}; > > > > > + > > > > > +static const struct lvds_codec_data lvds_codec_encoder_data = { > > > > > + .connector_type = DRM_MODE_CONNECTOR_LVDS, > > > > > + .device_specific = false, > > > > > +}; > > > > > + > > > > > +static const struct lvds_codec_data lvds_codec_thc63lvdm83d_data = { > > > > > + .connector_type = DRM_MODE_CONNECTOR_LVDS, > > > > > + .device_specific = true, > > > > > +}; > > > > > + > > > > > static const struct of_device_id lvds_codec_match[] = { > > > > > { > > > > > .compatible = "lvds-decoder", > > > > > - .data = (void *)DRM_MODE_CONNECTOR_DPI, > > > > > + .data = &lvds_codec_decoder_data, > > > > > }, > > > > > { > > > > > .compatible = "lvds-encoder", > > > > > - .data = (void *)DRM_MODE_CONNECTOR_LVDS, > > > > > + .data = &lvds_codec_encoder_data, > > > > > }, > > > > > { > > > > > .compatible = "thine,thc63lvdm83d", > > > > > - .data = (void *)DRM_MODE_CONNECTOR_LVDS, > > > > > + .data = &lvds_codec_thc63lvdm83d_data, > > > > > }, > > > > > {}, > > > > > }; Gr{oetje,eeting}s, Geert
On Tue, Nov 19, 2019 at 1:10 AM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > On Wed, Nov 13, 2019 at 03:51:29PM +0000, Fabrizio Castro wrote: > > The iwg20d comes with a 7" capacitive touch screen, therefore > > add support for it. > > > > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com> > > Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > I expect Geert to pick this up. Thanks, queued in renesas-devel for v5.6. 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
On Tue, Nov 19, 2019 at 1:12 AM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > On Wed, Nov 13, 2019 at 03:51:30PM +0000, Fabrizio Castro wrote: > > The iwg20d comes with an LCD panel from Emerging Display > > Technologies Corporation (EDT), therefore enable what's > > required to support it. > > > > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com> > > Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > I expect Geert to pick this up. Thanks, queued in renesas-devel for v5.6. Gr{oetje,eeting}s, Geert
Hi Geert, > From: Geert Uytterhoeven <geert@linux-m68k.org> > Sent: 22 November 2019 08:17 > Subject: Re: [PATCH v4 12/13] [HACK] drm/bridge: lvds-codec: Enforce device specific compatible strings > > Hi Fabrizio, > > On Thu, Nov 21, 2019 at 5:00 PM Fabrizio Castro > <fabrizio.castro@bp.renesas.com> wrote: > > > From: devicetree-owner@vger.kernel.org <devicetree-owner@vger.kernel.org> On Behalf Of Laurent Pinchart > > > Sent: 19 November 2019 21:52 > > > Subject: Re: [PATCH v4 12/13] [HACK] drm/bridge: lvds-codec: Enforce device specific compatible strings > > > > > > On Tue, Nov 19, 2019 at 11:17:34AM +0000, Fabrizio Castro wrote: > > > > On 19 November 2019 00:16 Laurent Pinchart wrote: > > > > > On Wed, Nov 13, 2019 at 03:51:31PM +0000, Fabrizio Castro wrote: > > > > > > The lvds-codec driver is a generic stub for transparent LVDS > > > > > > encoders and decoders. > > > > > > It's good practice to list a device specific compatible string > > > > > > before the generic fallback (if any) in the DT node for the relevant > > > > > > LVDS encoder/decoder, and it's also required by the dt-bindings. > > > > > > A notable exception to the generic fallback mechanism is the case > > > > > > of "thine,thc63lvdm83d", as documented in: > > > > > > Documentation/devicetree/bindings/display/bridge/thine,thc63lvdm83d.txt > > > > > > This patch enforces the adoption of a device specific compatible > > > > > > string (as fist string in the list), by using markers for the > > > > > > compatible string we match against and the index of the matching > > > > > > compatible string in the list. > > > > > > > > > > > > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com> > > > > > > > > > > > > --- > > > > > > Hi Laurent, > > > > > > > > > > > > I don't think we need to do anything in the driver to address your > > > > > > comment, as we can "enforce" this with the bindings (please see the > > > > > > next patch, as it would help with the "enforcing" of the compatible > > > > > > string for the thine device). > > > > > > I am sending this patch only so that you can see what a possible > > > > > > solution in the driver could look like. > > > > > > > > > > > > v3->v4: > > > > > > * New patch addressing the below comment from Laurent: > > > > > > "I think the lvds-decoder driver should error out at probe time if only > > > > > > one compatible string is listed." > > > > > > > --- a/drivers/gpu/drm/bridge/lvds-codec.c > > > > > > +++ b/drivers/gpu/drm/bridge/lvds-codec.c > > > > > > > @@ -65,7 +70,30 @@ 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->data = of_device_get_match_data(&pdev->dev); > > > > > > + if (!lvds_codec->data) > > > > > > + return -EINVAL; > > > > > > + > > > > > > + /* > > > > > > + * If we haven't matched a device specific compatible string, we need > > > > > > + * to work out if the generic compatible string we matched against was > > > > > > + * listed first in the compatible property. > > > > > > + */ > > > > > > > > > > Can't we do this unconditionally, and thus drop the lvds_codec_data > > > > > structure ? > > > > > > > > I don't think so, and the reason for this is that we have a corner case for > > > > thine,thc63lvdm83d. Here is what's allowed (according to the documentation) > > > > from what's supported upstream (+ this series): > > > > "ti,ds90c185", "lvds-encoder" > > > > "ti,ds90c187", "lvds-encoder" > > > > "ti,sn75lvds83", "lvds-encoder" > > > > "ti,ds90cf384a", "lvds-decoder" > > > > "thine,thc63lvdm83d" > > > > > > > > As you can see from the examples above, in most cases it's enough to say it's > > > > all good when we match a compatible string with index > 0, but for the thine > > > > device you _have_ to match the string with index 0 as that's what's currently > > > > documented (please see thine,thc63lvdm83d.txt) and that's what's supported > > > > by device trees already (please see arch/arm/boot/dts/r8a7779-marzen.dts). > > > > > > How about the following logic ? > > > > > > if (match_index("lvds-encoder") == 0 || > > > match_index("lvds-decoder") == 0) > > > return -EINVAL; > > > > > > > > > > Now I see what you mean > > > > > > This patch "classifies" compatible strings, and it considers a good match > > > > device specific compatible strings, or generic compatible strings as long > > > > as they are not listed first. > > > > > > > > These days you can leverage the yaml files to validate the device trees, > > > > therefore we should be focusing on writing yaml files in such a way we only > > > > pass the checks we mean to, and by checks I mean: > > > > make dtbs_check > > > > > > > > or more specifically, for this series: > > > > make dtbs_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml > > > > > > > > and that's of course on top of make dt_binding_check. > > > > > > Sure, but that doesn't prevent anyone ignoring the validation. > > > > > > > It's a very common requirement to have a part number specific compatible > > > > string first followed by a generic (fallback) compatible string in the device trees, > > > > most drivers for Renesas SoCs have similar requirements. > > > > > > > > If we start doing this here, we'll end up doing it elsewhere as well, and I really > > > > think we shouldn't, but others may see things differently, so I'll wait for others > > > > (and yourself with further comments) to jump in before doing any more work > > > > on this patch. > > > > > > I agree with this argument, it would set a precedent, and is probably > > > not worth duplicating similar code in all drivers. I wonder if this is > > > something we could handle with core helpers, but maybe it's overkill. > > > > I was hoping others would comment as well, but perhaps this topic is not too exciting. > > > > Geert, what do you think about this? Is this something we should enforce > > in drivers? > > So IIUIC, you want to enforce the presence of both specific and generic > compatible values (in that order) in the driver (except for > "thine,thc63lvdm83d", as that predates the introduction of the generic > compatible value)? Yeah, this is what Laurent would want ideally. > However, the driver would not really care about the actual hardware-specific > value, as it would still match against the generic one, and the > hardware-specific one may not even be listed in the driver's match table? Exactly. > > By definition, you can have one or more compatible values listed in a > device node, from most-specific to least-specific. Typically the driver > cannot know if a more specific value is missing, but YAML DT binding > validation can. > > In this case it is a bit special, as there is a generic one involved, so > you can assume there should be a more specific one, too. > If you want to handle this in the core, you probably need to add an > "is_generic" flag to struct of_device_id. That's actually an interesting way of looking at this. Laurent? Thanks, Fab > > Rob/Mark? > > > > > > > + if (!lvds_codec->data->device_specific) { > > > > > > + const struct of_device_id *match; > > > > > > + int compatible_index; > > > > > > + > > > > > > + match = of_match_node(dev->driver->of_match_table, > > > > > > + dev->of_node); > > > > > > + compatible_index = of_property_match_string(dev->of_node, > > > > > > + "compatible", > > > > > > + match->compatible); > > > > > > + if (compatible_index == 0) { > > > > > > + dev_err(dev, "Device specific compatible needed\n"); > > > > > > + return -EINVAL; > > -ENODEV? > So a "more generic" driver can take over? > > > > > > > + } > > > > > > + } > > > > > > + > > > > > > lvds_codec->powerdown_gpio = devm_gpiod_get_optional(dev, "powerdown", > > > > > > GPIOD_OUT_HIGH); > > > > > > if (IS_ERR(lvds_codec->powerdown_gpio)) { > > > > > > @@ -92,7 +120,7 @@ static int lvds_codec_probe(struct platform_device *pdev) > > > > > > > > > > > > lvds_codec->panel_bridge = > > > > > > devm_drm_panel_bridge_add_typed(dev, panel, > > > > > > - lvds_codec->connector_type); > > > > > > + lvds_codec->data->connector_type); > > > > > > if (IS_ERR(lvds_codec->panel_bridge)) > > > > > > return PTR_ERR(lvds_codec->panel_bridge); > > > > > > > > > > > > @@ -119,18 +147,33 @@ static int lvds_codec_remove(struct platform_device *pdev) > > > > > > return 0; > > > > > > } > > > > > > > > > > > > +static const struct lvds_codec_data lvds_codec_decoder_data = { > > > > > > + .connector_type = DRM_MODE_CONNECTOR_DPI, > > > > > > + .device_specific = false, > > > > > > +}; > > > > > > + > > > > > > +static const struct lvds_codec_data lvds_codec_encoder_data = { > > > > > > + .connector_type = DRM_MODE_CONNECTOR_LVDS, > > > > > > + .device_specific = false, > > > > > > +}; > > > > > > + > > > > > > +static const struct lvds_codec_data lvds_codec_thc63lvdm83d_data = { > > > > > > + .connector_type = DRM_MODE_CONNECTOR_LVDS, > > > > > > + .device_specific = true, > > > > > > +}; > > > > > > + > > > > > > static const struct of_device_id lvds_codec_match[] = { > > > > > > { > > > > > > .compatible = "lvds-decoder", > > > > > > - .data = (void *)DRM_MODE_CONNECTOR_DPI, > > > > > > + .data = &lvds_codec_decoder_data, > > > > > > }, > > > > > > { > > > > > > .compatible = "lvds-encoder", > > > > > > - .data = (void *)DRM_MODE_CONNECTOR_LVDS, > > > > > > + .data = &lvds_codec_encoder_data, > > > > > > }, > > > > > > { > > > > > > .compatible = "thine,thc63lvdm83d", > > > > > > - .data = (void *)DRM_MODE_CONNECTOR_LVDS, > > > > > > + .data = &lvds_codec_thc63lvdm83d_data, > > > > > > }, > > > > > > {}, > > > > > > }; > > 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 Mon, Nov 25, 2019 at 11:17:25AM +0000, Fabrizio Castro wrote: > On 22 November 2019 08:17 Geert Uytterhoeven wrote: > > On Thu, Nov 21, 2019 at 5:00 PM Fabrizio Castro wrote: > >> On 19 November 2019 21:52 Laurent Pinchart wrote: > >>> On Tue, Nov 19, 2019 at 11:17:34AM +0000, Fabrizio Castro wrote: > >>>> On 19 November 2019 00:16 Laurent Pinchart wrote: > >>>>> On Wed, Nov 13, 2019 at 03:51:31PM +0000, Fabrizio Castro wrote: > >>>>>> The lvds-codec driver is a generic stub for transparent LVDS > >>>>>> encoders and decoders. > >>>>>> It's good practice to list a device specific compatible string > >>>>>> before the generic fallback (if any) in the DT node for the relevant > >>>>>> LVDS encoder/decoder, and it's also required by the dt-bindings. > >>>>>> A notable exception to the generic fallback mechanism is the case > >>>>>> of "thine,thc63lvdm83d", as documented in: > >>>>>> Documentation/devicetree/bindings/display/bridge/thine,thc63lvdm83d.txt > >>>>>> This patch enforces the adoption of a device specific compatible > >>>>>> string (as fist string in the list), by using markers for the > >>>>>> compatible string we match against and the index of the matching > >>>>>> compatible string in the list. > >>>>>> > >>>>>> Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com> > >>>>>> > >>>>>> --- > >>>>>> Hi Laurent, > >>>>>> > >>>>>> I don't think we need to do anything in the driver to address your > >>>>>> comment, as we can "enforce" this with the bindings (please see the > >>>>>> next patch, as it would help with the "enforcing" of the compatible > >>>>>> string for the thine device). > >>>>>> I am sending this patch only so that you can see what a possible > >>>>>> solution in the driver could look like. > >>>>>> > >>>>>> v3->v4: > >>>>>> * New patch addressing the below comment from Laurent: > >>>>>> "I think the lvds-decoder driver should error out at probe time if only > >>>>>> one compatible string is listed." > >>>>>> > >>>>>> --- a/drivers/gpu/drm/bridge/lvds-codec.c > >>>>>> +++ b/drivers/gpu/drm/bridge/lvds-codec.c > >>>>>> > >>>>>> @@ -65,7 +70,30 @@ 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->data = of_device_get_match_data(&pdev->dev); > >>>>>> + if (!lvds_codec->data) > >>>>>> + return -EINVAL; > >>>>>> + > >>>>>> + /* > >>>>>> + * If we haven't matched a device specific compatible string, we need > >>>>>> + * to work out if the generic compatible string we matched against was > >>>>>> + * listed first in the compatible property. > >>>>>> + */ > >>>>> > >>>>> Can't we do this unconditionally, and thus drop the lvds_codec_data > >>>>> structure ? > >>>> > >>>> I don't think so, and the reason for this is that we have a corner case for > >>>> thine,thc63lvdm83d. Here is what's allowed (according to the documentation) > >>>> from what's supported upstream (+ this series): > >>>> "ti,ds90c185", "lvds-encoder" > >>>> "ti,ds90c187", "lvds-encoder" > >>>> "ti,sn75lvds83", "lvds-encoder" > >>>> "ti,ds90cf384a", "lvds-decoder" > >>>> "thine,thc63lvdm83d" > >>>> > >>>> As you can see from the examples above, in most cases it's enough to say it's > >>>> all good when we match a compatible string with index > 0, but for the thine > >>>> device you _have_ to match the string with index 0 as that's what's currently > >>>> documented (please see thine,thc63lvdm83d.txt) and that's what's supported > >>>> by device trees already (please see arch/arm/boot/dts/r8a7779-marzen.dts). > >>> > >>> How about the following logic ? > >>> > >>> if (match_index("lvds-encoder") == 0 || > >>> match_index("lvds-decoder") == 0) > >>> return -EINVAL; > >>> > >>> > >> > >> Now I see what you mean > >> > >>>> This patch "classifies" compatible strings, and it considers a good match > >>>> device specific compatible strings, or generic compatible strings as long > >>>> as they are not listed first. > >>>> > >>>> These days you can leverage the yaml files to validate the device trees, > >>>> therefore we should be focusing on writing yaml files in such a way we only > >>>> pass the checks we mean to, and by checks I mean: > >>>> make dtbs_check > >>>> > >>>> or more specifically, for this series: > >>>> make dtbs_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml > >>>> > >>>> and that's of course on top of make dt_binding_check. > >>> > >>> Sure, but that doesn't prevent anyone ignoring the validation. > >>> > >>>> It's a very common requirement to have a part number specific compatible > >>>> string first followed by a generic (fallback) compatible string in the device trees, > >>>> most drivers for Renesas SoCs have similar requirements. > >>>> > >>>> If we start doing this here, we'll end up doing it elsewhere as well, and I really > >>>> think we shouldn't, but others may see things differently, so I'll wait for others > >>>> (and yourself with further comments) to jump in before doing any more work > >>>> on this patch. > >>> > >>> I agree with this argument, it would set a precedent, and is probably > >>> not worth duplicating similar code in all drivers. I wonder if this is > >>> something we could handle with core helpers, but maybe it's overkill. > >> > >> I was hoping others would comment as well, but perhaps this topic is not too exciting. > >> > >> Geert, what do you think about this? Is this something we should enforce > >> in drivers? > > > > So IIUIC, you want to enforce the presence of both specific and generic > > compatible values (in that order) in the driver (except for > > "thine,thc63lvdm83d", as that predates the introduction of the generic > > compatible value)? > > Yeah, this is what Laurent would want ideally. > > > However, the driver would not really care about the actual hardware-specific > > value, as it would still match against the generic one, and the > > hardware-specific one may not even be listed in the driver's match table? > > Exactly. > > > By definition, you can have one or more compatible values listed in a > > device node, from most-specific to least-specific. Typically the driver > > cannot know if a more specific value is missing, but YAML DT binding > > validation can. > > > > In this case it is a bit special, as there is a generic one involved, so > > you can assume there should be a more specific one, too. > > If you want to handle this in the core, you probably need to add an > > "is_generic" flag to struct of_device_id. > > That's actually an interesting way of looking at this. > Laurent? I like the idea, it's better than doing it in each driver. > > Rob/Mark? Ping ? > >>>>>> + if (!lvds_codec->data->device_specific) { > >>>>>> + const struct of_device_id *match; > >>>>>> + int compatible_index; > >>>>>> + > >>>>>> + match = of_match_node(dev->driver->of_match_table, > >>>>>> + dev->of_node); > >>>>>> + compatible_index = of_property_match_string(dev->of_node, > >>>>>> + "compatible", > >>>>>> + match->compatible); > >>>>>> + if (compatible_index == 0) { > >>>>>> + dev_err(dev, "Device specific compatible needed\n"); > >>>>>> + return -EINVAL; > > > > -ENODEV? > > So a "more generic" driver can take over? > > > >>>>>> + } > >>>>>> + } > >>>>>> + > >>>>>> lvds_codec->powerdown_gpio = devm_gpiod_get_optional(dev, "powerdown", > >>>>>> GPIOD_OUT_HIGH); > >>>>>> if (IS_ERR(lvds_codec->powerdown_gpio)) { > >>>>>> @@ -92,7 +120,7 @@ static int lvds_codec_probe(struct platform_device *pdev) > >>>>>> > >>>>>> lvds_codec->panel_bridge = > >>>>>> devm_drm_panel_bridge_add_typed(dev, panel, > >>>>>> - lvds_codec->connector_type); > >>>>>> + lvds_codec->data->connector_type); > >>>>>> if (IS_ERR(lvds_codec->panel_bridge)) > >>>>>> return PTR_ERR(lvds_codec->panel_bridge); > >>>>>> > >>>>>> @@ -119,18 +147,33 @@ static int lvds_codec_remove(struct platform_device *pdev) > >>>>>> return 0; > >>>>>> } > >>>>>> > >>>>>> +static const struct lvds_codec_data lvds_codec_decoder_data = { > >>>>>> + .connector_type = DRM_MODE_CONNECTOR_DPI, > >>>>>> + .device_specific = false, > >>>>>> +}; > >>>>>> + > >>>>>> +static const struct lvds_codec_data lvds_codec_encoder_data = { > >>>>>> + .connector_type = DRM_MODE_CONNECTOR_LVDS, > >>>>>> + .device_specific = false, > >>>>>> +}; > >>>>>> + > >>>>>> +static const struct lvds_codec_data lvds_codec_thc63lvdm83d_data = { > >>>>>> + .connector_type = DRM_MODE_CONNECTOR_LVDS, > >>>>>> + .device_specific = true, > >>>>>> +}; > >>>>>> + > >>>>>> static const struct of_device_id lvds_codec_match[] = { > >>>>>> { > >>>>>> .compatible = "lvds-decoder", > >>>>>> - .data = (void *)DRM_MODE_CONNECTOR_DPI, > >>>>>> + .data = &lvds_codec_decoder_data, > >>>>>> }, > >>>>>> { > >>>>>> .compatible = "lvds-encoder", > >>>>>> - .data = (void *)DRM_MODE_CONNECTOR_LVDS, > >>>>>> + .data = &lvds_codec_encoder_data, > >>>>>> }, > >>>>>> { > >>>>>> .compatible = "thine,thc63lvdm83d", > >>>>>> - .data = (void *)DRM_MODE_CONNECTOR_LVDS, > >>>>>> + .data = &lvds_codec_thc63lvdm83d_data, > >>>>>> }, > >>>>>> {}, > >>>>>> };
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 :-)
Hi, On 13/11/2019 16:51, Fabrizio Castro wrote: > The iW-RainboW-G20D-Qseven RZ/G1M,G1N Qseven Development Platform > comes with a 7" capacitive display kit from Emerging Display > Technologies Corporation (EDT). This series adds all that's > necessary for supporting it. > > Thanks, > Fab > > v3->v4: > * Reworked according to Laurent's and Jacopo's comments > v2->v3: > * Split the dt-schema patch in two patches as per Rob's comment > * Made fixes to the dt-schema according to Rob's comment > * Made fixes to the lvds-codec driver according to Jacopo's comments > * Added two new patches: > * drm: Define DRM_MODE_CONNECTOR_PARALLEL > * drm/panel: panel-simple: Add connector type for etm0700g0dh6 > v1->v2: > * Convert dt-bindings to dt-schema > > Fabrizio Castro (13): > dt-bindings: display: bridge: Convert lvds-transmitter binding to > json-schema > dt-bindings: display: bridge: lvds-transmitter: Document > powerdown-gpios > dt-bindings: display: bridge: lvds-transmitter: Absorb ti,ds90c185.txt > dt-bindings: display: bridge: lvds-transmitter: Document > "ti,sn75lvds83" > drm/bridge: Repurpose lvds-encoder.c > drm/bridge: lvds-codec: Add "lvds-decoder" support > drm/bridge: lvds-codec: Simplify panel DT node localisation > dt-bindings: display: bridge: Repurpose lvds-encoder > dt-bindings: display: bridge: lvds-codec: Document ti,ds90cf384a > ARM: dts: iwg20d-q7-common: Add LCD support > ARM: shmobile_defconfig: Enable support for panels from EDT > [HACK] drm/bridge: lvds-codec: Enforce device specific compatible > strings > [HACK] dt-bindings: display: bridge: lvds-codec: Absorb > thine,thc63lvdm83d.txt > > .../bindings/display/bridge/lvds-codec.yaml | 131 +++++++++++++++ > .../bindings/display/bridge/lvds-transmitter.txt | 66 -------- > .../bindings/display/bridge/thine,thc63lvdm83d.txt | 50 ------ > .../bindings/display/bridge/ti,ds90c185.txt | 55 ------- > arch/arm/boot/dts/iwg20d-q7-common.dtsi | 85 ++++++++++ > arch/arm/boot/dts/iwg20d-q7-dbcm-ca.dtsi | 1 - > arch/arm/configs/shmobile_defconfig | 3 + > drivers/gpu/drm/bridge/Kconfig | 8 +- > drivers/gpu/drm/bridge/Makefile | 2 +- > .../drm/bridge/{lvds-encoder.c => lvds-codec.c} | 179 +++++++++++++-------- > 10 files changed, 333 insertions(+), 247 deletions(-) > create mode 100644 Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml > delete mode 100644 Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt > delete mode 100644 Documentation/devicetree/bindings/display/bridge/thine,thc63lvdm83d.txt > delete mode 100644 Documentation/devicetree/bindings/display/bridge/ti,ds90c185.txt > rename drivers/gpu/drm/bridge/{lvds-encoder.c => lvds-codec.c} (25%) > Applying patches 1->9 and 13 to drm-misc-next with correct SoB. Also applied change on patch 8 while applying. And checked the diff is the same as laurent's PR. Neil
> 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
Hi Neil, > From: Neil Armstrong <narmstrong@baylibre.com> > Sent: 18 December 2019 11:53 > Subject: Re: [PATCH v4 00/13] Add LCD panel support to iwg20d > > Hi, > > On 13/11/2019 16:51, Fabrizio Castro wrote: > > The iW-RainboW-G20D-Qseven RZ/G1M,G1N Qseven Development Platform > > comes with a 7" capacitive display kit from Emerging Display > > Technologies Corporation (EDT). This series adds all that's > > necessary for supporting it. > > > > Thanks, > > Fab > > > > v3->v4: > > * Reworked according to Laurent's and Jacopo's comments > > v2->v3: > > * Split the dt-schema patch in two patches as per Rob's comment > > * Made fixes to the dt-schema according to Rob's comment > > * Made fixes to the lvds-codec driver according to Jacopo's comments > > * Added two new patches: > > * drm: Define DRM_MODE_CONNECTOR_PARALLEL > > * drm/panel: panel-simple: Add connector type for etm0700g0dh6 > > v1->v2: > > * Convert dt-bindings to dt-schema > > > > Fabrizio Castro (13): > > dt-bindings: display: bridge: Convert lvds-transmitter binding to > > json-schema > > dt-bindings: display: bridge: lvds-transmitter: Document > > powerdown-gpios > > dt-bindings: display: bridge: lvds-transmitter: Absorb ti,ds90c185.txt > > dt-bindings: display: bridge: lvds-transmitter: Document > > "ti,sn75lvds83" > > drm/bridge: Repurpose lvds-encoder.c > > drm/bridge: lvds-codec: Add "lvds-decoder" support > > drm/bridge: lvds-codec: Simplify panel DT node localisation > > dt-bindings: display: bridge: Repurpose lvds-encoder > > dt-bindings: display: bridge: lvds-codec: Document ti,ds90cf384a > > ARM: dts: iwg20d-q7-common: Add LCD support > > ARM: shmobile_defconfig: Enable support for panels from EDT > > [HACK] drm/bridge: lvds-codec: Enforce device specific compatible > > strings > > [HACK] dt-bindings: display: bridge: lvds-codec: Absorb > > thine,thc63lvdm83d.txt > > > > .../bindings/display/bridge/lvds-codec.yaml | 131 +++++++++++++++ > > .../bindings/display/bridge/lvds-transmitter.txt | 66 -------- > > .../bindings/display/bridge/thine,thc63lvdm83d.txt | 50 ------ > > .../bindings/display/bridge/ti,ds90c185.txt | 55 ------- > > arch/arm/boot/dts/iwg20d-q7-common.dtsi | 85 ++++++++++ > > arch/arm/boot/dts/iwg20d-q7-dbcm-ca.dtsi | 1 - > > arch/arm/configs/shmobile_defconfig | 3 + > > drivers/gpu/drm/bridge/Kconfig | 8 +- > > drivers/gpu/drm/bridge/Makefile | 2 +- > > .../drm/bridge/{lvds-encoder.c => lvds-codec.c} | 179 +++++++++++++-------- > > 10 files changed, 333 insertions(+), 247 deletions(-) > > create mode 100644 Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml > > delete mode 100644 Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt > > delete mode 100644 Documentation/devicetree/bindings/display/bridge/thine,thc63lvdm83d.txt > > delete mode 100644 Documentation/devicetree/bindings/display/bridge/ti,ds90c185.txt > > rename drivers/gpu/drm/bridge/{lvds-encoder.c => lvds-codec.c} (25%) > > > > Applying patches 1->9 and 13 to drm-misc-next with correct SoB. > > Also applied change on patch 8 while applying. > > And checked the diff is the same as laurent's PR. Thank you. Fab > > Neil
The iW-RainboW-G20D-Qseven RZ/G1M,G1N Qseven Development Platform comes with a 7" capacitive display kit from Emerging Display Technologies Corporation (EDT). This series adds all that's necessary for supporting it. Thanks, Fab v3->v4: * Reworked according to Laurent's and Jacopo's comments v2->v3: * Split the dt-schema patch in two patches as per Rob's comment * Made fixes to the dt-schema according to Rob's comment * Made fixes to the lvds-codec driver according to Jacopo's comments * Added two new patches: * drm: Define DRM_MODE_CONNECTOR_PARALLEL * drm/panel: panel-simple: Add connector type for etm0700g0dh6 v1->v2: * Convert dt-bindings to dt-schema Fabrizio Castro (13): dt-bindings: display: bridge: Convert lvds-transmitter binding to json-schema dt-bindings: display: bridge: lvds-transmitter: Document powerdown-gpios dt-bindings: display: bridge: lvds-transmitter: Absorb ti,ds90c185.txt dt-bindings: display: bridge: lvds-transmitter: Document "ti,sn75lvds83" drm/bridge: Repurpose lvds-encoder.c drm/bridge: lvds-codec: Add "lvds-decoder" support drm/bridge: lvds-codec: Simplify panel DT node localisation dt-bindings: display: bridge: Repurpose lvds-encoder dt-bindings: display: bridge: lvds-codec: Document ti,ds90cf384a ARM: dts: iwg20d-q7-common: Add LCD support ARM: shmobile_defconfig: Enable support for panels from EDT [HACK] drm/bridge: lvds-codec: Enforce device specific compatible strings [HACK] dt-bindings: display: bridge: lvds-codec: Absorb thine,thc63lvdm83d.txt .../bindings/display/bridge/lvds-codec.yaml | 131 +++++++++++++++ .../bindings/display/bridge/lvds-transmitter.txt | 66 -------- .../bindings/display/bridge/thine,thc63lvdm83d.txt | 50 ------ .../bindings/display/bridge/ti,ds90c185.txt | 55 ------- arch/arm/boot/dts/iwg20d-q7-common.dtsi | 85 ++++++++++ arch/arm/boot/dts/iwg20d-q7-dbcm-ca.dtsi | 1 - arch/arm/configs/shmobile_defconfig | 3 + drivers/gpu/drm/bridge/Kconfig | 8 +- drivers/gpu/drm/bridge/Makefile | 2 +- .../drm/bridge/{lvds-encoder.c => lvds-codec.c} | 179 +++++++++++++-------- 10 files changed, 333 insertions(+), 247 deletions(-) create mode 100644 Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml delete mode 100644 Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt delete mode 100644 Documentation/devicetree/bindings/display/bridge/thine,thc63lvdm83d.txt delete mode 100644 Documentation/devicetree/bindings/display/bridge/ti,ds90c185.txt rename drivers/gpu/drm/bridge/{lvds-encoder.c => lvds-codec.c} (25%)