| Message ID | cover.1621279162.git.sander@svanheule.net |
|---|---|
| Headers | show |
| Series | RTL8231 GPIO expander support | expand |
On Mon, May 17, 2021 at 10:28 PM Sander Vanheule <sander@svanheule.net> wrote: > > Both single and bi-color scanning modes are supported. The driver will > verify that the addresses are valid for the current mode, before > registering the LEDs. LEDs can be turned on, off, or toggled at one of > six predefined rates from 40ms to 1280ms. > > Implements a platform device for use as child device with RTL8231 MFD, as a child > and uses the parent regmap to access the required registers. ... > + When built as a module, this module will be named rtl8231_leds. Again, what it's written here is not what is in Makefile. > +obj-$(CONFIG_LEDS_RTL8231) += leds-rtl8231.o ... > +/** > + * struct led_toggle_rate - description of an LED blinking mode > + * @interval: LED toggle rate in ms > + * @mode: Register field value used to active this mode activate > + * > + * For LED hardware accelerated blinking, with equal on and off delay. > + * Both delays are given by @interval, so the interval at which the LED blinks > + * (i.e. turn on and off once) is double this value. > + */ ... > +static unsigned int rtl8231_led_current_interval(struct rtl8231_led *pled) > +{ > + unsigned int mode; > + unsigned int i = 0; This... > + if (regmap_field_read(pled->reg_field, &mode)) > + return 0; > + > + while (i < pled->modes->num_toggle_rates && mode != pled->modes->toggle_rates[i].mode) > + i++; ...and this will be better as a for-loop. > + if (i < pled->modes->num_toggle_rates) > + return pled->modes->toggle_rates[i].interval; > + else Redundant. > + return 0; > +} ... > + unsigned int i = 0; As per above. ... > + interval = 500; interval_ms > + /* > + * If the current mode is blinking, choose the delay that (likely) changed. > + * Otherwise, choose the interval that would have the same total delay. > + */ > + interval = rtl8231_led_current_interval(pled); > + Redundant blank line. > + if (interval > 0 && interval == *delay_off) > + interval = *delay_on; > + else if (interval > 0 && interval == *delay_on) > + interval = *delay_off; > + else > + interval = (*delay_on + *delay_off) / 2; > + } ... > + u32 addr[2]; > + int err; > + > + if (!fwnode_property_count_u32(fwnode, "reg")) err = fwnode_property_count_u32(...); if (err < 0) return err; if (err == 0) return -ENODEV; > + return -ENODEV; > + > + err = fwnode_property_read_u32_array(fwnode, "reg", addr, ARRAY_SIZE(addr)); If count returns 1? What's the point of counting if you always want two? > + if (err) > + return err; > + > + *addr_port = addr[0]; > + *addr_led = addr[1]; > + > + return 0; > +} ... > + pled = devm_kzalloc(dev, sizeof(*pled), GFP_KERNEL); > + if (IS_ERR(pled)) Wrong. > + return PTR_ERR(pled); ... > + err = rtl8231_led_read_address(fwnode, &port_index, &led_index); > + Redundant blank line. > + if (err) { > + dev_err(dev, "LED address invalid\n"); > + return err; > + } else if (led_index >= RTL8231_NUM_LEDS || port_index >= port_counts[led_index]) { Redundant 'else' > + dev_err(dev, "LED address (%d.%d) invalid\n", port_index, led_index); > + return -ENODEV; > + } ... > + map = dev_get_regmap(dev->parent, NULL); > + if (IS_ERR_OR_NULL(map)) { Split it into two conditionals. > + dev_err(dev, "failed to retrieve regmap\n"); > + if (!map) > + return -ENODEV; > + else > + return PTR_ERR(map); > + } ... > + if (!device_property_match_string(dev, "realtek,led-scan-mode", "single-color")) { It seems that device_property_match_string() and accompanying functions have wrong description of returned codes, i.e. it returns the index of the matched string. It's possible that some APIs are broken (but I believe that the former is the case). That said, I think the proper comparison should be >= 0. > + port_counts = rtl8231_led_port_counts_single; > + regmap_update_bits(map, RTL8231_REG_FUNC0, > + RTL8231_FUNC0_SCAN_MODE, RTL8231_FUNC0_SCAN_SINGLE); > + } else if (!device_property_match_string(dev, "realtek,led-scan-mode", "bi-color")) { Ditto. > + port_counts = rtl8231_led_port_counts_bicolor; > + regmap_update_bits(map, RTL8231_REG_FUNC0, > + RTL8231_FUNC0_SCAN_MODE, RTL8231_FUNC0_SCAN_BICOLOR); > + } else { > + dev_err(dev, "scan mode missing or invalid\n"); > + return -EINVAL; > + }
On Mon, 17 May 2021 21:28:02 +0200, Sander Vanheule wrote: > The RTL8231 GPIO and LED expander can be configured for use as an MDIO or SMI > bus device. Currently only the MDIO mode is supported, although SMI mode > support should be fairly straightforward, once an SMI bus driver is available. > > Provided features by the RTL8231: > - Up to 37 GPIOs > - Configurable drive strength: 8mA or 4mA (currently unsupported) > - Input debouncing on high GPIOs (currently unsupported) > - Up to 88 LEDs in multiple scan matrix groups > - On, off, or one of six toggling intervals > - "single-color mode": 2×36 single color LEDs + 8 bi-color LEDs > - "bi-color mode": (12 + 2×6) bi-color LEDs + 24 single color LEDs > - Up to one PWM output (currently unsupported) > - Fixed duty cycle, 8 selectable frequencies (1.2kHz - 4.8kHz) > > [...] Applied to https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regmap.git for-next Thanks! [1/7] regmap: Add MDIO bus support commit: 1f89d2fe16072a74b34bdb895160910091427891 All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted. You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed. If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced. Please add any relevant lists and maintainers to the CCs when replying to this mail. Thanks, Mark
On Mon, May 17, 2021 at 09:28:03PM +0200, Sander Vanheule wrote: > Basic support for MDIO bus access. Support only includes clause-22 > register access, with 5-bit addresses, and 16-bit wide registers. The following changes since commit 6efb943b8616ec53a5e444193dccf1af9ad627b5: Linux 5.13-rc1 (2021-05-09 14:17:44 -0700) are available in the Git repository at: https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regmap.git tags/regmap-mdio for you to fetch changes up to 1f89d2fe16072a74b34bdb895160910091427891: regmap: Add MDIO bus support (2021-05-19 14:19:10 +0100) ---------------------------------------------------------------- regmap: Add MDIO bus support ---------------------------------------------------------------- Sander Vanheule (1): regmap: Add MDIO bus support drivers/base/regmap/Kconfig | 6 ++++- drivers/base/regmap/Makefile | 1 + drivers/base/regmap/regmap-mdio.c | 57 +++++++++++++++++++++++++++++++++++++++ include/linux/regmap.h | 36 +++++++++++++++++++++++++ 4 files changed, 99 insertions(+), 1 deletion(-) create mode 100644 drivers/base/regmap/regmap-mdio.c
Hi Andy, I've implemented the minor remarks (redundant assignments, if/else code structure...). Some extra details below. On Tue, 2021-05-18 at 00:18 +0300, Andy Shevchenko wrote: > On Mon, May 17, 2021 at 10:28 PM Sander Vanheule <sander@svanheule.net> wrote: > > > > The RTL8231 is implemented as an MDIO device, and provides a regmap > > interface for register access by the core and child devices. > > > > The chip can also be a device on an SMI bus, an I2C-like bus by Realtek. > > Since kernel support for SMI is limited, and no real-world SMI > > implementations have been encountered for this device, this is currently > > unimplemented. The use of the regmap interface should make any future > > support relatively straightforward. > > > > After reset, all pins are muxed to GPIO inputs before the pin drivers > > are enabled. This is done to prevent accidental system resets, when a > > pin is connected to the parent SoC's reset line. > > > [missing MDIO_BUS dependency, provided via REGMAP_MDIO] > > Reported-by: kernel test robot <lkp@intel.com> > > What is the culprit? Shouldn't this have a Fixes tag? But it doesn't actually fix an issue created by an existing commit, just something that was wrong in the first version of the patch. This patch is not dedicated to fixing that single issue though, it's just a part of it. Hence the note above the Reported-by tag. > > > > + mdiodev->reset_gpio = gpiod_get_optional(dev, "reset", > > GPIOD_OUT_LOW); > > + device_property_read_u32(dev, "reset-assert-delay", &mdiodev- > > >reset_assert_delay); > > + device_property_read_u32(dev, "reset-deassert-delay", &mdiodev- > > >reset_deassert_delay); > > + > > + err = rtl8231_init(dev, map); > > + if (err) > > Resource leakage. Replaced gpiod_get_optional by devm_gpiod_get_optional. > > > + return err; > > + > > + /* LED_START enables power to output pins, and starts the LED engine > > */ > > + regmap_field_write(led_start, 1); > > > + return devm_mfd_add_devices(dev, PLATFORM_DEVID_AUTO, rtl8231_cells, > > + ARRAY_SIZE(rtl8231_cells), NULL, 0, NULL); > > Ditto. > > > +} > > ... > > > +#ifdef CONFIG_PM > > Replace this with __maybe_unused attribute. Done. I've also used a few extra macros from PM header to clean this part up a bit more. Best, Sander
Hi Andy, Also here I've tried to address your remarks for v3, some extra details below. On Tue, 2021-05-18 at 01:00 +0300, Andy Shevchenko wrote: > On Mon, May 17, 2021 at 10:28 PM Sander Vanheule <sander@svanheule.net> wrote: > > +static unsigned int rtl8231_led_current_interval(struct rtl8231_led *pled) > > +{ > > + unsigned int mode; > > > + unsigned int i = 0; > > This... > > > + if (regmap_field_read(pled->reg_field, &mode)) > > + return 0; > > + > > + while (i < pled->modes->num_toggle_rates && mode != pled->modes- > > >toggle_rates[i].mode) > > + i++; > > ...and this will be better as a for-loop. > > > + if (i < pled->modes->num_toggle_rates) > > + return pled->modes->toggle_rates[i].interval; > > > + else > > Redundant. > > > + return 0; > > +} Shrunk down to "for (...) if (...) return ...;" in v3. > > > + interval = 500; > > interval_ms Good suggestion, thanks. Don't need those comments in the code then. > > > + u32 addr[2]; > > + int err; > > + > > > + if (!fwnode_property_count_u32(fwnode, "reg")) > > err = fwnode_property_count_u32(...); > if (err < 0) > return err; > if (err == 0) > return -ENODEV; > > > + return -ENODEV; > > + > > + err = fwnode_property_read_u32_array(fwnode, "reg", addr, > > ARRAY_SIZE(addr)); > > If count returns 1? What's the point of counting if you always want two? If count returns 1, or more than 2, that's an error. But this check was missing in v2, so I added it in v3. > > > + if (!device_property_match_string(dev, "realtek,led-scan-mode", > > "single-color")) { > > It seems that device_property_match_string() and accompanying > functions have wrong description of returned codes, i.e. it returns > the index of the matched string. It's possible that some APIs are > broken (but I believe that the former is the case). > > That said, I think the proper comparison should be >= 0. Thanks, fixed. Best, Sander
Hi Andy, I forgot to address one of your questions, so here's a short follow up. On Tue, 2021-05-18 at 00:18 +0300, Andy Shevchenko wrote: > On Mon, May 17, 2021 at 10:28 PM Sander Vanheule <sander@svanheule.net> wrote: > > + err = regmap_read(map, RTL8231_REG_FUNC1, &v); > > > + ready_code = FIELD_GET(RTL8231_FUNC1_READY_CODE_MASK, v); > > If we got an error why we need a read_core, what for? The chip has a static 5-bit field in register 0x01, called READY_CODE according to the datasheet. If a device is present, and a read from register 0x01 succeeds, I still check that this field has the correct value. For the RTL8231, it should return 0x37. If this isn't the case, I assume this isn't an RTL8231, so the driver probe stops and returns an error value. Best, Sander
On Mon, May 24, 2021 at 10:50 AM Sander Vanheule <sander@svanheule.net> wrote: > On Tue, 2021-05-18 at 00:18 +0300, Andy Shevchenko wrote: > > On Mon, May 17, 2021 at 10:28 PM Sander Vanheule <sander@svanheule.net> wrote: > > > + err = regmap_read(map, RTL8231_REG_FUNC1, &v); > > > > > + ready_code = FIELD_GET(RTL8231_FUNC1_READY_CODE_MASK, v); > > > > If we got an error why we need a read_core, what for? > > The chip has a static 5-bit field in register 0x01, called READY_CODE according > to the datasheet. If a device is present, and a read from register 0x01 > succeeds, I still check that this field has the correct value. For the RTL8231, > it should return 0x37. If this isn't the case, I assume this isn't an RTL8231, > so the driver probe stops and returns an error value. Right. And why do you get ready_code if you know that there is an error?
On Mon, 2021-05-24 at 10:55 +0300, Andy Shevchenko wrote: > On Mon, May 24, 2021 at 10:50 AM Sander Vanheule <sander@svanheule.net> wrote: > > On Tue, 2021-05-18 at 00:18 +0300, Andy Shevchenko wrote: > > > On Mon, May 17, 2021 at 10:28 PM Sander Vanheule <sander@svanheule.net> > > > wrote: > > > > + err = regmap_read(map, RTL8231_REG_FUNC1, &v); > > > > > > > + ready_code = FIELD_GET(RTL8231_FUNC1_READY_CODE_MASK, v); > > > > > > If we got an error why we need a read_core, what for? > > > > The chip has a static 5-bit field in register 0x01, called READY_CODE > > according > > to the datasheet. If a device is present, and a read from register 0x01 > > succeeds, I still check that this field has the correct value. For the > > RTL8231, > > it should return 0x37. If this isn't the case, I assume this isn't an > > RTL8231, > > so the driver probe stops and returns an error value. > > Right. And why do you get ready_code if you know that there is an error? This has changed in v3. I now check if there was an error reading the register, and return if there was. Only if there wasn't an error, the code continues to extract and verify the READY_CODE value. Best, Sander
The RTL8231 GPIO and LED expander can be configured for use as an MDIO or SMI bus device. Currently only the MDIO mode is supported, although SMI mode support should be fairly straightforward, once an SMI bus driver is available. Provided features by the RTL8231: - Up to 37 GPIOs - Configurable drive strength: 8mA or 4mA (currently unsupported) - Input debouncing on high GPIOs (currently unsupported) - Up to 88 LEDs in multiple scan matrix groups - On, off, or one of six toggling intervals - "single-color mode": 2×36 single color LEDs + 8 bi-color LEDs - "bi-color mode": (12 + 2×6) bi-color LEDs + 24 single color LEDs - Up to one PWM output (currently unsupported) - Fixed duty cycle, 8 selectable frequencies (1.2kHz - 4.8kHz) Register access is provided through a new MDIO regmap provider. The GPIO controller uses gpio-regmap, although a patch is required to support a limitation of the chip. There remain some log warnings when probing the device, possibly due to the way I'm using the MFD subsystem. Would it be possible to avoid these? [ 2.602242] rtl8231-pinctrl: Failed to locate of_node [id: -2] [ 2.609380] rtl8231-pinctrl rtl8231-pinctrl.0.auto: no of_node; not parsing pinctrl DT When no 'leds' sub-node is specified: [ 2.922262] rtl8231-leds: Failed to locate of_node [id: -2] [ 2.967149] rtl8231-leds rtl8231-leds.1.auto: no of_node; not parsing pinctrl DT [ 2.975673] rtl8231-leds rtl8231-leds.1.auto: scan mode missing or invalid [ 2.983531] rtl8231-leds: probe of rtl8231-leds.1.auto failed with error -22 Changes since v1: - Reintroduce MDIO regmap, with fixed Kconfig dependencies - Add configurable dir/value order for gpio-regmap direction_out call - Drop allocations for regmap fields that are used only on init - Move some definitions to MFD header - Add PM ops to replace driver remove for MFD - Change pinctrl driver to (modified) gpio-regmap - Change leds driver to use fwnode Link: https://lore.kernel.org/lkml/cover.1620735871.git.sander@svanheule.net/ Changes since RFC: - Dropped MDIO regmap interface. I was unable to resolve the Kconfig dependency issue, so have reverted to using regmap_config.reg_read/write. - Added pinctrl support - Added LED support - Changed root device to MFD, with pinctrl and leds child devices. Root device is now an mdio_device driver. Link: https://lore.kernel.org/linux-gpio/cover.1617914861.git.sander@svanheule.net/ Sander Vanheule (7): regmap: Add MDIO bus support gpio: regmap: Add configurable dir/value order dt-bindings: leds: Binding for RTL8231 scan matrix dt-bindings: mfd: Binding for RTL8231 mfd: Add RTL8231 core device pinctrl: Add RTL8231 pin control and GPIO support leds: Add support for RTL8231 LED scan matrix .../bindings/leds/realtek,rtl8231-leds.yaml | 159 ++++++++ .../bindings/mfd/realtek,rtl8231.yaml | 202 ++++++++++ drivers/base/regmap/Kconfig | 6 +- drivers/base/regmap/Makefile | 1 + drivers/base/regmap/regmap-mdio.c | 57 +++ drivers/gpio/gpio-regmap.c | 20 +- drivers/leds/Kconfig | 10 + drivers/leds/Makefile | 1 + drivers/leds/leds-rtl8231.c | 293 ++++++++++++++ drivers/mfd/Kconfig | 9 + drivers/mfd/Makefile | 1 + drivers/mfd/rtl8231.c | 153 +++++++ drivers/pinctrl/Kconfig | 11 + drivers/pinctrl/Makefile | 1 + drivers/pinctrl/pinctrl-rtl8231.c | 377 ++++++++++++++++++ include/linux/gpio/regmap.h | 3 + include/linux/mfd/rtl8231.h | 57 +++ include/linux/regmap.h | 36 ++ 18 files changed, 1393 insertions(+), 4 deletions(-) create mode 100644 Documentation/devicetree/bindings/leds/realtek,rtl8231-leds.yaml create mode 100644 Documentation/devicetree/bindings/mfd/realtek,rtl8231.yaml create mode 100644 drivers/base/regmap/regmap-mdio.c create mode 100644 drivers/leds/leds-rtl8231.c create mode 100644 drivers/mfd/rtl8231.c create mode 100644 drivers/pinctrl/pinctrl-rtl8231.c create mode 100644 include/linux/mfd/rtl8231.h