[PATCH 0/4] Simplify ISHTP client interface

[PATCH 0/4] Simplify ISHTP client interface

[Even Xu]

There are three ISHTP clients, they have same ISHTP interface to
connect, reset and disconnect. Also there are multiple steps for
connect, reset and disconnect.

Simplify multiple steps by creating two helper functions. This will
simplify code flow and also avoid code duplication. Also this removes
memory allocation calls during reset, by using these helper functions.

No functional changes are expected with this series.

Even Xu (4):
  HID: Intel-ish-hid: Ishtp: Add helper functions for client connection
  HID: intel-ish-hid: ishtp-hid-client: use helper functions for
    connection
  HID: intel-ish-hid: ishtp-fw-loader: use helper functions for
    connection
  platform: chrome: cros_ec_ishtp: use helper functions for connection

 drivers/hid/intel-ish-hid/ishtp-fw-loader.c  |  60 ++-------
 drivers/hid/intel-ish-hid/ishtp-hid-client.c |  63 ++-------
 drivers/hid/intel-ish-hid/ishtp/client.c     | 185 +++++++++++++++++++++++++--
 drivers/platform/chrome/cros_ec_ishtp.c      |  74 +++--------
 include/linux/intel-ish-client-if.h          |   3 +
 5 files changed, 217 insertions(+), 168 deletions(-)

-- 
2.7.4

[PATCH 1/4] HID: Intel-ish-hid: Ishtp: Add helper functions for client connection

[Even Xu]

For every ishtp client driver during initialization state, the flow is:
1 - Allocate an ISHTP client instance
2 - Reserve a host id and link the client instance
3 - Search a firmware client using UUID and get related
    client information
4 - Bind firmware client id to the ISHTP client instance
5 - Set the state the ISHTP client instance to CONNECTING
6 - Send connect request to firmware
7 - Register event callback for messages from the firmware

During deinitizalization state, the flow is:
9 - Set the state the ISHTP client instance to ISHTP_CL_DISCONNECTING
10 - Issue disconnect request to firmware
11 - Unlike the client instance
12 - Flush message queue
13 - Free ISHTP client instance

Step 2-7 are identical to the steps of client driver initialization
and driver reset flow, but reallocation of the RX/TX ring buffers
can be avoided in reset flow.

Also for step 9-12, they are identical to the steps of client driver
failure handling after connect request, driver reset flow and
driver removing.

So, add two helper functions to simplify client driver code.
ishtp_cl_establish_connection()
ishtp_cl_destroy_connection()

No functional changes are expected.

Signed-off-by: Even Xu <even.xu@intel.com>
Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
 drivers/hid/intel-ish-hid/ishtp/client.c | 185 +++++++++++++++++++++++++++++--
 include/linux/intel-ish-client-if.h      |   3 +
 2 files changed, 177 insertions(+), 11 deletions(-)

diff --git a/drivers/hid/intel-ish-hid/ishtp/client.c b/drivers/hid/intel-ish-hid/ishtp/client.c
index 2d92fc1..82c907f 100644
--- a/drivers/hid/intel-ish-hid/ishtp/client.c
+++ b/drivers/hid/intel-ish-hid/ishtp/client.c
@@ -339,16 +339,17 @@ static bool ishtp_cl_is_other_connecting(struct ishtp_cl *cl)
 }
 
 /**
- * ishtp_cl_connect() - Send connect request to firmware
+ * ishtp_cl_connect_to_fw() - Send connect request to firmware
  * @cl: client device instance
  *
- * Send a connect request for a client to firmware. If successful it will
- * RX and TX ring buffers
+ * Send a connect request to the firmware and wait for firmware response.
+ * If there is successful connection response from the firmware, change
+ * client state to ISHTP_CL_CONNECTED, and bind client to related
+ * firmware client_id.
  *
- * Return: 0 if successful connect response from the firmware and able
- * to bind and allocate ring buffers or error code on failure
+ * Return: 0 for success and error code on failure
  */
