Message-ID: <5678955B.4070700@mleia.com>
Date: Tue, 22 Dec 2015 02:12:11 +0200
From: Vladimir Zapolskiy <vz@mleia.com>
User-Agent: Mozilla/5.0 (X11; Linux i686; rv:31.0) Gecko/20100101 Icedove/31.6.0
MIME-Version: 1.0
To: Cory Tusar <cory.tusar@pid1solutions.com>, robh+dt@kernel.org,
        pawel.moll@arm.com, mark.rutland@arm.com,
        ijc+devicetree@hellion.org.uk, galak@codeaurora.org,
        gregkh@linuxfoundation.org
CC: jic23@kernel.org, broonie@kernel.org, afd@ti.com, andrew@lunn.ch,
        Chris.Healy@zii.aero, Keith.Vennel@zii.aero,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v4 3/5] misc: eeprom_93xx46: Implement eeprom_93xx46 DT
 bindings.
References: <1449720055-25965-1-git-send-email-cory.tusar@pid1solutions.com> <1449720055-25965-4-git-send-email-cory.tusar@pid1solutions.com>
In-Reply-To: <1449720055-25965-4-git-send-email-cory.tusar@pid1solutions.com>
Content-Type: text/plain; charset=windows-1252
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3559
Lines: 119

Hi Cory,

On 10.12.2015 06:00, Cory Tusar wrote:
> This commit implements bindings in the eeprom_93xx46 driver allowing
> device word size and read-only attributes to be specified via
> devicetree.
> 
> Signed-off-by: Cory Tusar <cory.tusar@pid1solutions.com>
> Tested-by: Chris Healy <chris.healy@zii.aero>
> ---
>  drivers/misc/eeprom/eeprom_93xx46.c | 49 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 49 insertions(+)
> 
> diff --git a/drivers/misc/eeprom/eeprom_93xx46.c b/drivers/misc/eeprom/eeprom_93xx46.c
> index e1bf0a5..cc27e11 100644
> --- a/drivers/misc/eeprom/eeprom_93xx46.c
> +++ b/drivers/misc/eeprom/eeprom_93xx46.c
> @@ -13,6 +13,8 @@
>  #include <linux/kernel.h>
>  #include <linux/module.h>
>  #include <linux/mutex.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
>  #include <linux/slab.h>
>  #include <linux/spi/spi.h>
>  #include <linux/sysfs.h>
> @@ -294,12 +296,58 @@ static ssize_t eeprom_93xx46_store_erase(struct device *dev,
>  }
>  static DEVICE_ATTR(erase, S_IWUSR, NULL, eeprom_93xx46_store_erase);
>  
> +static const struct of_device_id eeprom_93xx46_of_table[] = {
> +	{ .compatible = "eeprom-93xx46", },

immediately do you want to add the second (and much, much more preferred IMO)
mentioned compatible value "atmel,at93c46d" to the list? Also see a note
below.

> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, eeprom_93xx46_of_table);
> +
> +static int eeprom_93xx46_probe_dt(struct spi_device *spi)
> +{
> +	struct device_node *np = spi->dev.of_node;
> +	struct eeprom_93xx46_platform_data *pd;
> +	u32 tmp;
> +	int ret;
> +
> +	pd = devm_kzalloc(&spi->dev, sizeof(*pd), GFP_KERNEL);
> +	if (!pd)
> +		return -ENOMEM;
> +
> +	ret = of_property_read_u32(np, "data-size", &tmp);
> +	if (ret < 0) {
> +		dev_err(&spi->dev, "data-size property not found\n");
> +		return ret;
> +	}
> +
> +	if (tmp == 8) {
> +		pd->flags |= EE_ADDR8;
> +	} else if (tmp == 16) {
> +		pd->flags |= EE_ADDR16;
> +	} else {
> +		dev_err(&spi->dev, "invalid data-size (%d)\n", tmp);
> +		return -EINVAL;
> +	}

If you insist on mandatory "data-size" property, could you please check
arch/x86/platform/ce4100/falconfalls.dts , does it require updates?

In that DTS you may find "atmel,at93c46" compatible value, is that kind
of devices covered by this driver? If yes, does it make sense to
add "atmel,at93c46" to the list of compatible values replacing
"atmel,at93c46d" ?

> +
> +	if (of_property_read_bool(np, "read-only"))
> +		pd->flags |= EE_READONLY;
> +
> +	spi->dev.platform_data = pd;
> +
> +	return 0;
> +}
> +
>  static int eeprom_93xx46_probe(struct spi_device *spi)
>  {
>  	struct eeprom_93xx46_platform_data *pd;
>  	struct eeprom_93xx46_dev *edev;
>  	int err;
>  
> +	if (spi->dev.of_node) {
> +		err = eeprom_93xx46_probe_dt(spi);
> +		if (err < 0)
> +			return err;
> +	}
> +
>  	pd = spi->dev.platform_data;
>  	if (!pd) {
>  		dev_err(&spi->dev, "missing platform data\n");
> @@ -370,6 +418,7 @@ static int eeprom_93xx46_remove(struct spi_device *spi)
>  static struct spi_driver eeprom_93xx46_driver = {
>  	.driver = {
>  		.name	= "93xx46",
> +		.of_match_table = of_match_ptr(eeprom_93xx46_of_table),
>  	},
>  	.probe		= eeprom_93xx46_probe,
>  	.remove		= eeprom_93xx46_remove,
> 

Reviewed-by: Vladimir Zapolskiy <vz@mleia.com>

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