| Message ID | 20210405164643.21130-1-michael@walle.cc |
|---|---|
| Headers | show |
| Series | of: net: support non-platform devices in of_get_mac_address() | expand |
Hi Michael > -static int of_get_mac_addr_nvmem(struct device_node *np, u8 addr) > +static int of_get_mac_addr_nvmem(struct device_node *np, u8 *addr) > { > struct platform_device *pdev = of_find_device_by_node(np); > + struct nvmem_cell *cell; > + const void *mac; > + size_t len; > int ret; > > - if (!pdev) > - return -ENODEV; > + /* Try lookup by device first, there might be a nvmem_cell_lookup > + * associated with a given device. > + */ > + if (pdev) { > + ret = nvmem_get_mac_address(&pdev->dev, addr); > + put_device(&pdev->dev); > + return ret; > + } Can you think of any odd corner case where nvmem_get_mac_address() would fail, but of_nvmem_cell_get(np, "mac-address") would work? Andrew
On Mon, Apr 05, 2021 at 11:46:04PM +0200, Michael Walle wrote: > Hi Andrew, > > Am 2021-04-05 23:34, schrieb Andrew Lunn: > > > -static int of_get_mac_addr_nvmem(struct device_node *np, u8 addr) > > > +static int of_get_mac_addr_nvmem(struct device_node *np, u8 *addr) > > > { > > > struct platform_device *pdev = of_find_device_by_node(np); > > > + struct nvmem_cell *cell; > > > + const void *mac; > > > + size_t len; > > > int ret; > > > > > > - if (!pdev) > > > - return -ENODEV; > > > + /* Try lookup by device first, there might be a nvmem_cell_lookup > > > + * associated with a given device. > > > + */ > > > + if (pdev) { > > > + ret = nvmem_get_mac_address(&pdev->dev, addr); > > > + put_device(&pdev->dev); > > > + return ret; > > > + } > > > > Can you think of any odd corner case where nvmem_get_mac_address() > > would fail, but of_nvmem_cell_get(np, "mac-address") would work? > > You mean, it might make sense to just return here when > nvmem_get_mac_address() will succeed and fall back to the > of_nvmem_cell_get() in case of an error? I've not read the documentation for nvmem_get_mac_address(). I was thinking we might want to return real errors, and -EPROBE_DEFER. But maybe with -ENODEV we should try of_nvmem_cell_get()? But i'm not sure if there are any real use cases? The only thing i can think of is if np points to something deeper inside the device tree than what pdev does? Andrew
Am 2021-04-06 00:13, schrieb Andrew Lunn: > On Mon, Apr 05, 2021 at 11:46:04PM +0200, Michael Walle wrote: >> Hi Andrew, >> >> Am 2021-04-05 23:34, schrieb Andrew Lunn: >> > > -static int of_get_mac_addr_nvmem(struct device_node *np, u8 addr) >> > > +static int of_get_mac_addr_nvmem(struct device_node *np, u8 *addr) >> > > { >> > > struct platform_device *pdev = of_find_device_by_node(np); >> > > + struct nvmem_cell *cell; >> > > + const void *mac; >> > > + size_t len; >> > > int ret; >> > > >> > > - if (!pdev) >> > > - return -ENODEV; >> > > + /* Try lookup by device first, there might be a nvmem_cell_lookup >> > > + * associated with a given device. >> > > + */ >> > > + if (pdev) { >> > > + ret = nvmem_get_mac_address(&pdev->dev, addr); >> > > + put_device(&pdev->dev); >> > > + return ret; >> > > + } >> > >> > Can you think of any odd corner case where nvmem_get_mac_address() >> > would fail, but of_nvmem_cell_get(np, "mac-address") would work? >> >> You mean, it might make sense to just return here when >> nvmem_get_mac_address() will succeed and fall back to the >> of_nvmem_cell_get() in case of an error? > > I've not read the documentation for nvmem_get_mac_address(). I was > thinking we might want to return real errors, and -EPROBE_DEFER. I can't follow, nvmem_get_mac_address() should already return those. > But maybe with -ENODEV we should try of_nvmem_cell_get()? And if this happens - that is nvmem_get_mac_address(&pdev->dev) returns -ENODEV - then of_nvmem_cell_get(np) will also return -ENODEV. Because pdev->dev.of_node == np and nvmem_get_mac_address(&pdev->dev) tries of_nvmem_cell_get(pdev->dev.of_node) first. > But i'm not sure if there are any real use cases? The only thing i can > think of is if np points to something deeper inside the device tree > than what pdev does? But then pdev will be NULL and nvmem_get_mac_address() won't be called at all, no? -michael
> But then pdev will be NULL and nvmem_get_mac_address() won't be called > at all, no? Forget it, it can be added later if there is a real use case. Andrew