lkml.org 
[lkml]   [2017]   [Jul]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH v3 03/16] fpga: mgr: API change to replace fpga load functions with single function
On Mon, Jul 10, 2017 at 12:36 PM, Moritz Fischer
<moritz.fischer@ettus.com> wrote:

Hi Moritz,

Thanks for looking at this stuff.

>>
>> +struct fpga_image_info *fpga_image_info_alloc(struct device *dev)
>> +{
>> + struct fpga_image_info *info;
>> +
>> + info = devm_kzalloc(dev, sizeof(*info), GFP_KERNEL);
>> + if (!info)
>> + return ERR_PTR(-ENOMEM);
>
> Doesn't this make it more complex?

Yes!

> If in the end you'll anyway have to check
> if IS_ERR_OR_NULL()? As opposed to just checking if (!info) on the returned
> value.
>
> Just a thought.

Yes, I agree.

>> diff --git a/drivers/fpga/fpga-region.c b/drivers/fpga/fpga-region.c
>> index 655940f..c71c9cb 100644
>> --- a/drivers/fpga/fpga-region.c
>> +++ b/drivers/fpga/fpga-region.c
>> @@ -226,14 +226,11 @@ static int fpga_region_get_bridges(struct fpga_region *region,
>> /**
>> * fpga_region_program_fpga - program FPGA
>> * @region: FPGA region
>> - * @firmware_name: name of FPGA image firmware file
>> * @overlay: device node of the overlay
>> - * Program an FPGA using information in the device tree.
>> - * Function assumes that there is a firmware-name property.
>> + * Program an FPGA using information in the region's fpga image info.
>> * Return 0 for success or negative error code.
>> */
>> static int fpga_region_program_fpga(struct fpga_region *region,
>> - const char *firmware_name,
>> struct device_node *overlay)
>> {
>> struct fpga_manager *mgr;
>> @@ -264,7 +261,7 @@ static int fpga_region_program_fpga(struct fpga_region *region,
>> goto err_put_br;
>> }
>>
>> - ret = fpga_mgr_firmware_load(mgr, region->info, firmware_name);
>> + ret = fpga_mgr_load(mgr, region->info);
>> if (ret) {
>> pr_err("failed to load fpga image\n");
>> goto err_put_br;
>> @@ -357,16 +354,15 @@ static int child_regions_with_firmware(struct device_node *overlay)
>> static int fpga_region_notify_pre_apply(struct fpga_region *region,
>> struct of_overlay_notify_data *nd)
>> {
>> - const char *firmware_name = NULL;
>> + struct device *dev = &region->dev;
>> struct fpga_image_info *info;
>> + const char *firmware_name;
>> int ret;
>>
>> - info = devm_kzalloc(&region->dev, sizeof(*info), GFP_KERNEL);
>> + info = fpga_image_info_alloc(dev);
>> if (!info)
>> return -ENOMEM;
>
> Can't you return PTR_ERR(info) here (If you don't follow my suggestion above),
> or keep it the same here and follow my suggestion ;-)

I see I fell exactly into the trap :) I set for myself.

I'll fix the return value of fpga_image_info_alloc() as you suggested above.

Alan

\
 
 \ /
  Last update: 2017-07-10 20:24    [W:0.070 / U:1.260 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site