Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Thu, 1 Jul 2021 11:48:46 +0530
From:   Viresh Kumar <viresh.kumar@linaro.org>
To:     Jie Deng <jie.deng@intel.com>
Cc:     linux-i2c@vger.kernel.org,
        virtualization@lists.linux-foundation.org,
        linux-kernel@vger.kernel.org, wsa@kernel.org,
        wsa+renesas@sang-engineering.com, mst@redhat.com, arnd@arndb.de,
        jasowang@redhat.com, andriy.shevchenko@linux.intel.com,
        yu1.wang@intel.com, shuo.a.liu@intel.com, conghui.chen@intel.com,
        stefanha@redhat.com
Subject: Re: [PATCH v11] i2c: virtio: add a virtio i2c frontend driver
Message-ID: <20210701061846.7u4zorimzpmb66v7@vireshk-i7>
References: <510c876952efa693339ab0d6cc78ba7be9ef6897.1625104206.git.jie.deng@intel.com>
 <20210701040436.p7kega6rzeqz5tlm@vireshk-i7>
 <cb35472d-f79e-f3f8-405f-35c699d897a1@intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <cb35472d-f79e-f3f8-405f-35c699d897a1@intel.com>
User-Agent: NeoMutt/20180716-391-311a52
Precedence: bulk

On 01-07-21, 14:10, Jie Deng wrote:
> I think a fixed number of sgs will make things easier to develop backend.

Yeah, but it looks awkward to send a message buffer which isn't used
at all. From protocol's point of view, it just looks wrong/buggy.

The backend can just look at the number of elements received, they
can either be 2 (in case of zero-length) transfer, or 3 (for
read/write) and any other number is invalid.

> If you prefer to parse the number of descriptors instead of using the msg
> length to
> 
> distinguish the zero-length request from other requests, I'm OK to set a
> limit.

My concern is more about the specification here first.

> if (!msgs[i].len) {
> ??? sg_init_one(&msg_buf, reqs[i].buf, msgs[i].len);
> 
> ??? if (msgs[i].flags & I2C_M_RD)
> ??? ??? sgs[outcnt + incnt++] = &msg_buf;
> ??? else
> ??? ??? sgs[outcnt++] = &msg_buf;
> }

> > > +#ifdef CONFIG_PM_SLEEP
> > You could avoid this pair of ifdef by creating dummy versions of below
> > routines for !CONFIG_PM_SLEEP case. Up to you.
> 
> 
> Thank you. I'd like to keep the same.

Sure.

-- 
viresh