On Wed, Nov 30, 2016 at 2:16 AM, Olliver Schinagl <[email protected]> wrote: > Hey Simon, > > > > On 29-11-16 22:41, Simon Glass wrote: >> >> Hi Oliver, >> >> On 28 November 2016 at 03:38, Olliver Schinagl <[email protected]> wrote: >>> >>> On 27-11-16 18:02, Simon Glass wrote: >>>> >>>> Hi, >>>> >>>> On 25 November 2016 at 08:38, Olliver Schinagl <[email protected]> >>>> wrote: >>>>> >>>>> Add the read_rom_hwaddr net_op hook so that it can be called from >>>>> boards >>>>> to read the mac from a ROM chip. >>>>> >>>>> Signed-off-by: Olliver Schinagl <[email protected]> >>>>> --- >>>>> drivers/net/designware.c | 16 ++++++++++++++++ >>>>> 1 file changed, 16 insertions(+) >>>>> >>>>> diff --git a/drivers/net/designware.c b/drivers/net/designware.c >>>>> index 9e6d726..3f2f67c 100644 >>>>> --- a/drivers/net/designware.c >>>>> +++ b/drivers/net/designware.c >>>>> @@ -230,6 +230,21 @@ static int _dw_write_hwaddr(struct dw_eth_dev >>>>> *priv, >>>>> u8 *mac_id) >>>>> return 0; >>>>> } >>>>> >>>>> +__weak int dw_board_read_rom_hwaddr(unsigned char *enetaddr, int id) >>>>> +{ >>>>> + return -ENOSYS; >>>>> +} >>>>> + >>>>> +static int designware_eth_read_rom_hwaddr(struct udevice *dev) >>>>> +{ >>>>> + struct eth_pdata *pdata = dev_get_platdata(dev); >>>>> + >>>>> + if (!dev) >>>>> + return -ENOSYS; >>>>> + >>>>> + return dw_board_read_rom_hwaddr(pdata->enetaddr, dev->seq); >>>>> +} >>>>> + >>>>> static void dw_adjust_link(struct eth_mac_regs *mac_p, >>>>> struct phy_device *phydev) >>>>> { >>>>> @@ -685,6 +700,7 @@ static const struct eth_ops designware_eth_ops = { >>>>> .free_pkt = designware_eth_free_pkt, >>>>> .stop = designware_eth_stop, >>>>> .write_hwaddr = designware_eth_write_hwaddr, >>>>> + .read_rom_hwaddr = designware_eth_read_rom_hwaddr, >>>>> }; >>>>> >>>>> static int designware_eth_ofdata_to_platdata(struct udevice *dev) >>>> >>>> You should not call board code from a driver. But since this is a >>>> sunxi driver, why not move all the code that reads the serial number >>>> into this file? >>> >>> Hey Simon, >>> >>> unless I missunderstand, this is how Joe suggested in a while ago, and >>> how >>> it has been implemented in a few other drivers. Can you elaborate a bit >>> more? >> >> Yes...drivers must not call into board-specific code, nor have >> board-specific #defines. This limits their usefulness for other >> boards. > > Hmm, well as I said, I just followed Joe's suggestion with his example. also > isn't this exactly how the zynq does it as well?
Sorry for misleading you. Simon has since convinced me that making a separate board-specific driver that leverages the core driver's code is a cleaner approach, and now what I recommend. >> Adding hooks like this (particularly with the word 'board' in the >> name) should be avoided. >> >> We end up with bidirectional coupling between the board and the >> driver. The board should use the driver but not the other way around. >> I understand that you are trying to get around this by using a weak >> function, but with driver model I'm really trying hard to avoid weak >> functions. They normally indicate an ad-hoc API and can generally be >> avoided with a bit more design thought. >> >> If you have a standard way of reading the serial number which is >> supported by all sunxi boards, then you should be able to add your >> changes to the sunxi Ethernet driver (which uses designware.c?). Then >> you can leave the designware.c code alone and avoid adding a hook. > > Well yes and no. We use designware, but also sunxi_emac, and some > sdio_realtek that does not have a driver yet. But in essence, this is > somewhat what I do in this patch I guess. I have the weak driver specific > function in the sunxi code. > > But I think I'm starting to understand your solution and will read up on the > rockchip patches and rewrite this bit. > >> >> In a sense you end up subclassing the designware driver. >> >> Also see this series which deals with a similar problem with rockchip: >> >> http://lists.denx.de/pipermail/u-boot/2016-November/274256.html >> >> Regards, >> Simon > > > _______________________________________________ > U-Boot mailing list > [email protected] > http://lists.denx.de/mailman/listinfo/u-boot _______________________________________________ U-Boot mailing list [email protected] http://lists.denx.de/mailman/listinfo/u-boot
