mbox series

[v2,0/8] ACPI _CRS CSI-2 and MIPI DisCo for Imaging support

Message ID 20230123134617.265382-1-sakari.ailus@linux.intel.com
Headers show
Series ACPI _CRS CSI-2 and MIPI DisCo for Imaging support | expand

Message

Sakari Ailus Jan. 23, 2023, 1:46 p.m. UTC
Hello all,

Here's an implementation of ACPI 6.4 _CRS CSI-2 resource descriptor and
MIPI DisCo for Imaging 1.0 [1]. What the two basically provide is an
officially sanctioned way to describe CSI-2 connected cameras to operating
system software, something DT based systems have enjoyed for quite some
time already.

The implementation digs the information from ACPI tables (_CRS descriptors
and data + property extensions) and constructs software nodes that are
compatible with Documentation/firmware-guide/acpi/dsd/graph.rst and
Documentation/devicetree/bindings/media/video-interface-devices.yaml . No
specific driver changes are needed.

These patches are on the testing branch of the linux-acpi tree where they
depend on the patch constifying the ACPI pathname argument for
acpi_get_handle() (commit 91fdb91ccca2b48572a1ccf1d382fd599e3e1237).

[1] https://www.mipi.org/specifications/mipi-disco-imaging

since v1:

- Update copyright notices.

- Include linux/types.h instead of linux/kernel.h in drivers/acpi/mipi.c.

- Use SWNODE_GRAPH_PORT_NAME_FMT instead of plain "port@%u" in
  GRAPH_PORT_NAME macro.

- Make the condition in NEXT_PROPERTY() macro easier to read.

- Unwrap lines to make them moderately longer than 80 characters.

- Use * BITS_PER_TYPE(u8) instead of << 3 to convert bytes to bits in
  init_port_csi2_common().

- Test ACPI framework call success using ACPI_SUCCESS() instead of
  comparing with AE_OK. Likewise for ACPI_FAILURE and != AE_OK.

- Use newly added SOFTWARE_NODE() macro to construct the root software
  node.

- Use str_has_prefix() to test for a string prefix instead of memcmp().

- Add pr_fmt() macro to drivers/acpi/property.c.

- Move logical or operators to the end of the line in
  acpi_properties_prepare().

- Improve bad node type error in acpi_parse_string_ref().

Sakari Ailus (8):
  ACPI: property: Parse data node string references in properties
  ACPI: property: Parse _CRS CSI-2 descriptor
  device property: Add SOFTWARE_NODE() macro for defining software nodes
  ACPI: property: Generate camera swnodes for ACPI and DisCo for Imaging
  ACPI: property: Dig "rotation" property for devices with CSI2 _CRS
  ACPI: property: Rename parsed MIPI DisCo for Imaging properties
  ACPI: property: Skip MIPI property table without "mipi-img" prefix
  ACPI: property: Document _CRS CSI-2 and DisCo for Imaging support

 drivers/acpi/Makefile    |   2 +-
 drivers/acpi/internal.h  |   9 +
 drivers/acpi/mipi.c      | 761 +++++++++++++++++++++++++++++++++++++++
 drivers/acpi/property.c  | 128 +++++--
 drivers/acpi/scan.c      |  33 +-
 include/acpi/acpi_bus.h  |  61 ++++
 include/linux/property.h |   7 +
 7 files changed, 972 insertions(+), 29 deletions(-)
 create mode 100644 drivers/acpi/mipi.c

Comments

Sakari Ailus Jan. 23, 2023, 3:53 p.m. UTC | #1
Hi Andy,

On Mon, Jan 23, 2023 at 04:51:33PM +0200, Andy Shevchenko wrote:
> On Mon, Jan 23, 2023 at 03:46:10PM +0200, Sakari Ailus wrote:
> > Add support for parsing property references using strings, besides
> > reference objects that were previously supported. This allows also
> > referencing data nodes which was not possible with reference objects.
> > 
> > Also add pr_fmt() macro to prefix printouts.
> > 
> > While at it, update copyright.
> 
> ...
> 
> > - * Copyright (C) 2014, Intel Corporation
> > + * Copyright (C) 2014--2023, Intel Corporation
> 
> Isn't one dash enough? 
> 
> $ git grep -n 'opyright.*[0-9]--[0-9]' | wc -l
> 37
> 
> $ git grep -n 'opyright.*[0-9]-[0-9]' | wc -l
> 15064

This is a range, not hyphenation. There's no different character in the
ASCII character set for the former, commonly two regular dashes are used.
There probably would be a correct Unicode character though.

> 
> 
> >   * All rights reserved.
> >   *
> >   * Authors: Mika Westerberg <mika.westerberg@linux.intel.com>
> >   *          Darren Hart <dvhart@linux.intel.com>
> >   *          Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > + *	    Sakari Ailus <sakari.ailus@linux.intel.com>
> 
> Seems wrong indentation in comparison to the others.

Tabs are preferred for intendation. I can change all the lines to use tab.
How about that?

> 
> >   */
> 
> ...
> 
> > +static struct fwnode_handle *
> > +acpi_parse_string_ref(const struct fwnode_handle *fwnode, const char *refstring)
> > +{
> > +	acpi_handle scope, handle;
> > +	struct acpi_data_node *dn;
> > +	struct acpi_device *device;
> > +	acpi_status status;
> > +
> > +	if (is_acpi_device_node(fwnode)) {
> 
> > +		scope = to_acpi_device_node(fwnode)->handle;
> 
> Interestingly that we have a helper for this -- ACPI_HANDLE_FWNODE()...
> 
> > +	} else if (is_acpi_data_node(fwnode)) {
> 
> > +		scope = to_acpi_data_node(fwnode)->handle;
> 
> ...but not for this.

I'd either prefer to keep them as-is, as it's easy to see what's being done
there, or add a new macro --- or a function to do this.  Say,
acpi_fwnode_acpi_handle(), as this is clearly ACPI specific and to
differentiate between ACPI handles and fwnode handles.

ACPI_HANDLE_FWNODE()'s name suggests it would do something else than it
does, if you consider the current fwnode API.
Heikki Krogerus Jan. 24, 2023, 11:40 a.m. UTC | #2
On Mon, Jan 23, 2023 at 03:46:12PM +0200, Sakari Ailus wrote:
> Add SOFTWARE_NODE() macro in order to make defining software nodes look
> nicer. This is analogous to different PROPERTY_ENTRY_*() macros for
> defining properties.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

> ---
>  include/linux/property.h | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/include/linux/property.h b/include/linux/property.h
> index 37179e3abad5c..6745a86bc9b97 100644
> --- a/include/linux/property.h
> +++ b/include/linux/property.h
> @@ -477,6 +477,13 @@ struct software_node {
>  	const struct property_entry *properties;
>  };
>  
> +#define SOFTWARE_NODE(_name_, _properties_, _parent_)	\
> +	(struct software_node) {			\
> +		.name = _name_,				\
> +		.properties = _properties_,		\
> +		.parent = _parent_,			\
> +	}
> +
>  bool is_software_node(const struct fwnode_handle *fwnode);
>  const struct software_node *
>  to_software_node(const struct fwnode_handle *fwnode);
> -- 
> 2.30.2