Re: [PATCH 2/2] Adding i2c-cp2615 driver

[Wolfram Sang <wsa@kernel.org>]

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

Hi!

On Wed, Feb 10, 2021 at 10:08:54PM +0100, Bence Csókás wrote:
> For a hardware project, I need the I2C master of SiLabs' CP2615 chip
> to be visible from under Linux. This patchset adds i2c-cp2615, a
> driver which sets up an i2c_adapter for said chip.
> 
> This is my first contribution, so forgive me (but do let me know) if
> I've broken habit.

Thank you for your contribution and sorry for the delay (which is
only because of not enough time and not because of no interest).

First thing is that patch 1 & 2 should be squashed into one patch.


> Signed-off-by: Bence Csókás <bence98@sch.bme.hu>
> ---
>  drivers/i2c/busses/cp2615_drv.c | 150 ++++++++++++++++++++++++++++++++
>  drivers/i2c/busses/cp2615_iop.c |  32 +++++++
>  drivers/i2c/busses/cp2615_iop.h |  60 +++++++++++++

Then, all these files should go into one file named "i2c-cp2615.c". We
can factor out stuff later if another user turns up. But for starters,
all in one file is more convenient.

> +static int
> +cp2615_i2c_send(struct usb_interface *usbif, struct cp2615_i2c_transfer *i2c_w)
> +{
> +    struct cp2615_iop_msg *msg = kzalloc(sizeof(struct
> cp2615_iop_msg), GFP_KERNEL);

The patch look garbled with broken lines; are you using gmail WEB UI?
Hopefully, this document can help you:

	Documentation/process/email-clients.rst

If it is not garbled, then I can review it better. Some things already.

> +struct i2c_adapter_quirks cp2615_i2c_quirks = {
> +    .max_write_len = MAX_I2C_SIZE,
> +    .max_read_len = MAX_I2C_SIZE,

Yes, good, we need quirks. But IIUC these also on top:

	.flags = I2C_AQ_COMB_WRITE_THEN_READ,
	.max_comb_1st_msg_len = MAX_I2C_SIZE,
	.max_comb_2nd_msg_len = MAX_I2C_SIZE,

because the datasheet says "The transfer consists of a write cycle
followed by a read cycle." BTW this is kinda bad for multi-master:
"Repeated start between the write and read cycles is not supported". But
I guess this bus will not be really used in multi-master setups.
However, it should be commented somewhere.

> +static void
> +cp2615_i2c_remove(struct usb_interface *usbif)
> +{
> +    struct i2c_adapter *adap = usb_get_intfdata(usbif);
> +
> +    usb_set_intfdata(usbif, NULL);
> +    i2c_del_adapter(adap);
> +    kfree(adap);
> +    dev_info(&usbif->dev, "Removed CP2615's I2C bus\n");

This dev_info can go.

> +}
> +
> +static int
> +cp2615_i2c_probe(struct usb_interface *usbif, const struct usb_device_id *id)
> +{
> +    int ret = 0;
> +    struct i2c_adapter *adap;
> +    struct usb_device *usbdev = interface_to_usbdev(usbif);
> +
> +    ret = usb_set_interface(usbdev, IOP_IFN, IOP_ALTSETTING);
> +    if (ret)
> +        goto out;

'return ret;' instead of 'goto out;' here and later.

> +
> +    adap = kzalloc(sizeof(struct i2c_adapter), GFP_KERNEL);

devm_kzalloc? Then you can leave out the kfrees.

> +    if (!adap) {
> +        ret = -ENOMEM;
> +        goto out;
> +    }
> +
> +    strncpy(adap->name, usbdev->serial, sizeof(adap->name));
> +    adap->owner = THIS_MODULE;
> +    adap->class = I2C_CLASS_HWMON | I2C_CLASS_SPD;

I guess you instantiate client devices via sysfs? Then, this line can
go, too.

> +    adap->dev.parent = &usbif->dev;
> +    adap->dev.of_node = usbif->dev.of_node;
> +    adap->timeout = HZ;
> +    adap->algo = &cp2615_i2c_algo;
> +    adap->quirks = &cp2615_i2c_quirks;
> +    adap->algo_data = usbif;
> +
> +    ret = i2c_add_adapter(adap);
> +    if (ret) {
> +        kfree(adap);
> +        goto out;
> +    }
> +
> +    usb_set_intfdata(usbif, adap);
> +    dev_info(&usbif->dev, "Added CP2615's I2C bus\n");

This dev_info can go.

> +out:
> +    return ret;
> +}
> +
> +static const struct usb_device_id id_table[] = {
> +    { USB_DEVICE(CP2615_VID, CP2615_PID) },
> +    { }
> +};
> +
> +MODULE_DEVICE_TABLE(usb, id_table);
> +
> +static struct usb_driver cp2615_i2c_driver = {
> +    .name = "i2c-cp2615",
> +    .probe = cp2615_i2c_probe,
> +    .disconnect = cp2615_i2c_remove,
> +    .id_table = id_table,
> +//    .dev_groups = cp2615_groups,

This should go.

> +};
> +
> +module_usb_driver(cp2615_i2c_driver);
> +
> +MODULE_AUTHOR("Bence Csókás <bence98@sch.bme.hu>");
> +MODULE_DESCRIPTION("CP2615 I2C bus driver");
> +MODULE_LICENSE("GPL");

Please also add an SPDX header at the top. Check other i2c drivers for
an appropriate.

> +    if (ret) {
> +        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;

Curly braces around 'else' branch. Please run 'scripts/checkpatch' on
your patch.

> +enum cp2615_iop_msg_type {
> +    iop_GetAccessoryInfo = 0xD100,
> +    iop_AccessoryInfo = 0xA100,
> +    iop_GetPortConfiguration = 0xD203,
> +    iop_PortConfiguration = 0xA203,
> +    // ...

This should go.

> +    iop_DoI2cTransfer = 0xD400,
> +    iop_I2cTransferResult = 0xA400,
> +    iop_GetSerialState = 0xD501,
> +    iop_SerialState = 0xA501
> +};
> +struct cp2615_i2c_transfer {
> +    unsigned char tag, i2caddr, read_len, write_len;
> +    char data[MAX_I2C_SIZE];

u8?

> +};
> +
> +struct cp2615_i2c_transfer_result {
> +    unsigned char tag, i2caddr, status, read_len;
> +    char data[MAX_I2C_SIZE];

u8?

> +};
> +

I am not a USB expert, but maybe someone else can have a look on your
updated patch.

Despite the comments, looks quite good already.

Happy hacking,

   Wolfram


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