[v4,04/10] eeprom: Add a simple EEPROM framework for eeprom consumers

This patch adds just consumers part of the framework just to enable easy
review.

Up until now, EEPROM drivers were stored in drivers/misc, where they all had to
duplicate pretty much the same code to register a sysfs file, allow in-kernel
users to access the content of the devices they were driving, etc.

This was also a problem as far as other in-kernel users were involved, since
the solutions used were pretty much different from on driver to another, there
was a rather big abstraction leak.

This introduction of this framework aims at solving this. It also introduces DT
representation for consumer devices to go get the data they require (MAC
Addresses, SoC/Revision ID, part numbers, and so on) from the EEPROMs.

Having regmap interface to this framework would give much better
abstraction for eeproms on different buses.

Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
[Maxime Ripard: intial version of the framework]
Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
 drivers/eeprom/core.c           | 271 ++++++++++++++++++++++++++++++++++++++++
 include/linux/eeprom-consumer.h |  61 +++++++++
 2 files changed, 332 insertions(+)
 create mode 100644 include/linux/eeprom-consumer.h

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-arm-msm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Hi Stephen,

Sorry I took so long to reply.


On 09/04/15 15:45, Stephen Boyd wrote:
> On 04/07, Srinivas Kandagatla wrote:
>> On 07/04/15 19:45, Stephen Boyd wrote:
>>> On 03/30, Srinivas Kandagatla wrote:
>>>
>>> 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.
>
> Thanks.
>
>>
>> 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");
>>
>
> Ok. I guess I was hoping for a more device centric approach like
> we have for clks/regulators/etc. That way a driver doesn't need
> to know it's using DT or not to figure out which API to call.

That would be the best. Its easy to wrap up whats in eeprom core to
eeprom_get_cell(dev, "cell-name") for both DT and non-dt cases, if I
remove the nasty global name space thing.

Only thing which is limiting it is the existing bindings which are just 
phandle based. For eeprom to be more of device centric we need more
generic bindings/property names like

nvrom-cell = <&abc>, <&edf>
nvrom-cell-names = "cell1", "cell2";

Also we can have name associated to each eeprom cell which would help 
for non-dt cases. So they can just lookup by the cell name.


Sacha, Are you ok with such binding?  As this can provide a single 
interface for dt and non-dt. I remember you requested for changing from 
generic properties to specific property names.


> Also the global namespace is sort of worrying (qfprom0 part). How
> do we know it's qfprom0? What if it's qfprom1 sometimes?

I agree this is something which I don't like it in the first place too.
If we have something like names associated to each eeprom cell like clks
or regulators we can have some thing like eeprom_get_cell(dev, name);

>
> Also, how are we supposed to encode certain bits inside the
> stream of bytes (blocks)? In some situations (e.g. the qcom CPR
> stuff) we have quite a few fuse bits we need to read that are
> packed into different bytes and may cross byte boundaries (i.e.
> bits 6 to 10 of a 32-bit word). The current API looks to be byte
> focused when I have bit based/non-byte aligned data.
Yes, it is more of byte oriented. However we can add some new apis for
parsers like ones you are working on.

of_eeprom_get_provider_regmap(phandle) just to get handle to regmap from 
eeprom_core, which would provide most of the apis you would need.
Am guessing eeprom parsers would need need such interface to eeprom-core 
in future anyway.

>
> My current feeling is that I don't want to use any of the block
> stuff at all. I just want a simple byte-based API that allows me
> to read a number of contiguous bytes from the fuses. Then I'll
> need to shift that data down by a few bits and mask it with the
> width of the data I want to read to extract the data needed.
>
> The only thing after that where I have a problem is figuring out
> which eeprom is present in the consumer driver. I think I may
> need to open-code it and look at the phandle for the eeprom and
> do a compatible check to figure out which bits I want to read.
>
> The DT would be like this (note I left eeprom-cells/eeproms here
> because that allows us to generically find eeproms used by a
> device and find eeprom providers):

I though, having "eeprom-cells" would be make sense only if the bindings 
have possible arguments to a phandle. In this case it would be none all 
the time.

For provider lookups currently its generic/easy and fast as its done 
based on eeprom class. Am not sure what advantage would we get
Am I missing anything ?

If you are ok I will prepare a v5 with the proposed changes.


--srini
>
> 	qfprom: eeprom@58000 {
> 		compatible = "qcom,qfprom-msm8916";
> 		reg = <0x58000 0x7000>;
> 		#eeprom-cells = <0>;
> 	};
> 	
> 	cpr@b018000 {
> 		compatible = "qcom,cpr";
> 		reg = <0xb018000 0x1000>;
> 		interrupts = <0 15 1>, <0 16 1>, <0 17 1>;
> 		eeproms = <&qfprom>;
> 		...
> 	};
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html