-
Notifications
You must be signed in to change notification settings - Fork 5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
mfd: Add a driver for the RPI RP2040 GPIO bridge #6181
mfd: Add a driver for the RPI RP2040 GPIO bridge #6181
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
drivers/mfd/rpi-rp2040-gpio-bridge.c
Outdated
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Repeated multiple times. It might be nice to make it a common exit path at the end of the function.
drivers/mfd/rpi-rp2040-gpio-bridge.c
Outdated
|
||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Variable declaration in the middle of a function.
@@ -0,0 +1,20 @@ | |||
spi_bridge: spi@40 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bindings should be a separate patch to anything else.
Does it validate? make dt_binding_check
drivers/mfd/Kconfig
Outdated
@@ -2362,5 +2362,14 @@ config MFD_RSMU_SPI | |||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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 <richard.oliver@raspberrypi.com>
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 <richard.oliver@raspberrypi.com>
5875c3a
to
8c478ee
Compare
Driver has been moved to drivers/spi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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.