linux-kernel - Re: [PATCH v2 1/1] mfd: intel_quark_i2c_gpio: Add Intel Quark X1000 I2C-GPIO MFD Driver

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <20141125165105.GP4241@x1>
Date:	Tue, 25 Nov 2014 16:51:05 +0000
From:	Lee Jones <lee.jones@...aro.org>
To:	"Tan, Raymond" <raymond.tan@...el.com>
Cc:	Samuel Ortiz <sameo@...ux.intel.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"Chen, Alvin" <alvin.chen@...el.com>,
	"Shevchenko, Andriy" <andriy.shevchenko@...el.com>
Subject: Re: [PATCH v2 1/1] mfd: intel_quark_i2c_gpio: Add Intel Quark X1000
 I2C-GPIO MFD Driver

Mike,

Something for you down below.

> > > In Quark X1000, there's a single PCI device that provides both an I2C
> > > controller and a GPIO controller. This MFD driver will split the 2
> > > devices for their respective drivers.
> > >
> > > This patch is based on Josef Ahmad's initial work for Quark enabling.
> > >
> > > Reviewed-by: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
> > > Signed-off-by: Weike Chen <alvin.chen@...el.com>
> > > Signed-off-by: Raymond Tan <raymond.tan@...el.com>
> > > ---
> > >  drivers/mfd/Kconfig                |   11 ++
> > >  drivers/mfd/Makefile               |    1 +
> > >  drivers/mfd/intel_quark_i2c_gpio.c |  298
> > > ++++++++++++++++++++++++++++++++++++
> > >  3 files changed, 310 insertions(+)
> > >  create mode 100644 drivers/mfd/intel_quark_i2c_gpio.c

[...]

> > > +	depends on COMMON_CLK
> > 
> > I don't think you should depend on this.  Just use the API and fail if it doesn't
> > find the clock you're after.
> 
> This was added to make sure the clk initialization and other clk related calls will work with the MFD.

[...]

> > > +struct clk *intel_quark_i2c_clk;
> > > +struct clk_lookup *intel_quark_i2c_clk_lookups;
> > 
> > Why do these need to be global?
> 
> I was trying to keep these as runtime allocated variables, and cleaned up with removal of the driver / upon error handling.

They still can be.  But they need to be in a struct somewhere and not
global.

[...]

> > > +static int intel_quark_register_i2c_clk(struct pci_dev *pdev) {
> > > +	intel_quark_i2c_clk_lookups = devm_kcalloc(
> > > +
> > > +		&pdev->dev, INTEL_QUARK_I2C_NCLK,
> > > +		sizeof(*intel_quark_i2c_clk_lookups), GFP_KERNEL);
> > > +
> > > +	if (!intel_quark_i2c_clk_lookups)
> > > +		return -ENOMEM;
> > > +
> > > +	intel_quark_i2c_clk_lookups[0].dev_id =
> > > +INTEL_QUARK_I2C_CONTROLLER_CLK;
> > > +
> > > +	intel_quark_i2c_clk = clk_register_fixed_rate(
> > > +		&pdev->dev, INTEL_QUARK_I2C_CONTROLLER_CLK, NULL,
> > > +		CLK_IS_ROOT, INTEL_QUARK_I2C_CLK_HZ);
> > > +
> > > +	return clk_register_clkdevs(intel_quark_i2c_clk,
> > > +		intel_quark_i2c_clk_lookups, INTEL_QUARK_I2C_NCLK);
> > 
> > We don't normally register clks from MFD.  Normally they are registered in
> > drivers/clk and fetched when a device requires them.
> > Why is this different?
> 
> This is a static clk and on this platform, there's no clk hardware
> to have the clk driver to initialize this and add into the global
> struct for use
> later by i2c controller. The hardware on the platform itself is
> same, and the current driver (i2c_designware_platform) requires the
> clk 
> for its operation. Due to this, I decided to have the clk initialize
> in the MFD before the device being added, which later will trigger
> the probe on the i2c controller driver. 

Mike,

Can I get your input on this please?

[...]

> > > +	pdata = intel_quark_get_i2c_mode(pdev);
> > > +	if (IS_ERR(pdata))
> > > +		return PTR_ERR(pdata);
> > 
> > Bring this functionality into here?
> 
> Were you referring the contents of intel_quark_get_i2c_mode(), and avoid the additional function call instead?

Right.  Please rid intel_quark_get_i2c_mode().

[...]

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/