Thanks for your help.
> -----Original Message-----
> From: [email protected]
> [mailto:[email protected]] On Behalf Of Joe
> Perches
> Sent: Wednesday, June 24, 2015 3:51 PM
> To: Izumi, Taku/泉 拓
> Cc: [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected];
> [email protected]; [email protected]
> Subject: Re: [PATCH v2 01/22] fjes: Introduce FUJITSU Extended Socket Network
> Device driver
>
> On Wed, 2015-06-24 at 11:55 +0900, Taku Izumi wrote:
> > This patch adds the basic code of FUJITSU Extended Socket
> > Network Device driver.
>
> trivial notes:
>
> > diff --git a/drivers/net/fjes/fjes_main.c b/drivers/net/fjes/fjes_main.c
> []
> > +static int fjes_acpi_add(struct acpi_device *device)
> > +{
> []
> > + result =
> > + utf16s_to_utf8s((wchar_t *)str->string.pointer,
> > + str->string.length, UTF16_LITTLE_ENDIAN,
> > + str_buf, sizeof(str_buf) - 1);
>
> I dislike this style as it's not normally used in the kernel.
> Most would use another tab indent on the utf16s_to_utf8s line
> if the line didn't fit, but this would is more common:
To tell the truth, I dislike this style too.... OK, I'll fix.
I reformatted to fix " CHECK: Alignment should match open parenthesis"
style issue.
I thought the following style is easy to exceed 80 columns, I opted
the above style.
> result = utf16s_to_utf8s((wchar_t *)str->string.pointer,
> str->string.length, UTF16_LITTLE_ENDIAN,
> str_buf, sizeof(str_buf) - 1);
> >
> > + /* create platform_device */
> > + plat_dev =
> > + platform_device_register_simple(DRV_NAME, 0, fjes_resource,
> > + ARRAY_SIZE(fjes_resource));
>
> plat_dev = platform_device_register_simple(DRV_NAME, 0, fjes_resource,
> ARRAY_SIZE(fjes_resource));
>
> etc...
>
> > +static acpi_status
> > +fjes_get_acpi_resource(struct acpi_resource *acpi_res, void *data)
> > +{
> > + struct resource *res = data;
> > + struct acpi_resource_address32 *addr;
> > + struct acpi_resource_irq *irq;
> > +
> > + switch (acpi_res->type) {
> > + case ACPI_RESOURCE_TYPE_ADDRESS32:
> > + addr = &acpi_res->data.address32;
> > + res[0].start = addr->address.minimum;
> > + res[0].end = addr->address.minimum +
> > + addr->address.address_length - 1;
> > + break;
> > +
> > + case ACPI_RESOURCE_TYPE_IRQ:
> > + irq = &acpi_res->data.irq;
> > + if (irq->interrupt_count != 1)
> > + return AE_ERROR;
> > + res[1].start = irq->interrupts[0];
> > + res[1].end = irq->interrupts[0];
> > + break;
> > +
> > + default:
> > + break;
> > + }
> > +
> > + return AE_OK;
> > +}
>
> Is this function used in this compilation?
> If not, it's odd to add code that isn't used.
>
> Is it better to make the 2nd argument a
> struct resource * rather than a void * ?
This function is used as a callback function of "acpi_walk_resources".
Its signature is
acpi_status(*acpi_walk_resource_callback) (struct acpi_resource * resource,
void *context);
Sincerely,
Taku Izumi
>
> --
> To unsubscribe from this list: send the line "unsubscribe
> platform-driver-x86" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
