[v2] net: dm9000: Allow instantiation using device tree

This patch adds Device Tree support to dm9000 driver.

Signed-off-by: Tomasz Figa <tomasz.figa@gmail.com>
---

Changes since v1:
 - dropped davicom,simple-phy property as it seems to be of no use
 - used of_get_mac_address() to get MAC address from device tree
 - replaced #ifdef CONFIG_OF around dm9000_parse_dt() with IS_ENABLED

 .../devicetree/bindings/net/davicom-dm9000.txt     | 26 +++++++++++++
 .../devicetree/bindings/vendor-prefixes.txt        |  1 +
 drivers/net/ethernet/davicom/dm9000.c              | 44 ++++++++++++++++++++++
 3 files changed, 71 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/davicom-dm9000.txt

Hi,

On 05/19/2013 01:16 PM, Tomasz Figa wrote:
> +++ b/Documentation/devicetree/bindings/net/davicom-dm9000.txt
> @@ -0,0 +1,26 @@
> +Davicom DM9000 Fast Ethernet controller
> +
> +Required properties:
> +- compatible = "davicom,dm9000";
> +- reg : physical addresses and sizes of registers, must contain 2 entries:
> +    first entry : address register,
> +    second entry : address register.

Two address registers ? Shouldn't one of these be "data register" ?

> +Example:
> +
> +	ethernet@18000000 {
> +		compatible = "davicom,dm9000";
> +		reg =<0x18000000 0x2 0x18000004 0x2>;
> +		interrupt-parent =<&gpn>;
> +		interrupts =<7 4>;
> +		local-mac-address = [00 00 de ad be ef];
> +		davicom,no-eeprom;
> +	};

> +static struct dm9000_plat_data *dm9000_parse_dt(struct device *dev)
> +{
> +	struct dm9000_plat_data *pdata;
> +	struct device_node *np = dev->of_node;
> +	const void *mac_addr;
> +
> +	if (!IS_ENABLED(CONFIG_OF) || !np)
> +		return NULL;

Shouldn't ERR_PTR() value be returned here ?

> +	pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
> +	if (!pdata) {
> +		dev_err(dev, "failed to allocate platform data struct\n");

There is no need for this error log, k*alloc already logs any failures.

> @@ -1373,6 +1402,12 @@ dm9000_probe(struct platform_device *pdev)
>   	int i;
>   	u32 id_val;
>
> +	if (!pdata) {
> +		pdata = dm9000_parse_dt(&pdev->dev);
> +		if (IS_ERR(pdata))
> +			return PTR_ERR(pdata);
> +	}

Thanks,

Sylwester
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Hi Sylwester,

On Sunday 19 of May 2013 15:27:53 Sylwester Nawrocki wrote:
> Hi,
> 
> On 05/19/2013 01:16 PM, Tomasz Figa wrote:
> > +++ b/Documentation/devicetree/bindings/net/davicom-dm9000.txt
> > @@ -0,0 +1,26 @@
> > +Davicom DM9000 Fast Ethernet controller
> > +
> > +Required properties:
> > +- compatible = "davicom,dm9000";
> > +- reg : physical addresses and sizes of registers, must contain 2
> > entries: +    first entry : address register,
> > +    second entry : address register.
> 
> Two address registers ? Shouldn't one of these be "data register" ?

Oops. I thought I already corrected this typo. Thanks.

> > +Example:
> > +
> > +	ethernet@18000000 {
> > +		compatible = "davicom,dm9000";
> > +		reg =<0x18000000 0x2 0x18000004 0x2>;
> > +		interrupt-parent =<&gpn>;
> > +		interrupts =<7 4>;
> > +		local-mac-address = [00 00 de ad be ef];
> > +		davicom,no-eeprom;
> > +	};
> > 
> > +static struct dm9000_plat_data *dm9000_parse_dt(struct device *dev)
> > +{
> > +	struct dm9000_plat_data *pdata;
> > +	struct device_node *np = dev->of_node;
> > +	const void *mac_addr;
> > +
> > +	if (!IS_ENABLED(CONFIG_OF) || !np)
> > +		return NULL;
> 
> Shouldn't ERR_PTR() value be returned here ?

Nope. No platform data is a valid case, so no error here.

> > +	pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
> > +	if (!pdata) {
> > +		dev_err(dev, "failed to allocate platform data struct\n");
> 
> There is no need for this error log, k*alloc already logs any failures.

Hmm. Does it print what allocation exactly failed? (e.g. a backtrace)

Not that it would give anything that could help you in an out of memory 
condition like this, but in general it's good to know in what point the 
failure happened.

Best regards,
Tomasz

> > @@ -1373,6 +1402,12 @@ dm9000_probe(struct platform_device *pdev)
> > 
> >   	int i;
> >   	u32 id_val;
> > 
> > +	if (!pdata) {
> > +		pdata = dm9000_parse_dt(&pdev->dev);
> > +		if (IS_ERR(pdata))
> > +			return PTR_ERR(pdata);
> > +	}
> 
> Thanks,
> 
> Sylwester
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On 05/19/2013 03:41 PM, Tomasz Figa wrote:
>>> +static struct dm9000_plat_data *dm9000_parse_dt(struct device *dev)
>>> >  >  +{
>>> >  >  +	struct dm9000_plat_data *pdata;
>>> >  >  +	struct device_node *np = dev->of_node;
>>> >  >  +	const void *mac_addr;
>>> >  >  +
>>> >  >  +	if (!IS_ENABLED(CONFIG_OF) || !np)
>>> >  >  +		return NULL;
>> >
>> >  Shouldn't ERR_PTR() value be returned here ?
>
> Nope. No platform data is a valid case, so no error here.

OK, sorry, I should have checked that.

>>> >  >  +	pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
>>> >  >  +	if (!pdata) {
>>> >  >  +		dev_err(dev, "failed to allocate platform data struct\n");
>> >
>> >  There is no need for this error log, k*alloc already logs any failures.
>
> Hmm. Does it print what allocation exactly failed? (e.g. a backtrace)

It seems the caller tracking is supported in subset of the kernel 
configurations.

> Not that it would give anything that could help you in an out of memory
> condition like this, but in general it's good to know in what point the
> failure happened.

As long as it is ENOMEM and the driver core reports failed probe I wouldn't
really care. Nevertheless I saw patch series removing such already existing
"Not enough memory" kind of error logs from the kernel, pointing out that mm
already provides relevant error logging.

Thanks,
Sylwester
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html