-int ishtp_cl_connect(struct ishtp_cl *cl)
+static int ishtp_cl_connect_to_fw(struct ishtp_cl *cl)
 {
 	struct ishtp_device *dev;
 	int rets;
@@ -358,8 +359,6 @@ int ishtp_cl_connect(struct ishtp_cl *cl)
 
 	dev = cl->dev;
 
-	dev->print_log(dev, "%s() current_state = %d\n", __func__, cl->state);
-
 	if (ishtp_cl_is_other_connecting(cl)) {
 		dev->print_log(dev, "%s() Busy\n", __func__);
 		return	-EBUSY;
@@ -405,6 +404,38 @@ int ishtp_cl_connect(struct ishtp_cl *cl)
 		return rets;
 	}
 
+	return rets;
+}
+
+/**
+ * ishtp_cl_connect() - Build connection with firmware
+ * @cl: client device instance
+ *
+ * Call ishtp_cl_connect_to_fw() to connect and bind to firmware. If successful,
+ * allocate RX and TX ring buffers, and start flow control with firmware to
+ * start communication.
+ *
+ * Return: 0 if there is successful connection to the firmware, allocate
+ * ring buffers.
+ */
+int ishtp_cl_connect(struct ishtp_cl *cl)
+{
+	struct ishtp_device *dev;
+	int rets;
+
+	if (!cl || !cl->dev)
+		return -ENODEV;
+
+	dev = cl->dev;
+
+	dev->print_log(dev, "%s() current_state = %d\n", __func__, cl->state);
+
+	rets = ishtp_cl_connect_to_fw(cl);
+	if (rets) {
+		dev->print_log(dev, "%s() Connect to fw failed\n", __func__);
+		return rets;
+	}
+
 	rets = ishtp_cl_alloc_rx_ring(cl);
 	if (rets) {
 		dev->print_log(dev, "%s() Alloc RX ring failed\n", __func__);
@@ -422,16 +453,148 @@ int ishtp_cl_connect(struct ishtp_cl *cl)
 		return rets;
 	}
 
-	/* Upon successful connection and allocation, emit flow-control */
+	/*
+	 * Upon successful connection and allocation, start flow-control.
+	 */
 	rets = ishtp_cl_read_start(cl);
 
-	dev->print_log(dev, "%s() successful\n", __func__);
-
 	return rets;
 }
 EXPORT_SYMBOL(ishtp_cl_connect);
 
 /**
+ * ishtp_cl_establish_connection() - Establish connection with the firmware
+ * @cl: client device instance
+ * @uuid: uuid of the client to search
+ * @tx_size: TX ring buffer size
+ * @rx_size: RX ring buffer size
+ * @reset: true if called for reset connection, otherwise for first connection
+ *
+ * This is a helper function for client driver to build connection with firmware.
+ * If it's first time connecting to the firmware, set reset to false, this
+ * function will link client to bus, find client id and send connect request to
+ * the firmware.
+ *
+ * If it's called for reset handler where client lost connection after
+ * firmware reset, set reset to true, this function will reinit client state and
+ * establish connection again. In this case, this function reuses current client
+ * structure and ring buffers to avoid allocation failure and memory fragments.
+ *
+ * Return: 0 for successful connection with the firmware,
+ * or error code on failure
+ */
+int ishtp_cl_establish_connection(struct ishtp_cl *cl, const guid_t *uuid,
+				  int tx_size, int rx_size, bool reset)
+{
+	struct ishtp_device *dev;
+	struct ishtp_fw_client *fw_client;
+	int rets;
+
+	if (!cl || !cl->dev)
+		return -ENODEV;
+
+	dev = cl->dev;
+
+	ishtp_set_connection_state(cl, ISHTP_CL_INITIALIZING);
+
+	/* reinit ishtp_cl structure if call for reset */
+	if (reset) {
+		cl->host_client_id = 0;
+		cl->fw_client_id = 0;
+		cl->ishtp_flow_ctrl_creds = 0;
+		cl->out_flow_ctrl_creds = 0;
+
+		cl->last_tx_path = CL_TX_PATH_IPC;
+		cl->last_dma_acked = 1;
+		cl->last_dma_addr = NULL;
+		cl->last_ipc_acked = 1;
+
+		cl->sending = 0;
+		cl->err_send_msg = 0;
+		cl->err_send_fc = 0;
+
+		cl->send_msg_cnt_ipc = 0;
+		cl->send_msg_cnt_dma = 0;
+		cl->recv_msg_cnt_ipc = 0;
+		cl->recv_msg_cnt_dma = 0;
+		cl->recv_msg_num_frags = 0;
+		cl->ishtp_flow_ctrl_cnt = 0;
+		cl->out_flow_ctrl_cnt = 0;
+	}
+
+	/* link to bus */
+	rets = ishtp_cl_link(cl);
+	if (rets) {
+		dev->print_log(dev, "%s() ishtp_cl_link failed\n", __func__);
+		return rets;
+	}
+
+	/* find firmware client */
+	fw_client = ishtp_fw_cl_get_client(dev, uuid);
+	if (!fw_client) {
+		dev->print_log(dev,
+			       "%s() ish client uuid not found\n", __func__);
+		return -ENOENT;
+	}
+
+	ishtp_set_tx_ring_size(cl, tx_size);
+	ishtp_set_rx_ring_size(cl, rx_size);
+
+	ishtp_cl_set_fw_client_id(cl, ishtp_get_fw_client_id(fw_client));
+
+	ishtp_set_connection_state(cl, ISHTP_CL_CONNECTING);
+
+	/*
+	 * For reset case, not allocate tx/rx ring buffer which are already
+	 * done in ishtp_cl_connect() during first connection.
+	 */
+	if (reset) {
+		rets = ishtp_cl_connect_to_fw(cl);
+		if (!rets)
+			rets = ishtp_cl_read_start(cl);
+		else
+			dev->print_log(dev,
+				"%s() connect to fw failed\n", __func__);
+	} else {
+		rets = ishtp_cl_connect(cl);
+	}
+
+	return rets;
+}
+EXPORT_SYMBOL(ishtp_cl_establish_connection);
+
+/**
+ * ishtp_cl_destroy_connection() - Disconnect with the firmware
+ * @cl: client device instance
+ * @reset: true if called for firmware reset, false for normal disconnection
+ *
+ * This is a helper function for client driver to disconnect with firmware,
+ * unlink to bus and flush message queue.
+ */
+void ishtp_cl_destroy_connection(struct ishtp_cl *cl, bool reset)
+{
+	if (!cl)
+		return;
+
+	if (reset) {
+		/*
+		 * For reset case, connection is already lost during fw reset.
+		 * Just set state to DISCONNECTED is enough.
+		 */
+		ishtp_set_connection_state(cl, ISHTP_CL_DISCONNECTED);
+	} else {
+		if (cl->state != ISHTP_CL_DISCONNECTED) {
+			ishtp_set_connection_state(cl, ISHTP_CL_DISCONNECTING);
+			ishtp_cl_disconnect(cl);
+		}
+	}
+
+	ishtp_cl_unlink(cl);
+	ishtp_cl_flush_queues(cl);
+}
+EXPORT_SYMBOL(ishtp_cl_destroy_connection);
+
+/**
  * ishtp_cl_read_start() - Prepare to read client message
  * @cl: client device instance
  *
diff --git a/include/linux/intel-ish-client-if.h b/include/linux/intel-ish-client-if.h
index f45f133..7716226 100644
--- a/include/linux/intel-ish-client-if.h
+++ b/include/linux/intel-ish-client-if.h
@@ -94,6 +94,9 @@ int ishtp_cl_link(struct ishtp_cl *cl);
 void ishtp_cl_unlink(struct ishtp_cl *cl);
 int ishtp_cl_disconnect(struct ishtp_cl *cl);
 int ishtp_cl_connect(struct ishtp_cl *cl);
+int ishtp_cl_establish_connection(struct ishtp_cl *cl, const guid_t *uuid,
+				  int tx_size, int rx_size, bool reset);
+void ishtp_cl_destroy_connection(struct ishtp_cl *cl, bool reset);
 int ishtp_cl_send(struct ishtp_cl *cl, uint8_t *buf, size_t length);
 int ishtp_cl_flush_queues(struct ishtp_cl *cl);
 int ishtp_cl_io_rb_recycle(struct ishtp_cl_rb *rb);
-- 
2.7.4

[PATCH 2/4] HID: intel-ish-hid: ishtp-hid-client: use helper functions for connection

[Even Xu]

Use helper functions ishtp_cl_establish_connection() and
ishtp_cl_destroy_connection() to establish and destroy connection
respectively. These functions are used during initialization, reset and
deinitialization flows.

No functional changes are expected.

Signed-off-by: Even Xu <even.xu@intel.com>
Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
 drivers/hid/intel-ish-hid/ishtp-hid-client.c | 63 ++++++----------------------
 1 file changed, 13 insertions(+), 50 deletions(-)

diff --git a/drivers/hid/intel-ish-hid/ishtp-hid-client.c b/drivers/hid/intel-ish-hid/ishtp-hid-client.c
index e3d70c5..fbd4f8e 100644
--- a/drivers/hid/intel-ish-hid/ishtp-hid-client.c
+++ b/drivers/hid/intel-ish-hid/ishtp-hid-client.c
@@ -639,47 +639,26 @@ static int ishtp_get_report_descriptor(struct ishtp_cl *hid_ishtp_cl,
  *
  * Return: 0 on success, non zero on error
  */
-static int hid_ishtp_cl_init(struct ishtp_cl *hid_ishtp_cl, int reset)
+static int hid_ishtp_cl_init(struct ishtp_cl *hid_ishtp_cl, bool reset)
 {
-	struct ishtp_device *dev;
 	struct ishtp_cl_data *client_data = ishtp_get_client_data(hid_ishtp_cl);
-	struct ishtp_fw_client *fw_client;
 	int i;
 	int rv;
 
 	dev_dbg(cl_data_to_dev(client_data), "%s\n", __func__);
 	hid_ishtp_trace(client_data,  "%s reset flag: %d\n", __func__, reset);
 
-	rv = ishtp_cl_link(hid_ishtp_cl);
-	if (rv) {
-		dev_err(cl_data_to_dev(client_data),
-			"ishtp_cl_link failed\n");
-		return	-ENOMEM;
-	}
-
 	client_data->init_done = 0;
 
-	dev = ishtp_get_ishtp_device(hid_ishtp_cl);
-
-	/* Connect to FW client */
-	ishtp_set_tx_ring_size(hid_ishtp_cl, HID_CL_TX_RING_SIZE);
-	ishtp_set_rx_ring_size(hid_ishtp_cl, HID_CL_RX_RING_SIZE);
-
-	fw_client = ishtp_fw_cl_get_client(dev, &hid_ishtp_id_table[0].guid);
-	if (!fw_client) {
-		dev_err(cl_data_to_dev(client_data),
-			"ish client uuid not found\n");
-		return -ENOENT;
-	}
-	ishtp_cl_set_fw_client_id(hid_ishtp_cl,
-				  ishtp_get_fw_client_id(fw_client));
-	ishtp_set_connection_state(hid_ishtp_cl, ISHTP_CL_CONNECTING);
-
-	rv = ishtp_cl_connect(hid_ishtp_cl);
+	rv = ishtp_cl_establish_connection(hid_ishtp_cl,
+					   &hid_ishtp_id_table[0].guid,
+					   HID_CL_TX_RING_SIZE,
+					   HID_CL_RX_RING_SIZE,
+					   reset);
 	if (rv) {
 		dev_err(cl_data_to_dev(client_data),
 			"client connect fail\n");
-		goto err_cl_unlink;
+		goto err_cl_disconnect;
 	}
 
 	hid_ishtp_trace(client_data,  "%s client connected\n", __func__);
@@ -723,10 +702,7 @@ static int hid_ishtp_cl_init(struct ishtp_cl *hid_ishtp_cl, int reset)
 	return 0;
 
 err_cl_disconnect:
-	ishtp_set_connection_state(hid_ishtp_cl, ISHTP_CL_DISCONNECTING);
-	ishtp_cl_disconnect(hid_ishtp_cl);
-err_cl_unlink:
-	ishtp_cl_unlink(hid_ishtp_cl);
+	ishtp_cl_destroy_connection(hid_ishtp_cl, reset);
 	return rv;
 }
 
@@ -738,8 +714,7 @@ static int hid_ishtp_cl_init(struct ishtp_cl *hid_ishtp_cl, int reset)
  */
 static void hid_ishtp_cl_deinit(struct ishtp_cl *hid_ishtp_cl)
 {
-	ishtp_cl_unlink(hid_ishtp_cl);
-	ishtp_cl_flush_queues(hid_ishtp_cl);
+	ishtp_cl_destroy_connection(hid_ishtp_cl, false);
 
 	/* disband and free all Tx and Rx client-level rings */
 	ishtp_cl_free(hid_ishtp_cl);
@@ -749,33 +724,23 @@ static void hid_ishtp_cl_reset_handler(struct work_struct *work)
 {
 	struct ishtp_cl_data *client_data;
 	struct ishtp_cl *hid_ishtp_cl;
-	struct ishtp_cl_device *cl_device;
 	int retry;
 	int rv;
 
 	client_data = container_of(work, struct ishtp_cl_data, work);
 
 	hid_ishtp_cl = client_data->hid_ishtp_cl;
-	cl_device = client_data->cl_device;
 
 	hid_ishtp_trace(client_data, "%s hid_ishtp_cl %p\n", __func__,
 			hid_ishtp_cl);
 	dev_dbg(ishtp_device(client_data->cl_device), "%s\n", __func__);
 
-	hid_ishtp_cl_deinit(hid_ishtp_cl);
-
-	hid_ishtp_cl = ishtp_cl_allocate(cl_device);
-	if (!hid_ishtp_cl)
-		return;
-
-	ishtp_set_drvdata(cl_device, hid_ishtp_cl);
-	ishtp_set_client_data(hid_ishtp_cl, client_data);
-	client_data->hid_ishtp_cl = hid_ishtp_cl;
+	ishtp_cl_destroy_connection(hid_ishtp_cl, true);
 
 	client_data->num_hid_devices = 0;
 
 	for (retry = 0; retry < 3; ++retry) {
-		rv = hid_ishtp_cl_init(hid_ishtp_cl, 1);
+		rv = hid_ishtp_cl_init(hid_ishtp_cl, true);
 		if (!rv)
 			break;
 		dev_err(cl_data_to_dev(client_data), "Retry reset init\n");
@@ -841,7 +806,7 @@ static int hid_ishtp_cl_probe(struct ishtp_cl_device *cl_device)
 
 	ishtp_hid_print_trace = ishtp_trace_callback(cl_device);
 
-	rv = hid_ishtp_cl_init(hid_ishtp_cl, 0);
+	rv = hid_ishtp_cl_init(hid_ishtp_cl, false);
 	if (rv) {
 		ishtp_cl_free(hid_ishtp_cl);
 		return rv;
@@ -868,11 +833,9 @@ static void hid_ishtp_cl_remove(struct ishtp_cl_device *cl_device)
 			hid_ishtp_cl);
 
 	dev_dbg(ishtp_device(cl_device), "%s\n", __func__);
-	ishtp_set_connection_state(hid_ishtp_cl, ISHTP_CL_DISCONNECTING);
-	ishtp_cl_disconnect(hid_ishtp_cl);
+	hid_ishtp_cl_deinit(hid_ishtp_cl);
 	ishtp_put_device(cl_device);
 	ishtp_hid_remove(client_data);
-	hid_ishtp_cl_deinit(hid_ishtp_cl);
 
 	hid_ishtp_cl = NULL;
 
-- 
2.7.4

[PATCH 3/4] HID: intel-ish-hid: ishtp-fw-loader: use helper functions for connection

[Even Xu]

Use helper functions ishtp_cl_establish_connection() and
ishtp_cl_destroy_connection() to establish and destroy connection
respectively. These functions are used during initialization, reset and
deinitialization flows.

No functional changes are expected.

Signed-off-by: Even Xu <even.xu@intel.com>
Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
 drivers/hid/intel-ish-hid/ishtp-fw-loader.c | 60 ++++++-----------------------
 1 file changed, 12 insertions(+), 48 deletions(-)

diff --git a/drivers/hid/intel-ish-hid/ishtp-fw-loader.c b/drivers/hid/intel-ish-hid/ishtp-fw-loader.c
index 16aa030..e157863 100644
--- a/drivers/hid/intel-ish-hid/ishtp-fw-loader.c
+++ b/drivers/hid/intel-ish-hid/ishtp-fw-loader.c
@@ -840,43 +840,22 @@ static void load_fw_from_host_handler(struct work_struct *work)
  *
  * Return: 0 for success, negative error code for failure
  */
-static int loader_init(struct ishtp_cl *loader_ishtp_cl, int reset)
+static int loader_init(struct ishtp_cl *loader_ishtp_cl, bool reset)
 {
 	int rv;
-	struct ishtp_fw_client *fw_client;
 	struct ishtp_cl_data *client_data =
 		ishtp_get_client_data(loader_ishtp_cl);
 
 	dev_dbg(cl_data_to_dev(client_data), "reset flag: %d\n", reset);
 
-	rv = ishtp_cl_link(loader_ishtp_cl);
-	if (rv < 0) {
-		dev_err(cl_data_to_dev(client_data), "ishtp_cl_link failed\n");
-		return rv;
-	}
-
-	/* Connect to firmware client */
-	ishtp_set_tx_ring_size(loader_ishtp_cl, LOADER_CL_TX_RING_SIZE);
-	ishtp_set_rx_ring_size(loader_ishtp_cl, LOADER_CL_RX_RING_SIZE);
-
-	fw_client =
-		ishtp_fw_cl_get_client(ishtp_get_ishtp_device(loader_ishtp_cl),
-				       &loader_ishtp_id_table[0].guid);
-	if (!fw_client) {
-		dev_err(cl_data_to_dev(client_data),
-			"ISH client uuid not found\n");
-		rv = -ENOENT;
-		goto err_cl_unlink;
-	}
-
-	ishtp_cl_set_fw_client_id(loader_ishtp_cl,
-				  ishtp_get_fw_client_id(fw_client));
-	ishtp_set_connection_state(loader_ishtp_cl, ISHTP_CL_CONNECTING);
-
-	rv = ishtp_cl_connect(loader_ishtp_cl);
+	rv = ishtp_cl_establish_connection(loader_ishtp_cl,
+					   &loader_ishtp_id_table[0].guid,
+					   LOADER_CL_TX_RING_SIZE,
+					   LOADER_CL_RX_RING_SIZE,
+					   reset);
 	if (rv < 0) {
 		dev_err(cl_data_to_dev(client_data), "Client connect fail\n");
-		goto err_cl_unlink;
+		goto err_cl_disconnect;
 	}
 
 	dev_dbg(cl_data_to_dev(client_data), "Client connected\n");
@@ -885,17 +864,14 @@ static int loader_init(struct ishtp_cl *loader_ishtp_cl, int reset)
 
 	return 0;
 
-err_cl_unlink:
-	ishtp_cl_unlink(loader_ishtp_cl);
+err_cl_disconnect:
+	ishtp_cl_destroy_connection(loader_ishtp_cl, reset);
 	return rv;
 }
 
 static void loader_deinit(struct ishtp_cl *loader_ishtp_cl)
 {
-	ishtp_set_connection_state(loader_ishtp_cl, ISHTP_CL_DISCONNECTING);
-	ishtp_cl_disconnect(loader_ishtp_cl);
-	ishtp_cl_unlink(loader_ishtp_cl);
-	ishtp_cl_flush_queues(loader_ishtp_cl);
+	ishtp_cl_destroy_connection(loader_ishtp_cl, false);
 
 	/* Disband and free all Tx and Rx client-level rings */
 	ishtp_cl_free(loader_ishtp_cl);
@@ -914,19 +890,7 @@ static void reset_handler(struct work_struct *work)
 	loader_ishtp_cl = client_data->loader_ishtp_cl;
 	cl_device = client_data->cl_device;
 
-	/* Unlink, flush queues & start again */
-	ishtp_cl_unlink(loader_ishtp_cl);
-	ishtp_cl_flush_queues(loader_ishtp_cl);
-	ishtp_cl_free(loader_ishtp_cl);
-
-	loader_ishtp_cl = ishtp_cl_allocate(cl_device);
-	if (!loader_ishtp_cl)
-		return;
-
-	ishtp_set_drvdata(cl_device, loader_ishtp_cl);
-	ishtp_set_client_data(loader_ishtp_cl, client_data);
-	client_data->loader_ishtp_cl = loader_ishtp_cl;
-	client_data->cl_device = cl_device;
+	ishtp_cl_destroy_connection(loader_ishtp_cl, true);
 
 	rv = loader_init(loader_ishtp_cl, 1);
 	if (rv < 0) {
@@ -974,7 +938,7 @@ static int loader_ishtp_cl_probe(struct ishtp_cl_device *cl_device)
 	INIT_WORK(&client_data->work_fw_load,
 		  load_fw_from_host_handler);
 
-	rv = loader_init(loader_ishtp_cl, 0);
+	rv = loader_init(loader_ishtp_cl, false);
 	if (rv < 0) {
 		ishtp_cl_free(loader_ishtp_cl);
 		return rv;
-- 
2.7.4

[PATCH 4/4] platform: chrome: cros_ec_ishtp: use helper functions for connection

[Even Xu]

Use helper functions ishtp_cl_establish_connection() and
ishtp_cl_destroy_connection() to establish and destroy connection
respectively. These functions are used during initialization, reset and
deinitialization flows.

No functional changes are expected.

Signed-off-by: Even Xu <even.xu@intel.com>
Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
 drivers/platform/chrome/cros_ec_ishtp.c | 74 +++++++--------------------------
 1 file changed, 15 insertions(+), 59 deletions(-)

diff --git a/drivers/platform/chrome/cros_ec_ishtp.c b/drivers/platform/chrome/cros_ec_ishtp.c
index cb2031c..5ac37bd 100644
--- a/drivers/platform/chrome/cros_ec_ishtp.c
+++ b/drivers/platform/chrome/cros_ec_ishtp.c
@@ -367,55 +367,33 @@ static void ish_event_cb(struct ishtp_cl_device *cl_device)
 /**
  * cros_ish_init() - Init function for ISHTP client
  * @cros_ish_cl: ISHTP client instance
+ * @reset: true if called from reset handler
  *
  * This function complete the initializtion of the client.
  *
  * Return: 0 for success, negative error code for failure.
  */
-static int cros_ish_init(struct ishtp_cl *cros_ish_cl)
+static int cros_ish_init(struct ishtp_cl *cros_ish_cl, bool reset)
 {
 	int rv;
-	struct ishtp_device *dev;
-	struct ishtp_fw_client *fw_client;
 	struct ishtp_cl_data *client_data = ishtp_get_client_data(cros_ish_cl);
 
-	rv = ishtp_cl_link(cros_ish_cl);
-	if (rv) {
-		dev_err(cl_data_to_dev(client_data),
-			"ishtp_cl_link failed\n");
-		return rv;
-	}
-
-	dev = ishtp_get_ishtp_device(cros_ish_cl);
-
-	/* Connect to firmware client */
-	ishtp_set_tx_ring_size(cros_ish_cl, CROS_ISH_CL_TX_RING_SIZE);
-	ishtp_set_rx_ring_size(cros_ish_cl, CROS_ISH_CL_RX_RING_SIZE);
-
-	fw_client = ishtp_fw_cl_get_client(dev, &cros_ec_ishtp_id_table[0].guid);
-	if (!fw_client) {
-		dev_err(cl_data_to_dev(client_data),
-			"ish client uuid not found\n");
-		rv = -ENOENT;
-		goto err_cl_unlink;
-	}
-
-	ishtp_cl_set_fw_client_id(cros_ish_cl,
-				  ishtp_get_fw_client_id(fw_client));
-	ishtp_set_connection_state(cros_ish_cl, ISHTP_CL_CONNECTING);
-
-	rv = ishtp_cl_connect(cros_ish_cl);
+	rv = ishtp_cl_establish_connection(cros_ish_cl,
+					   &cros_ec_ishtp_id_table[0].guid,
+					   CROS_ISH_CL_TX_RING_SIZE,
+					   CROS_ISH_CL_RX_RING_SIZE,
+					   reset);
 	if (rv) {
 		dev_err(cl_data_to_dev(client_data),
 			"client connect fail\n");
-		goto err_cl_unlink;
+		goto err_cl_disconnect;
 	}
 
 	ishtp_register_event_cb(client_data->cl_device, ish_event_cb);
 	return 0;
 
-err_cl_unlink:
-	ishtp_cl_unlink(cros_ish_cl);
+err_cl_disconnect:
+	ishtp_cl_destroy_connection(cros_ish_cl, reset);
 	return rv;
 }
 
@@ -427,10 +405,7 @@ static int cros_ish_init(struct ishtp_cl *cros_ish_cl)
  */
 static void cros_ish_deinit(struct ishtp_cl *cros_ish_cl)
 {
-	ishtp_set_connection_state(cros_ish_cl, ISHTP_CL_DISCONNECTING);
-	ishtp_cl_disconnect(cros_ish_cl);
-	ishtp_cl_unlink(cros_ish_cl);
-	ishtp_cl_flush_queues(cros_ish_cl);
+	ishtp_cl_destroy_connection(cros_ish_cl, false);
 
 	/* Disband and free all Tx and Rx client-level rings */
 	ishtp_cl_free(cros_ish_cl);
@@ -592,7 +567,6 @@ static void reset_handler(struct work_struct *work)
 	int rv;
 	struct device *dev;
 	struct ishtp_cl *cros_ish_cl;
-	struct ishtp_cl_device *cl_device;
 	struct ishtp_cl_data *client_data =
 		container_of(work, struct ishtp_cl_data, work_ishtp_reset);
 
@@ -600,26 +574,11 @@ static void reset_handler(struct work_struct *work)
 	down_write(&init_lock);
 
 	cros_ish_cl = client_data->cros_ish_cl;
-	cl_device = client_data->cl_device;
 
-	/* Unlink, flush queues & start again */
-	ishtp_cl_unlink(cros_ish_cl);
-	ishtp_cl_flush_queues(cros_ish_cl);
-	ishtp_cl_free(cros_ish_cl);
-
-	cros_ish_cl = ishtp_cl_allocate(cl_device);
-	if (!cros_ish_cl) {
-		up_write(&init_lock);
-		return;
-	}
-
-	ishtp_set_drvdata(cl_device, cros_ish_cl);
-	ishtp_set_client_data(cros_ish_cl, client_data);
-	client_data->cros_ish_cl = cros_ish_cl;
+	ishtp_cl_destroy_connection(cros_ish_cl, true);
 
-	rv = cros_ish_init(cros_ish_cl);
+	rv = cros_ish_init(cros_ish_cl, true);
 	if (rv) {
-		ishtp_cl_free(cros_ish_cl);
 		dev_err(cl_data_to_dev(client_data), "Reset Failed\n");
 		up_write(&init_lock);
 		return;
@@ -672,7 +631,7 @@ static int cros_ec_ishtp_probe(struct ishtp_cl_device *cl_device)
 	INIT_WORK(&client_data->work_ec_evt,
 		  ish_evt_handler);
 
-	rv = cros_ish_init(cros_ish_cl);
+	rv = cros_ish_init(cros_ish_cl, false);
 	if (rv)
 		goto end_ishtp_cl_init_error;
 
@@ -690,10 +649,7 @@ static int cros_ec_ishtp_probe(struct ishtp_cl_device *cl_device)
 	return 0;
 
 end_cros_ec_dev_init_error:
-	ishtp_set_connection_state(cros_ish_cl, ISHTP_CL_DISCONNECTING);
-	ishtp_cl_disconnect(cros_ish_cl);
-	ishtp_cl_unlink(cros_ish_cl);
-	ishtp_cl_flush_queues(cros_ish_cl);
+	ishtp_cl_destroy_connection(cros_ish_cl, false);
 	ishtp_put_device(cl_device);
 end_ishtp_cl_init_error:
 	ishtp_cl_free(cros_ish_cl);
-- 
2.7.4

Re: [PATCH 4/4] platform: chrome: cros_ec_ishtp: use helper functions for connection

[Tzung-Bi Shih]

On Tue, Dec 05, 2023 at 09:50:33AM +0800, Even Xu wrote:
> Use helper functions ishtp_cl_establish_connection() and
> ishtp_cl_destroy_connection() to establish and destroy connection
> respectively. These functions are used during initialization, reset and
> deinitialization flows.
> 
> No functional changes are expected.
> 
> Signed-off-by: Even Xu <even.xu@intel.com>
> Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>

Acked-by: Tzung-Bi Shih <tzungbi@kernel.org>

One minor suggestion: we usually use "platform/chrome:" instead of
"platform: chrome:" for the title prefix.

RE: [PATCH 4/4] platform: chrome: cros_ec_ishtp: use helper functions for connection

[Xu, Even]

Thanks Tzung-Bi!

Best Regards,
Even Xu

-----Original Message-----
From: Tzung-Bi Shih <tzungbi@kernel.org> 
Sent: Wednesday, December 6, 2023 11:33 AM
To: Xu, Even <even.xu@intel.com>
Cc: jikos@kernel.org; srinivas.pandruvada@linux.intel.com; bleung@chromium.org; groeck@chromium.org; linux-input@vger.kernel.org; chrome-platform@lists.linux.dev
Subject: Re: [PATCH 4/4] platform: chrome: cros_ec_ishtp: use helper functions for connection

On Tue, Dec 05, 2023 at 09:50:33AM +0800, Even Xu wrote:
> Use helper functions ishtp_cl_establish_connection() and
> ishtp_cl_destroy_connection() to establish and destroy connection 
> respectively. These functions are used during initialization, reset 
> and deinitialization flows.
> 
> No functional changes are expected.
> 
> Signed-off-by: Even Xu <even.xu@intel.com>
> Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>

Acked-by: Tzung-Bi Shih <tzungbi@kernel.org>

One minor suggestion: we usually use "platform/chrome:" instead of
"platform: chrome:" for the title prefix.

Re: [PATCH 4/4] platform: chrome: cros_ec_ishtp: use helper functions for connection

[Jiri Kosina]

On Wed, 6 Dec 2023, Tzung-Bi Shih wrote:

> On Tue, Dec 05, 2023 at 09:50:33AM +0800, Even Xu wrote:
> > Use helper functions ishtp_cl_establish_connection() and
> > ishtp_cl_destroy_connection() to establish and destroy connection
> > respectively. These functions are used during initialization, reset and
> > deinitialization flows.
> > 
> > No functional changes are expected.
> > 
> > Signed-off-by: Even Xu <even.xu@intel.com>
> > Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> 
> Acked-by: Tzung-Bi Shih <tzungbi@kernel.org>

Thanks.

> One minor suggestion: we usually use "platform/chrome:" instead of
> "platform: chrome:" for the title prefix.

I have changed that, and am taking it together with the rest of the series 
through hid.git#for-6.8/intel-ish.

-- 
Jiri Kosina
SUSE Labs

RE: [PATCH 4/4] platform: chrome: cros_ec_ishtp: use helper functions for connection

[Xu, Even]

Thanks Jiri!

Best Regards,
Even Xu

-----Original Message-----
From: Jiri Kosina <jkosina@suse.com> 
Sent: Wednesday, December 6, 2023 6:34 PM
To: Tzung-Bi Shih <tzungbi@kernel.org>
Cc: Xu, Even <even.xu@intel.com>; srinivas.pandruvada@linux.intel.com; bleung@chromium.org; groeck@chromium.org; linux-input@vger.kernel.org; chrome-platform@lists.linux.dev
Subject: Re: [PATCH 4/4] platform: chrome: cros_ec_ishtp: use helper functions for connection

On Wed, 6 Dec 2023, Tzung-Bi Shih wrote:

> On Tue, Dec 05, 2023 at 09:50:33AM +0800, Even Xu wrote:
> > Use helper functions ishtp_cl_establish_connection() and
> > ishtp_cl_destroy_connection() to establish and destroy connection 
> > respectively. These functions are used during initialization, reset 
> > and deinitialization flows.
> > 
> > No functional changes are expected.
> > 
> > Signed-off-by: Even Xu <even.xu@intel.com>
> > Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> 
> Acked-by: Tzung-Bi Shih <tzungbi@kernel.org>

Thanks.

> One minor suggestion: we usually use "platform/chrome:" instead of
> "platform: chrome:" for the title prefix.

I have changed that, and am taking it together with the rest of the series through hid.git#for-6.8/intel-ish.

--
Jiri Kosina
SUSE Labs