mfd: Add a driver for the RPI RP2040 GPIO bridge#6181
mfd: Add a driver for the RPI RP2040 GPIO bridge#6181roliver-rpi merged 2 commits intoraspberrypi:rpi-6.6.yfrom
Conversation
6by9
left a comment
6by9
left a comment
There was a problem hiding this comment.
Largely coding style niggles.
General minor clean up of whether there is a space before the final return of a function would be nice (it's generally expected).
| controller->min_speed_hz = 35000000; | ||
| controller->max_speed_hz = 35000000; | ||
| controller->max_transfer_size = rp2040_gbdg_max_transfer_size; | ||
| controller->max_message_size = rp2040_gbdg_max_transfer_size; |
There was a problem hiding this comment.
Seeing as rp2040_gbdg_max_transfer_size just returns MAX_TRANSFER_SIZE, do we need a function for it?
drivers/mfd/rpi-rp2040-gpio-bridge.c
Outdated
| } | ||
| priv_data->shash_desc->tfm = priv_data->shash; | ||
|
|
||
| controller->bus_num = np ? -1 : client->adapter->nr; |
There was a problem hiding this comment.
Why is this conditional on np (aka dev->of_node)? It's only accessing client->adapter
drivers/mfd/rpi-rp2040-gpio-bridge.c
Outdated
| of_node_put(of_args[0].np); | ||
| if (of_args[1].np) | ||
| of_node_put(of_args[1].np); | ||
| return 0; |
There was a problem hiding this comment.
So this is a success path? cleanup implied to me at least that it was an error path. node_put or similar would be more obvious.
drivers/mfd/rpi-rp2040-gpio-bridge.c
Outdated
| "Could not allocate shash\n"); | ||
|
|
||
| if (crypto_shash_digestsize(priv_data->shash) != MD5_DIGEST_SIZE) { | ||
| crypto_free_shash(priv_data->shash); |
There was a problem hiding this comment.
Repeated multiple times. It might be nice to make it a common exit path at the end of the function.
|
|
||
| controller = devm_spi_alloc_master(dev, sizeof(struct rp2040_gbdg)); | ||
| if (!controller) | ||
| return dev_err_probe(dev, ENOMEM, |
There was a problem hiding this comment.
This (and subsequent) error path(s) don't clean up pm_runtime.
If devm_pm_runtime_enable does pass enable across to devm, then disabling in _remove feels unbalanced.
drivers/mfd/rpi-rp2040-gpio-bridge.c
Outdated
|
|
||
| u8 local_digest[MD5_DIGEST_SIZE]; | ||
| u8 remote_digest[MD5_DIGEST_SIZE]; | ||
| u8 ascii_digest[MD5_DIGEST_SIZE * 2 + 1] = { 0 }; |
There was a problem hiding this comment.
Variable declarations before all code.
drivers/mfd/rpi-rp2040-gpio-bridge.c
Outdated
| rp2040_gbdg_rp1_calc_offsets(priv_data->fast_xfer_clock_index, | ||
| &clock_bank, &clock_offset); | ||
|
|
||
| void __iomem *data_set = |
There was a problem hiding this comment.
Variable declaration in the middle of a function.
| @@ -0,0 +1,20 @@ | |||
| spi_bridge: spi@40 { | |||
There was a problem hiding this comment.
dtoverlay should be a separate patch to driver or bindings.
| @@ -0,0 +1,77 @@ | |||
| # SPDX-License-Identifier: GPL-2.0-only | |||
There was a problem hiding this comment.
Bindings should be a separate patch to anything else.
Does it validate? make dt_binding_check
drivers/mfd/Kconfig
Outdated
| Additional drivers must be enabled in order to use the functionality | ||
| of the device. | ||
|
|
||
| config MFD_RASPBERRYPI_RP2040_GPIO_BRIDGE |
There was a problem hiding this comment.
Is it an MFD in the kernel sense? Most MFD drivers allow for a parent that can acquire shared resources, and then you have separate drivers off it for the specific functions.
bcm2835-pm parents watchdog and power drivers
POE+ HAT uses simple-mfd-i2c as the core, with a PWM and power supply driver on top.
roliver-rpi
force-pushed
the
rpi-rp2040-gpio-bridge
branch
from
31f47f4 to
5875c3a
Compare
|
Any further comments on this? I believe all code review comments have been addressed. Perhaps the single remaining outstanding question is whether or not we're happy with this in mfd? Thoughts on potential alternate locations @pelwell @6by9 @naushir ? If we're happy with mfd then I believe this is ready to merge. |
What are the alternatives:
|
Add YAML device tree bindings for the Raspberry Pi RP2040 GPIO Bridge. Signed-off-by: Richard Oliver <[email protected]>
The Raspberry Pi RP2040 GPIO bridge is an I2C-attached device exposing both a Tx-only SPI controller, and a GPIO controller. Due to the relative difference in transfer rates between standard-mode I2C and SPI, the GPIO bridge makes use of 12 MiB of non-volatile storage to cache repeated transfers. This cache is arranged in ~8 KiB blocks and is addressed by the MD5 digest of the data contained therein. Optionally, this driver is able to take advantage of Raspberry Pi RP1 GPIOs to achieve faster than I2C data transfer rates. Signed-off-by: Richard Oliver <[email protected]>
roliver-rpi
force-pushed
the
rpi-rp2040-gpio-bridge
branch
from
5875c3a to
8c478ee
Compare
|
Driver has been moved to drivers/spi |
pelwell
left a comment
pelwell
left a comment
There was a problem hiding this comment.
Realistically I'm not going to fully grok this new driver, but apart from the lack of error return from the DT parsing it looks OK to me.
| return 0; | ||
| } | ||
|
|
||
| static void rp2040_gbdg_parse_dt(struct rp2040_gbdg *rp2040_gbdg) |
There was a problem hiding this comment.
Is it reasonable that this has no return value? It has many points where it returns early after an error.
There was a problem hiding this comment.
Only the parsing of the 'bypass-cache' property in rp2040_gbdg_parse_dt() is required for all platforms. This cannot fail due to the API of of_property_read_bool() returning the bool.
Any failure in the rest of rp2040_gbdg_parse_dt() is not an issue as the driver will use I2C to transfer blocks of data as a fallback (rather than fast-xfer using RP1 bit-bashing). In fact, we expect failure on platforms without RP1.
If we were to return a value from rp2040_gbdg_parse_dt, in it's current form, it would only be informational and not to be acted upon (other than perhaps a dev_info).
I'm happy to refactor rp2040_gbdg_parse_dt() if we can think of a cleaner/easier to understand API. Alternatively, I could comment the function as it's perhaps not sufficiently clear in its current form.
There was a problem hiding this comment.
It's OK - if you're happy justifying it being as it is now, I'm happy to merge it
kernel: dmaengine: dw-axi-dmac: Honour snps,block-size See: raspberrypi/linux#6263 kernel: mfd: Add a driver for the RPI RP2040 GPIO bridge See: raspberrypi/linux#6181 kernel: dts: Make camN_reg and camN_reg_gpio overrides generic See: raspberrypi/linux#6254 kernel: Backport mainline pispbe See: raspberrypi/linux#6259
kernel: dmaengine: dw-axi-dmac: Honour snps,block-size See: raspberrypi/linux#6263 kernel: mfd: Add a driver for the RPI RP2040 GPIO bridge See: raspberrypi/linux#6181 kernel: dts: Make camN_reg and camN_reg_gpio overrides generic See: raspberrypi/linux#6254 kernel: Backport mainline pispbe See: raspberrypi/linux#6259
3 participants
The Raspberry Pi RP2040 GPIO bridge is an I2C-attached MFD exposing both a Tx-only SPI controller, and a GPIO controller.
Due to the relative difference in transfer rates between standard-mode I2C and SPI, the GPIO bridge makes use of 12 MiB of non-volatile storage to cache repeated transfers. This cache is arranged in ~8 KiB blocks and is addressed by the MD5 digest of the data contained therein.
Optionally, this driver is able to take advantage of Raspberry Pi RP1 GPIOs to achieve faster than I2C data transfer rates.