linux-kernel - RE: [PATCH v3 01/11] MFD: DA9052 MFD core module

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <C3AE124F08223B42BC95AEB82F0F6CEDC805@KCHJEXMB02.kpit.com>
Date:	Tue, 9 Aug 2011 08:45:47 +0000
From:	Ashish Jangam <Ashish.Jangam@...tcummins.com>
To:	Mark Brown <broonie@...nsource.wolfsonmicro.com>
CC:	"sameo@...nedhand.com" <sameo@...nedhand.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Dajun <dajun.chen@...semi.com>,
	"linaro-dev@...ts.linaro.org" <linaro-dev@...ts.linaro.org>
Subject: RE: [PATCH v3 01/11] MFD: DA9052 MFD core module


> -----Original Message-----
> From: Mark Brown [mailto:broonie@...nsource.wolfsonmicro.com]
> Sent: Saturday, August 06, 2011 7:09 PM
> To: Ashish Jangam
> Cc: sameo@...nedhand.com; linux-kernel@...r.kernel.org; Dajun; linaro-
> dev@...ts.linaro.org
> Subject: Re: [PATCH v3 01/11] MFD: DA9052 MFD core module
> 
> On Fri, Aug 05, 2011 at 07:23:44PM +0530, ashishj3 wrote:
> 
> > +choice
> > +	prompt "Chip Type"
> > +	depends on MFD_DA9053_SPI || MFD_DA9053_I2C
> > +config PMIC_DA9053AA
> > +	bool "Support Dialog Semiconductor DA9053 AA PMIC"
> > +	help
> > +	  Support for Dialog semiconductor DA9053 AA PMIC.
> > +	  This driver provides common support for accessing  the device,
> > +	  additional drivers must be enabled in order to use the
> > +	  functionality of the device.
> > +config PMIC_DA9053Bx
> 
> Could do with blank lines between blocks.  Though looking at the code
> here I don't understand why these are compile options at all, or if they
> need to be compile options for some reason why they're not independantly
> selectable?
DA9052 PMIC chip id may get OTP therefore chip id cannot be used as 
a distinguishing factor. Hence these compile time options were introduced.
DA9053 is a higher version of DA9052 therefore not independently selectable.
This means that there can be either DA9052 or DA9053 on system. I need to correct
this Kconfig to take care of this. 
> 
> > +int da9052_reg_read(struct da9052 *da9052, unsigned char reg)
> > +{
> > +	int val, ret;
> > +
> > +	if (reg > DA9052_MAX_REG_CNT) {
> > +		dev_err(da9052->dev, "invalid reg %x\n", reg);
> > +		return -EINVAL;
> > +	}
> > +
> > +	#ifdef CONFIG_MFD_DA9052_SPI
> > +		reg = (reg << 1) | 1;
> > +	#endif
> 
> There's several problems here:
> 
> - You shouldn't be indenting preprocessor directives.
> - You shouldn't be adding extra indentation before.
> - This will break I2C devices if SPI support is built into the driver.
> 
> Please, when writing code try to understand the abstractions you're
> using.  For example here think about the purpose of being able to build
> I2C and SPI separately and simultaneously and the goal of the regmap
> API.
> 
> Looks like we need to add per device mangling for the SPI register
> read/write flag.
For now we will handle this as below:-
During SPI and I2C registration we will add bus type(SPI/I2C) info in the
struct da9052. And before initiating any device I/O this struct member will
be read and reg address will be manipulated if needed.
> 
> > +	da9052_group_write(da9052, DA9052_EVENT_A_REG, 4, v);
> > +
> > +	#ifndef CONFIG_PMIC_DA9053Bx
> > +	DA9052_FIXME();
> > +	#endif
> 
> This should be runtime detected based on the device name, either from
> the device registration or by reading back chip identification.
As said above getting chip info will not work in DA9053/53 case. Also DA9052 code 
works for DA9053 except for few minor changes in MFD and regulator module.
In this case registering different device will also require a preprocessor directive
Or separate DA9053 file therefore this option was not opt. 
>