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: <20180415161909.34877620@archlinux>
Date:   Sun, 15 Apr 2018 16:19:09 +0100
From:   Jonathan Cameron <jic23@...nel.org>
To:     Hernán Gonzalez <hernan@...guardiasur.com.ar>
Cc:     knaack.h@....de, lars@...afoo.de, pmeerw@...erw.net,
        gregkh@...uxfoundation.org, Michael.Hennerich@...log.com,
        linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 08/14] staging: iio: ad7746: Add dt-bindings

On Fri, 13 Apr 2018 13:36:45 -0300
Hernán Gonzalez <hernan@...guardiasur.com.ar> wrote:

> This patch adds dt bindings by populating a pdata struct in order to
> modify as little as possible the existing code. It supports both
> platform_data and dt-bindings but uses only one depending on
> CONFIG_OF's value.
> 
> Signed-off-by: Hernán Gonzalez <hernan@...guardiasur.com.ar>

Please reorder the patches in the next version to put the bindings
patch next to this one (before preferably, but after is fine as
well.)

Hmm. I can see why you want to minimize the effect of the older
code, but given that we will probably (eventually) drop the
platform data option, I wonder if it wouldn't be better
to move the data to a sensible location rather than faking
platform_data.

The pdata is only used in probe and only two bits of
it at that so it would be fine to use some local variables and
fill them from platform data or device tree as appropriate.

Jonathan

> ---
>  drivers/staging/iio/cdc/ad7746.c | 54 +++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 53 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/iio/cdc/ad7746.c b/drivers/staging/iio/cdc/ad7746.c
> index d39ab34..c29a221 100644
> --- a/drivers/staging/iio/cdc/ad7746.c
> +++ b/drivers/staging/iio/cdc/ad7746.c
> @@ -666,6 +666,43 @@ static const struct iio_info ad7746_info = {
>  /*
>   * device probe and remove
>   */
> +#ifdef CONFIG_OF
> +static struct ad7746_platform_data *ad7746_parse_dt(struct device *dev)
> +{
> +	struct device_node *np = dev->of_node;
> +	struct ad7746_platform_data *pdata;
> +	unsigned int tmp;
> +	int ret;
> +
> +	/* The default excitation outputs are not inverted, it should be stated
Please use standard multiline comment syntax.

/*
 * The...
 */
> +	 * in the dt if needed.
> +	 */
> +
> +	pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
> +	if (!pdata)
> +		return NULL;
> +
> +	ret = of_property_read_u32(np, "adi,exclvl", &tmp);
> +	if (ret || tmp > 3) {
> +		dev_warn(dev, "Wrong exclvl value, using default\n");

Seems a little odd to have the check in here rather than generic.

> +		pdata->exclvl = 3; /* default value */
> +	} else {
> +		pdata->exclvl = tmp;
> +	}
> +
> +	pdata->exca_en = true;
> +	pdata->excb_en = true;
> +	pdata->exca_inv_en = of_property_read_bool(np, "adi,nexca_en");
> +	pdata->excb_inv_en = of_property_read_bool(np, "adi,nexcb_en");
> +
> +	return pdata;
> +}
> +#else
> +static struct ad7746_platform_data *ad7746_parse_dt(struct device *dev)
> +{
> +	return NULL;
> +}
> +#endif
>  
>  static int ad7746_probe(struct i2c_client *client,
>  			const struct i2c_device_id *id)
> @@ -676,6 +713,11 @@ static int ad7746_probe(struct i2c_client *client,
>  	unsigned char regval = 0;
>  	int ret = 0;
>  
> +	if (client->dev.of_node)
> +		pdata = ad7746_parse_dt(&client->dev);
> +	else
> +		pdata = client->dev.platform_data;
> +
>  	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*chip));
>  	if (!indio_dev)
>  		return -ENOMEM;
> @@ -739,12 +781,22 @@ static const struct i2c_device_id ad7746_id[] = {
>  	{ "ad7747", 7747 },
>  	{}
>  };
> -
>  MODULE_DEVICE_TABLE(i2c, ad7746_id);
>  
> +#ifdef CONFIG_OF
> +static const struct of_device_id ad7746_of_match[] = {
> +	{ .compatible = "adi,ad7745" },
> +	{ .compatible = "adi,ad7746" },
> +	{ .compatible = "adi,ad7747" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, ad7746_of_match);
> +#endif
> +
>  static struct i2c_driver ad7746_driver = {
>  	.driver = {
>  		.name = KBUILD_MODNAME,
> +		.of_match_table = of_match_ptr(ad7746_of_match),
>  	},
>  	.probe = ad7746_probe,
>  	.id_table = ad7746_id,

Powered by blists - more mailing lists