lkml.org 
[lkml]   [2017]   [Apr]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH v6 1/5] firmware: add extensible driver data params
    On Wed, 2017-03-29 at 20:25 -0700, Luis R. Rodriguez wrote:
    > As the firmware API evolves we keep extending functions with more arguments.
    > Stop this nonsense by proving an extensible data structure which can be used
    > to represent both user parameters and private internal parameters.
    >
    > We introduce 3 data structures:
    >
    > o struct driver_data_req_params - used for user specified parameters
    > o struct driver_data_priv_params - used for internal use
    > o struct driver_data_params - stiches both of the the above together,
    > also only for internal use
    >
    > This starts off by just making the existing APIs use the new data
    > structures, it will make subsequent changes easier to review which will
    > be adding new flexible APIs.
    >
    > This commit should introduces no functional changes (TM).
    >
    > Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
    > ---

    Looks fine with a few nitpicks.


    > diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
    > index 54fc4c42f126..f702566554e1 100644
    > --- a/drivers/base/firmware_class.c
    > +++ b/drivers/base/firmware_class.c

    [...]

    > @@ -40,6 +41,77 @@ MODULE_AUTHOR("Manuel Estrada Sainz");
    > MODULE_DESCRIPTION("Multi purpose firmware loading support");
    > MODULE_LICENSE("GPL");
    >
    > +struct driver_data_priv_params {
    > + bool use_fallback;
    > + bool use_fallback_uevent;
    > + bool no_cache;
    > + void *alloc_buf;
    > + size_t alloc_buf_size;
    > +};
    > +
    > +struct driver_data_params {
    > + const struct firmware *driver_data;
    > + const struct driver_data_req_params req_params;
    > + struct driver_data_priv_params priv_params;
    > +};
    > +
    > +/*
    > + * These are kept to remain backward compatible with old behaviour. Do not
    > + * modify them unless you know what you are doing. These are to be used only
    > + * by the old API, so:
    > + *
    > + * Old sync APIs:
    > + * o request_firmware(): __DATA_REQ_FIRMWARE()
    > + * o request_firmware_direct(): __DATA_REQ_FIRMWARE_DIRECT()
    > + * o request_firmware_into_buf(): __DATA_REQ_FIRMWARE_BUF()
    > + *
    > + * Old async API:
    > + * o request_firmware_nowait(): __DATA_REQ_FIRMWARE_NOWAIT()
    > + */
    > +#define __DATA_REQ_FIRMWARE() \
    > + .priv_params = { \
    > + .use_fallback = !!FW_OPT_FALLBACK, \

    use_fallback is a boolean, so you don't need the double negation here.


    [...]

    > @@ -1332,12 +1433,15 @@ request_firmware_into_buf(const struct firmware **firmware_p, const char *name,
    > struct device *device, void *buf, size_t size)
    > {
    > int ret;
    > + struct driver_data_params data_params = {
    > + __DATA_REQ_FIRMWARE_BUF(buf, size),
    > + };
    >
    > __module_get(THIS_MODULE);
    > - ret = _request_firmware(firmware_p, name, device, buf, size,
    > - FW_OPT_UEVENT | FW_OPT_FALLBACK |
    > - FW_OPT_NOCACHE);
    > + ret = _request_firmware(firmware_p, name, &data_params, device);
    > module_put(THIS_MODULE);
    > +
    > +

    Double empty-lines here.


    > return ret;
    > }
    > EXPORT_SYMBOL(request_firmware_into_buf);
    >

    [...]

    > diff --git a/include/linux/driver_data.h b/include/linux/driver_data.h
    > new file mode 100644
    > index 000000000000..c3d3a4129838
    > --- /dev/null
    > +++ b/include/linux/driver_data.h
    > @@ -0,0 +1,88 @@
    > +#ifndef _LINUX_DRIVER_DATA_H
    > +#define _LINUX_DRIVER_DATA_H
    > +
    > +#include <linux/types.h>
    > +#include <linux/compiler.h>
    > +#include <linux/gfp.h>
    > +#include <linux/device.h>
    > +
    > +/*
    > + * Driver Data internals
    > + *
    > + * Copyright (C) 2017 Luis R. Rodriguez <mcgrof@kernel.org>
    > + *
    > + * This program is free software; you can redistribute it and/or modify it
    > + * under the terms of copyleft-next (version 0.3.1 or later) as published
    > + * at http://copyleft-next.org/.
    > + */
    > +
    > +/**
    > + * enum driver_data_mode - driver data mode of operation
    > + *
    > + * DRIVER_DATA_SYNC: your call to request driver data is synchronous. We will
    > + * look for the driver data file you have requested immediatley.
    > + * DRIVER_DATA_ASYNC: your call to request driver data is asynchronous. We will

    It should be @DRIVER_DATA_SYNC and @DRIVER_DATA_ASYNC here.

    --
    Cheers,
    Luca.

    \
     
     \ /
      Last update: 2017-04-06 09:28    [W:3.954 / U:0.248 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site