Re: [PATCH v3 03/16] fpga: mgr: API change to replace fpga load functions with single function

[Alan Tull]

On Mon, Jul 10, 2017 at 12:36 PM, Moritz Fischer
<moritz.fisc...@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