On Tue, Nov 20, 2018 at 02:38:32PM +0800, Ming Huang wrote:
My feedback was:
This would be more clear as "platform specific" than "cpld relative".
I did not realise this wasn't a Hisilicon component when reviewing the original set.
I approve of this change, but can you tell me why it is included in this set? If the goal is to make the M41T83 support platform independent, should the library also move to Silicon/ST/?
So could you please update the commit message, and add a subsequent patch moving Library/M41T83RealTimeClockLib to Silicon/STMicroelectronics (and updating D06.dsc to match)? I do not care if it is not perfectly abstracted yet - we can deal with that when we have other users of the component in the tree.
Sorry for missing update the commit message this patch. I will update it in v3. I try to move the library to Silicon/STMicroelectronics, but M41T83RealTimeClockLib depend on I2CLib in Hisilicon, so can't move the library to STMicroelectronics. Main gist of this patch is making this library as a common module in Hisilicon.
And a PCI component depends on PciLib - that does not mean it should live in edk2 MdePkg. And it does not mean that this library should be considered Hisilicon-specific. If we get other platforms with this component, they will not be permitted to add duplicated code - we will then need to make sure the library is suitable for all users.
There is no situation other than the maintainers (me) dropping the ball that lets you have a private driver for a generic component.
Regardless, I will leave this patch out for now, to avoid delaying the release.
/ Leif