diff mbox

net/smsc911x: add device tree probe support

Message ID 20110726010154.GG21641@S2100-06.ap.freescale.net
State New
Headers show

Commit Message

Shawn Guo July 26, 2011, 1:01 a.m. UTC 
On Mon, Jul 25, 2011 at 03:37:23PM -0600, Grant Likely wrote:
> On Mon, Jul 25, 2011 at 05:44:00PM +0800, Shawn Guo wrote:
> > It adds device tree probe support for smsc911x driver.
> > 
> > Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> > Cc: Grant Likely <grant.likely@secretlab.ca>
> > Cc: Steve Glendinning <steve.glendinning@smsc.com>
> > Cc: David S. Miller <davem@davemloft.net>
> > ---
> >  Documentation/devicetree/bindings/net/smsc.txt |   34 +++++++
> >  drivers/net/smsc911x.c                         |  123 +++++++++++++++++++-----
> >  2 files changed, 132 insertions(+), 25 deletions(-)
> >  create mode 100644 Documentation/devicetree/bindings/net/smsc.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/net/smsc.txt b/Documentation/devicetree/bindings/net/smsc.txt
> > new file mode 100644
> > index 0000000..1920695
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/net/smsc.txt
> > @@ -0,0 +1,34 @@
> > +* Smart Mixed-Signal Connectivity (SMSC) LAN Controller
> > +
> > +Required properties:
> > +- compatible : Should be "smsc,lan<model>""smsc,lan"
> 
> Drop "smsc,lan".  That's far too generic.
> 
The following devices are supported by the driver.

LAN9115, LAN9116, LAN9117, LAN9118
LAN9215, LAN9216, LAN9217, LAN9218
LAN9210, LAN9211
LAN9220, LAN9221

If we only keep specific <model> as the compatible, we will have a
long match table which is actually used nowhere to distinguish the
device.

So we need some level generic compatible to save the meaningless
long match table.  What about: 

static const struct of_device_id smsc_dt_ids[] = {
        { .compatible = "smsc,lan9", },
        { /* sentinel */ }
};

Or:

static const struct of_device_id smsc_dt_ids[] = {
        { .compatible = "smsc,lan91", },
        { .compatible = "smsc,lan92", },
        { /* sentinel */ }
};

> > +- reg : Address and length of the io space for SMSC LAN
> > +- smsc-int-gpios : Should specify the GPIO for SMSC LAN interrupt line
> 
> This looks broken.  Shouldn't this be specified as a normal
> "interrupts" property?
> 
> > +- phy-mode : String, operation mode of the PHY interface.
> > +  Supported values are: "mii", "gmii", "sgmii", "tbi", "rmii",
> > +  "rgmii", "rgmii-id", "rgmii-rxid", "rgmii-txid", "rtbi", "smii".
> > +
> > +Optional properties:
> > +- smsc,irq-active-high : Indicates the IRQ polarity is active-low
> > +- smsc,irq-push-pull : Indicates the IRQ type is push-pull
> > +- smsc,register-needs-shift : Indicates the register access needs shift
> > +- smsc,access-in-32bit : Indicates the access to controller is in 32-bit
> > +  mode
> 
> Currently, reg-io-width and reg-shift are being used to manipulate
> register access on ns16550 serial ports.  The same thing can be used
> here.  See bindings/tty/serial/of-serial.txt
> 
They are not exactly same.  reg-io-width and reg-shift in of-serial.txt
are giving a number, while register-needs-shift and access-in-32bit
here just tells a flag.  But if you have a strong position to make
them consistent, I can change them to reg-io-width and reg-shift and
get smsc911x parse numbers instead of flags.

> 
> > +- smsc,force-internal-phy : Forces SMSC LAN controller to use
> > +  internal PHY
> > +- smsc,force-external-phy : Forces SMSC LAN controller to use
> > +  external PHY
> 
> I would expect using an external phy would also expect a phy-device
> property to connect to the phy node.
> 
I do not understand the details.  But from the comment below in the
code, I guess the "external phy" is not external?

/* Autodetects and enables external phy if present on supported chips.
 * autodetection can be overridden by specifying SMSC911X_FORCE_INTERNAL_PHY
 * or SMSC911X_FORCE_EXTERNAL_PHY in the platform_data flags. */

> > +- smsc,save-mac-address : Indicates that mac address needs to be saved
> > +  before resetting the controller
> > +- local-mac-address : 6 bytes, mac address
> > +
> > +Examples:
> > +
> > +lan9220@f4000000 {
> > +	compatible = "smsc,lan9220", "smsc,lan";
> > +	reg = <0xf4000000 0x2000000>;
> > +	phy-mode = "mii";
> > +	smsc-int-gpios = <&gpio1 31 0>; /* GPIO2_31 */
> > +	smsc,irq-push-pull;
> > +	smsc,access-in-32bit;
> > +};
> > diff --git a/drivers/net/smsc911x.c b/drivers/net/smsc911x.c
> > index b9016a3..0097048 100644
> > --- a/drivers/net/smsc911x.c
> > +++ b/drivers/net/smsc911x.c
> > @@ -53,6 +53,10 @@
> >  #include <linux/phy.h>
> >  #include <linux/smsc911x.h>
> >  #include <linux/device.h>
> > +#include <linux/of.h>
> > +#include <linux/of_device.h>
> > +#include <linux/of_gpio.h>
> > +#include <linux/of_net.h>
> >  #include "smsc911x.h"
> >  
> >  #define SMSC_CHIPNAME		"smsc911x"
> > @@ -2095,25 +2099,67 @@ static const struct smsc911x_ops shifted_smsc911x_ops = {
> >  	.tx_writefifo = smsc911x_tx_writefifo_shift,
> >  };
> >  
> > +#ifdef CONFIG_OF
> > +static int __devinit smsc911x_probe_config_dt(
> > +				struct smsc911x_platform_config *config,
> > +				struct device_node *np)
> > +{
> > +	const char *mac;
> > +
> > +	if (!np)
> > +		return -ENODEV;
> > +
> > +	config->phy_interface = of_get_phy_mode(np);
> > +
> > +	mac = of_get_mac_address(np);
> > +	if (mac)
> > +		memcpy(config->mac, mac, ETH_ALEN);
> > +
> > +	if (of_get_property(np, "smsc,irq-active-high", NULL))
> > +		config->irq_polarity = SMSC911X_IRQ_POLARITY_ACTIVE_HIGH;
> > +
> > +	if (of_get_property(np, "smsc,irq-push-pull", NULL))
> > +		config->irq_type = SMSC911X_IRQ_TYPE_PUSH_PULL;
> > +
> > +	if (of_get_property(np, "smsc,register-needs-shift", NULL))
> > +		config->shift = 1;
> > +
> > +	if (of_get_property(np, "smsc,access-in-32bit", NULL))
> > +		config->flags |= SMSC911X_USE_32BIT;
> > +
> > +	if (of_get_property(np, "smsc,force-internal-phy", NULL))
> > +		config->flags |= SMSC911X_FORCE_INTERNAL_PHY;
> > +
> > +	if (of_get_property(np, "smsc,force-external-phy", NULL))
> > +		config->flags |= SMSC911X_FORCE_EXTERNAL_PHY;
> > +
> > +	if (of_get_property(np, "smsc,save-mac-address", NULL))
> > +		config->flags |= SMSC911X_SAVE_MAC_ADDRESS;
> > +
> > +	return 0;
> > +}
> > +#else
> > +static inline int smsc911x_probe_config_dt(
> > +				struct smsc911x_platform_config *config,
> > +				struct device_node *np)
> > +{
> > +	return -ENODEV;
> > +}
> > +#endif /* CONFIG_OF */
> > +
> >  static int __devinit smsc911x_drv_probe(struct platform_device *pdev)
> >  {
> > +	struct device_node *np = pdev->dev.of_node;
> >  	struct net_device *dev;
> >  	struct smsc911x_data *pdata;
> >  	struct smsc911x_platform_config *config = pdev->dev.platform_data;
> >  	struct resource *res, *irq_res;
> >  	unsigned int intcfg = 0;
> > -	int res_size, irq_flags;
> > -	int retval;
> > +	int irq_gpio, res_size, irq_flags = 0;
> > +	int retval = 0;
> >  
> >  	pr_info("Driver version %s\n", SMSC_DRV_VERSION);
> >  
> > -	/* platform data specifies irq & dynamic bus configuration */
> > -	if (!pdev->dev.platform_data) {
> > -		pr_warn("platform_data not provided\n");
> > -		retval = -ENODEV;
> > -		goto out_0;
> > -	}
> > -
> >  	res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> >  					   "smsc911x-memory");
> >  	if (!res)
> > @@ -2125,13 +2171,6 @@ static int __devinit smsc911x_drv_probe(struct platform_device *pdev)
> >  	}
> >  	res_size = resource_size(res);
> >  
> > -	irq_res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> > -	if (!irq_res) {
> > -		pr_warn("Could not allocate irq resource\n");
> > -		retval = -ENODEV;
> > -		goto out_0;
> > -	}
> > -
> 
> This should still work for the device-tree situation.  Why remove it?
> 
> >  	if (!request_mem_region(res->start, res_size, SMSC_CHIPNAME)) {
> >  		retval = -EBUSY;
> >  		goto out_0;
> > @@ -2148,26 +2187,53 @@ static int __devinit smsc911x_drv_probe(struct platform_device *pdev)
> >  
> >  	pdata = netdev_priv(dev);
> >  
> > -	dev->irq = irq_res->start;
> > -	irq_flags = irq_res->flags & IRQF_TRIGGER_MASK;
> > -	pdata->ioaddr = ioremap_nocache(res->start, res_size);
> > -
> > -	/* copy config parameters across to pdata */
> > -	memcpy(&pdata->config, config, sizeof(pdata->config));
> > +	if (np) {
> > +		irq_gpio = of_get_named_gpio(np, "smsc-int-gpios", 0);
> > +		retval = gpio_request_one(irq_gpio, GPIOF_IN, "smsc-int-gpio");
> > +		if (!retval)
> > +			dev->irq = gpio_to_irq(irq_gpio);
> 
> Yeah, that's definitely the wrong way to handle this.  If the
> device it wired to a gpio controller, then the gpio controller also
> need to be an interrupt controller to ensure that it can map interrupt
> numbers.
> 

Here it is.  Please let me know if it is what you mean.



> > +	} else {
> > +		irq_res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> > +		if (irq_res) {
> > +			dev->irq = irq_res->start;
> > +			irq_flags = irq_res->flags & IRQF_TRIGGER_MASK;
> > +		} else {
> > +			retval = -ENODEV;
> > +		}
> > +	}
> >  
> > -	pdata->dev = dev;
> > -	pdata->msg_enable = ((1 << debug) - 1);
> > +	if (retval) {
> > +		SMSC_WARN(pdata, probe, "Error smsc911x irq not found");
> > +		retval = -EINVAL;
> > +		goto out_free_netdev_2;
> > +	}
> >  
> > +	pdata->ioaddr = ioremap_nocache(res->start, res_size);
> >  	if (pdata->ioaddr == NULL) {
> >  		SMSC_WARN(pdata, probe, "Error smsc911x base address invalid");
> >  		retval = -ENOMEM;
> >  		goto out_free_netdev_2;
> >  	}
> >  
> > +	pdata->dev = dev;
> > +	pdata->msg_enable = ((1 << debug) - 1);
> > +
> > +	retval = smsc911x_probe_config_dt(&pdata->config, np);
> > +	if (retval && config) {
> > +		/* copy config parameters across to pdata */
> > +		memcpy(&pdata->config, config, sizeof(pdata->config));
> > +		retval = 0;
> > +	}
> > +
> > +	if (retval) {
> > +		SMSC_WARN(pdata, probe, "Error smsc911x config not found");
> > +		goto out_unmap_io_3;
> > +	}
> > +
> >  	/* assume standard, non-shifted, access to HW registers */
> >  	pdata->ops = &standard_smsc911x_ops;
> >  	/* apply the right access if shifting is needed */
> > -	if (config->shift)
> > +	if (pdata->config.shift)
> >  		pdata->ops = &shifted_smsc911x_ops;
> >  
> >  	retval = smsc911x_init(dev);
> > @@ -2314,6 +2380,12 @@ static const struct dev_pm_ops smsc911x_pm_ops = {
> >  #define SMSC911X_PM_OPS NULL
> >  #endif
> >  
> > +static const struct of_device_id smsc_dt_ids[] = {
> > +	{ .compatible = "smsc,lan", },
> 
> As mentioned above, "smsc,lan" is far too generic.
> 
Again

static const struct of_device_id smsc_dt_ids[] = {
        { .compatible = "smsc,lan9", },
        { /* sentinel */ }
};

Or:

static const struct of_device_id smsc_dt_ids[] = {
        { .compatible = "smsc,lan91", },
        { .compatible = "smsc,lan92", },
        { /* sentinel */ }
};

You pick :)

Comments

Nicolas Pitre July 26, 2011, 1:16 a.m. UTC | #1 
On Tue, 26 Jul 2011, Shawn Guo wrote:

> On Mon, Jul 25, 2011 at 03:37:23PM -0600, Grant Likely wrote:
> > On Mon, Jul 25, 2011 at 05:44:00PM +0800, Shawn Guo wrote:
> > > It adds device tree probe support for smsc911x driver.
> > > 
> > > Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> > > Cc: Grant Likely <grant.likely@secretlab.ca>
> > > Cc: Steve Glendinning <steve.glendinning@smsc.com>
> > > Cc: David S. Miller <davem@davemloft.net>
> > > ---
> > >  Documentation/devicetree/bindings/net/smsc.txt |   34 +++++++
> > >  drivers/net/smsc911x.c                         |  123 +++++++++++++++++++-----
> > >  2 files changed, 132 insertions(+), 25 deletions(-)
> > >  create mode 100644 Documentation/devicetree/bindings/net/smsc.txt
> > > 
> > > diff --git a/Documentation/devicetree/bindings/net/smsc.txt b/Documentation/devicetree/bindings/net/smsc.txt
> > > new file mode 100644
> > > index 0000000..1920695
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/net/smsc.txt
> > > @@ -0,0 +1,34 @@
> > > +* Smart Mixed-Signal Connectivity (SMSC) LAN Controller
> > > +
> > > +Required properties:
> > > +- compatible : Should be "smsc,lan<model>""smsc,lan"
> > 
> > Drop "smsc,lan".  That's far too generic.
> > 
> The following devices are supported by the driver.
> 
> LAN9115, LAN9116, LAN9117, LAN9118
> LAN9215, LAN9216, LAN9217, LAN9218
> LAN9210, LAN9211
> LAN9220, LAN9221
> 
> If we only keep specific <model> as the compatible, we will have a
> long match table which is actually used nowhere to distinguish the
> device.
> 
> So we need some level generic compatible to save the meaningless
> long match table.  What about: 
> 
> static const struct of_device_id smsc_dt_ids[] = {
>         { .compatible = "smsc,lan9", },
>         { /* sentinel */ }
> };
> 
> Or:
> 
> static const struct of_device_id smsc_dt_ids[] = {
>         { .compatible = "smsc,lan91", },
>         { .compatible = "smsc,lan92", },
>         { /* sentinel */ }
> };

None of this unambiguously distinguish the devices supported by this 
driver and the smc91x driver which supports LAN91C92, LAN91C94, 
LAN91C95, LAN91C96, LAN91C100, LAN91C110.


Nicolas
Shawn Guo July 26, 2011, 1:30 a.m. UTC | #2 
On Mon, Jul 25, 2011 at 09:16:40PM -0400, Nicolas Pitre wrote:
> On Tue, 26 Jul 2011, Shawn Guo wrote:
> 
> > On Mon, Jul 25, 2011 at 03:37:23PM -0600, Grant Likely wrote:
> > > On Mon, Jul 25, 2011 at 05:44:00PM +0800, Shawn Guo wrote:
> > > > It adds device tree probe support for smsc911x driver.
> > > > 
> > > > Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> > > > Cc: Grant Likely <grant.likely@secretlab.ca>
> > > > Cc: Steve Glendinning <steve.glendinning@smsc.com>
> > > > Cc: David S. Miller <davem@davemloft.net>
> > > > ---
> > > >  Documentation/devicetree/bindings/net/smsc.txt |   34 +++++++
> > > >  drivers/net/smsc911x.c                         |  123 +++++++++++++++++++-----
> > > >  2 files changed, 132 insertions(+), 25 deletions(-)
> > > >  create mode 100644 Documentation/devicetree/bindings/net/smsc.txt
> > > > 
> > > > diff --git a/Documentation/devicetree/bindings/net/smsc.txt b/Documentation/devicetree/bindings/net/smsc.txt
> > > > new file mode 100644
> > > > index 0000000..1920695
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/net/smsc.txt
> > > > @@ -0,0 +1,34 @@
> > > > +* Smart Mixed-Signal Connectivity (SMSC) LAN Controller
> > > > +
> > > > +Required properties:
> > > > +- compatible : Should be "smsc,lan<model>""smsc,lan"
> > > 
> > > Drop "smsc,lan".  That's far too generic.
> > > 
> > The following devices are supported by the driver.
> > 
> > LAN9115, LAN9116, LAN9117, LAN9118
> > LAN9215, LAN9216, LAN9217, LAN9218
> > LAN9210, LAN9211
> > LAN9220, LAN9221
> > 
> > If we only keep specific <model> as the compatible, we will have a
> > long match table which is actually used nowhere to distinguish the
> > device.
> > 
> > So we need some level generic compatible to save the meaningless
> > long match table.  What about: 
> > 
> > static const struct of_device_id smsc_dt_ids[] = {
> >         { .compatible = "smsc,lan9", },
> >         { /* sentinel */ }
> > };
> > 
> > Or:
> > 
> > static const struct of_device_id smsc_dt_ids[] = {
> >         { .compatible = "smsc,lan91", },
> >         { .compatible = "smsc,lan92", },
> >         { /* sentinel */ }
> > };
> 
> None of this unambiguously distinguish the devices supported by this 
> driver and the smc91x driver which supports LAN91C92, LAN91C94, 
> LAN91C95, LAN91C96, LAN91C100, LAN91C110.
> 
So you suggest to make a long list to explicitly tell the device type
that the driver supports?
Nicolas Pitre July 26, 2011, 2:28 a.m. UTC | #3 
On Tue, 26 Jul 2011, Shawn Guo wrote:

> On Mon, Jul 25, 2011 at 09:16:40PM -0400, Nicolas Pitre wrote:
> > On Tue, 26 Jul 2011, Shawn Guo wrote:
> > 
> > > On Mon, Jul 25, 2011 at 03:37:23PM -0600, Grant Likely wrote:
> > > > On Mon, Jul 25, 2011 at 05:44:00PM +0800, Shawn Guo wrote:
> > > > > It adds device tree probe support for smsc911x driver.
> > > > > 
> > > > > Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> > > > > Cc: Grant Likely <grant.likely@secretlab.ca>
> > > > > Cc: Steve Glendinning <steve.glendinning@smsc.com>
> > > > > Cc: David S. Miller <davem@davemloft.net>
> > > > > ---
> > > > >  Documentation/devicetree/bindings/net/smsc.txt |   34 +++++++
> > > > >  drivers/net/smsc911x.c                         |  123 +++++++++++++++++++-----
> > > > >  2 files changed, 132 insertions(+), 25 deletions(-)
> > > > >  create mode 100644 Documentation/devicetree/bindings/net/smsc.txt
> > > > > 
> > > > > diff --git a/Documentation/devicetree/bindings/net/smsc.txt b/Documentation/devicetree/bindings/net/smsc.txt
> > > > > new file mode 100644
> > > > > index 0000000..1920695
> > > > > --- /dev/null
> > > > > +++ b/Documentation/devicetree/bindings/net/smsc.txt
> > > > > @@ -0,0 +1,34 @@
> > > > > +* Smart Mixed-Signal Connectivity (SMSC) LAN Controller
> > > > > +
> > > > > +Required properties:
> > > > > +- compatible : Should be "smsc,lan<model>""smsc,lan"
> > > > 
> > > > Drop "smsc,lan".  That's far too generic.
> > > > 
> > > The following devices are supported by the driver.
> > > 
> > > LAN9115, LAN9116, LAN9117, LAN9118
> > > LAN9215, LAN9216, LAN9217, LAN9218
> > > LAN9210, LAN9211
> > > LAN9220, LAN9221
> > > 
> > > If we only keep specific <model> as the compatible, we will have a
> > > long match table which is actually used nowhere to distinguish the
> > > device.
> > > 
> > > So we need some level generic compatible to save the meaningless
> > > long match table.  What about: 
> > > 
> > > static const struct of_device_id smsc_dt_ids[] = {
> > >         { .compatible = "smsc,lan9", },
> > >         { /* sentinel */ }
> > > };
> > > 
> > > Or:
> > > 
> > > static const struct of_device_id smsc_dt_ids[] = {
> > >         { .compatible = "smsc,lan91", },
> > >         { .compatible = "smsc,lan92", },
> > >         { /* sentinel */ }
> > > };
> > 
> > None of this unambiguously distinguish the devices supported by this 
> > driver and the smc91x driver which supports LAN91C92, LAN91C94, 
> > LAN91C95, LAN91C96, LAN91C100, LAN91C110.
> > 
> So you suggest to make a long list to explicitly tell the device type
> that the driver supports?

I'm not suggesting anything.  :-)  I'm merely pointing out that the 
above .compatible = "smsc,lan9" or .compatible = "smsc,lan91" are too 
generic given that there is another driver with different devices to 
which they could also apply.


Nicolas
Shawn Guo July 29, 2011, 2:36 a.m. UTC | #4 
On Mon, Jul 25, 2011 at 10:28:05PM -0400, Nicolas Pitre wrote:
> On Tue, 26 Jul 2011, Shawn Guo wrote:
> 
> > On Mon, Jul 25, 2011 at 09:16:40PM -0400, Nicolas Pitre wrote:
> > > On Tue, 26 Jul 2011, Shawn Guo wrote:
> > > 
> > > > On Mon, Jul 25, 2011 at 03:37:23PM -0600, Grant Likely wrote:
> > > > > On Mon, Jul 25, 2011 at 05:44:00PM +0800, Shawn Guo wrote:
> > > > > > It adds device tree probe support for smsc911x driver.
> > > > > > 
> > > > > > Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> > > > > > Cc: Grant Likely <grant.likely@secretlab.ca>
> > > > > > Cc: Steve Glendinning <steve.glendinning@smsc.com>
> > > > > > Cc: David S. Miller <davem@davemloft.net>
> > > > > > ---
> > > > > >  Documentation/devicetree/bindings/net/smsc.txt |   34 +++++++
> > > > > >  drivers/net/smsc911x.c                         |  123 +++++++++++++++++++-----
> > > > > >  2 files changed, 132 insertions(+), 25 deletions(-)
> > > > > >  create mode 100644 Documentation/devicetree/bindings/net/smsc.txt
> > > > > > 
> > > > > > diff --git a/Documentation/devicetree/bindings/net/smsc.txt b/Documentation/devicetree/bindings/net/smsc.txt
> > > > > > new file mode 100644
> > > > > > index 0000000..1920695
> > > > > > --- /dev/null
> > > > > > +++ b/Documentation/devicetree/bindings/net/smsc.txt
> > > > > > @@ -0,0 +1,34 @@
> > > > > > +* Smart Mixed-Signal Connectivity (SMSC) LAN Controller
> > > > > > +
> > > > > > +Required properties:
> > > > > > +- compatible : Should be "smsc,lan<model>""smsc,lan"
> > > > > 
> > > > > Drop "smsc,lan".  That's far too generic.
> > > > > 
> > > > The following devices are supported by the driver.
> > > > 
> > > > LAN9115, LAN9116, LAN9117, LAN9118
> > > > LAN9215, LAN9216, LAN9217, LAN9218
> > > > LAN9210, LAN9211
> > > > LAN9220, LAN9221
> > > > 
> > > > If we only keep specific <model> as the compatible, we will have a
> > > > long match table which is actually used nowhere to distinguish the
> > > > device.
> > > > 
> > > > So we need some level generic compatible to save the meaningless
> > > > long match table.  What about: 
> > > > 
> > > > static const struct of_device_id smsc_dt_ids[] = {
> > > >         { .compatible = "smsc,lan9", },
> > > >         { /* sentinel */ }
> > > > };
> > > > 
> > > > Or:
> > > > 
> > > > static const struct of_device_id smsc_dt_ids[] = {
> > > >         { .compatible = "smsc,lan91", },
> > > >         { .compatible = "smsc,lan92", },
> > > >         { /* sentinel */ }
> > > > };
> > > 
> > > None of this unambiguously distinguish the devices supported by this 
> > > driver and the smc91x driver which supports LAN91C92, LAN91C94, 
> > > LAN91C95, LAN91C96, LAN91C100, LAN91C110.
> > > 
> > So you suggest to make a long list to explicitly tell the device type
> > that the driver supports?
> 
> I'm not suggesting anything.  :-)  I'm merely pointing out that the 
> above .compatible = "smsc,lan9" or .compatible = "smsc,lan91" are too 
> generic given that there is another driver with different devices to 
> which they could also apply.
> 
Since I do not get any suggestion here, I'm going to follow the driver
name to use '911' as the model number.  Please tell me if there is any
better one.
Grant Likely July 29, 2011, 3:45 p.m. UTC | #5 
On Tue, Jul 26, 2011 at 09:30:26AM +0800, Shawn Guo wrote:
> On Mon, Jul 25, 2011 at 09:16:40PM -0400, Nicolas Pitre wrote:
> > On Tue, 26 Jul 2011, Shawn Guo wrote:
> > 
> > > On Mon, Jul 25, 2011 at 03:37:23PM -0600, Grant Likely wrote:
> > > > On Mon, Jul 25, 2011 at 05:44:00PM +0800, Shawn Guo wrote:
> > > > > It adds device tree probe support for smsc911x driver.
> > > > > 
> > > > > Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> > > > > Cc: Grant Likely <grant.likely@secretlab.ca>
> > > > > Cc: Steve Glendinning <steve.glendinning@smsc.com>
> > > > > Cc: David S. Miller <davem@davemloft.net>
> > > > > ---
> > > > >  Documentation/devicetree/bindings/net/smsc.txt |   34 +++++++
> > > > >  drivers/net/smsc911x.c                         |  123 +++++++++++++++++++-----
> > > > >  2 files changed, 132 insertions(+), 25 deletions(-)
> > > > >  create mode 100644 Documentation/devicetree/bindings/net/smsc.txt
> > > > > 
> > > > > diff --git a/Documentation/devicetree/bindings/net/smsc.txt b/Documentation/devicetree/bindings/net/smsc.txt
> > > > > new file mode 100644
> > > > > index 0000000..1920695
> > > > > --- /dev/null
> > > > > +++ b/Documentation/devicetree/bindings/net/smsc.txt
> > > > > @@ -0,0 +1,34 @@
> > > > > +* Smart Mixed-Signal Connectivity (SMSC) LAN Controller
> > > > > +
> > > > > +Required properties:
> > > > > +- compatible : Should be "smsc,lan<model>""smsc,lan"
> > > > 
> > > > Drop "smsc,lan".  That's far too generic.
> > > > 
> > > The following devices are supported by the driver.
> > > 
> > > LAN9115, LAN9116, LAN9117, LAN9118
> > > LAN9215, LAN9216, LAN9217, LAN9218
> > > LAN9210, LAN9211
> > > LAN9220, LAN9221
> > > 
> > > If we only keep specific <model> as the compatible, we will have a
> > > long match table which is actually used nowhere to distinguish the
> > > device.
> > > 
> > > So we need some level generic compatible to save the meaningless
> > > long match table.  What about: 
> > > 
> > > static const struct of_device_id smsc_dt_ids[] = {
> > >         { .compatible = "smsc,lan9", },
> > >         { /* sentinel */ }
> > > };
> > > 
> > > Or:
> > > 
> > > static const struct of_device_id smsc_dt_ids[] = {
> > >         { .compatible = "smsc,lan91", },
> > >         { .compatible = "smsc,lan92", },
> > >         { /* sentinel */ }
> > > };
> > 
> > None of this unambiguously distinguish the devices supported by this 
> > driver and the smc91x driver which supports LAN91C92, LAN91C94, 
> > LAN91C95, LAN91C96, LAN91C100, LAN91C110.
> > 
> So you suggest to make a long list to explicitly tell the device type
> that the driver supports?

If newer devices are 100% backwards compatible with an older device,
then the newer device doesn't need to appear in this list because the
device node can claim compatibility.

If it isn't backwards compatible, then you do need an entry in this
list.

g.
Grant Likely July 29, 2011, 3:47 p.m. UTC | #6 
On Fri, Jul 29, 2011 at 10:36:26AM +0800, Shawn Guo wrote:
> On Mon, Jul 25, 2011 at 10:28:05PM -0400, Nicolas Pitre wrote:
> > On Tue, 26 Jul 2011, Shawn Guo wrote:
> > 
> > > On Mon, Jul 25, 2011 at 09:16:40PM -0400, Nicolas Pitre wrote:
> > > > On Tue, 26 Jul 2011, Shawn Guo wrote:
> > > > 
> > > > > On Mon, Jul 25, 2011 at 03:37:23PM -0600, Grant Likely wrote:
> > > > > > On Mon, Jul 25, 2011 at 05:44:00PM +0800, Shawn Guo wrote:
> > > > > > > It adds device tree probe support for smsc911x driver.
> > > > > > > 
> > > > > > > Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> > > > > > > Cc: Grant Likely <grant.likely@secretlab.ca>
> > > > > > > Cc: Steve Glendinning <steve.glendinning@smsc.com>
> > > > > > > Cc: David S. Miller <davem@davemloft.net>
> > > > > > > ---
> > > > > > >  Documentation/devicetree/bindings/net/smsc.txt |   34 +++++++
> > > > > > >  drivers/net/smsc911x.c                         |  123 +++++++++++++++++++-----
> > > > > > >  2 files changed, 132 insertions(+), 25 deletions(-)
> > > > > > >  create mode 100644 Documentation/devicetree/bindings/net/smsc.txt
> > > > > > > 
> > > > > > > diff --git a/Documentation/devicetree/bindings/net/smsc.txt b/Documentation/devicetree/bindings/net/smsc.txt
> > > > > > > new file mode 100644
> > > > > > > index 0000000..1920695
> > > > > > > --- /dev/null
> > > > > > > +++ b/Documentation/devicetree/bindings/net/smsc.txt
> > > > > > > @@ -0,0 +1,34 @@
> > > > > > > +* Smart Mixed-Signal Connectivity (SMSC) LAN Controller
> > > > > > > +
> > > > > > > +Required properties:
> > > > > > > +- compatible : Should be "smsc,lan<model>""smsc,lan"
> > > > > > 
> > > > > > Drop "smsc,lan".  That's far too generic.
> > > > > > 
> > > > > The following devices are supported by the driver.
> > > > > 
> > > > > LAN9115, LAN9116, LAN9117, LAN9118
> > > > > LAN9215, LAN9216, LAN9217, LAN9218
> > > > > LAN9210, LAN9211
> > > > > LAN9220, LAN9221
> > > > > 
> > > > > If we only keep specific <model> as the compatible, we will have a
> > > > > long match table which is actually used nowhere to distinguish the
> > > > > device.
> > > > > 
> > > > > So we need some level generic compatible to save the meaningless
> > > > > long match table.  What about: 
> > > > > 
> > > > > static const struct of_device_id smsc_dt_ids[] = {
> > > > >         { .compatible = "smsc,lan9", },
> > > > >         { /* sentinel */ }
> > > > > };
> > > > > 
> > > > > Or:
> > > > > 
> > > > > static const struct of_device_id smsc_dt_ids[] = {
> > > > >         { .compatible = "smsc,lan91", },
> > > > >         { .compatible = "smsc,lan92", },
> > > > >         { /* sentinel */ }
> > > > > };
> > > > 
> > > > None of this unambiguously distinguish the devices supported by this 
> > > > driver and the smc91x driver which supports LAN91C92, LAN91C94, 
> > > > LAN91C95, LAN91C96, LAN91C100, LAN91C110.
> > > > 
> > > So you suggest to make a long list to explicitly tell the device type
> > > that the driver supports?
> > 
> > I'm not suggesting anything.  :-)  I'm merely pointing out that the 
> > above .compatible = "smsc,lan9" or .compatible = "smsc,lan91" are too 
> > generic given that there is another driver with different devices to 
> > which they could also apply.
> > 
> Since I do not get any suggestion here, I'm going to follow the driver
> name to use '911' as the model number.  Please tell me if there is any
> better one.

What is the manufacturer part name?  lan9111 or lan91c11?  The
manufacturer's documented part number is almost always preferred.

g.
Shawn Guo July 29, 2011, 4:03 p.m. UTC | #7 
On Fri, Jul 29, 2011 at 09:47:23AM -0600, Grant Likely wrote:
> On Fri, Jul 29, 2011 at 10:36:26AM +0800, Shawn Guo wrote:
> > On Mon, Jul 25, 2011 at 10:28:05PM -0400, Nicolas Pitre wrote:
> > > On Tue, 26 Jul 2011, Shawn Guo wrote:
> > > 
> > > > On Mon, Jul 25, 2011 at 09:16:40PM -0400, Nicolas Pitre wrote:
> > > > > On Tue, 26 Jul 2011, Shawn Guo wrote:
> > > > > 
> > > > > > On Mon, Jul 25, 2011 at 03:37:23PM -0600, Grant Likely wrote:
> > > > > > > On Mon, Jul 25, 2011 at 05:44:00PM +0800, Shawn Guo wrote:
> > > > > > > > It adds device tree probe support for smsc911x driver.
> > > > > > > > 
> > > > > > > > Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> > > > > > > > Cc: Grant Likely <grant.likely@secretlab.ca>
> > > > > > > > Cc: Steve Glendinning <steve.glendinning@smsc.com>
> > > > > > > > Cc: David S. Miller <davem@davemloft.net>
> > > > > > > > ---
> > > > > > > >  Documentation/devicetree/bindings/net/smsc.txt |   34 +++++++
> > > > > > > >  drivers/net/smsc911x.c                         |  123 +++++++++++++++++++-----
> > > > > > > >  2 files changed, 132 insertions(+), 25 deletions(-)
> > > > > > > >  create mode 100644 Documentation/devicetree/bindings/net/smsc.txt
> > > > > > > > 
> > > > > > > > diff --git a/Documentation/devicetree/bindings/net/smsc.txt b/Documentation/devicetree/bindings/net/smsc.txt
> > > > > > > > new file mode 100644
> > > > > > > > index 0000000..1920695
> > > > > > > > --- /dev/null
> > > > > > > > +++ b/Documentation/devicetree/bindings/net/smsc.txt
> > > > > > > > @@ -0,0 +1,34 @@
> > > > > > > > +* Smart Mixed-Signal Connectivity (SMSC) LAN Controller
> > > > > > > > +
> > > > > > > > +Required properties:
> > > > > > > > +- compatible : Should be "smsc,lan<model>""smsc,lan"
> > > > > > > 
> > > > > > > Drop "smsc,lan".  That's far too generic.
> > > > > > > 
> > > > > > The following devices are supported by the driver.
> > > > > > 
> > > > > > LAN9115, LAN9116, LAN9117, LAN9118
> > > > > > LAN9215, LAN9216, LAN9217, LAN9218
> > > > > > LAN9210, LAN9211
> > > > > > LAN9220, LAN9221
> > > > > > 
> > > > > > If we only keep specific <model> as the compatible, we will have a
> > > > > > long match table which is actually used nowhere to distinguish the
> > > > > > device.
> > > > > > 
> > > > > > So we need some level generic compatible to save the meaningless
> > > > > > long match table.  What about: 
> > > > > > 
> > > > > > static const struct of_device_id smsc_dt_ids[] = {
> > > > > >         { .compatible = "smsc,lan9", },
> > > > > >         { /* sentinel */ }
> > > > > > };
> > > > > > 
> > > > > > Or:
> > > > > > 
> > > > > > static const struct of_device_id smsc_dt_ids[] = {
> > > > > >         { .compatible = "smsc,lan91", },
> > > > > >         { .compatible = "smsc,lan92", },
> > > > > >         { /* sentinel */ }
> > > > > > };
> > > > > 
> > > > > None of this unambiguously distinguish the devices supported by this 
> > > > > driver and the smc91x driver which supports LAN91C92, LAN91C94, 
> > > > > LAN91C95, LAN91C96, LAN91C100, LAN91C110.
> > > > > 
> > > > So you suggest to make a long list to explicitly tell the device type
> > > > that the driver supports?
> > > 
> > > I'm not suggesting anything.  :-)  I'm merely pointing out that the 
> > > above .compatible = "smsc,lan9" or .compatible = "smsc,lan91" are too 
> > > generic given that there is another driver with different devices to 
> > > which they could also apply.
> > > 
> > Since I do not get any suggestion here, I'm going to follow the driver
> > name to use '911' as the model number.  Please tell me if there is any
> > better one.
> 
> What is the manufacturer part name?  lan9111 or lan91c11?  The
> manufacturer's documented part number is almost always preferred.
> 
I just posted the v2 of the patch, and chose to use 'smsc,lan9115'
which is the first model supported in the driver.  This is the same
approach I used with i.mx device bindings.
Grant Likely July 29, 2011, 4:26 p.m. UTC | #8 
On Sat, Jul 30, 2011 at 12:03:44AM +0800, Shawn Guo wrote:
> On Fri, Jul 29, 2011 at 09:47:23AM -0600, Grant Likely wrote:
> > On Fri, Jul 29, 2011 at 10:36:26AM +0800, Shawn Guo wrote:
> > > Since I do not get any suggestion here, I'm going to follow the driver
> > > name to use '911' as the model number.  Please tell me if there is any
> > > better one.
> > 
> > What is the manufacturer part name?  lan9111 or lan91c11?  The
> > manufacturer's documented part number is almost always preferred.
> > 
> I just posted the v2 of the patch, and chose to use 'smsc,lan9115'
> which is the first model supported in the driver.  This is the same
> approach I used with i.mx device bindings.

You haven't answered the question.

g.
Shawn Guo July 29, 2011, 5:13 p.m. UTC | #9 
On Fri, Jul 29, 2011 at 10:26:29AM -0600, Grant Likely wrote:
> On Sat, Jul 30, 2011 at 12:03:44AM +0800, Shawn Guo wrote:
> > On Fri, Jul 29, 2011 at 09:47:23AM -0600, Grant Likely wrote:
> > > On Fri, Jul 29, 2011 at 10:36:26AM +0800, Shawn Guo wrote:
> > > > Since I do not get any suggestion here, I'm going to follow the driver
> > > > name to use '911' as the model number.  Please tell me if there is any
> > > > better one.
> > > 
> > > What is the manufacturer part name?  lan9111 or lan91c11?  The
> > > manufacturer's documented part number is almost always preferred.
> > > 
> > I just posted the v2 of the patch, and chose to use 'smsc,lan9115'
> > which is the first model supported in the driver.  This is the same
> > approach I used with i.mx device bindings.
> 
> You haven't answered the question.
> 
For 9115 example, the part number in data sheet is "LAN9115".
Grant Likely July 31, 2011, 12:42 a.m. UTC | #10 
On Sat, Jul 30, 2011 at 01:13:15AM +0800, Shawn Guo wrote:
> On Fri, Jul 29, 2011 at 10:26:29AM -0600, Grant Likely wrote:
> > On Sat, Jul 30, 2011 at 12:03:44AM +0800, Shawn Guo wrote:
> > > On Fri, Jul 29, 2011 at 09:47:23AM -0600, Grant Likely wrote:
> > > > On Fri, Jul 29, 2011 at 10:36:26AM +0800, Shawn Guo wrote:
> > > > > Since I do not get any suggestion here, I'm going to follow the driver
> > > > > name to use '911' as the model number.  Please tell me if there is any
> > > > > better one.
> > > > 
> > > > What is the manufacturer part name?  lan9111 or lan91c11?  The
> > > > manufacturer's documented part number is almost always preferred.
> > > > 
> > > I just posted the v2 of the patch, and chose to use 'smsc,lan9115'
> > > which is the first model supported in the driver.  This is the same
> > > approach I used with i.mx device bindings.
> > 
> > You haven't answered the question.
> > 
> For 9115 example, the part number in data sheet is "LAN9115".

Okay, good.  Thanks.

g.
diff mbox

Patch

diff --git a/arch/arm/boot/dts/imx53-ard.dts b/arch/arm/boot/dts/imx53-ard.dts
index 6a007f1..0b17af8 100644
--- a/arch/arm/boot/dts/imx53-ard.dts
+++ b/arch/arm/boot/dts/imx53-ard.dts
@@ -320,7 +320,8 @@ 
 			compatible = "smsc,lan9220", "smsc,lan";
 			reg = <0xf4000000 0x2000000>;
 			phy-mode = "mii";
-			smsc-int-gpios = <&gpio1 31 0>; /* GPIO2_31 */
+			interrupt-parent = <&gpio1>;
+			interrupts = <31>;
 			smsc,irq-push-pull;
 			smsc,access-in-32bit;
 		};
diff --git a/arch/arm/boot/dts/imx53.dtsi b/arch/arm/boot/dts/imx53.dtsi
index 746221c..224f67f 100644
--- a/arch/arm/boot/dts/imx53.dtsi
+++ b/arch/arm/boot/dts/imx53.dtsi
@@ -89,6 +89,8 @@ 
 			interrupts = <50 51>;
 			gpio-controller;
 			#gpio-cells = <2>;
+			interrupt-controller;
+			#interrupt-cells = <1>;
 		};
 
 		gpio1: gpio@53f88000 { /* GPIO2 */
@@ -97,6 +99,8 @@ 
 			interrupts = <52 53>;
 			gpio-controller;
 			#gpio-cells = <2>;
+			interrupt-controller;
+			#interrupt-cells = <1>;
 		};
 
 		gpio2: gpio@53f8c000 { /* GPIO3 */
@@ -105,6 +109,8 @@ 
 			interrupts = <54 55>;
 			gpio-controller;
 			#gpio-cells = <2>;
+			interrupt-controller;
+			#interrupt-cells = <1>;
 		};
 
 		gpio3: gpio@53f90000 { /* GPIO4 */
@@ -113,6 +119,8 @@ 
 			interrupts = <56 57>;
 			gpio-controller;
 			#gpio-cells = <2>;
+			interrupt-controller;
+			#interrupt-cells = <1>;
 		};
 
 		wdt@53f98000 { /* WDOG1 */
@@ -950,6 +958,8 @@ 
 			interrupts = <103 104>;
 			gpio-controller;
 			#gpio-cells = <2>;
+			interrupt-controller;
+			#interrupt-cells = <1>;
 		};
 
 		gpio5: gpio@53fe0000 { /* GPIO6 */
@@ -958,6 +968,8 @@ 
 			interrupts = <105 106>;
 			gpio-controller;
 			#gpio-cells = <2>;
+			interrupt-controller;
+			#interrupt-cells = <1>;
 		};
 
 		gpio6: gpio@53fe4000 { /* GPIO7 */
@@ -966,6 +978,8 @@ 
 			interrupts = <107 108>;
 			gpio-controller;
 			#gpio-cells = <2>;
+			interrupt-controller;
+			#interrupt-cells = <1>;
 		};
 
 		i2c@53fec000 { /* I2C3 */
diff --git a/arch/arm/mach-mx5/imx53-dt.c b/arch/arm/mach-mx5/imx53-dt.c
index ac06f04..04dc1ab 100644
--- a/arch/arm/mach-mx5/imx53-dt.c
+++ b/arch/arm/mach-mx5/imx53-dt.c
@@ -51,6 +51,11 @@  static const struct of_device_id imx53_tzic_of_match[] __initconst = {
 	{ /* sentinel */ }
 };
 
+static const struct of_device_id imx53_gpio_of_match[] __initconst = {
+	{ .compatible = "fsl,imx53-gpio", },
+	{ /* sentinel */ }
+};
+
 static const struct of_device_id imx53_iomuxc_of_match[] __initconst = {
 	{ .compatible = "fsl,imx53-iomuxc", },
 	{ /* sentinel */ }
@@ -90,12 +95,28 @@  static void __init imx53_ard_eim_config(void)
 
 static void __init imx53_dt_init(void)
 {
+	int gpio_irq = MXC_INTERNAL_IRQS + ARCH_NR_GPIOS;
+
 	if (of_machine_is_compatible("fsl,imx53-ard"))
 		imx53_ard_eim_config();
 
 	mxc_iomuxc_dt_init(imx53_iomuxc_of_match);
 
 	irq_domain_generate_simple(imx53_tzic_of_match, MX53_TZIC_BASE_ADDR, 0);
+	gpio_irq -= 32;
+	irq_domain_generate_simple(imx53_gpio_of_match, MX53_GPIO1_BASE_ADDR, gpio_irq);
+	gpio_irq -= 32;
+	irq_domain_generate_simple(imx53_gpio_of_match, MX53_GPIO2_BASE_ADDR, gpio_irq);
+	gpio_irq -= 32;
+	irq_domain_generate_simple(imx53_gpio_of_match, MX53_GPIO3_BASE_ADDR, gpio_irq);
+	gpio_irq -= 32;
+	irq_domain_generate_simple(imx53_gpio_of_match, MX53_GPIO4_BASE_ADDR, gpio_irq);
+	gpio_irq -= 32;
+	irq_domain_generate_simple(imx53_gpio_of_match, MX53_GPIO5_BASE_ADDR, gpio_irq);
+	gpio_irq -= 32;
+	irq_domain_generate_simple(imx53_gpio_of_match, MX53_GPIO6_BASE_ADDR, gpio_irq);
+	gpio_irq -= 32;
+	irq_domain_generate_simple(imx53_gpio_of_match, MX53_GPIO7_BASE_ADDR, gpio_irq);
 
 	of_platform_populate(NULL, of_default_bus_match_table,
 			     imx53_auxdata_lookup, NULL);
diff --git a/drivers/net/smsc911x.c b/drivers/net/smsc911x.c
index 0097048..6dd025e 100644
--- a/drivers/net/smsc911x.c
+++ b/drivers/net/smsc911x.c
@@ -2187,25 +2187,10 @@  static int __devinit smsc911x_drv_probe(struct platform_device *pdev)
 
 	pdata = netdev_priv(dev);
 
-	if (np) {
-		irq_gpio = of_get_named_gpio(np, "smsc-int-gpios", 0);
-		retval = gpio_request_one(irq_gpio, GPIOF_IN, "smsc-int-gpio");
-		if (!retval)
-			dev->irq = gpio_to_irq(irq_gpio);
-	} else {
-		irq_res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
-		if (irq_res) {
-			dev->irq = irq_res->start;
-			irq_flags = irq_res->flags & IRQF_TRIGGER_MASK;
-		} else {
-			retval = -ENODEV;
-		}
-	}
-
-	if (retval) {
-		SMSC_WARN(pdata, probe, "Error smsc911x irq not found");
-		retval = -EINVAL;
-		goto out_free_netdev_2;
+	irq_res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
+	if (irq_res) {
+		dev->irq = irq_res->start;
+		irq_flags = irq_res->flags & IRQF_TRIGGER_MASK;
 	}
 
 	pdata->ioaddr = ioremap_nocache(res->start, res_size);