| Message ID | 1390088935-7193-9-git-send-email-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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
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.
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
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
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
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
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
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,
Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/ata/ahci_platform.c | 41 +++++++++++++++++++++++++++++------------ 1 file changed, 29 insertions(+), 12 deletions(-)