Re: [PATCH v2 1/6] drm: Add SCDC helpers

[Jose Abreu <Jose.Abreu@synopsys.com>]

Hi Shashank,



On 07-02-2017 16:09, Sharma, Shashank wrote:
> Thanks for the review Jose, my comments inline.
>
>
> Regards
>
> Shashank
>
>
> On 2/7/2017 4:24 PM, Jose Abreu wrote:
>> Hi Shashank,
>>
>>
>> Sorry for the late review.
>>
>>
>> On 06-02-2017 13:59, Shashank Sharma wrote:
>>> From: Thierry Reding <treding@nvidia.com>
>>>
>>> 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@nvidia.com>
>>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
>>> ---
>>>   Documentation/gpu/drm-kms-helpers.rst |  12 ++++
>>>   drivers/gpu/drm/Makefile              |   3 +-
>>>   drivers/gpu/drm/drm_scdc_helper.c     | 111
>>> ++++++++++++++++++++++++++++
>>>   include/drm/drm_scdc_helper.h         | 132
>>> ++++++++++++++++++++++++++++++++++
>>>   4 files changed, 257 insertions(+), 1 deletion(-)
>>>   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 03040aa..7592756 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/Makefile
>>> b/drivers/gpu/drm/Makefile
>>> index 92de399..ad91cd9 100644
>>> --- a/drivers/gpu/drm/Makefile
>>> +++ b/drivers/gpu/drm/Makefile
>>> @@ -31,7 +31,8 @@ drm-$(CONFIG_DEBUG_FS) += drm_debugfs.o
>>> drm_debugfs_crc.o
>>>   drm_kms_helper-y := drm_crtc_helper.o drm_dp_helper.o
>>> drm_probe_helper.o \
>>>           drm_plane_helper.o drm_dp_mst_topology.o
>>> drm_atomic_helper.o \
>>>           drm_kms_helper_common.o drm_dp_dual_mode_helper.o \
>>> -        drm_simple_kms_helper.o drm_modeset_helper.o
>>> +        drm_simple_kms_helper.o drm_modeset_helper.o \
>>> +        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
>>> diff --git a/drivers/gpu/drm/drm_scdc_helper.c
>>> b/drivers/gpu/drm/drm_scdc_helper.c
>>> new file mode 100644
>>> index 0000000..fe0e121
>>> --- /dev/null
>>> +++ b/drivers/gpu/drm/drm_scdc_helper.c
>>> @@ -0,0 +1,111 @@
>>> +/*
>>> + * 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.
>>> + */
>>> +
>>> +#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,
>> .len = sizeof(offset) ?
> Technically correct, but wouldn't that mean that we are
> expecting to have I2C offsets with length more than one byte ?

I just commented this because it would be more consistent but
indeed you are correct. You can disregard my comment :)

Best regards,
Jose Miguel Abreu

>>
>>> +            .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,
>> .len = sizeof(offset) + size ?
> Same as above.
>>
>>> +        .buf = NULL,
>>> +    };
>>> +    void *data;
>>> +    int err;
>>> +
>>> +    data = kmalloc(1 + size, GFP_TEMPORARY);
>> Same as above.
> So on ...
>>> +    if (!data)
>>> +        return -ENOMEM;
>>> +
>>> +    msg.buf = data;
>>> +
>>> +    memcpy(data, &offset, sizeof(offset));
>>> +    memcpy(data + 1, buffer, size);
>> Same as above.
>>
> So on ..
>> Best regards,
>> Jose Miguel Abreu
> - Shashank
>>> +
>>> +    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 0000000..93b07bc
>>> --- /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)
>>> +#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
>>> +#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
>>> +#define  SCDC_CHANNEL_VALID (1 << 7)
>>> +
>>> +#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
>>> +
>>> +#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));
>>> +}
>>> +
>>> +/**
>>> + * 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));
>>> +}
>>> +
>>> +#endif
>

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx