[Intel-wired-lan] [PATCH intel-next 1/3] ice: remove u16 arithmetic in ice_gnss

[Intel-wired-lan] [PATCH intel-next 1/3] ice: remove u16 arithmetic in ice_gnss

[Karol Kolacinski]

Change u16 to u32 where arithmetic occured.

Signed-off-by: Karol Kolacinski <karol.kolacinski@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_gnss.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_gnss.c b/drivers/net/ethernet/intel/ice/ice_gnss.c
index 57586a2e6dec..f0b2091c3b5f 100644
--- a/drivers/net/ethernet/intel/ice/ice_gnss.c
+++ b/drivers/net/ethernet/intel/ice/ice_gnss.c
@@ -23,7 +23,7 @@ static void ice_gnss_read(struct kthread_work *work)
 	struct ice_hw *hw;
 	__be16 data_len_b;
 	char *buf = NULL;
-	u16 i, data_len;
+	u32 i, data_len;
 	int err = 0;
 
 	pf = gnss->back;
@@ -65,7 +65,7 @@ static void ice_gnss_read(struct kthread_work *work)
 		mdelay(10);
 	}
 
-	data_len = min(data_len, (u16)PAGE_SIZE);
+	data_len = min_t(typeof(data_len), data_len, PAGE_SIZE);
 	data_len = tty_buffer_request_room(port, data_len);
 	if (!data_len) {
 		err = -ENOMEM;
@@ -74,7 +74,7 @@ static void ice_gnss_read(struct kthread_work *work)
 
 	/* Read received data */
 	for (i = 0; i < data_len; i += bytes_read) {
-		u16 bytes_left = data_len - i;
+		u32 bytes_left = data_len - i;
 
 		bytes_read = min_t(typeof(bytes_left), bytes_left, ICE_MAX_I2C_DATA_SIZE);
 
-- 
2.34.1

[Intel-wired-lan] [PATCH intel-next 2/3] ice: add i2c write command

[Karol Kolacinski]

Add the possibility to write to connected i2c devices. FW may reject
the write if the device is not on allowlist.

Signed-off-by: Karol Kolacinski <karol.kolacinski@intel.com>
---
 .../net/ethernet/intel/ice/ice_adminq_cmd.h   |  7 +--
 drivers/net/ethernet/intel/ice/ice_common.c   | 45 ++++++++++++++++++-
 drivers/net/ethernet/intel/ice/ice_common.h   |  4 ++
 3 files changed, 52 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
index b25e27c4d887..bedc19f12cbd 100644
--- a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
+++ b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
@@ -1401,7 +1401,7 @@ struct ice_aqc_get_link_topo {
 	u8 rsvd[9];
 };
 
-/* Read I2C (direct, 0x06E2) */
+/* Read/Write I2C (direct, 0x06E2/0x06E3) */
 struct ice_aqc_i2c {
 	struct ice_aqc_link_topo_addr topo_addr;
 	__le16 i2c_addr;
@@ -1411,7 +1411,7 @@ struct ice_aqc_i2c {
 
 	u8 rsvd;
 	__le16 i2c_bus_addr;
-	u8 rsvd2[4];
+	u8 i2c_data[4]; /* Used only by write command, reserved in read. */
 };
 
 /* Read I2C Response (direct, 0x06E2) */
@@ -2130,7 +2130,7 @@ struct ice_aq_desc {
 		struct ice_aqc_get_link_status get_link_status;
 		struct ice_aqc_event_lan_overflow lan_overflow;
 		struct ice_aqc_get_link_topo get_link_topo;
-		struct ice_aqc_i2c read_i2c;
+		struct ice_aqc_i2c read_write_i2c;
 		struct ice_aqc_read_i2c_resp read_i2c_resp;
 	} params;
 };
@@ -2247,6 +2247,7 @@ enum ice_adminq_opc {
 	ice_aqc_opc_set_mac_lb				= 0x0620,
 	ice_aqc_opc_get_link_topo			= 0x06E0,
 	ice_aqc_opc_read_i2c				= 0x06E2,
+	ice_aqc_opc_write_i2c				= 0x06E3,
 	ice_aqc_opc_set_port_id_led			= 0x06E9,
 	ice_aqc_opc_set_gpio				= 0x06EC,
 	ice_aqc_opc_get_gpio				= 0x06ED,
diff --git a/drivers/net/ethernet/intel/ice/ice_common.c b/drivers/net/ethernet/intel/ice/ice_common.c
index 9619bdb9e49a..85dd30f99814 100644
--- a/drivers/net/ethernet/intel/ice/ice_common.c
+++ b/drivers/net/ethernet/intel/ice/ice_common.c
@@ -4823,7 +4823,7 @@ ice_aq_read_i2c(struct ice_hw *hw, struct ice_aqc_link_topo_addr topo_addr,
 	int status;
 
 	ice_fill_dflt_direct_cmd_desc(&desc, ice_aqc_opc_read_i2c);
-	cmd = &desc.params.read_i2c;
+	cmd = &desc.params.read_write_i2c;
 
 	if (!data)
 		return -EINVAL;
@@ -4850,6 +4850,49 @@ ice_aq_read_i2c(struct ice_hw *hw, struct ice_aqc_link_topo_addr topo_addr,
 	return status;
 }
 
+/**
+ * ice_aq_write_i2c
+ * @hw: pointer to the hw struct
+ * @topo_addr: topology address for a device to communicate with
+ * @bus_addr: 7-bit I2C bus address
+ * @addr: I2C memory address (I2C offset) with up to 16 bits
+ * @params: I2C parameters: bit [4] - I2C address type, bits [3:0] - data size to write (0-7 bytes)
+ * @data: pointer to data (0 to 4 bytes) to be written to the I2C device
+ * @cd: pointer to command details structure or NULL
+ *
+ * Write I2C (0x06E3)
+ */
+int
+ice_aq_write_i2c(struct ice_hw *hw, struct ice_aqc_link_topo_addr topo_addr,
+		 u16 bus_addr, __le16 addr, u8 params, u8 *data,
+		 struct ice_sq_cd *cd)
+{
+	struct ice_aq_desc desc = { 0 };
+	struct ice_aqc_i2c *cmd;
+	u8 i, data_size;
+
+	ice_fill_dflt_direct_cmd_desc(&desc, ice_aqc_opc_write_i2c);
+	cmd = &desc.params.read_write_i2c;
+
+	data_size = FIELD_GET(ICE_AQC_I2C_DATA_SIZE_M, params);
+
+	/* data_size limited to 4 */
+	if (data_size > 4)
+		return -EINVAL;
+
+	cmd->i2c_bus_addr = cpu_to_le16(bus_addr);
+	cmd->topo_addr = topo_addr;
+	cmd->i2c_params = params;
+	cmd->i2c_addr = addr;
+
+	for (i = 0; i < data_size; i++) {
+		cmd->i2c_data[i] = *data;
+		data++;
+	}
+
+	return ice_aq_send_cmd(hw, &desc, NULL, 0, cd);
+}
+
 /**
  * ice_aq_set_driver_param - Set driver parameter to share via firmware
  * @hw: pointer to the HW struct
diff --git a/drivers/net/ethernet/intel/ice/ice_common.h b/drivers/net/ethernet/intel/ice/ice_common.h
index 872ea7d2332d..61b7c60db689 100644
--- a/drivers/net/ethernet/intel/ice/ice_common.h
+++ b/drivers/net/ethernet/intel/ice/ice_common.h
@@ -214,5 +214,9 @@ int
 ice_aq_read_i2c(struct ice_hw *hw, struct ice_aqc_link_topo_addr topo_addr,
 		u16 bus_addr, __le16 addr, u8 params, u8 *data,
 		struct ice_sq_cd *cd);
+int
+ice_aq_write_i2c(struct ice_hw *hw, struct ice_aqc_link_topo_addr topo_addr,
+		 u16 bus_addr, __le16 addr, u8 params, u8 *data,
+		 struct ice_sq_cd *cd);
 bool ice_fw_supports_report_dflt_cfg(struct ice_hw *hw);
 #endif /* _ICE_COMMON_H_ */
-- 
2.34.1

[Intel-wired-lan] [PATCH intel-next 3/3] ice: add write functionality for GNSS TTY

[Karol Kolacinski]

Add the possibility to write raw bytes to the GNSS module through the
first TTY device. This allow user to configure the module.

Create a second read-only TTY device.

Signed-off-by: Karol Kolacinski <karol.kolacinski@intel.com>
---
 drivers/net/ethernet/intel/ice/ice.h      |   4 +-
 drivers/net/ethernet/intel/ice/ice_gnss.c | 230 +++++++++++++++++++---
 drivers/net/ethernet/intel/ice/ice_gnss.h |  23 ++-
 3 files changed, 231 insertions(+), 26 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice.h b/drivers/net/ethernet/intel/ice/ice.h
index 17fb1ddea7b0..41bd0c7c7da5 100644
--- a/drivers/net/ethernet/intel/ice/ice.h
+++ b/drivers/net/ethernet/intel/ice/ice.h
@@ -558,8 +558,8 @@ struct ice_pf {
 	u32 msg_enable;
 	struct ice_ptp ptp;
 	struct tty_driver *ice_gnss_tty_driver;
-	struct tty_port gnss_tty_port;
-	struct gnss_serial *gnss_serial;
+	struct tty_port *gnss_tty_port[ICE_GNSS_TTY_MINOR_DEVICES];
+	struct gnss_serial *gnss_serial[ICE_GNSS_TTY_MINOR_DEVICES];
 	u16 num_rdma_msix;		/* Total MSIX vectors for RDMA driver */
 	u16 rdma_base_vector;
 
diff --git a/drivers/net/ethernet/intel/ice/ice_gnss.c b/drivers/net/ethernet/intel/ice/ice_gnss.c
index f0b2091c3b5f..eafebd873236 100644
--- a/drivers/net/ethernet/intel/ice/ice_gnss.c
+++ b/drivers/net/ethernet/intel/ice/ice_gnss.c
@@ -5,6 +5,99 @@
 #include "ice_lib.h"
 #include <linux/tty_driver.h>
 
+/**
+ * ice_gnss_do_write - Write data to internal GNSS
+ * @pf: board private structure
+ * @buf: command buffer
+ * @size: command buffer size
+ *
+ * Write UBX command data to the GNSS receiver
+ */
+static void ice_gnss_do_write(struct ice_pf *pf, u8 *buf, u32 size)
+{
+	u32 i, num_writes, part_writes_num, last_write_bytes, offset = 0;
+	struct ice_aqc_link_topo_addr link_topo;
+	struct ice_hw *hw = &pf->hw;
+	int err = 0;
+
+	memset(&link_topo, 0, sizeof(struct ice_aqc_link_topo_addr));
+	link_topo.topo_params.index = ICE_E810T_GNSS_I2C_BUS;
+	link_topo.topo_params.node_type_ctx |=
+		ICE_AQC_LINK_TOPO_NODE_CTX_OVERRIDE <<
+		ICE_AQC_LINK_TOPO_NODE_CTX_S;
+
+	/* Write all bytes except the last partial write */
+	last_write_bytes = size % ICE_GNSS_UBX_WRITE_BYTES;
+	if (last_write_bytes == 0)
+		part_writes_num = 0;
+	else if (last_write_bytes == 1)
+		part_writes_num = 2;
+	else
+		part_writes_num = 1;
+
+	num_writes = size / ICE_GNSS_UBX_WRITE_BYTES - part_writes_num;
+
+	for (i = 0; i < num_writes; i++) {
+		err = ice_aq_write_i2c(hw, link_topo, ICE_GNSS_UBX_I2C_BUS_ADDR,
+				       cpu_to_le16(buf[offset]),
+				       ICE_MAX_I2C_WRITE_BYTES,
+				       &buf[offset + 1], NULL);
+		if (err)
+			goto err;
+
+		offset += ICE_GNSS_UBX_WRITE_BYTES;
+	}
+
+	if (part_writes_num == 2) {
+		/* We cannot write a single byte to ublox. Do 2 last writes
+		 * instead of 1.
+		 */
+		err = ice_aq_write_i2c(hw, link_topo, ICE_GNSS_UBX_I2C_BUS_ADDR,
+				       cpu_to_le16(buf[offset]),
+				       ICE_MAX_I2C_WRITE_BYTES - 1,
+				       &buf[offset + 1], NULL);
+		if (err)
+			goto err;
+
+		offset += ICE_GNSS_UBX_WRITE_BYTES - 1;
+		last_write_bytes = 2;
+	}
+
+	if (part_writes_num)
+		err = ice_aq_write_i2c(hw, link_topo, ICE_GNSS_UBX_I2C_BUS_ADDR,
+				       cpu_to_le16(buf[offset]),
+				       last_write_bytes - 1, &buf[offset + 1],
+				       NULL);
+
+err:
+	if (err)
+		dev_err(ice_pf_to_dev(pf), "GNSS failed to write err=%d\n",
+			err);
+}
+
+/**
+ * ice_gnss_write_pending - Write all pending data to internal GNSS
+ * @work: GNSS write work structure
+ */
+static void ice_gnss_write_pending(struct kthread_work *work)
+{
+	struct gnss_serial *gnss = container_of(work, struct gnss_serial,
+						write_work);
+	struct ice_pf *pf = gnss->back;
+
+	if (!list_empty(&gnss->queue)) {
+		struct gnss_write_buf *write_buf = NULL;
+
+		write_buf = list_first_entry(&gnss->queue,
+					     struct gnss_write_buf, queue);
+		list_del(&write_buf->queue);
+
+		ice_gnss_do_write(pf, write_buf->buf, write_buf->size);
+		kfree(write_buf->buf);
+		kfree(write_buf);
+	}
+}
+
 /**
  * ice_gnss_read - Read data from internal GNSS module
  * @work: GNSS read work structure
@@ -103,8 +196,9 @@ static void ice_gnss_read(struct kthread_work *work)
 /**
  * ice_gnss_struct_init - Initialize GNSS structure for the TTY
  * @pf: Board private structure
+ * @index: TTY device index
  */
-static struct gnss_serial *ice_gnss_struct_init(struct ice_pf *pf)
+static struct gnss_serial *ice_gnss_struct_init(struct ice_pf *pf, int index)
 {
 	struct device *dev = ice_pf_to_dev(pf);
 	struct kthread_worker *kworker;
@@ -117,9 +211,11 @@ static struct gnss_serial *ice_gnss_struct_init(struct ice_pf *pf)
 	mutex_init(&gnss->gnss_mutex);
 	gnss->open_count = 0;
 	gnss->back = pf;
-	pf->gnss_serial = gnss;
+	pf->gnss_serial[index] = gnss;
 
 	kthread_init_delayed_work(&gnss->read_work, ice_gnss_read);
+	INIT_LIST_HEAD(&gnss->queue);
+	kthread_init_work(&gnss->write_work, ice_gnss_write_pending);
 	/* Allocate a kworker for handling work required for the GNSS TTY
 	 * writes.
 	 */
@@ -155,10 +251,10 @@ static int ice_gnss_tty_open(struct tty_struct *tty, struct file *filp)
 	tty->driver_data = NULL;
 
 	/* Get the serial object associated with this tty pointer */
-	gnss = pf->gnss_serial;
+	gnss = pf->gnss_serial[tty->index];
 	if (!gnss) {
 		/* Initialize GNSS struct on the first device open */
-		gnss = ice_gnss_struct_init(pf);
+		gnss = ice_gnss_struct_init(pf, tty->index);
 		if (!gnss)
 			return -ENOMEM;
 	}
@@ -211,25 +307,96 @@ static void ice_gnss_tty_close(struct tty_struct *tty, struct file *filp)
 }
 
 /**
- * ice_gnss_tty_write - Dummy TTY write function to avoid kernel panic
+ * ice_gnss_tty_write - Write GNSS data
  * @tty: pointer to the tty_struct
  * @buf: pointer to the user data
  * @cnt: the number of characters that was able to be sent to the hardware (or
  *       queued to be sent at a later time)
+ *
+ * The write function call is called by the user when there is data to be sent
+ * to the hardware. First the tty core receives the call, and then it passes the
+ * data on to the tty driver?s write function. The tty core also tells the tty
+ * driver the size of the data being sent.
+ * If any errors happen during the write call, a negative error value should be
+ * returned instead of the number of characters that were written.
  */
 static int
 ice_gnss_tty_write(struct tty_struct *tty, const unsigned char *buf, int cnt)
 {
-	return 0;
+	struct gnss_write_buf *write_buf;
+	struct gnss_serial *gnss;
+	struct ice_pf *pf;
+	u8 *cmd_buf;
+
+	/* We cannot write a single byte using our I2C implementation. */
+	if (cnt <= 1 || cnt > ICE_GNSS_TTY_WRITE_BUF)
+		return -EINVAL;
+
+	gnss = tty->driver_data;
+	if (!gnss)
+		return -EFAULT;
+
+	pf = (struct ice_pf *)tty->driver->driver_state;
+	if (!pf)
+		return -EFAULT;
+
+	/* Allow write only on TTY 0 */
+	if (gnss != pf->gnss_serial[0])
+		return -EIO;
+
+	mutex_lock(&gnss->gnss_mutex);
+
+	if (!gnss->open_count) {
+		mutex_unlock(&gnss->gnss_mutex);
+		return -EINVAL;
+	}
+
+	cmd_buf = kzalloc(sizeof(*buf) * cnt, GFP_KERNEL);
+	if (!cmd_buf)
+		return -ENOMEM;
+
+	memcpy(cmd_buf, buf, cnt);
+
+	/* Send the data out to a hardware port */
+	write_buf = kzalloc(sizeof(*write_buf), GFP_KERNEL);
+	if (!write_buf)
+		return -ENOMEM;
+
+	write_buf->buf = cmd_buf;
+	write_buf->size = cnt;
+	INIT_LIST_HEAD(&write_buf->queue);
+	list_add_tail(&write_buf->queue, &gnss->queue);
+	kthread_queue_work(gnss->kworker, &gnss->write_work);
+	mutex_unlock(&gnss->gnss_mutex);
+	return cnt;
 }
 
 /**
- * ice_gnss_tty_write_room - Dummy TTY write_room function to avoid kernel panic
+ * ice_gnss_tty_write_room - Returns the numbers of characters to be written.
  * @tty: pointer to the tty_struct
+ *
+ * This routine returns the numbers of characters the tty driver will accept
+ * for queuing to be written. This number is subject to change as output buffers
+ * get emptied, or if the output flow control is acted.
  */
 static unsigned int ice_gnss_tty_write_room(struct tty_struct *tty)
 {
-	return 0;
+	struct gnss_serial *gnss = tty->driver_data;
+	unsigned int room = 0;
+
+	/* Allow write only on TTY 0 */
+	if (!gnss || gnss != gnss->back->gnss_serial[0])
+		return room;
+
+	mutex_lock(&gnss->gnss_mutex);
+
+	if (!gnss->open_count)
+		goto exit;
+
+	room = ICE_GNSS_TTY_WRITE_BUF;
+exit:
+	mutex_unlock(&gnss->gnss_mutex);
+	return room;
 }
 
 static const struct tty_operations tty_gps_ops = {
@@ -250,10 +417,12 @@ static struct tty_driver *ice_gnss_create_tty_driver(struct ice_pf *pf)
 	struct tty_driver *tty_driver;
 	char *ttydrv_name;
 	int err;
+	u32 i;
 
-	tty_driver = tty_alloc_driver(1, TTY_DRIVER_REAL_RAW);
+	tty_driver = tty_alloc_driver(ICE_GNSS_TTY_MINOR_DEVICES,
+				      TTY_DRIVER_REAL_RAW);
 	if (IS_ERR(tty_driver)) {
-		dev_err(ice_pf_to_dev(pf), "Failed to allocate memory for GNSS TTY\n");
+		dev_err(dev, "Failed to allocate memory for GNSS TTY\n");
 		return NULL;
 	}
 
@@ -283,23 +452,32 @@ static struct tty_driver *ice_gnss_create_tty_driver(struct ice_pf *pf)
 	tty_driver->driver_state = pf;
 	tty_set_operations(tty_driver, &tty_gps_ops);
 
-	pf->gnss_serial = NULL;
+	for (i = 0; i < ICE_GNSS_TTY_MINOR_DEVICES; i++) {
+		pf->gnss_tty_port[i] = kzalloc(sizeof(*pf->gnss_tty_port[i]),
+					       GFP_KERNEL);
+		pf->gnss_serial[i] = NULL;
 
-	tty_port_init(&pf->gnss_tty_port);
-	tty_port_link_device(&pf->gnss_tty_port, tty_driver, 0);
+		tty_port_init(pf->gnss_tty_port[i]);
+		tty_port_link_device(pf->gnss_tty_port[i], tty_driver, i);
+	}
 
 	err = tty_register_driver(tty_driver);
 	if (err) {
-		dev_err(ice_pf_to_dev(pf), "Failed to register TTY driver err=%d\n",
-			err);
+		dev_err(dev, "Failed to register TTY driver err=%d\n", err);
 
-		tty_port_destroy(&pf->gnss_tty_port);
+		for (i = 0; i < ICE_GNSS_TTY_MINOR_DEVICES; i++) {
+			tty_port_destroy(pf->gnss_tty_port[i]);
+			kfree(pf->gnss_tty_port[i]);
+		}
 		kfree(ttydrv_name);
 		tty_driver_kref_put(pf->ice_gnss_tty_driver);
 
 		return NULL;
 	}
 
+	for (i = 0; i < ICE_GNSS_TTY_MINOR_DEVICES; i++)
+		dev_info(dev, "%s%d registered\n", ttydrv_name, i);
+
 	return tty_driver;
 }
 
@@ -327,17 +505,25 @@ void ice_gnss_init(struct ice_pf *pf)
  */
 void ice_gnss_exit(struct ice_pf *pf)
 {
+	u32 i;
+
 	if (!test_bit(ICE_FLAG_GNSS, pf->flags) || !pf->ice_gnss_tty_driver)
 		return;
 
-	tty_port_destroy(&pf->gnss_tty_port);
+	for (i = 0; i < ICE_GNSS_TTY_MINOR_DEVICES; i++) {
+		if (pf->gnss_tty_port[i]) {
+			tty_port_destroy(pf->gnss_tty_port[i]);
+			kfree(pf->gnss_tty_port[i]);
+		}
 
-	if (pf->gnss_serial) {
-		struct gnss_serial *gnss = pf->gnss_serial;
+		if (pf->gnss_serial[i]) {
+			struct gnss_serial *gnss = pf->gnss_serial[i];
 
-		kthread_cancel_delayed_work_sync(&gnss->read_work);
-		kfree(gnss);
-		pf->gnss_serial = NULL;
+			kthread_cancel_work_sync(&gnss->write_work);
+			kthread_cancel_delayed_work_sync(&gnss->read_work);
+			kfree(gnss);
+			pf->gnss_serial[i] = NULL;
+		}
 	}
 
 	tty_unregister_driver(pf->ice_gnss_tty_driver);
diff --git a/drivers/net/ethernet/intel/ice/ice_gnss.h b/drivers/net/ethernet/intel/ice/ice_gnss.h
index 9211adb2372c..59ba5749143e 100644
--- a/drivers/net/ethernet/intel/ice/ice_gnss.h
+++ b/drivers/net/ethernet/intel/ice/ice_gnss.h
@@ -8,14 +8,31 @@
 #include <linux/tty_flip.h>
 
 #define ICE_E810T_GNSS_I2C_BUS		0x2
+#define ICE_GNSS_TIMER_DELAY_TIME	(HZ / 10) /* 0.1 second per message */
+#define ICE_GNSS_TTY_MINOR_DEVICES	2
+#define ICE_GNSS_TTY_WRITE_BUF		250
+#define ICE_MAX_I2C_DATA_SIZE		FIELD_MAX(ICE_AQC_I2C_DATA_SIZE_M)
+#define ICE_MAX_I2C_WRITE_BYTES		4
+
+/* ublox specific deifinitions */
 #define ICE_GNSS_UBX_I2C_BUS_ADDR	0x42
 /* Data length register is big endian */
 #define ICE_GNSS_UBX_DATA_LEN_H		0xFD
 #define ICE_GNSS_UBX_DATA_LEN_WIDTH	2
 #define ICE_GNSS_UBX_EMPTY_DATA		0xFF
-#define ICE_GNSS_TIMER_DELAY_TIME	(HZ / 10) /* 0.1 second per message */
-#define ICE_MAX_I2C_DATA_SIZE		FIELD_MAX(ICE_AQC_I2C_DATA_SIZE_M)
+/* For ublox writes are performed without address so the first byte to write is
+ * passed as I2C addr parameter.
+ */
+#define ICE_GNSS_UBX_WRITE_BYTES	(ICE_MAX_I2C_WRITE_BYTES + 1)
 #define ICE_MAX_UBX_READ_TRIES		255
+#define ICE_MAX_UBX_ACK_READ_TRIES	4095
+
+struct gnss_write_buf {
+	struct list_head queue;
+	u32 size;
+	u8 *buf;
+};
+
 
 /**
  * struct gnss_serial - data used to initialize GNSS TTY port
@@ -33,6 +50,8 @@ struct gnss_serial {
 	struct mutex gnss_mutex; /* protects GNSS serial structure */
 	struct kthread_worker *kworker;
 	struct kthread_delayed_work read_work;
+	struct kthread_work write_work;
+	struct list_head queue;
 };
 
 #if IS_ENABLED(CONFIG_TTY)
-- 
2.34.1

[Intel-wired-lan] [PATCH intel-next 1/3] ice: remove u16 arithmetic in ice_gnss

[Paul Menzel]

Dear Karol,


Am 15.04.22 um 12:31 schrieb Karol Kolacinski:
> Change u16 to u32 where arithmetic occured.

occur*r*e*s*

Why does 16-bit width not work?


Kind regards,

Paul


> Signed-off-by: Karol Kolacinski <karol.kolacinski@intel.com>
> ---
>   drivers/net/ethernet/intel/ice/ice_gnss.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_gnss.c b/drivers/net/ethernet/intel/ice/ice_gnss.c
> index 57586a2e6dec..f0b2091c3b5f 100644
> --- a/drivers/net/ethernet/intel/ice/ice_gnss.c
> +++ b/drivers/net/ethernet/intel/ice/ice_gnss.c
> @@ -23,7 +23,7 @@ static void ice_gnss_read(struct kthread_work *work)
>   	struct ice_hw *hw;
>   	__be16 data_len_b;
>   	char *buf = NULL;
> -	u16 i, data_len;
> +	u32 i, data_len;

Why does the length need to be specified at all, and no native types can 
be used?


Kind regards,

Paul


>   	int err = 0;
>   
>   	pf = gnss->back;
> @@ -65,7 +65,7 @@ static void ice_gnss_read(struct kthread_work *work)
>   		mdelay(10);
>   	}
>   
> -	data_len = min(data_len, (u16)PAGE_SIZE);
> +	data_len = min_t(typeof(data_len), data_len, PAGE_SIZE);
>   	data_len = tty_buffer_request_room(port, data_len);
>   	if (!data_len) {
>   		err = -ENOMEM;
> @@ -74,7 +74,7 @@ static void ice_gnss_read(struct kthread_work *work)
>   
>   	/* Read received data */
>   	for (i = 0; i < data_len; i += bytes_read) {
> -		u16 bytes_left = data_len - i;
> +		u32 bytes_left = data_len - i;
>   
>   		bytes_read = min_t(typeof(bytes_left), bytes_left, ICE_MAX_I2C_DATA_SIZE);
>   

[Intel-wired-lan] [PATCH intel-next 2/3] ice: add i2c write command

[Paul Menzel]

Dear Karol,


Thank you for your patch.

Am 15.04.22 um 12:31 schrieb Karol Kolacinski:
> Add the possibility to write to connected i2c devices. FW may reject
> the write if the device is not on allowlist.

Did you use a datasheet for implementing this. If so, please mention the 
name and revision.

> Signed-off-by: Karol Kolacinski <karol.kolacinski@intel.com>
> ---
>   .../net/ethernet/intel/ice/ice_adminq_cmd.h   |  7 +--
>   drivers/net/ethernet/intel/ice/ice_common.c   | 45 ++++++++++++++++++-
>   drivers/net/ethernet/intel/ice/ice_common.h   |  4 ++
>   3 files changed, 52 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
> index b25e27c4d887..bedc19f12cbd 100644
> --- a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
> +++ b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
> @@ -1401,7 +1401,7 @@ struct ice_aqc_get_link_topo {
>   	u8 rsvd[9];
>   };
>   
> -/* Read I2C (direct, 0x06E2) */
> +/* Read/Write I2C (direct, 0x06E2/0x06E3) */
>   struct ice_aqc_i2c {
>   	struct ice_aqc_link_topo_addr topo_addr;
>   	__le16 i2c_addr;
> @@ -1411,7 +1411,7 @@ struct ice_aqc_i2c {
>   
>   	u8 rsvd;
>   	__le16 i2c_bus_addr;
> -	u8 rsvd2[4];
> +	u8 i2c_data[4]; /* Used only by write command, reserved in read. */
>   };
>   
>   /* Read I2C Response (direct, 0x06E2) */
> @@ -2130,7 +2130,7 @@ struct ice_aq_desc {
>   		struct ice_aqc_get_link_status get_link_status;
>   		struct ice_aqc_event_lan_overflow lan_overflow;
>   		struct ice_aqc_get_link_topo get_link_topo;
> -		struct ice_aqc_i2c read_i2c;
> +		struct ice_aqc_i2c read_write_i2c;
>   		struct ice_aqc_read_i2c_resp read_i2c_resp;
>   	} params;
>   };
> @@ -2247,6 +2247,7 @@ enum ice_adminq_opc {
>   	ice_aqc_opc_set_mac_lb				= 0x0620,
>   	ice_aqc_opc_get_link_topo			= 0x06E0,
>   	ice_aqc_opc_read_i2c				= 0x06E2,
> +	ice_aqc_opc_write_i2c				= 0x06E3,
>   	ice_aqc_opc_set_port_id_led			= 0x06E9,
>   	ice_aqc_opc_set_gpio				= 0x06EC,
>   	ice_aqc_opc_get_gpio				= 0x06ED,
> diff --git a/drivers/net/ethernet/intel/ice/ice_common.c b/drivers/net/ethernet/intel/ice/ice_common.c
> index 9619bdb9e49a..85dd30f99814 100644
> --- a/drivers/net/ethernet/intel/ice/ice_common.c
> +++ b/drivers/net/ethernet/intel/ice/ice_common.c
> @@ -4823,7 +4823,7 @@ ice_aq_read_i2c(struct ice_hw *hw, struct ice_aqc_link_topo_addr topo_addr,
>   	int status;
>   
>   	ice_fill_dflt_direct_cmd_desc(&desc, ice_aqc_opc_read_i2c);
> -	cmd = &desc.params.read_i2c;
> +	cmd = &desc.params.read_write_i2c;
>   
>   	if (!data)
>   		return -EINVAL;
> @@ -4850,6 +4850,49 @@ ice_aq_read_i2c(struct ice_hw *hw, struct ice_aqc_link_topo_addr topo_addr,
>   	return status;
>   }
>   
> +/**
> + * ice_aq_write_i2c
> + * @hw: pointer to the hw struct
> + * @topo_addr: topology address for a device to communicate with
> + * @bus_addr: 7-bit I2C bus address
> + * @addr: I2C memory address (I2C offset) with up to 16 bits
> + * @params: I2C parameters: bit [4] - I2C address type, bits [3:0] - data size to write (0-7 bytes)
> + * @data: pointer to data (0 to 4 bytes) to be written to the I2C device
> + * @cd: pointer to command details structure or NULL

Also document the return value?

> + *
> + * Write I2C (0x06E3)
> + */
> +int
> +ice_aq_write_i2c(struct ice_hw *hw, struct ice_aqc_link_topo_addr topo_addr,
> +		 u16 bus_addr, __le16 addr, u8 params, u8 *data,
> +		 struct ice_sq_cd *cd)
> +{
> +	struct ice_aq_desc desc = { 0 };
> +	struct ice_aqc_i2c *cmd;
> +	u8 i, data_size;

The loop variable should be a native type.

> +
> +	ice_fill_dflt_direct_cmd_desc(&desc, ice_aqc_opc_write_i2c);
> +	cmd = &desc.params.read_write_i2c;
> +
> +	data_size = FIELD_GET(ICE_AQC_I2C_DATA_SIZE_M, params);
> +
> +	/* data_size limited to 4 */
> +	if (data_size > 4)
> +		return -EINVAL;
> +
> +	cmd->i2c_bus_addr = cpu_to_le16(bus_addr);
> +	cmd->topo_addr = topo_addr;
> +	cmd->i2c_params = params;
> +	cmd->i2c_addr = addr;
> +
> +	for (i = 0; i < data_size; i++) {
> +		cmd->i2c_data[i] = *data;
> +		data++;
> +	}
> +
> +	return ice_aq_send_cmd(hw, &desc, NULL, 0, cd);
> +}
> +
>   /**
>    * ice_aq_set_driver_param - Set driver parameter to share via firmware
>    * @hw: pointer to the HW struct
> diff --git a/drivers/net/ethernet/intel/ice/ice_common.h b/drivers/net/ethernet/intel/ice/ice_common.h
> index 872ea7d2332d..61b7c60db689 100644
> --- a/drivers/net/ethernet/intel/ice/ice_common.h
> +++ b/drivers/net/ethernet/intel/ice/ice_common.h
> @@ -214,5 +214,9 @@ int
>   ice_aq_read_i2c(struct ice_hw *hw, struct ice_aqc_link_topo_addr topo_addr,
>   		u16 bus_addr, __le16 addr, u8 params, u8 *data,
>   		struct ice_sq_cd *cd);
> +int
> +ice_aq_write_i2c(struct ice_hw *hw, struct ice_aqc_link_topo_addr topo_addr,
> +		 u16 bus_addr, __le16 addr, u8 params, u8 *data,
> +		 struct ice_sq_cd *cd);
>   bool ice_fw_supports_report_dflt_cfg(struct ice_hw *hw);
>   #endif /* _ICE_COMMON_H_ */


Kind regards,

Paul

[Intel-wired-lan] [PATCH intel-next 3/3] ice: add write functionality for GNSS TTY

[Paul Menzel]

Dear Karol,


Thank you for the patch.

Am 15.04.22 um 12:31 schrieb Karol Kolacinski:
> Add the possibility to write raw bytes to the GNSS module through the
> first TTY device. This allow user to configure the module.

allow*s*

> 
> Create a second read-only TTY device.

Could you please give an example how to configure the module?

> Signed-off-by: Karol Kolacinski <karol.kolacinski@intel.com>
> ---
>   drivers/net/ethernet/intel/ice/ice.h      |   4 +-
>   drivers/net/ethernet/intel/ice/ice_gnss.c | 230 +++++++++++++++++++---
>   drivers/net/ethernet/intel/ice/ice_gnss.h |  23 ++-
>   3 files changed, 231 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice.h b/drivers/net/ethernet/intel/ice/ice.h
> index 17fb1ddea7b0..41bd0c7c7da5 100644
> --- a/drivers/net/ethernet/intel/ice/ice.h
> +++ b/drivers/net/ethernet/intel/ice/ice.h
> @@ -558,8 +558,8 @@ struct ice_pf {
>   	u32 msg_enable;
>   	struct ice_ptp ptp;
>   	struct tty_driver *ice_gnss_tty_driver;
> -	struct tty_port gnss_tty_port;
> -	struct gnss_serial *gnss_serial;
> +	struct tty_port *gnss_tty_port[ICE_GNSS_TTY_MINOR_DEVICES];
> +	struct gnss_serial *gnss_serial[ICE_GNSS_TTY_MINOR_DEVICES];
>   	u16 num_rdma_msix;		/* Total MSIX vectors for RDMA driver */
>   	u16 rdma_base_vector;
>   
> diff --git a/drivers/net/ethernet/intel/ice/ice_gnss.c b/drivers/net/ethernet/intel/ice/ice_gnss.c
> index f0b2091c3b5f..eafebd873236 100644
> --- a/drivers/net/ethernet/intel/ice/ice_gnss.c
> +++ b/drivers/net/ethernet/intel/ice/ice_gnss.c
> @@ -5,6 +5,99 @@
>   #include "ice_lib.h"
>   #include <linux/tty_driver.h>
>   
> +/**
> + * ice_gnss_do_write - Write data to internal GNSS
> + * @pf: board private structure
> + * @buf: command buffer
> + * @size: command buffer size
> + *
> + * Write UBX command data to the GNSS receiver
> + */
> +static void ice_gnss_do_write(struct ice_pf *pf, u8 *buf, u32 size)
> +{
> +	u32 i, num_writes, part_writes_num, last_write_bytes, offset = 0;

Can native types be used at least for the loop variable?

Write out `partial_writes_num`?

> +	struct ice_aqc_link_topo_addr link_topo;
> +	struct ice_hw *hw = &pf->hw;
> +	int err = 0;
> +
> +	memset(&link_topo, 0, sizeof(struct ice_aqc_link_topo_addr));
> +	link_topo.topo_params.index = ICE_E810T_GNSS_I2C_BUS;
> +	link_topo.topo_params.node_type_ctx |=
> +		ICE_AQC_LINK_TOPO_NODE_CTX_OVERRIDE <<
> +		ICE_AQC_LINK_TOPO_NODE_CTX_S;
> +
> +	/* Write all bytes except the last partial write */
> +	last_write_bytes = size % ICE_GNSS_UBX_WRITE_BYTES;
> +	if (last_write_bytes == 0)
> +		part_writes_num = 0;
> +	else if (last_write_bytes == 1)
> +		part_writes_num = 2;
> +	else
> +		part_writes_num = 1;

I do not fully understand the formula. But I am ignorant, so please 
ignore, if it?s obvious.

> +
> +	num_writes = size / ICE_GNSS_UBX_WRITE_BYTES - part_writes_num;
> +
> +	for (i = 0; i < num_writes; i++) {
> +		err = ice_aq_write_i2c(hw, link_topo, ICE_GNSS_UBX_I2C_BUS_ADDR,
> +				       cpu_to_le16(buf[offset]),
> +				       ICE_MAX_I2C_WRITE_BYTES,
> +				       &buf[offset + 1], NULL);
> +		if (err)
> +			goto err;
> +
> +		offset += ICE_GNSS_UBX_WRITE_BYTES;
> +	}
> +
> +	if (part_writes_num == 2) {
> +		/* We cannot write a single byte to ublox. Do 2 last writes
> +		 * instead of 1.
> +		 */
> +		err = ice_aq_write_i2c(hw, link_topo, ICE_GNSS_UBX_I2C_BUS_ADDR,
> +				       cpu_to_le16(buf[offset]),
> +				       ICE_MAX_I2C_WRITE_BYTES - 1,
> +				       &buf[offset + 1], NULL);
> +		if (err)
> +			goto err;
> +
> +		offset += ICE_GNSS_UBX_WRITE_BYTES - 1;
> +		last_write_bytes = 2;
> +	}
> +
> +	if (part_writes_num)

Please add a comment here, how you the logic works (that 
`part_writes_num == 2` is also effected).

> +		err = ice_aq_write_i2c(hw, link_topo, ICE_GNSS_UBX_I2C_BUS_ADDR,
> +				       cpu_to_le16(buf[offset]),
> +				       last_write_bytes - 1, &buf[offset + 1],
> +				       NULL);
> +

I wonder if some kind of while could model the algorithm better.

> +err:
> +	if (err)
> +		dev_err(ice_pf_to_dev(pf), "GNSS failed to write err=%d\n",
> +			err);

Please add the offset, and other values to the message.

> +}
> +
> +/**
> + * ice_gnss_write_pending - Write all pending data to internal GNSS
> + * @work: GNSS write work structure
> + */
> +static void ice_gnss_write_pending(struct kthread_work *work)
> +{
> +	struct gnss_serial *gnss = container_of(work, struct gnss_serial,
> +						write_work);
> +	struct ice_pf *pf = gnss->back;
> +
> +	if (!list_empty(&gnss->queue)) {
> +		struct gnss_write_buf *write_buf = NULL;
> +
> +		write_buf = list_first_entry(&gnss->queue,
> +					     struct gnss_write_buf, queue);
> +		list_del(&write_buf->queue);

Why does the queue need to be deleted here, and not after the write below?

> +
> +		ice_gnss_do_write(pf, write_buf->buf, write_buf->size);

Should `ice_gnss_do_write` return some success/failure code, or the 
number of writes?

> +		kfree(write_buf->buf);
> +		kfree(write_buf);
> +	}
> +}
> +
>   /**
>    * ice_gnss_read - Read data from internal GNSS module
>    * @work: GNSS read work structure
> @@ -103,8 +196,9 @@ static void ice_gnss_read(struct kthread_work *work)
>   /**
>    * ice_gnss_struct_init - Initialize GNSS structure for the TTY
>    * @pf: Board private structure
> + * @index: TTY device index
>    */
> -static struct gnss_serial *ice_gnss_struct_init(struct ice_pf *pf)
> +static struct gnss_serial *ice_gnss_struct_init(struct ice_pf *pf, int index)
>   {
>   	struct device *dev = ice_pf_to_dev(pf);
>   	struct kthread_worker *kworker;
> @@ -117,9 +211,11 @@ static struct gnss_serial *ice_gnss_struct_init(struct ice_pf *pf)
>   	mutex_init(&gnss->gnss_mutex);
>   	gnss->open_count = 0;
>   	gnss->back = pf;
> -	pf->gnss_serial = gnss;
> +	pf->gnss_serial[index] = gnss;
>   
>   	kthread_init_delayed_work(&gnss->read_work, ice_gnss_read);
> +	INIT_LIST_HEAD(&gnss->queue);
> +	kthread_init_work(&gnss->write_work, ice_gnss_write_pending);
>   	/* Allocate a kworker for handling work required for the GNSS TTY
>   	 * writes.
>   	 */
> @@ -155,10 +251,10 @@ static int ice_gnss_tty_open(struct tty_struct *tty, struct file *filp)
>   	tty->driver_data = NULL;
>   
>   	/* Get the serial object associated with this tty pointer */
> -	gnss = pf->gnss_serial;
> +	gnss = pf->gnss_serial[tty->index];
>   	if (!gnss) {
>   		/* Initialize GNSS struct on the first device open */
> -		gnss = ice_gnss_struct_init(pf);
> +		gnss = ice_gnss_struct_init(pf, tty->index);
>   		if (!gnss)
>   			return -ENOMEM;
>   	}
> @@ -211,25 +307,96 @@ static void ice_gnss_tty_close(struct tty_struct *tty, struct file *filp)
>   }
>   
>   /**
> - * ice_gnss_tty_write - Dummy TTY write function to avoid kernel panic
> + * ice_gnss_tty_write - Write GNSS data
>    * @tty: pointer to the tty_struct
>    * @buf: pointer to the user data
>    * @cnt: the number of characters that was able to be sent to the hardware (or

s/was/were/

>    *       queued to be sent at a later time)

But you are passing `cnt` in, and not set it in the implementation 
below, aren?t you?

> + *
> + * The write function call is called by the user when there is data to be sent
> + * to the hardware. First the tty core receives the call, and then it passes the
> + * data on to the tty driver?s write function. The tty core also tells the tty
> + * driver the size of the data being sent.
> + * If any errors happen during the write call, a negative error value should be
> + * returned instead of the number of characters that were written.
>    */
>   static int
>   ice_gnss_tty_write(struct tty_struct *tty, const unsigned char *buf, int cnt)

Should `cnt` be unsigned? Also, I?d use `count`.

>   {
> -	return 0;
> +	struct gnss_write_buf *write_buf;
> +	struct gnss_serial *gnss;
> +	struct ice_pf *pf;
> +	u8 *cmd_buf;
> +
> +	/* We cannot write a single byte using our I2C implementation. */
> +	if (cnt <= 1 || cnt > ICE_GNSS_TTY_WRITE_BUF)
> +		return -EINVAL;
> +
> +	gnss = tty->driver_data;
> +	if (!gnss)
> +		return -EFAULT;
> +
> +	pf = (struct ice_pf *)tty->driver->driver_state;
> +	if (!pf)
> +		return -EFAULT;
> +
> +	/* Allow write only on TTY 0 */

Only allow to write on TTY 0

> +	if (gnss != pf->gnss_serial[0])
> +		return -EIO;
> +
> +	mutex_lock(&gnss->gnss_mutex);
> +
> +	if (!gnss->open_count) {
> +		mutex_unlock(&gnss->gnss_mutex);
> +		return -EINVAL;
> +	}
> +
> +	cmd_buf = kzalloc(sizeof(*buf) * cnt, GFP_KERNEL);
> +	if (!cmd_buf)
> +		return -ENOMEM;

Does some unlocking need to happen here?

> +
> +	memcpy(cmd_buf, buf, cnt);
> +
> +	/* Send the data out to a hardware port */
> +	write_buf = kzalloc(sizeof(*write_buf), GFP_KERNEL);
> +	if (!write_buf)
> +		return -ENOMEM;

Ditto.

> +
> +	write_buf->buf = cmd_buf;
> +	write_buf->size = cnt;
> +	INIT_LIST_HEAD(&write_buf->queue);
> +	list_add_tail(&write_buf->queue, &gnss->queue);
> +	kthread_queue_work(gnss->kworker, &gnss->write_work);
> +	mutex_unlock(&gnss->gnss_mutex);
> +	return cnt;
>   }
>   
>   /**
> - * ice_gnss_tty_write_room - Dummy TTY write_room function to avoid kernel panic
> + * ice_gnss_tty_write_room - Returns the numbers of characters to be written.
>    * @tty: pointer to the tty_struct
> + *
> + * This routine returns the numbers of characters the tty driver will accept
> + * for queuing to be written. This number is subject to change as output buffers
> + * get emptied, or if the output flow control is acted.

What do you mean by ?is acted??

>    */
>   static unsigned int ice_gnss_tty_write_room(struct tty_struct *tty)
>   {
> -	return 0;
> +	struct gnss_serial *gnss = tty->driver_data;
> +	unsigned int room = 0;
> +
> +	/* Allow write only on TTY 0 */

Ditto.

> +	if (!gnss || gnss != gnss->back->gnss_serial[0])
> +		return room;

Do you need `room`? `return 0` here, and the macro in the end?

> +
> +	mutex_lock(&gnss->gnss_mutex);
> +
> +	if (!gnss->open_count)
> +		goto exit;
> +
> +	room = ICE_GNSS_TTY_WRITE_BUF;
> +exit:
> +	mutex_unlock(&gnss->gnss_mutex);
> +	return room;
>   }
>   
>   static const struct tty_operations tty_gps_ops = {
> @@ -250,10 +417,12 @@ static struct tty_driver *ice_gnss_create_tty_driver(struct ice_pf *pf)
>   	struct tty_driver *tty_driver;
>   	char *ttydrv_name;
>   	int err;
> +	u32 i;

Use native types?

>   
> -	tty_driver = tty_alloc_driver(1, TTY_DRIVER_REAL_RAW);
> +	tty_driver = tty_alloc_driver(ICE_GNSS_TTY_MINOR_DEVICES,
> +				      TTY_DRIVER_REAL_RAW);
>   	if (IS_ERR(tty_driver)) {
> -		dev_err(ice_pf_to_dev(pf), "Failed to allocate memory for GNSS TTY\n");
> +		dev_err(dev, "Failed to allocate memory for GNSS TTY\n");
>   		return NULL;
>   	}
>   
> @@ -283,23 +452,32 @@ static struct tty_driver *ice_gnss_create_tty_driver(struct ice_pf *pf)
>   	tty_driver->driver_state = pf;
>   	tty_set_operations(tty_driver, &tty_gps_ops);
>   
> -	pf->gnss_serial = NULL;
> +	for (i = 0; i < ICE_GNSS_TTY_MINOR_DEVICES; i++) {
> +		pf->gnss_tty_port[i] = kzalloc(sizeof(*pf->gnss_tty_port[i]),
> +					       GFP_KERNEL);
> +		pf->gnss_serial[i] = NULL;
>   
> -	tty_port_init(&pf->gnss_tty_port);
> -	tty_port_link_device(&pf->gnss_tty_port, tty_driver, 0);
> +		tty_port_init(pf->gnss_tty_port[i]);
> +		tty_port_link_device(pf->gnss_tty_port[i], tty_driver, i);
> +	}
>   
>   	err = tty_register_driver(tty_driver);
>   	if (err) {
> -		dev_err(ice_pf_to_dev(pf), "Failed to register TTY driver err=%d\n",
> -			err);
> +		dev_err(dev, "Failed to register TTY driver err=%d\n", err);
>   
> -		tty_port_destroy(&pf->gnss_tty_port);
> +		for (i = 0; i < ICE_GNSS_TTY_MINOR_DEVICES; i++) {
> +			tty_port_destroy(pf->gnss_tty_port[i]);
> +			kfree(pf->gnss_tty_port[i]);
> +		}
>   		kfree(ttydrv_name);
>   		tty_driver_kref_put(pf->ice_gnss_tty_driver);
>   
>   		return NULL;
>   	}
>   
> +	for (i = 0; i < ICE_GNSS_TTY_MINOR_DEVICES; i++)
> +		dev_info(dev, "%s%d registered\n", ttydrv_name, i);
> +
>   	return tty_driver;
>   }
>   
> @@ -327,17 +505,25 @@ void ice_gnss_init(struct ice_pf *pf)
>    */
>   void ice_gnss_exit(struct ice_pf *pf)
>   {
> +	u32 i;

Ditto.

> +
>   	if (!test_bit(ICE_FLAG_GNSS, pf->flags) || !pf->ice_gnss_tty_driver)
>   		return;
>   
> -	tty_port_destroy(&pf->gnss_tty_port);
> +	for (i = 0; i < ICE_GNSS_TTY_MINOR_DEVICES; i++) {
> +		if (pf->gnss_tty_port[i]) {
> +			tty_port_destroy(pf->gnss_tty_port[i]);
> +			kfree(pf->gnss_tty_port[i]);
> +		}
>   
> -	if (pf->gnss_serial) {
> -		struct gnss_serial *gnss = pf->gnss_serial;
> +		if (pf->gnss_serial[i]) {
> +			struct gnss_serial *gnss = pf->gnss_serial[i];
>   
> -		kthread_cancel_delayed_work_sync(&gnss->read_work);
> -		kfree(gnss);
> -		pf->gnss_serial = NULL;
> +			kthread_cancel_work_sync(&gnss->write_work);
> +			kthread_cancel_delayed_work_sync(&gnss->read_work);
> +			kfree(gnss);
> +			pf->gnss_serial[i] = NULL;
> +		}
>   	}
>   
>   	tty_unregister_driver(pf->ice_gnss_tty_driver);
> diff --git a/drivers/net/ethernet/intel/ice/ice_gnss.h b/drivers/net/ethernet/intel/ice/ice_gnss.h
> index 9211adb2372c..59ba5749143e 100644
> --- a/drivers/net/ethernet/intel/ice/ice_gnss.h
> +++ b/drivers/net/ethernet/intel/ice/ice_gnss.h
> @@ -8,14 +8,31 @@
>   #include <linux/tty_flip.h>
>   
>   #define ICE_E810T_GNSS_I2C_BUS		0x2
> +#define ICE_GNSS_TIMER_DELAY_TIME	(HZ / 10) /* 0.1 second per message */
> +#define ICE_GNSS_TTY_MINOR_DEVICES	2
> +#define ICE_GNSS_TTY_WRITE_BUF		250
> +#define ICE_MAX_I2C_DATA_SIZE		FIELD_MAX(ICE_AQC_I2C_DATA_SIZE_M)
> +#define ICE_MAX_I2C_WRITE_BYTES		4
> +
> +/* ublox specific deifinitions */
>   #define ICE_GNSS_UBX_I2C_BUS_ADDR	0x42
>   /* Data length register is big endian */
>   #define ICE_GNSS_UBX_DATA_LEN_H		0xFD
>   #define ICE_GNSS_UBX_DATA_LEN_WIDTH	2
>   #define ICE_GNSS_UBX_EMPTY_DATA		0xFF
> -#define ICE_GNSS_TIMER_DELAY_TIME	(HZ / 10) /* 0.1 second per message */
> -#define ICE_MAX_I2C_DATA_SIZE		FIELD_MAX(ICE_AQC_I2C_DATA_SIZE_M)
> +/* For ublox writes are performed without address so the first byte to write is
> + * passed as I2C addr parameter.
> + */
> +#define ICE_GNSS_UBX_WRITE_BYTES	(ICE_MAX_I2C_WRITE_BYTES + 1)
>   #define ICE_MAX_UBX_READ_TRIES		255
> +#define ICE_MAX_UBX_ACK_READ_TRIES	4095
> +
> +struct gnss_write_buf {
> +	struct list_head queue;
> +	u32 size;

Use native types.

> +	u8 *buf;
> +};
> +
>   
>   /**
>    * struct gnss_serial - data used to initialize GNSS TTY port
> @@ -33,6 +50,8 @@ struct gnss_serial {
>   	struct mutex gnss_mutex; /* protects GNSS serial structure */
>   	struct kthread_worker *kworker;
>   	struct kthread_delayed_work read_work;
> +	struct kthread_work write_work;
> +	struct list_head queue;
>   };
>   
>   #if IS_ENABLED(CONFIG_TTY)


Kind regards,

Paul

[Intel-wired-lan] [PATCH intel-next 1/3] ice: remove u16 arithmetic in ice_gnss

[Kolacinski, Karol]

Dear Paul,

Thank you for your review.

On 4/15/22 3:51 PM, Paul Menzel wrote:
>> Change u16 to u32 where arithmetic occured.
>
>occur*r*e*s*
>
>Why does 16-bit width not work?

In the previous GNSS patch review, David Laight wrote that I should not do
arithmetic on anything smaller than 'int'.

>
>> Signed-off-by: Karol Kolacinski <karol.kolacinski@intel.com>
>> ---
>>?? drivers/net/ethernet/intel/ice/ice_gnss.c | 6 +++---
>>?? 1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/intel/ice/ice_gnss.c b/drivers/net/ethernet/intel/ice/ice_gnss.c
>> index 57586a2e6dec..f0b2091c3b5f 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_gnss.c
>> +++ b/drivers/net/ethernet/intel/ice/ice_gnss.c
>> @@ -23,7 +23,7 @@ static void ice_gnss_read(struct kthread_work *work)
>>??????? struct ice_hw *hw;
>>??????? __be16 data_len_b;
>>??????? char *buf = NULL;
>> -???? u16 i, data_len;
>> +???? u32 i, data_len;
>
>Why does the length need to be specified at all, and no native types can
>be used?

I changed it to use native type. Length of the buffer is read from the device
and then used in a read loop.

Kind regards,
Karol

---------------------------------------------------------------------

Intel Technology Poland sp. z o.o.
ul. Slowackiego 173, 80-298 Gdansk
KRS 101882
NIP 957-07-52-316

[Intel-wired-lan] [PATCH intel-next 2/3] ice: add i2c write command

[Kolacinski, Karol]

Dear Paul,

Thank you for your review.

On 4/15/22 3:55 PM, Paul Menzel wrote:
>> Add the possibility to write to connected i2c devices. FW may reject
>> the write if the device is not on allowlist.
>
>Did you use a datasheet for implementing this. If so, please mention the 
>name and revision.

Unfortunately, the original author did not leave the datasheet.

>> +/**
>> + * ice_aq_write_i2c
>> + * @hw: pointer to the hw struct
>> + * @topo_addr: topology address for a device to communicate with
>> + * @bus_addr: 7-bit I2C bus address
>> + * @addr: I2C memory address (I2C offset) with up to 16 bits
>> + * @params: I2C parameters: bit [4] - I2C address type, bits [3:0] - data size to write (0-7 bytes)
>> + * @data: pointer to data (0 to 4 bytes) to be written to the I2C device
>> + * @cd: pointer to command details structure or NULL
>
>Also document the return value?

Done.

>> + *
>> + * Write I2C (0x06E3)
>> + */
>> +int
>> +ice_aq_write_i2c(struct ice_hw *hw, struct ice_aqc_link_topo_addr topo_addr,
>> +????????????? u16 bus_addr, __le16 addr, u8 params, u8 *data,
>> +????????????? struct ice_sq_cd *cd)
>> +{
>> +???? struct ice_aq_desc desc = { 0 };
>> +???? struct ice_aqc_i2c *cmd;
>> +???? u8 i, data_size;
>
>The loop variable should be a native type.

Done.

Kind regards,
Karol

---------------------------------------------------------------------

Intel Technology Poland sp. z o.o.
ul. Slowackiego 173, 80-298 Gdansk
KRS 101882
NIP 957-07-52-316

[Intel-wired-lan] [PATCH intel-next 3/3] ice: add write functionality for GNSS TTY

[Kolacinski, Karol]

Dear Paul,

Thank you for your review.

On 4/15/22 7:12 PM, Paul Menzel wrote:
>> Add the possibility to write raw bytes to the GNSS module through the
>> first TTY device. This allow user to configure the module.
>
>allow*s*

Done.

>>
>> Create a second read-only TTY device.
>
>Could you please give an example how to configure the module?

The module is configured using the u-blox UBX protocol, I updated the commit message to
reflect this.

>> Signed-off-by: Karol Kolacinski <karol.kolacinski@intel.com>
>> ---
>>   drivers/net/ethernet/intel/ice/ice.h      |   4 +-
>>   drivers/net/ethernet/intel/ice/ice_gnss.c | 230 +++++++++++++++++++---
>>   drivers/net/ethernet/intel/ice/ice_gnss.h |  23 ++-
>>   3 files changed, 231 insertions(+), 26 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/intel/ice/ice.h b/drivers/net/ethernet/intel/ice/ice.h
>> index 17fb1ddea7b0..41bd0c7c7da5 100644
>> --- a/drivers/net/ethernet/intel/ice/ice.h
>> +++ b/drivers/net/ethernet/intel/ice/ice.h
>> @@ -558,8 +558,8 @@ struct ice_pf {
>>        u32 msg_enable;
>>        struct ice_ptp ptp;
>>        struct tty_driver *ice_gnss_tty_driver;
>> -     struct tty_port gnss_tty_port;
>> -     struct gnss_serial *gnss_serial;
>> +     struct tty_port *gnss_tty_port[ICE_GNSS_TTY_MINOR_DEVICES];
>> +     struct gnss_serial *gnss_serial[ICE_GNSS_TTY_MINOR_DEVICES];
>>        u16 num_rdma_msix;              /* Total MSIX vectors for RDMA driver */
>>        u16 rdma_base_vector;
>>  
>> diff --git a/drivers/net/ethernet/intel/ice/ice_gnss.c b/drivers/net/ethernet/intel/ice/ice_gnss.c
>> index f0b2091c3b5f..eafebd873236 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_gnss.c
>> +++ b/drivers/net/ethernet/intel/ice/ice_gnss.c
>> @@ -5,6 +5,99 @@
>>   #include "ice_lib.h"
>>   #include <linux/tty_driver.h>
>>  
>> +/**
>> + * ice_gnss_do_write - Write data to internal GNSS
>> + * @pf: board private structure
>> + * @buf: command buffer
>> + * @size: command buffer size
>> + *
>> + * Write UBX command data to the GNSS receiver
>> + */
>> +static void ice_gnss_do_write(struct ice_pf *pf, u8 *buf, u32 size)
>> +{
>> +     u32 i, num_writes, part_writes_num, last_write_bytes, offset = 0;
>
>Can native types be used at least for the loop variable?
>
>Write out `partial_writes_num`?

Done.

>> +     struct ice_aqc_link_topo_addr link_topo;
>> +     struct ice_hw *hw = &pf->hw;
>> +     int err = 0;
>> +
>> +     memset(&link_topo, 0, sizeof(struct ice_aqc_link_topo_addr));
>> +     link_topo.topo_params.index = ICE_E810T_GNSS_I2C_BUS;
>> +     link_topo.topo_params.node_type_ctx |=
>> +             ICE_AQC_LINK_TOPO_NODE_CTX_OVERRIDE <<
>> +             ICE_AQC_LINK_TOPO_NODE_CTX_S;
>> +
>> +     /* Write all bytes except the last partial write */
>> +     last_write_bytes = size % ICE_GNSS_UBX_WRITE_BYTES;
>> +     if (last_write_bytes == 0)
>> +             part_writes_num = 0;
>> +     else if (last_write_bytes == 1)
>> +             part_writes_num = 2;
>> +     else
>> +             part_writes_num = 1;
>
>I do not fully understand the formula. But I am ignorant, so please
>ignore, if it?s obvious.

I changed the formula. Basically, I wanted to split the bytes between the last 2
writes if the last one would be only a byte.

>> +
>> +     num_writes = size / ICE_GNSS_UBX_WRITE_BYTES - part_writes_num;
>> +
>> +     for (i = 0; i < num_writes; i++) {
>> +             err = ice_aq_write_i2c(hw, link_topo, ICE_GNSS_UBX_I2C_BUS_ADDR,
>> +                                    cpu_to_le16(buf[offset]),
>> +                                    ICE_MAX_I2C_WRITE_BYTES,
>> +                                    &buf[offset + 1], NULL);
>> +             if (err)
>> +                     goto err;
>> +
>> +             offset += ICE_GNSS_UBX_WRITE_BYTES;
>> +     }
>> +
>> +     if (part_writes_num == 2) {
>> +             /* We cannot write a single byte to ublox. Do 2 last writes
>> +              * instead of 1.
>> +              */
>> +             err = ice_aq_write_i2c(hw, link_topo, ICE_GNSS_UBX_I2C_BUS_ADDR,
>> +                                    cpu_to_le16(buf[offset]),
>> +                                    ICE_MAX_I2C_WRITE_BYTES - 1,
>> +                                    &buf[offset + 1], NULL);
>> +             if (err)
>> +                     goto err;
>> +
>> +             offset += ICE_GNSS_UBX_WRITE_BYTES - 1;
>> +             last_write_bytes = 2;
>> +     }
>> +
>> +     if (part_writes_num)
>
>Please add a comment here, how you the logic works (that
>`part_writes_num == 2` is also effected).

Done.

>> +             err = ice_aq_write_i2c(hw, link_topo, ICE_GNSS_UBX_I2C_BUS_ADDR,
>> +                                    cpu_to_le16(buf[offset]),
>> +                                    last_write_bytes - 1, &buf[offset + 1],
>> +                                    NULL);
>> +
>
>I wonder if some kind of while could model the algorithm better.

Done.

>> +err:
>> +     if (err)
>> +             dev_err(ice_pf_to_dev(pf), "GNSS failed to write err=%d\n",
>> +                     err);
>
>Please add the offset, and other values to the message.

Done.

>> +}
>> +
>> +/**
>> + * ice_gnss_write_pending - Write all pending data to internal GNSS
>> + * @work: GNSS write work structure
>> + */
>> +static void ice_gnss_write_pending(struct kthread_work *work)
>> +{
>> +     struct gnss_serial *gnss = container_of(work, struct gnss_serial,
>> +                                             write_work);
>> +     struct ice_pf *pf = gnss->back;
>> +
>> +     if (!list_empty(&gnss->queue)) {
>> +             struct gnss_write_buf *write_buf = NULL;
>> +
>> +             write_buf = list_first_entry(&gnss->queue,
>> +                                          struct gnss_write_buf, queue);
>> +             list_del(&write_buf->queue);
>
>Why does the queue need to be deleted here, and not after the write below?

Done.

>> +
>> +             ice_gnss_do_write(pf, write_buf->buf, write_buf->size);
>
>Should `ice_gnss_do_write` return some success/failure code, or the
>number of writes?

Done.

>> +             kfree(write_buf->buf);
>> +             kfree(write_buf);
>> +     }
>> +}
>> +
>>   /**
>>    * ice_gnss_read - Read data from internal GNSS module
>>    * @work: GNSS read work structure
>> @@ -103,8 +196,9 @@ static void ice_gnss_read(struct kthread_work *work)
>>   /**
>>    * ice_gnss_struct_init - Initialize GNSS structure for the TTY
>>    * @pf: Board private structure
>> + * @index: TTY device index
>>    */
>> -static struct gnss_serial *ice_gnss_struct_init(struct ice_pf *pf)
>> +static struct gnss_serial *ice_gnss_struct_init(struct ice_pf *pf, int index)
>>   {
>>        struct device *dev = ice_pf_to_dev(pf);
>>        struct kthread_worker *kworker;
>> @@ -117,9 +211,11 @@ static struct gnss_serial *ice_gnss_struct_init(struct ice_pf *pf)
>>        mutex_init(&gnss->gnss_mutex);
>>        gnss->open_count = 0;
>>        gnss->back = pf;
>> -     pf->gnss_serial = gnss;
>> +     pf->gnss_serial[index] = gnss;
>>  
>>        kthread_init_delayed_work(&gnss->read_work, ice_gnss_read);
>> +     INIT_LIST_HEAD(&gnss->queue);
>> +     kthread_init_work(&gnss->write_work, ice_gnss_write_pending);
>>        /* Allocate a kworker for handling work required for the GNSS TTY
>>         * writes.
>>         */
>> @@ -155,10 +251,10 @@ static int ice_gnss_tty_open(struct tty_struct *tty, struct file *filp)
>>        tty->driver_data = NULL;
>>  
>>        /* Get the serial object associated with this tty pointer */
>> -     gnss = pf->gnss_serial;
>> +     gnss = pf->gnss_serial[tty->index];
>>        if (!gnss) {
>>                /* Initialize GNSS struct on the first device open */
>> -             gnss = ice_gnss_struct_init(pf);
>> +             gnss = ice_gnss_struct_init(pf, tty->index);
>>                if (!gnss)
>>                        return -ENOMEM;
>>        }
>> @@ -211,25 +307,96 @@ static void ice_gnss_tty_close(struct tty_struct *tty, struct file *filp)
>>   }
>>  
>>   /**
>> - * ice_gnss_tty_write - Dummy TTY write function to avoid kernel panic
>> + * ice_gnss_tty_write - Write GNSS data
>>    * @tty: pointer to the tty_struct
>>    * @buf: pointer to the user data
>>    * @cnt: the number of characters that was able to be sent to the hardware (or
>
>s/was/were/

Done.

>>    *       queued to be sent at a later time)
>
>But you are passing `cnt` in, and not set it in the implementation
>below, aren?t you?

Done.

>> + *
>> + * The write function call is called by the user when there is data to be sent
>> + * to the hardware. First the tty core receives the call, and then it passes the
>> + * data on to the tty driver?s write function. The tty core also tells the tty
>> + * driver the size of the data being sent.
>> + * If any errors happen during the write call, a negative error value should be
>> + * returned instead of the number of characters that were written.
>>    */
>>   static int
>>   ice_gnss_tty_write(struct tty_struct *tty, const unsigned char *buf, int cnt)
>
>Should `cnt` be unsigned? Also, I?d use `count`.

Changed to 'count'. I cannot use unsigned here as it's defined by the kernel
interface.

>>   {
>> -     return 0;
>> +     struct gnss_write_buf *write_buf;
>> +     struct gnss_serial *gnss;
>> +     struct ice_pf *pf;
>> +     u8 *cmd_buf;
>> +
>> +     /* We cannot write a single byte using our I2C implementation. */
>> +     if (cnt <= 1 || cnt > ICE_GNSS_TTY_WRITE_BUF)
>> +             return -EINVAL;
>> +
>> +     gnss = tty->driver_data;
>> +     if (!gnss)
>> +             return -EFAULT;
>> +
>> +     pf = (struct ice_pf *)tty->driver->driver_state;
>> +     if (!pf)
>> +             return -EFAULT;
>> +
>> +     /* Allow write only on TTY 0 */
>
>Only allow to write on TTY 0

Done.

>> +     if (gnss != pf->gnss_serial[0])
>> +             return -EIO;
>> +
>> +     mutex_lock(&gnss->gnss_mutex);
>> +
>> +     if (!gnss->open_count) {
>> +             mutex_unlock(&gnss->gnss_mutex);
>> +             return -EINVAL;
>> +     }
>> +
>> +     cmd_buf = kzalloc(sizeof(*buf) * cnt, GFP_KERNEL);
>> +     if (!cmd_buf)
>> +             return -ENOMEM;
>
>Does some unlocking need to happen here?

Done.

>> +
>> +     memcpy(cmd_buf, buf, cnt);
>> +
>> +     /* Send the data out to a hardware port */
>> +     write_buf = kzalloc(sizeof(*write_buf), GFP_KERNEL);
>> +     if (!write_buf)
>> +             return -ENOMEM;
>
>Ditto.

Done.

>> +
>> +     write_buf->buf = cmd_buf;
>> +     write_buf->size = cnt;
>> +     INIT_LIST_HEAD(&write_buf->queue);
>> +     list_add_tail(&write_buf->queue, &gnss->queue);
>> +     kthread_queue_work(gnss->kworker, &gnss->write_work);
>> +     mutex_unlock(&gnss->gnss_mutex);
>> +     return cnt;
>>   }
>>  
>>   /**
>> - * ice_gnss_tty_write_room - Dummy TTY write_room function to avoid kernel panic
>> + * ice_gnss_tty_write_room - Returns the numbers of characters to be written.
>>    * @tty: pointer to the tty_struct
>> + *
>> + * This routine returns the numbers of characters the tty driver will accept
>> + * for queuing to be written. This number is subject to change as output buffers
>> + * get emptied, or if the output flow control is acted.
>
>What do you mean by ?is acted??

Rephrased the description.

>>    */
>>   static unsigned int ice_gnss_tty_write_room(struct tty_struct *tty)
>>   {
>> -     return 0;
>> +     struct gnss_serial *gnss = tty->driver_data;
>> +     unsigned int room = 0;
>> +
>> +     /* Allow write only on TTY 0 */
>
>Ditto.

Done.

>> +     if (!gnss || gnss != gnss->back->gnss_serial[0])
>> +             return room;
>
>Do you need `room`? `return 0` here, and the macro in the end?

Done.

>> +
>> +     mutex_lock(&gnss->gnss_mutex);
>> +
>> +     if (!gnss->open_count)
>> +             goto exit;
>> +
>> +     room = ICE_GNSS_TTY_WRITE_BUF;
>> +exit:
>> +     mutex_unlock(&gnss->gnss_mutex);
>> +     return room;
>>   }
>>  
>>   static const struct tty_operations tty_gps_ops = {
>> @@ -250,10 +417,12 @@ static struct tty_driver *ice_gnss_create_tty_driver(struct ice_pf *pf)
>>        struct tty_driver *tty_driver;
>>        char *ttydrv_name;
>>        int err;
>> +     u32 i;
>
>Use native types?

Done.

>>  
>> -     tty_driver = tty_alloc_driver(1, TTY_DRIVER_REAL_RAW);
>> +     tty_driver = tty_alloc_driver(ICE_GNSS_TTY_MINOR_DEVICES,
>> +                                   TTY_DRIVER_REAL_RAW);
>>        if (IS_ERR(tty_driver)) {
>> -             dev_err(ice_pf_to_dev(pf), "Failed to allocate memory for GNSS TTY\n");
>> +             dev_err(dev, "Failed to allocate memory for GNSS TTY\n");
>>                return NULL;
>>        }
>>  
>> @@ -283,23 +452,32 @@ static struct tty_driver *ice_gnss_create_tty_driver(struct ice_pf *pf)
>>        tty_driver->driver_state = pf;
>>        tty_set_operations(tty_driver, &tty_gps_ops);
>>  
>> -     pf->gnss_serial = NULL;
>> +     for (i = 0; i < ICE_GNSS_TTY_MINOR_DEVICES; i++) {
>> +             pf->gnss_tty_port[i] = kzalloc(sizeof(*pf->gnss_tty_port[i]),
>> +                                            GFP_KERNEL);
>> +             pf->gnss_serial[i] = NULL;
>>  
>> -     tty_port_init(&pf->gnss_tty_port);
>> -     tty_port_link_device(&pf->gnss_tty_port, tty_driver, 0);
>> +             tty_port_init(pf->gnss_tty_port[i]);
>> +             tty_port_link_device(pf->gnss_tty_port[i], tty_driver, i);
>> +     }
>>  
>>        err = tty_register_driver(tty_driver);
>>        if (err) {
>> -             dev_err(ice_pf_to_dev(pf), "Failed to register TTY driver err=%d\n",
>> -                     err);
>> +             dev_err(dev, "Failed to register TTY driver err=%d\n", err);
>>  
>> -             tty_port_destroy(&pf->gnss_tty_port);
>> +             for (i = 0; i < ICE_GNSS_TTY_MINOR_DEVICES; i++) {
>> +                     tty_port_destroy(pf->gnss_tty_port[i]);
>> +                     kfree(pf->gnss_tty_port[i]);
>> +             }
>>                kfree(ttydrv_name);
>>                tty_driver_kref_put(pf->ice_gnss_tty_driver);
>>  
>>                return NULL;
>>        }
>>  
>> +     for (i = 0; i < ICE_GNSS_TTY_MINOR_DEVICES; i++)
>> +             dev_info(dev, "%s%d registered\n", ttydrv_name, i);
>> +
>>        return tty_driver;
>>   }
>>  
>> @@ -327,17 +505,25 @@ void ice_gnss_init(struct ice_pf *pf)
>>    */
>>   void ice_gnss_exit(struct ice_pf *pf)
>>   {
>> +     u32 i;
>
>Ditto.

Done.

>> +
>>        if (!test_bit(ICE_FLAG_GNSS, pf->flags) || !pf->ice_gnss_tty_driver)
>>                return;
>>  
>> -     tty_port_destroy(&pf->gnss_tty_port);
>> +     for (i = 0; i < ICE_GNSS_TTY_MINOR_DEVICES; i++) {
>> +             if (pf->gnss_tty_port[i]) {
>> +                     tty_port_destroy(pf->gnss_tty_port[i]);
>> +                     kfree(pf->gnss_tty_port[i]);
>> +             }
>>  
>> -     if (pf->gnss_serial) {
>> -             struct gnss_serial *gnss = pf->gnss_serial;
>> +             if (pf->gnss_serial[i]) {
>> +                     struct gnss_serial *gnss = pf->gnss_serial[i];
>>  
>> -             kthread_cancel_delayed_work_sync(&gnss->read_work);
>> -             kfree(gnss);
>> -             pf->gnss_serial = NULL;
>> +                     kthread_cancel_work_sync(&gnss->write_work);
>> +                     kthread_cancel_delayed_work_sync(&gnss->read_work);
>> +                     kfree(gnss);
>> +                     pf->gnss_serial[i] = NULL;
>> +             }
>>        }
>>  
>>        tty_unregister_driver(pf->ice_gnss_tty_driver);
>> diff --git a/drivers/net/ethernet/intel/ice/ice_gnss.h b/drivers/net/ethernet/intel/ice/ice_gnss.h
>> index 9211adb2372c..59ba5749143e 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_gnss.h
>> +++ b/drivers/net/ethernet/intel/ice/ice_gnss.h
>> @@ -8,14 +8,31 @@
>>   #include <linux/tty_flip.h>
>>  
>>   #define ICE_E810T_GNSS_I2C_BUS              0x2
>> +#define ICE_GNSS_TIMER_DELAY_TIME    (HZ / 10) /* 0.1 second per message */
>> +#define ICE_GNSS_TTY_MINOR_DEVICES   2
>> +#define ICE_GNSS_TTY_WRITE_BUF               250
>> +#define ICE_MAX_I2C_DATA_SIZE                FIELD_MAX(ICE_AQC_I2C_DATA_SIZE_M)
>> +#define ICE_MAX_I2C_WRITE_BYTES              4
>> +
>> +/* ublox specific deifinitions */
>>   #define ICE_GNSS_UBX_I2C_BUS_ADDR   0x42
>>   /* Data length register is big endian */
>>   #define ICE_GNSS_UBX_DATA_LEN_H             0xFD
>>   #define ICE_GNSS_UBX_DATA_LEN_WIDTH 2
>>   #define ICE_GNSS_UBX_EMPTY_DATA             0xFF
>> -#define ICE_GNSS_TIMER_DELAY_TIME    (HZ / 10) /* 0.1 second per message */
>> -#define ICE_MAX_I2C_DATA_SIZE                FIELD_MAX(ICE_AQC_I2C_DATA_SIZE_M)
>> +/* For ublox writes are performed without address so the first byte to write is
>> + * passed as I2C addr parameter.
>> + */
>> +#define ICE_GNSS_UBX_WRITE_BYTES     (ICE_MAX_I2C_WRITE_BYTES + 1)
>>   #define ICE_MAX_UBX_READ_TRIES              255
>> +#define ICE_MAX_UBX_ACK_READ_TRIES   4095
>> +
>> +struct gnss_write_buf {
>> +     struct list_head queue;
>> +     u32 size;
>
>Use native types.

Done.

Kind regards,
Karol

---------------------------------------------------------------------

Intel Technology Poland sp. z o.o.
ul. Slowackiego 173, 80-298 Gdansk
KRS 101882
NIP 957-07-52-316