Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1425466 > unrolled thread

Re: [PATCH 2/6] hid: intel_ish-hid: ISH Transport layer

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 2/6] hid: intel_ish-hid: ISH Transport layer Jiri Kosina <jikos@kernel.org> - 2016-06-17 22:50 +0200
    Re: [PATCH 2/6] hid: intel_ish-hid: ISH Transport layer Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> - 2016-06-17 23:20 +0200
      Re: [PATCH 2/6] hid: intel_ish-hid: ISH Transport layer Jiri Kosina <jikos@kernel.org> - 2016-06-20 11:40 +0200
        Re: [PATCH 2/6] hid: intel_ish-hid: ISH Transport layer One Thousand Gnomes <gnomes@lxorguk.ukuu.org.uk> - 2016-06-20 16:30 +0200

#1425466 — Re: [PATCH 2/6] hid: intel_ish-hid: ISH Transport layer

On Sat, 11 Jun 2016, Srinivas Pandruvada wrote:

[ ... snip ... ]
> diff --git a/drivers/hid/intel-ish-hid/Kconfig b/drivers/hid/intel-ish-hid/Kconfig
> new file mode 100644
> index 0000000..8914f3b
> --- /dev/null
> +++ b/drivers/hid/intel-ish-hid/Kconfig
> @@ -0,0 +1,22 @@
> +menu "Intel ISH HID support"
> +	depends on X86 && PCI
> +
> +config INTEL_ISH_HID_TRANSPORT
> +	bool
> +	default n
> +
> +config INTEL_ISH_HID
> +	bool "Intel Integrated Sensor Hub"

Why can't the transport driver be built as a module?

