Re: [RFC PATCH for Juno 1/2] net: smsc911x add support for probing from ACPI

[Mark Brown]

On Sun, Sep 14, 2014 at 09:14:04PM -0700, Grant Likely wrote:
> On Mon, 01 Sep 2014 19:11:44 +0200, Arnd Bergmann  wrote:

> > > > + 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;
> > > > +}

...

> > There is of course the possibility to set those values based on the
> > acpi_device_id, but that is exactly the part that _DSD is trying to
> > avoid.

> These are merely defaults. DSD parsing, when implemented, would be
> override these default values.

One note of caution here: I do agree that default settings are good but
it's worth having clear rules for how we pick the defaults, and advice
for maintainers on how to pick those rules.  If there's a default people
often want to pick it to match their particular system and it can
sometimes be hard for the maintainer to identify if a given tweak
someone is proposing in the defaults might break some other existing
system.  Having clear guidelines for picking the defaults avoids
arguments and breakage.

The two basic rules I've seen are that we either follow the defaults
the hardware has after reset or we follow the state the hardware is left
in when the kernel starts.  Neither is perfect and sometimes the
bootloader option just doesn't make sense at all but they're at least
clear and simple to understand.

It's not the end of the world to do something else, and sometimes the
way systems are done just doesn't lend itself to providing clear rules,
but if we encourage people to set them they can save grief.


signature.asc
Description: Digital signature

Re: [Linaro-acpi] [RFC PATCH for Juno 1/2] net: smsc911x add support for probing from ACPI

[Catalin Marinas]

On Mon, Sep 15, 2014 at 05:08:44AM +0100, Grant Likely wrote:
> On Mon, 1 Sep 2014 16:28:54 +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
> > 
> > > 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

-- 
Catalin

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [Linaro-acpi] [RFC PATCH for Juno 1/2] net: smsc911x add support for probing from ACPI

[Catalin Marinas]

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.greg...@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

-- 
Catalin

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC PATCH for Juno 1/2] net: smsc911x add support for probing from ACPI

[Mark Brown]

On Sun, Sep 14, 2014 at 09:14:04PM -0700, Grant Likely wrote:
 On Mon, 01 Sep 2014 19:11:44 +0200, Arnd Bergmann a...@arndb.de wrote:

+ 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;
+}

...

  There is of course the possibility to set those values based on the
  acpi_device_id, but that is exactly the part that _DSD is trying to
  avoid.

 These are merely defaults. DSD parsing, when implemented, would be
 override these default values.

One note of caution here: I do agree that default settings are good but
it's worth having clear rules for how we pick the defaults, and advice
for maintainers on how to pick those rules.  If there's a default people
often want to pick it to match their particular system and it can
sometimes be hard for the maintainer to identify if a given tweak
someone is proposing in the defaults might break some other existing
system.  Having clear guidelines for picking the defaults avoids
arguments and breakage.

The two basic rules I've seen are that we either follow the defaults
the hardware has after reset or we follow the state the hardware is left
in when the kernel starts.  Neither is perfect and sometimes the
bootloader option just doesn't make sense at all but they're at least
clear and simple to understand.

It's not the end of the world to do something else, and sometimes the
way systems are done just doesn't lend itself to providing clear rules,
but if we encourage people to set them they can save grief.


signature.asc
Description: Digital signature

Re: [RFC PATCH for Juno 1/2] net: smsc911x add support for probing from ACPI

[Grant Likely]

On Mon, 01 Sep 2014 19:11:44 +0200, Arnd Bergmann  wrote:
> On Monday 01 September 2014 18:04:47 Catalin Marinas wrote:
> > 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).
> 
> Good point, I totally missed that.
> 
> There is of course the possibility to set those values based on the
> acpi_device_id, but that is exactly the part that _DSD is trying to
> avoid.

These are merely defaults. DSD parsing, when implemented, would be
override these default values.

> 
> > >  static int smsc911x_drv_probe(struct platform_device *pdev)
> > >  {
> > >   struct device_node *np = pdev->dev.of_node;
> > > + acpi_handle *ahandle = ACPI_HANDLE(>dev);
> > >   struct net_device *dev;
> > >   struct smsc911x_data *pdata;
> > >   struct smsc911x_platform_config *config = 
> > > dev_get_platdata(>dev);
> > > @@ -2436,6 +2464,9 @@ static int smsc911x_drv_probe(struct 
> > > platform_device *pdev)
> > >   }
> > >  
> > >   retval = smsc911x_probe_config_dt(>config, np);
> > > + if (retval)
> > > + retval = smsc911x_probe_config_acpi(>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.
> 
> Does this really make a difference?

Nope. Only one of DT or ACPI will be matched.

> 
> > I would prefer
> > something which probes only ACPI if the ACPI is enabled (run-time, not
> > config) otherwise DT only. E.g.
> 
> (example missing?)
> 
> I think we should have the equivalent of of_have_populated_dt(), to
> check whether acpi is being used to boot, and have that new function
> be hardcoded to zero in case of !IS_ENABLED(CONFIG_ACPI).

The code already accounts for it. If ACPI isn't enabled, or isn't
populated, then the ACPI_HANDLE macro will return NULL and the
smsc911x_probe_config_acpi() function will fail.

g.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [Linaro-acpi] [RFC PATCH for Juno 1/2] net: smsc911x add support for probing from ACPI

[Grant Likely]

On Mon, 1 Sep 2014 16:28:54 +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
> 
> > 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-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [Linaro-acpi] [RFC PATCH for Juno 1/2] net: smsc911x add support for probing from ACPI

[Grant Likely]

On Mon, 1 Sep 2014 16:28:54 +0100, Graeme Gregory graeme.greg...@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-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC PATCH for Juno 1/2] net: smsc911x add support for probing from ACPI

