On Wed, Feb 05, 2025 at 10:15:45AM +0000, Oleksii Moisieiev wrote:
...
> +#define RP1_PAD_DRIVE_LSB 4
> +#define RP1_PAD_IN_ENABLE_MASK 0x00000040
> +#define RP1_PAD_IN_ENABLE_LSB 6
> +#define RP1_PAD_OUT_DISABLE_MASK 0x00000080
> +#define RP1_PAD_OUT_DISABLE_LSB 7
> +
> +#define RP1_PAD_DRIVE_2MA 0x00000000
> +#define RP1_PAD_DRIVE_4MA 0x00000010
> +#define RP1_PAD_DRIVE_8MA 0x00000020
> +#define RP1_PAD_DRIVE_12MA 0x00000030
Can we use enumerator for these, instead of preprocessor macros?
Also in addition to BI(), can we use GENMASK() and FIELD_PREP_CONST(),
i.e.:
enum {
...
RP1_PAD_DRIVE_MASK = GENMASK(7, 4),
RP1_PAD_DRIVE_2MA = FIELD_PREP_CONST(RP1_PAD_DRIVE_MASK, 0),
RP1_PAD_DRIVE_4MA = FIELD_PREP_CONST(RP1_PAD_DRIVE_MASK, 1),
RP1_PAD_DRIVE_8MA = FIELD_PREP_CONST(RP1_PAD_DRIVE_MASK, 2),
RP1_PAD_DRIVE_12MA = FIELD_PREP_CONST(RP1_PAD_DRIVE_MASK, 3),
...
};
You need to import FIELD_PREP_CONST() into include/linux/bitfield.h,
from Linux sources, since we don't have it in U-Boot yet.
> +#define FLD_GET(r, f) (((r) & (f ## _MASK)) >> (f ## _LSB))
> +#define FLD_SET(r, f, v) r = (((r) & ~(f ## _MASK)) | ((v) << (f ## _LSB)))
Use FIELD_GET and FIELD_SET.
...
> +static void rp1_pad_update(struct rp1_pin_info *pin, u32 clr, u32 set)
> +{
> + u32 padctrl = readl(pin->pad);
> +
> + padctrl &= ~clr;
> + padctrl |= set;
> +
> + writel(padctrl, pin->pad);
> +}
Instead of implementing your own function, you can just use
clrsetbits_le32() ...
...
> +static void rp1_set_fsel(struct rp1_pin_info *pin, u32 fsel)
> +{
> + u32 ctrl = readl(pin->gpio + RP1_GPIO_CTRL);
> +
> + if (fsel >= RP1_FSEL_COUNT)
> + fsel = RP1_FSEL_NONE_HW;
> +
> + rp1_input_enable(pin, 1);
> + rp1_output_enable(pin, 1);
Shouldn't this be done with one read-modify-write? It seems to be doing
two read-modify-writes to one register.
> +
> + if (fsel == RP1_FSEL_NONE) {
> + FLD_SET(ctrl, RP1_GPIO_CTRL_OEOVER, RP1_OEOVER_DISABLE);
> + } else {
> + FLD_SET(ctrl, RP1_GPIO_CTRL_OUTOVER, RP1_OUTOVER_PERI);
> + FLD_SET(ctrl, RP1_GPIO_CTRL_OEOVER, RP1_OEOVER_PERI);
> + }
> + FLD_SET(ctrl, RP1_GPIO_CTRL_FUNCSEL, fsel);
> +
> + writel(ctrl, pin->gpio + RP1_GPIO_CTRL);
> +}
> +
> +static int rp1_get_dir(struct rp1_pin_info *pin)
> +{
> + return !(readl(pin->rio + RP1_RIO_OE) & (1 << pin->offset)) ?
> + RP1_DIR_INPUT : RP1_DIR_OUTPUT;
Why use own constants (RP1_DIR_INPUT/OUTPUT) ?
1 << pin->offset should be BIT(pin->offset). This happens more time in
the subsequent code.
> +}
> +
> +static void rp1_set_dir(struct rp1_pin_info *pin, bool is_input)
> +{
> + int offset = is_input ? RP1_CLR_OFFSET : RP1_SET_OFFSET;
size_t offset
> +
> + writel(1 << pin->offset, pin->rio + RP1_RIO_OE + offset);
> +}
> +
> +static int rp1_get_value(struct rp1_pin_info *pin)
> +{
> + return !!(readl(pin->rio + RP1_RIO_IN) & (1 << pin->offset));
> +}
> +
> +static void rp1_set_value(struct rp1_pin_info *pin, int value)
> +{
> + /* Assume the pin is already an output */
> + writel(1 << pin->offset,
> + pin->rio + RP1_RIO_OUT + (value ? RP1_SET_OFFSET :
> RP1_CLR_OFFSET));
In rp1_set_dir() you have helper variable offset, why not here?
> +}
> +
> +static int rp1_gpio_get(struct udevice *dev, unsigned int offset)
> +{
> + struct rp1_pin_info *pin = rp1_get_pin(dev, offset);
> + int ret;
> +
> + if (!pin)
> + return -EINVAL;
> +
> + ret = rp1_get_value(pin);
> + return ret;
return rp1_get_value(pin); ? And you can drop the ret variable and save
two lines.
> +}
> +
> +static int rp1_gpio_set(struct udevice *dev, unsigned int offset, int value)
> +{
> + struct rp1_pin_info *pin = rp1_get_pin(dev, offset);
> +
> + if (pin)
> + rp1_set_value(pin, value);
Shouldn't this return some error if pin is NULL?
> + return 0;
> +}
> +
> +static int rp1_gpio_direction_input(struct udevice *dev, unsigned int offset)
> +{
> + struct rp1_pin_info *pin = rp1_get_pin(dev, offset);
> +
> + if (!pin)
> + return -EINVAL;
> +
> + rp1_set_dir(pin, RP1_DIR_INPUT);
> + rp1_set_fsel(pin, RP1_FSEL_GPIO);
Empty line before return.
> + return 0;
> +}
> +
> +static int rp1_gpio_direction_output(struct udevice *dev, unsigned int
> offset, int value)
> +{
> + struct rp1_pin_info *pin = rp1_get_pin(dev, offset);
> +
> + if (!pin)
> + return -EINVAL;
> +
> + rp1_set_value(pin, value);
> + rp1_set_dir(pin, RP1_DIR_OUTPUT);
> + rp1_set_fsel(pin, RP1_FSEL_GPIO);
Empty line before return.
> + return 0;
> +}
> +
> +static int rp1_gpio_get_direction(struct udevice *dev, unsigned int offset)
> +{
> + struct rp1_pin_info *pin = rp1_get_pin(dev, offset);
> + u32 fsel;
> +
> + if (!pin)
> + return -EINVAL;
> +
> + fsel = rp1_get_fsel(pin);
> + if (fsel != RP1_FSEL_GPIO)
> + return -EINVAL;
> +
> + return (rp1_get_dir(pin) == RP1_DIR_OUTPUT) ?
> + GPIOF_OUTPUT :
> + GPIOF_INPUT;
rp1_get_dir() could have already returned GPIOF_INPUT/OUTPUT instead of
the custom constants.
> +}
> +
> +static const struct dm_gpio_ops rp1_gpio_ops = {
> + .direction_input = rp1_gpio_direction_input,
> + .direction_output = rp1_gpio_direction_output,
> + .get_value = rp1_gpio_get,
> + .set_value = rp1_gpio_set,
> + .get_function = rp1_gpio_get_direction,
> +};
Align the '=' with tabs.
> +
> +static int rp1_gpio_probe(struct udevice *dev)
> +{
> + int i;
> + struct gpio_dev_priv *uc_priv = dev_get_uclass_priv(dev);
> + struct rp1_gpio_priv *priv = dev_get_priv(dev);
Reverse christmas tree variables declaration if possible:
struct gpio_dev_priv *uc_priv = dev_get_uclass_priv(dev);
struct rp1_gpio_priv *priv = dev_get_priv(dev);
int i;
But you don't need declaring i here, see below.
> +
> + priv->gpio_base = dev_remap_addr_index(dev, 0);
> + if (!priv->gpio_base)
> + return -EINVAL;
> +
> + priv->rio_base = dev_remap_addr_index(dev, 1);
> + if (!priv->rio_base)
> + return -EINVAL;
> +
> + priv->pads_base = dev_remap_addr_index(dev, 2);
> + if (!priv->pads_base)
> + return -EINVAL;
> +
> + uc_priv->gpio_count = RP1_NUM_GPIOS;
> + uc_priv->bank_name = dev->name;
> +
> + for (i = 0; i < RP1_NUM_BANKS; i++) {
for (int i = 0; ...)
> + const struct rp1_iobank_desc *bank = &rp1_iobanks[i];
> + int j;
> +
> + for (j = 0; j < bank->num_gpios; j++) {
for (int j = 0; ...)
> + struct rp1_pin_info *pin =
> + &priv->pins[bank->min_gpio + j];
> +
> + pin->num = bank->min_gpio + j;
> + pin->bank = i;
> + pin->offset = j;
> +
> + pin->gpio = priv->gpio_base + bank->gpio_offset +
> + j * sizeof(u32) * 2;
> + pin->inte = priv->gpio_base + bank->inte_offset;
> + pin->ints = priv->gpio_base + bank->ints_offset;
> + pin->rio = priv->rio_base + bank->rio_offset;
> + pin->pad = priv->pads_base + bank->pads_offset +
> + j * sizeof(u32);
> + }
> + }
> +
> + return 0;
> +}
> +
> +static const struct udevice_id rp1_gpio_ids[] = {
> + { .compatible = "raspberrypi,rp1-gpio" },
> + { /* sentinel */ }
No need to write the sentinel comment. IMO we should drop this from all
drivers that use it.
> +};
> +
> +U_BOOT_DRIVER(rp1_gpio) = {
> + .name = "rp1-gpio",
> + .id = UCLASS_GPIO,
> + .of_match = rp1_gpio_ids,
> + .ops = &rp1_gpio_ops,
> + .priv_auto = sizeof(struct rp1_gpio_priv),
> + .probe = rp1_gpio_probe,
Align '=' with tabs.
Marek
