Any,
Thanks for your review.
------------------------------------------------------------------------
BRs,
Intel VTG - Linux & Chrome IPU SW
Bingbu Cao
> -----Original Message-----
> From: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> Sent: Monday, March 6, 2023 19:27
> To: Cao, Bingbu <bingbu.cao@xxxxxxxxx>
> Cc: linux-media@xxxxxxxxxxxxxxx; sakari.ailus@xxxxxxxxxxxxxxx;
> djrscally@xxxxxxxxx; bingbu.cao@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH] media: ipu3-cio2: support multiple sensors and VCMs
> with HID name
>
> On Fri, Mar 03, 2023 at 10:44:32PM +0800, bingbu.cao@xxxxxxxxx wrote:
> > From: Bingbu Cao <bingbu.cao@xxxxxxxxx>
> >
> > In current cio2-bridge, it is using the hid as node name to register
> > software node and swnode will create kobject and sysfs entry with the
> > node name, if there are multiple sensors and VCMs which are sharing
> > same HID name, it will cause the software nodes registration failure:
> >
> > [ 7.142311] sysfs: cannot create duplicate filename
> '/kernel/software_nodes/dw9714'
> > ...
> > [ 7.142328] Call Trace:
> > [ 7.142330] <TASK>
> > [ 7.142336] dump_stack_lvl+0x49/0x63
> > [ 7.142341] dump_stack+0x10/0x16
> > [ 7.142343] sysfs_warn_dup.cold+0x17/0x2b [ 7.142346]
> > sysfs_create_dir_ns+0xbc/0xd0 [ 7.142351]
> > kobject_add_internal+0xb1/0x2b0 [ 7.142356]
> > kobject_init_and_add+0x71/0xa0 [ 7.142360]
> > swnode_register+0x136/0x210 [ 7.142363]
> > software_node_register+0xd2/0x120 [ 7.142364]
> > software_node_register_nodes+0x83/0xf0
> > [ 7.142366] ? acpi_get_physical_device_location+0x65/0xc0
> > [ 7.142371] cio2_bridge_init+0x82a/0xb5e ...
> > [ 7.142448] kobject_add_internal failed for dw9714 with -EEXIST, don't
> > try to register things with the same name in the same directory.
>
> Please cut unneeded context of the backtrace as it's explained in the
> Submitting Patches documentation.
Thanks, will amend in v2.
>
> > One solution is appending the sensor link(Mipi Port) in SSDB as suffix
> > of the node name to fix this problem.
>
> ...
>
> > + if (sensor->ssdb.vcmtype) {
> > + scnprintf(vcm_name, sizeof(vcm_name), "%s-%u",
> > + cio2_vcm_types[sensor->ssdb.vcmtype - 1],
> > + sensor->ssdb.link);
>
> Is using 'c' variant a cargo cult? Otherwise explain, why dropping the
> last part of the number is not a problem.
Sorry, I can't understand. What is cargo cult?
>
> > + nodes[SWNODE_VCM] = NODE_VCM(vcm_name);
> > + }
>
> ...
>
> > + scnprintf(sensor->name, sizeof(sensor->name), "%s-%u",
> > + cfg->hid, sensor->ssdb.link);
>
>
> Ditto.
>
> ...
>
> > - char name[ACPI_ID_LEN];
> > + char name[ACPI_ID_LEN + 4];
>
> Why 4 is chosen? This needs an explanation.
'link' is u8, so it is supposed to be max 4 characters along with '-'.
>
> --
> With Best Regards,
> Andy Shevchenko
>
