Re: [PATCH v4 3/5] misc: eeprom_93xx46: Implement eeprom_93xx46 DT bindings.

[Vladimir Zapolskiy <vz@mleia.com>]

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