Re: [PATCH 2/2] mfd: arizona: Add DT/ACPI u32 helper functions

[Lee Jones <lee.jones@linaro.org>]

On Mon, 25 May 2015, Charles Keepax wrote:

> Add helpers to read u32's and arrays of u32's from device tree or ACPI
> using the new device properties API.
> 
> Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
> ---
>  drivers/mfd/arizona-core.c       |   39 ++++++++++++++++++++++++++++++++++++++
>  include/linux/mfd/arizona/core.h |    5 ++++
>  2 files changed, 44 insertions(+), 0 deletions(-)

Me no likey!

There are 100's of 'required' and 'optional' properties in the kernel
and it's normally the decision to early return or not that seals the
fate of a given driver.  I'm not happy for platforms to start adding
their own variants.

If you really do think these kinds of "helpers" add value, then make
them generic, so everyone can make use of them.  Personally, I'm
dubious however.

> diff --git a/drivers/mfd/arizona-core.c b/drivers/mfd/arizona-core.c
> index bebf58a..fb55769 100644
> --- a/drivers/mfd/arizona-core.c
> +++ b/drivers/mfd/arizona-core.c
> @@ -20,6 +20,7 @@
>  #include <linux/of_device.h>
>  #include <linux/of_gpio.h>
>  #include <linux/pm_runtime.h>
> +#include <linux/property.h>
>  #include <linux/regmap.h>
>  #include <linux/regulator/consumer.h>
>  #include <linux/regulator/machine.h>
> @@ -746,6 +747,44 @@ int arizona_of_get_named_gpio(struct arizona *arizona, const char *prop,
>  }
>  EXPORT_SYMBOL_GPL(arizona_of_get_named_gpio);
>  
> +int arizona_device_read_u32_array(struct arizona *arizona,
> +				  const char *prop, bool mandatory,
> +				  u32 *data, size_t num)
> +{
> +	int ret;
> +
> +	ret = device_property_read_u32_array(arizona->dev, prop, data, num);
> +	switch (ret) {
> +	case 0:
> +		break;
> +	case -EINVAL:
> +		if (!mandatory)
> +			break;
> +
> +		dev_err(arizona->dev, "Mandatory property %s is missing\n",
> +			prop);
> +		break;
> +	default:
> +		dev_err(arizona->dev, "Property %s is malformed: %d\n",
> +			prop, ret);
> +		break;
> +	}
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(arizona_device_read_u32_array);
> +
> +unsigned int arizona_device_read_u32(struct arizona *arizona,
> +				     const char *prop, bool mandatory)
> +{
> +	u32 data = 0;
> +
> +	arizona_device_read_u32_array(arizona, prop, mandatory, &data, 1);
> +
> +	return data;
> +}
> +EXPORT_SYMBOL_GPL(arizona_device_read_u32);
> +
>  static int arizona_of_get_core_pdata(struct arizona *arizona)
>  {
>  	struct arizona_pdata *pdata = &arizona->pdata;
> diff --git a/include/linux/mfd/arizona/core.h b/include/linux/mfd/arizona/core.h
> index 847bdaf..1c3d39d 100644
> --- a/include/linux/mfd/arizona/core.h
> +++ b/include/linux/mfd/arizona/core.h
> @@ -160,5 +160,10 @@ int wm8997_patch(struct arizona *arizona);
>  
>  extern int arizona_of_get_named_gpio(struct arizona *arizona, const char *prop,
>  				     bool mandatory);
> +extern int arizona_device_read_u32_array(struct arizona *arizona,
> +					 const char *prop, bool mandatory,
> +					 u32 *data, size_t num);
> +extern unsigned int arizona_device_read_u32(struct arizona *arizona,
> +					    const char *prop, bool mandatory);
>  
>  #endif

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog