Re: [PATCH] spidev: add platform driver support

From: Mark Brown <broonie@kernel.org>
To: Christian Gmeiner <christian.gmeiner@gmail.com>
Cc: linux-kernel@vger.kernel.org, linux-spi@vger.kernel.org
Subject: Re: [PATCH] spidev: add platform driver support
Date: Thu, 27 May 2021 11:10:21 +0100	[thread overview]
Message-ID: <YK9wDd/+c1uAjwk7@sirena.org.uk> (raw)
In-Reply-To: <20210527084531.18989-1-christian.gmeiner@gmail.com>

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

On Thu, May 27, 2021 at 10:45:15AM +0200, Christian Gmeiner wrote:

> This makes it possible to use spidev in combination with the
> MFD subsystem. The MFD subsystem add platform_driver devices.

This is a really strange thing to want to do so it needs a
changelog which explains what the goal is and why this is a good
way of accomplishing that goal.

> +static int spidev_platform_probe(struct platform_device *pdev)
> +{
> +	struct device *parent = pdev->dev.parent;
> +	struct spi_device *spi;
> +
> +	if (strcmp(parent->bus->name, "spi"))
> +		return -ENODEV;
> +
> +	spi = to_spi_device(parent);
> +
> +	/* This only works if no drvdata is stored */
> +	if (spi_get_drvdata(spi)) {
> +		dev_err(&pdev->dev, "drvdata is not NULL\n");

Why?

> +		return -EOPNOTSUPP;
> +	}
> +
> +	return spidev_probe(spi);

This really does not seem like a good idea, this is exposing the
entire device to userspace in a completely unstructured fashion
while there will be other drivers controlling the same hardware.
That seems like it's asking for trouble, there's absolutely
nothing ensuring that userspace doesn't break things the drivers
are doing.

I really don't think it makes sense to mix kernel drivers with
unmoderated userspace access to the hardware in a single driver.

> +static struct platform_driver spidev_platfoem_driver = {

platfoem?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]