lkml.org 
[lkml]   [2015]   [Feb]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [RFC PATCH 1/3] eeprom: Add a simple EEPROM framework
Thanks Andrew for your comments,

On 19/02/15 18:12, Andrew Lunn wrote:
>> +
>> +Required properties:
>> +
>> +eeproms: List of phandle and data cell specifier triplet, one triplet
>> + for each data cell the device might be interested in. The
>> + triplet consists of the phandle to the eeprom provider, then
>> + the offset in byte within that storage device, and the length
>
> bytes
>
>> + in byte of the data we care about.
>
> bytes
Yep will fix it in next version.
>
>> +
>> +Optional properties:
>> +
>> +eeprom-names: List of data cell name strings sorted in the same order
>> + as the resets property. Consumers drivers will use
>
> resets? I guess this text was cut/paste from the reset documentation?\
>
I think so. Will fix it.
>> + eeprom-names to differentiate between multiple cells,
>> + and hence being able to know what these cells are for.
>> +
>> +For example:
>> +
>> + device {
>> + eeproms = <&at24 14 42>;
>
> I like to use 42, but is it realistic to have a soc-rev-id which is 42
> bytes long? How about using 42 as the offset and a sensible length of
> say 4?
Ok, will fix it..
>
>> + eeprom-names = "soc-rev-id";
>> +menuconfig EEPROM
>> + bool "EEPROM Support"
>> + depends on OF
>> + help
>> + Support for EEPROM alike devices.
>
> like.
Ok
>
>> +
>> + This framework is designed to provide a generic interface to EEPROM
>
> EPROMs
Ok.
>
>> +
>> +
>> +static ssize_t bin_attr_eeprom_read_write(struct kobject *kobj,
>> + char *buf, loff_t offset,
>> + size_t count, bool read)
>> +{
>> + struct device *dev = container_of(kobj, struct device, kobj);
>> + struct eeprom_device *eeprom = container_of(dev, struct eeprom_device,
>> + edev);
>> + int rc;
>> +
>> + if (offset > eeprom->size)
>> + return 0;
>> +
>> + if (offset + count > eeprom->size)
>> + count = eeprom->size - offset;
>> +
>> + if (read)
>> + rc = regmap_bulk_read(eeprom->regmap, offset,
>> + buf, count/eeprom->stride);
>
> This division will round down, so you could get one less byte than
> what you expected, and the value you actually return. It seems like
> there should be a check here, the count is a multiple of stride and
> return an error if it is not.
Thats a good catch, I will fix this for other such instances too.
>
>> + else
>> + rc = regmap_bulk_write(eeprom->regmap, offset,
>> + buf, count/eeprom->stride);
>> +
>> + if (IS_ERR_VALUE(rc))
>> + return 0;
>> +
>
> I don't think returning 0 here, and above is the best thing to
> do. Return the real error code from regmap, or EINVAL or some other
> error code for going off the end of the eerpom.
Ok, I will fix the return value here for both the cases.

>
>> + return count;
>> +}
>> +
>> +static ssize_t bin_attr_eeprom_read(struct file *filp, struct kobject *kobj,
>> + struct bin_attribute *attr,
>> + char *buf, loff_t offset, size_t count)
>> +{
>> + return bin_attr_eeprom_read_write(kobj, buf, offset, count, true);
>> +}
>> +
>> +static ssize_t bin_attr_eeprom_write(struct file *filp, struct kobject *kobj,
>> + struct bin_attribute *attr,
>> + char *buf, loff_t offset, size_t count)
>> +{
>> + return bin_attr_eeprom_read_write(kobj, buf, offset, count, false);
>> +}
>>
>
> These two functions seem to be identical. So just have one of them?

One is read and other is write.. there is a true and false flag at the
end of the call to bin_attr_eeprom_read_write().
>
> +
>> +static struct bin_attribute bin_attr_eeprom = {
>> + .attr = {
>> + .name = "eeprom",
>> + .mode = 0660,
>
> Symbolic values like S_IRUGO | S_IWUSR would be better.
Yep, thats correct, I will fix it.
>
> Are you also sure you want group write?
>
S_IWUSR should be enough.

>> + },
>> + .read = bin_attr_eeprom_read,
>> + .write = bin_attr_eeprom_write,
>> +};
>> +
>> +static struct eeprom_cell *__eeprom_cell_get(struct device_node *node,
>> + int index)
>> +{
>> + struct of_phandle_args args;
>> + struct eeprom_cell *cell;
>> + struct eeprom_device *e, *eeprom = NULL;
>> + int ret;
>> +
>> + ret = of_parse_phandle_with_args(node, "eeproms",
>> + "#eeprom-cells", index, &args);
>> + if (ret)
>> + return ERR_PTR(ret);
>> +
>> + if (args.args_count != 2)
>> + return ERR_PTR(-EINVAL);
>> +
>> + mutex_lock(&eeprom_list_mutex);
>> +
>> + list_for_each_entry(e, &eeprom_list, list) {
>> + if (args.np == e->edev.of_node) {
>> + eeprom = e;
>> + break;
>> + }
>> + }
>> + mutex_unlock(&eeprom_list_mutex);
>
> Shouldn't you increment a reference count to the eeprom here? You are
> going to have trouble if the eeprom is unregistered and there is a
> call still referring to it.
Yes, Stephen Byod also pointed the same, having owner in eeprom_device
should fix this.
I will fix it in next version.

>
>> +
>> + if (!eeprom)
>> + return ERR_PTR(-EPROBE_DEFER);
>> +
>> + cell = kzalloc(sizeof(*cell), GFP_KERNEL);
>> + if (!cell)
>> + return ERR_PTR(-ENOMEM);
>> +
>> + cell->eeprom = eeprom;
>> + cell->offset = args.args[0];
>> + cell->count = args.args[1];
>> +
>> + return cell;
>> +}
>> +
>> +
>> diff --git a/include/linux/eeprom-consumer.h b/include/linux/eeprom-consumer.h
>> new file mode 100644
>> index 0000000..706ae9d
>> --- /dev/null
>> +++ b/include/linux/eeprom-consumer.h
>> @@ -0,0 +1,73 @@
>> +/*
>> + * EEPROM framework consumer.
>> + *
>> + * Copyright (C) 2015 Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>> + * Copyright (C) 2013 Maxime Ripard <maxime.ripard@free-electrons.com>
>> + *
>> + * This file is licensed under the terms of the GNU General Public
>> + * License version 2. This program is licensed "as is" without any
>> + * warranty of any kind, whether express or implied.
>> + */
>> +
>> +#ifndef _LINUX_EEPROM_CONSUMER_H
>> +#define _LINUX_EEPROM_CONSUMER_H
>> +
>> +struct eeprom_cell;
>> +
>> +/**
>> + * eeprom_cell_get(): Get eeprom cell of device form a given index.
>
> of a device for a
>
Ok, will be fixed in next version.
>> + *
>> + * @dev: Device that will be interacted with
>> + * @index: Index of the eeprom cell.
>> + *
>> + * 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(struct device *dev, int index);
>> +
>> +/**
>> + * eeprom_cell_get(): Get eeprom cell of device form a given name.
>
> same again
Ok, will be fixed in next version.
>
>> + *
>> + * @dev: Device that will be interacted with
>> + * @name: Name of the eeprom cell.
>> + *
>> + * 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_byname(struct device *dev,
>> + const char *name);
>> +
>> +/**
>> + * eeprom_cell_put(): Release previously allocated eeprom cell.
>> + *
>> + * @cell: Previously allocated eeprom cell by eeprom_cell_get()
>> + * or eeprom_cell_get_byname().
>> + */
>> +void eeprom_cell_put(struct eeprom_cell *cell);
>> +
>> +/**
>> + * eeprom_cell_read(): Read a given eeprom cell
>> + *
>> + * @cell: eeprom cell to be read.
>> + * @len: pointer to length of cell which will be populated on successful read.
>> + *
>> + * The return value will be an ERR_PTR() on error or a valid pointer
>> + * to a char * bufffer. The buffer should be freed by the consumer with a
>> + * kfree().
>> + */
>> +char *eeprom_cell_read(struct eeprom_cell *cell, ssize_t *len);
>
> Would void * be better than char *? I guess the contents is mostly
> data, not strings.
Yes, thats sounds sensible.
>
> Andrew
>
>> +
>> +/**

>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel


\
 
 \ /
  Last update: 2015-02-20 09:41    [W:0.337 / U:0.284 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site