On Fri, 29 Jan 2021, Mauro Carvalho Chehab wrote:
> Hi Lee,
>
> Em Wed, 27 Jan 2021 11:05:37 +0000
> Lee Jones <[email protected]> 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
[email protected]
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
