| Message ID | 20201119191051.46363-1-cristian.marussi@arm.com |
|---|---|
| Headers | show |
| Series | Add support for SCMIv3.0 Voltage Domain Protocol and SCMI-Regulator | expand |
On Thu, 19 Nov 2020 19:10:46 +0000, Cristian Marussi wrote: > this series introduces the support for the new SCMI Voltage Domain Protocol > defined by the upcoming SCMIv3.0 specification, whose BETA release is > available at [1]. > > Afterwards, a new generic SCMI Regulator driver is developed on top of the > new SCMI VD Protocol. > > [...] Applied to sudeep.holla/linux (for-next/scmi-voltage), thanks! I will soon send pull request to Mark Brown so tha he can pick by the regulator driver patches with these as agreed. [4/5] dt-bindings: arm: Add support for SCMI Regulators https://git.kernel.org/sudeep.holla/c/0f80fcec08 [1/5] firmware: arm_scmi: Add voltage domain management protocol support https://git.kernel.org/sudeep.holla/c/2add5cacff [2/5] firmware: arm_scmi: Add support to enumerated SCMI voltage domain device https://git.kernel.org/sudeep.holla/c/ec88381936 -- Regards, Sudeep
On Thu, Nov 19, 2020 at 07:10:51PM +0000, Cristian Marussi wrote: > + ret = handle->voltage_ops->config_get(handle, sreg->id, > + &config); > + if (ret) { > + dev_err(&sreg->sdev->dev, > + "Error %d reading regulator %s status.\n", > + ret, sreg->desc.name); > + return 0; > + } If we failed to read the status we should return an error rather than claim the regulator is off, other functions return errors so I'm not sure why this one would be different. > + vinfo = handle->voltage_ops->info_get(handle, sreg->id); > + if (!vinfo) { > + dev_warn(dev, "Skipping invalid voltage domain %d\n", > + sreg->id); > + return -ENODEV; I'm not sure that this error message is the most informative - the issue is that we failed to read information, we don't know if that information would have been valid or not. Same for some of the other enumeration, it's a failure to read not a lack of validity isn't it? > + /* Allocate pointers' array for all possible domains */ No ' > + rinfo->num_doms = num_doms; > + /* Several places like this with missing blank lines.
Hi Mark On Mon, Nov 23, 2020 at 05:49:41PM +0000, Mark Brown wrote: > On Thu, Nov 19, 2020 at 07:10:51PM +0000, Cristian Marussi wrote: > > > + ret = handle->voltage_ops->config_get(handle, sreg->id, > > + &config); > > + if (ret) { > > + dev_err(&sreg->sdev->dev, > > + "Error %d reading regulator %s status.\n", > > + ret, sreg->desc.name); > > + return 0; > > + } > > If we failed to read the status we should return an error rather than > claim the regulator is off, other functions return errors so I'm not > sure why this one would be different. > Yes this seems a bug, I'll fix. > > + vinfo = handle->voltage_ops->info_get(handle, sreg->id); > > + if (!vinfo) { > > + dev_warn(dev, "Skipping invalid voltage domain %d\n", > > + sreg->id); > > + return -ENODEV; > > I'm not sure that this error message is the most informative - the issue > is that we failed to read information, we don't know if that information > would have been valid or not. Same for some of the other enumeration, > it's a failure to read not a lack of validity isn't it? > In fact not reading information here could be due to a failure to communicate with fw or to the fact that the underlying Voltage Domain protocol during its initialization failed to validate the domain for some reason (like getting garbage reads of implauusible out of spec levels from the FW) and so VD decided not to expose the domain entry identified by id. I'll report a generic "Failure to get voltage domain" here at this point if it's fine. > > + /* Allocate pointers' array for all possible domains */ > > No ' > Ok > > + rinfo->num_doms = num_doms; > > + /* > > Several places like this with missing blank lines. What do you mean ? a blank before the comment ? Sorry but checkpatch --strict does not complain, I was not aware of this styling. I'll do (if you confirm that's what you want) How do you prefer these changes (and the DT one) ? All as followup patches in a V7 series on top of sudeep/for-next/scmi-voltage ? Thanks Cristian
On Mon, Nov 23, 2020 at 06:49:56PM +0000, Cristian Marussi wrote: > On Mon, Nov 23, 2020 at 05:49:41PM +0000, Mark Brown wrote: > > On Thu, Nov 19, 2020 at 07:10:51PM +0000, Cristian Marussi wrote: > > > + rinfo->num_doms = num_doms; > > > + /* > > Several places like this with missing blank lines. > What do you mean ? a blank before the comment ? > Sorry but checkpatch --strict does not complain, I was not aware of > this styling. I'll do (if you confirm that's what you want) Yes, a blank line between separate semantic blocks. This is normal coding style for the kernel, while checkpatch being a very simple script can't detect it it's surprising that this seems surprising to you. > How do you prefer these changes (and the DT one) ? > All as followup patches in a V7 series on top of > sudeep/for-next/scmi-voltage ? Incremental patches on top of what's applied.
On Thu, 19 Nov 2020 19:10:46 +0000, Cristian Marussi wrote: > this series introduces the support for the new SCMI Voltage Domain Protocol > defined by the upcoming SCMIv3.0 specification, whose BETA release is > available at [1]. > > Afterwards, a new generic SCMI Regulator driver is developed on top of the > new SCMI VD Protocol. > > [...] Applied to https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git for-next Thanks! [1/1] regulator: core: add of_match_full_name boolean flag commit: e7095c35abfc5a5d566941a87434c0fd5ffb570f 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
Hi, this series introduces the support for the new SCMI Voltage Domain Protocol defined by the upcoming SCMIv3.0 specification, whose BETA release is available at [1]. Afterwards, a new generic SCMI Regulator driver is developed on top of the new SCMI VD Protocol. In V4 Patch 3/5 introduced a needed fix in Regulator framework to cope with generic named nodes. The series is currently based on for-next/scmi [2] on top of: commit b141fca08207 ("firmware: arm_scmi: Fix missing destroy_workqueue()") Any feedback welcome, Thanks, Cristian --- v5 --> v6 - reordered dt bindings patch - removed single field struct - reviewed args to scmi_init_voltage_levels() - allocating scmi_voltage_info_array contiguously v4 --> v5 - rebased - VD Protocol - removed inline - moved segmented intervals defines - fixed some macros complaints by checkpatch v3 --> v4 - DT bindings - using generic node names - listing explicitly subset of supported regulators bindings - SCMI Regulator - using of_match_full_name core regulator flag - avoid coccinelle false flag complaints - VD Protocol - avoid coccinelle false flag complaints - avoiding fixed size typing v2 --> v3 - DT bindings - avoid awkard examples based on _cpu/_gpu regulators - SCMI Regulator - remove multiple linear mappings support - removed duplicated voltage name printout - added a few comments - simplified return path in scmi_reg_set_voltage_sel() - VD Protocol - restrict segmented voltage domain descriptors to one triplet - removed unneeded inline - free allocated resources for invalid voltage domain - added __must_check to info_get voltage operations - added a few comments - removed fixed size typing from struct voltage_info v1 --> v2 - rebased on for-next/scmi v5.10 - DT bindings - removed any reference to negative voltages - SCMI Regulator - removed duplicate regulator naming - removed redundant .get/set_voltage ops: only _sel variants implemented - removed condexpr on fail path to increase readability - VD Protocol - fix voltage levels query loop to reload full cmd description between iterations as reported by Etienne Carriere - ensure transport rx buffer is properly sized calli scmi_reset_rx_to_maxsz between transfers [1]:https://developer.arm.com/documentation/den0056/c/ [2]:https://git.kernel.org/pub/scm/linux/kernel/git/sudeep.holla/linux.git/log/?h=for-next/scmi Cristian Marussi (5): firmware: arm_scmi: Add Voltage Domain Support firmware: arm_scmi: add SCMI Voltage Domain devname regulator: core: add of_match_full_name boolean flag dt-bindings: arm: add support for SCMI Regulators regulator: add SCMI driver .../devicetree/bindings/arm/arm,scmi.txt | 43 ++ drivers/firmware/arm_scmi/Makefile | 2 +- drivers/firmware/arm_scmi/common.h | 1 + drivers/firmware/arm_scmi/driver.c | 3 + drivers/firmware/arm_scmi/voltage.c | 380 ++++++++++++++++ drivers/regulator/Kconfig | 9 + drivers/regulator/Makefile | 1 + drivers/regulator/of_regulator.c | 8 +- drivers/regulator/scmi-regulator.c | 409 ++++++++++++++++++ include/linux/regulator/driver.h | 3 + include/linux/scmi_protocol.h | 64 +++ 11 files changed, 920 insertions(+), 3 deletions(-) create mode 100644 drivers/firmware/arm_scmi/voltage.c create mode 100644 drivers/regulator/scmi-regulator.c