On Fri, 29 Jan 2021, Mauro Carvalho Chehab wrote: > Hi Lee, > > Em Wed, 27 Jan 2021 11:05:37 +0000 > Lee Jones <lee.jones@xxxxxxxxxx> escreveu: > > > On Tue, 19 Jan 2021, Mauro Carvalho Chehab wrote: > > > > > This driver is ready for mainstream. So, move it out of staging. > > > > > > Replied to an earlier submission where I was able to reply in-line. > > Sorry! Infradead seemed to have some problem between Jan 26-27: emails > got late-delivered. No problem. > > > +static const struct mfd_cell hi6421v600_devs[] = { > > > + { .name = "hi6421v600-regulator", }, > > > +}; > > > > Where are the reset of the devices? > > Not sure what you mean here. > > This MFD device provides: > > - an IRQ handler; > - several LDO lines used by a regulator driver. > > The IRQ handler is properly initialized here, while the > regulators are initialized by the regulator driver. The initial > state of this device is set up by u-boot. > > So, AFAIKT, there's no need to have any reset line > attached here. Oh wow! Sorry about that. It's a misspelling. "Where are the *rest* of the devices?" Registering one device does not qualify this as an MFD. > Yet, I'm still figuring out how the PCIe chips at Hikey 970 > should be properly initialized. So, I may need to add something > somewhere in order to properly reset and power up the Ethernet, > M.2 and PCIe 1x slot. > > Those are linked to the LDO 33. > > > > +static void hi6421_spmi_irq_mask(struct irq_data *d) > > > +{ > > > + struct hi6421_spmi_pmic *pmic = irq_data_get_irq_chip_data(d); > > > + unsigned long flags; > > > + unsigned int data; > > > + u32 offset; > > > + > > > + offset = (irqd_to_hwirq(d) >> 3); > > > > Why 3? > > No idea. I don't have any datasheets from this device. > > > Probably better to define these shifts/masks rather than use > > magic numbers with no comments. > > I'll change the above to: > > #define HISI_IRQ_MASK GENMASK(1, 0) > > offset = (irqd_to_hwirq(d) >> HISI_IRQ_MASK); This is a shift surely? Marks are usually &'ed. > > > + regmap_read(pmic->map, offset, &data); > > > + data |= (1 << (irqd_to_hwirq(d) & 0x07)); > > > > What are you doing here? > > > > Maybe improved defines will be enough. If not, please supply a > > suitable comment. > > Again, no idea. The only documentation I had access to this chip is > at: > > https://www.96boards.org/documentation/consumer/hikey/hikey970/ > > With doesn't mention any register details. > > The driver itself came from the Linaro's 96boards git tree, with also > doesn't contain any register mapping. Well that's awkward. I'm not keen on merging code that no one knows what it does. Maybe I can do some digging. > > > + pr_debug("PMU IRQ address value:irq[0x%x] = 0x%x\n", > > > + SOC_PMIC_IRQ0_ADDR + i, pending); > > > > Again, is this actually useful to anyone now that the driver is nearly > > 10 years old. Particularly anyone who can't add a quick printk() > > during a debug session? > > With regards to the debug stuff, I'm dropping everything. Great. > On a side comment, I doubt that the driver has 10 years old ;-) > > See, Hikey 970 uses Kirin 970 SoC, which it was launched in Sept, 2017. The header has a copyright from 2011. // Copyright (c) 2013 Linaro Ltd. // Copyright (c) 2011 Hisilicon. > The original version of this driver publicly debuted on this tree: > > https://github.com/96boards-hikey/linux/blob/hikey970-v4.9/drivers/mfd/hisi_pmic_spmi.c > > On a commit made on Feb, 2018. > > Ok, Hi6421v600 is a separate silicon, probably derivative from > Hi6421 (used on Hikey 960). Its copyright mentions 2011, but > that's probably because the code itself came from older generations > of the regulator chipset. So we've inherited a copyright from another driver? Sounds suspect. > Please see the enclosed patch for the new code after fixing the issues > you pointed. I'll re-submit it as a series once you're ok with the > code. Would you mind just resubmitting please? We can go from there. -- Lee Jones [李琼斯] Senior Technical Lead - Developer Services Linaro.org │ Open source software for Arm SoCs Follow Linaro: Facebook | Twitter | Blog _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel