On Mon, May 06, 2019 at 02:06:20PM +0200, Jochen Henneberg wrote:
This driver has huge amounts of non-standard interfaces in it,
especially the userpace ABI it adds. It would be a lot easier to review
if it were split up so that it's a series where the core SPI
functionality is added initially and then other things were layered on
top as additional patches.
> @@ -0,0 +1,1672 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +/* Kernel driver for Silicon Labs CP2130 USB-to-SPI bridge.
> + *
> + * Copyright (C) 2019 Jochen Henneberg (jh@xxxxxxxxxxxxxxxxxxxxxxxxxx)
> + */
Please keep the entire comment a C++ comment, it makes things look more
intentional.
> +/* Prototypes */
> +static int cp2130_probe(struct usb_interface *intf,
> + const struct usb_device_id *id);
> +static void cp2130_disconnect(struct usb_interface *intf);
These forward declarations are really weird in a Linux driver, normally
things like probe() are defined at the bottom of the driver just before
the driver structure which also usually goes at the end.
> +/* USB device functions */
> +static struct usb_driver cp2130_driver = {
> + .name = "cp2130",
> + .probe = cp2130_probe,
> + .disconnect = cp2130_disconnect,
> + .suspend = NULL,
> + .resume = NULL,
> + .reset_resume = NULL,
> + .id_table = cp2130_devices,
> + .supports_autosuspend = 0,
> +};
Static variables are initialized to 0 by default, no need to explicitly
do that.
> +static int __init cp2130_init(void)
> +{
module_usb_driver().
> +static int cp2130_spi_setup(struct spi_device *spi)
> +{
> + return 0;
> +}
> +
> +static void cp2130_spi_cleanup(struct spi_device *spi)
> +{
> +}
Omit empty functions. If the framework won't let you omit empty
functions they probably can't safely be empty.
> + ret = sprintf(out, "channel\tcs_mode\tirq_pin\tclock_phase\tpolarity"
> + "\tcs_pin_mode\tclock_freq\tdelay_mask"
> + "\tinter_byte_delay\tpre_delay\tpost_delay"
> + "\tmod_alias\n");
> + strcat(buf, out);
> + mutex_lock(&chip->chn_config_lock);
> + for (i = 0; i < CP2130_NUM_GPIOS; i++) {
> + chn = &chip->chn_configs[i];
> + ret += sprintf(out, "%d\t%d\t%d\t%d\t\t%d\t\t%d\t\t%s\t%d"
> + "\t\t%d\t\t\t%d\t\t%d\t\t'%s'\n",
> + i, chn->cs_en, chn->irq_pin, chn->clock_phase,
> + chn->polarity, chn->cs_pin_mode,
> + cp2130_spi_speed_to_string(chn->clock_freq),
> + chn->delay_mask, chn->inter_byte_delay,
> + chn->pre_deassert_delay, chn->post_assert_delay,
> + chn->modalias);
> + strcat(buf, out);
> + }
This looks like a bunch of mostly very generic diagnostic data, if it's
useful to have it should be added in the framework so it's available for
all drivers.
> +static ssize_t channel_config_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
This is adding a completely non-standard ABI for configuring things -
why not use standard interfaces?
> +static DEVICE_ATTR_RW(channel_config);
Device attributes in sysfs should follow sysfs rules, including having
just a single value per file to ease machine parsing.
> +out:
> + return (!ret ? len : ret);
Please write normal conditional statements to make things easier for
people reading the driver.
> + mutex_lock(&dev->usb_bus_lock);
What is this protecting?
> + /* iterate through all transfers */
> + list_for_each_entry(xfer, &mesg->transfers, transfer_list) {
> + dev_dbg(&master->dev, "spi transfer stats: %p, %p, %d",
> + xfer->tx_buf, xfer->rx_buf, xfer->len);
It's not clear to me why the driver can't use transfer_one() instead of
transfer_one_message().
Attachment:
signature.asc
Description: PGP signature
