[PATCH linux v2 16/17] drivers/fsi: Set up CFAMs for communication

[Jeremy Kerr]

Hi Chris,

> +/*
> + * Issue a break commad on this link
> + */
> +static int fsi_master_break(struct fsi_master *master, int link)
> +{
> +	int rc = 0;
> +
> +	if (master->send_break)
> +		rc = master->send_break(master, link);
> +
> +	return rc;
> +}

OK, looks good.

> +static int fsi_master_fake_break(struct fsi_master *master, int link)
> +{
> +	if (link != 0)
> +		return -ENODEV;
> +
> +	return 0;
> +}
> +
> +static int fsi_master_fake_set_smode(struct fsi_master *master, int link,
> +					uint32_t smode)
> +{
> +	if (link != 0)
> +		return -ENODEV;
> +
> +	return 0;
> +}
> +

Since these are no-ops...

>  static int fsi_master_fake_probe(struct platform_device *pdev)
>  {
>  	struct fsi_master *master;
> @@ -63,6 +80,8 @@ static int fsi_master_fake_probe(struct platform_device *pdev)
>  	master->n_links = 1;
>  	master->read = fsi_master_fake_read;
>  	master->write = fsi_master_fake_write;
> +	master->send_break = fsi_master_fake_break;
> +	master->set_smode = fsi_master_fake_set_smode;

... we can just leave the callbacks NULL (the wrapper functions check
this).

However:

> +static int fsi_master_gpio_set_smode(struct fsi_master *_master, int link,
> +						uint32_t smode)
> +{
> +	int rc;
> +
> +	rc = _master->write(_master, link, 0, FSI_SLAVE_BASE + FSI_SMODE,
> +			&smode, sizeof(smode));
> +
> +	return rc;
> +}
> +

Won't this always be implemented the same way on all masters (ie, a
write to a particular slave register)? Why the need for a
master-specific callback here?

> +#define FSI_CMD_BREAK		0xc0de0000	/* Break command */
> +

This isn't used anywhere; what's it for? It doesn't look like something
that all masters will use, so shouldn't be in the global master header.

> @@ -29,6 +31,9 @@ struct fsi_master {
>  	int		(*write)(struct fsi_master *, int link,
>  				uint8_t slave, uint32_t addr,
>  				const void *val, size_t size);
> +	int		(*send_break)(struct fsi_master *, int link);
> +	int		(*set_smode)(struct fsi_master *, int link,
> +				uint32_t smode);

See above comments - do we really need a set_smode callback?

Cheers,


Jeremy