Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1684559

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

Show all headers | View raw

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

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

[PATCH v3 00/16] Enable upper layers using FPGA region w/o device tree Alan Tull <atull@kernel.org> - 2017-07-06 20:50 +0200
  [PATCH v3 10/16] fpga: region: use image info as parameter for programming region Alan Tull <atull@kernel.org> - 2017-07-06 20:50 +0200
  [PATCH v3 15/16] fpga: region: add fpga_region_class_find Alan Tull <atull@kernel.org> - 2017-07-06 20:50 +0200
  [PATCH v3 01/16] doc: fpga: update documents for the FPGA API Alan Tull <atull@kernel.org> - 2017-07-06 20:50 +0200
  [PATCH v3 08/16] fpga: region: check for child regions before allocing image info Alan Tull <atull@kernel.org> - 2017-07-06 20:50 +0200
  [PATCH v3 16/16] fpga: region: move device tree support to of-fpga-region.c Alan Tull <atull@kernel.org> - 2017-07-06 20:50 +0200
  [PATCH v3 12/16] fpga: region: add fpga-region.h header Alan Tull <atull@kernel.org> - 2017-07-06 20:50 +0200
  [PATCH v3 05/16] fpga: region: use dev_err instead of pr_err Alan Tull <atull@kernel.org> - 2017-07-06 20:50 +0200
  [PATCH v3 03/16] fpga: mgr: API change to replace fpga load functions with single function Alan Tull <atull@kernel.org> - 2017-07-06 20:50 +0200
    Re: [PATCH v3 03/16] fpga: mgr: API change to replace fpga load  functions with single function Moritz Fischer <moritz.fischer@ettus.com> - 2017-07-10 19:40 +0200
      Re: [PATCH v3 03/16] fpga: mgr: API change to replace fpga load  functions with single function Alan Tull <atull@kernel.org> - 2017-07-10 20:30 +0200
      Re: [PATCH v3 03/16] fpga: mgr: API change to replace fpga load  functions with single function Alan Tull <atull@kernel.org> - 2017-07-12 18:40 +0200
  [PATCH v3 04/16] fpga: mgr: separate getting/locking FPGA manager Alan Tull <atull@kernel.org> - 2017-07-06 21:00 +0200
  [PATCH v3 02/16] fpga: bridge: support getting bridge from device Alan Tull <atull@kernel.org> - 2017-07-06 21:00 +0200
  [PATCH v3 06/16] fpga: region: remove unneeded of_node_get and put Alan Tull <atull@kernel.org> - 2017-07-06 21:00 +0200

csiph-web