Patchwork [RFC,v3,08/13] ahci-platform: Allow specifying platform_data through of_device_id

login
register
mail settings
Submitter Hans de Goede
Date Jan. 18, 2014, 11:48 p.m.
Message ID <1390088935-7193-9-git-send-email-hdegoede@redhat.com>
Download mbox | patch
Permalink /patch/312350/
State Not Applicable
Delegated to: David Miller
Headers show

Comments

Hans de Goede - Jan. 18, 2014, 11:48 p.m.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/ata/ahci_platform.c | 41 +++++++++++++++++++++++++++++------------
 1 file changed, 29 insertions(+), 12 deletions(-)
Tejun Heo - Jan. 19, 2014, 11:38 a.m.
On Sun, Jan 19, 2014 at 12:48:50AM +0100, Hans de Goede wrote:
> @@ -126,7 +151,7 @@ static void ahci_put_clks(struct ahci_host_priv *hpriv)
>  static int ahci_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
> -	struct ahci_platform_data *pdata = dev_get_platdata(dev);
> +	const struct ahci_platform_data *pdata = ahci_get_pdata(dev);

Let's please not add const.  For data types which are known to be
terminal, const more or less works, I suppose but for anything even
mildly complicated it ends up being more of a headache.  C just
doesn't have enough language support to make const actually useful.
e.g. now if somebody wants to add an accessor to ahci_platform_data()
which is applicable to both readers and writers, we either need two
separate accessors for const and !const paths or have to cast away
const.

Thanks.
Russell King - ARM Linux - Jan. 19, 2014, 12:30 p.m.
On Sun, Jan 19, 2014 at 06:38:38AM -0500, Tejun Heo wrote:
> On Sun, Jan 19, 2014 at 12:48:50AM +0100, Hans de Goede wrote:
> > @@ -126,7 +151,7 @@ static void ahci_put_clks(struct ahci_host_priv *hpriv)
> >  static int ahci_probe(struct platform_device *pdev)
> >  {
> >  	struct device *dev = &pdev->dev;
> > -	struct ahci_platform_data *pdata = dev_get_platdata(dev);
> > +	const struct ahci_platform_data *pdata = ahci_get_pdata(dev);
> 
> Let's please not add const.  For data types which are known to be
> terminal, const more or less works, I suppose but for anything even
> mildly complicated it ends up being more of a headache.  C just
> doesn't have enough language support to make const actually useful.
> e.g. now if somebody wants to add an accessor to ahci_platform_data()
> which is applicable to both readers and writers, we either need two
> separate accessors for const and !const paths or have to cast away
> const.

I don't see anything wrong with this.  Platform data should _never_ be
written to, because doing so will change the driver behaviour between
bindings.  It really should be read-only to driver code.
Tejun Heo - Jan. 19, 2014, 1:19 p.m.
On Sun, Jan 19, 2014 at 12:30:55PM +0000, Russell King - ARM Linux wrote:
> I don't see anything wrong with this.  Platform data should _never_ be
> written to, because doing so will change the driver behaviour between
> bindings.  It really should be read-only to driver code.

Hmmm, problems usually arise when const is used to distinguish ro and
rw users.  When it's simple, it seems okay but later on it often ends
up requiring dropping const in almost arbitrary places or forced casts
somewhere random.  Over time, it makes const annotations in the kernel
sparse and inconsistent.  For anything non-trivial, I think it's best
to ignore it.

That said, if the object is actually immutable once initialized, it
shouldn't cause any trouble.  That probably is one of the few proper
use case for const on complex data types.

Thanks.
Hans de Goede - Jan. 19, 2014, 6:56 p.m.
Hi,

On 01/19/2014 12:38 PM, Tejun Heo wrote:
> On Sun, Jan 19, 2014 at 12:48:50AM +0100, Hans de Goede wrote:
>> @@ -126,7 +151,7 @@ static void ahci_put_clks(struct ahci_host_priv *hpriv)
>>   static int ahci_probe(struct platform_device *pdev)
>>   {
>>   	struct device *dev = &pdev->dev;
>> -	struct ahci_platform_data *pdata = dev_get_platdata(dev);
>> +	const struct ahci_platform_data *pdata = ahci_get_pdata(dev);
>
> Let's please not add const.  For data types which are known to be
> terminal, const more or less works, I suppose but for anything even
> mildly complicated it ends up being more of a headache.  C just
> doesn't have enough language support to make const actually useful.
> e.g. now if somebody wants to add an accessor to ahci_platform_data()
> which is applicable to both readers and writers, we either need two
> separate accessors for const and !const paths or have to cast away
> const.

The problem is that:

1) of_match_device returns const, so without the const the code would need
to cast that const away somewhere
2) of_match_device is right to return const because
2a) the data can actually be const
2b) even if not const, it is shared between multiple instances of the same
device-type and thus should never be written too

