| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <52B23618.2040508@broadcom.com>
Date: Wed, 18 Dec 2013 15:56:08 -0800
From: Sherman Yin <syin@...adcom.com>
To: Linus Walleij <linus.walleij@...aro.org>,
Mark Brown <broonie@...nel.org>
CC: Rob Herring <rob.herring@...xeda.com>,
Heiko Stübner
<heiko@...ech.de>, Pawel Moll <pawel.moll@....com>,
Mark Rutland <mark.rutland@....com>,
Stephen Warren <swarren@...dotorg.org>,
Ian Campbell <ijc+devicetree@...lion.org.uk>,
Rob Landley <rob@...dley.net>,
Christian Daudt <bcm@...thebug.org>,
Russell King <linux@....linux.org.uk>,
Grant Likely <grant.likely@...aro.org>,
Matt Porter <matt.porter@...aro.org>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
"linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
bcm-kernel-feedback-list <bcm-kernel-feedback-list@...adcom.com>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH v3 5/6] ARM: pinctrl: Add Broadcom Capri pinctrl driver
On 13-12-12 12:54 PM, Linus Walleij wrote:
>> +#define CAPRI_PIN_SHIFT(type, param) \
>> + (CAPRI_ ## type ## _PIN_REG_ ## param ## _SHIFT)
>> +
>> +#define CAPRI_PIN_MASK(type, param) \
>> + (CAPRI_ ## type ## _PIN_REG_ ## param ## _MASK)
>> +
>> +/* Macro to update reg with new pin config param */
>> +#define CAPRI_PIN_UPDATE(reg, type, param, val) \
>> + (((reg) & ~CAPRI_PIN_MASK(type, param)) | \
>> + (((val) << CAPRI_PIN_SHIFT(type, param)) & CAPRI_PIN_MASK(type, param)))
>
> Yuck! Are you sure you cannot convert these to static inlines and
> make them much simpler in the process?
>
> We do have an optimizing compiler, you don't need to do
> everything on one line... besides we're not on the hotpath.
If I were to convert the first 2 #defines to functions, it would either
be a 2-level switch statement or a 2D lookup table. IMO both of these
options are more difficult to read than this simple concatenation, so I
really rather keep them this way.
CAPRI_PIN_UPDATE, OTOH, doesn't require any concatenation so I can
easily make that into an inline.
>
>> +/*
>> + * Write to the register using the value and mask if current value is different
>> + */
>> +static void capri_reg_write(struct pinctrl_dev *pctldev,
>> + void __iomem *reg,
>> + u32 val,
>> + u32 mask)
>> +{
>> + u32 old_val;
>> + u32 new_val;
>> +
>> + old_val = readl(reg);
>> + new_val = (old_val & ~mask) | (val & mask);
>> +
>> + if (new_val == old_val) {
>> + dev_dbg(pctldev->dev,
>> + "Reg 0x%p=0x%x (no change)\n",
>> + reg, old_val);
>> + return;
>> + }
>> +
>> + dev_dbg(pctldev->dev,
>> + "Reg 0x%p change from 0x%x to 0x%x\n",
>> + reg, old_val, new_val);
>> + writel(new_val, reg);
>> +}
>
> This is a reimplementation of regmap for MMIO.
> See drivers/base/regmap/regmap-mmio.c
> Notice how regmap_update_bits() is used throughout the
> kernel.
>
> If you want to do this, use regmap.
Ok.
>
>> + case PIN_CONFIG_DRIVE_STRENGTH:
>> + /* Valid range is 2-16 mA, even numbers only */
>> + if ((arg < 2) || (arg > 16) || (arg % 2)) {
>> + dev_err(pctldev->dev,
>> + "Invalid Drive Strength value (%d) for "
>> + "pin %s (%d). Valid values are "
>> + "(2..16) mA, even numbers only.\n",
>> + arg, pdata->pins[pin].name, pin);
>> + return -EINVAL;
>> + }
>> + *val = CAPRI_PIN_UPDATE(*val, STD, DRV_STR, (arg/2)-1);
>> + *mask |= CAPRI_STD_PIN_REG_DRV_STR_MASK;
>> + break;
>
> Hm rather nice integer math...
Sorry, I can't tell if you are being sarcastic :) Drive strength is
represented by 3 bits in the register. If the values were 2-14 I could
have done some bit-checking instead of those 3 conditions in the if
statement. Or, if we use a enum of 0-7 then the check is much easier.
But as we discussed re: pull-up resistance, we rather let users specify
real numbers. The (arg/2)-1 is just to convert the mA into the 3 bits.
>> +/* Goes through the configs and update register val/mask */
>> +static int capri_i2c_pin_update(struct pinctrl_dev *pctldev,
>> + unsigned pin,
>> + unsigned long *configs,
>> + unsigned num_configs,
>> + u32 *val,
>> + u32 *mask)
>> +{
>> + struct capri_pinctrl_data *pdata = pinctrl_dev_get_drvdata(pctldev);
>> + int i;
>> + enum pin_config_param param;
>> + u16 arg;
>> +
>> + for (i = 0; i < num_configs; i++) {
>> + param = pinconf_to_config_param(configs[i]);
>> + arg = pinconf_to_config_argument(configs[i]);
>> +
>> + switch (param) {
>> + case PIN_CONFIG_BIAS_PULL_UP:
>> + if ((arg < 1) || (arg > 7)) {
>> + dev_err(pctldev->dev,
>> + "Invalid Pull Up value (%d) for pin %s "
>> + "(%d). Valid values are (1..7).\n",
>> + arg, pdata->pins[pin].name, pin);
>> + return -EINVAL;
>> + }
>
> No don't do that as mentioned in the other patch. Pass pull up strength
> in Ohms.
>
> Then have a translation table here, and do some best-effort fuzzy match.
Sure. I'm just going to error out if the user-supplied value is off,
since it is very clear what is acceptable from the binding documentation.
>> + /* Different pins have different configuration options */
>> + switch (pin_type) {
>> + case CAPRI_PIN_TYPE_STD:
>> + rc = capri_std_pin_update(pctldev, pin, configs, num_configs,
>> + &cfg_val, &cfg_mask);
>> + break;
>> +
>> + case CAPRI_PIN_TYPE_I2C:
>> + rc = capri_i2c_pin_update(pctldev, pin, configs, num_configs,
>> + &cfg_val, &cfg_mask);
>> + break;
>> +
>> + case CAPRI_PIN_TYPE_HDMI:
>> + rc = capri_hdmi_pin_update(pctldev, pin, configs, num_configs,
>> + &cfg_val, &cfg_mask);
>> + break;
>
> This is really nice and elegant.
Thanks. :)
Regards,
Sherman
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists