[RFC PATCH] soc: bcm2835: auxiliar devices enable infrastructure

[Stephen Warren]

On 08/04/2015 02:41 AM, kernel at martin.sperl.org wrote:
> From: Martin Sperl <kernel at martin.sperl.org>
> 
> The bcm2835 soc contains 3 auxiliar devices (spi1, spi2 and uart1)
> that all are enabled via a shared register.
> 
> To serialize access this soc-driver is created that implements
>   bcm2835aux_enable(struct device *dev, const char *property);
>   bcm2835aux_disable(struct device *dev, const char *property);
> 
> Ehich will read the property from the device tree of the device
> and enable/disable that specific device as per device tree.
> 
> First use of this api will be spi-bcm2835aux, which will
> set the following property in the device tree for the devices:
>   brcm,aux-enable = <&aux_enable 4>;  
> 
> And call:
>   bcm2835aux_enable(&spi->dev, "brcm,aux-enable");
> 
> This allows to define different property names based on the
> driver.

You need to send patches like this to a broader audience, such as CCing
the ARM kernel mailing list.

> diff --git a/Documentation/devicetree/bindings/soc/bcm/brcm,bcm2835-aux.txt b/Documentation/devicetree/bindings/soc/bcm/brcm,bcm2835-aux.txt

> +Broadcom BCM2835 auxiliar device enable register

auxiliary. Same elsewhere.

> +- reg: Should contain register location and length for the enabl
> +       register

> +	reg = <0x7e215004 0x04>;

I think both registers 0x7e215000 and 0x7e215004 are part of the
"global" auxiliary register set. As such, I'd expect:

	reg = <0x7e215000 0x08>;

> diff --git a/drivers/soc/bcm/bcm2835-aux.c b/drivers/soc/bcm/bcm2835-aux.c

> +static void *bcm2835aux_find_base(struct device *dev, const char *property)

> +	/* now find the device it points to */
> +	found = driver_find_device(&bcm2835aux_driver.driver, NULL,
> +				   np, bcm2835aux_dev_match);
> +	if (!found) {
> +		dev_err(dev, "device for phandle of %s not found\n",
> +			property);
> +		return ERR_PTR(-ENODEV);

That should probably be -EPROBE_DEFER, so that client drivers can defer
their own probe if this driver isn't yet probed but the client driver
will need its services.

> diff --git a/include/linux/soc/bcm/bcm2835-aux.h b/include/linux/soc/bcm/bcm2835-aux.h

> +int bcm2835aux_enable(struct device *dev, const char *property);
> +int bcm2835aux_disable(struct device *dev, const char *property);

Will client drivers call bcm2835aux_enable() only from their probe()? If
so, that set of APIs is fine. Otherwise, I would expect more functions:

struct bcm2835aux;
struct bcm2835aux *bcm2835aux_get_handle(struct device *dev,
		const char *property);
int bcm2835aux_enable(struct bcm2835aux *aux);
int bcm2835aux_disable(struct bcm2835aux *aux);