Messages in this thread | | | Date | Tue, 16 May 2023 13:03:01 +0200 | From | "Arnd Bergmann" <> | Subject | Re: [PATCH v14 8/8] soc: amd: Add support for AMD Pensando SoC Controller |
| |
On Mon, May 15, 2023, at 20:16, Brad Larson wrote: > The Pensando SoC controller is a SPI connected companion device > that is present in all Pensando SoC board designs. The essential > board management registers are accessed on chip select 0 with > board mgmt IO support accessed using additional chip selects. > > Signed-off-by: Brad Larson <blarson@amd.com>
Hi Brad,
I'm sorry I wasn't paying enough attention to this driver as the past 13 revisions went by.
> v10 changes: > - Different driver implementation specific to this Pensando controller device. > - Moved to soc/amd directory under new name based on guidance. This driver is > of no use to any design other than all Pensando SoC based cards. > - Removed use of builtin_driver, can be built as a module.
it looks like this was a fundamental change that I failed to see.
> +# SPDX-License-Identifier: GPL-2.0-only > +menu "AMD Pensando SoC drivers" > + > +config AMD_PENSANDO_CTRL > + tristate "AMD Pensando SoC Controller" > + depends on SPI_MASTER=y > + depends on (ARCH_PENSANDO && OF) || COMPILE_TEST > + default ARCH_PENSANDO > + select REGMAP_SPI > + select MFD_SYSCON > + help > + Enables AMD Pensando SoC controller device support. This is a SPI > + attached companion device in all Pensando SoC board designs which > + provides essential board control/status registers and management IO > + support.
So generally speaking, I don't want custom user interfaces in drivers/soc. It looks like this one has internal interfaces for a reset controller and the regmap, so those parts are fine, but I'm confused about the purpose of the ioctl interface:
> +static long > +penctrl_ioctl(struct file *filp, unsigned int cmd, unsigned long arg) > +{
> + if (num_msgs > 1) { > + msg++; > + if (msg->len > PENCTRL_MAX_MSG_LEN) { > + ret = -EINVAL; > + goto out_unlock; > + } > + t[1].rx_buf = rx_buf; > + t[1].len = msg->len; > + } > + spi_message_init_with_transfers(&m, t, num_msgs);
This seems to be just a passthrough of user space messages, which is what the spidev driver already provides, but using a different ioctl interface. I don't really want any user-level interfaces in drivers/soc as a rule, but having one that duplicates existing functionality seems particularly wrong.
Can you explain what the purpose is? Is this about serializing access between the in-kernel reset control and the user-side access?
Also, can you explain why this needs a low-lever user interface in the first place, rather than something that can be expressed using high-level abstractions as you already do with the reset control?
All of the above should be part of the changelog text to get a driver like this merged. I don't think we can get a quick solution here though, so maybe you can start by removing the ioctl side and having the rest of the driver in drivers/reset?
Arnd
| |