From mboxrd@z Thu Jan 1 00:00:00 1970 From: Simon Glass Date: Tue, 18 Apr 2017 18:12:37 -0600 Subject: [U-Boot] [PATCH 03/11] drivers: phy: add generic PHY framework In-Reply-To: References: <1492168089-15437-1-git-send-email-jjhiblot@ti.com> <1492168089-15437-4-git-send-email-jjhiblot@ti.com> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hi ,[..] >>> +UCLASS_DRIVER(generic_phy) = { >>> + .id = UCLASS_PHY, >> >> I would like the uclass name to match the header file and the uclass >> name. So if you are calling this generic_phy, then the uclass should >> be named this too. Same with the directory drivers/phy. If you want to >> rename it all to 'phy' then that is fine too. > > IMO 'phy' would be the best option. > Unfortunately there are already tons of functions starting with 'phy_' and > they're used for the ethernet phy. So I would propose to use 'phy' > everywhere except for the API where 'generic_phy_' can be used to prefix the > functions. > What do you think ? Sounds good. That would be easy to clean up later once Ethernet Phy is done. > >>> + .name = "generic_phy", >>> +}; >>> diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h >>> index 8c92d0b..9d34a32 100644 >>> --- a/include/dm/uclass-id.h >>> +++ b/include/dm/uclass-id.h >>> @@ -83,6 +83,7 @@ enum uclass_id { >>> UCLASS_VIDEO, /* Video or LCD device */ >>> UCLASS_VIDEO_BRIDGE, /* Video bridge, e.g. DisplayPort to >>> LVDS */ >>> UCLASS_VIDEO_CONSOLE, /* Text console driver for video device >>> */ >>> + UCLASS_PHY, /* generic PHY device */ >> >> Physical layer device? Can you make your comment a bit more useful? >> >>> UCLASS_COUNT, >>> UCLASS_INVALID = -1, >>> diff --git a/include/generic-phy.h b/include/generic-phy.h >>> new file mode 100644 >>> index 0000000..24475f0 >>> --- /dev/null >>> +++ b/include/generic-phy.h >>> @@ -0,0 +1,36 @@ >>> +/* >>> + * Copyright (C) 2017 Texas Instruments Incorporated - >>> http://www.ti.com/ >>> + * Written by Jean-Jacques Hiblot >>> + * >>> + * SPDX-License-Identifier: GPL-2.0+ >>> + */ >>> + >>> +#ifndef __GENERIC_PHY_H >>> +#define __GENERIC_PHY_H >>> + >>> +/* >>> + * struct udevice_ops - set of function pointers for phy operations >>> + * @init: operation to be performed for initializing phy (optionnal) >> >> optional >> >> Can you please put these comments in front of each function in struct >> generic_phy_ops as is done with other uclasses? Your description >> should explain what the operation does. For example, what happens for >> init()? Since the phy has already been probed, what should you not do >> in probe() but do in init()? > > I'll work on documenting this. Too bad that linux also lacks this > documentation. > The core idea is that probe() does not do much hardware-wise, it sets up > everyting (irqs, mapping, clocks) but the actual initialization of the > hardware is done in init(). OK sounds good. Regards, Simon