Re: [PATCH 4/6] i2c: busses: add SLIMpro I2C device driver on APM X-Gene platform

Re: [PATCH 4/6] i2c: busses: add SLIMpro I2C device driver on APM X-Gene platform

[Feng Kan]

On Tue, Nov 11, 2014 at 1:51 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Tuesday 07 October 2014 17:06:47 Feng Kan wrote:
>> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
>> index 2e45ae3..a03042c 100644
>> --- a/drivers/i2c/busses/Kconfig
>> +++ b/drivers/i2c/busses/Kconfig
>> @@ -1009,6 +1009,15 @@ config I2C_CROS_EC_TUNNEL
>>         connected there. This will work whatever the interface used to
>>         talk to the EC (SPI, I2C or LPC).
>>
>> +config I2C_XGENE_SLIMPRO
>> +     tristate "APM X-Gene SoC I2C SLIMpro devices support"
>> +     depends on ARCH_XGENE && XGENE_SLIMPRO_MBOX
>
> Why this dependency on XGENE_SLIMPRO_MBOX?
>
> Better replace it with a dependency on MAILBOX.
Arnd, just a question. Is this because this possibly help with future
compatibility by choosing a more broad dependency?
>
>> +     } else {
>> +             spin_lock_irqsave(&ctx->lock, flags);
>> +             ctx->i2c_rx_poll = 1;
>> +             for (count = SLIMPRO_I2C_WAIT_COUNT; count > 0; count--) {
>> +                     if (ctx->i2c_rx_poll == 0)
>> +                             break;
>> +                     udelay(100);
>> +             }
>
> No, you can't block the CPU for an extended amount of time with
> interrupts disabled. Please kill this code.
>
>> +     ctx->resp_msg = data;
>> +     if (ctx->mbox_client.tx_block)
>> +             init_completion(&ctx->rd_complete);
>
> reinit_completion()?
>
>> +static int slimpro_i2c_blkrd(struct slimpro_i2c_dev *ctx, u32 chip, u32 addr,
>> +                             u32 addrlen, u32 protocol, u32 readlen,
>> +                             u32 with_data_len, void *data)
>> +{
>> +     dma_addr_t paddr;
>> +     u32 msg[3];
>> +     int rc;
>> +
>> +     paddr = dma_map_single(ctx->dev, ctx->dma_buffer, readlen,
>> +                            DMA_FROM_DEVICE);
>
> ctx->dev is probably the wrong device here. The i2c controller is not
> DMA capable itself, you need to have a pointer to the device that actually
> performs the DMA here.
>
>
>> +     /* Request mailbox channel */
>> +     cl->dev = &pdev->dev;
>> +     cl->rx_callback = slimpro_i2c_rx_cb;
>> +     cl->tx_done = slimpro_i2c_tx_done;
>> +     cl->tx_block = true;
>> +     cl->tx_tout = SLIMPRO_OP_TO_MS;
>> +     cl->knows_txdone = false;
>> +     cl->chan_name = "i2c-slimpro";
>> +     ctx->mbox_chan = mbox_request_channel(cl);
>
> This is not the correct interface.
>
>> +     rc = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64));
>> +     if (rc)
>> +             dev_warn(&pdev->dev, "Unable to set dma mask\n");
>
> Are you sure that this is the correct device to perform the DMA?
>
> Moreover, the mask doesn't match the usage: the slimpro_i2c_blkrd
> function passes in only the lower 32 bit of the address, which would
> be DMA_BIT_MASK(32).
>
>> +#ifdef CONFIG_OF
>> +static struct of_device_id xgene_slimpro_i2c_id[] = {
>> +     {.compatible = "apm,xgene-slimpro-i2c" },
>> +     {},
>> +};
>> +MODULE_DEVICE_TABLE(of, xgene_slimpro_i2c_dt_ids);
>> +#endif
>> +
>> +static struct platform_driver xgene_slimpro_i2c_driver = {
>> +     .probe  = xgene_slimpro_i2c_probe,
>> +     .remove = xgene_slimpro_i2c_remove,
>> +     .driver = {
>> +             .name   = XGENE_SLIMPRO_I2C,
>> +             .owner  = THIS_MODULE,
>> +             .of_match_table = of_match_ptr(xgene_slimpro_i2c_id)
>> +     },
>> +};
>
> The driver only supports DT, so just drop the #ifdef and the of_match_ptr().
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/6] i2c: busses: add SLIMpro I2C device driver on APM X-Gene platform

[Arnd Bergmann]

On Friday 09 January 2015 10:56:51 Feng Kan wrote:
> On Tue, Nov 11, 2014 at 1:51 PM, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> wrote:
> > On Tuesday 07 October 2014 17:06:47 Feng Kan wrote:
> >> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> >> index 2e45ae3..a03042c 100644
> >> --- a/drivers/i2c/busses/Kconfig
> >> +++ b/drivers/i2c/busses/Kconfig
> >> @@ -1009,6 +1009,15 @@ config I2C_CROS_EC_TUNNEL
> >>         connected there. This will work whatever the interface used to
> >>         talk to the EC (SPI, I2C or LPC).
> >>
> >> +config I2C_XGENE_SLIMPRO
> >> +     tristate "APM X-Gene SoC I2C SLIMpro devices support"
> >> +     depends on ARCH_XGENE && XGENE_SLIMPRO_MBOX
> >
> > Why this dependency on XGENE_SLIMPRO_MBOX?
> >
> > Better replace it with a dependency on MAILBOX.
> Arnd, just a question. Is this because this possibly help with future
> compatibility by choosing a more broad dependency?

The dependency should ideally describe build-time dependencies,
to make it possible to build on other architectures for static
code analysis purposes. If the driver makes no sense on other
platforms you can also use

	depends on ARCH_XGENE || COMPILE_TEST
	depends on MAILBOX

to cover both the build-time and run-time dependencies. But the
dependency on XGENE_SLIMPRO_MBOX just shouldn't be there, the driver
will work with any mailbox implementation if someone puts the same
hardware into a different SoC.

	Arnd