Re: [PATCH v4 18/21] mfd: hi6421-spmi-pmic: move driver from staging

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux Driver Backports]     [DMA Engine]     [Linux GPIO]     [Linux SPI]     [Video for Linux]     [Linux USB Devel]     [Linux Coverity]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux