Re: [PATCH v6 2/6] platform/x86/intel/vsec: Add PMT read callbacks

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

On Wed, 10 Jul 2024, Michael J. Ruhl wrote:

> From: "David E. Box" <david.e.box@linux.intel.com>
> 
> Some PMT providers require device specific actions before their telemetry
> can be read. Provide assignable PMT read callbacks to allow providers to
> perform those actions.
> 
> Reviewed-by: Michael J. Ruhl <michael.j.ruhl@intel.com>
> Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> ---
>  drivers/platform/x86/intel/vsec.c |  1 +
>  include/linux/intel_vsec.h        | 14 ++++++++++++++
>  2 files changed, 15 insertions(+)
> 
> diff --git a/drivers/platform/x86/intel/vsec.c b/drivers/platform/x86/intel/vsec.c
> index 2b46807f868b..7b5cc9993974 100644
> --- a/drivers/platform/x86/intel/vsec.c
> +++ b/drivers/platform/x86/intel/vsec.c
> @@ -212,6 +212,7 @@ static int intel_vsec_add_dev(struct pci_dev *pdev, struct intel_vsec_header *he
>  	intel_vsec_dev->num_resources = header->num_entries;
>  	intel_vsec_dev->quirks = info->quirks;
>  	intel_vsec_dev->base_addr = info->base_addr;
> +	intel_vsec_dev->priv_data = info->priv_data;
>  
>  	if (header->id == VSEC_ID_SDSI)
>  		intel_vsec_dev->ida = &intel_vsec_sdsi_ida;
> diff --git a/include/linux/intel_vsec.h b/include/linux/intel_vsec.h
> index 1a287541a2f9..4569a55e8645 100644
> --- a/include/linux/intel_vsec.h
> +++ b/include/linux/intel_vsec.h
> @@ -67,6 +67,19 @@ enum intel_vsec_quirks {
>  	VSEC_QUIRK_EARLY_HW     = BIT(4),
>  };
>  
> +/**
> + * struct pmt_callbacks - Callback infrastructure for PMT devices
> + * ->read_telem() when specified, called by client driver to access PMT data (instead
> + * of direct copy).
> + * @args:  pci device info pointer

PCI

> + * @guid:  ID of data to acccss
> + * @data:  buffer for the data to be copied
> + * @count: size of buffer
> + */
> +struct pmt_callbacks {
> +	int (*read_telem)(void *args, u32 guid, u64 *data, u32 count);

Can you explain why args is void * and not struct pci_dev *? I was already 
unsure earlier why void * is needed but now you even explicitly documented
it to be "pci device info pointer".

> +};
> +
>  /**
>   * struct intel_vsec_platform_info - Platform specific data
>   * @parent:    parent device in the auxbus chain
> @@ -78,6 +91,7 @@ enum intel_vsec_quirks {
>  struct intel_vsec_platform_info {
>  	struct device *parent;
>  	struct intel_vsec_header **headers;
> +	void *priv_data;
>  	unsigned long caps;
>  	unsigned long quirks;
>  	u64 base_addr;
> 

-- 
 i.