Message-ID: <94a1ea50-8c83-aa65-6905-d8e6aacf3d65@linux.intel.com>
Date:   Thu, 18 Nov 2021 22:41:10 -0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.3.1
Subject: Re: [PATCH v2 04/14] net: wwan: t7xx: Add port proxy infrastructure
Content-Language: en-US
To:     Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc:     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,
        dinesh.sharma@intel.com, eliot.lee@intel.com,
        mika.westerberg@linux.intel.com, moises.veleta@intel.com,
        pierre-louis.bossart@intel.com, muralidharan.sethuraman@intel.com,
        Soumya.Prakash.Mishra@intel.com, sreehari.kancharla@intel.com,
        suresh.nagaraj@intel.com
References: <20211101035635.26999-1-ricardo.martinez@linux.intel.com>
 <20211101035635.26999-5-ricardo.martinez@linux.intel.com>
 <YYKs+DHYRHYFEYEN@smile.fi.intel.com>
From:   "Martinez, Ricardo" <ricardo.martinez@linux.intel.com>
In-Reply-To: <YYKs+DHYRHYFEYEN@smile.fi.intel.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 11/3/2021 8:38 AM, Andy Shevchenko wrote:
> On Sun, Oct 31, 2021 at 08:56:25PM -0700, Ricardo Martinez wrote:
>> From: Haijun Lio <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`.
> Same here, assuming that the comments from the previous patches are applied
> here as well, only unique are given.

Thanks for the feedback.

...

>> +	nlh = nlmsg_put(nl_skb, 0, 1, NLMSG_DONE, len, 0);
>> +	if (!nlh) {
>> +		dev_err(port->dev, "could not release netlink\n");
> I'm wondering why you are not using net_err() / netdev_err() / netif_err()
> where it's appropriate.

The original idea was to avoid mixing different types of APIs, but yes, 
it makes sense to use

those APIs where it's appropriate, I'm thinking on using them at 
t7xx_netdev.c where the

required net_device is available.

...

>> +		nlmsg_free(nl_skb);
>> +		return -EFAULT;
>> +	}
>>
>> ...
>>
>> +	ccci_h = (struct ccci_header *)skb->data;
> Do you need casting?
Yes, skb->data is an unsigned char*
> ...
...