Re: [PATCH v2] Adding i2c-cp2615: i2c support for Silicon Labs' CP2615 Digital Audio Bridge

[Wolfram Sang <wsa@kernel.org>]

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

On Wed, Mar 17, 2021 at 10:30:21AM +0000, Bence Csókás wrote:
> Signed-off-by: Bence Csókás <bence98@sch.bme.hu>

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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]