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]
Date:	Wed, 24 Sep 2014 12:02:12 +0100
From:	Lee Jones <lee.jones@...aro.org>
To:	Jacob Pan <jacob.jun.pan@...ux.intel.com>
Cc:	IIO <linux-iio@...r.kernel.org>,
	LKML <linux-kernel@...r.kernel.org>,
	DEVICE TREE <devicetree@...r.kernel.org>,
	Carlo Caione <carlo@...one.org>,
	Srinivas Pandruvada <srinivas.pandruvada@...ux.intel.com>,
	Aaron Lu <aaron.lu@...el.com>, Alan Cox <alan@...ux.intel.com>,
	Jean Delvare <khali@...ux-fr.org>,
	Samuel Ortiz <sameo@...ux.intel.com>,
	Liam Girdwood <lgirdwood@...il.com>,
	Mark Brown <broonie@...nel.org>,
	Grant Likely <grant.likely@...aro.org>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Rob Herring <robh+dt@...nel.org>,
	Lars-Peter Clausen <lars@...afoo.de>,
	Hartmut Knaack <knaack.h@....de>,
	Fugang Duan <B38611@...escale.com>,
	Arnd Bergmann <arnd@...db.de>,
	Zubair Lutfullah <zubair.lutfullah@...il.com>,
	Sebastian Reichel <sre@...ian.org>,
	Johannes Thumshirn <johannes.thumshirn@....de>,
	Philippe Reynes <tremyfr@...oo.fr>,
	Angelo Compagnucci <angelo.compagnucci@...il.com>,
	Doug Anderson <dianders@...omium.org>,
	Ramakrishna Pallala <ramakrishna.pallala@...el.com>,
	Peter Meerwald <pmeerw@...erw.net>,
	Maxime Ripard <maxime.ripard@...e-electrons.com>,
	Rafael Wysocki <rafael.j.wysocki@...el.com>
Subject: Re: [PATCH v4 1/3] mfd/axp20x: extend axp20x to support axp288 pmic

On Thu, 18 Sep 2014, Jacob Pan wrote:
> Lee Jones <lee.jones@...aro.org> wrote:
> > On Tue, 16 Sep 2014, Jacob Pan wrote:
> > 
> > > X-Powers AXP288 is a customized PMIC for Intel Baytrail-CR
> > > platforms. Similar to AXP202/209, AXP288 comes with USB charger,
> > > more LDO and BUCK channels, and AD converters. It also provides
> > > extended status and interrupt reporting capabilities than the
> > > devices currently supported in axp20x.c.
> > > 
> > > In addition to feature extension, this patch also adds ACPI binding
> > > for enumeration.
> > > 
> > > This consolidated driver should support more X-Powers' PMICs in
> > > both device tree and ACPI enumerated platforms.
> > > 
> > > Signed-off-by: Jacob Pan <jacob.jun.pan@...ux.intel.com>
> > > ---
> > >  drivers/mfd/Kconfig        |   3 +-
> > >  drivers/mfd/axp20x.c       | 353
> > > +++++++++++++++++++++++++++++++++++++--------
> > > include/linux/mfd/axp20x.h |  58 ++++++++ 3 files changed, 354
> > > insertions(+), 60 deletions(-)

[...]

> > > -static const struct regmap_irq_chip axp20x_regmap_irq_chip = {
> > > +static struct acpi_device_id axp20x_acpi_match[] = {
> > > +	{
> > > +		.id = "INT33F4",
> > > +		.driver_data = (kernel_ulong_t)AXP288_ID,
> > 
> > Why do you need to cast this?
> > 
> to make sure match driver_data which is different in acpi_device_id than
> of_device_id.

You don't need the cast.

[...]

> > > +static int axp20x_match_device(struct axp20x_dev *axp20x, struct
> > > device *dev) +{
> > > +	const struct acpi_device_id *acpi_id;
> > > +	const struct of_device_id *of_id;
> > > +
> > > +	of_id = of_match_device(axp20x_of_match, dev);
> > > +	if (of_id)
> > > +		axp20x->variant = (long) of_id->data;
> > > +	else {
> > > +		acpi_id =
> > > acpi_match_device(dev->driver->acpi_match_table, dev);
> > > +		if (!acpi_id || !acpi_id->driver_data) {
> > > +			dev_err(dev, "Unable to determine ID\n");
> > > +			return -ENODEV;
> > > +		}
> > > +		axp20x->variant = (long) acpi_id->driver_data;
> > > +	}
> > 
> > We can do better error handling here and give the user a better sense
> > of what happened if anything were to go wrong.  Do:
> > 
> > if (dev->of_node)
> >   of_id = of_match_device()
> >   if (!of_id)
> >     error()
> this will give false error on ACPI based platforms, right? in reality

Why would it?  dev->of_node should be NULL if running ACPI?

[...]

> > > +	switch (axp20x->variant) {
> > > +	case AXP202_ID:
> > > +	case AXP209_ID:
> > > +		axp20x->nr_cells = ARRAY_SIZE(axp20x_cells);
> > > +		axp20x->cells = axp20x_cells;
> > > +		axp20x->regmap_cfg = &axp20x_regmap_config;
> > > +		axp20x_regmap_irq_chip.num_regs	= 5;
> > > +		axp20x_regmap_irq_chip.irqs = axp20x_regmap_irqs;
> > > +		axp20x_regmap_irq_chip.num_irqs	=
> > > +			ARRAY_SIZE(axp20x_regmap_irqs);
> > > +		break;
> > > +	case AXP288_ID:
> > > +		axp20x->cells = axp288_cells;
> > > +		axp20x->nr_cells = ARRAY_SIZE(axp288_cells);
> > > +		axp20x->regmap_cfg = &axp288_regmap_config;
> > > +		axp20x_regmap_irq_chip.irqs = axp288_regmap_irqs;
> > > +		axp20x_regmap_irq_chip.num_irqs	=
> > > +			ARRAY_SIZE(axp288_regmap_irqs);
> > > +		axp20x_regmap_irq_chip.num_regs	= 6;
> > > +		break;
> > > +	default:
> > > +		dev_err(dev, "unsupported AXP20X ID %lu\n",
> > > axp20x->variant);
> > > +		return -ENODEV;
> > 
> > -EINVAL might be better here.
> I was considering the return value gets propagated to probe function
> which is used to query the existence of a device per driver model. But
> I have no strong preference.

I think -EINVAL would be better as the argument passed in
axp20x->variant is invalid.

define EINVAL          22      /* Invalid argument */

> > > +	}
> > > +	dev_info(dev, "AXP20x variant %s found\n",
> > > +		axp20x_model_names[axp20x->variant]);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > >  static int axp20x_i2c_probe(struct i2c_client *i2c,
> > > -			 const struct i2c_device_id *id)
> > > +			const struct i2c_device_id *id)
> > 
> > Sneaky. ;)
> I should not fix the extra white spaces here, unrelated to this patch.
> will remove.

It's okay.  I don't mind little things like this occasionally.  I find
them more amusing than harmful.

[...]

-- 
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/

Powered by blists - more mailing lists