[PATCH v6 01/24] drm: Add ddc link in sysfs created by drm_connector

[Laurent Pinchart]

Hi Andrzej,

Thank you for the patch, and sorry for the late review (I've been
travelling for the past few weeks).

On Fri, Jul 26, 2019 at 07:22:55PM +0200, Andrzej Pietrasiewicz wrote:
> Add generic code which creates symbolic links in sysfs, pointing to ddc
> interface used by a particular video output. For example:
> 
> ls -l /sys/class/drm/card0-HDMI-A-1/ddc
> lrwxrwxrwx 1 root root 0 Jun 24 10:42 /sys/class/drm/card0-HDMI-A-1/ddc \
> 	-> ../../../../soc/13880000.i2c/i2c-2
> 
> This makes it easy for user to associate a display with its ddc adapter
> and use e.g. ddcutil to control the chosen monitor.
> 
> This patch adds an i2c_adapter pointer to struct drm_connector. Particular
> drivers can then use it instead of using their own private instance. If a
> connector contains a ddc, then create a symbolic link in sysfs.
> 
> Signed-off-by: Andrzej Pietrasiewicz <andrzej.p at collabora.com>
> Acked-by: Daniel Vetter <daniel.vetter at ffwll.ch>
> Reviewed-by: Andrzej Hajda <a.hajda at samsung.com>
> ---
>  drivers/gpu/drm/drm_sysfs.c |  8 ++++++++
>  include/drm/drm_connector.h | 11 +++++++++++
>  2 files changed, 19 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c
> index ad10810bc972..e962a9d45f7e 100644
> --- a/drivers/gpu/drm/drm_sysfs.c
> +++ b/drivers/gpu/drm/drm_sysfs.c
> @@ -14,6 +14,7 @@
>  #include <linux/err.h>
>  #include <linux/export.h>
>  #include <linux/gfp.h>
> +#include <linux/i2c.h>
>  #include <linux/kdev_t.h>
>  #include <linux/slab.h>
>  
> @@ -294,6 +295,9 @@ int drm_sysfs_connector_add(struct drm_connector *connector)
>  	/* Let userspace know we have a new connector */
>  	drm_sysfs_hotplug_event(dev);
>  
> +	if (connector->ddc)
> +		return sysfs_create_link(&connector->kdev->kobj,
> +				 &connector->ddc->dev.kobj, "ddc");
>  	return 0;
>  }
>  
> @@ -301,6 +305,10 @@ void drm_sysfs_connector_remove(struct drm_connector *connector)
>  {
>  	if (!connector->kdev)
>  		return;
> +
> +	if (connector->ddc)
> +		sysfs_remove_link(&connector->kdev->kobj, "ddc");
> +
>  	DRM_DEBUG("removing \"%s\" from sysfs\n",
>  		  connector->name);
>  
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index 4c30d751487a..33a6fff85fdb 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -41,6 +41,7 @@ struct drm_property;
>  struct drm_property_blob;
>  struct drm_printer;
>  struct edid;
> +struct i2c_adapter;
>  
>  enum drm_connector_force {
>  	DRM_FORCE_UNSPECIFIED,
> @@ -1311,6 +1312,16 @@ struct drm_connector {
>  	 * [0]: progressive, [1]: interlaced
>  	 */
>  	int audio_latency[2];
> +
> +	/**
> +	 * @ddc: associated ddc adapter.
> +	 * A connector usually has its associated ddc adapter. If a driver uses
> +	 * this field, then an appropriate symbolic link is created in connector
> +	 * sysfs directory to make it easy for the user to tell which i2c
> +	 * adapter is for a particular display.

The first sentence isn't very clear. The rest is mixing "ddc adapter"
and "i2c adapter". How about the following ?

"When the connector carries DDC signals, this field points to the I2C
adapter connected to the DDC signals, if any. When this field is not
NULL a symbolic link is created in the connector's sysfs directory to
expose the I2C adapter used by the connector."

Should we also mention that the field isn't meant to be set directly,
but shall be set with drm_connector_init_with_ddc() ?

"This field shall not be set directly by drivers, use
drm_connector_init_with_ddc() instead."

I'm also slightly concerned about the lifetime of this pointer, and what
would happen if the I2C adapter disappears while the connector is still
exposed to userspace, but I suppose this isn't a new issue, existing
code likely suffers from this.

With the updated documentation,

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> +	 */
> +	struct i2c_adapter *ddc;
> +
>  	/**
>  	 * @null_edid_counter: track sinks that give us all zeros for the EDID.
>  	 * Needed to workaround some HW bugs where we get all 0s

-- 
Regards,

Laurent Pinchart