Date:   Tue, 28 Feb 2023 14:41:14 +0800
From:   "Ye, Xiang" <xiang.ye@intel.com>
To:     Mark Brown <broonie@kernel.org>
CC:     Lee Jones <lee@kernel.org>, Wolfram Sang <wsa@kernel.org>,
        Tyrone Ting <kfting@nuvoton.com>,
        Linus Walleij <linus.walleij@linaro.org>,
        "Bartosz Golaszewski" <brgl@bgdev.pl>,
        "linux-i2c@vger.kernel.org" <linux-i2c@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "linux-spi@vger.kernel.org" <linux-spi@vger.kernel.org>,
        "linux-gpio@vger.kernel.org" <linux-gpio@vger.kernel.org>,
        "Pandruvada, Srinivas" <srinivas.pandruvada@intel.com>,
        "heikki.krogerus@linux.intel.com" <heikki.krogerus@linux.intel.com>,
        "andriy.shevchenko@linux.intel.com" 
        <andriy.shevchenko@linux.intel.com>,
        "sakari.ailus@linux.intel.com" <sakari.ailus@linux.intel.com>,
        "Wang, Zhifeng" <zhifeng.wang@intel.com>,
        "Wu, Wentong" <Wentong.Wu@intel.com>,
        "Zhang, Lixu" <Lixu.Zhang@intel.com>
Subject: Re: [PATCH v2 4/5] spi: Add support for Intel LJCA USB SPI driver
Message-ID: <Y/2iCn6M/+HWsjZD@ye-NUC7i7DNHE>
References: <20230225140118.2037220-1-xiang.ye@intel.com>
 <20230225140118.2037220-5-xiang.ye@intel.com>
 <Y/zm/Uk/d6VRxLBx@sirena.org.uk>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <Y/zm/Uk/d6VRxLBx@sirena.org.uk>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?mvKnwpm2U1Gn/k6YtkR0Le+w9+AWF1IyzO2rAy+/ZhDfKilNLLdbK9oILgWp?=
 =?us-ascii?Q?utCVJpEWA7Jp4JFHTxIIuDJ6UMqGdAwrhw4gZY0isvioHGJPSR57nC0V0WPM?=
 =?us-ascii?Q?JeKbaUCzCk7/3zef9v9bd1qQHHjtFrvVbBPH84bo9EvC0wEnLQHwEG0MIVYQ?=
 =?us-ascii?Q?AXSoSWaLv2JJCQgUFG2VbPZqX4Y3OhtTJGF+WHte9ay8gIr/QGN/unJGFQch?=
 =?us-ascii?Q?rWONQiu+XqnAikvB3QedNj0oSxIzW1TaXxd82fCn3TGFgch+CkSP/War7F4u?=
 =?us-ascii?Q?de27Meg2mqPOUll9qSREa7dmOOVRKsUdaVQdD2kFsdWak5tKMJtoGU8WTcwN?=
 =?us-ascii?Q?xM3kZoEfpsaPbe7fDkN9JZtMhmFr8XFX8wSKzJFxKPFRmdnbLjW6BrE7UfFP?=
 =?us-ascii?Q?9m6mhqOZOuurYRCmEoz4kfC6QNnT4KQz+JFeQuGo8fceazaYVe54qd4O0Lls?=
 =?us-ascii?Q?PXAdg/vx9NwDQirTWs5hh8rllZsk6lpeJUNRjh9MC3TYPvJrhFSQd+qch7ub?=
 =?us-ascii?Q?eOjC19vVGk+HuBBIH/NMUr23aiFXPPtYwF47gieOVE4wv4xKR5zyxZ2ZQzB6?=
 =?us-ascii?Q?78wVktlthzhRIzDcSbp+ijRD9MDPxmEIzFM4IV26ms016BmJC/OkRSdo55eb?=
 =?us-ascii?Q?6+N23BqM9tAG/89702i2W8puhMwopWi+tZOvervLjiiINEb9LICAf1nWqOxU?=
 =?us-ascii?Q?xPoxcVdlWtaqzt7lb+7ZAjwrDqH7XD91py6pxKCG+/Sc3sPv2WCK7Q9US8KZ?=
 =?us-ascii?Q?gEM6dhMMmfeUqyzbPQBgoUREDx1cWqWszC7Pq0UmR08614HJOhjNEgQgXsMl?=
 =?us-ascii?Q?3e0vwb2e36MVQgseqt6Gp/wUh8KBOrF6RGUUPtNcJOoStdxvj2mtgMoZr+Eb?=
 =?us-ascii?Q?QH3t59/fSngcU+mctvjYMVgL775aQHNxfvKT88ZkrQ3WfLrppOhHrbg8koP0?=
 =?us-ascii?Q?ds/lp0UqFI9v0nkW4OKj0o6PQs9Hr7dBXiQ6EdZ9HmXUk0V0zY7zwXdjoBtD?=
 =?us-ascii?Q?daPhtkrHnzaxWX7M6TEhPNA+aBSABrFfHDS53bd42kwuE2jOld6YCNvdacCX?=
 =?us-ascii?Q?LA80wAKGxjXHnms8FcjGEgI/HaIMnGhwqcZcoeiiH73ZRRVtv7GJbNeCu4z6?=
 =?us-ascii?Q?eIvIiAKQ5FVoihuXQmYIWGBcPulO3qA5iyhU1yzuNMFLez8v6XbQ7s6xqbzF?=
 =?us-ascii?Q?cZVNtd/WArV+di8nff+iKnn2oBVNGmryxnGm9Cqu6f4pyFoqrIPFxhfoLlgx?=
 =?us-ascii?Q?3NqbwlVQBvHP80NnPWXAk16J6CnqHXJAgqzdkDckmGBZq5arS6hOsy/tWkwI?=
 =?us-ascii?Q?WcWPgCpDBJ4TXVpDSoQBuPgJSwA1IyKhPBXkJC/aCqrH7KI4HygR8lFD4IeU?=
 =?us-ascii?Q?g5kApssRssQbZqu1+YjH4TZ+1mfuI8BCd6ui/Z/jdChNOpdg5Vlcg81r0Qnv?=
 =?us-ascii?Q?tw9KXzMzCSR6/UMFLNqksq/rTlTZokl8x28yMWkhnIq+ok11n0YMSmSHdBY0?=
 =?us-ascii?Q?i6vC3eJQpVJzI0Os74u72an6wPEFxCln6wU0buHcVRf4f4gzQkU+XAY70+Yl?=
 =?us-ascii?Q?tBEGGiSm72FxG7zV0LbTqlFzhK7agcRPvkAoxpjB?=
