lkml.org 
[lkml]   [2013]   [Oct]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRE: [PATCH 3/4] ARM: pinctrl: Add Broadcom Capri pinctrl driver
    Date
    >> +config PINCTRL_CAPRI
    >> + bool "Broadcom Capri pinctrl driver"
    >> + select PINMUX
    >> + select PINCONF
    >> + help
    >> + Say Y here to support Broadcom Capri pinctrl driver, which is used for
    >> + the BCM281xx SoC family, including BCM11130, BCM11140, BCM11351,
    >> + BCM28145, and BCM28155 SoCs. This driver requires the pinctrl
    >> + framework. GPIO is provided by a separate GPIO driver.
    >
    >As mentioned this should be selecting and using GENERIC_PINCONF.
    >Basically I want that to happen before we look further at this code.
    >
    >(But I'll take a quick glance anyway...)

    Sure, working on it.

    >> +#define CAPRI_PINCONF_PACK(val, mask) (((val) << 16) | ((mask) & 0xffff))
    >> +#define CAPRI_PINCONF_UNPACK_VAL(conf) ((conf) >> 16)
    >> +#define CAPRI_PINCONF_UNPACK_MASK(conf) ((conf) & 0xffff)
    >
    >This custom horror goes away by using the macros from genric pin config
    ><linux/pinctrl/pinconf-generic.h> instead.

    Actually these are used differently than the pack/unpack functions in the pinconf-
    generic.h. These pack the bit value and bit mask to be written to a register,
    whereas the pinconf-generic ones pack the parameter id and value. But I get what
    you're saying and will try to reuse as much as possible from the pinconf generic
    functions and utils.

    >> +static const struct capri_cfg_param capri_pinconf_params[] = {
    >> + {"brcm,hysteresis", CAPRI_PINCONF_PARAM_HYST},
    >> + {"brcm,pull", CAPRI_PINCONF_PARAM_PULL},
    >> + {"brcm,slew", CAPRI_PINCONF_PARAM_SLEW},
    >> + {"brcm,input_dis", CAPRI_PINCONF_PARAM_INPUT_DIS},
    >> + {"brcm,drive_str", CAPRI_PINCONF_PARAM_DRV_STR},
    >> + {"brcm,pull_up_str", CAPRI_PINCONF_PARAM_PULL_UP_STR},
    >> + {"brcm,mode", CAPRI_PINCONF_PARAM_MODE},
    >> +};
    >
    >As well as all this stuff...

    OK. Will see if I can find something suitable for "input disable" and "mode"

    >> +/* Standard pin register */
    >
    >Add some more elaborate explanation here. I am half-guessing that
    >most of the registers are laid out like this and then there are
    >two exceptions, then write that right here in the comment.
    >
    >(BTW: this is a HW/driver detail and should not be written in the
    >device tree doc as was done in patch 2)

    Yes you guessed correctly. Most of the pin have bit-field definitions like
    the "standard" or "std" pin register. There are 12 pins that have the i2c
    definitions, and 2 pins that have the hdmi definitions. Will add
    explanation to the code.

    However, I wanted to explain this in the DT doc as well because, for example,
    setting a slew rate for an hdmi pin would be invalid.

    >> +/* Macro to update reg with new pin config param */
    >> +#define CAPRI_PIN_REG_SET(reg, type, param, val) \
    >> + (((reg) & ~CAPRI_ ## type ## _PIN_REG_ ## param ## _MASK) \
    >> + | (((val) << CAPRI_ ## type ## _PIN_REG_ ## param ## _SHIFT) \
    >> + & CAPRI_ ## type ## _PIN_REG_ ## param ## _MASK))
    >
    >No thanks.
    >
    >Rewrite this into a static inline which has the same effect when
    >compiling, but produces *WAY* more readable code than this
    >horrid thing. Plus you can type the arguments.

    Ok. The reason I used a macro is to take shortcuts given how the SHIFT
    and MASK #defines follow the format

    CAPRI_<pin type>_PIN_REG_<parameter name>_<mask or shift>

    I'll try to simply this.

    >> +#define _PIN(offset) (offset)
    >
    >This macro isn't exactly useful.
    >
    >> +/*
    >> + * Pin number definition. The order here must be the same as defined in the
    >> + * PADCTRLREG block in the RDB.
    >> + */
    >> +#define CAPRI_PIN_ADCSYNC _PIN(0)
    >
    >If it's just going to be like that, then skip the _PIN() macro.

    Removed. Was following pinctrl-tegra20.c where they have the _GPIO and _PIN
    macros. I guess it would be useful if we need to change the order of the enums
    in the future, but that's not the case at the moment for capri.

    >> +/* Process the pin configuration node */
    >> +static int capri_pinctrl_dt_node_to_map(struct pinctrl_dev *pctldev,
    >> + struct device_node *pnode,
    >> + struct pinctrl_map **map,
    >> + unsigned *nmaps)
    >> +{
    >
    >Make use of these from pinctrl-utils.c:
    >pinctrl_utils_reserve_map()
    >pinctrl_utils_add_map_mux()
    >pinctrl_utils_add_map_configs()
    >pinctrl_utils_dt_free_map()
    >
    >Then you get rid of another big chunk of boilerplate code.
    >
    >After these changes the driver will be much smaller and
    >cleaner and we can take a closer look.

    Yea actually the generic code is similar to what I have, there are some
    differences in terms of pre-allocating / re-allocating the pinctrl_maps.
    Seems like there are very few users of these functions at the moment.
    Anyway I'll try to make use of them.

    Thanks for the review.

    Regards,
    Sherman



    \
     
     \ /
      Last update: 2013-10-11 02:21    [W:5.139 / U:0.896 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site