Attention is currently required from: Felix Singer, Nico Huber, Maxim Polyakov, Werner Zeh.
16 comments:
Commit Message:
Patch Set #5, Line 11: kempel/kempeld
typos? `kempld/kempld`
Patch Set #5, Line 12: configure/manage each pad using the API
Are there plans to use this API?
File src/ec/kontron/kempld/chip.h:
Patch Set #5, Line 7: KEMPLD_NUM_GPIO_PADS
Why not `KEMPLD_NUM_GPIOS`?
#define KEMPLD_PAD_GPIO_DIR_IN 0
#define KEMPLD_PAD_GPIO_DIR_OUT 1
#define KEMPLD_PAD_GPIO_CFG_DIR(in_out) ((KEMPLD_PAD_GPIO_DIR_##in_out) << 7)
#define KEMPLD_PAD_GPIO_CFG_VAL(v) (v)
#define KEMPLD_PAD_GPIO_CFG_IN KEMPLD_PAD_GPIO_CFG_DIR(IN)
#define KEMPLD_PAD_GPIO_CFG_OUT(v) \
(KEMPLD_PAD_GPIO_CFG_DIR(OUT) | KEMPLD_PAD_GPIO_CFG_VAL(v))
Do we really need to use macros for this? Why not go with enums, like the rest of the code does?
enum kempld_gpio_mode {
KEMPLD_GPIO_DEFAULT,
KEMPLD_GPIO_INPUT,
KEMPLD_GPIO_OUTPUT_LOW,
KEMPLD_GPIO_OUTPUT_HIGH,
};
The `KEMPLD_GPIO_DEFAULT` value allows skipping config for unspecified GPIOs, which preserves current behavior.
Patch Set #5, Line 38: gpio_pad
I'd call this `gpios`. Also, the type should be `enum kempld_gpio_mode`.
File src/ec/kontron/kempld/kempld.c:
Patch Set #5, Line 99: GPIOs configuration
nit: either `GPIO configuration` or `Configuring GPIOs`. I prefer the former.
else, warn? it would be consistent with the code above:
} else {
printk(BIOS_WARNING, "KEMPLD: Spurious GPIO device %s.\n", dev_path(dev));
}
File src/ec/kontron/kempld/kempld_gpio.c:
Patch Set #5, Line 6: * Author: Michael Brunner <michael.brunner@kontron.com>
Is this comment because the code was taken from the Linux kernel? Do we need it?
Patch Set #5, Line 16: offset
Why is this called `offset`? Unless I'm missing something, it's just the GPIO index?
Patch Set #5, Line 18: int status
Why is this an int (should be u8)? Why is it called `status`?
Are these casts needed? I guess they might be because `offset` is an `unsigned int`.
Patch Set #5, Line 33: int status
Why is this an int (should be u8)? Why is it called `status`?
int kempld_gpio_direction_input(unsigned int offset)
{
int status;
if (kempld_get_mutex(100) < 0)
return -1;
status = kempld_read8((u8)KEMPLD_GPIO_DIR_NUM(offset));
status &= ~KEMPLD_GPIO_MASK(offset);
kempld_write8((u8)KEMPLD_GPIO_DIR_NUM(offset), (u8)status);
kempld_release_mutex();
return 0;
}
int kempld_gpio_direction_output(unsigned int offset, int value)
{
int status;
if (kempld_get_mutex(100) < 0)
return -1;
status = kempld_read8((u8)KEMPLD_GPIO_LVL_NUM(offset));
if (value)
status |= KEMPLD_GPIO_MASK(offset);
else
status &= ~KEMPLD_GPIO_MASK(offset);
kempld_write8((u8)KEMPLD_GPIO_LVL_NUM(offset), (u8)status);
status = kempld_read8((u8)KEMPLD_GPIO_DIR_NUM(offset));
status |= KEMPLD_GPIO_MASK(offset);
kempld_write8((u8)KEMPLD_GPIO_DIR_NUM(offset), (u8)status);
kempld_release_mutex();
return 0;
}
This would need to be adapted to use the enum type I suggested in another comment:
enum kempld_gpio_mode {
KEMPLD_GPIO_DEFAULT,
KEMPLD_GPIO_INPUT,
KEMPLD_GPIO_OUTPUT_LOW,
KEMPLD_GPIO_OUTPUT_HIGH,
};
With the enum, one function is enough:
int kempld_configure_gpio(unsigned int gpio, enum kempld_gpio_mode mode)
{
const u8 mask = KEMPLD_GPIO_MASK(gpio);
u8 dir, lvl, val;
switch (mode) {
case KEMPLD_GPIO_DEFAULT:
return 0;
case KEMPLD_GPIO_INPUT:
dir = 0;
lvl = 0;
break;
case KEMPLD_GPIO_OUTPUT_LOW:
dir = mask;
lvl = 0;
break;
case KEMPLD_GPIO_OUTPUT_HIGH:
dir = mask;
lvl = mask;
break;
default:
return -1;
}
if (kempld_get_mutex(100) < 0)
return -1;
/* For outputs, set level before direction to avoid glitches */
if (dir) {
val = kempld_read8((u8)KEMPLD_GPIO_LVL_NUM(gpio));
val &= ~mask;
val |= lvl;
kempld_write8((u8)KEMPLD_GPIO_LVL_NUM(gpio), val);
}
val = kempld_read8((u8)KEMPLD_GPIO_DIR_NUM(gpio));
val &= ~mask;
val |= dir;
kempld_write8((u8)KEMPLD_GPIO_DIR_NUM(gpio), val);
kempld_release_mutex();
return 0;
}
I know the switch-block is rather verbose. I did it on purpose, I feel it's clearer this way.
int kempld_gpio_pad_config(struct device *const dev)
{
const struct ec_kontron_kempld_config *const config = dev->chip_info;
int i, status;
if (!config)
return -1;
for (i = 0; i < KEMPLD_NUM_GPIO_PADS; ++i) {
if (config->gpio_pad[i] & KEMPLD_PAD_GPIO_CFG_DIR(OUT))
status = kempld_gpio_direction_output(i, config->gpio_pad[i] & 0x1);
else
status = kempld_gpio_direction_input(i);
if (status < 0)
return status;
}
return 0;
}
With the changes suggested above, I'd rewrite this as follows:
int kempld_gpio_pad_config(struct device *const dev)
{
const struct ec_kontron_kempld_config *const config = dev->chip_info;
if (!config)
return -1;
for (unsigned int i = 0; i < KEMPLD_NUM_GPIO_PADS; ++i) {
if (kempld_configure_gpio(i, config->gpios[i]) < 0)
return -1;
}
return 0;
}
File src/ec/kontron/kempld/kempld_internal.h:
Given that `KEMPLD_NUM_GPIO_PADS` is 8, the parametrization seems pointless. Or do other kempld's support more than 8 GPIOs? If so, I wouldn't define two macros for each register:
#define KEMPLD_GPIO_DIR(gpio) (0x40 + (gpio) / 8)
#define KEMPLD_GPIO_LVL(gpio) (0x42 + (gpio) / 8)
nit: spaces around `/`
To view, visit change 47595. To unsubscribe, or for help writing mail filters, visit settings.