[Grant Likely]

On Mon, 01 Sep 2014 19:11:44 +0200, Arnd Bergmann a...@arndb.de wrote:
 On Monday 01 September 2014 18:04:47 Catalin Marinas wrote:
  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).
 
 Good point, I totally missed that.
 
 There is of course the possibility to set those values based on the
 acpi_device_id, but that is exactly the part that _DSD is trying to
 avoid.

These are merely defaults. DSD parsing, when implemented, would be
override these default values.

 
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.
 
 Does this really make a difference?

Nope. Only one of DT or ACPI will be matched.

 
  I would prefer
  something which probes only ACPI if the ACPI is enabled (run-time, not
  config) otherwise DT only. E.g.
 
 (example missing?)
 
 I think we should have the equivalent of of_have_populated_dt(), to
 check whether acpi is being used to boot, and have that new function
 be hardcoded to zero in case of !IS_ENABLED(CONFIG_ACPI).

The code already accounts for it. If ACPI isn't enabled, or isn't
populated, then the ACPI_HANDLE macro will return NULL and the
smsc911x_probe_config_acpi() function will fail.

g.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [Linaro-acpi] [RFC PATCH for Juno 1/2] net: smsc911x add support for probing from ACPI

[Jon Masters]

Hi Arnd,

On 09/03/2014 11:09 AM, Arnd Bergmann wrote:
> On Wednesday 03 September 2014 01:00:23 Rafael J. Wysocki wrote:

>> Our intention is specifically not to use "random incompatible bindings"
>> in that.  We'd rather have a common venue and process for establishing
>> new bindings for both DT and _DSD in a compatible way.
> 
> Right, I think everyone is on the same page for the embedded x86 case,
> my point was that there is no consensus about that yet among the
> parties involved in arm64 servers.

However, in the case of MAC devices where you might need to specify just
a couple of properties using _DSD, a specific set of discussions has
been instigated. As you mentioned, though, this pertains only to certain
devices and isn't necessarily true for every Ethernet device.
Consequently, while a Juno PoC patch might use a kludge today, that
doesn't mean it's a limitation of ACPI, just of certain devices.

Jon.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [Linaro-acpi] [RFC PATCH for Juno 1/2] net: smsc911x add support for probing from ACPI

[Jon Masters]

Hi Arnd,

On 09/03/2014 11:09 AM, Arnd Bergmann wrote:
 On Wednesday 03 September 2014 01:00:23 Rafael J. Wysocki wrote:

 Our intention is specifically not to use random incompatible bindings
 in that.  We'd rather have a common venue and process for establishing
 new bindings for both DT and _DSD in a compatible way.
 
 Right, I think everyone is on the same page for the embedded x86 case,
 my point was that there is no consensus about that yet among the
 parties involved in arm64 servers.

However, in the case of MAC devices where you might need to specify just
a couple of properties using _DSD, a specific set of discussions has
been instigated. As you mentioned, though, this pertains only to certain
devices and isn't necessarily true for every Ethernet device.
Consequently, while a Juno PoC patch might use a kludge today, that
doesn't mean it's a limitation of ACPI, just of certain devices.

Jon.

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [Linaro-acpi] [RFC PATCH for Juno 1/2] net: smsc911x add support for probing from ACPI

[Arnd Bergmann]

On Wednesday 03 September 2014 01:00:23 Rafael J. Wysocki wrote:
> On Tuesday, September 02, 2014 05:26:06 PM Mark Brown wrote:
> > 
> > --s3puAW9DMBtS2ARW
> > Content-Type: text/plain; charset=us-ascii
> > Content-Disposition: inline
> > 
> > On Tue, Sep 02, 2014 at 03:42:53PM +0200, Arnd Bergmann wrote:
> > 
> > > The way I recall the discussion, most people were on one extreme
> > > side of the discussion or the other:
> > 
> > > a) We should use _DSD for ARM64 servers to maximize code reuse with
> > > DT-enabled drivers, work around the slow UEFI standardization process,
> > > remain in control of the actual bindings, and avoid the need for
> > > endless per-ID platform-data definitions in drivers.
> > 
> > > b) We should never use _DSD at all, since doing that would have no
> > > advantage over using DT directly, and we should force every device
> > > manufacturer to specify their bindings in an official ACPI document
> > > to prevent random incompatible bindings from being established.
> > > Any device that shows up in servers should not need arbitrary detailed
> > > properties anyway, as the details are supposed to be hidden in AML.
> > 
> > > I can understand the reasons for both approaches, and I find it hard
> > > to say either one is invalid. However, the worst possible outcome in
> > > my opinion would be having to support a mix of the two.
> > 
> > Right, and the x86 embedded folks are going full steam ahead with _DSD
> > regardless so it seems there will be some systems out there using it
> > even if they're not ARM servers.
> 
> Our intention is specifically not to use "random incompatible bindings"
> in that.  We'd rather have a common venue and process for establishing
> new bindings for both DT and _DSD in a compatible way.

