On Mon, Jul 12, 2021 at 06:57:37PM -0700, Brian Norris wrote: > On Mon, Jul 5, 2021 at 1:59 AM Pkshih <pkshih@xxxxxxxxxxx> wrote: > > > -----Original Message----- > > > From: Oleksij Rempel [mailto:o.rempel@xxxxxxxxxxxxxx] > > > > Based on this and other part of this driver I would recommend to use > > > regmap. It will provide to additional interface for the register > > > access. And typically for the network devices we have an ethtool > > > interface for that. > > > > > > > Could I know the 'regmap' you mentioned? > > include/linux/regmap.h > drivers/base/regmap/ > > It's a driver framework and API for abstracting register accesses, > whether they are accessed directly via MMIO, or behind some kind of > indirect bus (I2C, SPI, etc.). It also happens to have its own debugfs > operators for doing various kinds of register get/set/dump. So if you > can successfully teach your driver to use it, then you don't need to > implement your own debugfs files for it. > > I've only ever used regmap with Device Tree systems (which can more > easily specify syscon nodes, etc. -- see > Documentation/devicetree/bindings/regmap/regmap.txt). I'm totally > unfamiliar how to use this with ACPI (which I'm sure you want to > support). I'm sure it's possible somehow. > > FWIW, search engines turn up a few basic articles about it, if you > find its documentation or code examples too sparse: > https://www.collabora.com/news-and-blog/blog/2020/05/27/using-regmaps-to-make-linux-drivers-more-generic/ There are not ACPI specific register accesses in this driver. It is using simple readl/writel for the PCI accesses. This driver would need to use devm_regmap_init() and register own regmap_bus. I motivate to use it not only to cover debugfs use case: 1. Current driver implements only PCI access, but potentially wont to support SDIO and USB buses: drivers/net/wireless/realtek/rtw89/core.h enum rtw89_hci_type { RTW89_HCI_TYPE_PCIE, RTW89_HCI_TYPE_USB, RTW89_HCI_TYPE_SDIO, }; SDIO and USB buses may return error on any access. So, driver should test if return value is error on every access. regmap read/write functions separate error value from actual register read value and motivate to handle errors in the driver. Suddenly not every mainline driver is doing it. 2. Current driver implements patter based error detection for the PCI bus: static u32 rtw89_pci_ops_read32_cmac(struct rtw89_dev *rtwdev, u32 addr) { struct rtw89_pci *rtwpci = (struct rtw89_pci *)rtwdev->priv; u32 val = readl(rtwpci->mmap + addr); int count; for (count = 0; ; count++) { if (val != RTW89_R32_DEAD) return val; if (count >= MAC_REG_POOL_COUNT) { rtw89_warn(rtwdev, "addr %#x = %#x\n", addr, val); return RTW89_R32_DEAD; } rtw89_pci_ops_write32(rtwdev, R_AX_CK_EN, B_AX_CMAC_ALLCKEN); val = readl(rtwpci->mmap + addr); } return val; } and handle this patter only in one place: .... int rtw89_mac_check_mac_en(struct rtw89_dev *rtwdev, u8 mac_idx, enum rtw89_mac_hwmod_sel sel) { .... r_val = rtw89_read32(rtwdev, R_AX_SYS_ISO_CTRL_EXTEND); .... if (r_val == RTW89_R32_EA || r_val == RTW89_R32_DEAD || (val & r_val) != val) return -EFAULT; return 0; } potentially this should be done on every read attempt for the PCI, and on every read/write for SDIO and USB buses. 3. This driver has traces of watchdog support on the firmware side, so potentially, firmware can return error on any access if it was reset by the watchdog. 4. regmap provide a way to define support register ranges and will warn if calculated register offset goes outside of this range. 5. regamp is endianness aware and will help to make this driver work properly on big-endian systems. Regards, Oleksij -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
