[igt-dev] [PATCH i-g-t 3/6] tools/dpcd_reg: Unify file handling.

[Dhinakaran Pandiyan]

On Fri, 2018-08-31 at 21:18 -0700, Rodrigo Vivi wrote:
> Besides removing few code duplication this will
> allow us to read multiple regs without having
> to keep closing and opening the file for every
> read.
> 
> Cc: Tarun Vyas <tarun.vyas at intel.com>
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan at intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi at intel.com>
> ---
>  tools/dpcd_reg.c | 98 +++++++++++++++++++++++++++-------------------
> --
>  1 file changed, 55 insertions(+), 43 deletions(-)
> 
> diff --git a/tools/dpcd_reg.c b/tools/dpcd_reg.c
> index d139007c..09da4109 100644
> --- a/tools/dpcd_reg.c
> +++ b/tools/dpcd_reg.c
> @@ -53,61 +53,56 @@ static void print_usage(char *tool, int help)
>  	exit((help == 1)? EXIT_SUCCESS : EXIT_FAILURE);
>  }
>  
> -static int dpcd_read(char *device, const uint32_t offset, size_t
> count)
> +static int dpcd_read(int fd, const uint32_t offset, size_t count)
>  {
> -	int fd, ret, i;
> +	int ret, i;
>  	void *buf = malloc(sizeof(uint8_t) * count);
>  
> +	ret = lseek(fd, offset, SEEK_SET);
Does this offer any benefit over pread()/pwrite()?

> +	if (ret < 0) {
> +		igt_warn("lseek to %04x failed!\n", offset);
> +		return ret;
> +	}
> +
>  	if (NULL != buf)
>  		memset(buf, 0, sizeof(uint8_t) * count);
>  	else {
>  		igt_warn("Can't allocate read buffer\n");
> -		return EXIT_FAILURE;
> +		return -1;
>  	}
>  
> -	fd = open(device, O_RDONLY);
> -	if (fd > 0) {
> -		ret = pread(fd, buf, count, offset);
> -		close(fd);
> -		if (ret != count) {
> -			igt_warn("Failed to read from %s aux device
> - error %s\n", device, strerror(errno));
> -			ret = EXIT_FAILURE;
> -			goto out;
> -		}
> -
> -		igt_info("%04x:", offset);
> -		for (i = 0; i < count; i++)
> -			igt_info(" %02x", *(((uint8_t *)(buf)) +
> i));
> -		igt_info("\n");
> -	}
> -	else {
> -		igt_warn("Failed to open %s aux device - error:
> %s\n", device, strerror(errno));
> -		ret = EXIT_FAILURE;
> +	ret = read(fd, buf, count);
> +	if (ret != count) {
> +		igt_warn("Failed to read - error %s\n",
> strerror(errno));
> +		goto out;
>  	}
> +
> +	igt_info("%04x:", offset);
> +	for (i = 0; i < count; i++)
> +		igt_info(" %02x", *(((uint8_t *)(buf)) + i));
> +	igt_info("\n");
> +
>  out:
>  	free(buf);
>  	return ret;
>  }
>  
> -static int dpcd_write(char *device, const uint32_t offset, const
> uint8_t *val)
> +static int dpcd_write(int fd, const uint32_t offset, const uint8_t
> *val)
>  {
> -	int fd, ret;
> -
> -	fd = open(device, O_RDWR);
> -	if (fd > 0) {
> -		ret = pwrite(fd, (const void *)val, sizeof(uint8_t),
> offset);
> -		close(fd);
> -		if (ret < 0) {
> -			igt_warn("Failed to write to %s aux device -
> error %s\n", device, strerror(errno));
> -			ret = EXIT_FAILURE;
> -			goto out;
> -		}
> -		ret = dpcd_read(device, offset, sizeof(uint8_t));
> +	int ret;
> +
> +	ret = lseek(fd, offset, SEEK_SET);
> +	if (ret < 0) {
> +		igt_warn("lseek to %04x failed!\n", offset);
> +		return ret;
>  	}
> -	else {
> -		igt_warn("Failed to open %s aux device - error:
> %s\n", device, strerror(errno));
> -		ret = EXIT_FAILURE;
> +
> +	ret = write(fd, (const void *)val, sizeof(uint8_t));
> +	if (ret < 0) {
> +		igt_warn("Failed to write - error %s\n",
> strerror(errno));
> +		goto out;
>  	}
> +	ret = dpcd_read(fd, offset, sizeof(uint8_t));
>  out:
>  	return ret;
>  }
> @@ -115,10 +110,11 @@ out:
>  int main(int argc, char **argv)
>  {
>  	char dev_name[20];
> -	int ret, devid, help_flg;
> +	int fd, ret, devid, help_flg;
>  	uint32_t offset;
>  	uint8_t val;
>  	size_t count;
> +	int file_op = O_RDONLY;
>  
>  	enum command {
>  		INV = -1,
> @@ -157,11 +153,12 @@ int main(int argc, char **argv)
>  			break;
>  		/* Command parsing */
>  		case 1:
> -			if (strcmp(optarg, "read") == 0)
> +			if (strcmp(optarg, "read") == 0) {
>  				cmd = READ;
> -			else if (strcmp(optarg, "write") == 0)
> +			} else if (strcmp(optarg, "write") == 0) {
>  				cmd = WRITE;
> -			break;
> +				file_op = O_RDWR;
> +			} break;
>  		case ':':
>  			igt_warn("The -%c option of %s requires an
> argument\n",
>  				 optopt, argv[0]);
> @@ -185,20 +182,33 @@ int main(int argc, char **argv)
>  	memset(dev_name, '\0', 20);
>  	snprintf(dev_name, sizeof(aux_dev) + 2, "%s%d", aux_dev,
> devid);
>  
> +	fd = open(dev_name, file_op);
> +	if (fd < 0) {
> +		igt_warn("Failed to open %s aux device - error:
> %s\n", dev_name,
> +			 strerror(errno));
> +		return EXIT_FAILURE;
> +	}
> +
>  	switch (cmd) {
>  	case READ:
>  		if (count == INVALID) {
>  			igt_warn("Please specify the count in
> bytes\n");
>  			print_usage(argv[0], 0);
>  		}
> -		ret = dpcd_read(dev_name, offset, count);
> +		ret = dpcd_read(fd, offset, count);
> +		if (ret != count)
> +			igt_warn("Failed to read from %s aux
> device\n",
> +				 dev_name);
>  		break;
>  	case WRITE:
>  		if (val == INVALID) {
>  			igt_warn("Write value is missing\n");
>  			print_usage(argv[0], 0);
>  		}
> -		ret = dpcd_write(dev_name, offset, &val);
> +		ret = dpcd_write(fd, offset, &val);
> +		if (ret < 0)
> +			igt_warn("Failed to write to %s aux
> device\n",
> +				 dev_name);
>  		break;
>  	case INV:
>  	default:
> @@ -206,5 +216,7 @@ int main(int argc, char **argv)
>  		print_usage(argv[0], 0);
>  	}
>  
> +	close(fd);
> +
>  	return ret;
>  }