Hi,
the purpose of this series is to improve ina226 identification. On systems
like Xilinx ZynqMP zcu102 (check dt in the tree) you can find out almost 20
ina226 chips and it is impossible to find out based on simple ina226 which
chip is repsonsible for what.
The series is adding two ways how to improve device identification.
Please let me know what you think I am happy to do changes in it.
Thanks,
Michal
Michal Simek (4):
iio: adc: ina2xx: Define *device_node only once
iio: adc: ina2xx: Setup better name then simple ina2xx
dt-bindings: Add optional label property for ina2xx
iio: adc: ina2xx: Use label proper for device identification
.../devicetree/bindings/hwmon/ina2xx.txt | 2 ++
drivers/iio/adc/ina2xx-adc.c | 15 +++++++++++----
2 files changed, 13 insertions(+), 4 deletions(-)
--
2.17.1
Using optional "label" property is adding an option to user to use better
name for device identification.
Signed-off-by: Michal Simek <[email protected]>
---
Documentation/devicetree/bindings/hwmon/ina2xx.txt | 2 ++
1 file changed, 2 insertions(+)
diff --git a/Documentation/devicetree/bindings/hwmon/ina2xx.txt b/Documentation/devicetree/bindings/hwmon/ina2xx.txt
index 02af0d94e921..d15bf7e46fd7 100644
--- a/Documentation/devicetree/bindings/hwmon/ina2xx.txt
+++ b/Documentation/devicetree/bindings/hwmon/ina2xx.txt
@@ -14,6 +14,8 @@ Optional properties:
- shunt-resistor
Shunt resistor value in micro-Ohm
+- label
+ Symbolic name for a power monitor
Example:
--
2.17.1
Add support for using label property for easier device identification via
iio framework.
Signed-off-by: Michal Simek <[email protected]>
---
drivers/iio/adc/ina2xx-adc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
index 7c7c63677bf4..077c54915f70 100644
--- a/drivers/iio/adc/ina2xx-adc.c
+++ b/drivers/iio/adc/ina2xx-adc.c
@@ -1033,7 +1033,7 @@ static int ina2xx_probe(struct i2c_client *client,
snprintf(chip->name, sizeof(chip->name), "%s-%s",
client->name, dev_name(&client->dev));
- indio_dev->name = chip->name;
+ indio_dev->name = of_get_property(np, "label", NULL) ? : chip->name;
indio_dev->setup_ops = &ina2xx_setup_ops;
buffer = devm_iio_kfifo_allocate(&indio_dev->dev);
--
2.17.1
On systems with multiple ina2xx chips it is impossible to find out which
iio device is which one based on probe order. That's why it is necessary to
setup better name based on possition.
The patch is reusing dev_name which is setup by core with client->name.
name char array was setup to 128 byte length to correspond the same name
length by HID device.
Before this patch:
iio:device9: ina226 (buffer capable)
After:
iio:device9: ina226-3-004a (buffer capable)
Signed-off-by: Michal Simek <[email protected]>
---
Also id->name can be used as prefix. On ina226 output is the same.
Also I am happy to change that space for name will be dynamicky allocated
to save a space if needed.
---
drivers/iio/adc/ina2xx-adc.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
index 37058d9c2054..7c7c63677bf4 100644
--- a/drivers/iio/adc/ina2xx-adc.c
+++ b/drivers/iio/adc/ina2xx-adc.c
@@ -146,6 +146,7 @@ struct ina2xx_chip_info {
int range_vbus; /* Bus voltage maximum in V */
int pga_gain_vshunt; /* Shunt voltage PGA gain */
bool allow_async_readout;
+ char name[128];
};
static const struct ina2xx_config ina2xx_config[] = {
@@ -1027,7 +1028,12 @@ static int ina2xx_probe(struct i2c_client *client,
indio_dev->num_channels = ARRAY_SIZE(ina219_channels);
indio_dev->info = &ina219_info;
}
- indio_dev->name = id->name;
+
+ /* Compose chip name to unified i2c format */
+ snprintf(chip->name, sizeof(chip->name), "%s-%s",
+ client->name, dev_name(&client->dev));
+
+ indio_dev->name = chip->name;
indio_dev->setup_ops = &ina2xx_setup_ops;
buffer = devm_iio_kfifo_allocate(&indio_dev->dev);
--
2.17.1
There is no reason to c&p full client->dev.of_node link when simple
variable can keep it.
Signed-off-by: Michal Simek <[email protected]>
---
drivers/iio/adc/ina2xx-adc.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
index bdd7cba6f6b0..37058d9c2054 100644
--- a/drivers/iio/adc/ina2xx-adc.c
+++ b/drivers/iio/adc/ina2xx-adc.c
@@ -951,6 +951,7 @@ static int ina2xx_probe(struct i2c_client *client,
struct ina2xx_chip_info *chip;
struct iio_dev *indio_dev;
struct iio_buffer *buffer;
+ struct device_node *np = client->dev.of_node;
unsigned int val;
enum ina2xx_ids type;
int ret;
@@ -970,7 +971,7 @@ static int ina2xx_probe(struct i2c_client *client,
return PTR_ERR(chip->regmap);
}
- if (client->dev.of_node)
+ if (np)
type = (enum ina2xx_ids)of_device_get_match_data(&client->dev);
else
type = id->driver_data;
@@ -978,7 +979,7 @@ static int ina2xx_probe(struct i2c_client *client,
mutex_init(&chip->state_lock);
- if (of_property_read_u32(client->dev.of_node,
+ if (of_property_read_u32(np,
"shunt-resistor", &val) < 0) {
struct ina2xx_platform_data *pdata =
dev_get_platdata(&client->dev);
@@ -1016,7 +1017,7 @@ static int ina2xx_probe(struct i2c_client *client,
indio_dev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_SOFTWARE;
indio_dev->dev.parent = &client->dev;
- indio_dev->dev.of_node = client->dev.of_node;
+ indio_dev->dev.of_node = np;
if (id->driver_data == ina226) {
indio_dev->channels = ina226_channels;
indio_dev->num_channels = ARRAY_SIZE(ina226_channels);
--
2.17.1
On 20/08/2019 22:11, Michal Simek wrote:
> On systems with multiple ina2xx chips it is impossible to find out which
> iio device is which one based on probe order. That's why it is necessary to
> setup better name based on possition.
> The patch is reusing dev_name which is setup by core with client->name.
>
> name char array was setup to 128 byte length to correspond the same name
> length by HID device.
>
> Before this patch:
> iio:device9: ina226 (buffer capable)
> After:
> iio:device9: ina226-3-004a (buffer capable)
Could this break existing user space code that's just looking for just ina226.
I2c bus numbers aren't all that great at id'ing devices either. It's better than
nothing but depending on what cards we have plugged into our system the same device gets
a different bus number.
>
> Signed-off-by: Michal Simek <[email protected]>
> ---
>
> Also id->name can be used as prefix. On ina226 output is the same.
>
> Also I am happy to change that space for name will be dynamicky allocated
> to save a space if needed.
> ---
> drivers/iio/adc/ina2xx-adc.c | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
> index 37058d9c2054..7c7c63677bf4 100644
> --- a/drivers/iio/adc/ina2xx-adc.c
> +++ b/drivers/iio/adc/ina2xx-adc.c
> @@ -146,6 +146,7 @@ struct ina2xx_chip_info {
> int range_vbus; /* Bus voltage maximum in V */
> int pga_gain_vshunt; /* Shunt voltage PGA gain */
> bool allow_async_readout;
> + char name[128];
> };
>
> static const struct ina2xx_config ina2xx_config[] = {
> @@ -1027,7 +1028,12 @@ static int ina2xx_probe(struct i2c_client *client,
> indio_dev->num_channels = ARRAY_SIZE(ina219_channels);
> indio_dev->info = &ina219_info;
> }
> - indio_dev->name = id->name;
> +
> + /* Compose chip name to unified i2c format */
> + snprintf(chip->name, sizeof(chip->name), "%s-%s",
> + client->name, dev_name(&client->dev));
> +
> + indio_dev->name = chip->name;
> indio_dev->setup_ops = &ina2xx_setup_ops;
>
> buffer = devm_iio_kfifo_allocate(&indio_dev->dev);
>
--
Regards
Phil Reid
On 20/08/2019 22:11, Michal Simek wrote:
> Add support for using label property for easier device identification via
> iio framework.
>
> Signed-off-by: Michal Simek <[email protected]>
> ---
>
> drivers/iio/adc/ina2xx-adc.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
> index 7c7c63677bf4..077c54915f70 100644
> --- a/drivers/iio/adc/ina2xx-adc.c
> +++ b/drivers/iio/adc/ina2xx-adc.c
> @@ -1033,7 +1033,7 @@ static int ina2xx_probe(struct i2c_client *client,
> snprintf(chip->name, sizeof(chip->name), "%s-%s",
> client->name, dev_name(&client->dev));
>
> - indio_dev->name = chip->name;
> + indio_dev->name = of_get_property(np, "label", NULL) ? : chip->name;
> indio_dev->setup_ops = &ina2xx_setup_ops;
>
> buffer = devm_iio_kfifo_allocate(&indio_dev->dev);
>
I like this personally. It'd be nice if it was a core function so
it could be an opt in to any iio device.
Don't know how well received that'd be thou.
--
Regards
Phil Reid
On 20/08/2019 22:11, Michal Simek wrote:
> There is no reason to c&p full client->dev.of_node link when simple
> variable can keep it.
One comment
>
> Signed-off-by: Michal Simek <[email protected]>
> ---
>
> drivers/iio/adc/ina2xx-adc.c | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
> index bdd7cba6f6b0..37058d9c2054 100644
> --- a/drivers/iio/adc/ina2xx-adc.c
> +++ b/drivers/iio/adc/ina2xx-adc.c
> @@ -951,6 +951,7 @@ static int ina2xx_probe(struct i2c_client *client,
> struct ina2xx_chip_info *chip;
> struct iio_dev *indio_dev;
> struct iio_buffer *buffer;
> + struct device_node *np = client->dev.of_node;
> unsigned int val;
> enum ina2xx_ids type;
> int ret;
> @@ -970,7 +971,7 @@ static int ina2xx_probe(struct i2c_client *client,
> return PTR_ERR(chip->regmap);
> }
>
> - if (client->dev.of_node)
> + if (np)
> type = (enum ina2xx_ids)of_device_get_match_data(&client->dev);
> else
> type = id->driver_data;
> @@ -978,7 +979,7 @@ static int ina2xx_probe(struct i2c_client *client,
>
> mutex_init(&chip->state_lock);
>
> - if (of_property_read_u32(client->dev.of_node,
> + if (of_property_read_u32(np,
> "shunt-resistor", &val) < 0) {
This will fit on one line <80 now.
> struct ina2xx_platform_data *pdata =
> dev_get_platdata(&client->dev);
> @@ -1016,7 +1017,7 @@ static int ina2xx_probe(struct i2c_client *client,
>
> indio_dev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_SOFTWARE;
> indio_dev->dev.parent = &client->dev;
> - indio_dev->dev.of_node = client->dev.of_node;
> + indio_dev->dev.of_node = np;
> if (id->driver_data == ina226) {
> indio_dev->channels = ina226_channels;
> indio_dev->num_channels = ARRAY_SIZE(ina226_channels);
>
--
Regards
Phil Reid
ElectroMagnetic Imaging Technology Pty Ltd
Development of Geophysical Instrumentation & Software
http://www.electromag.com.au
3 The Avenue, Midland WA 6056, AUSTRALIA
Ph: +61 8 9250 8100
Fax: +61 8 9250 7100
Email: [email protected]
On 21. 08. 19 4:10, Phil Reid wrote:
> On 20/08/2019 22:11, Michal Simek wrote:
>> On systems with multiple ina2xx chips it is impossible to find out which
>> iio device is which one based on probe order. That's why it is
>> necessary to
>> setup better name based on possition.
>> The patch is reusing dev_name which is setup by core with client->name.
>>
>> name char array was setup to 128 byte length to correspond the same name
>> length by HID device.
>>
>> Before this patch:
>> iio:device9: ina226 (buffer capable)
>> After:
>> iio:device9: ina226-3-004a (buffer capable)
>
> Could this break existing user space code that's just looking for just
> ina226.
> I2c bus numbers aren't all that great at id'ing devices either. It's
> better than
> nothing but depending on what cards we have plugged into our system the
> same device gets
> a different bus number.
Fair point. Let's wait what dt guys say about label property.
I can simply drop this patch and use just this.
indio_dev->name = of_get_property(np, "label", NULL) ? : id->name;
Thanks,
Michal
On 21. 08. 19 4:11, Phil Reid wrote:
> On 20/08/2019 22:11, Michal Simek wrote:
>> Add support for using label property for easier device identification via
>> iio framework.
>>
>> Signed-off-by: Michal Simek <[email protected]>
>> ---
>>
>> drivers/iio/adc/ina2xx-adc.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
>> index 7c7c63677bf4..077c54915f70 100644
>> --- a/drivers/iio/adc/ina2xx-adc.c
>> +++ b/drivers/iio/adc/ina2xx-adc.c
>> @@ -1033,7 +1033,7 @@ static int ina2xx_probe(struct i2c_client *client,
>> snprintf(chip->name, sizeof(chip->name), "%s-%s",
>> client->name, dev_name(&client->dev));
>> - indio_dev->name = chip->name;
>> + indio_dev->name = of_get_property(np, "label", NULL) ? : chip->name;
>> indio_dev->setup_ops = &ina2xx_setup_ops;
>> buffer = devm_iio_kfifo_allocate(&indio_dev->dev);
>>
> I like this personally. It'd be nice if it was a core function so
> it could be an opt in to any iio device.
>
> Don't know how well received that'd be thou.
>
Something like this?
diff --git a/drivers/iio/industrialio-core.c
b/drivers/iio/industrialio-core.c
index 524a686077ca..d21b495d36a1 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -1647,6 +1647,9 @@ int __iio_device_register(struct iio_dev
*indio_dev, struct module *this_mod)
if (!indio_dev->dev.of_node && indio_dev->dev.parent)
indio_dev->dev.of_node = indio_dev->dev.parent->of_node;
+ indio_dev->name = of_get_property(indio_dev->dev.of_node,
"label", NULL) ? :
+ indio_dev->name;
+
ret = iio_check_unique_scan_index(indio_dev);
if (ret < 0)
return ret;
M
On Wed, 21 Aug 2019 11:12:00 +0200
Michal Simek <[email protected]> wrote:
> On 21. 08. 19 4:11, Phil Reid wrote:
> > On 20/08/2019 22:11, Michal Simek wrote:
> >> Add support for using label property for easier device identification via
> >> iio framework.
> >>
> >> Signed-off-by: Michal Simek <[email protected]>
> >> ---
> >>
> >> drivers/iio/adc/ina2xx-adc.c | 2 +-
> >> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
> >> index 7c7c63677bf4..077c54915f70 100644
> >> --- a/drivers/iio/adc/ina2xx-adc.c
> >> +++ b/drivers/iio/adc/ina2xx-adc.c
> >> @@ -1033,7 +1033,7 @@ static int ina2xx_probe(struct i2c_client *client,
> >> snprintf(chip->name, sizeof(chip->name), "%s-%s",
> >> client->name, dev_name(&client->dev));
> >> - indio_dev->name = chip->name;
> >> + indio_dev->name = of_get_property(np, "label", NULL) ? : chip->name;
> >> indio_dev->setup_ops = &ina2xx_setup_ops;
> >> buffer = devm_iio_kfifo_allocate(&indio_dev->dev);
> >>
> > I like this personally. It'd be nice if it was a core function so
> > it could be an opt in to any iio device.
> >
> > Don't know how well received that'd be thou.
> >
I'm not particularly keen on changing the semantics of existing
ABI, but how about adding new ABI to provide this?
/sys/bus/iio/devices/iio\:device0/label for example?
I haven't thought about it in depth yet though. If you spin
a patch with that and the DT docs we'll be more likely to get
a view from DT maintainers if this is acceptable use of label.
Thanks
Jonathan
>
> Something like this?
>
> diff --git a/drivers/iio/industrialio-core.c
> b/drivers/iio/industrialio-core.c
> index 524a686077ca..d21b495d36a1 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -1647,6 +1647,9 @@ int __iio_device_register(struct iio_dev
> *indio_dev, struct module *this_mod)
> if (!indio_dev->dev.of_node && indio_dev->dev.parent)
> indio_dev->dev.of_node = indio_dev->dev.parent->of_node;
>
> + indio_dev->name = of_get_property(indio_dev->dev.of_node,
> "label", NULL) ? :
> + indio_dev->name;
> +
> ret = iio_check_unique_scan_index(indio_dev);
> if (ret < 0)
> return ret;
>
>
> M
On 26/08/2019 02:07, Jonathan Cameron wrote:
> On Wed, 21 Aug 2019 11:12:00 +0200
> Michal Simek <[email protected]> wrote:
>
>> On 21. 08. 19 4:11, Phil Reid wrote:
>>> On 20/08/2019 22:11, Michal Simek wrote:
>>>> Add support for using label property for easier device identification via
>>>> iio framework.
>>>>
>>>> Signed-off-by: Michal Simek <[email protected]>
>>>> ---
>>>>
>>>> drivers/iio/adc/ina2xx-adc.c | 2 +-
>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
>>>> index 7c7c63677bf4..077c54915f70 100644
>>>> --- a/drivers/iio/adc/ina2xx-adc.c
>>>> +++ b/drivers/iio/adc/ina2xx-adc.c
>>>> @@ -1033,7 +1033,7 @@ static int ina2xx_probe(struct i2c_client *client,
>>>> snprintf(chip->name, sizeof(chip->name), "%s-%s",
>>>> client->name, dev_name(&client->dev));
>>>> - indio_dev->name = chip->name;
>>>> + indio_dev->name = of_get_property(np, "label", NULL) ? : chip->name;
>>>> indio_dev->setup_ops = &ina2xx_setup_ops;
>>>> buffer = devm_iio_kfifo_allocate(&indio_dev->dev);
>>>>
>>> I like this personally. It'd be nice if it was a core function so
>>> it could be an opt in to any iio device.
>>>
>>> Don't know how well received that'd be thou.
>>>
> I'm not particularly keen on changing the semantics of existing
> ABI, but how about adding new ABI to provide this?
>
> /sys/bus/iio/devices/iio\:device0/label for example?
>
> I haven't thought about it in depth yet though. If you spin
> a patch with that and the DT docs we'll be more likely to get
> a view from DT maintainers if this is acceptable use of label.
>
I've sent "iio: core: Add optional symbolic label to device attributes"
for further discussion.
> Thanks
>
> Jonathan
>
>>
>> Something like this?
>>
>> diff --git a/drivers/iio/industrialio-core.c
>> b/drivers/iio/industrialio-core.c
>> index 524a686077ca..d21b495d36a1 100644
>> --- a/drivers/iio/industrialio-core.c
>> +++ b/drivers/iio/industrialio-core.c
>> @@ -1647,6 +1647,9 @@ int __iio_device_register(struct iio_dev
>> *indio_dev, struct module *this_mod)
>> if (!indio_dev->dev.of_node && indio_dev->dev.parent)
>> indio_dev->dev.of_node = indio_dev->dev.parent->of_node;
>>
>> + indio_dev->name = of_get_property(indio_dev->dev.of_node,
>> "label", NULL) ? :
>> + indio_dev->name;
>> +
>> ret = iio_check_unique_scan_index(indio_dev);
>> if (ret < 0)
>> return ret;
>>
>>
>> M
>
>
>
--
Regards
Phil Reid
ElectroMagnetic Imaging Technology Pty Ltd
Development of Geophysical Instrumentation & Software
http://www.electromag.com.au
3 The Avenue, Midland WA 6056, AUSTRALIA
Ph: +61 8 9250 8100
Fax: +61 8 9250 7100
Email: [email protected]
On 27. 08. 19 5:55, Phil Reid wrote:
> On 26/08/2019 02:07, Jonathan Cameron wrote:
>> On Wed, 21 Aug 2019 11:12:00 +0200
>> Michal Simek <[email protected]> wrote:
>>
>>> On 21. 08. 19 4:11, Phil Reid wrote:
>>>> On 20/08/2019 22:11, Michal Simek wrote:
>>>>> Add support for using label property for easier device
>>>>> identification via
>>>>> iio framework.
>>>>>
>>>>> Signed-off-by: Michal Simek <[email protected]>
>>>>> ---
>>>>>
>>>>> drivers/iio/adc/ina2xx-adc.c | 2 +-
>>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/iio/adc/ina2xx-adc.c
>>>>> b/drivers/iio/adc/ina2xx-adc.c
>>>>> index 7c7c63677bf4..077c54915f70 100644
>>>>> --- a/drivers/iio/adc/ina2xx-adc.c
>>>>> +++ b/drivers/iio/adc/ina2xx-adc.c
>>>>> @@ -1033,7 +1033,7 @@ static int ina2xx_probe(struct i2c_client
>>>>> *client,
>>>>> snprintf(chip->name, sizeof(chip->name), "%s-%s",
>>>>> client->name, dev_name(&client->dev));
>>>>> - indio_dev->name = chip->name;
>>>>> + indio_dev->name = of_get_property(np, "label", NULL) ? :
>>>>> chip->name;
>>>>> indio_dev->setup_ops = &ina2xx_setup_ops;
>>>>> buffer = devm_iio_kfifo_allocate(&indio_dev->dev);
>>>>>
>>>> I like this personally. It'd be nice if it was a core function so
>>>> it could be an opt in to any iio device.
>>>>
>>>> Don't know how well received that'd be thou.
>>>>
>> I'm not particularly keen on changing the semantics of existing
>> ABI, but how about adding new ABI to provide this?
>>
>> /sys/bus/iio/devices/iio\:device0/label for example?
>>
>> I haven't thought about it in depth yet though. If you spin
>> a patch with that and the DT docs we'll be more likely to get
>> a view from DT maintainers if this is acceptable use of label.
>>
>
> I've sent "iio: core: Add optional symbolic label to device attributes"
> for further discussion.
Series looks good. I expect that there will be also update in libiio if
this is accepted.
Thanks,
Michal
On Tue, 20 Aug 2019 16:11:40 +0200, Michal Simek wrote:
> Using optional "label" property is adding an option to user to use better
> name for device identification.
>
> Signed-off-by: Michal Simek <[email protected]>
> ---
>
> Documentation/devicetree/bindings/hwmon/ina2xx.txt | 2 ++
> 1 file changed, 2 insertions(+)
>
Reviewed-by: Rob Herring <[email protected]>