[PATCH 0/3] usb: typec: tipd: minor improvements in the firmware update code

[PATCH 0/3] usb: typec: tipd: minor improvements in the firmware update code

[Javier Carrasco]

This series makes some minor modifications to the firmware update
mechanism, mainly (but not only) for the tps6598x variant.

The modifications are the following:

1. Use a single call to read the "firmware-name" property. The current
   implementations (tps25750 and tps6598x) of the firmware update
   mechanism read that property, even though tps_request_firmware()
   already does it.
2. Provide the same information via error logs for the both
   implementations (firmware name and size missing in tps6598x).
3. Minor code cleanup found by Coccinelle (min() instead of if...else).

The series has been tested with a TPS65987DDHRSHR.

Signed-off-by: Javier Carrasco <javier.carrasco@wolfvision.net>
---
Javier Carrasco (3):
      usb: typec: tipd: drop second firmware name read
      usb: typec: tipd: add error log to provide firmware name and size
      usb: typec: tipd: use min() to set tps6598x firmware packet size

 drivers/usb/typec/tipd/core.c | 33 ++++++++++++---------------------
 1 file changed, 12 insertions(+), 21 deletions(-)
---
base-commit: 234cb065ad82915ff8d06ce01e01c3e640b674d2
change-id: 20240605-tps6598x_fw_update_log-1d181faab2ba

Best regards,
-- 
Javier Carrasco <javier.carrasco@wolfvision.net>

[PATCH 1/3] usb: typec: tipd: drop second firmware name read

[Javier Carrasco]

tps_request_firmware() reads the firmware name and there is no need to
repeat the action in the device-specific implementations of the firmware
update mechanism.

Provide the firmware name as a parameter in tps_request_firmware() to
avoid repetitive operations in the device-specific implementations.

Signed-off-by: Javier Carrasco <javier.carrasco@wolfvision.net>
---
 drivers/usb/typec/tipd/core.c | 24 +++++++-----------------
 1 file changed, 7 insertions(+), 17 deletions(-)

diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
index ad76dbd20e65..851b0d02727a 100644
--- a/drivers/usb/typec/tipd/core.c
+++ b/drivers/usb/typec/tipd/core.c
@@ -892,19 +892,19 @@ tps6598x_register_port(struct tps6598x *tps, struct fwnode_handle *fwnode)
 	return 0;
 }
 
-static int tps_request_firmware(struct tps6598x *tps, const struct firmware **fw)
+static int tps_request_firmware(struct tps6598x *tps, const struct firmware **fw,
+				const char **firmware_name)
 {
-	const char *firmware_name;
 	int ret;
 
 	ret = device_property_read_string(tps->dev, "firmware-name",
-					  &firmware_name);
+					  firmware_name);
 	if (ret)
 		return ret;
 
-	ret = request_firmware(fw, firmware_name, tps->dev);
+	ret = request_firmware(fw, *firmware_name, tps->dev);
 	if (ret) {
-		dev_err(tps->dev, "failed to retrieve \"%s\"\n", firmware_name);
+		dev_err(tps->dev, "failed to retrieve \"%s\"\n", *firmware_name);
 		return ret;
 	}
 
@@ -999,12 +999,7 @@ static int tps25750_start_patch_burst_mode(struct tps6598x *tps)
 	u32 addr;
 	struct device_node *np = tps->dev->of_node;
 
-	ret = device_property_read_string(tps->dev, "firmware-name",
-					  &firmware_name);
-	if (ret)
-		return ret;
-
-	ret = tps_request_firmware(tps, &fw);
+	ret = tps_request_firmware(tps, &fw, &firmware_name);
 	if (ret)
 		return ret;
 
@@ -1155,12 +1150,7 @@ static int tps6598x_apply_patch(struct tps6598x *tps)
 	const char *firmware_name;
 	int ret;
 
-	ret = device_property_read_string(tps->dev, "firmware-name",
-					  &firmware_name);
-	if (ret)
-		return ret;
-
-	ret = tps_request_firmware(tps, &fw);
+	ret = tps_request_firmware(tps, &fw, &firmware_name);
 	if (ret)
 		return ret;
 

