Attention is currently required from: Angel Pons.
12 comments:
Commit Message:
Patch Set #5, Line 11: kempel/kempeld
typos? `kempld/kempld`
It was removed from the message
Patch Set #5, Line 12: configure/manage each pad using the API
Are there plans to use this API?
Yes, but I decided to do it in a separate patch.
Sometimes carrier board manufacturers need to read the ID, and the easiest way is to use GPIO. In this case, the COMe board uses only GPIO from CPLD.
File src/ec/kontron/kempld/chip.h:
Patch Set #5, Line 7: KEMPLD_NUM_GPIO_PADS
Why not `KEMPLD_NUM_GPIOS`?
KEMPLD_NUM_GPIOS
Done
Also, I have specified the number of pads - 16. But on my board, I can only check the 8 that are used in the connectors of the COMe module.
#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? […]
Ok, done. Thanks
Patch Set #5, Line 38: gpio_pad
I'd call this `gpios`. Also, the type should be `enum kempld_gpio_mode`.
I would prefer gpio in the option name (array), if you don't mind?
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?
Yes, from the kernel. In the beginning I tried to keep this code unchanged (the following comments are related to this reason). After the fixes, this is no longer required.
Patch Set #5, Line 16: offset
Why is this called `offset`? Unless I'm missing something, it's just the GPIO index?
I would prefer pin_num as the variable name.
Patch Set #5, Line 18: int status
Why is this an int (should be u8)? Why is it called `status`?
Reworked.
Are these casts needed? I guess they might be because `offset` is an `unsigned int`.
Reworked. At the moment, this is not needed, since the variables already have the type u8.
Patch Set #5, Line 33: int status
Why is this an int (should be u8)? Why is it called `status`?
Reworked
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: […]
Thanks for your time on this work, but I would like to separate operations and use functions, if you don't mind? Perhaps this is only the opinion of the author, but in this case, the code will be easier to read if it is divided into functions )
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: […]
Done. Thanks for your help on this.
To view, visit change 47595. To unsubscribe, or for help writing mail filters, visit settings.