lkml.org 
[lkml]   [2017]   [Jan]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH V10 2/3] ACPI: Add support for ResourceSource/IRQ domain mapping
Hi Andy,

On 2017-01-18 14:45, Andy Shevchenko wrote:
> On Wed, Jan 18, 2017 at 9:04 PM, Agustin Vega-Frias
> <agustinv@codeaurora.org> wrote:
>
> [...]
>
>
>> + */
>> +static struct fwnode_handle *
>> +acpi_get_irq_source_fwhandle(const struct acpi_resource_source
>> *source)
>> +{
>> + struct fwnode_handle *result;
>> + struct acpi_device *device;
>> + acpi_handle handle;
>> + acpi_status status;
>> +
>> + if (!source->string_length)
>> + return acpi_gsi_domain_id;
>> +
>> + status = acpi_get_handle(NULL, source->string_ptr, &handle);
>> + if (ACPI_FAILURE(status)) {
>
>> + pr_warn("Could not find handle for %s\n",
>> source->string_ptr);
>> + return NULL;
>> + }
>> +
>> + device = acpi_bus_get_acpi_device(handle);
>> + if (!device) {
>> + pr_warn("Could not get device for %s\n",
>> source->string_ptr);
>
> I'm not sure both messages have a value.

I'll probably just drop the explicit pr_warns and just use WARN_ON on
the if condition. Is that a reasonable alternative in your view?

>
>> + return NULL;
>> + }
>
>
> [...]
>
>> + */
>> +static int acpi_irq_parse_one(acpi_handle handle, unsigned int index,
>> + struct irq_fwspec *fwspec, unsigned long
>> *flags)
>> +{
>> + struct acpi_irq_parse_one_ctx ctx = { -EINVAL, index, flags,
>> fwspec };
>> + acpi_status status;
>> +
>> + status = acpi_walk_resources(handle, METHOD_NAME__CRS,
>> + acpi_irq_parse_one_cb, &ctx);
>
>> + if (ACPI_FAILURE(status))
>> + return -EINVAL;
>
> Shouldn't you have the same in rc? Would be redundant.
>

Good point, since we always return -EINVAL on failure we can skip
the check since rc will only be updated on success.

>> + return ctx.rc;
>> +}
>> +
>
> [...]
>
>> + domain = irq_find_matching_fwnode(fwspec.fwnode,
>> DOMAIN_BUS_ANY);
>> + if (!domain)
>> + return -EPROBE_DEFER;
>
> Hmm... Could it be other issues here?

I'll comment on this on Lorenzo's reply.

>
>> +
>> + rc = irq_create_fwspec_mapping(&fwspec);
>> + if (rc <= 0)
>> + return -EINVAL;
>> +
>
>> + res->start = rc;
>> + res->end = rc;
>> + res->flags = flags;
>
> Perhaps struct resource *r should be a parameter to
> acpi_irq_parse_one().

I'll comment on this on Lorenzo's reply.

>
>> +
>> + return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(acpi_irq_get);
>> +
>> +/**
>> * acpi_set_irq_model - Setup the GSI irqdomain information
>> * @model: the value assigned to acpi_irq_model
>> * @fwnode: the irq_domain identifier for mapping and looking up

I'll address the other issues on V11.

Thanks for your thorough review,
Agustin

>
> --
> With Best Regards,
> Andy Shevchenko

--
Qualcomm Datacenter Technologies, Inc. on behalf of the Qualcomm
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a
Linux Foundation Collaborative Project.

\
 
 \ /
  Last update: 2017-01-19 16:41    [W:0.094 / U:0.052 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site