Re: [PATCH 1/2] spi: bcm2835: add spi-bcm2835aux driver for the auxiliar spi1 and spi2

[Martin Sperl <kernel@xxxxxxxxxxxxxxxx>]

> On 30.07.2015, at 09:10, Martin Sperl <kernel@xxxxxxxxxxxxxxxx> wrote:
>> What about adding “bcrm,bcm2835-aux-enable” to drivers/mfd/syscon.c
>> compatibility?
>> 
>> That way we:
>> * have a clean dt that only represents hardware
>> * reuse existing code with minimal modifications
>> * minimal effort
>> 
>> That would be a minimal patch to enable this, so we can ask if that is
>> acceptable and if it is not then we can still think of something else,
>> which would be mostly replicating the bit-management portion of syscon.
> 
> that minimal patch extending compatibility of syscon and also including
> the config portion for the kernel as well as the device tree would look
> something like this (note that it is not tested yet):

> --- a/drivers/mfd/syscon.c
> +++ b/drivers/mfd/syscon.c
> @@ -211,6 +211,12 @@ static int syscon_probe(struct platform_device *pdev)
>        return 0;
> }
> 
> +static const struct of_device_id syscon_match[] = {
> +       { .compatible = "bcrm,bcm2835-aux-enable", },
> +       {}
> +};
> +MODULE_DEVICE_TABLE(of, syscon_match);
> +
> static const struct platform_device_id syscon_ids[] = {
>        { "syscon", },
>        { }

Unfortunately the above does not work with syscon, as it
_explicitly_ requires a compatibility of “syscon”:

        if (!of_device_is_compatible(np, "syscon"))
                return ERR_PTR(-EINVAL);

So by the rational of device-tree representing only the
hardware this driver should never get used because of this
limitation

Replicating syscon/regmap into a separate driver that has explicit
compatibility of “bcrm, bcm2835-aux” results in more than 162 lines
of extra code (plus changes to Makefile and Kconfig)

So I wonder if it would not be acceptable to allow:
  compatiblity = “bcrm,bcm2835-aux”, “syscon”;

and avoid an unnecessary extra driver which provides only
the following functions:

void bcm2835aux_enable(struct bcm2835aux *aux, u32 mask);
void bcm2835aux_disable(struct bcm2835aux *aux, u32 mask);
struct bcm2835aux *bcm2835aux_lookup_by_phandle(
	struct device_node *np,
	const char *property);

all of which are available almost identical in syscon/regmap.

We can also try to discuss if a modification to syscon so that it
also checks the “explicit” compatibility in the configured list.

Martin--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html