From: Suniel Mahesh <[email protected]>
There is no need to create a local pointer variable "dev" and
pass it various API's, instead use plat_dev which is enumerated
by platform core on successful probe.
Signed-off-by: Suniel Mahesh <[email protected]>
---
Note:
- Patch was tested and built(ARCH=arm) on staging-testing.
- No build issues reported, however it was not tested on
real hardware.
---
drivers/staging/ccree/ssi_driver.c | 63 +++++++++++++++++++-------------------
1 file changed, 31 insertions(+), 32 deletions(-)
diff --git a/drivers/staging/ccree/ssi_driver.c b/drivers/staging/ccree/ssi_driver.c
index 5f03c25..eb907ce 100644
--- a/drivers/staging/ccree/ssi_driver.c
+++ b/drivers/staging/ccree/ssi_driver.c
@@ -205,12 +205,11 @@ static int init_cc_resources(struct platform_device *plat_dev)
struct resource *req_mem_cc_regs = NULL;
void __iomem *cc_base = NULL;
struct ssi_drvdata *new_drvdata;
- struct device *dev = &plat_dev->dev;
- struct device_node *np = dev->of_node;
+ struct device_node *np = plat_dev->dev.of_node;
u32 signature_val;
int rc = 0;
- new_drvdata = devm_kzalloc(dev, sizeof(*new_drvdata), GFP_KERNEL);
+ new_drvdata = devm_kzalloc(&plat_dev->dev, sizeof(*new_drvdata), GFP_KERNEL);
if (!new_drvdata) {
rc = -ENOMEM;
goto post_drvdata_err;
@@ -225,16 +224,16 @@ static int init_cc_resources(struct platform_device *plat_dev)
/* First CC registers space */
req_mem_cc_regs = platform_get_resource(plat_dev, IORESOURCE_MEM, 0);
/* Map registers space */
- new_drvdata->cc_base = devm_ioremap_resource(dev, req_mem_cc_regs);
+ new_drvdata->cc_base = devm_ioremap_resource(&plat_dev->dev, req_mem_cc_regs);
if (IS_ERR(new_drvdata->cc_base)) {
- dev_err(dev, "Failed to ioremap registers");
+ dev_err(&plat_dev->dev, "Failed to ioremap registers");
rc = PTR_ERR(new_drvdata->cc_base);
goto post_drvdata_err;
}
- dev_dbg(dev, "Got MEM resource (%s): %pR\n", req_mem_cc_regs->name,
+ dev_dbg(&plat_dev->dev, "Got MEM resource (%s): %pR\n", req_mem_cc_regs->name,
req_mem_cc_regs);
- dev_dbg(dev, "CC registers mapped from %pa to 0x%p\n",
+ dev_dbg(&plat_dev->dev, "CC registers mapped from %pa to 0x%p\n",
&req_mem_cc_regs->start, new_drvdata->cc_base);
cc_base = new_drvdata->cc_base;
@@ -242,120 +241,120 @@ static int init_cc_resources(struct platform_device *plat_dev)
/* Then IRQ */
new_drvdata->irq = platform_get_irq(plat_dev, 0);
if (new_drvdata->irq < 0) {
- dev_err(dev, "Failed getting IRQ resource\n");
+ dev_err(&plat_dev->dev, "Failed getting IRQ resource\n");
rc = new_drvdata->irq;
goto post_drvdata_err;
}
- rc = devm_request_irq(dev, new_drvdata->irq, cc_isr,
+ rc = devm_request_irq(&plat_dev->dev, new_drvdata->irq, cc_isr,
IRQF_SHARED, "arm_cc7x", new_drvdata);
if (rc) {
- dev_err(dev, "Could not register to interrupt %d\n",
+ dev_err(&plat_dev->dev, "Could not register to interrupt %d\n",
new_drvdata->irq);
goto post_drvdata_err;
}
- dev_dbg(dev, "Registered to IRQ: %d\n", new_drvdata->irq);
+ dev_dbg(&plat_dev->dev, "Registered to IRQ: %d\n", new_drvdata->irq);
rc = cc_clk_on(new_drvdata);
if (rc)
goto post_drvdata_err;
- if (!dev->dma_mask)
- dev->dma_mask = &dev->coherent_dma_mask;
+ if (!plat_dev->dev.dma_mask)
+ plat_dev->dev.dma_mask = &plat_dev->dev.coherent_dma_mask;
- if (!dev->coherent_dma_mask)
- dev->coherent_dma_mask = DMA_BIT_MASK(DMA_BIT_MASK_LEN);
+ if (!plat_dev->dev.coherent_dma_mask)
+ plat_dev->dev.coherent_dma_mask = DMA_BIT_MASK(DMA_BIT_MASK_LEN);
/* Verify correct mapping */
signature_val = CC_HAL_READ_REGISTER(CC_REG_OFFSET(HOST_RGF, HOST_SIGNATURE));
if (signature_val != DX_DEV_SIGNATURE) {
- dev_err(dev, "Invalid CC signature: SIGNATURE=0x%08X != expected=0x%08X\n",
+ dev_err(&plat_dev->dev, "Invalid CC signature: SIGNATURE=0x%08X != expected=0x%08X\n",
signature_val, (u32)DX_DEV_SIGNATURE);
rc = -EINVAL;
goto post_clk_err;
}
- dev_dbg(dev, "CC SIGNATURE=0x%08X\n", signature_val);
+ dev_dbg(&plat_dev->dev, "CC SIGNATURE=0x%08X\n", signature_val);
/* Display HW versions */
- dev_info(dev, "ARM CryptoCell %s Driver: HW version 0x%08X, Driver version %s\n",
+ dev_info(&plat_dev->dev, "ARM CryptoCell %s Driver: HW version 0x%08X, Driver version %s\n",
SSI_DEV_NAME_STR,
CC_HAL_READ_REGISTER(CC_REG_OFFSET(HOST_RGF, HOST_VERSION)),
DRV_MODULE_VERSION);
rc = init_cc_regs(new_drvdata, true);
if (unlikely(rc != 0)) {
- dev_err(dev, "init_cc_regs failed\n");
+ dev_err(&plat_dev->dev, "init_cc_regs failed\n");
goto post_clk_err;
}
#ifdef ENABLE_CC_SYSFS
- rc = ssi_sysfs_init(&dev->kobj, new_drvdata);
+ rc = ssi_sysfs_init(&plat_dev->dev.kobj, new_drvdata);
if (unlikely(rc != 0)) {
- dev_err(dev, "init_stat_db failed\n");
+ dev_err(&plat_dev->dev, "init_stat_db failed\n");
goto post_regs_err;
}
#endif
rc = ssi_fips_init(new_drvdata);
if (unlikely(rc != 0)) {
- dev_err(dev, "SSI_FIPS_INIT failed 0x%x\n", rc);
+ dev_err(&plat_dev->dev, "SSI_FIPS_INIT failed 0x%x\n", rc);
goto post_sysfs_err;
}
rc = ssi_sram_mgr_init(new_drvdata);
if (unlikely(rc != 0)) {
- dev_err(dev, "ssi_sram_mgr_init failed\n");
+ dev_err(&plat_dev->dev, "ssi_sram_mgr_init failed\n");
goto post_fips_init_err;
}
new_drvdata->mlli_sram_addr =
ssi_sram_mgr_alloc(new_drvdata, MAX_MLLI_BUFF_SIZE);
if (unlikely(new_drvdata->mlli_sram_addr == NULL_SRAM_ADDR)) {
- dev_err(dev, "Failed to alloc MLLI Sram buffer\n");
+ dev_err(&plat_dev->dev, "Failed to alloc MLLI Sram buffer\n");
rc = -ENOMEM;
goto post_sram_mgr_err;
}
rc = request_mgr_init(new_drvdata);
if (unlikely(rc != 0)) {
- dev_err(dev, "request_mgr_init failed\n");
+ dev_err(&plat_dev->dev, "request_mgr_init failed\n");
goto post_sram_mgr_err;
}
rc = ssi_buffer_mgr_init(new_drvdata);
if (unlikely(rc != 0)) {
- dev_err(dev, "buffer_mgr_init failed\n");
+ dev_err(&plat_dev->dev, "buffer_mgr_init failed\n");
goto post_req_mgr_err;
}
rc = ssi_power_mgr_init(new_drvdata);
if (unlikely(rc != 0)) {
- dev_err(dev, "ssi_power_mgr_init failed\n");
+ dev_err(&plat_dev->dev, "ssi_power_mgr_init failed\n");
goto post_buf_mgr_err;
}
rc = ssi_ivgen_init(new_drvdata);
if (unlikely(rc != 0)) {
- dev_err(dev, "ssi_ivgen_init failed\n");
+ dev_err(&plat_dev->dev, "ssi_ivgen_init failed\n");
goto post_power_mgr_err;
}
/* Allocate crypto algs */
rc = ssi_ablkcipher_alloc(new_drvdata);
if (unlikely(rc != 0)) {
- dev_err(dev, "ssi_ablkcipher_alloc failed\n");
+ dev_err(&plat_dev->dev, "ssi_ablkcipher_alloc failed\n");
goto post_ivgen_err;
}
/* hash must be allocated before aead since hash exports APIs */
rc = ssi_hash_alloc(new_drvdata);
if (unlikely(rc != 0)) {
- dev_err(dev, "ssi_hash_alloc failed\n");
+ dev_err(&plat_dev->dev, "ssi_hash_alloc failed\n");
goto post_cipher_err;
}
rc = ssi_aead_alloc(new_drvdata);
if (unlikely(rc != 0)) {
- dev_err(dev, "ssi_aead_alloc failed\n");
+ dev_err(&plat_dev->dev, "ssi_aead_alloc failed\n");
goto post_hash_err;
}
@@ -392,7 +391,7 @@ static int init_cc_resources(struct platform_device *plat_dev)
post_clk_err:
cc_clk_off(new_drvdata);
post_drvdata_err:
- dev_err(dev, "ccree init error occurred!\n");
+ dev_err(&plat_dev->dev, "ccree init error occurred!\n");
return rc;
}
--
1.9.1
Hi Suniel,
On Wed, Oct 4, 2017 at 10:12 PM, <[email protected]> wrote:
> From: Suniel Mahesh <[email protected]>
>
> There is no need to create a local pointer variable "dev" and
> pass it various API's, instead use plat_dev which is enumerated
> by platform core on successful probe.
>
> Signed-off-by: Suniel Mahesh <[email protected]>
> ---
I'm sorry but I don't understand what is the problem you are trying to solve or
what is the improvement suggested.
Why is having a local variable undesirable? I think having it makes
the code more
readable, not less.
Thanks,
Gilad
> Note:
> - Patch was tested and built(ARCH=arm) on staging-testing.
> - No build issues reported, however it was not tested on
> real hardware.
> ---
> drivers/staging/ccree/ssi_driver.c | 63 +++++++++++++++++++-------------------
> 1 file changed, 31 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/staging/ccree/ssi_driver.c b/drivers/staging/ccree/ssi_driver.c
> index 5f03c25..eb907ce 100644
> --- a/drivers/staging/ccree/ssi_driver.c
> +++ b/drivers/staging/ccree/ssi_driver.c
> @@ -205,12 +205,11 @@ static int init_cc_resources(struct platform_device *plat_dev)
> struct resource *req_mem_cc_regs = NULL;
> void __iomem *cc_base = NULL;
> struct ssi_drvdata *new_drvdata;
> - struct device *dev = &plat_dev->dev;
> - struct device_node *np = dev->of_node;
> + struct device_node *np = plat_dev->dev.of_node;
> u32 signature_val;
> int rc = 0;
>
> - new_drvdata = devm_kzalloc(dev, sizeof(*new_drvdata), GFP_KERNEL);
> + new_drvdata = devm_kzalloc(&plat_dev->dev, sizeof(*new_drvdata), GFP_KERNEL);
> if (!new_drvdata) {
> rc = -ENOMEM;
> goto post_drvdata_err;
> @@ -225,16 +224,16 @@ static int init_cc_resources(struct platform_device *plat_dev)
> /* First CC registers space */
> req_mem_cc_regs = platform_get_resource(plat_dev, IORESOURCE_MEM, 0);
> /* Map registers space */
> - new_drvdata->cc_base = devm_ioremap_resource(dev, req_mem_cc_regs);
> + new_drvdata->cc_base = devm_ioremap_resource(&plat_dev->dev, req_mem_cc_regs);
> if (IS_ERR(new_drvdata->cc_base)) {
> - dev_err(dev, "Failed to ioremap registers");
> + dev_err(&plat_dev->dev, "Failed to ioremap registers");
> rc = PTR_ERR(new_drvdata->cc_base);
> goto post_drvdata_err;
> }
>
> - dev_dbg(dev, "Got MEM resource (%s): %pR\n", req_mem_cc_regs->name,
> + dev_dbg(&plat_dev->dev, "Got MEM resource (%s): %pR\n", req_mem_cc_regs->name,
> req_mem_cc_regs);
> - dev_dbg(dev, "CC registers mapped from %pa to 0x%p\n",
> + dev_dbg(&plat_dev->dev, "CC registers mapped from %pa to 0x%p\n",
> &req_mem_cc_regs->start, new_drvdata->cc_base);
>
> cc_base = new_drvdata->cc_base;
> @@ -242,120 +241,120 @@ static int init_cc_resources(struct platform_device *plat_dev)
> /* Then IRQ */
> new_drvdata->irq = platform_get_irq(plat_dev, 0);
> if (new_drvdata->irq < 0) {
> - dev_err(dev, "Failed getting IRQ resource\n");
> + dev_err(&plat_dev->dev, "Failed getting IRQ resource\n");
> rc = new_drvdata->irq;
> goto post_drvdata_err;
> }
>
> - rc = devm_request_irq(dev, new_drvdata->irq, cc_isr,
> + rc = devm_request_irq(&plat_dev->dev, new_drvdata->irq, cc_isr,
> IRQF_SHARED, "arm_cc7x", new_drvdata);
> if (rc) {
> - dev_err(dev, "Could not register to interrupt %d\n",
> + dev_err(&plat_dev->dev, "Could not register to interrupt %d\n",
> new_drvdata->irq);
> goto post_drvdata_err;
> }
> - dev_dbg(dev, "Registered to IRQ: %d\n", new_drvdata->irq);
> + dev_dbg(&plat_dev->dev, "Registered to IRQ: %d\n", new_drvdata->irq);
>
> rc = cc_clk_on(new_drvdata);
> if (rc)
> goto post_drvdata_err;
>
> - if (!dev->dma_mask)
> - dev->dma_mask = &dev->coherent_dma_mask;
> + if (!plat_dev->dev.dma_mask)
> + plat_dev->dev.dma_mask = &plat_dev->dev.coherent_dma_mask;
>
> - if (!dev->coherent_dma_mask)
> - dev->coherent_dma_mask = DMA_BIT_MASK(DMA_BIT_MASK_LEN);
> + if (!plat_dev->dev.coherent_dma_mask)
> + plat_dev->dev.coherent_dma_mask = DMA_BIT_MASK(DMA_BIT_MASK_LEN);
>
> /* Verify correct mapping */
> signature_val = CC_HAL_READ_REGISTER(CC_REG_OFFSET(HOST_RGF, HOST_SIGNATURE));
> if (signature_val != DX_DEV_SIGNATURE) {
> - dev_err(dev, "Invalid CC signature: SIGNATURE=0x%08X != expected=0x%08X\n",
> + dev_err(&plat_dev->dev, "Invalid CC signature: SIGNATURE=0x%08X != expected=0x%08X\n",
> signature_val, (u32)DX_DEV_SIGNATURE);
> rc = -EINVAL;
> goto post_clk_err;
> }
> - dev_dbg(dev, "CC SIGNATURE=0x%08X\n", signature_val);
> + dev_dbg(&plat_dev->dev, "CC SIGNATURE=0x%08X\n", signature_val);
>
> /* Display HW versions */
> - dev_info(dev, "ARM CryptoCell %s Driver: HW version 0x%08X, Driver version %s\n",
> + dev_info(&plat_dev->dev, "ARM CryptoCell %s Driver: HW version 0x%08X, Driver version %s\n",
> SSI_DEV_NAME_STR,
> CC_HAL_READ_REGISTER(CC_REG_OFFSET(HOST_RGF, HOST_VERSION)),
> DRV_MODULE_VERSION);
>
> rc = init_cc_regs(new_drvdata, true);
> if (unlikely(rc != 0)) {
> - dev_err(dev, "init_cc_regs failed\n");
> + dev_err(&plat_dev->dev, "init_cc_regs failed\n");
> goto post_clk_err;
> }
>
> #ifdef ENABLE_CC_SYSFS
> - rc = ssi_sysfs_init(&dev->kobj, new_drvdata);
> + rc = ssi_sysfs_init(&plat_dev->dev.kobj, new_drvdata);
> if (unlikely(rc != 0)) {
> - dev_err(dev, "init_stat_db failed\n");
> + dev_err(&plat_dev->dev, "init_stat_db failed\n");
> goto post_regs_err;
> }
> #endif
>
> rc = ssi_fips_init(new_drvdata);
> if (unlikely(rc != 0)) {
> - dev_err(dev, "SSI_FIPS_INIT failed 0x%x\n", rc);
> + dev_err(&plat_dev->dev, "SSI_FIPS_INIT failed 0x%x\n", rc);
> goto post_sysfs_err;
> }
> rc = ssi_sram_mgr_init(new_drvdata);
> if (unlikely(rc != 0)) {
> - dev_err(dev, "ssi_sram_mgr_init failed\n");
> + dev_err(&plat_dev->dev, "ssi_sram_mgr_init failed\n");
> goto post_fips_init_err;
> }
>
> new_drvdata->mlli_sram_addr =
> ssi_sram_mgr_alloc(new_drvdata, MAX_MLLI_BUFF_SIZE);
> if (unlikely(new_drvdata->mlli_sram_addr == NULL_SRAM_ADDR)) {
> - dev_err(dev, "Failed to alloc MLLI Sram buffer\n");
> + dev_err(&plat_dev->dev, "Failed to alloc MLLI Sram buffer\n");
> rc = -ENOMEM;
> goto post_sram_mgr_err;
> }
>
> rc = request_mgr_init(new_drvdata);
> if (unlikely(rc != 0)) {
> - dev_err(dev, "request_mgr_init failed\n");
> + dev_err(&plat_dev->dev, "request_mgr_init failed\n");
> goto post_sram_mgr_err;
> }
>
> rc = ssi_buffer_mgr_init(new_drvdata);
> if (unlikely(rc != 0)) {
> - dev_err(dev, "buffer_mgr_init failed\n");
> + dev_err(&plat_dev->dev, "buffer_mgr_init failed\n");
> goto post_req_mgr_err;
> }
>
> rc = ssi_power_mgr_init(new_drvdata);
> if (unlikely(rc != 0)) {
> - dev_err(dev, "ssi_power_mgr_init failed\n");
> + dev_err(&plat_dev->dev, "ssi_power_mgr_init failed\n");
> goto post_buf_mgr_err;
> }
>
> rc = ssi_ivgen_init(new_drvdata);
> if (unlikely(rc != 0)) {
> - dev_err(dev, "ssi_ivgen_init failed\n");
> + dev_err(&plat_dev->dev, "ssi_ivgen_init failed\n");
> goto post_power_mgr_err;
> }
>
> /* Allocate crypto algs */
> rc = ssi_ablkcipher_alloc(new_drvdata);
> if (unlikely(rc != 0)) {
> - dev_err(dev, "ssi_ablkcipher_alloc failed\n");
> + dev_err(&plat_dev->dev, "ssi_ablkcipher_alloc failed\n");
> goto post_ivgen_err;
> }
>
> /* hash must be allocated before aead since hash exports APIs */
> rc = ssi_hash_alloc(new_drvdata);
> if (unlikely(rc != 0)) {
> - dev_err(dev, "ssi_hash_alloc failed\n");
> + dev_err(&plat_dev->dev, "ssi_hash_alloc failed\n");
> goto post_cipher_err;
> }
>
> rc = ssi_aead_alloc(new_drvdata);
> if (unlikely(rc != 0)) {
> - dev_err(dev, "ssi_aead_alloc failed\n");
> + dev_err(&plat_dev->dev, "ssi_aead_alloc failed\n");
> goto post_hash_err;
> }
>
> @@ -392,7 +391,7 @@ static int init_cc_resources(struct platform_device *plat_dev)
> post_clk_err:
> cc_clk_off(new_drvdata);
> post_drvdata_err:
> - dev_err(dev, "ccree init error occurred!\n");
> + dev_err(&plat_dev->dev, "ccree init error occurred!\n");
> return rc;
> }
>
> --
> 1.9.1
>
--
Gilad Ben-Yossef
Chief Coffee Drinker
"If you take a class in large-scale robotics, can you end up in a
situation where the homework eats your dog?"
-- Jean-Baptiste Queru
On Thu, 2017-10-05 at 10:07 +0300, Gilad Ben-Yossef wrote:
> On Wed, Oct 4, 2017 at 10:12 PM, <[email protected]> wrote:
> > There is no need to create a local pointer variable "dev" and
> > pass it various API's, instead use plat_dev which is enumerated
> > by platform core on successful probe.
[]
> I'm sorry but I don't understand what is the problem you are trying to solve or
> what is the improvement suggested.
>
> Why is having a local variable undesirable? I think having it makes
> the code more readable, not less.
IMO: The local variable is _not_ undesirable.
It does make the code more readable and
shortens line lengths too.
On Thursday 05 October 2017 11:30 PM, Joe Perches wrote:
> On Thu, 2017-10-05 at 10:07 +0300, Gilad Ben-Yossef wrote:
>> On Wed, Oct 4, 2017 at 10:12 PM, <[email protected]> wrote:
>>> There is no need to create a local pointer variable "dev" and
>>> pass it various API's, instead use plat_dev which is enumerated
>>> by platform core on successful probe.
> []
>> I'm sorry but I don't understand what is the problem you are trying to solve or
>> what is the improvement suggested.
Hi Gilad and all,
Actually this patch is not a major improvement nor trying to solve some thing.
As struct device is a member of struct platform_device, I thought why can't we use
plat_dev (pointer to struct platform_device) through out the code. May be I was trying
to make code look compact, may be I am wrong.
>>
>> Why is having a local variable undesirable? I think having it makes
>> the code more readable, not less.
>
> IMO: The local variable is _not_ undesirable.
> It does make the code more readable and
> shortens line lengths too.
>
Yes, as you guys pointed out, we can use local variables. It is definitely making
the code more readable and shortening line lengths.
Please drop the patch, if this not helping the code look better.
Thanks
Suniel