linux-kernel - Re: [PATCH v5 1/2] mfd: act8945a: add Active-semi ACT8945A PMIC 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: <87pox41dy9.fsf@dell.be.48ers.dk>
Date:	Thu, 14 Jan 2016 14:35:26 +0100
From:	Peter Korsgaard <peter@...sgaard.com>
To:	Wenyou Yang <wenyou.yang@...el.com>
Cc:	Lee Jones <lee.jones@...aro.org>, Rob Herring <robh+dt@...nel.org>,
	"Pawel Moll" <pawel.moll@....com>,
	Mark Rutland <mark.rutland@....com>,
	Ian Campbell <ijc+devicetree@...lion.org.uk>,
	Kumar Gala <galak@...eaurora.org>, devicetree@...r.kernel.org,
	Krzysztof Kozlowski <k.kozlowski@...sung.com>,
	Nicolas Ferre <nicolas.ferre@...el.com>,
	linux-kernel@...r.kernel.org,
	Javier Martinez Canillas <javier@...hile0.org>,
	linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v5 1/2] mfd: act8945a: add Active-semi ACT8945A PMIC MFD driver

>>>>> "Wenyou" == Wenyou Yang <wenyou.yang@...el.com> writes:

 > This patch adds support for the Active-semi ACT8945A PMIC.
 > It is a Multi Function Device with the following subdevices:
 >  - Regulator
 >  - Charger

 > It is interfaced to the host controller using I2C interface,
 > ACT8945A is a child device of the I2C.

 > Signed-off-by: Wenyou Yang <wenyou.yang@...el.com>
 > Reviewed-by: Krzysztof Kozlowski <k.kozlowski@...sung.com>
 > ---

 > Changes in v5:
 >  - change depends on to 'I2C=y'.

Why not make it a tristate instead? Having regulators as modules is
perhaps not a very wise thing to do, but conceptually I don't see why
this code couldn't be a module.
 
 > +config MFD_ACT8945A
 > +	bool "Active-semi ACT8945A"
 > +	select MFD_CORE
 > +	select REGMAP_I2C
 > +	depends on I2C=y && OF
 > +	help
 > +	  Support for the ACT8945A PMIC from Active-semi. This device
 > +	  features three step-down DC/DC converters and four low-dropout
 > +	  linear regulators, along with a complete ActivePath battery
 > +	  charger.
 > +

[snip]

> +++ b/drivers/mfd/act8945a.c
 > @@ -0,0 +1,109 @@
 > +/*
 > + * MFD driver for Active-semi ACT8945a PMIC
 > + *
 > + * Copyright (C) 2015 Atmel Corporation.
 > + *
 > + * Author: Wenyou Yang <wenyou.yang@...el.com>
 > + *
 > + * This program is free software; you can redistribute it and/or modify it
 > + * under  the terms of the GNU General  Public License as published by the
 > + * Free Software Foundation;  either version 2 of the License, or (at your
 > + * option) any later version.
 > + */
 > +
 > +#include <linux/i2c.h>
 > +#include <linux/mfd/act8945a.h>
 > +#include <linux/mfd/core.h>
 > +#include <linux/module.h>
 > +#include <linux/of_device.h>
 > +#include <linux/regmap.h>
 > +
 > +static const struct mfd_cell act8945a_devs[] = {
 > +	{
 > +		.name = "act8945a-pmic",
 > +		.of_compatible = "active-semi,act8945a-regulator",
 > +	},
 > +	{
 > +		.name = "act8945a-charger",
 > +		.of_compatible = "active-semi,act8945a-charger",
 > +	},
 > +};
 > +
 > +static const struct regmap_config act8945a_regmap_config = {
 > +	.reg_bits = 8,
 > +	.val_bits = 8,
 > +};
 > +
 > +static int act8945a_i2c_probe(struct i2c_client *i2c,
 > +			      const struct i2c_device_id *id)
 > +{
 > +	struct act8945a_dev *act8945a;
 > +	int ret;
 > +
 > +	act8945a = devm_kzalloc(&i2c->dev, sizeof(*act8945a), GFP_KERNEL);
 > +	if (!act8945a)
 > +		return -ENOMEM;
 > +

What is the point of this structure (and the header file)? Can't the
subdevices just do dev_get_regmap(dev->parent)? regulator_register()
afaik already does this by default.

-- 
Bye, Peter Korsgaard