RE: [PATCH V1] mfd: da9053: ensure the FAULT_LOG is cleared during MFD driver probe

[Steve Twiss]

On 08 August 2016 12:06, Steve Twiss wrote:

> On 05 August 2016 10:05, Lee Jones wrote:
> > On Wed, 06 Jul 2016, Steve Twiss wrote:
> > > From: Steve Twiss <stwiss.opensource@xxxxxxxxxxx>
> > >
> > > Clearance of any the persistent information within the DA9053 FAULT_LOG
> > > register must be completed during start-up so the fault-log does not
> > > continue with previous values. A clearance function has been added here in
> > > the kernel driver because wiping the fault-log cannot be counted on outside
> > > the Linux kernel.
> 
> [...]
> 
> > > This patch is similar to the requirements for DA9062 and DA9063: to ensure
> > > the FAULT_LOG is started from a 'clean' position.
> > >
> > > The Dialog datasheet for DA9053 suggests: "The FAULT_LOG register has to
> > > be cleared from the host after reading, by writing a value of 11111111".
> > >
> > > See reference:
> > >   commit 9011e4a8a6fe57f76511609930ed00d305389089
> > >   Author: Steve Twiss <stwiss.opensource@xxxxxxxxxxx>
> > >   Date:   Tue May 19 11:32:45 2015 +0100
> >
> > Looks like much of the same code.  Can you have a pop at generifying
> > this to avoid any unnecessary duplication?
> 
> Hi Lee,
> 
> Although the function looks like those found in both the DA9062 and DA9063 drivers, I was just
> intending to highlight the "requirement similarities" to clear the fault register on start-up. This
> requirement for the Dialog PMIC chips extends across those devices, but the function is not
> really general enough to be merged into the kernel core or reused across other Dialog devices.
> 
> I don't think there is unnecessary duplication in this particular case for the DA9052/53 driver.
> 
> This da9052_clear_fault_log() function will apply to both the Dialog DA9052 and DA9053 PMICs
> and so I am making good use of code re-use in this place. But, this does not extend to the
> DA9062 or DA9063.
> 
> I don't think this FAULT_LOG clearance function in this case is really open to generalisation past
> the DA9052/53 driver.

Hi Lee,

There is another point I missed out for this.
The fault log function is individual for each chip and would be customisable to meet the needs
of the customer's target platform.

The function is meant to be individual. This allows for multiple chips in the same platform -- each
fault log function could be cleared slightly differently in that case.

To start with, I just added the fault log function in my last patch and highlighted its similarity
to DA9062 and DA9063 so that previous commits of the same type could be used to back-up my
request to add this new one to the DA9052/53 driver. I did not intended to cause confusion or
and it was not intended to suggest a possible point to generalise functionality across a range 
of Dialog PMICs.

Regards,
Steve

> > >  drivers/mfd/da9052-core.c | 51
> > +++++++++++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 51 insertions(+)
> > >
> > > diff --git a/drivers/mfd/da9052-core.c b/drivers/mfd/da9052-core.c
> > > index c0bf68a..a88c206 100644
> > > --- a/drivers/mfd/da9052-core.c
> > > +++ b/drivers/mfd/da9052-core.c
> > > @@ -167,6 +167,7 @@ static bool da9052_reg_writeable(struct device *dev, unsigned int reg)
> > >  	case DA9052_EVENT_B_REG:
> > >  	case DA9052_EVENT_C_REG:
> > >  	case DA9052_EVENT_D_REG:
> > > +	case DA9052_FAULTLOG_REG:
> > >  	case DA9052_IRQ_MASK_A_REG:
> > >  	case DA9052_IRQ_MASK_B_REG:
> > >  	case DA9052_IRQ_MASK_C_REG:
> > > @@ -541,6 +542,52 @@ const struct regmap_config da9052_regmap_config = {
> > >  };
> > >  EXPORT_SYMBOL_GPL(da9052_regmap_config);
> > >
> > > +static int da9052_clear_fault_log(struct da9052 *da9052)
> > > +{
> > > +	int ret = 0;
> > > +	int fault_log = 0;
> > > +
> > > +	fault_log = da9052_reg_read(da9052, DA9052_FAULTLOG_REG);
> > > +	if (fault_log < 0) {
> > > +		dev_err(da9052->dev,
> > > +			"Cannot read FAULT_LOG %d\n", fault_log);
> > > +		return fault_log;
> > > +	}
> > > +
> > > +	if (fault_log) {
> > > +		if (fault_log & DA9052_FAULTLOG_TWDERROR)
> > > +			dev_dbg(da9052->dev,
> > > +				"Fault log entry detected: TWD_ERROR\n");
> > > +		if (fault_log & DA9052_FAULTLOG_VDDFAULT)
> > > +			dev_dbg(da9052->dev,
> > > +				"Fault log entry detected: VDD_FAULT\n");
> > > +		if (fault_log & DA9052_FAULTLOG_VDDSTART)
> > > +			dev_dbg(da9052->dev,
> > > +				"Fault log entry detected: VDD_START\n");
> > > +		if (fault_log & DA9052_FAULTLOG_TEMPOVER)
> > > +			dev_dbg(da9052->dev,
> > > +				"Fault log entry detected: TEMP_OVER\n");
> > > +		if (fault_log & DA9052_FAULTLOG_KEYSHUT)
> > > +			dev_dbg(da9052->dev,
> > > +				"Fault log entry detected: KEY_SHUT\n");
> > > +		if (fault_log & DA9052_FAULTLOG_NSDSET)
> > > +			dev_dbg(da9052->dev,
> > > +				"Fault log entry detected: nSD_SHUT\n");
> > > +		if (fault_log & DA9052_FAULTLOG_WAITSET)
> > > +			dev_dbg(da9052->dev,
> > > +				"Fault log entry detected: WAIT_SHUT\n");
> > > +
> > > +		ret = da9052_reg_write(da9052,
> > > +					DA9052_FAULTLOG_REG,
> > > +					0xFF);
> > > +		if (ret < 0)
> > > +			dev_err(da9052->dev,
> > > +				"Cannot reset FAULT_LOG values %d\n", ret);
> > > +	}
> > > +
> > > +	return ret;
> > > +}
> > > +
> > >  int da9052_device_init(struct da9052 *da9052, u8 chip_id)
> > >  {
> > >  	struct da9052_pdata *pdata = dev_get_platdata(da9052->dev);
> > > @@ -549,6 +596,10 @@ int da9052_device_init(struct da9052 *da9052, u8 chip_id)
> > >  	mutex_init(&da9052->auxadc_lock);
> > >  	init_completion(&da9052->done);
> > >
> > > +	ret = da9052_clear_fault_log(da9052);
> > > +	if (ret < 0)
> > > +		dev_warn(da9052->dev, "Cannot clear FAULT_LOG\n");
> > > +
> > >  	if (pdata && pdata->init != NULL)
> > >  		pdata->init(da9052);
> > >