| Message ID | cover.1597247164.git.mchehab+huawei@kernel.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | SPMI patches needed by Hikey 970 | expand |
Perhaps these trivial bits on top:
---
drivers/staging/hikey9xx/hi6421-spmi-pmic.c | 5 +++--
drivers/staging/hikey9xx/hi6421v600-regulator.c | 6 +++---
drivers/staging/hikey9xx/hisi-spmi-controller.c | 21 +++++++++++++--------
3 files changed, 19 insertions(+), 13 deletions(-)
diff --git a/drivers/staging/hikey9xx/hi6421-spmi-pmic.c b/drivers/staging/hikey9xx/hi6421-spmi-pmic.c
index 76766e7b8bf9..9d73458ca65a 100644
--- a/drivers/staging/hikey9xx/hi6421-spmi-pmic.c
+++ b/drivers/staging/hikey9xx/hi6421-spmi-pmic.c
@@ -99,7 +99,7 @@ int hi6421_spmi_pmic_write(struct hi6421_spmi_pmic *pmic, int reg, u32 val)
EXPORT_SYMBOL(hi6421_spmi_pmic_write);
int hi6421_spmi_pmic_rmw(struct hi6421_spmi_pmic *pmic, int reg,
- u32 mask, u32 bits)
+ u32 mask, u32 bits)
{
unsigned long flags;
u32 data;
@@ -130,7 +130,8 @@ static irqreturn_t hi6421_spmi_irq_handler(int irq, void *data)
hi6421_spmi_pmic_write(pmic, (i + pmic->irq_addr), pending);
/* solve powerkey order */
- if ((i == HISI_IRQ_KEY_NUM) && ((pending & HISI_IRQ_KEY_VALUE) == HISI_IRQ_KEY_VALUE)) {
+ if ((i == HISI_IRQ_KEY_NUM) &&
+ ((pending & HISI_IRQ_KEY_VALUE) == HISI_IRQ_KEY_VALUE)) {
generic_handle_irq(pmic->irqs[HISI_IRQ_KEY_DOWN]);
generic_handle_irq(pmic->irqs[HISI_IRQ_KEY_UP]);
pending &= (~HISI_IRQ_KEY_VALUE);
diff --git a/drivers/staging/hikey9xx/hi6421v600-regulator.c b/drivers/staging/hikey9xx/hi6421v600-regulator.c
index 29ef6bcadd84..82635ff54a74 100644
--- a/drivers/staging/hikey9xx/hi6421v600-regulator.c
+++ b/drivers/staging/hikey9xx/hi6421v600-regulator.c
@@ -227,7 +227,7 @@ static int hi6421_spmi_dt_parse(struct platform_device *pdev,
ret = of_property_read_u32(np, "reg", &rdesc->enable_reg);
if (ret) {
- dev_err(dev, "missing reg property\nn");
+ dev_err(dev, "missing reg property\n");
return ret;
}
@@ -303,13 +303,13 @@ static int hi6421_spmi_dt_parse(struct platform_device *pdev,
*/
rdesc->vsel_mask = (1 << (fls(rdesc->n_voltages) - 1)) - 1;
- dev_dbg(dev, "voltage selector settings: reg: 0x%x, mask: 0x%x",
+ dev_dbg(dev, "voltage selector settings: reg: 0x%x, mask: 0x%x\n",
rdesc->vsel_reg, rdesc->vsel_mask);
return 0;
}
-static struct regulator_ops hi6421_spmi_ldo_rops = {
+static const struct regulator_ops hi6421_spmi_ldo_rops = {
.is_enabled = hi6421_spmi_regulator_is_enabled,
.enable = hi6421_spmi_regulator_enable,
.disable = hi6421_spmi_regulator_disable,
diff --git a/drivers/staging/hikey9xx/hisi-spmi-controller.c b/drivers/staging/hikey9xx/hisi-spmi-controller.c
index 583df10cbf1a..513d962b8bce 100644
--- a/drivers/staging/hikey9xx/hisi-spmi-controller.c
+++ b/drivers/staging/hikey9xx/hisi-spmi-controller.c
@@ -102,7 +102,7 @@ static int spmi_controller_wait_for_done(struct device *dev,
return 0;
}
udelay(1);
- } while(timeout--);
+ } while (timeout--);
dev_err(dev, "%s: timeout, status 0x%x\n", __func__, status);
return -ETIMEDOUT;
@@ -121,7 +121,7 @@ static int spmi_read_cmd(struct spmi_controller *ctrl,
if (bc > SPMI_CONTROLLER_MAX_TRANS_BYTES) {
dev_err(&ctrl->dev,
- "spmi_controller supports 1..%d bytes per trans, but:%ld requested",
+ "spmi_controller supports 1..%d bytes per trans, but:%ld requested\n",
SPMI_CONTROLLER_MAX_TRANS_BYTES, bc);
return -EINVAL;
}
@@ -137,7 +137,7 @@ static int spmi_read_cmd(struct spmi_controller *ctrl,
op_code = SPMI_CMD_EXT_REG_READ_L;
break;
default:
- dev_err(&ctrl->dev, "invalid read cmd 0x%x", opc);
+ dev_err(&ctrl->dev, "invalid read cmd 0x%x\n", opc);
return -EINVAL;
}
@@ -157,7 +157,10 @@ static int spmi_read_cmd(struct spmi_controller *ctrl,
goto done;
for (i = 0; bc > i * SPMI_PER_DATAREG_BYTE; i++) {
- data = readl(spmi_controller->base + chnl_ofst + SPMI_SLAVE_OFFSET * slave_id + SPMI_APB_SPMI_RDATA0_BASE_ADDR + i * SPMI_PER_DATAREG_BYTE);
+ data = readl(spmi_controller->base + chnl_ofst +
+ SPMI_SLAVE_OFFSET * slave_id +
+ SPMI_APB_SPMI_RDATA0_BASE_ADDR +
+ i * SPMI_PER_DATAREG_BYTE);
data = be32_to_cpu((__be32)data);
if ((bc - i * SPMI_PER_DATAREG_BYTE) >> 2) {
memcpy(buf, &data, sizeof(data));
@@ -194,7 +197,7 @@ static int spmi_write_cmd(struct spmi_controller *ctrl,
if (bc > SPMI_CONTROLLER_MAX_TRANS_BYTES) {
dev_err(&ctrl->dev,
- "spmi_controller supports 1..%d bytes per trans, but:%ld requested",
+ "spmi_controller supports 1..%d bytes per trans, but:%ld requested\n",
SPMI_CONTROLLER_MAX_TRANS_BYTES, bc);
return -EINVAL;
}
@@ -210,7 +213,7 @@ static int spmi_write_cmd(struct spmi_controller *ctrl,
op_code = SPMI_CMD_EXT_REG_WRITE_L;
break;
default:
- dev_err(&ctrl->dev, "invalid write cmd 0x%x", opc);
+ dev_err(&ctrl->dev, "invalid write cmd 0x%x\n", opc);
return -EINVAL;
}
@@ -234,8 +237,10 @@ static int spmi_write_cmd(struct spmi_controller *ctrl,
}
writel((u32)cpu_to_be32(data),
- spmi_controller->base + chnl_ofst + SPMI_APB_SPMI_WDATA0_BASE_ADDR + SPMI_PER_DATAREG_BYTE * i);
- };
+ spmi_controller->base + chnl_ofst +
+ SPMI_APB_SPMI_WDATA0_BASE_ADDR +
+ SPMI_PER_DATAREG_BYTE * i);
+ }
/* Start the transaction */
writel(cmd, spmi_controller->base + chnl_ofst + SPMI_APB_SPMI_CMD_BASE_ADDR);
Em Wed, 12 Aug 2020 10:13:51 -0700 Joe Perches <joe@perches.com> escreveu: > Perhaps these trivial bits on top: Sounds fine for me. Feel free to send it with your SOB, adding my reviewed by: Reviewed-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org> > --- > drivers/staging/hikey9xx/hi6421-spmi-pmic.c | 5 +++-- > drivers/staging/hikey9xx/hi6421v600-regulator.c | 6 +++--- > drivers/staging/hikey9xx/hisi-spmi-controller.c | 21 +++++++++++++-------- > 3 files changed, 19 insertions(+), 13 deletions(-) > > diff --git a/drivers/staging/hikey9xx/hi6421-spmi-pmic.c b/drivers/staging/hikey9xx/hi6421-spmi-pmic.c > index 76766e7b8bf9..9d73458ca65a 100644 > --- a/drivers/staging/hikey9xx/hi6421-spmi-pmic.c > +++ b/drivers/staging/hikey9xx/hi6421-spmi-pmic.c > @@ -99,7 +99,7 @@ int hi6421_spmi_pmic_write(struct hi6421_spmi_pmic *pmic, int reg, u32 val) > EXPORT_SYMBOL(hi6421_spmi_pmic_write); > > int hi6421_spmi_pmic_rmw(struct hi6421_spmi_pmic *pmic, int reg, > - u32 mask, u32 bits) > + u32 mask, u32 bits) > { > unsigned long flags; > u32 data; > @@ -130,7 +130,8 @@ static irqreturn_t hi6421_spmi_irq_handler(int irq, void *data) > hi6421_spmi_pmic_write(pmic, (i + pmic->irq_addr), pending); > > /* solve powerkey order */ > - if ((i == HISI_IRQ_KEY_NUM) && ((pending & HISI_IRQ_KEY_VALUE) == HISI_IRQ_KEY_VALUE)) { > + if ((i == HISI_IRQ_KEY_NUM) && > + ((pending & HISI_IRQ_KEY_VALUE) == HISI_IRQ_KEY_VALUE)) { > generic_handle_irq(pmic->irqs[HISI_IRQ_KEY_DOWN]); > generic_handle_irq(pmic->irqs[HISI_IRQ_KEY_UP]); > pending &= (~HISI_IRQ_KEY_VALUE); > diff --git a/drivers/staging/hikey9xx/hi6421v600-regulator.c b/drivers/staging/hikey9xx/hi6421v600-regulator.c > index 29ef6bcadd84..82635ff54a74 100644 > --- a/drivers/staging/hikey9xx/hi6421v600-regulator.c > +++ b/drivers/staging/hikey9xx/hi6421v600-regulator.c > @@ -227,7 +227,7 @@ static int hi6421_spmi_dt_parse(struct platform_device *pdev, > > ret = of_property_read_u32(np, "reg", &rdesc->enable_reg); > if (ret) { > - dev_err(dev, "missing reg property\nn"); > + dev_err(dev, "missing reg property\n"); > return ret; > } > > @@ -303,13 +303,13 @@ static int hi6421_spmi_dt_parse(struct platform_device *pdev, > */ > rdesc->vsel_mask = (1 << (fls(rdesc->n_voltages) - 1)) - 1; > > - dev_dbg(dev, "voltage selector settings: reg: 0x%x, mask: 0x%x", > + dev_dbg(dev, "voltage selector settings: reg: 0x%x, mask: 0x%x\n", > rdesc->vsel_reg, rdesc->vsel_mask); > > return 0; > } > > -static struct regulator_ops hi6421_spmi_ldo_rops = { > +static const struct regulator_ops hi6421_spmi_ldo_rops = { > .is_enabled = hi6421_spmi_regulator_is_enabled, > .enable = hi6421_spmi_regulator_enable, > .disable = hi6421_spmi_regulator_disable, > diff --git a/drivers/staging/hikey9xx/hisi-spmi-controller.c b/drivers/staging/hikey9xx/hisi-spmi-controller.c > index 583df10cbf1a..513d962b8bce 100644 > --- a/drivers/staging/hikey9xx/hisi-spmi-controller.c > +++ b/drivers/staging/hikey9xx/hisi-spmi-controller.c > @@ -102,7 +102,7 @@ static int spmi_controller_wait_for_done(struct device *dev, > return 0; > } > udelay(1); > - } while(timeout--); > + } while (timeout--); > > dev_err(dev, "%s: timeout, status 0x%x\n", __func__, status); > return -ETIMEDOUT; > @@ -121,7 +121,7 @@ static int spmi_read_cmd(struct spmi_controller *ctrl, > > if (bc > SPMI_CONTROLLER_MAX_TRANS_BYTES) { > dev_err(&ctrl->dev, > - "spmi_controller supports 1..%d bytes per trans, but:%ld requested", > + "spmi_controller supports 1..%d bytes per trans, but:%ld requested\n", > SPMI_CONTROLLER_MAX_TRANS_BYTES, bc); > return -EINVAL; > } > @@ -137,7 +137,7 @@ static int spmi_read_cmd(struct spmi_controller *ctrl, > op_code = SPMI_CMD_EXT_REG_READ_L; > break; > default: > - dev_err(&ctrl->dev, "invalid read cmd 0x%x", opc); > + dev_err(&ctrl->dev, "invalid read cmd 0x%x\n", opc); > return -EINVAL; > } > > @@ -157,7 +157,10 @@ static int spmi_read_cmd(struct spmi_controller *ctrl, > goto done; > > for (i = 0; bc > i * SPMI_PER_DATAREG_BYTE; i++) { > - data = readl(spmi_controller->base + chnl_ofst + SPMI_SLAVE_OFFSET * slave_id + SPMI_APB_SPMI_RDATA0_BASE_ADDR + i * SPMI_PER_DATAREG_BYTE); > + data = readl(spmi_controller->base + chnl_ofst + > + SPMI_SLAVE_OFFSET * slave_id + > + SPMI_APB_SPMI_RDATA0_BASE_ADDR + > + i * SPMI_PER_DATAREG_BYTE); > data = be32_to_cpu((__be32)data); > if ((bc - i * SPMI_PER_DATAREG_BYTE) >> 2) { > memcpy(buf, &data, sizeof(data)); > @@ -194,7 +197,7 @@ static int spmi_write_cmd(struct spmi_controller *ctrl, > > if (bc > SPMI_CONTROLLER_MAX_TRANS_BYTES) { > dev_err(&ctrl->dev, > - "spmi_controller supports 1..%d bytes per trans, but:%ld requested", > + "spmi_controller supports 1..%d bytes per trans, but:%ld requested\n", > SPMI_CONTROLLER_MAX_TRANS_BYTES, bc); > return -EINVAL; > } > @@ -210,7 +213,7 @@ static int spmi_write_cmd(struct spmi_controller *ctrl, > op_code = SPMI_CMD_EXT_REG_WRITE_L; > break; > default: > - dev_err(&ctrl->dev, "invalid write cmd 0x%x", opc); > + dev_err(&ctrl->dev, "invalid write cmd 0x%x\n", opc); > return -EINVAL; > } > > @@ -234,8 +237,10 @@ static int spmi_write_cmd(struct spmi_controller *ctrl, > } > > writel((u32)cpu_to_be32(data), > - spmi_controller->base + chnl_ofst + SPMI_APB_SPMI_WDATA0_BASE_ADDR + SPMI_PER_DATAREG_BYTE * i); > - }; > + spmi_controller->base + chnl_ofst + > + SPMI_APB_SPMI_WDATA0_BASE_ADDR + > + SPMI_PER_DATAREG_BYTE * i); > + } > > /* Start the transaction */ > writel(cmd, spmi_controller->base + chnl_ofst + SPMI_APB_SPMI_CMD_BASE_ADDR); > >
On Wed, 2020-08-12 at 15:47 -0300, Mauro Carvalho Chehab wrote: > Em Wed, 12 Aug 2020 10:13:51 -0700 > Joe Perches <joe@perches.com> escreveu: > > > Perhaps these trivial bits on top: > > Sounds fine for me. Feel free to send it with your SOB, adding my reviewed by: > > Reviewed-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org> I don't know that your original series is going to be applied as-is so I think you should carry it. cheers, Joe
Em Wed, 12 Aug 2020 11:58:55 -0700 Joe Perches <joe@perches.com> escreveu: > On Wed, 2020-08-12 at 15:47 -0300, Mauro Carvalho Chehab wrote: > > Em Wed, 12 Aug 2020 10:13:51 -0700 > > Joe Perches <joe@perches.com> escreveu: > > > > > Perhaps these trivial bits on top: > > > > Sounds fine for me. Feel free to send it with your SOB, adding my reviewed by: > > > > Reviewed-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org> > > I don't know that your original > series is going to be applied as-is > so I think you should carry it. Ok. I'll then add the hunks you wrote to the affected changesets. > > cheers, Joe > >
On Wed, 12 Aug 2020, Mauro Carvalho Chehab wrote: > Hi Greg, > > This patch series is part of a work I'm doing in order to be able to support > a HiKey 970 board that I recently got on my hands. > > I received some freedback from Mark and from Jonathan on a first > attempt I made to upstream this. > > I'm opting to submit it via staging, because I had to start from the > patch that originally added this driver on a 4.9 Kernel tree: > > https://github.com/96boards-hikey/linux/tree/hikey970-v4.9 > > In order to preserve the original SOB from the driver's author. > > The patches following it are on the standard way: one patch per > logical change. > > This is part of a bigger work whose goal is to have upstream support > for USB and DRM/KMS on such boards. > > I suspect that, maybe except for the DT part, those 3 specific drivers > are more or less ready to be moved from staging, but the other > drivers that are also part of this attempt aren't ready. Specially the > DRM driver has some bugs that came from the OOT version. > > So, my current plan is to submit those drivers to staging for 5.9 > and move the ones that are ok out of staging on Kernel 5.10. What a mess. This is no way to upstream a new driver. Firstly, could you please add versioning to your submissions. I know this at least version 2. Were there previous submissions? Is this the latest? Secondly and more importantly, you have submitted what looks like a new driver (bearing in mind that I'm only concerning myself with the MFD related changes), then in the same submission you are adding and removing large chunks. Please just submit the new driver, on its own as a single patch, complete with its associated Makefile and Kconfig changes. What are your reasons for submitting this via Staging? Is it not ready yet? Are the resultant components not at a high enough level of quality or enablement to go straight into the subsystems, which is more typical? From an MFD perspective, I would be reviewing the driver as a whole when (if) it moves from Staging into MFD anyway, so why are you jumping through hoops with this additional, seemingly superfluous step? Finally, the subject of authorship is often a contentious one, but this is a problem you need to work out with the original author, not something that should require special handing by upstream. You have a couple of choices, but bear in mind that upstreaming a non-suitable driver then bringing it up to standard is not one of them. 1. Keep the original author's authorship and SoB, make your changes and get them to review to ensure they are still happy about being associated with the resultant code. Ensure you mention all of the changes you make in the commit message and follow-up by adding your own SoB. 2. This is the contentious bit. If you've made enough changes, there is an argument for you to adopt authorship. You should discuss with the original author whether they are happy for you to retain their SoB. My suggestion is always try to keep the SoB as a bare minimum to preserve patch history and out of pure courtesy. > Mauro Carvalho Chehab (41): > staging: spmi: hisi-spmi-controller: coding style fixup > staging: spmi: hisi-spmi-controller: fix it to probe successfully > staging: spmi: hisi-spmi-controller: fix a typo > staging: spmi: hisi-spmi-controller: adjust whitespaces at defines > staging: spmi: hisi-spmi-controller: use le32 macros where needed > staging: spmi: hisi-spmi-controller: add debug when values are > read/write > staging: spmi: hisi-spmi-controller: fix the dev_foo() logic > staging: spmi: hisi-spmi-controller: add it to the building system > staging: spmi: hisi-spmi-controller: do some code cleanups > staging: mfd: hi6421-spmi-pmic: get rid of unused code > staging: mfd: hi6421-spmi-pmic: deal with non-static functions > staging: mfd: hi6421-spmi-pmic: get rid of the static vars > staging: mfd: hi6421-spmi-pmic: cleanup hi6421-spmi-pmic.h header > staging: mfd: hi6421-spmi-pmic: change the binding logic > staging: mfd: hi6421-spmi-pmic: get rid of unused OF properties > staging: mfd: hi6421-spmi-pmic: cleanup OF properties > staging: mfd: hi6421-spmi-pmic: change namespace on its functions > staging: mfd: hi6421-spmi-pmic: fix some coding style issues > staging: mfd: hi6421-spmi-pmic: add it to the building system > staging: mfd: hi6421-spmi-pmic: cleanup the code > staging: regulator: hi6421v600-regulator: get rid of unused code > staging: regulator: hi6421v600-regulator: port it to upstream > staging: regulator: hi6421v600-regulator: coding style fixups > staging: regulator: hi6421v600-regulator: change the binding logic > staging: regulator: hi6421v600-regulator: cleanup struct > hisi_regulator > staging: regulator: hi6421v600-regulator: cleanup debug messages > staging: regulator: hi6421v600-regulator: use shorter names for OF > properties > staging: regulator: hi6421v600-regulator: better handle modes > staging: regulator: hi6421v600-regulator: change namespace > staging: regulator: hi6421v600-regulator: convert to use get/set > voltage_sel > staging: regulator: hi6421v600-regulator: don't use usleep_range for > off_on_delay > staging: regulator: hi6421v600-regulator: add a driver-specific debug > macro > staging: regulator: hi6421v600-regulator: initialize ramp_delay > staging: regulator: hi6421v600-regulator: cleanup DT settings > staging: regulator: hi6421v600-regulator: fix some coding style issues > staging: regulator: hi6421v600-regulator: add it to the building > system > staging: regulator: hi6421v600-regulator: code cleanup > staging: hikey9xx: add a TODO list > MAINTAINERS: add an entry for HiSilicon 6421v600 drivers > dt: document HiSilicon SPMI controller and mfd/regulator properties > dt: hisilicon: add support for the PMIC found on Hikey 970 > > Mayulong (3): > staging: spmi: add Hikey 970 SPMI controller driver > staging: mfd: add a PMIC driver for HiSilicon 6421 SPMI version > staging: regulator: add a regulator driver for HiSilicon 6421v600 SPMI > PMIC > > .../mfd/hisilicon,hi6421-spmi-pmic.yaml | 182 +++++++ > .../spmi/hisilicon,hisi-spmi-controller.yaml | 54 ++ > MAINTAINERS | 6 + > .../boot/dts/hisilicon/hi3670-hikey970.dts | 16 +- > .../boot/dts/hisilicon/hikey970-pmic.dtsi | 200 ++++++++ > drivers/staging/Kconfig | 2 + > drivers/staging/Makefile | 1 + > drivers/staging/hikey9xx/Kconfig | 35 ++ > drivers/staging/hikey9xx/Makefile | 5 + > drivers/staging/hikey9xx/TODO | 5 + > drivers/staging/hikey9xx/hi6421-spmi-pmic.c | 381 ++++++++++++++ > .../staging/hikey9xx/hi6421v600-regulator.c | 479 ++++++++++++++++++ > .../staging/hikey9xx/hisi-spmi-controller.c | 351 +++++++++++++ > include/linux/mfd/hi6421-spmi-pmic.h | 68 +++ > 14 files changed, 1773 insertions(+), 12 deletions(-) > create mode 100644 Documentation/devicetree/bindings/mfd/hisilicon,hi6421-spmi-pmic.yaml > create mode 100644 Documentation/devicetree/bindings/spmi/hisilicon,hisi-spmi-controller.yaml > create mode 100644 arch/arm64/boot/dts/hisilicon/hikey970-pmic.dtsi > create mode 100644 drivers/staging/hikey9xx/Kconfig > create mode 100644 drivers/staging/hikey9xx/Makefile > create mode 100644 drivers/staging/hikey9xx/TODO > create mode 100644 drivers/staging/hikey9xx/hi6421-spmi-pmic.c > create mode 100644 drivers/staging/hikey9xx/hi6421v600-regulator.c > create mode 100644 drivers/staging/hikey9xx/hisi-spmi-controller.c > create mode 100644 include/linux/mfd/hi6421-spmi-pmic.h >
Hi Lee, Em Thu, 13 Aug 2020 08:58:27 +0100 Lee Jones <lee.jones@linaro.org> escreveu: > On Wed, 12 Aug 2020, Mauro Carvalho Chehab wrote: > > > Hi Greg, > > > > This patch series is part of a work I'm doing in order to be able to support > > a HiKey 970 board that I recently got on my hands. > > > > I received some freedback from Mark and from Jonathan on a first > > attempt I made to upstream this. > > > > I'm opting to submit it via staging, because I had to start from the > > patch that originally added this driver on a 4.9 Kernel tree: > > > > https://github.com/96boards-hikey/linux/tree/hikey970-v4.9 > > > > In order to preserve the original SOB from the driver's author. > > > > The patches following it are on the standard way: one patch per > > logical change. > > > > This is part of a bigger work whose goal is to have upstream support > > for USB and DRM/KMS on such boards. > > > > I suspect that, maybe except for the DT part, those 3 specific drivers > > are more or less ready to be moved from staging, but the other > > drivers that are also part of this attempt aren't ready. Specially the > > DRM driver has some bugs that came from the OOT version. > > > > So, my current plan is to submit those drivers to staging for 5.9 > > and move the ones that are ok out of staging on Kernel 5.10. > > What a mess. This is no way to upstream a new driver. > > Firstly, could you please add versioning to your submissions. I know > this at least version 2. Were there previous submissions? Is this > the latest? Yeah, that's the second attempt. The first one was: https://lore.kernel.org/lkml/176043f329dfa9889f014feec04e7e1553077873.1597160086.git.mchehab+huawei@kernel.org/T/#u I was in doubt about adding a v2 in this specific case or not, since I ended submitting it to the staging tree. > Secondly and more importantly, you have submitted what looks like a > new driver (bearing in mind that I'm only concerning myself with the > MFD related changes), then in the same submission you are adding and > removing large chunks. Please just submit the new driver, on its own > as a single patch, complete with its associated Makefile and Kconfig > changes. I can't do like that because I'm not the author of the original patch that added the driver. The original patch came from the 96board's android-kernel based 4.9 tree: https://github.com/96boards-hikey/linux/tree/hikey970-v4.9 > What are your reasons for submitting this via Staging? The main reason is to preserve both the patch authorship and its history. After the original patch, I wrote several incremental changes cleaning up the original driver and stripping parts of it that aren't needed. By preserving the history, if someone wants to restore some removed functionality, it is just a matter of reverting a patch. For example, the original driver had its own sysfs interface for debugging the regulator driver. This is not needed for it to work. Also, the right interface for such things is via configfs. Yet, someone could think on restoring such feat and start from the existing code, instead of coming with something else from scratch. > Is it not ready yet? From my side, I believe that, after my changes, the code now meets upstream requirements, maybe except for DT (and the parsing code). There are a few things at the DT properties on this driver that could be named on a different (more standard way). Yet, I'm not a regular contributor for mfd/regulator/spmi. So, I may have missed something. > Are the resultant components not at a high enough level of > quality or enablement to go straight into the subsystems, which is > more typical? From an MFD perspective, I would be reviewing the > driver as a whole when (if) it moves from Staging into MFD anyway, so > why are you jumping through hoops with this additional, seemingly > superfluous step? I'm OK if this gets reviewed by MFD people only after moving it out of staging. Assuming that this would be merged for Kernel 5.10, I'll likely send a patch moving it out of staging for 5.11. Then, you can do a comprehensive review. > Finally, the subject of authorship is often a contentious one, but > this is a problem you need to work out with the original author, not > something that should require special handing by upstream. You have a > couple of choices, but bear in mind that upstreaming a non-suitable > driver then bringing it up to standard is not one of them. > > 1. Keep the original author's authorship and SoB, make your changes > and get them to review to ensure they are still happy about being > associated with the resultant code. Ensure you mention all of the > changes you make in the commit message and follow-up by adding your > own SoB. > > 2. This is the contentious bit. If you've made enough changes, there > is an argument for you to adopt authorship. You should discuss > with the original author whether they are happy for you to retain > their SoB. My suggestion is always try to keep the SoB as a bare > minimum to preserve patch history and out of pure courtesy. It is not only the above. Both the original author and anyone touching the code should comply with applicable internal policies. From my experience, dealing with such things takes a lot more of time then coding, as it require talking with legal departments on different continents, and with developers and with their bosses in order to be able to do things like that. This can also be a very frustrating process. During almost 20 years of being the media maintainer, I've seen several cases where trying to enforce a folded initial patch caused devs to receive NACKS, preventing them so submit otherwise good stuff. So, at the media subsystem, I'm perfectly fine if someone starts from the original OOT driver, preserving its original authorships. We're also dealing there with the patches sent to drivers/staging/media. I'm not saying that other subsystem maintainers should do the same. Dealing with staging is time consuming, and I completely understand that most maintainers prefer to stay out of it ;-) - Since when staging tree started, if someone has to start from the original patch, such things can be merged at staging. Then, incremental patches are applied at the top until it reaches what's required to be promoted. That's said, there's no hush to have those drivers out of staging. My end goal is to have DRM/KMS and USB support for Hikey 970. The patchsets I have so far are at: https://github.com/mchehab/linux/commits/hikey970/to_upstream-2.0-v1.1 (this branch has the v1 of my patchset) Porting this driver is part of such effort. While this driver is on a good situation, the other ones may require some time to mature. The DRM/KMS driver for example, is not ready to be merged outside staging, as it carries several bugs that came from the original driver and are present at the official tree at 96boards. For example, there is a a very dirty hack that enforces the HDMI chipset to only work with a limited set of resolutions that are known to work: https://github.com/96boards-hikey/linux/blob/hikey970-v4.9/drivers/gpu/drm/hisilicon/kirin9xx/hdmi/adv7535.c#L869 It also has problems reading the frequencies via EDID interface. Due to that, the driver fakes an EDID table: https://github.com/96boards-hikey/linux/blob/hikey970-v4.9/drivers/gpu/drm/hisilicon/kirin9xx/hdmi/adv7535.c#L463 It sounds to me that some clocks are not properly set for a random resolution, but fixing it is not trivial and requires deep knowledge about how the display registers should be tuned to better support resolutions. The current settings cause underflows with 1080p, which in turn makes the display driver to (silently) stop working. So, in summary, I believe that some drivers from my port will require being at staging for a while. While I was planning to do that on my next patch submission, placing the PM drivers there won't make much difference from my side, as I'll need to be submitting patches anyway moving drivers out of staging as they become ready. Thanks, Mauro
Hi Greg, This patch series is part of a work I'm doing in order to be able to support a HiKey 970 board that I recently got on my hands. I received some freedback from Mark and from Jonathan on a first attempt I made to upstream this. I'm opting to submit it via staging, because I had to start from the patch that originally added this driver on a 4.9 Kernel tree: https://github.com/96boards-hikey/linux/tree/hikey970-v4.9 In order to preserve the original SOB from the driver's author. The patches following it are on the standard way: one patch per logical change. This is part of a bigger work whose goal is to have upstream support for USB and DRM/KMS on such boards. I suspect that, maybe except for the DT part, those 3 specific drivers are more or less ready to be moved from staging, but the other drivers that are also part of this attempt aren't ready. Specially the DRM driver has some bugs that came from the OOT version. So, my current plan is to submit those drivers to staging for 5.9 and move the ones that are ok out of staging on Kernel 5.10. Regards, Mauro. Mauro Carvalho Chehab (41): staging: spmi: hisi-spmi-controller: coding style fixup staging: spmi: hisi-spmi-controller: fix it to probe successfully staging: spmi: hisi-spmi-controller: fix a typo staging: spmi: hisi-spmi-controller: adjust whitespaces at defines staging: spmi: hisi-spmi-controller: use le32 macros where needed staging: spmi: hisi-spmi-controller: add debug when values are read/write staging: spmi: hisi-spmi-controller: fix the dev_foo() logic staging: spmi: hisi-spmi-controller: add it to the building system staging: spmi: hisi-spmi-controller: do some code cleanups staging: mfd: hi6421-spmi-pmic: get rid of unused code staging: mfd: hi6421-spmi-pmic: deal with non-static functions staging: mfd: hi6421-spmi-pmic: get rid of the static vars staging: mfd: hi6421-spmi-pmic: cleanup hi6421-spmi-pmic.h header staging: mfd: hi6421-spmi-pmic: change the binding logic staging: mfd: hi6421-spmi-pmic: get rid of unused OF properties staging: mfd: hi6421-spmi-pmic: cleanup OF properties staging: mfd: hi6421-spmi-pmic: change namespace on its functions staging: mfd: hi6421-spmi-pmic: fix some coding style issues staging: mfd: hi6421-spmi-pmic: add it to the building system staging: mfd: hi6421-spmi-pmic: cleanup the code staging: regulator: hi6421v600-regulator: get rid of unused code staging: regulator: hi6421v600-regulator: port it to upstream staging: regulator: hi6421v600-regulator: coding style fixups staging: regulator: hi6421v600-regulator: change the binding logic staging: regulator: hi6421v600-regulator: cleanup struct hisi_regulator staging: regulator: hi6421v600-regulator: cleanup debug messages staging: regulator: hi6421v600-regulator: use shorter names for OF properties staging: regulator: hi6421v600-regulator: better handle modes staging: regulator: hi6421v600-regulator: change namespace staging: regulator: hi6421v600-regulator: convert to use get/set voltage_sel staging: regulator: hi6421v600-regulator: don't use usleep_range for off_on_delay staging: regulator: hi6421v600-regulator: add a driver-specific debug macro staging: regulator: hi6421v600-regulator: initialize ramp_delay staging: regulator: hi6421v600-regulator: cleanup DT settings staging: regulator: hi6421v600-regulator: fix some coding style issues staging: regulator: hi6421v600-regulator: add it to the building system staging: regulator: hi6421v600-regulator: code cleanup staging: hikey9xx: add a TODO list MAINTAINERS: add an entry for HiSilicon 6421v600 drivers dt: document HiSilicon SPMI controller and mfd/regulator properties dt: hisilicon: add support for the PMIC found on Hikey 970 Mayulong (3): staging: spmi: add Hikey 970 SPMI controller driver staging: mfd: add a PMIC driver for HiSilicon 6421 SPMI version staging: regulator: add a regulator driver for HiSilicon 6421v600 SPMI PMIC .../mfd/hisilicon,hi6421-spmi-pmic.yaml | 182 +++++++ .../spmi/hisilicon,hisi-spmi-controller.yaml | 54 ++ MAINTAINERS | 6 + .../boot/dts/hisilicon/hi3670-hikey970.dts | 16 +- .../boot/dts/hisilicon/hikey970-pmic.dtsi | 200 ++++++++ drivers/staging/Kconfig | 2 + drivers/staging/Makefile | 1 + drivers/staging/hikey9xx/Kconfig | 35 ++ drivers/staging/hikey9xx/Makefile | 5 + drivers/staging/hikey9xx/TODO | 5 + drivers/staging/hikey9xx/hi6421-spmi-pmic.c | 381 ++++++++++++++ .../staging/hikey9xx/hi6421v600-regulator.c | 479 ++++++++++++++++++ .../staging/hikey9xx/hisi-spmi-controller.c | 351 +++++++++++++ include/linux/mfd/hi6421-spmi-pmic.h | 68 +++ 14 files changed, 1773 insertions(+), 12 deletions(-) create mode 100644 Documentation/devicetree/bindings/mfd/hisilicon,hi6421-spmi-pmic.yaml create mode 100644 Documentation/devicetree/bindings/spmi/hisilicon,hisi-spmi-controller.yaml create mode 100644 arch/arm64/boot/dts/hisilicon/hikey970-pmic.dtsi create mode 100644 drivers/staging/hikey9xx/Kconfig create mode 100644 drivers/staging/hikey9xx/Makefile create mode 100644 drivers/staging/hikey9xx/TODO create mode 100644 drivers/staging/hikey9xx/hi6421-spmi-pmic.c create mode 100644 drivers/staging/hikey9xx/hi6421v600-regulator.c create mode 100644 drivers/staging/hikey9xx/hisi-spmi-controller.c create mode 100644 include/linux/mfd/hi6421-spmi-pmic.h