| Message ID | 1409583961-7466-2-git-send-email-hanjun.guo@linaro.org |
|---|---|
| State | New |
| Headers | show |
On Mon, Sep 01, 2014 at 05:17:51PM +0200, Arnd Bergmann wrote: > On Monday 01 September 2014 23:06:00 Hanjun Guo wrote: > > +#ifdef CONFIG_ACPI > > +/* Configure some sensible defaults for ACPI mode */ > > +static int smsc911x_probe_config_acpi(struct smsc911x_platform_config *config, > > + acpi_handle *ahandle) > > +{ > > + if (!ahandle) > > + return -ENOSYS; > > + > > + config->phy_interface = PHY_INTERFACE_MODE_MII; > > > > Please remove the #ifdef and use > > if (!IS_ENABLED(CONFIG_ACPI) || !ahandle) > > to check for ACPI support. This should result in the same object code > in all cases, but give better compile-time coverage when ACPI is > disabled. > struct acpi_handle does not exist in the case !CONFIG_ACPI > Also, -ENOSYS is probably the wrong return value. I think you mean > -ENXIO. > Yes that would make sense thanks. Not sure if we are planning to actually upstream this patch, I guess it depends if ARM start shipping Junos with the ACPI tables loaded on them. Graeme -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
On Monday 01 September 2014 16:28:54 Graeme Gregory wrote: > On Mon, Sep 01, 2014 at 05:17:51PM +0200, Arnd Bergmann wrote: > > On Monday 01 September 2014 23:06:00 Hanjun Guo wrote: > > > +#ifdef CONFIG_ACPI > > > +/* Configure some sensible defaults for ACPI mode */ > > > +static int smsc911x_probe_config_acpi(struct smsc911x_platform_config *config, > > > + acpi_handle *ahandle) > > > +{ > > > + if (!ahandle) > > > + return -ENOSYS; > > > + > > > + config->phy_interface = PHY_INTERFACE_MODE_MII; > > > > > > > Please remove the #ifdef and use > > > > if (!IS_ENABLED(CONFIG_ACPI) || !ahandle) > > > > to check for ACPI support. This should result in the same object code > > in all cases, but give better compile-time coverage when ACPI is > > disabled. > > > struct acpi_handle does not exist in the case !CONFIG_ACPI That should be easy to fix, and a good idea in general, independent of this patch. We generally make function declarations and type definitions visible (possibly empty) for disabled subsystems so the code using them silently goes away when that subsystem is disabled. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Sep 01, 2014 at 04:28:54PM +0100, Graeme Gregory wrote: > On Mon, Sep 01, 2014 at 05:17:51PM +0200, Arnd Bergmann wrote: > > On Monday 01 September 2014 23:06:00 Hanjun Guo wrote: > > > +#ifdef CONFIG_ACPI > > > +/* Configure some sensible defaults for ACPI mode */ > > > +static int smsc911x_probe_config_acpi(struct smsc911x_platform_config *config, > > > + acpi_handle *ahandle) > > > +{ > > > + if (!ahandle) > > > + return -ENOSYS; > > > + > > > + config->phy_interface = PHY_INTERFACE_MODE_MII; > > > > > > > Please remove the #ifdef and use > > > > if (!IS_ENABLED(CONFIG_ACPI) || !ahandle) > > > > to check for ACPI support. This should result in the same object code > > in all cases, but give better compile-time coverage when ACPI is > > disabled. > > > struct acpi_handle does not exist in the case !CONFIG_ACPI Confused. Then how come smsc911x_drv_probe() has this line: acpi_handle *ahandle = ACPI_HANDLE(&pdev->dev); without any #ifdef's.
On Mon, Sep 01, 2014 at 05:53:33PM +0100, Catalin Marinas wrote: > Confused. Then how come smsc911x_drv_probe() has this line: > acpi_handle *ahandle = ACPI_HANDLE(&pdev->dev); > without any #ifdef's. There's a stub smsc911x_probe_config_acpi() provided in the non-ACPI case, Arnd's suggestion is basically to remove the stub.
On Mon, Sep 01, 2014 at 05:53:33PM +0100, Catalin Marinas wrote: > On Mon, Sep 01, 2014 at 04:28:54PM +0100, Graeme Gregory wrote: > > On Mon, Sep 01, 2014 at 05:17:51PM +0200, Arnd Bergmann wrote: > > > On Monday 01 September 2014 23:06:00 Hanjun Guo wrote: > > > > +#ifdef CONFIG_ACPI > > > > +/* Configure some sensible defaults for ACPI mode */ > > > > +static int smsc911x_probe_config_acpi(struct smsc911x_platform_config *config, > > > > + acpi_handle *ahandle) > > > > +{ > > > > + if (!ahandle) > > > > + return -ENOSYS; > > > > + > > > > + config->phy_interface = PHY_INTERFACE_MODE_MII; > > > > > > > > > > Please remove the #ifdef and use > > > > > > if (!IS_ENABLED(CONFIG_ACPI) || !ahandle) > > > > > > to check for ACPI support. This should result in the same object code > > > in all cases, but give better compile-time coverage when ACPI is > > > disabled. > > > > > struct acpi_handle does not exist in the case !CONFIG_ACPI > > Confused. Then how come smsc911x_drv_probe() has this line: > > acpi_handle *ahandle = ACPI_HANDLE(&pdev->dev); > > without any #ifdef's. > It is possible I confused myself as some types moved around in ACPI recently. I will re-check! Graeme -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
On Mon, Sep 01, 2014 at 04:06:00PM +0100, Hanjun Guo wrote: > +#ifdef CONFIG_ACPI > +/* Configure some sensible defaults for ACPI mode */ > +static int smsc911x_probe_config_acpi(struct smsc911x_platform_config *config, > + acpi_handle *ahandle) > +{ > + if (!ahandle) > + return -ENOSYS; > + > + config->phy_interface = PHY_INTERFACE_MODE_MII; > + > + config->flags |= SMSC911X_USE_32BIT; > + > + config->irq_polarity = SMSC911X_IRQ_POLARITY_ACTIVE_HIGH; > + > + config->irq_type = SMSC911X_IRQ_TYPE_PUSH_PULL; > + > + return 0; > +} > +#else I don't like this and it shows issues we have with ACPI on certain ARM platforms. You hard-code these values to match the Juno platform. What if we get another SoC which has different configuration here? For DT, we have the smsc911x_probe_config_dt() which reads the relevant information from DT. I think this kind of configuration would be more suitable as _DSD properties and sharing the similar names with DT (but we go back to the question about who's in charge of the _DSD properties). > static int smsc911x_drv_probe(struct platform_device *pdev) > { > struct device_node *np = pdev->dev.of_node; > + acpi_handle *ahandle = ACPI_HANDLE(&pdev->dev); > struct net_device *dev; > struct smsc911x_data *pdata; > struct smsc911x_platform_config *config = dev_get_platdata(&pdev->dev); > @@ -2436,6 +2464,9 @@ static int smsc911x_drv_probe(struct platform_device *pdev) > } > > retval = smsc911x_probe_config_dt(&pdata->config, np); > + if (retval) > + retval = smsc911x_probe_config_acpi(&pdata->config, ahandle); > + In most of the ACPI patches so far we check for ACPI first with DT as a fall-back if ACPI is not enabled. This changes here. I would prefer something which probes only ACPI if the ACPI is enabled (run-time, not config) otherwise DT only. E.g.
Hi Mark, On 01/09/14 17:58, Mark Brown wrote: > On Mon, Sep 01, 2014 at 05:53:33PM +0100, Catalin Marinas wrote: > >> Confused. Then how come smsc911x_drv_probe() has this line: > >> acpi_handle *ahandle = ACPI_HANDLE(&pdev->dev); > >> without any #ifdef's. > > There's a stub smsc911x_probe_config_acpi() provided in the non-ACPI > case, Arnd's suggestion is basically to remove the stub. > I think Catalin is referring to ACPI_HANDLE used without any #ifdefs Catalin, few macros like ACPI_HANDLE and ACPI_PTR are defined in include/linux/acpi.h even when CONFIG_ACPI is not set mainly to avoid #ifdef's around simple assignments like the above one and one in platform_driver.acpi_match_table Regards, Sudeep -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Sep 01, 2014 at 06:08:49PM +0100, Sudeep Holla wrote: > On 01/09/14 17:58, Mark Brown wrote: > > On Mon, Sep 01, 2014 at 05:53:33PM +0100, Catalin Marinas wrote: > > > >> Confused. Then how come smsc911x_drv_probe() has this line: > > > >> acpi_handle *ahandle = ACPI_HANDLE(&pdev->dev); > > > >> without any #ifdef's. > > > > There's a stub smsc911x_probe_config_acpi() provided in the non-ACPI > > case, Arnd's suggestion is basically to remove the stub. > > > > I think Catalin is referring to ACPI_HANDLE used without any #ifdefs > > Catalin, few macros like ACPI_HANDLE and ACPI_PTR are defined in > include/linux/acpi.h even when CONFIG_ACPI is not set mainly to > avoid #ifdef's around simple assignments like the above one and one > in platform_driver.acpi_match_table My comment was to Graeme who said that #ifdef's were needed because acpi_handle (lowercase) was not defined in the !CONFIG_ACPI case. However, further down in the patch it was used without any #ifdef's.
On 01/09/14 18:14, Catalin Marinas wrote: > On Mon, Sep 01, 2014 at 06:08:49PM +0100, Sudeep Holla wrote: >> On 01/09/14 17:58, Mark Brown wrote: >>> On Mon, Sep 01, 2014 at 05:53:33PM +0100, Catalin Marinas wrote: >>> >>>> Confused. Then how come smsc911x_drv_probe() has this line: >>> >>>> acpi_handle *ahandle = ACPI_HANDLE(&pdev->dev); >>> >>>> without any #ifdef's. >>> >>> There's a stub smsc911x_probe_config_acpi() provided in the non-ACPI >>> case, Arnd's suggestion is basically to remove the stub. >>> >> >> I think Catalin is referring to ACPI_HANDLE used without any #ifdefs >> >> Catalin, few macros like ACPI_HANDLE and ACPI_PTR are defined in >> include/linux/acpi.h even when CONFIG_ACPI is not set mainly to >> avoid #ifdef's around simple assignments like the above one and one >> in platform_driver.acpi_match_table > > My comment was to Graeme who said that #ifdef's were needed because > acpi_handle (lowercase) was not defined in the !CONFIG_ACPI case. > However, further down in the patch it was used without any #ifdef's. > Ah OK, I misunderstood. In that case Graeme statement is wrong. IIRC acpi_handle is not structure, it's just a ptr used for object references in ACPI namespace and is available even when !CONFIG_ACPI Regards, Sudeep -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
On Mon, 1 Sep 2014 16:28:54 +0100, Graeme Gregory <graeme.gregory@linaro.org> wrote: > On Mon, Sep 01, 2014 at 05:17:51PM +0200, Arnd Bergmann wrote: > > On Monday 01 September 2014 23:06:00 Hanjun Guo wrote: > > > +#ifdef CONFIG_ACPI > > > +/* Configure some sensible defaults for ACPI mode */ > > > +static int smsc911x_probe_config_acpi(struct smsc911x_platform_config *config, > > > + acpi_handle *ahandle) > > > +{ > > > + if (!ahandle) > > > + return -ENOSYS; > > > + > > > + config->phy_interface = PHY_INTERFACE_MODE_MII; > > > > > > > Please remove the #ifdef and use > > > > if (!IS_ENABLED(CONFIG_ACPI) || !ahandle) > > > > to check for ACPI support. This should result in the same object code > > in all cases, but give better compile-time coverage when ACPI is > > disabled. > > > struct acpi_handle does not exist in the case !CONFIG_ACPI > > > Also, -ENOSYS is probably the wrong return value. I think you mean > > -ENXIO. > > > Yes that would make sense thanks. > > Not sure if we are planning to actually upstream this patch, I guess it > depends if ARM start shipping Junos with the ACPI tables loaded on them. I think we do want this upstreamed. Juno is intended to be a readily available reference platform, regardless of the firmware loaded when shipped. There will be users who use it as a test platform for ACPI development. g. -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Sep 15, 2014 at 05:08:44AM +0100, Grant Likely wrote: > On Mon, 1 Sep 2014 16:28:54 +0100, Graeme Gregory <graeme.gregory@linaro.org> wrote: > > On Mon, Sep 01, 2014 at 05:17:51PM +0200, Arnd Bergmann wrote: > > > On Monday 01 September 2014 23:06:00 Hanjun Guo wrote: > > > > +#ifdef CONFIG_ACPI > > > > +/* Configure some sensible defaults for ACPI mode */ > > > > +static int smsc911x_probe_config_acpi(struct smsc911x_platform_config *config, > > > > + acpi_handle *ahandle) > > > > +{ > > > > + if (!ahandle) > > > > + return -ENOSYS; > > > > + > > > > + config->phy_interface = PHY_INTERFACE_MODE_MII; > > > > > > > > > > Please remove the #ifdef and use > > > > > > if (!IS_ENABLED(CONFIG_ACPI) || !ahandle) > > > > > > to check for ACPI support. This should result in the same object code > > > in all cases, but give better compile-time coverage when ACPI is > > > disabled. > > > > > struct acpi_handle does not exist in the case !CONFIG_ACPI > > > > > Also, -ENOSYS is probably the wrong return value. I think you mean > > > -ENXIO. > > > > > Yes that would make sense thanks. > > > > Not sure if we are planning to actually upstream this patch, I guess it > > depends if ARM start shipping Junos with the ACPI tables loaded on them. > > I think we do want this upstreamed. Juno is intended to be a readily > available reference platform, regardless of the firmware loaded when > shipped. There will be users who use it as a test platform for ACPI > development. The patches for Juno are not intrusive. The only problem I have is giving the wrong example on how to deal with the platform-specific device information like this patch. I would much prefer to use _DSD (once we agree on how to do this) than hard-coding information based on the ACPI device id (you would need one for each SoC). I don't think we have reached an agreement yet: https://lkml.kernel.org/g/4816592.tj3on6vUaC@wuerfel
diff --git a/drivers/net/ethernet/smsc/smsc911x.c b/drivers/net/ethernet/smsc/smsc911x.c index 5e13fa5..69d725a 100644 --- a/drivers/net/ethernet/smsc/smsc911x.c +++ b/drivers/net/ethernet/smsc/smsc911x.c @@ -59,6 +59,7 @@ #include <linux/of_device.h> #include <linux/of_gpio.h> #include <linux/of_net.h> +#include <linux/acpi.h> #include "smsc911x.h" #define SMSC_CHIPNAME "smsc911x" @@ -2369,9 +2370,36 @@ static inline int smsc911x_probe_config_dt( } #endif /* CONFIG_OF */ +#ifdef CONFIG_ACPI +/* Configure some sensible defaults for ACPI mode */ +static int smsc911x_probe_config_acpi(struct smsc911x_platform_config *config, + acpi_handle *ahandle) +{ + if (!ahandle) + return -ENOSYS; + + config->phy_interface = PHY_INTERFACE_MODE_MII; + + config->flags |= SMSC911X_USE_32BIT; + + config->irq_polarity = SMSC911X_IRQ_POLARITY_ACTIVE_HIGH; + + config->irq_type = SMSC911X_IRQ_TYPE_PUSH_PULL; + + return 0; +} +#else +static int smsc911x_probe_config_acpi(struct smsc911x_platform_config *config, + acpi_handle *ahandle) +{ + return -ENOSYS; +} +#endif /* CONFIG_ACPI */ + static int smsc911x_drv_probe(struct platform_device *pdev) { struct device_node *np = pdev->dev.of_node; + acpi_handle *ahandle = ACPI_HANDLE(&pdev->dev); struct net_device *dev; struct smsc911x_data *pdata; struct smsc911x_platform_config *config = dev_get_platdata(&pdev->dev); @@ -2436,6 +2464,9 @@ static int smsc911x_drv_probe(struct platform_device *pdev) } retval = smsc911x_probe_config_dt(&pdata->config, np); + if (retval) + retval = smsc911x_probe_config_acpi(&pdata->config, ahandle); + if (retval && config) { /* copy config parameters across to pdata */ memcpy(&pdata->config, config, sizeof(pdata->config)); @@ -2606,6 +2637,12 @@ static const struct of_device_id smsc911x_dt_ids[] = { MODULE_DEVICE_TABLE(of, smsc911x_dt_ids); #endif +static const struct acpi_device_id smsc911x_acpi_ids[] = { + { "LNRO001B", }, + { "ARMH9118", }, + { } +}; + static struct platform_driver smsc911x_driver = { .probe = smsc911x_drv_probe, .remove = smsc911x_drv_remove, @@ -2614,6 +2651,7 @@ static struct platform_driver smsc911x_driver = { .owner = THIS_MODULE, .pm = SMSC911X_PM_OPS, .of_match_table = of_match_ptr(smsc911x_dt_ids), + .acpi_match_table = ACPI_PTR(smsc911x_acpi_ids), }, };