RE: [PATCH v2 1/3] drm: Add SCDC helpers

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



- dri-devel@xxxxxxxxxxxxxxxx 
+ dri-devel@xxxxxxxxxxxxxxxxxxxxx 

Regards
Shashank
-----Original Message-----
From: Sharma, Shashank [mailto:shashank.sharma@xxxxxxxxx] 
Sent: Monday, December 19, 2016 7:00 PM
To: Thierry Reding <thierry.reding@xxxxxxxxx>; dri-devel@xxxxxxxxxxxxxxxx
Cc: Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx>; Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>; Jose Abreu <joabreu@xxxxxxxxxxxx>
Subject: Re: [PATCH v2 1/3] drm: Add SCDC helpers

Regards

Shashank

On 12/3/2016 12:52 AM, Thierry Reding wrote:
> From: Thierry Reding <treding@xxxxxxxxxx>
>
> SCDC is a mechanism defined in the HDMI 2.0 specification that allows 
> the source and sink devices to communicate.
>
> This commit introduces helpers to access the SCDC and provides the 
> symbolic names for the various registers defined in the specification.
>
> Signed-off-by: Thierry Reding <treding@xxxxxxxxxx>
> ---
> Changes in v2:
> - indent register field definitions for readability (Ville Syrjälä)
> - use GFP_TEMPORARY for temporary buffer allocation (Jani Nikula)
>
>   Documentation/gpu/drm-kms-helpers.rst |  12 ++++
>   drivers/gpu/drm/Kconfig               |   4 ++
>   drivers/gpu/drm/Makefile              |   1 +
>   drivers/gpu/drm/drm_scdc_helper.c     | 111 ++++++++++++++++++++++++++++
>   include/drm/drm_scdc_helper.h         | 132 ++++++++++++++++++++++++++++++++++
>   5 files changed, 260 insertions(+)
>   create mode 100644 drivers/gpu/drm/drm_scdc_helper.c
>   create mode 100644 include/drm/drm_scdc_helper.h
>
> diff --git a/Documentation/gpu/drm-kms-helpers.rst 
> b/Documentation/gpu/drm-kms-helpers.rst
> index 03040aa14fe8..759275629fcf 100644
> --- a/Documentation/gpu/drm-kms-helpers.rst
> +++ b/Documentation/gpu/drm-kms-helpers.rst
> @@ -217,6 +217,18 @@ EDID Helper Functions Reference
>   .. kernel-doc:: drivers/gpu/drm/drm_edid.c
>      :export:
>   
> +SCDC Helper Functions Reference
> +===============================
> +
> +.. kernel-doc:: drivers/gpu/drm/drm_scdc_helper.c
> +   :doc: scdc helpers
> +
> +.. kernel-doc:: include/drm/drm_scdc_helper.h
> +   :internal:
> +
> +.. kernel-doc:: drivers/gpu/drm/drm_scdc_helper.c
> +   :export:
> +
>   Rectangle Utilities Reference
>   =============================
>   
> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig index 
> 95fc0410e129..d0031fe45bab 100644
> --- a/drivers/gpu/drm/Kconfig
> +++ b/drivers/gpu/drm/Kconfig
> @@ -84,6 +84,10 @@ config DRM_FBDEV_EMULATION
>   
>   	  If in doubt, say "Y".
>   
> +config DRM_SCDC
> +	bool
> +	depends on DRM_KMS_HELPER
> +
>   config DRM_LOAD_EDID_FIRMWARE
>   	bool "Allow to specify an EDID data set instead of probing for it"
>   	depends on DRM_KMS_HELPER
> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile index 
> 883f3e75cfbc..71c38b6dd546 100644
> --- a/drivers/gpu/drm/Makefile
> +++ b/drivers/gpu/drm/Makefile
> @@ -31,6 +31,7 @@ drm_kms_helper-y := drm_crtc_helper.o drm_dp_helper.o drm_probe_helper.o \
>   		drm_kms_helper_common.o drm_dp_dual_mode_helper.o \
>   		drm_simple_kms_helper.o drm_modeset_helper.o
>   
> +drm_kms_helper-$(CONFIG_DRM_SCDC) += drm_scdc_helper.o
>   drm_kms_helper-$(CONFIG_DRM_LOAD_EDID_FIRMWARE) += drm_edid_load.o
>   drm_kms_helper-$(CONFIG_DRM_FBDEV_EMULATION) += drm_fb_helper.o
>   drm_kms_helper-$(CONFIG_DRM_KMS_CMA_HELPER) += drm_fb_cma_helper.o 
> diff --git a/drivers/gpu/drm/drm_scdc_helper.c 
> b/drivers/gpu/drm/drm_scdc_helper.c
> new file mode 100644
> index 000000000000..fe0e1211e873
> --- /dev/null
> +++ b/drivers/gpu/drm/drm_scdc_helper.c
> @@ -0,0 +1,111 @@
> +/*
> + * Copyright (c) 2015 NVIDIA Corporation. All rights reserved.
2016 Now in copyright notice ?
> + *
> + * Permission is hereby granted, free of charge, to any person 
> + obtaining a
> + * copy of this software and associated documentation files (the 
> + "Software"),
> + * to deal in the Software without restriction, including without 
> + limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sub 
> + license,
> + * and/or sell copies of the Software, and to permit persons to whom 
> + the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including 
> + the
> + * next paragraph) shall be included in all copies or substantial 
> + portions
> + * of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, 
> + EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF 
> + MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NON-INFRINGEMENT. IN NO EVENT 
> + SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES 
> + OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, 
> + ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR 
> + OTHER
> + * DEALINGS IN THE SOFTWARE.
> + */
> +
> +#include <linux/slab.h>
> +
> +#include <drm/drm_scdc_helper.h>
> +
> +/**
> + * DOC: scdc helpers
> + *
> + * Status and Control Data Channel (SCDC) is a mechanism introduced 
> +by the
> + * HDMI 2.0 specification. It is a point-to-point protocol that 
> +allows the
> + * HDMI source and HDMI sink to exchange data. The same I2C interface 
> +that
> + * is used to access EDID serves as the transport mechanism for SCDC.
> + */
> +
> +#define SCDC_I2C_SLAVE_ADDRESS 0x54
> +
> +/**
> + * drm_scdc_read - read a block of data from SCDC
> + * @adapter: I2C controller
> + * @offset: start offset of block to read
> + * @buffer: return location for the block to read
> + * @size: size of the block to read
> + *
> + * Reads a block of data from SCDC, starting at a given offset.
> + *
> + * Returns:
> + * The number of bytes read from SCDC or a negative error code on failure.
> + */
> +ssize_t drm_scdc_read(struct i2c_adapter *adapter, u8 offset, void *buffer,
> +		      size_t size)
> +{
> +	struct i2c_msg msgs[2] = {
> +		{
> +			.addr = SCDC_I2C_SLAVE_ADDRESS,
> +			.flags = 0,
> +			.len = 1,
> +			.buf = &offset,
> +		}, {
> +			.addr = SCDC_I2C_SLAVE_ADDRESS,
> +			.flags = I2C_M_RD,
> +			.len = size,
> +			.buf = buffer,
> +		}
> +	};
> +
> +	return i2c_transfer(adapter, msgs, ARRAY_SIZE(msgs)); } 
> +EXPORT_SYMBOL(drm_scdc_read);
> +
> +/**
> + * drm_scdc_write - write a block of data to SCDC
> + * @adapter: I2C controller
> + * @offset: start offset of block to write
> + * @buffer: block of data to write
> + * @size: size of the block to write
> + *
> + * Writes a block of data to SCDC, starting at a given offset.
> + *
> + * Returns:
> + * The number of bytes written to SCDC or a negative error code on failure.
> + */
> +ssize_t drm_scdc_write(struct i2c_adapter *adapter, u8 offset,
> +		       const void *buffer, size_t size) {
> +	struct i2c_msg msg = {
> +		.addr = SCDC_I2C_SLAVE_ADDRESS,
> +		.flags = 0,
> +		.len = 1 + size,
> +		.buf = NULL,
> +	};
> +	void *data;
> +	int err;
> +
> +	data = kmalloc(1 + size, GFP_TEMPORARY);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	msg.buf = data;
> +
> +	memcpy(data, &offset, sizeof(offset));
we are already assuming sizeof(offset) = 1, as the next memcpy is at 
data+1, so do we need to have sizeof() ?
> +	memcpy(data + 1, buffer, size);
or may be data + sizeof(offset) instead of data + 1 here.
> +
> +	err = i2c_transfer(adapter, &msg, 1);
> +
> +	kfree(data);
> +
> +	return err;
> +}
> +EXPORT_SYMBOL(drm_scdc_write);
> diff --git a/include/drm/drm_scdc_helper.h 
> b/include/drm/drm_scdc_helper.h new file mode 100644 index 
> 000000000000..93b07bcf0291
> --- /dev/null
> +++ b/include/drm/drm_scdc_helper.h
> @@ -0,0 +1,132 @@
> +/*
> + * Copyright (c) 2015 NVIDIA Corporation. All rights reserved.
> + *
> + * Permission is hereby granted, free of charge, to any person 
> +obtaining a
> + * copy of this software and associated documentation files (the 
> +"Software"),
> + * to deal in the Software without restriction, including without 
> +limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sub 
> +license,
> + * and/or sell copies of the Software, and to permit persons to whom 
> +the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including 
> +the
> + * next paragraph) shall be included in all copies or substantial 
> +portions
> + * of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, 
> +EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF 
> +MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NON-INFRINGEMENT. IN NO EVENT 
> +SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES 
> +OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, 
> +ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR 
> +OTHER
> + * DEALINGS IN THE SOFTWARE.
> + */
> +
> +#ifndef DRM_SCDC_HELPER_H
> +#define DRM_SCDC_HELPER_H
> +
> +#include <linux/i2c.h>
> +#include <linux/types.h>
> +
> +#define SCDC_SINK_VERSION 0x01
> +
> +#define SCDC_SOURCE_VERSION 0x02
> +
> +#define SCDC_UPDATE_0 0x10
> +#define  SCDC_READ_REQUEST_TEST (1 << 2) #define  SCDC_CED_UPDATE (1 
> +<< 1) #define  SCDC_STATUS_UPDATE (1 << 0)
> +
> +#define SCDC_UPDATE_1 0x11
> +
> +#define SCDC_TMDS_CONFIG 0x20
> +#define  SCDC_TMDS_BIT_CLOCK_RATIO_BY_40 (1 << 1) #define  
> +SCDC_TMDS_BIT_CLOCK_RATIO_BY_10 (0 << 1)
would it be more readable to create a mask, and then call for 40 or 10 ?
> +#define  SCDC_SCRAMBLING_ENABLE (1 << 0)
> +
> +#define SCDC_SCRAMBLER_STATUS 0x21
> +#define  SCDC_SCRAMBLING_STATUS (1 << 0)
> +
> +#define SCDC_CONFIG_0 0x30
> +#define  SCDC_READ_REQUEST_ENABLE (1 << 0)
> +
> +#define SCDC_STATUS_FLAGS_0 0x40
> +#define  SCDC_CH2_LOCK (1 < 3)
> +#define  SCDC_CH1_LOCK (1 < 2)
> +#define  SCDC_CH0_LOCK (1 < 1)
> +#define  SCDC_CH_LOCK_MASK (SCDC_CH2_LOCK | SCDC_CH1_LOCK | 
> +SCDC_CH0_LOCK) #define  SCDC_CLOCK_DETECT (1 << 0)
> +
> +#define SCDC_STATUS_FLAGS_1 0x41
> +
> +#define SCDC_ERR_DET_0_L 0x50
> +#define SCDC_ERR_DET_0_H 0x51
should we call it SCDC_ERR_DET_CH0_L and  SCDC_ERR_DET_CH0_H ?
> +#define SCDC_ERR_DET_1_L 0x52
> +#define SCDC_ERR_DET_1_H 0x53
> +#define SCDC_ERR_DET_2_L 0x54
> +#define SCDC_ERR_DET_2_H 0x55
Same here for 1 and 2
> +#define  SCDC_CHANNEL_VALID (1 << 7)
> +
would it make sense to add a macro here for scdc_channel_valid(channel) which does SCDC_ERR_DET_<CH>_H & SCDC_CHANNEL_VALID ?
> +#define SCDC_ERR_DET_CHECKSUM 0x56
> +
> +#define SCDC_TEST_CONFIG_0 0xc0
> +#define  SCDC_TEST_READ_REQUEST (1 << 7) #define  
> +SCDC_TEST_READ_REQUEST_DELAY(x) ((x) & 0x7f)
> +
> +#define SCDC_MANUFACTURER_IEEE_OUI 0xd0 #define 
> +SCDC_MANUFACTURER_IEEE_OUI_SIZE 3
I guess per octet read will be unnecessary ... is it ?
> +
> +#define SCDC_DEVICE_ID 0xd3
> +#define SCDC_DEVICE_ID_SIZE 8
> +
> +#define SCDC_DEVICE_HARDWARE_REVISION 0xdb #define  
> +SCDC_DEVICE_HARDWARE_REVISION_MAJOR(x) (((x) >> 4) & 0xf) #define  
> +SCDC_DEVICE_HARDWARE_REVISION_MINOR(x) (((x) >> 0) & 0xf)
> +
> +#define SCDC_DEVICE_SOFTWARE_MAJOR_REVISION 0xdc #define 
> +SCDC_DEVICE_SOFTWARE_MINOR_REVISION 0xdd
> +
> +#define SCDC_MANUFACTURER_SPECIFIC 0xde #define 
> +SCDC_MANUFACTURER_SPECIFIC_SIZE 34
> +
> +ssize_t drm_scdc_read(struct i2c_adapter *adapter, u8 offset, void *buffer,
> +		      size_t size);
> +ssize_t drm_scdc_write(struct i2c_adapter *adapter, u8 offset,
> +		       const void *buffer, size_t size);
> +
> +/**
> + * drm_scdc_readb - read a single byte from SCDC
> + * @adapter: I2C adapter
> + * @offset: offset of register to read
> + * @value: return location for the register value
> + *
> + * Reads a single byte from SCDC. This is a convenience wrapper 
> +around the
> + * drm_scdc_read() function.
> + *
> + * Returns:
> + * 0 on success or a negative error code on failure.
> + */
> +static inline int drm_scdc_readb(struct i2c_adapter *adapter, u8 offset,
> +				 u8 *value)
> +{
> +	return drm_scdc_read(adapter, offset, value, sizeof(*value));
We know we are reading one byte in this wrapper, do we still need sizeof ?
> +}
> +
> +/**
> + * drm_scdc_writeb - write a single byte to SCDC
> + * @adapter: I2C adapter
> + * @offset: offset of register to read
> + * @value: return location for the register value
> + *
> + * Writes a single byte to SCDC. This is a convenience wrapper around 
> +the
> + * drm_scdc_write() function.
> + *
> + * Returns:
> + * 0 on success or a negative error code on failure.
> + */
> +static inline int drm_scdc_writeb(struct i2c_adapter *adapter, u8 offset,
> +				  u8 value)
> +{
> +	return drm_scdc_write(adapter, offset, &value, sizeof(value));
Same as above
> +}
> +
> +#endif

_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux