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@xxxxxxxx>
...
> 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@xxxxxxxxxxx)
> + */
> +
> +#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:
signature.asc
Description: PGP signature
