Re: [PATCH V2 4/8] spi: rspi: Add support for 8-bit Data Register access

[Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 1492 bytes --]

On Sun, Jan 12, 2014 at 11:27:40AM +0100, Geert Uytterhoeven wrote:

> +static void rspi_write_data(const struct rspi_data *rspi, u16 data)
> +{
> +	if (rspi->data_width == 8)
> +		rspi_write8(rspi, data, RSPI_SPDR);
> +	else /* 16 bit only */
> +		rspi_write16(rspi, data, RSPI_SPDR);
> +}

A switch statement is the more natural way of writing this.

> +static int rspi_parse_platform_data(struct rspi_data *rspi,
> +				    const struct rspi_plat_data *rspi_pd)
> +{
> +	if (rspi_pd && rspi_pd->data_width) {
> +		rspi->data_width = rspi_pd->data_width;
> +		switch (rspi_pd->data_width) {
> +		case 8:
> +			rspi->spdcr = SPDCR_SPLBYTE;
> +			break;
> +		case 16:
> +			rspi->spdcr = SPDCR_SPLWORD;
> +			break;
> +		default:
> +			return -EINVAL;
> +		}
> +	} else {
> +		/*
> +		 * Use legacy 16-bit width data access if a data_width value
> +		 * isn't defined in the platform data.
> +		 */
> +		rspi->data_width = 16;
> +		rspi->spdcr = 0;
> +	}

This looks very strange - if no platform data is specified the value set
for spdcr is one that can't be arrived at via platform data.  What's
going on here?  It'd seem more idiomatic to treat missing platform data
and a zero data_width identically too.

> +	ret = ops->parse_platform_data(rspi, rspi_pd);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "rspi invalid platform data.\n");
> +		goto error1;
> +	}

I'd move the error logging up into the parse function and have it say
what exactly is wrong - that'd be more friendly to users.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]