So as Russell already said, the use of const is correct here, and the best
thing to do is to simply keep it.

Regards,

Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tejun Heo - Jan. 19, 2014, 7:22 p.m.
On Sun, Jan 19, 2014 at 07:56:53PM +0100, Hans de Goede wrote:
> 1) of_match_device returns const, so without the const the code would need
> to cast that const away somewhere
> 2) of_match_device is right to return const because
> 2a) the data can actually be const
> 2b) even if not const, it is shared between multiple instances of the same
> device-type and thus should never be written too

Sure, if it has already gone that way, keeping it on is probably the
easiest way.

> So as Russell already said, the use of const is correct here, and the best
> thing to do is to simply keep it.

It's not about whether this specific annotation is correct or not.
It's that C as a language doesn't have good enough const support to
have proper const annotations and tends to lead to practical problems
often making the trade-off unclear in complex cases.

Anyways, as written above, if it's propagation of existing ones and
doesn't have mixed ro/rw usages, keeping it on probably is the eaiest.

Thanks.
Sascha Hauer - Jan. 20, 2014, 8:24 a.m.
On Sun, Jan 19, 2014 at 12:48:50AM +0100, Hans de Goede wrote:
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/ata/ahci_platform.c | 41 +++++++++++++++++++++++++++++------------
>  1 file changed, 29 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/ata/ahci_platform.c b/drivers/ata/ahci_platform.c
> index 3bc2dab..0676d72 100644
> --- a/drivers/ata/ahci_platform.c
> +++ b/drivers/ata/ahci_platform.c
> @@ -20,6 +20,7 @@
>  #include <linux/init.h>
>  #include <linux/interrupt.h>
>  #include <linux/device.h>
> +#include <linux/of_device.h>
>  #include <linux/platform_device.h>
>  #include <linux/libata.h>
>  #include <linux/ahci_platform.h>
> @@ -87,6 +88,30 @@ static struct scsi_host_template ahci_platform_sht = {
>  	AHCI_SHT("ahci_platform"),
>  };
>  
> +static const struct of_device_id ahci_of_match[] = {
> +	{ .compatible = "snps,spear-ahci", },
> +	{ .compatible = "snps,exynos5440-ahci", },
> +	{ .compatible = "ibm,476gtr-ahci", },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, ahci_of_match);
> +
> +static const struct ahci_platform_data *ahci_get_pdata(struct device *dev)
> +{
> +	struct ahci_platform_data *pdata;
> +	const struct of_device_id *of_id;
> +
> +	pdata = dev_get_platdata(dev);
> +	if (pdata)
> +		return pdata;
> +
> +	of_id = of_match_device(ahci_of_match, dev);
> +	if (of_id)
> +		return of_id->data;

I don't think it's a good idea to force of_id->data to be of type struct
struct ahci_platform_data *. With this we don't have a place to store
SoC specific data anymore.

Sascha
Hans de Goede - Jan. 20, 2014, 8:35 a.m.
Hi,

On 01/20/2014 09:24 AM, Sascha Hauer wrote:
> On Sun, Jan 19, 2014 at 12:48:50AM +0100, Hans de Goede wrote:
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>   drivers/ata/ahci_platform.c | 41 +++++++++++++++++++++++++++++------------
>>   1 file changed, 29 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/ata/ahci_platform.c b/drivers/ata/ahci_platform.c
>> index 3bc2dab..0676d72 100644
>> --- a/drivers/ata/ahci_platform.c
>> +++ b/drivers/ata/ahci_platform.c
>> @@ -20,6 +20,7 @@
>>   #include <linux/init.h>
>>   #include <linux/interrupt.h>
>>   #include <linux/device.h>
>> +#include <linux/of_device.h>
>>   #include <linux/platform_device.h>
>>   #include <linux/libata.h>
>>   #include <linux/ahci_platform.h>
>> @@ -87,6 +88,30 @@ static struct scsi_host_template ahci_platform_sht = {
>>   	AHCI_SHT("ahci_platform"),
>>   };
>>
>> +static const struct of_device_id ahci_of_match[] = {
>> +	{ .compatible = "snps,spear-ahci", },
>> +	{ .compatible = "snps,exynos5440-ahci", },
>> +	{ .compatible = "ibm,476gtr-ahci", },
>> +	{},
>> +};
>> +MODULE_DEVICE_TABLE(of, ahci_of_match);
>> +
>> +static const struct ahci_platform_data *ahci_get_pdata(struct device *dev)
>> +{
>> +	struct ahci_platform_data *pdata;
>> +	const struct of_device_id *of_id;
>> +
>> +	pdata = dev_get_platdata(dev);
>> +	if (pdata)
>> +		return pdata;
>> +
>> +	of_id = of_match_device(ahci_of_match, dev);
>> +	if (of_id)
>> +		return of_id->data;
>
> I don't think it's a good idea to force of_id->data to be of type struct
> struct ahci_platform_data *. With this we don't have a place to store
> SoC specific data anymore.

?? ahci_platform_data *is* soc specific data, it allows various soc
specific overrides.

Regards,

Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sascha Hauer - Jan. 20, 2014, 9:09 a.m.
On Mon, Jan 20, 2014 at 09:35:06AM +0100, Hans de Goede wrote:
> Hi,
> 
> On 01/20/2014 09:24 AM, Sascha Hauer wrote:
> >>+
> >>+static const struct ahci_platform_data *ahci_get_pdata(struct device *dev)
> >>+{
> >>+	struct ahci_platform_data *pdata;
> >>+	const struct of_device_id *of_id;
> >>+
> >>+	pdata = dev_get_platdata(dev);
> >>+	if (pdata)
> >>+		return pdata;
> >>+
> >>+	of_id = of_match_device(ahci_of_match, dev);
> >>+	if (of_id)
> >>+		return of_id->data;
> >
> >I don't think it's a good idea to force of_id->data to be of type struct
> >struct ahci_platform_data *. With this we don't have a place to store
> >SoC specific data anymore.
> 
> ?? ahci_platform_data *is* soc specific data, it allows various soc
> specific overrides.

I know, but it might not be enough for encding the slight differences
between i.MX53 and i.MX6. So you say then we would need to different
instances of struct ahci_platform_data, one for i.MX53 and one for
i.MX6. Ok, that works.

Overall I must say that I'm not really happy with giving up control over
the probe function and putting ahci_platform as a midlayer between the
SoC and the ahci lib. Just my 2 cents, if I'm the only one feel free to
ignore me, but maybe there are others that have the same feeling.

Sascha
Hans de Goede - Jan. 20, 2014, 9:17 a.m.
Hi,

On 01/20/2014 10:09 AM, Sascha Hauer wrote:
> On Mon, Jan 20, 2014 at 09:35:06AM +0100, Hans de Goede wrote:
>> Hi,
>>
>> On 01/20/2014 09:24 AM, Sascha Hauer wrote:
>>>> +
>>>> +static const struct ahci_platform_data *ahci_get_pdata(struct device *dev)
>>>> +{
>>>> +	struct ahci_platform_data *pdata;
>>>> +	const struct of_device_id *of_id;
>>>> +
>>>> +	pdata = dev_get_platdata(dev);
>>>> +	if (pdata)
>>>> +		return pdata;
>>>> +
>>>> +	of_id = of_match_device(ahci_of_match, dev);
>>>> +	if (of_id)
>>>> +		return of_id->data;
>>>
>>> I don't think it's a good idea to force of_id->data to be of type struct
>>> struct ahci_platform_data *. With this we don't have a place to store
>>> SoC specific data anymore.
>>
>> ?? ahci_platform_data *is* soc specific data, it allows various soc
>> specific overrides.
>
> I know, but it might not be enough for encding the slight differences
> between i.MX53 and i.MX6. So you say then we would need to different
> instances of struct ahci_platform_data, one for i.MX53 and one for
> i.MX6. Ok, that works.
>
> Overall I must say that I'm not really happy with giving up control over
> the probe function and putting ahci_platform as a midlayer between the
> SoC and the ahci lib. Just my 2 cents, if I'm the only one feel free to
> ignore me, but maybe there are others that have the same feeling.

I'm currently working on a slightly different implementation of a more
generic ahci_platform.c where ahci_platform.c exports some standard platform
related functionality as library functions.

Drivers which need to override some of ahci_platform.c's behavior because of
non standard hw, will then export their own struct platform_driver and can
call into the exported functions for standard stuff to avoid code duplication
where appropriate, while still having 100% freedom to do things in a custom
way where necessary.

I hope to post a PATCH RFC v4 with these changes later today, which you will
hopefully like better. Input on v4, even just a "yep better" remark would be
much appreciated.

Regards,

Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sascha Hauer - Jan. 20, 2014, 9:57 a.m.
On Mon, Jan 20, 2014 at 10:17:24AM +0100, Hans de Goede wrote:
> Hi,
> 
> I'm currently working on a slightly different implementation of a more
> generic ahci_platform.c where ahci_platform.c exports some standard platform
> related functionality as library functions.
> 
> Drivers which need to override some of ahci_platform.c's behavior because of
> non standard hw, will then export their own struct platform_driver and can
> call into the exported functions for standard stuff to avoid code duplication
> where appropriate, while still having 100% freedom to do things in a custom
> way where necessary.

Nice, thanks.

> 
> I hope to post a PATCH RFC v4 with these changes later today, which you will
> hopefully like better. Input on v4, even just a "yep better" remark would be
> much appreciated.

Yes, I will give feedback on v4.

Sascha

Patch

diff --git a/drivers/ata/ahci_platform.c b/drivers/ata/ahci_platform.c
index 3bc2dab..0676d72 100644
--- a/drivers/ata/ahci_platform.c
+++ b/drivers/ata/ahci_platform.c
@@ -20,6 +20,7 @@ 
 #include <linux/init.h>
 #include <linux/interrupt.h>
 #include <linux/device.h>
+#include <linux/of_device.h>
 #include <linux/platform_device.h>
 #include <linux/libata.h>
 #include <linux/ahci_platform.h>
@@ -87,6 +88,30 @@  static struct scsi_host_template ahci_platform_sht = {
 	AHCI_SHT("ahci_platform"),
 };
 
+static const struct of_device_id ahci_of_match[] = {
+	{ .compatible = "snps,spear-ahci", },
+	{ .compatible = "snps,exynos5440-ahci", },
+	{ .compatible = "ibm,476gtr-ahci", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, ahci_of_match);
+
+static const struct ahci_platform_data *ahci_get_pdata(struct device *dev)
+{
+	struct ahci_platform_data *pdata;
+	const struct of_device_id *of_id;
+
+	pdata = dev_get_platdata(dev);
+	if (pdata)
+		return pdata;
+
+	of_id = of_match_device(ahci_of_match, dev);
+	if (of_id)
+		return of_id->data;
+
+	return NULL;
+}
+
 static int ahci_enable_clks(struct device *dev, struct ahci_host_priv *hpriv)
 {
 	int c, rc;
@@ -126,7 +151,7 @@  static void ahci_put_clks(struct ahci_host_priv *hpriv)
 static int ahci_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
-	struct ahci_platform_data *pdata = dev_get_platdata(dev);
+	const struct ahci_platform_data *pdata = ahci_get_pdata(dev);
 	const struct platform_device_id *id = platform_get_device_id(pdev);
 	struct ata_port_info pi = ahci_port_info[id ? id->driver_data : 0];
 	const struct ata_port_info *ppi[] = { &pi, NULL };
@@ -290,7 +315,7 @@  free_clk:
 static void ahci_host_stop(struct ata_host *host)
 {
 	struct device *dev = host->dev;
-	struct ahci_platform_data *pdata = dev_get_platdata(dev);
+	const struct ahci_platform_data *pdata = ahci_get_pdata(dev);
 	struct ahci_host_priv *hpriv = host->private_data;
 
 	if (hpriv->target_pwr)
@@ -306,7 +331,7 @@  static void ahci_host_stop(struct ata_host *host)
 #ifdef CONFIG_PM_SLEEP
 static int ahci_suspend(struct device *dev)
 {
-	struct ahci_platform_data *pdata = dev_get_platdata(dev);
+	const struct ahci_platform_data *pdata = ahci_get_pdata(dev);
 	struct ata_host *host = dev_get_drvdata(dev);
 	struct ahci_host_priv *hpriv = host->private_data;
 	void __iomem *mmio = hpriv->mmio;
@@ -345,7 +370,7 @@  static int ahci_suspend(struct device *dev)
 
 static int ahci_resume(struct device *dev)
 {
-	struct ahci_platform_data *pdata = dev_get_platdata(dev);
+	const struct ahci_platform_data *pdata = ahci_get_pdata(dev);
 	struct ata_host *host = dev_get_drvdata(dev);
 	struct ahci_host_priv *hpriv = host->private_data;
 	int rc;
@@ -393,14 +418,6 @@  disable_unprepare_clk:
 
 static SIMPLE_DEV_PM_OPS(ahci_pm_ops, ahci_suspend, ahci_resume);
 
-static const struct of_device_id ahci_of_match[] = {
-	{ .compatible = "snps,spear-ahci", },
-	{ .compatible = "snps,exynos5440-ahci", },
-	{ .compatible = "ibm,476gtr-ahci", },
-	{},
-};
-MODULE_DEVICE_TABLE(of, ahci_of_match);
-
 static struct platform_driver ahci_driver = {
 	.probe = ahci_probe,
 	.remove = ata_platform_remove_one,