Right, I think everyone is on the same page for the embedded x86 case,
my point was that there is no consensus about that yet among the
parties involved in arm64 servers.

Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [Linaro-acpi] [RFC PATCH for Juno 1/2] net: smsc911x add support for probing from ACPI

[Arnd Bergmann]

On Wednesday 03 September 2014 01:00:23 Rafael J. Wysocki wrote:
 On Tuesday, September 02, 2014 05:26:06 PM Mark Brown wrote:
  
  --s3puAW9DMBtS2ARW
  Content-Type: text/plain; charset=us-ascii
  Content-Disposition: inline
  
  On Tue, Sep 02, 2014 at 03:42:53PM +0200, Arnd Bergmann wrote:
  
   The way I recall the discussion, most people were on one extreme
   side of the discussion or the other:
  
   a) We should use _DSD for ARM64 servers to maximize code reuse with
   DT-enabled drivers, work around the slow UEFI standardization process,
   remain in control of the actual bindings, and avoid the need for
   endless per-ID platform-data definitions in drivers.
  
   b) We should never use _DSD at all, since doing that would have no
   advantage over using DT directly, and we should force every device
   manufacturer to specify their bindings in an official ACPI document
   to prevent random incompatible bindings from being established.
   Any device that shows up in servers should not need arbitrary detailed
   properties anyway, as the details are supposed to be hidden in AML.
  
   I can understand the reasons for both approaches, and I find it hard
   to say either one is invalid. However, the worst possible outcome in
   my opinion would be having to support a mix of the two.
  
  Right, and the x86 embedded folks are going full steam ahead with _DSD
  regardless so it seems there will be some systems out there using it
  even if they're not ARM servers.
 
 Our intention is specifically not to use random incompatible bindings
 in that.  We'd rather have a common venue and process for establishing
 new bindings for both DT and _DSD in a compatible way.

Right, I think everyone is on the same page for the embedded x86 case,
my point was that there is no consensus about that yet among the
parties involved in arm64 servers.

Arnd
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [Linaro-acpi] [RFC PATCH for Juno 1/2] net: smsc911x add support for probing from ACPI

[Rafael J. Wysocki]

On Tuesday, September 02, 2014 05:26:06 PM Mark Brown wrote:
> 
> --s3puAW9DMBtS2ARW
> Content-Type: text/plain; charset=us-ascii
> Content-Disposition: inline
> 
> On Tue, Sep 02, 2014 at 03:42:53PM +0200, Arnd Bergmann wrote:
> 
> > The way I recall the discussion, most people were on one extreme
> > side of the discussion or the other:
> 
> > a) We should use _DSD for ARM64 servers to maximize code reuse with
> > DT-enabled drivers, work around the slow UEFI standardization process,
> > remain in control of the actual bindings, and avoid the need for
> > endless per-ID platform-data definitions in drivers.
> 
> > b) We should never use _DSD at all, since doing that would have no
> > advantage over using DT directly, and we should force every device
> > manufacturer to specify their bindings in an official ACPI document
> > to prevent random incompatible bindings from being established.
> > Any device that shows up in servers should not need arbitrary detailed
> > properties anyway, as the details are supposed to be hidden in AML.
> 
> > I can understand the reasons for both approaches, and I find it hard
> > to say either one is invalid. However, the worst possible outcome in
> > my opinion would be having to support a mix of the two.
> 
> Right, and the x86 embedded folks are going full steam ahead with _DSD
> regardless so it seems there will be some systems out there using it
> even if they're not ARM servers.

Our intention is specifically not to use "random incompatible bindings"
in that.  We'd rather have a common venue and process for establishing
new bindings for both DT and _DSD in a compatible way.

Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [Linaro-acpi] [RFC PATCH for Juno 1/2] net: smsc911x add support for probing from ACPI

[Mark Brown]

On Tue, Sep 02, 2014 at 03:42:53PM +0200, Arnd Bergmann wrote:

> The way I recall the discussion, most people were on one extreme
> side of the discussion or the other:

> a) We should use _DSD for ARM64 servers to maximize code reuse with
> DT-enabled drivers, work around the slow UEFI standardization process,
> remain in control of the actual bindings, and avoid the need for
> endless per-ID platform-data definitions in drivers.

> b) We should never use _DSD at all, since doing that would have no
> advantage over using DT directly, and we should force every device
> manufacturer to specify their bindings in an official ACPI document
> to prevent random incompatible bindings from being established.
> Any device that shows up in servers should not need arbitrary detailed
> properties anyway, as the details are supposed to be hidden in AML.

