Hi Uli,
> +struct max9260_device {
> + struct serdev_device *serdev;
> + u8 *rx_buf;
> + int rx_len;
> + int rx_state;
> + wait_queue_head_t rx_wq;
> + struct i2c_adapter adap;
> +};
> +
> +static void wait_for_transaction(struct max9260_device *dev)
max9260_ prefix as well?
> +{
> + wait_event_interruptible_timeout(dev->rx_wq,
> + dev->rx_state <= RX_FRAME_ERROR,
> + HZ/2);
I'd suggest to drop the interruptible. It can be done but it is usually
not trivial to abort the operation gracefully when a signal comes in.
Also, timeout is superfluous since you don't get the return value?
> +static int max9260_setup(struct max9260_device *dev)
> +{
> + int ret;
> +
> + ret = max9260_read_reg(dev, 0x1e);
> +
> + if (ret != 0x02) {
> + dev_err(&dev->serdev->dev,
> + "device does not identify as MAX9260\n");
> + return -EINVAL;
I think -ENODEV is the proper errno for a not-found device. Also, the
error message could probably go.
> + }
> +
> + return 0;
> +}
> +
> +static void max9260_uart_write_wakeup(struct serdev_device *serdev)
> +{
Maybe a FIXME comment for this empty function?
> +static u32 max9260_i2c_func(struct i2c_adapter *adapter)
> +{
> + return I2C_FUNC_SMBUS_BYTE|I2C_FUNC_SMBUS_BYTE_DATA;
Spaces around operators.
> + dev = kzalloc(sizeof(*dev), GFP_KERNEL);
> + if (!dev)
> + return -ENOMEM;
You don't like devm_* it seems ;)
Rest looks good to me!
Regards,
Wolfram
Attachment:
signature.asc
Description: PGP signature
