Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 25 Feb 2022 13:47:09 +0200 (EET)
From:   =?ISO-8859-15?Q?Ilpo_J=E4rvinen?= <ilpo.jarvinen@linux.intel.com>
To:     Ricardo Martinez <ricardo.martinez@linux.intel.com>
cc:     Netdev <netdev@vger.kernel.org>, linux-wireless@vger.kernel.org,
        kuba@kernel.org, davem@davemloft.net, johannes@sipsolutions.net,
        ryazanov.s.a@gmail.com, loic.poulain@linaro.org,
        m.chetan.kumar@intel.com, chandrashekar.devegowda@intel.com,
        linuxwwan@intel.com, chiranjeevi.rapolu@linux.intel.com,
        haijun.liu@mediatek.com, amir.hanania@intel.com,
        Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
        dinesh.sharma@intel.com, eliot.lee@intel.com,
        moises.veleta@intel.com, pierre-louis.bossart@intel.com,
        muralidharan.sethuraman@intel.com, Soumya.Prakash.Mishra@intel.com,
        sreehari.kancharla@intel.com, madhusmita.sahu@intel.com
Subject: Re: [PATCH net-next v5 04/13] net: wwan: t7xx: Add port proxy
 infrastructure
In-Reply-To: <20220223223326.28021-5-ricardo.martinez@linux.intel.com>
Message-ID: <b7b7c5d1-e6c8-4c74-d5aa-b12443d5aee@linux.intel.com>
References: <20220223223326.28021-1-ricardo.martinez@linux.intel.com> <20220223223326.28021-5-ricardo.martinez@linux.intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk

On Wed, 23 Feb 2022, Ricardo Martinez wrote:

> From: Haijun Liu <haijun.liu@mediatek.com>
> 
> Port-proxy provides a common interface to interact with different types
> of ports. Ports export their configuration via `struct t7xx_port` and
> operate as defined by `struct port_ops`.
> 
> Signed-off-by: Haijun Liu <haijun.liu@mediatek.com>
> Co-developed-by: Chandrashekar Devegowda <chandrashekar.devegowda@intel.com>
> Signed-off-by: Chandrashekar Devegowda <chandrashekar.devegowda@intel.com>
> Co-developed-by: Ricardo Martinez <ricardo.martinez@linux.intel.com>
> Signed-off-by: Ricardo Martinez <ricardo.martinez@linux.intel.com>
> 
> >From a WWAN framework perspective:
> Reviewed-by: Loic Poulain <loic.poulain@linaro.org>
> ---


> +static u16 t7xx_port_next_rx_seq_num(struct t7xx_port *port, struct ccci_header *ccci_h)
> +{
> +	u16 seq_num, next_seq_num, assert_bit;

assert_bit could be bool.

> +
> +	seq_num = FIELD_GET(CCCI_H_SEQ_FLD, le32_to_cpu(ccci_h->status));
> +	next_seq_num = (seq_num + 1) & FIELD_MAX(CCCI_H_SEQ_FLD);
> +	assert_bit = !!(le32_to_cpu(ccci_h->status) & CCCI_H_AST_BIT);
> +	if (!assert_bit || port->seq_nums[MTK_RX] > FIELD_MAX(CCCI_H_SEQ_FLD))

Is this an obfuscated way to say:
	... || port->seq_nums[MTK_RX] == INVALID_SEQ_NUM
?

> +int t7xx_port_proxy_node_control(struct t7xx_modem *md, struct port_msg *port_msg)
> +{
> +	u32 *port_info_base = (void *)port_msg + sizeof(*port_msg);

__le32 *port_info_base = (void *)port_msg + sizeof(*port_msg);

As port_msg has __le32 fields, the endianess likely should disappear in 
this casting?

> +	struct device *dev = &md->t7xx_dev->pdev->dev;
> +	unsigned int version, ports, i;
> +
> +	version = FIELD_GET(PORT_MSG_VERSION, le32_to_cpu(port_msg->info));
> +	if (version != PORT_ENUM_VER ||
> +	    le32_to_cpu(port_msg->head_pattern) != PORT_ENUM_HEAD_PATTERN ||
> +	    le32_to_cpu(port_msg->tail_pattern) != PORT_ENUM_TAIL_PATTERN) {
> +		dev_err(dev, "Invalid port control message %x:%x:%x\n",
> +			version, le32_to_cpu(port_msg->head_pattern),
> +			le32_to_cpu(port_msg->tail_pattern));
> +		return -EFAULT;
> +	}
> +
> +	ports = FIELD_GET(PORT_MSG_PRT_CNT, le32_to_cpu(port_msg->info));
> +	for (i = 0; i < ports; i++) {
> +		struct t7xx_port_static *port_static;
> +		u32 *port_info = port_info_base + i;

__le32 *port_info = port_info_base + i;

> +		struct t7xx_port *port;
> +		unsigned int ch_id;
> +		bool en_flag;
> +
> +		ch_id = FIELD_GET(PORT_INFO_CH_ID, *port_info);

ch_id = FIELD_GET(PORT_INFO_CH_ID, le32_to_cpu(*port_info));

> +		port = t7xx_proxy_get_port_by_ch(md->port_prox, ch_id);
> +		if (!port) {
> +			dev_dbg(dev, "Port:%x not found\n", ch_id);
> +			continue;
> +		}
> +
> +		en_flag = !!(PORT_INFO_ENFLG & *port_info);

*port_info & PORT_INFO_ENFLG


-- 
 i.