Re: [RESEND RFC PATCH 1/5] platform: x86: add driver for UP Board I/O CPLD

[Bryan O'Donoghue]

On Mon, 2016-07-04 at 17:07 +0100, Dan O'Donovan wrote:
> +static int cpld_reg_update(struct up_board_cpld *cpld)
> +{
> +	u64 dir_reg_verify = 0;
> +	int i;
> +
> +	/* Reset the CPLD internal counters */
> +	gpiod_set_value(cpld->reset_gpio.soc_gpiod, 0);
> +	gpiod_set_value(cpld->reset_gpio.soc_gpiod, 1);
> +
> +	/*
> +	Â* Update the CPLD dir register
> +	Â* data_in will be sampled on each rising edge of the strobe
> signal
> +	Â*/
> +	for (i = cpld->dir_reg_size - 1; i >= 0; i--) {
> +		gpiod_set_value(cpld->strobe_gpio.soc_gpiod, 0);
> +		gpiod_set_value(cpld->data_in_gpio.soc_gpiod,
> +				(cpld->dir_reg >> i) & 0x1);
> +		gpiod_set_value(cpld->strobe_gpio.soc_gpiod, 1);
> +	}
> +
> +	/*
> +	Â* Read back and verify the value
> +	Â* data_out will be set on each rising edge of the strobe
> signal
> +	Â*/
> +	for (i = cpld->dir_reg_size - 1; i >= 0; i--) {
> +		int data_out;
> +
> +		gpiod_set_value(cpld->strobe_gpio.soc_gpiod, 0);
> +		gpiod_set_value(cpld->strobe_gpio.soc_gpiod, 1);
> +		data_out = gpiod_get_value(cpld-
> >data_out_gpio.soc_gpiod);
> +		dir_reg_verify |= (u64)data_out << i;
> +	}
> +
> +	if (dir_reg_verify != cpld->dir_reg) {
> +		pr_err("CPLD verify error (expected: %llX, actual:
> %llX)\n",
> +		ÂÂÂÂÂÂÂcpld->dir_reg, dir_reg_verify);

dev_err();

> +		return -EIO;
> +	}
> +
> +	/* Issue a dummy STB cycle to latch the dir register updates
> */
> +	gpiod_set_value(cpld->strobe_gpio.soc_gpiod, 0);
> +	gpiod_set_value(cpld->strobe_gpio.soc_gpiod, 1);
> +
> +	return 0;
> +}
> +
> +/**
> + * up_board_cpld_reg_set_bit() - update CPLD configuration
> + * @cpld:	CPLD internal context info reference
> + * @offset:	bit offset in CPLD register to set
> + * @value:	boolean value to set in CPLD register bit selected
> by offset
> + *
> + * Return:	Returns 0 if successful, or negative error value
> otherwise
> + */
> +static int up_board_cpld_reg_set_bit(struct up_board_cpld *cpld,
> +				ÂÂÂÂÂunsigned int offset, int value)
> +{
> +	u64 old_regval;
> +	int ret = 0;
> +
> +	spin_lock(&cpld->lock);
> +
> +	old_regval = cpld->dir_reg;
> +
> +	if (value)
> +		cpld->dir_reg |= 1ULL << offset;
> +	else
> +		cpld->dir_reg &= ~(1ULL << offset);
> +
> +	/* Only update the CPLD register if it has changed */
> +	if (cpld->dir_reg != old_regval)
> +		ret = cpld_reg_update(cpld);
> +
> +	spin_unlock(&cpld->lock);

Seems to me as though cpld_reg_update() could be quite lengthy. Would a
mutex be a better choice here ?


> +static int up_board_cpld_setup(struct up_board_cpld *cpld)
> +{
> +	struct up_board_gpio_info *cpld_gpios[] = {
> +		&cpld->strobe_gpio,
> +		&cpld->reset_gpio,
> +		&cpld->data_in_gpio,
> +		&cpld->data_out_gpio,
> +		&cpld->oe_gpio,
> +	};
> +	int i, ret;
> +
> +	spin_lock_init(&cpld->lock);
> +
> +	/* Initialise the CPLD config input GPIOs as outputs,
> initially low */
> +	for (i = 0; i < ARRAY_SIZE(cpld_gpios); i++) {
> +		struct up_board_gpio_info *gpio = cpld_gpios[i];
> +
> +		ret = up_board_soc_gpio_setup(cpld, gpio);
> +		if (ret)
> +			return ret;
> +
> +		ret = devm_gpio_request_one(cpld->dev, gpio-
> >soc_gpio,
> +					ÂÂÂÂgpio->soc_gpio_flags,
> +					ÂÂÂÂdev_name(cpld->dev));
> +		if (ret)
> +			return ret;
> +	}
> +
> +	/* Load initial CPLD configuration (all pins set for GPIO
> input) */
> +	ret = cpld_reg_update(cpld);
> +	if (ret) {

devm_gpio_free() ?

---
bod