linux-kernel - Re: [PATCH v4 04/10] eeprom: Add a simple EEPROM framework for eeprom consumers

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]
Date:	Tue, 07 Apr 2015 21:09:38 +0100
From:	Srinivas Kandagatla <srinivas.kandagatla@...aro.org>
To:	Stephen Boyd <sboyd@...eaurora.org>
CC:	linux-arm-kernel@...ts.infradead.org,
	Maxime Ripard <maxime.ripard@...e-electrons.com>,
	Rob Herring <robh+dt@...nel.org>,
	Kumar Gala <galak@...eaurora.org>,
	Mark Brown <broonie@...nel.org>, s.hauer@...gutronix.de,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	linux-api@...r.kernel.org, linux-kernel@...r.kernel.org,
	devicetree@...r.kernel.org, linux-arm-msm@...r.kernel.org,
	arnd@...db.de
Subject: Re: [PATCH v4 04/10] eeprom: Add a simple EEPROM framework for eeprom
 consumers

Thanks Stephen for review,

On 07/04/15 19:45, Stephen Boyd wrote:
> On 03/30, Srinivas Kandagatla wrote:
>> @@ -130,6 +138,37 @@ static struct class eeprom_class = {
>>   	.dev_release	= eeprom_release,
>>   };
>>
>> +static int of_eeprom_match(struct device *dev, const void *eeprom_np)
>> +{
>> +	return dev->of_node == eeprom_np;
>> +}
>> +
>> +static struct eeprom_device *of_eeprom_find(struct device_node *eeprom_np)
>> +{
>> +	struct device *d;
>> +
>> +	if (!eeprom_np)
>> +		return NULL;
>> +
>> +	d = class_find_device(&eeprom_class, NULL, eeprom_np, of_eeprom_match);
>> +
>> +	return d ? to_eeprom(d) : NULL;
>> +}
>> +
>> +static int eeprom_match(struct device *dev, const void *data)
>> +{
>> +	return !strcmp(dev_name(dev), (const char *)data);
>
> Is this cast necessary?
May be an over do here, I will fix it.
>
>> +}
>> +
>> +static struct eeprom_device *eeprom_find(const char *name)
>> +{
>> +	struct device *d;
>> +
>> +	d = class_find_device(&eeprom_class, NULL, (void *)name, eeprom_match);
>
> Is this cast necessary?

I Will fix it in next version.
>
>> +
>> +	return d ? to_eeprom(d) : NULL;
>> +}
>> +
>>   /**
>>    * eeprom_register(): Register a eeprom device for given eeprom.
>>    * Also creates an binary entry in /sys/class/eeprom/name-id/eeprom
>> +
>> +/**
>> + * eeprom_cell_get(): Get eeprom cell of device form a given eeprom name
>
> s/form/from/

Will fix this in next version.
>
>> + * and blocks.
>> + *
>> + * @ename: eeprom device name that needs to be looked-up.
>> + * @blocks: eeprom blocks containing offset and length information.
>> + * @nblocks: number of eeprom blocks.
>> + *
>> + * The return value will be an ERR_PTR() on error or a valid pointer
>> + * to a struct eeprom_cell.  The eeprom_cell will be freed by the
>> + * eeprom_cell_put().
>> + */
>> +struct eeprom_cell *eeprom_cell_get(const char *ename,
>> +				    struct eeprom_block *blocks, int nblocks)
>> +{
>> +	return __eeprom_cell_get(NULL, ename, blocks, nblocks);
>> +}
>> +EXPORT_SYMBOL_GPL(eeprom_cell_get);
>> +
>> +/**
>> + * of_eeprom_cell_get(): Get eeprom cell of device form a given index
>
> s/form/from/
>
Ok,
>> + *
>> + * @dev node: Device tree node that uses the eeprom cell
>> +
>> +#ifndef _LINUX_EEPROM_CONSUMER_H
>> +#define _LINUX_EEPROM_CONSUMER_H
>> +
>> +struct eeprom_cell;
>> +
>> +struct eeprom_block {
>> +	loff_t offset;
>> +	size_t count;
>> +};
>> +#if IS_ENABLED(CONFIG_EEPROM)
>> +struct eeprom_cell *eeprom_cell_get(const char *ename,
>> +				    struct eeprom_block *blocks, int nblocks);
>> +void eeprom_cell_put(struct eeprom_cell *cell);
>> +char *eeprom_cell_read(struct eeprom_cell *cell, ssize_t *len);
>> +int eeprom_cell_write(struct eeprom_cell *cell, const char *buf, ssize_t len);
> [...]
>> +
>> +#if IS_ENABLED(CONFIG_EEPROM) && IS_ENABLED(CONFIG_OF)
>> +struct eeprom_cell *of_eeprom_cell_get(struct device_node *dev,
>> +				       const char *property);
>> +#else
>> +static inline struct eeprom_cell *of_eeprom_cell_get(struct device_node *np,
>> +						     const char *property)
>> +{
>> +	return ERR_PTR(-ENOSYS);
>> +}
>> +#endif
>> +#endif  /* ifndef _LINUX_EEPROM_CONSUMER_H */
>
> Do you have an overview of how to use these APIs? Maybe some
> Documentation/ is in order? I'm mostly interested in how the
> blocks array is supposed to work and how this hooks up to drivers
> that are using DT.

Only doc ATM is function level kernel doc in c file.
May be I can explain you for now and I will try to add some 
documentation with some usage examples in next version.

eeprom block array is just another way intended to get hold of eeprom 
content for non-DT providers/consumers, but DT consumers/providers can 
also use it. As of today SOC/mach level code could use it as well.

In eeprom_cell_get() case the lookup of provider is done based on 
provider name, this provider name is generally supplied by all the 
providers (both DT/non DT).

for example in qfprom case,
provider is registered from DT with eeprom config containing a unique name:
static struct eeprom_config econfig = {
	.name = "qfprom",
	.id = 0,
};

In the consumer case, the tsens driver could do some like in non DT way:

	struct eeprom_block blocks[4] ={
		{
		.offset = 0x404,
		.count = 0x4,
		},
		{
		.offset = 0x408,
		.count = 0x4,
		},
		{
		.offset = 0x40c,
		.count = 0x4,
		},
		{
		.offset = 0x410,
		.count = 0x4,
		},
	};
	calib_cell = eeprom_cell_get("qfprom0", blocks, 4);


Or in DT way
calib_cell  = of_eeprom_cell_get(np, "calib");


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