Hi Andy,
Thanks for the review.
I think Rafael has already applied this but I can send a new patch...
On Wed, Feb 21, 2024 at 12:21:13AM +0200, andy.shevchenko@xxxxxxxxx wrote:
> Tue, Feb 13, 2024 at 03:46:06PM +0200, Sakari Ailus kirjoitti:
> > Some systems were shipped with both Windows and Linux camera descriptions.
> > In general, if Linux description exist, they will be used and Windows
> > description ignored.
> >
> > In this case the Linux descriptions were buggy so use Windows definition
> > instead. This patch ignores the bad graph port nodes on Dell XPS 9315 and
> > there are likely other such systems, too. The corresponding information
> > has been added to ipu-bridge to cover the missing bits.
>
> ...
>
> > void acpi_mipi_scan_crs_csi2(void);
> > void acpi_mipi_init_crs_csi2_swnodes(void);
> > void acpi_mipi_crs_csi2_cleanup(void);
>
> + blank line?
The usa of blank lines elsewhere in the file is consistent with the above.
These are all related.
>
> > +bool acpi_graph_ignore_port(acpi_handle handle);
> >
> > #endif /* _ACPI_INTERNAL_H_ */
>
> ...
>
> > +static const struct dmi_system_id dmi_ignore_port_nodes[] = {
> > + {
> > + .matches = {
> > + DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> > + DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "XPS 9315"),
> > + },
> > + },
> > + { 0 }
>
> 0 is not needed. Moreover, it's a bit unusual.
But also required by the C standard.
> > +};
>
> ...
>
> > +static const char *strnext(const char *s1, const char *s2)
> > +{
> > + s1 = strstr(s1, s2);
> > +
> > + if (!s1)
> > + return NULL;
> > +
> > + return s1 + strlen(s2);
> > +}
>
> NIH str_has_prefix() ?
It doesn't do the same thing. Yes, it could be used, but the code looks
cleaner with this.
>
> ...
>
> > +/**
> > + * acpi_graph_ignore_port - Tell whether a port node should be ignored
> > + * @handle: The ACPI handle of the node (which may be a port node)
> > + *
> > + * Returns true if a port node should be ignored and the data to that should
>
> Please, run kernel-doc validation and fix the warnings.
Running kernel-doc on the file doesn't produce any here.
>
> > + * come from other sources instead (Windows ACPI definitions and
> > + * ipu-bridge). This is currently used to ignore bad port nodes related to IPU6
> > + * ("IPU?") and camera sensor devices ("LNK?") in certain Dell systems with
> > + * Intel VSC.
> > + */
> > +bool acpi_graph_ignore_port(acpi_handle handle)
> > +{
> > + const char *path = NULL, *orig_path;
> > + static bool dmi_tested, ignore_port;
> > +
> > + if (!dmi_tested) {
> > + ignore_port = dmi_first_match(dmi_ignore_port_nodes);
> > + dmi_tested = true;
> > + }
> > +
> > + if (!ignore_port)
> > + return false;
> > +
> > + /* Check if the device is either "IPU" or "LNK" (sensor). */
> > + orig_path = acpi_handle_path(handle);
> > + if (!orig_path)
> > + return false;
> > + path = strnext(orig_path, "IPU");
>
> IIUC this can be rewritten as
>
> path += str_has_prefix();
> if (path == orig_path)
You could do that here, but now below, without introducing a new temporary
variable and shuffling that and "path". I prefer to keep it as-is.
> ...
>
> > + if (!path)
> > + path = strnext(orig_path, "LNK");
> > + if (!path)
> > + goto out_free;
> > +
> > + if (!(path[0] >= '0' && path[0] <= '9' && path[1] == '.'))
>
> isdigit() ?
Makes sense.
>
> > + goto out_free;
> > +
> > + /* Check if the node has a "PRT" prefix. */
> > + path = strnext(path, "PRT");
> > + if (path && path[0] >= '0' && path[0] <= '9' && !path[1]) {
>
> Ditto.
Yes.
>
> > + acpi_handle_debug(handle, "ignoring data node\n");
> > +
> > + kfree(orig_path);
> > + return true;
> > + }
> > +
> > +out_free:
> > + kfree(orig_path);
> > + return false;
> > +}
>
--
Kind regards,
Sakari Ailus
