Re: [PATCH v4 1/3] mfd/axp20x: extend axp20x to support axp288 pmic

[Jacob Pan]

On Wed, 17 Sep 2014 22:39:26 -0700
Lee Jones <lee.jones@xxxxxxxxxx> 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@xxxxxxxxxxxxxxx>
> > ---
> >  drivers/mfd/Kconfig        |   3 +-
> >  drivers/mfd/axp20x.c       | 353
> > +++++++++++++++++++++++++++++++++++++--------
> > include/linux/mfd/axp20x.h |  58 ++++++++ 3 files changed, 354
> > insertions(+), 60 deletions(-)
> > 
> > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> > index de5abf2..c183edb 100644
> > --- a/drivers/mfd/Kconfig
> > +++ b/drivers/mfd/Kconfig
> > @@ -74,7 +74,8 @@ config MFD_AXP20X
> >  	select REGMAP_IRQ
> >  	depends on I2C=y
> >  	help
> > -	  If you say Y here you get support for the X-Powers
> > AXP202 and AXP209.
> > +	  If you say Y here you get support for the X-Powers
> > AXP202, AXP209 and
> > +	  AXP288 power management IC (PMIC).
> >  	  This driver include only the core APIs. You have to
> > select individual components like regulators or the PEK (Power
> > Enable Key) under the corresponding menus.
> 
> [...]
> 
> > -static const struct regmap_config axp20x_regmap_config = {
> 
> [...]
> 
> > +static struct regmap_config axp20x_regmap_config = {
> 
> Why have you removed const status?
I did not use const * in axp20x_dev, let me add that. good point.
> 
> >  	.reg_bits	= 8,
> >  	.val_bits	= 8,
> >  	.wr_table	= &axp20x_writeable_table,
> > @@ -70,47 +129,96 @@ static const struct regmap_config
> > axp20x_regmap_config = { .cache_type	= REGCACHE_RBTREE,
> >  };
> >  
> > -#define AXP20X_IRQ(_irq, _off, _mask) \
> > -	[AXP20X_IRQ_##_irq] = { .reg_offset = (_off), .mask =
> > BIT(_mask) } +static struct regmap_config axp288_regmap_config = {
> 
> const?
> 
ditto
> > +	.reg_bits	= 8,
> > +	.val_bits	= 8,
> > +	.wr_table	= &axp288_writeable_table,
> > +	.volatile_table	= &axp288_volatile_table,
> > +	.max_register	= AXP288_FG_TUNE5,
> > +	.cache_type	= REGCACHE_RBTREE,
> > +};
> 
> [...]
> 
> > -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.
> > +	},
> > +	{ },
> > +};
> > +MODULE_DEVICE_TABLE(acpi, axp20x_acpi_match);
> > +
> > +/* common irq chip attributes only */
> > +static struct regmap_irq_chip axp20x_regmap_irq_chip = {
> >  	.name			= "axp20x_irq_chip",
> >  	.status_base		= AXP20X_IRQ1_STATE,
> >  	.ack_base		= AXP20X_IRQ1_STATE,
> >  	.mask_base		= AXP20X_IRQ1_EN,
> > -	.num_regs		= 5,
> > -	.irqs			= axp20x_regmap_irqs,
> > -	.num_irqs		= ARRAY_SIZE(axp20x_regmap_irqs),
> >  	.mask_invert		= true,
> >  	.init_ack_masked	= true,
> >  };
> > @@ -161,36 +276,155 @@ static struct mfd_cell axp20x_cells[] = {
> >  	},
> >  };
> 
> [...]
> 
> > +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
this driver will only run on ACPI or OF platform not both, so IMHO
there is not need to give user a specific error since they already know
what platform I am running.
>   variant = of_id->data;
> else
>   acpi_id = acpi_match_device()
>   if (!acpi_id)
>     error()
>   variant = acpi_id->driver_data;
> 
> > +	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.
> 
> > +	}
> > +	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.
> 
> >  {
> >  	struct axp20x_dev *axp20x;
> > -	const struct of_device_id *of_id;
> >  	int ret;
> >  
> >  	axp20x = devm_kzalloc(&i2c->dev, sizeof(*axp20x),
> > GFP_KERNEL); if (!axp20x)
> >  		return -ENOMEM;
> >  
> > -	of_id = of_match_device(axp20x_of_match, &i2c->dev);
> > -	if (!of_id) {
> > -		dev_err(&i2c->dev, "Unable to setup AXP20X
> > data\n");
> > -		return -ENODEV;
> > -	}
> > -	axp20x->variant = (long) of_id->data;
> > +	ret = axp20x_match_device(axp20x, &i2c->dev);
> > +	if (ret)
> > +		return ret;
> >  
> >  	axp20x->i2c_client = i2c;
> >  	axp20x->dev = &i2c->dev;
> >  	dev_set_drvdata(axp20x->dev, axp20x);
> >  
> > -	axp20x->regmap = devm_regmap_init_i2c(i2c,
> > &axp20x_regmap_config);
> > +	axp20x->regmap = devm_regmap_init_i2c(i2c,
> > axp20x->regmap_cfg); if (IS_ERR(axp20x->regmap)) {
> >  		ret = PTR_ERR(axp20x->regmap);
> >  		dev_err(&i2c->dev, "regmap init failed: %d\n",
> > ret); @@ -206,8 +440,8 @@ static int axp20x_i2c_probe(struct
> > i2c_client *i2c, return ret;
> >  	}
> >  
> > -	ret = mfd_add_devices(axp20x->dev, -1, axp20x_cells,
> > -			      ARRAY_SIZE(axp20x_cells), NULL, 0,
> > NULL);
> > +	ret = mfd_add_devices(axp20x->dev, -1, axp20x->cells,
> > +			axp20x->nr_cells, NULL, 0, NULL);
> >  
> >  	if (ret) {
> >  		dev_err(&i2c->dev, "failed to add MFD devices:
> > %d\n", ret);
> 
> [...]
> 
> > diff --git a/include/linux/mfd/axp20x.h b/include/linux/mfd/axp20x.h
> > index d0e31a2..ffc69e2 100644
> > --- a/include/linux/mfd/axp20x.h
> > +++ b/include/linux/mfd/axp20x.h
> 
> [...]
> 
> >  struct axp20x_dev {
> >  	struct device			*dev;
> >  	struct i2c_client		*i2c_client;
> >  	struct regmap			*regmap;
> >  	struct regmap_irq_chip_data	*regmap_irqc;
> >  	long				variant;
> > +	int                             nr_cells;
> > +	struct mfd_cell                 *cells;
> > +	struct regmap_config            *regmap_cfg;
> 
> const?
> 
yes
> >  };
> >  
> >  #endif /* __LINUX_MFD_AXP20X_H */
> 

thanks for the review.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/