[ ... snip ... ]
> +/**
> + * ishtp_bus_add_device() - Function to create device on bus
> + *
> + * @dev:	ishtp device
> + * @uuid:	uuid of the client
> + * @name:	Name of the client
> + *
> + * Allocate ISHTP bus client device, attach it to uuid
> + * and register with ISHTP bus.
> + */
> +struct ishtp_cl_device *ishtp_bus_add_device(struct ishtp_device *dev,
> +					     uuid_le uuid, char *name)
> +{

Should be static.

[ ... snip ... ]
> +/**
> + * ishtp_bus_remove_device() - Function to relase device on bus
> + *
> + * @device:	client device instance
> + *
> + * This is a counterpart of ishtp_bus_add_device.
> + * Device is unregistered.
> + * the device structure is freed in 'ishtp_cl_dev_release' function
> + * Called only during error in pci driver init path.
> + */
> +void ishtp_bus_remove_device(struct ishtp_cl_device *device)
> +{

Should be static.

[ ... snip ... ]
> +/*
> + * ishtp_hbm_dma_xfer_ack - receive ack for ISHTP-over-DMA client message
> + *
> + * Constraint:
> + * First implementation is one ISHTP message per DMA transfer
> + */
> +void ishtp_hbm_dma_xfer_ack(struct ishtp_device *dev,

Should be static.

[ ... snip ... ]
> +/* ishtp_hbm_dma_xfer - receive ISHTP-over-DMA client message */
> +void ishtp_hbm_dma_xfer(struct ishtp_device *dev,
> +			struct dma_xfer_hbm *dma_xfer)

Should be static.

-- 
Jiri Kosina
SUSE Labs

[toc] | [next] | [standalone]

#1425489

On Fri, 2016-06-17 at 22:43 +0200, Jiri Kosina wrote:
> On Sat, 11 Jun 2016, Srinivas Pandruvada wrote:
> 
> [ ... snip ... ]
> > diff --git a/drivers/hid/intel-ish-hid/Kconfig b/drivers/hid/intel-
> ish-hid/Kconfig
> > new file mode 100644
> > index 0000000..8914f3b
> > --- /dev/null
> > +++ b/drivers/hid/intel-ish-hid/Kconfig
> > @@ -0,0 +1,22 @@
> > +menu "Intel ISH HID support"
> > +     depends on X86 && PCI
> > +
> > +config INTEL_ISH_HID_TRANSPORT
> > +     bool
> > +     default n
> > +
> > +config INTEL_ISH_HID
> > +     bool "Intel Integrated Sensor Hub"
> 
> Why can't the transport driver be built as a module?
In current use case for PM, we don't want anyone to unload and
complain.
But if this is a strong requirement, I will change this to a module.

Thanks,
Srinivas

> 
> [ ... snip ... ]
> > +/**
> > + * ishtp_bus_add_device() - Function to create device on bus
> > + *
> > + * @dev:     ishtp device
> > + * @uuid:    uuid of the client
> > + * @name:    Name of the client
> > + *
> > + * Allocate ISHTP bus client device, attach it to uuid
> > + * and register with ISHTP bus.
> > + */
> > +struct ishtp_cl_device *ishtp_bus_add_device(struct ishtp_device
> *dev,
> > +                                          uuid_le uuid, char
> *name)
> > +{
> 
> Should be static.
> 
> [ ... snip ... ]
> > +/**
> > + * ishtp_bus_remove_device() - Function to relase device on bus
> > + *
> > + * @device:  client device instance
> > + *
> > + * This is a counterpart of ishtp_bus_add_device.
> > + * Device is unregistered.
> > + * the device structure is freed in 'ishtp_cl_dev_release'
> function
> > + * Called only during error in pci driver init path.
> > + */
> > +void ishtp_bus_remove_device(struct ishtp_cl_device *device)
> > +{
> 
> Should be static.
> 
> [ ... snip ... ]
> > +/*
> > + * ishtp_hbm_dma_xfer_ack - receive ack for ISHTP-over-DMA client
> message
> > + *
> > + * Constraint:
> > + * First implementation is one ISHTP message per DMA transfer
> > + */
> > +void ishtp_hbm_dma_xfer_ack(struct ishtp_device *dev,
> 
> Should be static.
> 
> [ ... snip ... ]
> > +/* ishtp_hbm_dma_xfer - receive ISHTP-over-DMA client message */
> > +void ishtp_hbm_dma_xfer(struct ishtp_device *dev,
> > +                     struct dma_xfer_hbm *dma_xfer)
> 
> Should be static.
> 
> -- 
> Jiri Kosina
> SUSE Labs
> 

[toc] | [prev] | [next] | [standalone]

#1426420

On Fri, 17 Jun 2016, Srinivas Pandruvada wrote:

> > > +config INTEL_ISH_HID_TRANSPORT
> > > +     bool
> > > +     default n
> > > +
> > > +config INTEL_ISH_HID
> > > +     bool "Intel Integrated Sensor Hub"
> > 
> > Why can't the transport driver be built as a module?
> In current use case for PM, we don't want anyone to unload and
> complain.

Sorry, I don't understand this explanation, could you please elaborate?

Thanks,

-- 
Jiri Kosina
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1426649

On Mon, 20 Jun 2016 11:29:28 +0200 (CEST)
Jiri Kosina <jikos@kernel.org> wrote:

> On Fri, 17 Jun 2016, Srinivas Pandruvada wrote:
> 
> > > > +config INTEL_ISH_HID_TRANSPORT
> > > > +     bool
> > > > +     default n
> > > > +
> > > > +config INTEL_ISH_HID
> > > > +     bool "Intel Integrated Sensor Hub"  
> > > 
> > > Why can't the transport driver be built as a module?  
> > In current use case for PM, we don't want anyone to unload and
> > complain.  
> 
> Sorry, I don't understand this explanation, could you please elaborate?

If the driver isn't loaded your machine doesn't do power management.
There are a small set of drivers that need to be loaded to get pm even if
not using that device. ISH is one of them, and unlike the others not
sucked into a standard configuration.

If it's going to get loaded due to the presence of the PCI identifiers on
any normal distribution then it's in the same category as graphics and
ADSP based audio, both of which can be modules and the only real world
impact is they get loaded a second or two later during boot.

Alan

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web