Re: [PATCH net-next 3/3] ice: add write functionality for GNSS TTY

[Paolo Abeni <pabeni@redhat.com>]

On Tue, 2022-05-17 at 14:19 -0700, Tony Nguyen wrote:
> From: Karol Kolacinski <karol.kolacinski@intel.com>
> 
> Add the possibility to write raw bytes to the GNSS module through the
> first TTY device. This allows user to configure the module using the
> publicly available u-blox UBX protocol (version 29) commands.
> 
> Create a second read-only TTY device.
> 
> Signed-off-by: Karol Kolacinski <karol.kolacinski@intel.com>
> Tested-by: Gurucharan <gurucharanx.g@intel.com> (A Contingent worker at Intel)
> Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
> ---
>  drivers/net/ethernet/intel/ice/ice.h      |   4 +-
>  drivers/net/ethernet/intel/ice/ice_gnss.c | 241 +++++++++++++++++++---
>  drivers/net/ethernet/intel/ice/ice_gnss.h |  26 ++-
>  3 files changed, 240 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice.h b/drivers/net/ethernet/intel/ice/ice.h
> index 60453b3b8d23..5c472ed99c7a 100644
> --- a/drivers/net/ethernet/intel/ice/ice.h
> +++ b/drivers/net/ethernet/intel/ice/ice.h
> @@ -544,8 +544,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 c6d755f707aa..4b7b762cd787 100644
> --- a/drivers/net/ethernet/intel/ice/ice_gnss.c
> +++ b/drivers/net/ethernet/intel/ice/ice_gnss.c
> @@ -1,10 +1,102 @@
>  // SPDX-License-Identifier: GPL-2.0
> -/* Copyright (C) 2018-2021, Intel Corporation. */
> +/* Copyright (C) 2021-2022, Intel Corporation. */
>  
>  #include "ice.h"
>  #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 unsigned int
> +ice_gnss_do_write(struct ice_pf *pf, unsigned char *buf, unsigned int size)
> +{
> +	struct ice_aqc_link_topo_addr link_topo;
> +	struct ice_hw *hw = &pf->hw;
> +	unsigned int offset = 0;
> +	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 |=
> +		FIELD_PREP(ICE_AQC_LINK_TOPO_NODE_CTX_M,
> +			   ICE_AQC_LINK_TOPO_NODE_CTX_OVERRIDE);
> +
> +	/* It's not possible to write a single byte to u-blox.
> +	 * Write all bytes in a loop until there are 6 or less bytes left. If
> +	 * there are exactly 6 bytes left, the last write would be only a byte.
> +	 * In this case, do 4+2 bytes writes instead of 5+1. Otherwise, do the
> +	 * last 2 to 5 bytes write.
> +	 */
> +	while (size - offset > ICE_GNSS_UBX_WRITE_BYTES + 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,
> +				       &buf[offset + 1], NULL);
> +		if (err)
> +			goto exit;
> +
> +		offset += ICE_GNSS_UBX_WRITE_BYTES;
> +	}
> +
> +	/* Single byte would be written. Write 4 bytes instead of 5. */
> +	if (size - offset == ICE_GNSS_UBX_WRITE_BYTES + 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 exit;
> +
> +		offset += ICE_GNSS_UBX_WRITE_BYTES - 1;
> +	}
> +
> +	/* Do the last write, 2 to 5 bytes. */
> +	err = ice_aq_write_i2c(hw, link_topo, ICE_GNSS_UBX_I2C_BUS_ADDR,
> +			       cpu_to_le16(buf[offset]), size - offset - 1,
> +			       &buf[offset + 1], NULL);
> +	if (!err)
> +		offset = size;
> +
> +exit:
> +	if (err)
> +		dev_err(ice_pf_to_dev(pf), "GNSS failed to write, offset=%u, size=%u, err=%d\n",
> +			offset, size, err);
> +
> +	return offset;


IMHO more readable with:
	if (err)
		goto err_out;

	return size;

err_out:
	dev_err(ice_pf_to_dev(pf), "GNSS failed to write, offset=%u, size=%u, err=%d\n",
			offset, size, err);
	return offset;

(plus adjusting the goto above)

Thanks!

Paolo