X-MS-Exchange-CrossTenant-Network-Message-Id: aa5b8b0c-0018-4e38-3ff9-08db1956cf05
X-MS-Exchange-CrossTenant-AuthSource: DM5PR11MB1418.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 28 Feb 2023 06:41:23.6197
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: SbEje9jIdKpQuk6KAXc1tcTVTapvJUTRMUAquYuHB5WXaes5dA2ohoc+nkrkmrcatqlib/L48Wx4++Sp+Ke84g==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS7PR11MB6296
X-OriginatorOrg: intel.com
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi Mark,

Thanks for the review.
On Tue, Feb 28, 2023 at 01:23:09AM +0800, Mark Brown wrote:
> On Sat, Feb 25, 2023 at 10:01:17PM +0800, Ye Xiang wrote:
> 
> > +struct spi_xfer_packet {
> > +	u8 indicator;
> > +	s8 len;
> > +	u8 data[];
> > +} __packed;
> 
> > +static int ljca_spi_read_write(struct ljca_spi_dev *ljca_spi, const u8 *w_data, u8 *r_data, int len,
> > +			       int id, int complete, int cmd)
> > +{
> > +	struct spi_xfer_packet *w_packet = (struct spi_xfer_packet *)ljca_spi->obuf;
> > +	struct spi_xfer_packet *r_packet = (struct spi_xfer_packet *)ljca_spi->ibuf;
> > +	unsigned int ibuf_len = LJCA_SPI_BUF_SIZE;
> > +	int ret;
> > +
> > +	w_packet->indicator = FIELD_PREP(LJCA_SPI_XFER_INDICATOR_ID, id) |
> > +			      FIELD_PREP(LJCA_SPI_XFER_INDICATOR_CMPL, complete) |
> > +			      FIELD_PREP(LJCA_SPI_XFER_INDICATOR_INDEX,
> > +					 ljca_spi->spi_info->id);
> > +
> > +	if (cmd == LJCA_SPI_READ) {
> > +		w_packet->len = sizeof(u16);
> > +		*(u16 *)&w_packet->data[0] = len;
> 
> Are there no endianness considerations here?
Yes, it should be little endian. Will address this.
> 
> > +static int ljca_spi_transfer(struct ljca_spi_dev *ljca_spi, const u8 *tx_data,
> > +			     u8 *rx_data, u16 len)
> > +{
> 
> This function has one caller with barely anything in it - perhaps just
> inline it?
Agree. Will make this function inline.

--
Thanks
Ye Xiang