On Wed, Mar 17, 2021 at 10:30:21AM +0000, Bence Csókás wrote:
> Signed-off-by: Bence Csókás <bence98@xxxxxxxxxx>
Thanks, this looks good now and I think we are very close.
> ---
Next, time please provide a small summary of changes since last version.
I get enough patches that it becomes confusing otherwise.
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-cp2615.c
> @@ -0,0 +1,282 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
If you want GPL v2 only, then you need to say in MODULE_LICENSE also
"GPL v2".
> +#include <linux/string.h>
> +#include <linux/kernel.h>
> +#include <linux/errno.h>
> +#include <linux/i2c.h>
> +#include <linux/usb.h>
Please sort this to avoid double entries get added in the future.
> +enum cp2615_iop_msg_type {
> + iop_GetAccessoryInfo = 0xD100,
> + iop_AccessoryInfo = 0xA100,
> + iop_GetPortConfiguration = 0xD203,
> + iop_PortConfiguration = 0xA203,
> + // ...
This comment can go?
> +/** Possible values for struct cp2615_i2c_transfer_result.status
> + *
> + * Values extracted from the USBXpress(r) SDK
> + */
I'd think this can go, too.
> +int cp2615_init_iop_msg(struct cp2615_iop_msg *ret, enum cp2615_iop_msg_type msg, const void *data, size_t data_len)
Please break it into two lines. And please run 'checkpatch --strict' on
your patch to find things like too long lines and spaces around
operators etc... I will skip pointing them out here.
> +{
> + if (data_len > MAX_IOP_PAYLOAD_SIZE)
> + return -EFBIG;
> +
> + if (ret) {
Minor nit:
if (!ret)
return -EINVAL;
and then save the indentation for the main code path.
> + ret->preamble = 0x2A2A;
> + ret->length = htons(data_len+6);
> + ret->msg = htons(msg);
> + if(data && data_len)
> + memcpy(&ret->data, data, data_len);
> + return 0;
> + } else {
> + return -EINVAL;
> + }
> +}
> +
> +int cp2615_init_i2c_msg(struct cp2615_iop_msg *ret, const struct cp2615_i2c_transfer *data)
> +{
> + return cp2615_init_iop_msg(ret, iop_DoI2cTransfer, data, 4 + data->write_len);
> +}
I'll leave it to you but maybe this function can go and
cp2615_init_iop_msg can be used directly? Both are only called once.
> +
> +/* Translates status codes to Linux errno's */
> +int cp2615_check_status(enum cp2615_i2c_status status)
> +{
> + switch (status) {
> + case CP2615_SUCCESS:
> + return 0;
> + case CP2615_BUS_ERROR:
> + return -ECOMM;
-EIO probably.
We have 'Documentation/i2c/fault-codes.rst' as a guide.
> + case CP2615_BUS_BUSY:
> + return -EAGAIN;
> + case CP2615_TIMEOUT:
> + return -ETIMEDOUT;
> + case CP2615_INVALID_PARAM:
> + return -EINVAL;
> + case CP2615_CFG_LOCKED:
> + return -EPERM;
> + }
> + /* Unknown error code */
> + return -EPROTO;
> +}
> +
...
> +/*
> + * This chip has some limitations: one is that the USB endpoint
> + * can only receive 64 bytes/transfer, that leaves 54 bytes for
> + * the I2C transfer. On top of that, EITHER read_len OR write_len
> + * may be zero, but not both. If both are non-zero, the adapter
> + * issues a write followed by a read. And the chip does not
> + * support repeated START between the write and read phases.
Good and useful paragraph!
> + * FIXME: There in no quirk flag for specifying that the adapter
> + * does not support empty transfers, or that it cannot emit a
Can't we use I2C_AQ_NO_ZERO_LEN here?
> + * START condition between the combined phases.
True! But it makes sense, so we can fix that. We just need to add
I2C_AQ_NO_REP_START and a short explanation to i2c.h. If you want, you
can do it in a seperate patch. I can do it, too, if you prefer.
> + */
> +struct i2c_adapter_quirks cp2615_i2c_quirks = {
> + .max_write_len = MAX_I2C_SIZE,
> + .max_read_len = MAX_I2C_SIZE,
> + .flags = I2C_AQ_COMB_WRITE_THEN_READ,
> + .max_comb_1st_msg_len = MAX_I2C_SIZE,
> + .max_comb_2nd_msg_len = MAX_I2C_SIZE
> +};
quirks struct looks good otherwise!
> +static const struct usb_device_id id_table[] = {
> + { USB_DEVICE(CP2615_VID, CP2615_PID) },
Maybe skip the defines for VID and PID and use the values directly?
I am not a USB expert, not really sure what the consistent way is.
So, this and the checkpatch issues and I think we are done.
Thanks,
Wolfram
Attachment:
signature.asc
Description: PGP signature
