On Wed, Feb 18, 2015 at 11:34:01AM -0800, Feng Kan wrote:
> Add SLIMpro I2C device driver on APM X-Gene platform. This I2C
> device driver use the SLIMpro Mailbox driver to tunnel message to
> the SLIMpro coprocessor to do the work of accessing I2C components.
>
> Signed-off-by: Feng Kan <[email protected]>
> Signed-off-by: Hieu Le <[email protected]>
Sigh, I lost my first review, so here we go again...
It looks mostly good. Using checkpatch with '--strict' will show some
whitespace issues. Please fix these.
> +config I2C_XGENE_SLIMPRO
> + tristate "APM X-Gene SoC I2C SLIMpro devices support"
> + depends on ARCH_XGENE && MAILBOX
COMPILE_TEST?
> +static int start_i2c_msg_xfer(struct slimpro_i2c_dev *ctx)
> +{
> + if (ctx->mbox_client.tx_block) {
> + if (!wait_for_completion_timeout(&ctx->rd_complete,
> + msecs_to_jiffies
> + (MAILBOX_OP_TIMEOUT)))
Don't be too strict with the 80 char limit IMHO. I think it is more
readable to combine the last two lines into one.
> + msg[0] = SLIMPRO_IIC_ENCODE_MSG(SLIMPRO_IIC_BUS, chip,
> + SLIMPRO_IIC_READ, protocol, addrlen,
> + readlen);
ditto
> + msg[0] = SLIMPRO_IIC_ENCODE_MSG(SLIMPRO_IIC_BUS, chip,
> + SLIMPRO_IIC_WRITE, protocol, addrlen,
> + writelen);
ditto
> + paddr = dma_map_single(ctx->dev, ctx->dma_buffer, readlen,
> + DMA_FROM_DEVICE);
The device should be the device of the dma channel.
> + paddr = dma_map_single(ctx->dev, ctx->dma_buffer, writelen,
> + DMA_TO_DEVICE);
ditto
> + rc = dma_mapping_error(ctx->dev, paddr);
> + if (dma_mapping_error(ctx->dev, paddr)) {
if (rc)
> +static struct i2c_algorithm xgene_slimpro_i2c_algorithm = {
> + .smbus_xfer = xgene_slimpro_i2c_xfer,
Might be a tad less confusing to name this function
xgene_slimpro_smbus_xfer. You decide.
> + rc = i2c_add_adapter(adapter);
> + if (rc) {
> + dev_err(&pdev->dev, "Adapter registeration failed\n");
> + return rc;
> + }
> +
> + rc = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64));
> + if (rc)
> + dev_warn(&pdev->dev, "Unable to set dma mask\n");
Shouldn't that be before i2c_add_adapter?
> +static struct platform_driver xgene_slimpro_i2c_driver = {
> + .probe = xgene_slimpro_i2c_probe,
> + .remove = xgene_slimpro_i2c_remove,
> + .driver = {
> + .name = "xgene-slimpro-i2c",
> + .of_match_table = of_match_ptr(xgene_slimpro_i2c_id)
You are DT only, do we need of_match_ptr?
> +MODULE_DESCRIPTION("APM X-Gene SLIMpro I2C driver");
> +MODULE_LICENSE("GPL");
MODULE_AUTHOR left out intentionally?
Thanks,
Wolfram
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
