RE: [NEW DRIVER V3 1/8] DA9058 MFD core driver

["Opensource [Anthony Olech]" <anthony.olech.opensource@diasemi.com>]

> -----Original Message-----
> From: Mark Brown [mailto:broonie@opensource.wolfsonmicro.com]
> Sent: 15 August 2012 19:53
> To: Opensource [Anthony Olech]
> Cc: Samuel Ortiz; Arnd Bergmann; Mauro Carvalho Chehab; Steven Toth;
> Michael Krufky; LKML; David Dajun Chen
> Subject: Re: [NEW DRIVER V3 1/8] DA9058 MFD core driver
> On Wed, Aug 15, 2012 at 04:05:21PM +0100, Anthony Olech wrote:
> >
> >  if HAS_IOMEM
> > +
> >  menu "Multifunction device drivers"
> This random change is still present from the first version....
> > +	/*
> > +	 * the init_board_irq() call-back function should be defined in
> > +	 * the machine driver initialization code and is used to set up
> > +	 * the actual (probably GPIO) line as an interrupt line.
> > +	 */
> > +	if (pdata->init_board_irq) {
> > +		ret = pdata->init_board_irq();
> > +		if (ret)
> > +			goto failed_to_setup_the_actual_i2c_hw_irq;
> > +	}
> You appear to have ignored my previous review comments about this...  it still
> shouldn't be needed with modern kernels.


The comment you made was "Why is this conditional?  With irqdomains there's no real reason
to not allocate the IRQs even if you can't usefully use them and it helps make the code simpler."

I obviously did not understand, because you also wrote that the driver must not rely of platform
data. That therefore means that the call-back function pdata->init_board_irq might be NULL,
which in turn means that a check must be done.
 
The only other interpretation of your comment is that the h/w IRQ line setup code must be done
in the machine driver, in my case for testing is arch/arm/mach-s3c64xx/mach-smdk6410.c
But that view is contricted by the init call-backs from the wm8350 and wm1192 drivers.

I am confused, but it does make sense to do the GPIO --> IRQ initialization in the machine driver,
so I will try to do that. But who would be responsible for changing the wm8350 and wm1192 drivers
to also follow that philosophy?


> > +static bool da9058_register_volatile(struct device *dev, unsigned int
> > +reg) {
> > +	switch (reg) {
> > +	case DA9058_ADCMAN_REG:
> > +	case DA9058_ADCRESH_REG:
> > +	case DA9058_ADCRESL_REG:
> > +	case DA9058_ALARMD_REG:
> > +	case DA9058_ALARMH_REG:
> > +	case DA9058_ALARMMI_REG:
> > +	case DA9058_ALARMMO_REG:
> > +	case DA9058_ALARMS_REG:
> > +	case DA9058_ALARMY_REG:
> Are all the alarm registers really volatile?


register DA9058_ADCMAN_REG has one bit that is set by the PMIC
register DA9058_ADCRESH_REG has all bits set by the PMIC
register DA9058_ADCRESL_REG has some bits set by the PMIC

so those are volatile

the registers DA9058_ALARMD... are, on detailed inspection of the chip
documentation, not volatile. I misread the docs previous. I will make the
change. Well done for spotting my mistake - thanks!


> > +	case DA9058_LDO9_REG:
> > +	case DA9058_TOFFSET_REG:
> > +	default:
> > +		return false;
> Just use the default.


The compiler will (I hope) produce the same code even if some of the
default values are enumerated. I produced those call-backs by starting
with all the registers and moving the cases around. I seemed a good
idea at the time. Your objection, I presume, is based on source file size
or aesthetics, but I will eliminate the functionally redundant cases.


> > +static struct regulator_consumer_supply platform_vddarm_consumers[] = {
> > +	{.supply = "vcc",}
> > +};
> No, this and all your other regulator configuration is board specific.


I will remove that.


Thank you Mark for the review.

Tony Olech