> I can understand the reasons for both approaches, and I find it hard
> to say either one is invalid. However, the worst possible outcome in
> my opinion would be having to support a mix of the two.

Right, and the x86 embedded folks are going full steam ahead with _DSD
regardless so it seems there will be some systems out there using it
even if they're not ARM servers.


signature.asc
Description: Digital signature

Re: [Linaro-acpi] [RFC PATCH for Juno 1/2] net: smsc911x add support for probing from ACPI

[Arnd Bergmann]

On Tuesday 02 September 2014 14:26:52 Catalin Marinas wrote:
> 
> Not much at the KS, I think it will need to be followed up on lkml
> (https://lkml.org/lkml/2014/8/17/10 is the last I'm aware of, not sure
> about any updates in the meantime).
> 
> While the above gets sorted, what's the position from an ARM
> perspective (and covered by Documentation/arm64/arm-acpi.txt)? I think
> the "Device Enumeration" section in this document is fine, it's just the
> kernel infrastructure missing.
> 
> Alternatively, you can say _DSD is not allowed (yet?) but I don't
> particularly like basing the configuration on acpi_device_id like in
> this patch. Which would leave us with ignoring any SoC containing
> devices that require such specific configuration.

The way I recall the discussion, most people were on one extreme
side of the discussion or the other:

a) We should use _DSD for ARM64 servers to maximize code reuse with
DT-enabled drivers, work around the slow UEFI standardization process,
remain in control of the actual bindings, and avoid the need for
endless per-ID platform-data definitions in drivers.

b) We should never use _DSD at all, since doing that would have no
advantage over using DT directly, and we should force every device
manufacturer to specify their bindings in an official ACPI document
to prevent random incompatible bindings from being established.
Any device that shows up in servers should not need arbitrary detailed
properties anyway, as the details are supposed to be hidden in AML.

I can understand the reasons for both approaches, and I find it hard
to say either one is invalid. However, the worst possible outcome in
my opinion would be having to support a mix of the two.

Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC PATCH for Juno 1/2] net: smsc911x add support for probing from ACPI

[Catalin Marinas]

On Mon, Sep 01, 2014 at 06:32:45PM +0100, Graeme Gregory wrote:
> On Mon, Sep 01, 2014 at 07:11:44PM +0200, Arnd Bergmann wrote:
> > On Monday 01 September 2014 18:04:47 Catalin Marinas wrote:
> > > 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).
> > 
> > Good point, I totally missed that.
> > 
> > There is of course the possibility to set those values based on the
> > acpi_device_id, but that is exactly the part that _DSD is trying to
> > avoid.
> 
> This will of course most likely be replaced by _DSD values. I just
> hardcoded for now as _DSD is not yet in the kernel and issues around
> maintenance of bindings are not solved (unless this happened at KS where
> I was not present).

Not much at the KS, I think it will need to be followed up on lkml
(https://lkml.org/lkml/2014/8/17/10 is the last I'm aware of, not sure
about any updates in the meantime).

While the above gets sorted, what's the position from an ARM
perspective (and covered by Documentation/arm64/arm-acpi.txt)? I think
the "Device Enumeration" section in this document is fine, it's just the
kernel infrastructure missing.

Alternatively, you can say _DSD is not allowed (yet?) but I don't
particularly like basing the configuration on acpi_device_id like in
this patch. Which would leave us with ignoring any SoC containing
devices that require such specific configuration.

-- 
Catalin
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC PATCH for Juno 1/2] net: smsc911x add support for probing from ACPI

[Catalin Marinas]

On Mon, Sep 01, 2014 at 06:32:45PM +0100, Graeme Gregory wrote:
 On Mon, Sep 01, 2014 at 07:11:44PM +0200, Arnd Bergmann wrote:
  On Monday 01 September 2014 18:04:47 Catalin Marinas wrote:
   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).
  
  Good point, I totally missed that.
  
  There is of course the possibility to set those values based on the
  acpi_device_id, but that is exactly the part that _DSD is trying to
  avoid.
 
 This will of course most likely be replaced by _DSD values. I just
 hardcoded for now as _DSD is not yet in the kernel and issues around
 maintenance of bindings are not solved (unless this happened at KS where
 I was not present).

Not much at the KS, I think it will need to be followed up on lkml
(https://lkml.org/lkml/2014/8/17/10 is the last I'm aware of, not sure
about any updates in the meantime).

While the above gets sorted, what's the position from an ARM
perspective (and covered by Documentation/arm64/arm-acpi.txt)? I think
the Device Enumeration section in this document is fine, it's just the
kernel infrastructure missing.

Alternatively, you can say _DSD is not allowed (yet?) but I don't
particularly like basing the configuration on acpi_device_id like in
this patch. Which would leave us with ignoring any SoC containing
devices that require such specific configuration.

-- 
Catalin
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [Linaro-acpi] [RFC PATCH for Juno 1/2] net: smsc911x add support for probing from ACPI

[Arnd Bergmann]

On Tuesday 02 September 2014 14:26:52 Catalin Marinas wrote:
 
 Not much at the KS, I think it will need to be followed up on lkml
 (https://lkml.org/lkml/2014/8/17/10 is the last I'm aware of, not sure
 about any updates in the meantime).
 
 While the above gets sorted, what's the position from an ARM
 perspective (and covered by Documentation/arm64/arm-acpi.txt)? I think
 the Device Enumeration section in this document is fine, it's just the
 kernel infrastructure missing.
 
 Alternatively, you can say _DSD is not allowed (yet?) but I don't
 particularly like basing the configuration on acpi_device_id like in
 this patch. Which would leave us with ignoring any SoC containing
 devices that require such specific configuration.

The way I recall the discussion, most people were on one extreme
side of the discussion or the other:

a) We should use _DSD for ARM64 servers to maximize code reuse with
DT-enabled drivers, work around the slow UEFI standardization process,
remain in control of the actual bindings, and avoid the need for
endless per-ID platform-data definitions in drivers.

b) We should never use _DSD at all, since doing that would have no
advantage over using DT directly, and we should force every device
manufacturer to specify their bindings in an official ACPI document
to prevent random incompatible bindings from being established.
Any device that shows up in servers should not need arbitrary detailed
properties anyway, as the details are supposed to be hidden in AML.

I can understand the reasons for both approaches, and I find it hard
to say either one is invalid. However, the worst possible outcome in
my opinion would be having to support a mix of the two.

Arnd
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [Linaro-acpi] [RFC PATCH for Juno 1/2] net: smsc911x add support for probing from ACPI

[Mark Brown]

On Tue, Sep 02, 2014 at 03:42:53PM +0200, Arnd Bergmann wrote:

 The way I recall the discussion, most people were on one extreme
 side of the discussion or the other:

 a) We should use _DSD for ARM64 servers to maximize code reuse with
 DT-enabled drivers, work around the slow UEFI standardization process,
 remain in control of the actual bindings, and avoid the need for
 endless per-ID platform-data definitions in drivers.

 b) We should never use _DSD at all, since doing that would have no
 advantage over using DT directly, and we should force every device
 manufacturer to specify their bindings in an official ACPI document
 to prevent random incompatible bindings from being established.
 Any device that shows up in servers should not need arbitrary detailed
 properties anyway, as the details are supposed to be hidden in AML.

 I can understand the reasons for both approaches, and I find it hard
 to say either one is invalid. However, the worst possible outcome in
 my opinion would be having to support a mix of the two.

Right, and the x86 embedded folks are going full steam ahead with _DSD
regardless so it seems there will be some systems out there using it
even if they're not ARM servers.


signature.asc
Description: Digital signature

Re: [Linaro-acpi] [RFC PATCH for Juno 1/2] net: smsc911x add support for probing from ACPI

[Rafael J. Wysocki]

On Tuesday, September 02, 2014 05:26:06 PM Mark Brown wrote:
 
 --s3puAW9DMBtS2ARW
 Content-Type: text/plain; charset=us-ascii
 Content-Disposition: inline
 
 On Tue, Sep 02, 2014 at 03:42:53PM +0200, Arnd Bergmann wrote:
 
  The way I recall the discussion, most people were on one extreme
  side of the discussion or the other:
 
  a) We should use _DSD for ARM64 servers to maximize code reuse with
  DT-enabled drivers, work around the slow UEFI standardization process,
  remain in control of the actual bindings, and avoid the need for
  endless per-ID platform-data definitions in drivers.
 
  b) We should never use _DSD at all, since doing that would have no
  advantage over using DT directly, and we should force every device
  manufacturer to specify their bindings in an official ACPI document
  to prevent random incompatible bindings from being established.
  Any device that shows up in servers should not need arbitrary detailed
  properties anyway, as the details are supposed to be hidden in AML.
 
  I can understand the reasons for both approaches, and I find it hard
  to say either one is invalid. However, the worst possible outcome in
  my opinion would be having to support a mix of the two.
 
 Right, and the x86 embedded folks are going full steam ahead with _DSD
 regardless so it seems there will be some systems out there using it
 even if they're not ARM servers.

Our intention is specifically not to use random incompatible bindings
in that.  We'd rather have a common venue and process for establishing
new bindings for both DT and _DSD in a compatible way.

Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC PATCH for Juno 1/2] net: smsc911x add support for probing from ACPI

[Mark Brown]

On Mon, Sep 01, 2014 at 06:32:45PM +0100, Graeme Gregory wrote:

> This will of course most likely be replaced by _DSD values. I just
> hardcoded for now as _DSD is not yet in the kernel and issues around
> maintenance of bindings are not solved (unless this happened at KS where
> I was not present).

Not really.  AIUI the official thing is still some combination of
getting a DT binding defined and then use that verbatim (as the x86
embedded people want to though I'm still not clear that the Windows
people are on board with this) and the UEFI processes.  It's possible I
misunderstood though.

Realistically if something is usable on x86 embedded it's going to be
supported on ARMv8 servers too unless we jump through hoops, and even
there real implementations in the wild will probably win out.


signature.asc
Description: Digital signature

Re: [RFC PATCH for Juno 1/2] net: smsc911x add support for probing from ACPI

[Graeme Gregory]

On Mon, Sep 01, 2014 at 07:11:44PM +0200, Arnd Bergmann wrote:
> On Monday 01 September 2014 18:04:47 Catalin Marinas wrote:
> > 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).
> 
> Good point, I totally missed that.
> 
> There is of course the possibility to set those values based on the
> acpi_device_id, but that is exactly the part that _DSD is trying to
> avoid.
> 
This will of course most likely be replaced by _DSD values. I just
hardcoded for now as _DSD is not yet in the kernel and issues around
maintenance of bindings are not solved (unless this happened at KS where
I was not present).

> > >  static int smsc911x_drv_probe(struct platform_device *pdev)
> > >  {
> > >   struct device_node *np = pdev->dev.of_node;
> > > + acpi_handle *ahandle = ACPI_HANDLE(>dev);
> > >   struct net_device *dev;
> > >   struct smsc911x_data *pdata;
> > >   struct smsc911x_platform_config *config = 
> > > dev_get_platdata(>dev);
> > > @@ -2436,6 +2464,9 @@ static int smsc911x_drv_probe(struct 
> > > platform_device *pdev)
> > >   }
> > >  
> > >   retval = smsc911x_probe_config_dt(>config, np);
> > > + if (retval)
> > > + retval = smsc911x_probe_config_acpi(>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.
> 
> Does this really make a difference?
> 
> > I would prefer
> > something which probes only ACPI if the ACPI is enabled (run-time, not
> > config) otherwise DT only. E.g.
> 
> (example missing?)
> 
> I think we should have the equivalent of of_have_populated_dt(), to
> check whether acpi is being used to boot, and have that new function
> be hardcoded to zero in case of !IS_ENABLED(CONFIG_ACPI).
> 
if (!acpi_disabled) is the equivalent if I understand you correctly.

But people until this point had expressed a preference for checking
.of_node and ACPI_HANDLE() to this point. This is obviously mutable
though depending on community preference.

Graeme

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [Linaro-acpi] [RFC PATCH for Juno 1/2] net: smsc911x add support for probing from ACPI

[Sudeep Holla]

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(>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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC PATCH for Juno 1/2] net: smsc911x add support for probing from ACPI

[Catalin Marinas]

On Mon, Sep 01, 2014 at 06:11:44PM +0100, Arnd Bergmann wrote:
> On Monday 01 September 2014 18:04:47 Catalin Marinas wrote:
> > 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).
> 
> Good point, I totally missed that.
> 
> There is of course the possibility to set those values based on the
> acpi_device_id, but that is exactly the part that _DSD is trying to
> avoid.

I would prefer to avoid acpi_device_id checks. This would defeat the
alleged aim of ACPI to run newer hardware configuration with older
kernels.

> > >  static int smsc911x_drv_probe(struct platform_device *pdev)
> > >  {
> > >   struct device_node *np = pdev->dev.of_node;
> > > + acpi_handle *ahandle = ACPI_HANDLE(>dev);
> > >   struct net_device *dev;
> > >   struct smsc911x_data *pdata;
> > >   struct smsc911x_platform_config *config = 
> > > dev_get_platdata(>dev);
> > > @@ -2436,6 +2464,9 @@ static int smsc911x_drv_probe(struct 
> > > platform_device *pdev)
> > >   }
> > >  
> > >   retval = smsc911x_probe_config_dt(>config, np);
> > > + if (retval)
> > > + retval = smsc911x_probe_config_acpi(>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.
> 
> Does this really make a difference?

Not the order (well, someone may think that if they unflatten the DT in
a vendor kernel even though it boot as ACPI, they could get away with a
mix of DT and ACPI ;)).

> > I would prefer
> > something which probes only ACPI if the ACPI is enabled (run-time, not
> > config) otherwise DT only. E.g.
> 
> (example missing?)

I was looking through the code and forgot. Something like:

if (acpi_disabled)
retval = smsc911x_probe_config_dt();
else
retval = smsc911x_probe_config_acpi();

> I think we should have the equivalent of of_have_populated_dt(), to
> check whether acpi is being used to boot, and have that new function
> be hardcoded to zero in case of !IS_ENABLED(CONFIG_ACPI).

I think you meant hardcoded to 1 when !ACPI. That would work as well.

-- 
Catalin
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [Linaro-acpi] [RFC PATCH for Juno 1/2] net: smsc911x add support for probing from ACPI

[Catalin Marinas]

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(>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.

-- 
Catalin
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC PATCH for Juno 1/2] net: smsc911x add support for probing from ACPI

[Arnd Bergmann]

On Monday 01 September 2014 18:04:47 Catalin Marinas wrote:
> 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).

Good point, I totally missed that.

There is of course the possibility to set those values based on the
acpi_device_id, but that is exactly the part that _DSD is trying to
avoid.

> >  static int smsc911x_drv_probe(struct platform_device *pdev)
> >  {
> >   struct device_node *np = pdev->dev.of_node;
> > + acpi_handle *ahandle = ACPI_HANDLE(>dev);
> >   struct net_device *dev;
> >   struct smsc911x_data *pdata;
> >   struct smsc911x_platform_config *config = 
> > dev_get_platdata(>dev);
> > @@ -2436,6 +2464,9 @@ static int smsc911x_drv_probe(struct platform_device 
> > *pdev)
> >   }
> >  
> >   retval = smsc911x_probe_config_dt(>config, np);
> > + if (retval)
> > + retval = smsc911x_probe_config_acpi(>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.

Does this really make a difference?

> I would prefer
> something which probes only ACPI if the ACPI is enabled (run-time, not
> config) otherwise DT only. E.g.

(example missing?)

I think we should have the equivalent of of_have_populated_dt(), to
check whether acpi is being used to boot, and have that new function
be hardcoded to zero in case of !IS_ENABLED(CONFIG_ACPI).

Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [Linaro-acpi] [RFC PATCH for Juno 1/2] net: smsc911x add support for probing from ACPI

[Sudeep Holla]

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(>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-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC PATCH for Juno 1/2] net: smsc911x add support for probing from ACPI

[Catalin Marinas]

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(>dev);
>   struct net_device *dev;
>   struct smsc911x_data *pdata;
>   struct smsc911x_platform_config *config = dev_get_platdata(>dev);
> @@ -2436,6 +2464,9 @@ static int smsc911x_drv_probe(struct platform_device 
> *pdev)
>   }
>  
>   retval = smsc911x_probe_config_dt(>config, np);
> + if (retval)
> + retval = smsc911x_probe_config_acpi(>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.

-- 
Catalin
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [Linaro-acpi] [RFC PATCH for Juno 1/2] net: smsc911x add support for probing from ACPI

[Graeme Gregory]

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(>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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [Linaro-acpi] [RFC PATCH for Juno 1/2] net: smsc911x add support for probing from ACPI

[Mark Brown]

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(>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.


signature.asc
Description: Digital signature

Re: [Linaro-acpi] [RFC PATCH for Juno 1/2] net: smsc911x add support for probing from ACPI

[Catalin Marinas]

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(>dev);

without any #ifdef's.

-- 
Catalin
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [Linaro-acpi] [RFC PATCH for Juno 1/2] net: smsc911x add support for probing from ACPI

[Arnd Bergmann]

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-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [Linaro-acpi] [RFC PATCH for Juno 1/2] net: smsc911x add support for probing from ACPI

[Graeme Gregory]

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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [Linaro-acpi] [RFC PATCH for Juno 1/2] net: smsc911x add support for probing from ACPI

[Arnd Bergmann]

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.

Also, -ENOSYS is probably the wrong return value. I think you mean
-ENXIO.

Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[RFC PATCH for Juno 1/2] net: smsc911x add support for probing from ACPI

[Hanjun Guo]

From: Graeme Gregory 

This is a standard platform device to resources are converted in the
ACPI core in the same fasion as DT resources. For the other DT
provided information there is _DSD for ACPI.

Signed-off-by: Graeme Gregory 
---
 drivers/net/ethernet/smsc/smsc911x.c |   38 ++
 1 file changed, 38 insertions(+)

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 
 #include 
 #include 
+#include 
 #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(>dev);
struct net_device *dev;
struct smsc911x_data *pdata;
struct smsc911x_platform_config *config = dev_get_platdata(>dev);
@@ -2436,6 +2464,9 @@ static int smsc911x_drv_probe(struct platform_device 
*pdev)
}
 
retval = smsc911x_probe_config_dt(>config, np);
+   if (retval)
+   retval = smsc911x_probe_config_acpi(>config, ahandle);
+
if (retval && config) {
/* copy config parameters across to pdata */
memcpy(>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),
},
 };
 
-- 
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[RFC PATCH for Juno 1/2] net: smsc911x add support for probing from ACPI

[Hanjun Guo]

From: Graeme Gregory graeme.greg...@linaro.org

This is a standard platform device to resources are converted in the
ACPI core in the same fasion as DT resources. For the other DT
provided information there is _DSD for ACPI.

Signed-off-by: Graeme Gregory graeme.greg...@linaro.org
---
 drivers/net/ethernet/smsc/smsc911x.c |   38 ++
 1 file changed, 38 insertions(+)

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),
},
 };
 
-- 
1.7.9.5

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [Linaro-acpi] [RFC PATCH for Juno 1/2] net: smsc911x add support for probing from ACPI

[Arnd Bergmann]

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.

Also, -ENOSYS is probably the wrong return value. I think you mean
-ENXIO.

Arnd
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [Linaro-acpi] [RFC PATCH for Juno 1/2] net: smsc911x add support for probing from ACPI

[Graeme Gregory]

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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [Linaro-acpi] [RFC PATCH for Juno 1/2] net: smsc911x add support for probing from ACPI

[Arnd Bergmann]

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-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [Linaro-acpi] [RFC PATCH for Juno 1/2] net: smsc911x add support for probing from ACPI

[Catalin Marinas]

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.

-- 
Catalin
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [Linaro-acpi] [RFC PATCH for Juno 1/2] net: smsc911x add support for probing from ACPI

[Mark Brown]

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.


signature.asc
Description: Digital signature

Re: [Linaro-acpi] [RFC PATCH for Juno 1/2] net: smsc911x add support for probing from ACPI

[Graeme Gregory]

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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC PATCH for Juno 1/2] net: smsc911x add support for probing from ACPI

[Catalin Marinas]

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.

-- 
Catalin
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [Linaro-acpi] [RFC PATCH for Juno 1/2] net: smsc911x add support for probing from ACPI

[Sudeep Holla]

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-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC PATCH for Juno 1/2] net: smsc911x add support for probing from ACPI

[Arnd Bergmann]

On Monday 01 September 2014 18:04:47 Catalin Marinas wrote:
 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).

Good point, I totally missed that.

There is of course the possibility to set those values based on the
acpi_device_id, but that is exactly the part that _DSD is trying to
avoid.

   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.

Does this really make a difference?

 I would prefer
 something which probes only ACPI if the ACPI is enabled (run-time, not
 config) otherwise DT only. E.g.

(example missing?)

I think we should have the equivalent of of_have_populated_dt(), to
check whether acpi is being used to boot, and have that new function
be hardcoded to zero in case of !IS_ENABLED(CONFIG_ACPI).

Arnd
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [Linaro-acpi] [RFC PATCH for Juno 1/2] net: smsc911x add support for probing from ACPI

[Catalin Marinas]

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.

-- 
Catalin
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC PATCH for Juno 1/2] net: smsc911x add support for probing from ACPI

[Catalin Marinas]

On Mon, Sep 01, 2014 at 06:11:44PM +0100, Arnd Bergmann wrote:
 On Monday 01 September 2014 18:04:47 Catalin Marinas wrote:
  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).
 
 Good point, I totally missed that.
 
 There is of course the possibility to set those values based on the
 acpi_device_id, but that is exactly the part that _DSD is trying to
 avoid.

I would prefer to avoid acpi_device_id checks. This would defeat the
alleged aim of ACPI to run newer hardware configuration with older
kernels.

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.
 
 Does this really make a difference?

Not the order (well, someone may think that if they unflatten the DT in
a vendor kernel even though it boot as ACPI, they could get away with a
mix of DT and ACPI ;)).

  I would prefer
  something which probes only ACPI if the ACPI is enabled (run-time, not
  config) otherwise DT only. E.g.
 
 (example missing?)

I was looking through the code and forgot. Something like:

if (acpi_disabled)
retval = smsc911x_probe_config_dt();
else
retval = smsc911x_probe_config_acpi();

 I think we should have the equivalent of of_have_populated_dt(), to
 check whether acpi is being used to boot, and have that new function
 be hardcoded to zero in case of !IS_ENABLED(CONFIG_ACPI).

I think you meant hardcoded to 1 when !ACPI. That would work as well.

-- 
Catalin
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [Linaro-acpi] [RFC PATCH for Juno 1/2] net: smsc911x add support for probing from ACPI

[Sudeep Holla]

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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC PATCH for Juno 1/2] net: smsc911x add support for probing from ACPI

[Graeme Gregory]

On Mon, Sep 01, 2014 at 07:11:44PM +0200, Arnd Bergmann wrote:
 On Monday 01 September 2014 18:04:47 Catalin Marinas wrote:
  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).
 
 Good point, I totally missed that.
 
 There is of course the possibility to set those values based on the
 acpi_device_id, but that is exactly the part that _DSD is trying to
 avoid.
 
This will of course most likely be replaced by _DSD values. I just
hardcoded for now as _DSD is not yet in the kernel and issues around
maintenance of bindings are not solved (unless this happened at KS where
I was not present).

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.
 
 Does this really make a difference?
 
  I would prefer
  something which probes only ACPI if the ACPI is enabled (run-time, not
  config) otherwise DT only. E.g.
 
 (example missing?)
 
 I think we should have the equivalent of of_have_populated_dt(), to
 check whether acpi is being used to boot, and have that new function
 be hardcoded to zero in case of !IS_ENABLED(CONFIG_ACPI).
 
if (!acpi_disabled) is the equivalent if I understand you correctly.

But people until this point had expressed a preference for checking
.of_node and ACPI_HANDLE() to this point. This is obviously mutable
though depending on community preference.

Graeme

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC PATCH for Juno 1/2] net: smsc911x add support for probing from ACPI

[Mark Brown]

On Mon, Sep 01, 2014 at 06:32:45PM +0100, Graeme Gregory wrote:

 This will of course most likely be replaced by _DSD values. I just
 hardcoded for now as _DSD is not yet in the kernel and issues around
 maintenance of bindings are not solved (unless this happened at KS where
 I was not present).

Not really.  AIUI the official thing is still some combination of
getting a DT binding defined and then use that verbatim (as the x86
embedded people want to though I'm still not clear that the Windows
people are on board with this) and the UEFI processes.  It's possible I
misunderstood though.

Realistically if something is usable on x86 embedded it's going to be
supported on ARMv8 servers too unless we jump through hoops, and even
there real implementations in the wild will probably win out.


signature.asc
Description: Digital signature