[igt-dev] [PATCH 1/2] lHEib/igt_dpcd: move dpcd_read and dpcd_write to lib
Joshi, Kunal1
kunal1.joshi at intel.com
Thu Oct 26 06:32:01 UTC 2023
Hello Kamil,
On 10/17/2023 2:25 PM, Kamil Konieczny wrote:
> Hi Kunal,
>
> On 2023-10-17 at 11:22:51 +0530, Kunal Joshi wrote:
>> moved dpcd_read and dpcd_write to lib/igt_dpcd
> - ^
> s/moved/Moved/
>
> Start with uppercase, also imho this should be "Copy"
> not a move, see below.
>
>> v2: added copyright (swati)
> -----------------------^
> Uppercase: s/swati/Swati/
>
>> added comment for functions (swati)
> ---------------------------------- ^
> Same here.
>
>> sorted headers (swati)
> --------------------- ^
> Same here.
>
>
>> added new function dpcd_read_buf (kunal)
> --------------------------------------- ^
> Same here, s/kunal/Kunal/
>
>> Cc: Swati Sharma <swati2.sharma at intel.com>
>> Cc: Bhanuprakash Modem <bhanuprakash.modem at intel.com>
>> Signed-off-by: Kunal Joshi <kunal1.joshi at intel.com>
>> ---
>> lib/igt_dpcd.c | 104 +++++++++++++++++++++++++++++++++++++++++++++++
>> lib/igt_dpcd.h | 24 +++++++++++
>> tools/dpcd_reg.c | 60 +--------------------------
> It would help if you add a useage of these new functions in
> actual test.
>
>> 3 files changed, 129 insertions(+), 59 deletions(-)
>> create mode 100644 lib/igt_dpcd.c
>> create mode 100644 lib/igt_dpcd.h
>>
>> diff --git a/lib/igt_dpcd.c b/lib/igt_dpcd.c
>> new file mode 100644
>> index 000000000..edddad8c7
>> --- /dev/null
>> +++ b/lib/igt_dpcd.c
>> @@ -0,0 +1,104 @@
> Add at begin of file licence:
>
> // SPDX-License-Identifier: MIT
>
>> +/*
>> + * Copyright © 2023 Intel Corporation
>> +*/
>> +
>> +#include "igt_dpcd.h"
>> +
>> +/**
>> + * dpcd_read:
>> + * @fd: drm_fd
>> + * @offset: dpcd address to read
>> + * @count: bytes to read from @offset
> If regs are byte/word/dword you may consider returning it's value
> with pointer.
>
>> + *
>> + * Returns: EXIT_SUCCESS on success else errno
> ------------------------------------------- ^
> imho better -errno
>
>> + */
>> +int dpcd_read(int fd, uint32_t offset, size_t count)
>> +{
>> + int ret = EXIT_SUCCESS;
>> + uint8_t *buf = calloc(count, sizeof(uint8_t));
> --------------- ^
> This should be parameter of function.
>
>> + ssize_t bytes_read;
>> +
>> + if (!buf) {
>> + fprintf(stderr, "Can't allocate read buffer\n");
> ---------- ^
> In igt lib you should use igt_debug.
>
>> + return ENOMEM;
> Here better to igt_assert.
>
>> + }
>> +
>> + bytes_read = pread(fd, buf, count, offset);
>> +
>> + if (bytes_read < 0) {
>> + fprintf(stderr, "Failed to read - %s\n", strerror(errno));
> ---------- ^
> Same here, use igt_debug.
>
>> + ret = errno;
>> + } else {
>> + printf("0x%04x: ", offset);
> ---------- ^
> Same here, use igt_debug.
>
>> + for (ssize_t i = 0; i < bytes_read; i++) {
>> + printf(" %02x", buf[i]);
> -------------- ^
> Same here, use igt_debug.
>
>> + }
>> + printf("\n");
> -------------- ^
> Same here, use igt_debug.
>
>> + }
>> +
>> + free(buf);
>> + return ret;
>> +}
>> +
>> +/* dpcd_read_buf:
>> + * @fd: drm_fd
>> + * @offset: dpcd address to read
>> + * @count: bytes to read from @offset
>> + * @buf: buf to write read data
>> + *
>> + * Returns: EXIT_SUCCESS on success else errno
>> + */
>> +int dpcd_read_buf(int fd, uint32_t offset, size_t count, uint8_t *buf)
> The same goes here - rewrite it as above.
>
>> +{
>> + int ret = EXIT_SUCCESS;
>> + *buf = calloc(count, sizeof(uint8_t));
> ------- ^^^
> This is your param?
>
>> + ssize_t bytes_read;
>> +
>> + if (!buf) {
>> + fprintf(stderr, "Can't allocate read buffer\n");
>> + return ENOMEM;
>> + }
>> +
>> + bytes_read = pread(fd, buf, count, offset);
>> +
>> + if (bytes_read < 0) {
>> + fprintf(stderr, "Failed to read - %s\n", strerror(errno));
>> + ret = errno;
>> + } else {
>> + printf("0x%04x: ", offset);
>> + for (ssize_t i = 0; i < bytes_read; i++) {
>> + printf(" %02x", buf[i]);
>> + }
>> + printf("\n");
>> + }
>> +
>> + free(buf);
>> + return ret;
>> +}
>> +
>> +
>> +/**
>> + * dpcd_write:
>> + * @fd: drm_fd
>> + * @offset: dpcd address to write
>> + * @cal : value to write
> -------^
> s/cal/val/
>
>> + *
>> + * Returns: EXIT_SUCCESS on success else errno
> ------------------------------------------- ^^
>
>> + */
>> +int dpcd_write(int fd, uint32_t offset, uint8_t val)
> Same here - no printf/fprintf in lib.
>
>> +{
>> + int ret = EXIT_SUCCESS;
>> + ssize_t bytes_written;
>> +
>> + bytes_written = pwrite(fd, &val, sizeof(uint8_t), offset);
>> +
>> + if (bytes_written < 0) {
>> + fprintf(stderr, "Failed to write - %s\n", strerror(errno));
>> + ret = errno;
>> + } else if (bytes_written == 0) {
>> + fprintf(stderr, "Zero bytes were written\n");
>> + ret = EXIT_FAILURE;
>> + }
>> +
>> + return ret;
>> +}
>> diff --git a/lib/igt_dpcd.h b/lib/igt_dpcd.h
>> new file mode 100644
>> index 000000000..a5abe900f
>> --- /dev/null
>> +++ b/lib/igt_dpcd.h
>> @@ -0,0 +1,24 @@
>> +/*
>> + * Copyright © 2023 Intel Corporation
>> +*/
>> +
>> +#ifndef IGT_DPCD_H
>> +#define IGT_DPCD_H
>> +
>> +#include <errno.h>
>> +#include <fcntl.h>
>> +#include <getopt.h>
> -------------^^^^^^
> No need for this.
>
>> +#include <limits.h>
>> +#include <stdbool.h>
>> +#include <stdint.h>
>> +#include <stdio.h>
>> +#include <stdlib.h>
>> +#include <string.h>
>> +#include <unistd.h>
>> +
>> +int dpcd_read(int fd, uint32_t offset, size_t count);
>> +int dpcd_read_buf(int fd, uint32_t offset, size_t count,
>> + uint8_t *buf);
>> +int dpcd_write(int fd, uint32_t offset, uint8_t val);
>> +
>> +#endif /* IGT_DPCD_H */
>> diff --git a/tools/dpcd_reg.c b/tools/dpcd_reg.c
>> index 2761168d0..b7454cca1 100644
>> --- a/tools/dpcd_reg.c
>> +++ b/tools/dpcd_reg.c
> imho we should keep this tool without change as it may help in
> debugging.
>
> Regards,
> Kamil
Agree, will keep tool intact just rewrite function in a useful way for
tests.
Does that sound ok?
>
>> @@ -25,16 +25,7 @@
>> * and write, so CONFIG_DRM_DP_AUX_DEV needs to be set.
>> */
>>
>> -#include <stdio.h>
>> -#include <errno.h>
>> -#include <string.h>
>> -#include <stdlib.h>
>> -#include <fcntl.h>
>> -#include <getopt.h>
>> -#include <stdint.h>
>> -#include <unistd.h>
>> -#include <limits.h>
>> -#include <stdbool.h>
>> +#include "igt_dpcd.h"
>>
>> #define MAX_DP_OFFSET 0xfffff
>> #define DRM_AUX_MINORS 256
>> @@ -214,55 +205,6 @@ static int parse_opts(struct dpcd_data *dpcd, int argc, char **argv)
>> return EXIT_SUCCESS;
>> }
>>
>> -static int dpcd_read(int fd, uint32_t offset, size_t count)
>> -{
>> - int ret = EXIT_SUCCESS, pret, i;
>> - uint8_t *buf = calloc(count, sizeof(uint8_t));
>> -
>> - if (!buf) {
>> - fprintf(stderr, "Can't allocate read buffer\n");
>> - return ENOMEM;
>> - }
>> -
>> - pret = pread(fd, buf, count, offset);
>> - if (pret < 0) {
>> - fprintf(stderr, "Failed to read - %s\n", strerror(errno));
>> - ret = errno;
>> - goto out;
>> - }
>> -
>> - if (pret < count) {
>> - fprintf(stderr,
>> - "Read %u byte(s), expected %zu bytes, starting at offset %x\n\n", pret, count, offset);
>> - ret = EXIT_FAILURE;
>> - }
>> -
>> - printf("0x%04x: ", offset);
>> - for (i = 0; i < pret; i++)
>> - printf(" %02x", *(buf + i));
>> - printf("\n");
>> -
>> -out:
>> - free(buf);
>> - return ret;
>> -}
>> -
>> -static int dpcd_write(int fd, uint32_t offset, uint8_t val)
>> -{
>> - int ret = EXIT_SUCCESS, pret;
>> -
>> - pret = pwrite(fd, (const void *)&val, sizeof(uint8_t), offset);
>> - if (pret < 0) {
>> - fprintf(stderr, "Failed to write - %s\n", strerror(errno));
>> - ret = errno;
>> - } else if (pret == 0) {
>> - fprintf(stderr, "Zero bytes were written\n");
>> - ret = EXIT_FAILURE;
>> - }
>> -
>> - return ret;
>> -}
>> -
>> static int dpcd_dump(int fd)
>> {
>> size_t count;
>> --
>> 2.25.1
>>
More information about the igt-dev
mailing list