| Message ID | cover.1624525118.git.mchehab+huawei@kernel.org |
|---|---|
| Headers | show |
| Series | Move Hisilicon 6421v600 SPMI and USB drivers out of staging | expand |
Em Thu, 24 Jun 2021 15:08:00 +0100 Lee Jones <lee.jones@linaro.org> escreveu: > > > > +/* > > > > + * The IRQs are mapped as: > > > > + * > > > > + * ====================== ============= ============ ===== > > > > + * IRQ MASK REGISTER IRQ REGISTER BIT > > > > + * ====================== ============= ============ ===== > > > > + * OTMP 0x0202 0x212 bit 0 > > > > + * VBUS_CONNECT 0x0202 0x212 bit 1 > > > > + * VBUS_DISCONNECT 0x0202 0x212 bit 2 > > > > + * ALARMON_R 0x0202 0x212 bit 3 > > > > + * HOLD_6S 0x0202 0x212 bit 4 > > > > + * HOLD_1S 0x0202 0x212 bit 5 > > > > + * POWERKEY_UP 0x0202 0x212 bit 6 > > > > + * POWERKEY_DOWN 0x0202 0x212 bit 7 > > > > + * > > > > + * OCP_SCP_R 0x0203 0x213 bit 0 > > > > + * COUL_R 0x0203 0x213 bit 1 > > > > + * SIM0_HPD_R 0x0203 0x213 bit 2 > > > > + * SIM0_HPD_F 0x0203 0x213 bit 3 > > > > + * SIM1_HPD_R 0x0203 0x213 bit 4 > > > > + * SIM1_HPD_F 0x0203 0x213 bit 5 > > > > + * ====================== ============= ============ ===== > > > > + * > > > > + * Each mask register contains 8 bits. The ancillary macros below > > > > + * convert a number from 0 to 14 into a register address and a bit mask > > > > + */ > > > > +#define HISI_IRQ_MASK_REG(irq_data) (SOC_PMIC_IRQ_MASK_0_ADDR + \ > > > > + (irqd_to_hwirq(irq_data) / BITS_PER_BYTE)) > > > > +#define HISI_IRQ_MASK_BIT(irq_data) BIT(irqd_to_hwirq(irq_data) & (BITS_PER_BYTE - 1)) > > > > +#define HISI_8BITS_MASK GENMASK(BITS_PER_BYTE - 1, 0) > > > > > > Are these lines up in real code? Looks like they're not in the diff. > > > > Weird. The changes to use those are at patch 3/8. All the above > > macros are used at the patch. > > Sorry, that made no sense - it's been a long few days! > > I meant to say "do these (the tabs) line up?" Yes, they line up (and aligned with the parenthesis, in the case of HISI_IRQ_MASK_REG). > > > > +static const struct mfd_cell hi6421v600_devs[] = { > > > > + { .name = "hi6421v600-regulator", }, > > > > +}; > > > > > > Where are the other devices? > > > > While this is a MFD device, as it has regulators, ADC and other > > stuff, right now, only the regulator and the IRQs are implemented. > > > > The IRQs are at the core of this driver, while the regulator > > is at the separate regulator driver. > > The rule usually goes: > > Drivers don't qualify as MFDs until you register >1 device. Do you mean that, in order for this to be accepted, should I move the irq code to a separate driver? > > > > + for (i = 0; i < PMIC_IRQ_LIST_MAX; i++) { > > > > + virq = irq_create_mapping(ddata->domain, i); > > > > + if (!virq) { > > > > + dev_err(dev, "Failed to map H/W IRQ\n"); > > > > + return -ENOSPC; > > > > > > -ENOSPC doesn't seem right here. > > > > > > Can't find any other uses of it for irq_create_mapping() either. > > > > There are two drivers returning -ENOSPC: > > > > arch/powerpc/platforms/pseries/msi.c > > arch/powerpc/sysdev/mpic_u3msi.c > > I only looked in drivers/ > > > But others return -EIO, -EINVAL, -ENOMEM, -ENODEV, -ENXIO. > > > > I think that -ENODEV would fit better here. > > I think -ENXIO is the most common, followed by -EINVAL. > > This doesn't have anything to do with devices per say. Ok. I'll change it to -ENXIO. > > > > +static void hi6421_spmi_pmic_remove(struct spmi_device *pdev) > > > > +{ > > > > + struct hi6421_spmi_pmic *ddata = dev_get_drvdata(&pdev->dev); > > > > + > > > > + free_irq(ddata->irq, ddata); > > > > > > No devm_* version? > > > > Are there a devm_* variant for gpio_to_irq()? > > Please refer to Dan's response. Ok. Thanks, Mauro
Em Thu, 24 Jun 2021 16:26:00 +0200 Johan Hovold <johan@kernel.org> escreveu: > On Thu, Jun 24, 2021 at 03:08:00PM +0100, Lee Jones wrote: > > On Thu, 24 Jun 2021, Mauro Carvalho Chehab wrote: > > > > > Em Thu, 24 Jun 2021 12:33:28 +0100 > > > Lee Jones <lee.jones@linaro.org> escreveu: > > > > > > --- /dev/null > > > > > +++ b/drivers/mfd/hi6421-spmi-pmic.c > > > > > @@ -0,0 +1,316 @@ > > > > > +// SPDX-License-Identifier: GPL-2.0 > > > > > +/* > > > > > + * Device driver for regulators in HISI PMIC IC > > > > > + * > > > > > + * Copyright (c) 2013 Linaro Ltd. > > > > > + * Copyright (c) 2011 Hisilicon. > > > > > + * Copyright (c) 2020-2021 Huawei Technologies Co., Ltd > > > > > > > > Can this be updated? > > > > > > Do you mean updating the copyrights to cover this year? E.g. > > > something like this: > > > > > > * Copyright (c) 2013-2021 Linaro Ltd. > > > * Copyright (c) 2011-2021 Hisilicon. > > > * Copyright (c) 2020-2021 Huawei Technologies Co., Ltd > > > > > > Right? Or are you meaning something else? > > > > Yes, that's it. I know this is just a move, but to MFD, it's new. > > That's not how copyright works. Unless Linaro and Hisilicon made > nontrivial changes every year from 2011/2013 to 2021 you should not > change those lines like this. Only Linaro can answer what happened up to 2018, as this driver originally came from a Linaro tree, which has exactly one commit for this driver: https://github.com/96boards-hikey/linux/commit/08464419fba2417aa849fce976fac9c5f51b3ada#diff-604ef8563dcd9ace6e3e58aac38337c72924b0889f6972d7ee9e15e2335ba964 After merged upstream at staging, all changes are covered by the Huawei's copyright (2020-2021). So, I'll just drop this patch. If the information is not accurate, someone from the original copyright holders can send followup patches. Thanks, Mauro
On Fri, 25 Jun 2021, Mauro Carvalho Chehab wrote: > Em Thu, 24 Jun 2021 16:26:00 +0200 > Johan Hovold <johan@kernel.org> escreveu: > > > On Thu, Jun 24, 2021 at 03:08:00PM +0100, Lee Jones wrote: > > > On Thu, 24 Jun 2021, Mauro Carvalho Chehab wrote: > > > > > > > Em Thu, 24 Jun 2021 12:33:28 +0100 > > > > Lee Jones <lee.jones@linaro.org> escreveu: > > > > > > > > --- /dev/null > > > > > > +++ b/drivers/mfd/hi6421-spmi-pmic.c > > > > > > @@ -0,0 +1,316 @@ > > > > > > +// SPDX-License-Identifier: GPL-2.0 > > > > > > +/* > > > > > > + * Device driver for regulators in HISI PMIC IC > > > > > > + * > > > > > > + * Copyright (c) 2013 Linaro Ltd. > > > > > > + * Copyright (c) 2011 Hisilicon. > > > > > > + * Copyright (c) 2020-2021 Huawei Technologies Co., Ltd > > > > > > > > > > Can this be updated? > > > > > > > > Do you mean updating the copyrights to cover this year? E.g. > > > > something like this: > > > > > > > > * Copyright (c) 2013-2021 Linaro Ltd. > > > > * Copyright (c) 2011-2021 Hisilicon. > > > > * Copyright (c) 2020-2021 Huawei Technologies Co., Ltd > > > > > > > > Right? Or are you meaning something else? > > > > > > Yes, that's it. I know this is just a move, but to MFD, it's new. > > > > That's not how copyright works. Unless Linaro and Hisilicon made > > nontrivial changes every year from 2011/2013 to 2021 you should not > > change those lines like this. > > Only Linaro can answer what happened up to 2018, as this driver > originally came from a Linaro tree, which has exactly one commit > for this driver: > > https://github.com/96boards-hikey/linux/commit/08464419fba2417aa849fce976fac9c5f51b3ada#diff-604ef8563dcd9ace6e3e58aac38337c72924b0889f6972d7ee9e15e2335ba964 > > After merged upstream at staging, all changes are covered by the > Huawei's copyright (2020-2021). > > So, I'll just drop this patch. If the information is not accurate, > someone from the original copyright holders can send followup > patches. After taking the time to read up on Copyright rules and expectations, I think we can pretty much safely say that all of the Copyright entries above are incorrect. Copyright dates should only be listed if significant works were undertaken in each of the years listed. So unless large adaptions happened every year since 2011, which I doubt very much, they're wrong. -- Lee Jones [李琼斯] Senior Technical Lead - Developer Services Linaro.org │ Open source software for Arm SoCs Follow Linaro: Facebook | Twitter | Blog
Notes: 1. Patch resent with --no-renames, in order to show the full code when moving the code out of staging (on this RESEND); 2. the previous series is at: https://lore.kernel.org/lkml/cover.1616695231.git.mchehab+huawei@kernel.org/ Hi Greg, Those are the remaining patches that are needed for the USB to work with Hikey970. This series address the comments made on v5. Sorry for taking so long to return back on this. Got sidetracked by other unrelated stuff. Changes from v5: - Some changes at DT to comply with Rob Herring's feedback; - a couple of cleanups at the phy-hi3670-usb3; - Vinod's ack added to patch 4; - Several cleanups at hi6421-spmi-pmic.c, in order to address Lee Jones feedbacks. On this series, I opted to keep using "gpios" for the DT IRQ gpios needed by the PMIC driver, as this is the string expected by of_get_gpio(), and it is the most common pattern for IRQ gpios. If required, I'll send a followup patch changing it to use, instead, the of_get_named_gpio_flags() variant. Mauro Carvalho Chehab (8): staging: phy-hi3670-usb3: do a some minor cleanups staging: hisi-spmi-controller: rename spmi-channel property staging: phy-hi3670-usb3: do some additional cleanups phy: phy-hi3670-usb3: move driver from staging into phy spmi: hisi-spmi-controller: move driver from staging mfd: hi6421-spmi-pmic: move driver from staging dts: hisilicon: add support for the PMIC found on Hikey 970 dts: hisilicon: add support for USB3 on Hikey 970 .../mfd/hisilicon,hi6421-spmi-pmic.yaml | 134 ++++ .../bindings/phy/hisilicon,hi3670-usb3.yaml | 73 ++ .../spmi/hisilicon,hisi-spmi-controller.yaml | 73 ++ MAINTAINERS | 23 +- .../boot/dts/hisilicon/hi3670-hikey970.dts | 129 +++- arch/arm64/boot/dts/hisilicon/hi3670.dtsi | 56 ++ .../boot/dts/hisilicon/hikey970-pmic.dtsi | 87 +++ drivers/mfd/Kconfig | 16 + drivers/mfd/Makefile | 1 + drivers/mfd/hi6421-spmi-pmic.c | 316 +++++++++ drivers/phy/hisilicon/Kconfig | 10 + drivers/phy/hisilicon/Makefile | 1 + drivers/phy/hisilicon/phy-hi3670-usb3.c | 661 +++++++++++++++++ drivers/spmi/Kconfig | 9 + drivers/spmi/Makefile | 1 + drivers/spmi/hisi-spmi-controller.c | 367 ++++++++++ drivers/staging/Kconfig | 2 - drivers/staging/Makefile | 1 - drivers/staging/hikey9xx/Kconfig | 41 -- drivers/staging/hikey9xx/Makefile | 6 - drivers/staging/hikey9xx/TODO | 5 - drivers/staging/hikey9xx/hi6421-spmi-pmic.c | 297 -------- .../staging/hikey9xx/hisi-spmi-controller.c | 367 ---------- .../hikey9xx/hisilicon,hi6421-spmi-pmic.yaml | 135 ---- .../hisilicon,hisi-spmi-controller.yaml | 71 -- drivers/staging/hikey9xx/phy-hi3670-usb3.c | 668 ------------------ drivers/staging/hikey9xx/phy-hi3670-usb3.yaml | 73 -- 27 files changed, 1937 insertions(+), 1686 deletions(-) create mode 100644 Documentation/devicetree/bindings/mfd/hisilicon,hi6421-spmi-pmic.yaml create mode 100644 Documentation/devicetree/bindings/phy/hisilicon,hi3670-usb3.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/mfd/hi6421-spmi-pmic.c create mode 100644 drivers/phy/hisilicon/phy-hi3670-usb3.c create mode 100644 drivers/spmi/hisi-spmi-controller.c delete mode 100644 drivers/staging/hikey9xx/Kconfig delete mode 100644 drivers/staging/hikey9xx/Makefile delete mode 100644 drivers/staging/hikey9xx/TODO delete mode 100644 drivers/staging/hikey9xx/hi6421-spmi-pmic.c delete mode 100644 drivers/staging/hikey9xx/hisi-spmi-controller.c delete mode 100644 drivers/staging/hikey9xx/hisilicon,hi6421-spmi-pmic.yaml delete mode 100644 drivers/staging/hikey9xx/hisilicon,hisi-spmi-controller.yaml delete mode 100644 drivers/staging/hikey9xx/phy-hi3670-usb3.c delete mode 100644 drivers/staging/hikey9xx/phy-hi3670-usb3.yaml