Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <825b7446-b082-8c6c-7cb0-f97a19bf0862@zago.net>
Date:   Wed, 15 Jun 2022 20:22:57 -0500
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.9.1
Subject: Re: [PATCH v5 3/3] i2c: ch341: add I2C MFD cell driver for the CH341
Content-Language: en-US
To:     Wolfram Sang <wsa@kernel.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        linux-kernel@vger.kernel.org,
        Bartosz Golaszewski <bgolaszewski@baylibre.com>,
        Johan Hovold <johan@kernel.org>, linux-usb@vger.kernel.org,
        Lee Jones <lee.jones@linaro.org>,
        Linus Walleij <linus.walleij@linaro.org>,
        linux-gpio@vger.kernel.org, linux-i2c@vger.kernel.org
References: <20220401023306.79532-1-frank@zago.net>
 <20220401023306.79532-4-frank@zago.net> <YojVHBofkBOFVYap@shikoro>
From:   Frank Zago <frank@zago.net>
In-Reply-To: <YojVHBofkBOFVYap@shikoro>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi Wolfram,

On 5/21/22 07:03, Wolfram Sang wrote:

> Hi Frank,
> 
> I am not super familiar with USB drivers, so mostly some high level
> review questions first:
> 
> On Thu, Mar 31, 2022 at 09:33:06PM -0500, frank zago wrote:
>> The I2C interface can run at 4 different speeds. This driver currently
>> only offer 100MHz. Tested with a variety of I2C sensors, and the IIO
> 
> 100kHz.
> 
>> subsystem.
>>
>> Signed-off-by: frank zago <frank@zago.net>
> 
> ...
> 
>> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
>> index a1bae59208e3..db9797345ad5 100644
>> --- a/drivers/i2c/busses/Kconfig
>> +++ b/drivers/i2c/busses/Kconfig
>> @@ -1199,6 +1199,16 @@ config I2C_RCAR
>>  
>>  comment "External I2C/SMBus adapter drivers"
>>  
>> +config I2C_CH341
>> +	tristate "CH341 USB to I2C support"
>> +	select MFD_CH341
> 
> Hmm, it selects a symbol which depends on USB. Not good AFAIK. I think
> this driver should depend on MFD_CH341.

I've been asked earlier to change it to select. 

>> +
>> +/*
>> + * The maximum request size is 4096 bytes, both for reading and
>> + * writing, split in up to 128 32-byte segments. The I2C stream must
>> + * start and stop in each 32-byte segment. Reading must also be split,
>> + * with up to 32-byte per segment.
>> + */
>> +#define SEG_COUNT 128
> 
> You mean between every 32 bytes, there is a START and STOP condition on
> the bus? Then, the maximum message size is 32 byte only, sadly. Or did I
> misunderstand?

I've tried to reword that section. The usb command is up to 4kb, but each 
32-byte section is a command to the chip, not i2c.

> 
> Can the driver send an arbitrary number of messages within one transfer?
> E.g. write, read, read, write, read? All connected with a REPEATED START
> and not with STOP and START?

Yes it can.

> 
> ...
> 
>> +static u32 ch341_i2c_func(struct i2c_adapter *adap)
>> +{
>> +	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
>> +}
> 
> Have you also tested zero length messages AKA SMBus Quick commands?

Not properly at the time, although this didn't break the driver. 
I've fixed that by adding the special case.

Regards,
  Frank