Re: [PATCH v5 3/3] i2c: ch341: add I2C MFD cell driver for the CH341

[Wolfram Sang <wsa@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 3678 bytes --]

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.

> +	help
> +	  If you say yes to this option, I2C support will be included for the
> +	  WCH CH341, a USB to I2C/SPI/GPIO interface.
> +
> +	  This driver can also be built as a module.  If so, the module
> +	  will be called i2c-ch341.
> +
>  config I2C_DIOLAN_U2C
>  	tristate "Diolan U2C-12 USB adapter"
>  	depends on USB

...

> diff --git a/drivers/i2c/busses/i2c-ch341.c b/drivers/i2c/busses/i2c-ch341.c
> new file mode 100644
> index 000000000000..3da11e358976
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-ch341.c
> @@ -0,0 +1,331 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * I2C cell driver for the CH341A, CH341B and CH341T.
> + *
> + * Copyright 2022, Frank Zago
> + * Copyright (c) 2016 Tse Lun Bien
> + * Copyright (c) 2014 Marco Gittler
> + * Copyright (C) 2006-2007 Till Harbaum (Till@Harbaum.org)
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/types.h>
> +#include <linux/usb.h>
> +
> +#include <linux/i2c.h>
> +
> +#include <linux/mfd/ch341.h>

Please sort the includes. No need for emtpy lines.

> +
> +/* I2C bus speed. Speed selection is not implemented. */
> +#define CH341_I2C_20KHZ  0
> +#define CH341_I2C_100KHZ 1
> +#define CH341_I2C_400KHZ 2
> +#define CH341_I2C_750KHZ 3
> +
> +/* I2C chip commands */
> +#define CH341_CMD_I2C_STREAM 0xAA
> +#define CH341_CMD_I2C_STM_END 0x00
> +
> +#define CH341_CMD_I2C_STM_STA 0x74
> +#define CH341_CMD_I2C_STM_STO 0x75
> +#define CH341_CMD_I2C_STM_OUT 0x80
> +#define CH341_CMD_I2C_STM_IN 0xC0
> +#define CH341_CMD_I2C_STM_SET 0x60
> +
> +/*
> + * 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?

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?

...

> +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?

...

> +
> +MODULE_AUTHOR("Various");

Please name the relevant authors. Only the ones which directly worked
on this driver.

> +MODULE_DESCRIPTION("CH341 USB to I2C");
> +MODULE_LICENSE("GPL");

SPDX header says "GPL v2".

So much for now, thanks for your submission!

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]