-- 
2.40.1

Re: [PATCH 1/3] usb: typec: tipd: drop second firmware name read

[Heikki Krogerus]

On Thu, Jun 06, 2024 at 11:03:55AM +0200, Javier Carrasco wrote:
> tps_request_firmware() reads the firmware name and there is no need to
> repeat the action in the device-specific implementations of the firmware
> update mechanism.
> 
> Provide the firmware name as a parameter in tps_request_firmware() to
> avoid repetitive operations in the device-specific implementations.
> 
> Signed-off-by: Javier Carrasco <javier.carrasco@wolfvision.net>

Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

> ---
>  drivers/usb/typec/tipd/core.c | 24 +++++++-----------------
>  1 file changed, 7 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
> index ad76dbd20e65..851b0d02727a 100644
> --- a/drivers/usb/typec/tipd/core.c
> +++ b/drivers/usb/typec/tipd/core.c
> @@ -892,19 +892,19 @@ tps6598x_register_port(struct tps6598x *tps, struct fwnode_handle *fwnode)
>  	return 0;
>  }
>  
> -static int tps_request_firmware(struct tps6598x *tps, const struct firmware **fw)
> +static int tps_request_firmware(struct tps6598x *tps, const struct firmware **fw,
> +				const char **firmware_name)
>  {
> -	const char *firmware_name;
>  	int ret;
>  
>  	ret = device_property_read_string(tps->dev, "firmware-name",
> -					  &firmware_name);
> +					  firmware_name);
>  	if (ret)
>  		return ret;
>  
> -	ret = request_firmware(fw, firmware_name, tps->dev);
> +	ret = request_firmware(fw, *firmware_name, tps->dev);
>  	if (ret) {
> -		dev_err(tps->dev, "failed to retrieve \"%s\"\n", firmware_name);
> +		dev_err(tps->dev, "failed to retrieve \"%s\"\n", *firmware_name);
>  		return ret;
>  	}
>  
> @@ -999,12 +999,7 @@ static int tps25750_start_patch_burst_mode(struct tps6598x *tps)
>  	u32 addr;
>  	struct device_node *np = tps->dev->of_node;
>  
> -	ret = device_property_read_string(tps->dev, "firmware-name",
> -					  &firmware_name);
> -	if (ret)
> -		return ret;
> -
> -	ret = tps_request_firmware(tps, &fw);
> +	ret = tps_request_firmware(tps, &fw, &firmware_name);
>  	if (ret)
>  		return ret;
>  
> @@ -1155,12 +1150,7 @@ static int tps6598x_apply_patch(struct tps6598x *tps)
>  	const char *firmware_name;
>  	int ret;
>  
> -	ret = device_property_read_string(tps->dev, "firmware-name",
> -					  &firmware_name);
> -	if (ret)
> -		return ret;
> -
> -	ret = tps_request_firmware(tps, &fw);
> +	ret = tps_request_firmware(tps, &fw, &firmware_name);
>  	if (ret)
>  		return ret;
>  
> 
> -- 
> 2.40.1

-- 
heikki

[PATCH 2/3] usb: typec: tipd: add error log to provide firmware name and size

[Javier Carrasco]

The current error logs do not show the firmware name and size for the
tps6598x. On the other hand, this information is provided for the
tps25750. Both implementations have access to that information, and the
existing message for the tps25750 can be used for the tps6598x without
extra modifications.

Signed-off-by: Javier Carrasco <javier.carrasco@wolfvision.net>
---
 drivers/usb/typec/tipd/core.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
index 851b0d02727a..58f6eeffd070 100644
--- a/drivers/usb/typec/tipd/core.c
+++ b/drivers/usb/typec/tipd/core.c
@@ -1195,6 +1195,10 @@ static int tps6598x_apply_patch(struct tps6598x *tps)
 
 release_fw:
 	release_firmware(fw);
+	if (ret) {
+		dev_err(tps->dev, "Failed to write patch %s of %zu bytes\n",
+			firmware_name, fw->size);
+	}
 
 	return ret;
 };

-- 
2.40.1

