- 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
