The current code expected that every LED had an unique name. This is a
legit expectation when the device tree can no be modified or extended.
But with device tree overlays this requirement can be easily broken.
This patch finds out if the name is already in use and adds the suffix
_1, _2... if not.
Signed-off-by: Ricardo Ribalda Delgado <[email protected]>
---
drivers/leds/led-class.c | 30 ++++++++++++++++++++++++++++--
1 file changed, 28 insertions(+), 2 deletions(-)
diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
index dbeebac..b60f942 100644
--- a/drivers/leds/led-class.c
+++ b/drivers/leds/led-class.c
@@ -208,6 +208,13 @@ static const struct dev_pm_ops leds_class_dev_pm_ops = {
.resume = led_resume,
};
+static int match_child(struct device *dev, void *data)
+{
+ if (!dev_name(dev))
+ return 0;
+ return !strcmp(dev_name(dev), (char *)data);
+}
+
/**
* led_classdev_register - register a new object of led_classdev class.
* @parent: The device to register.
@@ -215,9 +222,28 @@ static const struct dev_pm_ops leds_class_dev_pm_ops = {
*/
int led_classdev_register(struct device *parent, struct led_classdev *led_cdev)
{
+ char *name;
+ char *temp_name = NULL;
+ int i = 0;
+
+ do {
+ if (i == 0)
+ name = (char *)led_cdev->name;
+ else {
+ kfree(temp_name);
+ temp_name = kasprintf(GFP_KERNEL, "%s_%d",
+ led_cdev->name, i);
+ if (!temp_name)
+ return -ENOMEM;
+ name = temp_name;
+ }
+
+ i++;
+ } while (device_find_child(parent, name, match_child));
+
led_cdev->dev = device_create_with_groups(leds_class, parent, 0,
- led_cdev, led_cdev->groups,
- "%s", led_cdev->name);
+ led_cdev, led_cdev->groups, name);
+ kfree(temp_name);
if (IS_ERR(led_cdev->dev))
return PTR_ERR(led_cdev->dev);
--
2.1.4
On Wed, Feb 11, 2015 at 4:26 AM, Ricardo Ribalda Delgado
<[email protected]> wrote:
> The current code expected that every LED had an unique name. This is a
> legit expectation when the device tree can no be modified or extended.
> But with device tree overlays this requirement can be easily broken.
>
Could you please give me some real error message or example about this
device tree overlays? Looks like it's not only for LED subsystem and
how is it handled in other subsystem?
> This patch finds out if the name is already in use and adds the suffix
> _1, _2... if not.
>
> Signed-off-by: Ricardo Ribalda Delgado <[email protected]>
> ---
> drivers/leds/led-class.c | 30 ++++++++++++++++++++++++++++--
> 1 file changed, 28 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
> index dbeebac..b60f942 100644
> --- a/drivers/leds/led-class.c
> +++ b/drivers/leds/led-class.c
> @@ -208,6 +208,13 @@ static const struct dev_pm_ops leds_class_dev_pm_ops = {
> .resume = led_resume,
> };
>
> +static int match_child(struct device *dev, void *data)
> +{
> + if (!dev_name(dev))
> + return 0;
> + return !strcmp(dev_name(dev), (char *)data);
strncmp instead of strcmp?
> +}
> +
> /**
> * led_classdev_register - register a new object of led_classdev class.
> * @parent: The device to register.
> @@ -215,9 +222,28 @@ static const struct dev_pm_ops leds_class_dev_pm_ops = {
> */
> int led_classdev_register(struct device *parent, struct led_classdev *led_cdev)
> {
> + char *name;
> + char *temp_name = NULL;
> + int i = 0;
> +
> + do {
> + if (i == 0)
> + name = (char *)led_cdev->name;
> + else {
> + kfree(temp_name);
> + temp_name = kasprintf(GFP_KERNEL, "%s_%d",
> + led_cdev->name, i);
> + if (!temp_name)
> + return -ENOMEM;
> + name = temp_name;
> + }
> +
> + i++;
> + } while (device_find_child(parent, name, match_child));
> +
I still think we need solve this issue in device tree core code not in
the user of DT.
> led_cdev->dev = device_create_with_groups(leds_class, parent, 0,
> - led_cdev, led_cdev->groups,
> - "%s", led_cdev->name);
> + led_cdev, led_cdev->groups, name);
> + kfree(temp_name);
> if (IS_ERR(led_cdev->dev))
> return PTR_ERR(led_cdev->dev);
>
> --
> 2.1.4
>
Hello Bryan
On Thu, Feb 12, 2015 at 8:48 PM, Bryan Wu <[email protected]> wrote:
>
> Could you please give me some real error message or example about this
> device tree overlays? Looks like it's not only for LED subsystem and
> how is it handled in other subsystem?
In my case I have a device tree overlay describing a camera. This
camera has 3 leds, named red, green and yellow. When I connect the
second camera, kobject complains about a repeated name. The error is
REALLY verbose. I am out of the office right now, so I cannot show you
the error message. You can trigger the same error by trying to
register two leds with the same name.
A similar error can be triggered for example when we want to create
two misc devices with the same name
[106998.862865] ------------[ cut here ]------------
[106998.862885] WARNING: CPU: 1 PID: 12979 at
/build/linux-CMiYW9/linux-3.16.7-ckt2/fs/sysfs/dir.c:31
sysfs_warn_dup+0x5f/0x70()
[106998.862890] sysfs: cannot create duplicate filename
'/devices/virtual/misc/eudyptula'
[106998.862894] Modules linked in: hello(O+) btrfs xor raid6_pq ufs
qnx4 hfsplus hfs minix ntfs msdos jfs xfs libcrc32c dm_mod bnep
bluetooth 6lowpan_iphc mmc_block tun ctr ccm binfmt_misc nfsd
auth_rpcgss oid_registry nfs_acl nfs lockd fscache sunrpc nls_utf8
nls_cp437 vfat fat x86_pkg_temp_thermal intel_powerclamp intel_rapl
uvcvideo coretemp videobuf2_vmalloc kvm_intel videobuf2_memops
videobuf2_core v4l2_common videodev kvm media joydev crc32_pclmul
ghash_clmulni_intel aesni_intel arc4 aes_x86_64 iwldvm mac80211
iwlwifi cfg80211 lrw gf128mul glue_helper iTCO_wdt iTCO_vendor_support
ablk_helper evdev psmouse i915 snd_hda_codec_hdmi cryptd serio_raw
pcspkr ac shpchp efi_pstore drm_kms_helper snd_hda_codec_conexant
snd_hda_codec_generic lpc_ich thinkpad_acpi nvram tpm_tis efivars drm
i2c_algo_bit i2c_i801
[106998.863005] snd_hda_intel snd_hda_controller mfd_core mei_me
snd_hda_codec i2c_core mei wmi snd_hwdep rfkill snd_pcm tpm battery
snd_timer snd soundcore button video processor fuse parport_pc ppdev
lp parport autofs4 ext4 crc16 mbcache jbd2 sg sd_mod sr_mod crc_t10dif
cdrom crct10dif_generic crct10dif_pclmul crct10dif_common crc32c_intel
ahci libahci libata ehci_pci ehci_hcd scsi_mod sdhci_pci sdhci
mmc_core e1000e usbcore ptp pps_core usb_common thermal thermal_sys
[106998.863077] CPU: 1 PID: 12979 Comm: insmod Tainted: G O
3.16.0-4-amd64 #1 Debian 3.16.7-ckt2-1
[106998.863083] Hardware name: LENOVO 4180DY4/4180DY4, BIOS 83ET76WW
(1.46 ) 07/05/2013
[106998.863087] 0000000000000009 ffffffff81507263 ffff880021757af8
ffffffff81065847
[106998.863096] ffff880065c6d000 ffff880021757b48 ffff880214c1c820
0000000000000000
[106998.863103] 0000000000000000 ffffffff810658ac ffffffff8172ff20
ffffffff00000028
[106998.863111] Call Trace:
[106998.863125] [<ffffffff81507263>] ? dump_stack+0x41/0x51
[106998.863137] [<ffffffff81065847>] ? warn_slowpath_common+0x77/0x90
[106998.863145] [<ffffffff810658ac>] ? warn_slowpath_fmt+0x4c/0x50
[106998.863153] [<ffffffff81212ae3>] ? kernfs_path+0x43/0x50
[106998.863161] [<ffffffff81215f1f>] ? sysfs_warn_dup+0x5f/0x70
[106998.863169] [<ffffffff81215fae>] ? sysfs_create_dir_ns+0x7e/0x90
[106998.863180] [<ffffffff812a9706>] ? kobject_add_internal+0xc6/0x3e0
[106998.863190] [<ffffffff812b0f56>] ? string.isra.7+0x36/0xe0
[106998.863197] [<ffffffff812a9e25>] ? kobject_add+0x65/0xb0
[106998.863204] [<ffffffff812b1e8e>] ? pointer.isra.18+0x9e/0x470
[106998.863213] [<ffffffff8139c584>] ? device_add+0x124/0x610
[106998.863221] [<ffffffff8139cc68>] ? device_create_groups_vargs+0xd8/0x100
[106998.863239] [<ffffffffa0025000>] ? 0xffffffffa0024fff
[106998.863247] [<ffffffff8139ccf1>] ? device_create+0x41/0x50
[106998.863257] [<ffffffff812b621e>] ? kasprintf+0x3e/0x40
[106998.863268] [<ffffffff8138c6a2>] ? misc_register+0xc2/0x120
[106998.863277] [<ffffffffa002500c>] ? hello_init+0xc/0x1000 [hello]
[106998.863285] [<ffffffff8100213c>] ? do_one_initcall+0xcc/0x200
[106998.863295] [<ffffffff810d8d1a>] ? load_module+0x20da/0x26b0
[106998.863303] [<ffffffff810d4900>] ? store_uevent+0x40/0x40
[106998.863312] [<ffffffff810d944d>] ? SyS_finit_module+0x7d/0xa0
[106998.863322] [<ffffffff8150d32d>] ? system_call_fast_compare_end+0x10/0x15
[106998.863327] ---[ end trace af819f702b51bbb0 ]---
[106998.863332] ------------[ cut here ]------------
>> +static int match_child(struct device *dev, void *data)
>> +{
>> + if (!dev_name(dev))
>> + return 0;
>> + return !strcmp(dev_name(dev), (char *)data);
>
> strncmp instead of strcmp?
I dont think it would make it safer here. You need to know the size of
the string, and finding out the lenght of the string will produce the
same access error (if that is what concerns you).
> I still think we need solve this issue in device tree core code not in
> the user of DT.
Other subsystem do not have the issue of unique naming.
ie, in video4linux the device name is created dynamically (video0,
video1.....), others like hwmon have a hierarchical name
(hmon0/input0, hwmon1/input0)
I think this is a problem related to the led subsystem that does not
have a way to handled repeated names. Before we could blame the system
designer for choosing the same name for two leds, but with dt overlays
this is not more in full control of the sys designer. Please correct
me if I am wrong.
Thanks!!!
--
Ricardo Ribalda
Hello Bryan
This is a real error message:
[ 6.335055] ------------[ cut here ]------------
[ 6.335089] WARNING: CPU: 1 PID: 74 at fs/sysfs/dir.c:31
sysfs_warn_dup+0x68/0x80()
[ 6.335100] sysfs: cannot create duplicate filename
'/devices/pci0000:00/0000:00:05.0/0000:01:00.0/b0040000.leds/leds/red'
[ 6.335108] Modules linked in: qt5023_video(+) qtec_testgen(+)
qtec_white(+) qtec_xform(+) videobuf2_dma_sg gpio_xilinx qtec_mem
videobuf2_vmalloc videobuf2_memops videobuf2_core qtec_cmosis(+)
qtec_pcie 8250_fintek qt5023 spi_xilinx spi_bitbang
[ 6.335174] CPU: 1 PID: 74 Comm: kworker/u4:1 Not tainted
3.19.0-qtec-standard+ #161
[ 6.335184] Hardware name: QTechnology QT5022/QT5022, BIOS
PM_2.1.0.309 X64 09/27/2013
[ 6.335200] Workqueue: deferwq deferred_probe_work_func
[ 6.335210] ffffffff81b4d24a ffff8800878fb8c8 ffffffff8183fb8a
ffffffff81c6bff8
[ 6.335225] ffff8800878fb918 ffff8800878fb908 ffffffff8108555a
00000000000068cf
[ 6.335238] ffff880151a29000 ffff880151251488 ffff8800861a5d98
ffff880151a5e800
[ 6.335253] Call Trace:
[ 6.335278] [<ffffffff8183fb8a>] dump_stack+0x4c/0x65
[ 6.335298] [<ffffffff8108555a>] warn_slowpath_common+0x8a/0xc0
[ 6.335314] [<ffffffff810855d6>] warn_slowpath_fmt+0x46/0x50
[ 6.335332] [<ffffffff8120cb38>] ? kernfs_path+0x48/0x60
[ 6.335345] [<ffffffff81210258>] sysfs_warn_dup+0x68/0x80
[ 6.335359] [<ffffffff812102fd>] sysfs_create_dir_ns+0x8d/0xa0
[ 6.335375] [<ffffffff8137a738>] kobject_add_internal+0xb8/0x370
[ 6.335468] [<ffffffff8138226f>] ? string.isra.7+0x3f/0xf0
[ 6.335482] [<ffffffff8137abe3>] kobject_add+0x63/0xb0
[ 6.335499] [<ffffffff81844e59>] ? mutex_lock+0x29/0x50
[ 6.335517] [<ffffffff81452516>] device_add+0xf6/0x5f0
[ 6.335533] [<ffffffff81452c38>] device_create_groups_vargs+0xe8/0x100
[ 6.335547] [<ffffffff81452ce1>] device_create_with_groups+0x31/0x40
[ 6.335566] [<ffffffffa00771ef>] ? xgpio_dir_out+0xbf/0x110 [gpio_xilinx]
[ 6.335583] [<ffffffff813a2f1a>] ? _gpiod_direction_output_raw+0x7a/0x240
[ 6.335599] [<ffffffff81650a30>] led_classdev_register+0x40/0x180
[ 6.335613] [<ffffffff81651ad5>] create_gpio_led+0xe5/0x1a0
[ 6.335627] [<ffffffff81651d5b>] gpio_led_probe+0x1cb/0x390
[ 6.335643] [<ffffffff814579fb>] platform_drv_probe+0x4b/0xc0
[ 6.335656] [<ffffffff81455913>] driver_probe_device+0xa3/0x410
[ 6.335670] [<ffffffff81455c80>] ? driver_probe_device+0x410/0x410
[ 6.335682] [<ffffffff81455cbb>] __device_attach+0x3b/0x40
[ 6.335695] [<ffffffff81453843>] bus_for_each_drv+0x63/0xa0
[ 6.335708] [<ffffffff81455808>] device_attach+0x98/0xb0
[ 6.335721] [<ffffffff81454c20>] bus_probe_device+0xa0/0xc0
[ 6.335734] [<ffffffff81455128>] deferred_probe_work_func+0x38/0xe0
[ 6.335750] [<ffffffff8109cdee>] process_one_work+0x14e/0x410
[ 6.335766] [<ffffffff8109d81b>] worker_thread+0x6b/0x4a0
[ 6.335781] [<ffffffff8109d7b0>] ? init_pwq.part.31+0x10/0x10
[ 6.335795] [<ffffffff810a21bf>] kthread+0xef/0x110
[ 6.335807] [<ffffffff810a0000>] ? alloc_pid+0x150/0x4a0
[ 6.335821] [<ffffffff810a20d0>] ? kthread_create_on_node+0x180/0x180
[ 6.335835] [<ffffffff818473ec>] ret_from_fork+0x7c/0xb0
[ 6.335849] [<ffffffff810a20d0>] ? kthread_create_on_node+0x180/0x180
[ 6.335860] ---[ end trace 4f3ef7818083fc76 ]---
--
Ricardo Ribalda
On Thu, Feb 12, 2015 at 12:45 PM, Ricardo Ribalda Delgado
<[email protected]> wrote:
> Hello Bryan
>
> On Thu, Feb 12, 2015 at 8:48 PM, Bryan Wu <[email protected]> wrote:
>>
>> Could you please give me some real error message or example about this
>> device tree overlays? Looks like it's not only for LED subsystem and
>> how is it handled in other subsystem?
>
> In my case I have a device tree overlay describing a camera. This
> camera has 3 leds, named red, green and yellow. When I connect the
> second camera, kobject complains about a repeated name. The error is
> REALLY verbose. I am out of the office right now, so I cannot show you
> the error message. You can trigger the same error by trying to
> register two leds with the same name.
>
Can you show me your device tree code for overlaying describing a camera?
-Bryan
> A similar error can be triggered for example when we want to create
> two misc devices with the same name
>
> [106998.862865] ------------[ cut here ]------------
> [106998.862885] WARNING: CPU: 1 PID: 12979 at
> /build/linux-CMiYW9/linux-3.16.7-ckt2/fs/sysfs/dir.c:31
> sysfs_warn_dup+0x5f/0x70()
> [106998.862890] sysfs: cannot create duplicate filename
> '/devices/virtual/misc/eudyptula'
> [106998.862894] Modules linked in: hello(O+) btrfs xor raid6_pq ufs
> qnx4 hfsplus hfs minix ntfs msdos jfs xfs libcrc32c dm_mod bnep
> bluetooth 6lowpan_iphc mmc_block tun ctr ccm binfmt_misc nfsd
> auth_rpcgss oid_registry nfs_acl nfs lockd fscache sunrpc nls_utf8
> nls_cp437 vfat fat x86_pkg_temp_thermal intel_powerclamp intel_rapl
> uvcvideo coretemp videobuf2_vmalloc kvm_intel videobuf2_memops
> videobuf2_core v4l2_common videodev kvm media joydev crc32_pclmul
> ghash_clmulni_intel aesni_intel arc4 aes_x86_64 iwldvm mac80211
> iwlwifi cfg80211 lrw gf128mul glue_helper iTCO_wdt iTCO_vendor_support
> ablk_helper evdev psmouse i915 snd_hda_codec_hdmi cryptd serio_raw
> pcspkr ac shpchp efi_pstore drm_kms_helper snd_hda_codec_conexant
> snd_hda_codec_generic lpc_ich thinkpad_acpi nvram tpm_tis efivars drm
> i2c_algo_bit i2c_i801
> [106998.863005] snd_hda_intel snd_hda_controller mfd_core mei_me
> snd_hda_codec i2c_core mei wmi snd_hwdep rfkill snd_pcm tpm battery
> snd_timer snd soundcore button video processor fuse parport_pc ppdev
> lp parport autofs4 ext4 crc16 mbcache jbd2 sg sd_mod sr_mod crc_t10dif
> cdrom crct10dif_generic crct10dif_pclmul crct10dif_common crc32c_intel
> ahci libahci libata ehci_pci ehci_hcd scsi_mod sdhci_pci sdhci
> mmc_core e1000e usbcore ptp pps_core usb_common thermal thermal_sys
> [106998.863077] CPU: 1 PID: 12979 Comm: insmod Tainted: G O
> 3.16.0-4-amd64 #1 Debian 3.16.7-ckt2-1
> [106998.863083] Hardware name: LENOVO 4180DY4/4180DY4, BIOS 83ET76WW
> (1.46 ) 07/05/2013
> [106998.863087] 0000000000000009 ffffffff81507263 ffff880021757af8
> ffffffff81065847
> [106998.863096] ffff880065c6d000 ffff880021757b48 ffff880214c1c820
> 0000000000000000
> [106998.863103] 0000000000000000 ffffffff810658ac ffffffff8172ff20
> ffffffff00000028
> [106998.863111] Call Trace:
> [106998.863125] [<ffffffff81507263>] ? dump_stack+0x41/0x51
> [106998.863137] [<ffffffff81065847>] ? warn_slowpath_common+0x77/0x90
> [106998.863145] [<ffffffff810658ac>] ? warn_slowpath_fmt+0x4c/0x50
> [106998.863153] [<ffffffff81212ae3>] ? kernfs_path+0x43/0x50
> [106998.863161] [<ffffffff81215f1f>] ? sysfs_warn_dup+0x5f/0x70
> [106998.863169] [<ffffffff81215fae>] ? sysfs_create_dir_ns+0x7e/0x90
> [106998.863180] [<ffffffff812a9706>] ? kobject_add_internal+0xc6/0x3e0
> [106998.863190] [<ffffffff812b0f56>] ? string.isra.7+0x36/0xe0
> [106998.863197] [<ffffffff812a9e25>] ? kobject_add+0x65/0xb0
> [106998.863204] [<ffffffff812b1e8e>] ? pointer.isra.18+0x9e/0x470
> [106998.863213] [<ffffffff8139c584>] ? device_add+0x124/0x610
> [106998.863221] [<ffffffff8139cc68>] ? device_create_groups_vargs+0xd8/0x100
> [106998.863239] [<ffffffffa0025000>] ? 0xffffffffa0024fff
> [106998.863247] [<ffffffff8139ccf1>] ? device_create+0x41/0x50
> [106998.863257] [<ffffffff812b621e>] ? kasprintf+0x3e/0x40
> [106998.863268] [<ffffffff8138c6a2>] ? misc_register+0xc2/0x120
> [106998.863277] [<ffffffffa002500c>] ? hello_init+0xc/0x1000 [hello]
> [106998.863285] [<ffffffff8100213c>] ? do_one_initcall+0xcc/0x200
> [106998.863295] [<ffffffff810d8d1a>] ? load_module+0x20da/0x26b0
> [106998.863303] [<ffffffff810d4900>] ? store_uevent+0x40/0x40
> [106998.863312] [<ffffffff810d944d>] ? SyS_finit_module+0x7d/0xa0
> [106998.863322] [<ffffffff8150d32d>] ? system_call_fast_compare_end+0x10/0x15
> [106998.863327] ---[ end trace af819f702b51bbb0 ]---
> [106998.863332] ------------[ cut here ]------------
>
>
>>> +static int match_child(struct device *dev, void *data)
>>> +{
>>> + if (!dev_name(dev))
>>> + return 0;
>>> + return !strcmp(dev_name(dev), (char *)data);
>>
>> strncmp instead of strcmp?
>
> I dont think it would make it safer here. You need to know the size of
> the string, and finding out the lenght of the string will produce the
> same access error (if that is what concerns you).
>
>> I still think we need solve this issue in device tree core code not in
>> the user of DT.
>
> Other subsystem do not have the issue of unique naming.
>
> ie, in video4linux the device name is created dynamically (video0,
> video1.....), others like hwmon have a hierarchical name
> (hmon0/input0, hwmon1/input0)
>
> I think this is a problem related to the led subsystem that does not
> have a way to handled repeated names. Before we could blame the system
> designer for choosing the same name for two leds, but with dt overlays
> this is not more in full control of the sys designer. Please correct
> me if I am wrong.
>
>
> Thanks!!!
>
> --
> Ricardo Ribalda
Hello Bryan
On Tue, Feb 17, 2015 at 11:34 PM, Bryan Wu <[email protected]> wrote:
>
> Can you show me your device tree code for overlaying describing a camera?
>
The camera is composed by fpga modules. The fpga is attached via pcie
to the host.
The whole dt would be too long.
Here you can see the relevant parts
&axi1 {
packer_0: packer_0{
#address-cells = <1>;
#size-cells = <1>;
compatible = "qtec,axi_matrix_packer-1.00.a";
reg = < 0x30060000 0x10000 >;
qtec,serial_number = "Invalid Head Serial Number";
qtec,bitstream_version = < 0 >;
qtec,head_i2c_address = < 0 >;
qtec,cdma = <&cdma_0>;
qtec,desc_mem= <&cdma_desc_mem>;
qtec,pcie_bridge= <&pcie_bridge_0>;
qtec,aperture-addr= < 0 4 >;
qtec,circular_buffer= <&video_mem>;
qtec,white_balance= <&white_0>;
qtec,sensor = <&ccd_fg_0>;
qtec,pll = < &pll_0 >;
qtec,xform = <&xform_0>;
qtec,testgen = <&testgen_0>;
qtec,encoder = <&encoder_0>;
interrupt-parent = <&xps_intc_0>;
interrupts = < 5 2 >;
};
gpio_0: gpio_0 {
#gpio-cells = <2>;
compatible = "xlnx,xps-gpio-1.00.a";
reg = < 0x30040000 010000 >;
xlnx,dout-default = < 0xffffffff >;
xlnx,tri-default = < 0xfffffff8 >;
xlnx,gpio-width = < 5 >;
};
/*Leds*/
leds {
reg = < 0x30040000 010000 >;
compatible = "gpio-leds";
red {
gpios = <&gpio_0 0 0>;
linux,default-trigger = "drop-qt5023_video0";
};
yellow {
gpios = <&gpio_0 1 0>;
linux,default-trigger = "packer-qt5023_video0";
};
green {
gpios = <&gpio_0 2 0>;
linux,default-trigger = "heartbeat";
};
};
If there are multiple cameras, then there will be multiple red, yellow
and green leds, throwing out the error I copied earlier.
Thanks!
--
Ricardo Ribalda
On Tue, Feb 17, 2015 at 2:54 PM, Ricardo Ribalda Delgado
<[email protected]> wrote:
> Hello Bryan
>
> On Tue, Feb 17, 2015 at 11:34 PM, Bryan Wu <[email protected]> wrote:
>>
>> Can you show me your device tree code for overlaying describing a camera?
>>
>
> The camera is composed by fpga modules. The fpga is attached via pcie
> to the host.
>
> The whole dt would be too long.
>
> Here you can see the relevant parts
>
> &axi1 {
>
>
> packer_0: packer_0{
> #address-cells = <1>;
> #size-cells = <1>;
> compatible = "qtec,axi_matrix_packer-1.00.a";
> reg = < 0x30060000 0x10000 >;
> qtec,serial_number = "Invalid Head Serial Number";
> qtec,bitstream_version = < 0 >;
> qtec,head_i2c_address = < 0 >;
> qtec,cdma = <&cdma_0>;
> qtec,desc_mem= <&cdma_desc_mem>;
> qtec,pcie_bridge= <&pcie_bridge_0>;
> qtec,aperture-addr= < 0 4 >;
> qtec,circular_buffer= <&video_mem>;
> qtec,white_balance= <&white_0>;
> qtec,sensor = <&ccd_fg_0>;
> qtec,pll = < &pll_0 >;
> qtec,xform = <&xform_0>;
> qtec,testgen = <&testgen_0>;
> qtec,encoder = <&encoder_0>;
> interrupt-parent = <&xps_intc_0>;
> interrupts = < 5 2 >;
> };
>
> gpio_0: gpio_0 {
> #gpio-cells = <2>;
> compatible = "xlnx,xps-gpio-1.00.a";
> reg = < 0x30040000 010000 >;
> xlnx,dout-default = < 0xffffffff >;
> xlnx,tri-default = < 0xfffffff8 >;
> xlnx,gpio-width = < 5 >;
> };
>
>
> /*Leds*/
> leds {
> reg = < 0x30040000 010000 >;
> compatible = "gpio-leds";
> red {
So why not use name "red0" and name another one "red1"? since you have
multiple different red leds here any way.
-Bryan
> gpios = <&gpio_0 0 0>;
> linux,default-trigger = "drop-qt5023_video0";
> };
> yellow {
> gpios = <&gpio_0 1 0>;
> linux,default-trigger = "packer-qt5023_video0";
> };
> green {
> gpios = <&gpio_0 2 0>;
> linux,default-trigger = "heartbeat";
> };
> };
>
>
> If there are multiple cameras, then there will be multiple red, yellow
> and green leds, throwing out the error I copied earlier.
>
>
> Thanks!
>
> --
> Ricardo Ribalda
Hi
On Wed, Feb 18, 2015 at 12:35 AM, Bryan Wu <[email protected]> wrote:
> So why not use name "red0" and name another one "red1"? since you have
> multiple different red leds here any way.
I cannot control how many cameras the user will decide to attach to
the system, and if two similar cameras are attached to pciA and pciB,
they would try to load the same dt.
So I would have to parse the device tree on-line, finding the leds,
renaming them... and that code would be duplicated for every driver
doing similar work. Not only that, I would have to avoid collisions
to my "cameras", also to other devices that may have decided to use
similar naming.
I think the proposed solution is more simple, and solves the issue of
duplicated led names around the whole system. Also, if there is no
duplicated names it follows the old behaviours.
Thanks!
>
> -Bryan
>> --
>> Ricardo Ribalda
--
Ricardo Ribalda
On Tue, Feb 17, 2015 at 3:47 PM, Ricardo Ribalda Delgado
<[email protected]> wrote:
> Hi
>
> On Wed, Feb 18, 2015 at 12:35 AM, Bryan Wu <[email protected]> wrote:
>
>> So why not use name "red0" and name another one "red1"? since you have
>> multiple different red leds here any way.
>
> I cannot control how many cameras the user will decide to attach to
> the system, and if two similar cameras are attached to pciA and pciB,
> they would try to load the same dt.
>
DT just describe the hardware, so if it's a different hardware, they
should have different name.
red0 for GPIO 0, red1 for GPIO 1 or choose other good name instead of 0 and 1.
If this is true, you can just name one LED like gpio-led and let the
kernel to add number after that name. I don't think this is a good
idea to name the hardware. And These aren't like other devices which
will create device node like /dev/video0 /dev/video1. This is from
device tree to describe the hardware.
Thanks,
-Bryan
Hello Bryan
On Wed, Feb 18, 2015 at 12:59 AM, Bryan Wu <[email protected]> wrote:
>
> DT just describe the hardware, so if it's a different hardware, they
> should have different name.
> red0 for GPIO 0, red1 for GPIO 1 or choose other good name instead of 0 and 1.
I think I have not managed to explain myself properly.
We have a host computer. with 2 pcie slots. The host is described with
a DT that looks like:
&axi1 {
pci0{
reg = < 0x20000000 0x10000000 >
}
pci1{
reg = < 0x30000000 0x10000000 >
}
}
The user can connect anything to the pci slots. (pci0 and pci1)
Lets say that we have a type of add-on card. Described by this DT
overlay (card.dtb):
&pci {
gpio_0: gpio_0 {
#gpio-cells = <2>;
compatible = "xlnx,xps-gpio-1.00.a";
reg = < 0x30040000 010000 >;
};
/*Leds*/
leds {
reg = < 0x30040000 010000 >;
compatible = "gpio-leds";
red {
gpios = <&gpio_0 0 0>;
linux,default-trigger = "drop-qt5023_video0";
};
}
}
The user connects two of those cards to the system (at locations pci0 and pci1).
Then we have TWO gpios chip. Each of them have a led named red. When
the second gpio-led is probed we have an error. Everything else
(address offset, phandle, device renaming) is handled properly already
by the kernel.
On this system I cannot control card.dtb, or which type of cards will
the user connect to the system. The DT is generated in run-time based
on the hardware connected to the pci slots.
I humbly believe that the issue here is that the subsystem does not
protect ourselves against name collisions, because a month ago a
device tree was considered immutable and in full control of the system
designer, unfortunately this is not the case anymore.
Thanks!
--
Ricardo Ribalda
On Tue, Feb 17, 2015 at 4:32 PM, Ricardo Ribalda Delgado
<[email protected]> wrote:
> Hello Bryan
>
> On Wed, Feb 18, 2015 at 12:59 AM, Bryan Wu <[email protected]> wrote:
>>
>> DT just describe the hardware, so if it's a different hardware, they
>> should have different name.
>> red0 for GPIO 0, red1 for GPIO 1 or choose other good name instead of 0 and 1.
>
> I think I have not managed to explain myself properly.
>
> We have a host computer. with 2 pcie slots. The host is described with
> a DT that looks like:
>
> &axi1 {
>
> pci0{
> reg = < 0x20000000 0x10000000 >
> }
>
> pci1{
> reg = < 0x30000000 0x10000000 >
> }
> }
>
> The user can connect anything to the pci slots. (pci0 and pci1)
>
>
> Lets say that we have a type of add-on card. Described by this DT
> overlay (card.dtb):
>
I think who write this card.dtb should understand this issue. And
choose the right name.
> &pci {
>
> gpio_0: gpio_0 {
What happen if you just use name 'gpio: gpio {' here.? Any conflicts
or kernel oops?
> #gpio-cells = <2>;
> compatible = "xlnx,xps-gpio-1.00.a";
> reg = < 0x30040000 010000 >;
> };
>
>
> /*Leds*/
> leds {
> reg = < 0x30040000 010000 >;
> compatible = "gpio-leds";
> red {
> gpios = <&gpio_0 0 0>;
> linux,default-trigger = "drop-qt5023_video0";
> };
> }
>
> }
>
> The user connects two of those cards to the system (at locations pci0 and pci1).
>
> Then we have TWO gpios chip. Each of them have a led named red. When
> the second gpio-led is probed we have an error. Everything else
> (address offset, phandle, device renaming) is handled properly already
> by the kernel.
>
> On this system I cannot control card.dtb, or which type of cards will
> the user connect to the system. The DT is generated in run-time based
> on the hardware connected to the pci slots.
>
So you're supposed to get 2 card.dtb files for 2 PCI cards, right?
They should be different and you need to choose different name for the
hardware.
> I humbly believe that the issue here is that the subsystem does not
> protect ourselves against name collisions, because a month ago a
> device tree was considered immutable and in full control of the system
> designer, unfortunately this is not the case anymore.
>
>From device tree point of view, I believe different device should got
different name although they can match to same compatible string. Let
me invite DT folks for help.
Thanks,
-Bryan
Hello Bryan
On Wed, Feb 18, 2015 at 1:52 AM, Bryan Wu <[email protected]> wrote:
>>
>> Lets say that we have a type of add-on card. Described by this DT
>> overlay (card.dtb):
>>
>
> I think who write this card.dtb should understand this issue. And
> choose the right name.
card.dtb just describe the hardware in the card, and it is not be
aware of the rest of the system.
I dont think it is practical to have card_HOST0_PCI1.dtb,
card_HOST0_PCI2.dtb to HOST0_PCI16.dtb and then HOST1_, HOST2....
>> gpio_0: gpio_0 {
>What happen if you just use name 'gpio: gpio {' here.? Any conflicts
>or kernel oops?
No problem here, one will create the device
/sys/devices/pci0000:00/0000:00:05.0/0000:01:00.0/30040000.gpio
and the other:
/sys/devices/pci0000:00/0000:00:06.0/0000:01:00.0/40040000.gpio
Name is created with hierarnchy
/sys/class/gpio/ will also work fine, because the gpiochip id is
created dynamically
On the other hand all the leds are under,
/sys/class/leds/NAME
Do not have any dynamic naming or hierarchical name.
> So you're supposed to get 2 card.dtb files for 2 PCI cards, right?
> They should be different and you need to choose different name for the
> hardware.
There is only one card.dtb because both cards are identical cards,
they are just connected to different ports
>
> From device tree point of view, I believe different device should got
> different name although they can match to same compatible string. Let
> me invite DT folks for help.
Another example of duplicated names could be partitions on an mtd. You
can have two devices with a partition called Golden.
If my memory is right, you can even have two partition with the same
name on the same device. The offset of the partition will be part of
the name in that case.
>
> Thanks,
> -Bryan
Thanks!
--
Ricardo Ribalda
> Thanks!
On Tue, Feb 17, 2015 at 5:11 PM, Ricardo Ribalda Delgado
<[email protected]> wrote:
> Hello Bryan
>
> On Wed, Feb 18, 2015 at 1:52 AM, Bryan Wu <[email protected]> wrote:
>>>
>>> Lets say that we have a type of add-on card. Described by this DT
>>> overlay (card.dtb):
>>>
>>
>> I think who write this card.dtb should understand this issue. And
>> choose the right name.
>
> card.dtb just describe the hardware in the card, and it is not be
> aware of the rest of the system.
>
> I dont think it is practical to have card_HOST0_PCI1.dtb,
> card_HOST0_PCI2.dtb to HOST0_PCI16.dtb and then HOST1_, HOST2....
>
>>> gpio_0: gpio_0 {
>>What happen if you just use name 'gpio: gpio {' here.? Any conflicts
>>or kernel oops?
>
> No problem here, one will create the device
>
> /sys/devices/pci0000:00/0000:00:05.0/0000:01:00.0/30040000.gpio
>
> and the other:
>
> /sys/devices/pci0000:00/0000:00:06.0/0000:01:00.0/40040000.gpio
>
> Name is created with hierarnchy
>
> /sys/class/gpio/ will also work fine, because the gpiochip id is
> created dynamically
>
> On the other hand all the leds are under,
>
> /sys/class/leds/NAME
>
> Do not have any dynamic naming or hierarchical name.
>
I got it. In this case we need to give the leds device a unique name.
Go back to your patch, you're adding 0, 1 at the end of the name of
leds. It's better like GPIO I think you can pick up <reg> value of
leds device node and put it in front of the name of leds. like
/sys/class/leds/30040000.red and /sys/class/leds/40040000.red.
Thanks,
-Bryan
Hi
On Wed, Feb 18, 2015 at 2:36 AM, Bryan Wu <[email protected]> wrote:
>>
>
> I got it. In this case we need to give the leds device a unique name.
> Go back to your patch, you're adding 0, 1 at the end of the name of
> leds. It's better like GPIO I think you can pick up <reg> value of
> leds device node and put it in front of the name of leds. like
> /sys/class/leds/30040000.red and /sys/class/leds/40040000.red.
Hmmm.... That will not solve the issue for every device.
If I had a mmio, the gpio would be located at
/sys/devices/pci0000:00/0000:00:05.0/0000:01:00.0/30040000.gpio
and
/sys/devices/pci0000:00/0000:00:06.0/0000:01:00.0/30040000.gpio
Also it could be the case where the gpio is not memory mapped, then it
would be something like:
/sys/devices/pci0000:00/0000:00:05.0/0000:01:00.0/gpio
and
/sys/devices/pci0000:00/0000:00:06.0/0000:01:00.0/gpio
And at any case we should respect the old api, we can only rename the
second device, not the first one.
What is your concern about the initial proposal? What about NAME_dup0
instead of NAME_0?
We could throw a dev_info(), so the system developer will have a
chance to fix it (if he can) and the user to ignore it safely.
Thanks!
--
Ricardo Ribalda
On Tue, Feb 17, 2015 at 10:55 PM, Ricardo Ribalda Delgado
<[email protected]> wrote:
> Hi
>
> On Wed, Feb 18, 2015 at 2:36 AM, Bryan Wu <[email protected]> wrote:
>
>>>
>>
>> I got it. In this case we need to give the leds device a unique name.
>> Go back to your patch, you're adding 0, 1 at the end of the name of
>> leds. It's better like GPIO I think you can pick up <reg> value of
>> leds device node and put it in front of the name of leds. like
>> /sys/class/leds/30040000.red and /sys/class/leds/40040000.red.
>
> Hmmm.... That will not solve the issue for every device.
>
> If I had a mmio, the gpio would be located at
>
> /sys/devices/pci0000:00/0000:00:05.0/0000:01:00.0/30040000.gpio
> and
> /sys/devices/pci0000:00/0000:00:06.0/0000:01:00.0/30040000.gpio
>
> Also it could be the case where the gpio is not memory mapped, then it
> would be something like:
>
> /sys/devices/pci0000:00/0000:00:05.0/0000:01:00.0/gpio
> and
> /sys/devices/pci0000:00/0000:00:06.0/0000:01:00.0/gpio
>
> And at any case we should respect the old api, we can only rename the
> second device, not the first one.
>
> What is your concern about the initial proposal? What about NAME_dup0
> instead of NAME_0?
>
Actually I'd like to see some meaningful unique name for each device
when we create.
But looks like there is no such way to find some unique properties
from device tree node.
> We could throw a dev_info(), so the system developer will have a
> chance to fix it (if he can) and the user to ignore it safely.
>
Yes, this is what I want. It's good to let the user know there are
multiple leds share the same name in DT. Sometime they made some
mistake in the DTS file.
Please update the patch, we can start to discuss the implementation, then.
Actually I think we don't need the temp_name and just use the "name".
And one more thing is device_find_child() will find child of the
parent. But in your 2 PCI card case, these 2 parents are different
then device_find_child() will return false twice even if your 2 red
leds have the same name.
I think the better solution is search the name in registered leds
device, if found name is the same, then add a number or something. And
move out this name processing code to a separated function.
Thanks,
-Bryan
Hello Bryan
On Thu, Feb 19, 2015 at 8:12 PM, Bryan Wu <[email protected]> wrote:
> Yes, this is what I want. It's good to let the user know there are
> multiple leds share the same name in DT. Sometime they made some
> mistake in the DTS file.
I have added that message now:
dev_info(parent, "Led %s renamed to %s due to name collision",
led_cdev->name, dev_name(led_cdev->dev));
>
> Please update the patch, we can start to discuss the implementation, then.
>
> Actually I think we don't need the temp_name and just use the "name".
We need to keep a pointer to the generated string, to release it later
with kfree.
Also I rather allocate new memory only if there is a name collision.
> And one more thing is device_find_child() will find child of the
> parent. But in your 2 PCI card case, these 2 parents are different
> then device_find_child() will return false twice even if your 2 red
> leds have the same name.
I am now iterating with
while (class_find_device(leds_class, NULL, name, match_name));
Thanks
--
Ricardo Ribalda
On Tue, Feb 17, 2015 at 04:52:45PM -0800, Bryan Wu wrote:
> On Tue, Feb 17, 2015 at 4:32 PM, Ricardo Ribalda Delgado
[..]
> > Then we have TWO gpios chip. Each of them have a led named red. When
> > the second gpio-led is probed we have an error. Everything else
> > (address offset, phandle, device renaming) is handled properly already
> > by the kernel.
> >
> > On this system I cannot control card.dtb, or which type of cards will
> > the user connect to the system. The DT is generated in run-time based
> > on the hardware connected to the pci slots.
> >
> So you're supposed to get 2 card.dtb files for 2 PCI cards, right?
> They should be different and you need to choose different name for the
> hardware.
Is this solvable by adding support for led properties in an aliases { }
node? I suppose that would mean you'd need to also overlay a selection
of which LED is "red", but I don't know if that is a problem.
Josh
Helo Josh
On Fri, Feb 20, 2015 at 12:15 AM, Josh Cartwright <[email protected]> wrote:
>
> Is this solvable by adding support for led properties in an aliases { }
> node? I suppose that would mean you'd need to also overlay a selection
> of which LED is "red", but I don't know if that is a problem.
I think there are two concens here. One is to provide an unique name
when a led is register and the other is to locate the led.
This patch is just mend for providing unique names, so the led
subsystem can play nicer with overlays.
The other might be solved with an sysfs symbolic link.... I need to
check it out, but it is a different issue.
>
> Josh
--
Ricardo Ribalda