Re: [PATCH v3 11/50] drm/bridge: Add bridge driver for display connectors
On 18/12/2019 03:45, Laurent Pinchart wrote: + dev_info(&pdev->dev, +"Found %s display connector '%s' %s DDC bus and %s HPD GPIO (ops 0x%x)\n", +drm_get_connector_type_name(conn->bridge.type), +conn->label ? conn->label : "", +conn->bridge.ddc ? "with" : "without", +conn->hpd_gpio ? "with" : "without", +conn->bridge.ops); On AM5 EVM, we have HDMI output with DDC and HPD, but I get a kernel print: display-connector connector: Found HDMI-A display connector 'hdmi' without DDC bus and without HPD GPIO (ops 0x0) I think that print may be quite confusing for someone who doesn't know the details of the drivers involved. I agree, but the information could be useful for developers. Do you think it should be dropped ? Or do you have an alternative wording to propose ? I would just go with dev_dbg. I personally don't like to use dev_info unless it's really something a normal user needs to see. Otherwise it's just spam for most of the people. If everything is fine, I think the driver should be quiet. Did we discuss this already earlier? =) That said, even as a dev_dbg it's somewhat confusing. But I can't come up with alternate working... Somehow it should indicate that this piece of the display pipeline doesn't handle DDC/HPD, without implying that there are no such features. "unassigned DDC"? I don't know... I'm fine with the print as dev_dbg. Tomi -- Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3 11/50] drm/bridge: Add bridge driver for display connectors
Hi Sam,
Thank you for the review.
On Sun, Dec 15, 2019 at 01:03:31PM +0100, Sam Ravnborg wrote:
> Hi Laurent.
>
> One nit below.
>
> > +
> > +struct display_connector {
> > + struct drm_bridge bridge;
> > +
> > + const char *label;
>
> label is defined here.
>
> > + struct gpio_desc*hpd_gpio;
> > + int hpd_irq;
> > +};
> > +
> ...
> > +
> > + /* Get the optional connector label. */
> > + of_property_read_string(pdev->dev.of_node, "label", &conn->label);
>
> Assinged here.
>
> > +
> > + /*
> > +* Get the HPD GPIO for DVI and HDMI connectors. If the GPIO can provide
> > +* edge interrupts, register an interrupt handler.
> > +*/
>...
> > +
> > + dev_info(&pdev->dev,
> > +"Found %s display connector '%s' %s DDC bus and %s HPD GPIO
> > (ops 0x%x)\n",
> > +drm_get_connector_type_name(conn->bridge.type),
> > +conn->label ? conn->label : "",
> > +conn->bridge.ddc ? "with" : "without",
> > +conn->hpd_gpio ? "with" : "without",
> > +conn->bridge.ops);
>
> And this is the only user - within the same function where label is
> assigned.
> We could use a loacal variable, no need to have it in struct display_connector
> unless futher use is planned.
Agreed, I'll move the field to a local variable. We can always move it
back to the display_connector structure later if needed.
--
Regards,
Laurent Pinchart
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3 11/50] drm/bridge: Add bridge driver for display connectors
Hi Tomi, On Mon, Dec 16, 2019 at 03:11:23PM +0200, Tomi Valkeinen wrote: > On 11/12/2019 00:57, Laurent Pinchart wrote: > > Display connectors are modelled in DT as a device node, but have so far > > been handled manually in several bridge drivers. This resulted in > > duplicate code in several bridge drivers, with slightly different (and > > thus confusing) logics. > > > > In order to fix this, implement a bridge driver for display connectors. > > The driver centralises logic for the DVI, HDMI, VGAn composite and > > S-video connectors and exposes corresponding bridge operations. > > > > This driver in itself doesn't solve the issue completely, changes in > > bridge and display controller drivers are needed to make use of the new > > connector driver. > > > > Signed-off-by: Laurent Pinchart > > Reviewed-by: Maxime Ripard > > --- > > Changes since v2: > > > > - Fall back to polling if the GPIO IRQ chip doesn't support > >edge-triggered interrupts > > > > Changes since v1: > > > > - Use drm_get_connector_type_name() instead of open-coding > >display_connector_type_name() > > - Remove empty .hpd_enable() and .hpd_disable() operations > > - Set bridge.ddc > > --- > > drivers/gpu/drm/bridge/Kconfig | 11 + > > drivers/gpu/drm/bridge/Makefile| 1 + > > drivers/gpu/drm/bridge/display-connector.c | 292 + > > 3 files changed, 304 insertions(+) > > create mode 100644 drivers/gpu/drm/bridge/display-connector.c > > > > > + dev_info(&pdev->dev, > > +"Found %s display connector '%s' %s DDC bus and %s HPD GPIO > > (ops 0x%x)\n", > > +drm_get_connector_type_name(conn->bridge.type), > > +conn->label ? conn->label : "", > > +conn->bridge.ddc ? "with" : "without", > > +conn->hpd_gpio ? "with" : "without", > > +conn->bridge.ops); > > On AM5 EVM, we have HDMI output with DDC and HPD, but I get a kernel print: > > display-connector connector: Found HDMI-A display connector 'hdmi' without > DDC bus and without HPD > GPIO (ops 0x0) > > I think that print may be quite confusing for someone who doesn't know the > details of the drivers > involved. I agree, but the information could be useful for developers. Do you think it should be dropped ? Or do you have an alternative wording to propose ? -- Regards, Laurent Pinchart ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3 11/50] drm/bridge: Add bridge driver for display connectors
On 11/12/2019 00:57, Laurent Pinchart wrote: Display connectors are modelled in DT as a device node, but have so far been handled manually in several bridge drivers. This resulted in duplicate code in several bridge drivers, with slightly different (and thus confusing) logics. In order to fix this, implement a bridge driver for display connectors. The driver centralises logic for the DVI, HDMI, VGAn composite and S-video connectors and exposes corresponding bridge operations. This driver in itself doesn't solve the issue completely, changes in bridge and display controller drivers are needed to make use of the new connector driver. Signed-off-by: Laurent Pinchart Reviewed-by: Maxime Ripard --- Changes since v2: - Fall back to polling if the GPIO IRQ chip doesn't support edge-triggered interrupts Changes since v1: - Use drm_get_connector_type_name() instead of open-coding display_connector_type_name() - Remove empty .hpd_enable() and .hpd_disable() operations - Set bridge.ddc --- drivers/gpu/drm/bridge/Kconfig | 11 + drivers/gpu/drm/bridge/Makefile| 1 + drivers/gpu/drm/bridge/display-connector.c | 292 + 3 files changed, 304 insertions(+) create mode 100644 drivers/gpu/drm/bridge/display-connector.c + dev_info(&pdev->dev, +"Found %s display connector '%s' %s DDC bus and %s HPD GPIO (ops 0x%x)\n", +drm_get_connector_type_name(conn->bridge.type), +conn->label ? conn->label : "", +conn->bridge.ddc ? "with" : "without", +conn->hpd_gpio ? "with" : "without", +conn->bridge.ops); On AM5 EVM, we have HDMI output with DDC and HPD, but I get a kernel print: display-connector connector: Found HDMI-A display connector 'hdmi' without DDC bus and without HPD GPIO (ops 0x0) I think that print may be quite confusing for someone who doesn't know the details of the drivers involved. Tomi -- Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3 11/50] drm/bridge: Add bridge driver for display connectors
On Wed, 11 Dec 2019 00:57:11 +0200 Laurent Pinchart wrote: > Display connectors are modelled in DT as a device node, but have so far > been handled manually in several bridge drivers. This resulted in > duplicate code in several bridge drivers, with slightly different (and > thus confusing) logics. > > In order to fix this, implement a bridge driver for display connectors. > The driver centralises logic for the DVI, HDMI, VGAn composite and > S-video connectors and exposes corresponding bridge operations. > > This driver in itself doesn't solve the issue completely, changes in > bridge and display controller drivers are needed to make use of the new > connector driver. > > Signed-off-by: Laurent Pinchart > Reviewed-by: Maxime Ripard Looks good to me: Reviewed-by: Boris Brezillon ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3 11/50] drm/bridge: Add bridge driver for display connectors
Hi Laurent.
One nit below.
> +
> +struct display_connector {
> + struct drm_bridge bridge;
> +
> + const char *label;
label is defined here.
> + struct gpio_desc*hpd_gpio;
> + int hpd_irq;
> +};
> +
...
> +
> + /* Get the optional connector label. */
> + of_property_read_string(pdev->dev.of_node, "label", &conn->label);
Assinged here.
> +
> + /*
> + * Get the HPD GPIO for DVI and HDMI connectors. If the GPIO can provide
> + * edge interrupts, register an interrupt handler.
> + */
...
> +
> + dev_info(&pdev->dev,
> + "Found %s display connector '%s' %s DDC bus and %s HPD GPIO
> (ops 0x%x)\n",
> + drm_get_connector_type_name(conn->bridge.type),
> + conn->label ? conn->label : "",
> + conn->bridge.ddc ? "with" : "without",
> + conn->hpd_gpio ? "with" : "without",
> + conn->bridge.ops);
And this is the only user - within the same function where label is
assigned.
We could use a loacal variable, no need to have it in struct display_connector
unless futher use is planned.
Sam
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
