> Could you please have a look at this patch?
Sure!
> +static void mlxbf3_gpio_set(struct gpio_chip *chip, unsigned int
> +offset, int val) {
Put opening bracket on new line.
Run your code through scripts/checkpatch.pl before submitting.
> + struct mlxbf3_gpio_context *gs = gpiochip_get_data(chip);
> +
> + /* Software can only control GPIO pins defined by ctrl_gpio_mask */
> + if (!(BIT(offset) & gs->ctrl_gpio_mask))
> + return;
> +
> + if (val)
> + writel(BIT(offset), gs->gpio_io + YU_GPIO_FW_DATA_OUT_SET);
> + else
> + writel(BIT(offset), gs->gpio_io + YU_GPIO_FW_DATA_OUT_CLEAR);
> +
> + /* Make sure all previous writes are done before changing YU_GPIO_FW_CONTROL_SET */
> + wmb();
> +
> + /* This needs to be done last to avoid glitches */
> + writel(BIT(offset), gs->gpio_io + YU_GPIO_FW_CONTROL_SET); }
Bracket on new line. This coding style is very odd.
The hardware is a bit odd too but I see why you can't use GPIO_GENERIC
properly with this FW_CONTROL_SET business :/
> +static int mlxbf3_gpio_direction_input(struct gpio_chip *chip,
> + unsigned int offset)
> +{
> + struct mlxbf3_gpio_context *gs = gpiochip_get_data(chip);
> + unsigned long flags;
> +
> + spin_lock_irqsave(&gs->gc.bgpio_lock, flags);
> +
> + writel(BIT(offset), gs->gpio_io + YU_GPIO_FW_OUTPUT_ENABLE_CLEAR);
> + writel(BIT(offset), gs->gpio_io + YU_GPIO_FW_CONTROL_CLEAR);
> +
> + spin_unlock_irqrestore(&gs->gc.bgpio_lock, flags);
> +
> + return 0;
> +}
> +
> +static int mlxbf3_gpio_direction_output(struct gpio_chip *chip,
> + unsigned int offset,
> + int value)
> +{
> + struct mlxbf3_gpio_context *gs = gpiochip_get_data(chip);
> + unsigned long flags;
> +
> + spin_lock_irqsave(&gs->gc.bgpio_lock, flags);
> +
> + writel(BIT(offset), gs->gpio_io + YU_GPIO_FW_OUTPUT_ENABLE_SET);
> +
> + spin_unlock_irqrestore(&gs->gc.bgpio_lock, flags);
> +
> + return 0;
> +}
Why isn't FW_CONTROL_CLEAR/SET used when making a line
into an output? Seems unsymmetric? At least put a comment in the
code why this is different from making a line into input.
> + /* To set the direction to input, just give control to HW by setting
> + * YU_GPIO_FW_CONTROL_CLEAR.
> + * If the GPIO is controlled by HW, read its value via read_data_in register.
> + *
> + * When the direction = output, the GPIO is controlled by SW and
> + * datain=dataout. If software modifies the value of the GPIO pin,
> + * the value can be read from read_data_in without changing the direction.
> + */
> + ret = bgpio_init(gc, dev, 4,
> + gs->gpio_io + YU_GPIO_READ_DATA_IN,
> + NULL,
> + NULL,
> + NULL,
> + NULL,
> + 0);
Hm. Is it still worth it?
MVH
Linus Walleij