Re: [PATCH 2/3] usb: typec: tipd: add error log to provide firmware name and size

[Heikki Krogerus]

On Thu, Jun 06, 2024 at 11:03:56AM +0200, Javier Carrasco wrote:
> The current error logs do not show the firmware name and size for the
> tps6598x. On the other hand, this information is provided for the
> tps25750. Both implementations have access to that information, and the
> existing message for the tps25750 can be used for the tps6598x without
> extra modifications.
> 
> Signed-off-by: Javier Carrasco <javier.carrasco@wolfvision.net>

Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

> ---
>  drivers/usb/typec/tipd/core.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
> index 851b0d02727a..58f6eeffd070 100644
> --- a/drivers/usb/typec/tipd/core.c
> +++ b/drivers/usb/typec/tipd/core.c
> @@ -1195,6 +1195,10 @@ static int tps6598x_apply_patch(struct tps6598x *tps)
>  
>  release_fw:
>  	release_firmware(fw);
> +	if (ret) {
> +		dev_err(tps->dev, "Failed to write patch %s of %zu bytes\n",
> +			firmware_name, fw->size);
> +	}
>  
>  	return ret;
>  };
> 
> -- 
> 2.40.1

-- 
heikki

[PATCH 3/3] usb: typec: tipd: use min() to set tps6598x firmware packet size

[Javier Carrasco]

Simplify the current firmware packet size check in the tps6598x
implementation by means of a single call to min(), which is what the
current code does in a more verbose, less elegant way.

This patch fixes a cocci warning ("WARNING opportunity for min()").

Suggested-by: Julia Lawall <julia.lawall@inria.fr>
Signed-off-by: Javier Carrasco <javier.carrasco@wolfvision.net>
---
 drivers/usb/typec/tipd/core.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
index 58f6eeffd070..ea768b19a7f1 100644
--- a/drivers/usb/typec/tipd/core.c
+++ b/drivers/usb/typec/tipd/core.c
@@ -1165,10 +1165,7 @@ static int tps6598x_apply_patch(struct tps6598x *tps)
 
 	bytes_left = fw->size;
 	while (bytes_left) {
-		if (bytes_left < TPS_MAX_LEN)
-			in_len = bytes_left;
-		else
-			in_len = TPS_MAX_LEN;
+		in_len = min(bytes_left, TPS_MAX_LEN);
 		ret = tps6598x_exec_cmd(tps, "PTCd", in_len,
 					fw->data + copied_bytes,
 					TPS_PTCD_OUT_BYTES, out);

-- 
2.40.1

Re: [PATCH 3/3] usb: typec: tipd: use min() to set tps6598x firmware packet size

[Heikki Krogerus]

On Thu, Jun 06, 2024 at 11:03:57AM +0200, Javier Carrasco wrote:
> Simplify the current firmware packet size check in the tps6598x
> implementation by means of a single call to min(), which is what the
> current code does in a more verbose, less elegant way.
> 
> This patch fixes a cocci warning ("WARNING opportunity for min()").
> 
> Suggested-by: Julia Lawall <julia.lawall@inria.fr>
> Signed-off-by: Javier Carrasco <javier.carrasco@wolfvision.net>

Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

> ---
>  drivers/usb/typec/tipd/core.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
> index 58f6eeffd070..ea768b19a7f1 100644
> --- a/drivers/usb/typec/tipd/core.c
> +++ b/drivers/usb/typec/tipd/core.c
> @@ -1165,10 +1165,7 @@ static int tps6598x_apply_patch(struct tps6598x *tps)
>  
>  	bytes_left = fw->size;
>  	while (bytes_left) {
> -		if (bytes_left < TPS_MAX_LEN)
> -			in_len = bytes_left;
> -		else
> -			in_len = TPS_MAX_LEN;
> +		in_len = min(bytes_left, TPS_MAX_LEN);
>  		ret = tps6598x_exec_cmd(tps, "PTCd", in_len,
>  					fw->data + copied_bytes,
>  					TPS_PTCD_OUT_BYTES, out);
> 
> -- 
> 2.40.1

-- 
heikki