Re: [PATCH 2/4] i2c: iproc: Add Broadcom iProc I2C Driver

[Ray Jui]

On 12/9/2014 7:28 PM, Varka Bhadram wrote:
> On Wed, Dec 10, 2014 at 8:51 AM, Varka Bhadram <varkabhadram@xxxxxxxxx
> <mailto:varkabhadram@xxxxxxxxx>> wrote:
>
>     On Wed, Dec 10, 2014 at 7:11 AM, Ray Jui <rjui@xxxxxxxxxxxx
>     <mailto:rjui@xxxxxxxxxxxx>> wrote:
>
>
>
>         On 12/9/2014 5:33 PM, Varka Bhadram wrote:
>
>
>             On Wednesday 10 December 2014 06:24 AM, Ray Jui wrote:
>
>                 Add initial support to the Broadcom iProc I2C controller
>                 found in the
>                 iProc family of SoCs.
>
>                 The iProc I2C controller has separate internal TX and RX
>                 FIFOs, each has
>                 a size of 64 bytes. The iProc I2C controller supports
>                 two bus speeds
>                 including standard mode (100kHz) and fast mode (400kHz)
>
>                 Signed-off-by: Ray Jui <rjui@xxxxxxxxxxxx
>                 <mailto:rjui@xxxxxxxxxxxx>>
>                 Reviewed-by: Scott Branden <sbranden@xxxxxxxxxxxx
>                 <mailto:sbranden@xxxxxxxxxxxx>>
>                 ---
>                    drivers/i2c/busses/Kconfig         |    9 +
>                    drivers/i2c/busses/Makefile        |    1 +
>                    drivers/i2c/busses/i2c-bcm-iproc.c |  503
>                 ++++++++++++++++++++++++++++++++++++
>                    3 files changed, 513 insertions(+)
>                    create mode 100644 drivers/i2c/busses/i2c-bcm-iproc.c
>
>     (...)
>
>                 +static int bcm_iproc_i2c_probe(struct platform_device
>                 *pdev)
>                 +{
>                 +    int irq, ret = 0;
>                 +    struct bcm_iproc_i2c_dev *dev;
>                 +    struct i2c_adapter *adap;
>                 +    struct resource *res;
>                 +
>                 +    dev = devm_kzalloc(&pdev->dev, sizeof(*dev),
>                 GFP_KERNEL);
>                 +    if (!dev)
>                 +        return -ENOMEM;
>                 +
>                 +    platform_set_drvdata(pdev, dev);
>                 +    dev->device = &pdev->dev;
>                 +    init_completion(&dev->done);
>                 +
>                 +    res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>                 +    if (!res)
>                 +        return -ENODEV;
>
>
>             We can remove this resource check. This checking will happen
>             with
>             devm_ioremap_resource()
>
>         Don't you need to obtain a valid resource and pass it into
>         devm_ioremap_resource? Without 'res' being assigned a valid
>         resource, devm_ioremap_resource will reject with "invalid resource".
>
>     platform_get_resource() will return a resource, checking on this
>     resource is happening at
>     http://lxr.free-electrons.com/source/lib/devres.c#L115. So no need
>     to check it explicitly.
>
>     If you check here it will be duplication of check with resource. Two
>     times we are checking on
>     the resource. No point of doing like that.
>
>     Thanks.
Sorry I misunderstood what you meant. Okay I'll get rid of if (!res) 
check there. Thanks.

> See this: http://lxr.free-electrons.com/source/lib/devres.c#L102
>
> --
> Thanks and Regards,
> Varka Bhadram.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/