Re: [PATCH v3 3/8] iio: common: cros_ec_sensors: move registration to core
On Sat, 22 Jun 2019 11:13:08 +0100
Jonathan Cameron wrote:
> On Tue, 18 Jun 2019 11:06:34 +0200
> Fabien Lahoudere wrote:
>
> > In order to simplify derivated drivers from cros_ec_sensors_core,
> > a new core function is created to registered IIO stricture.
> >
> > Signed-off-by: Fabien Lahoudere
> This one looks good to me.
> I'll pick it up once the minor stuff in other patches is sorted.
Sorry, I confused a couple of patches as held some of these back
in my queue to see how you used them later.
This one I'm actually unconvinced by. It's a 'cleanup'
that adds code and hides what is going on. I was expecting to find
more stuff being added to the resulting function later in the series.
I didn't find any, but may have missed something.
So not keen on this one patch in the series!
thanks,
Jonathan
>
> Thanks,
>
> Jonathan
>
> > ---
> > .../common/cros_ec_sensors/cros_ec_sensors.c | 9 +-
> > .../cros_ec_sensors/cros_ec_sensors_core.c| 97 ---
> > drivers/iio/light/cros_ec_light_prox.c| 7 +-
> > drivers/iio/pressure/cros_ec_baro.c | 7 +-
> > .../linux/iio/common/cros_ec_sensors_core.h | 16 ++-
> > 5 files changed, 72 insertions(+), 64 deletions(-)
> >
> > diff --git a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c
> > b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c
> > index 897dc83a3355..c4bee9265246 100644
> > --- a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c
> > +++ b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c
> > @@ -14,7 +14,7 @@
> > #include
> > #include
> > #include
> > -#include
> > +
> > #include
> > #include
> > #include
> > @@ -233,12 +233,7 @@ static int cros_ec_sensors_probe(struct
> > platform_device *pdev)
> > else
> > state->core.read_ec_sensors_data = cros_ec_sensors_read_cmd;
> >
> > - ret = devm_iio_triggered_buffer_setup(dev, indio_dev, NULL,
> > - cros_ec_sensors_capture, NULL);
> > - if (ret)
> > - return ret;
> > -
> > - return devm_iio_device_register(dev, indio_dev);
> > + return cros_ec_sensors_core_register(pdev, indio_dev);
> > }
> >
> > static const struct platform_device_id cros_ec_sensors_ids[] = {
> > diff --git a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c
> > b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c
> > index e5181e007dd7..3880849c5cca 100644
> > --- a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c
> > +++ b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c
> > @@ -12,6 +12,7 @@
> > #include
> > #include
> > #include
> > +#include
> > #include
> > #include
> > #include
> > @@ -95,6 +96,67 @@ int cros_ec_sensors_core_init(struct platform_device
> > *pdev,
> > }
> > EXPORT_SYMBOL_GPL(cros_ec_sensors_core_init);
> >
> > +/**
> > + * cros_ec_sensors_capture() - the trigger handler function
> > + * @irq: the interrupt number.
> > + * @p: a pointer to the poll function.
> > + *
> > + * On a trigger event occurring, if the pollfunc is attached then this
> > + * handler is called as a threaded interrupt (and hence may sleep). It
> > + * is responsible for grabbing data from the device and pushing it into
> > + * the associated buffer.
> > + *
> > + * Return: IRQ_HANDLED
> > + */
> > +static irqreturn_t cros_ec_sensors_capture(int irq, void *p)
> > +{
> > + struct iio_poll_func *pf = p;
> > + struct iio_dev *indio_dev = pf->indio_dev;
> > + struct cros_ec_sensors_core_state *st = iio_priv(indio_dev);
> > + int ret;
> > +
> > + mutex_lock(>cmd_lock);
> > +
> > + /* Clear capture data. */
> > + memset(st->samples, 0, indio_dev->scan_bytes);
> > +
> > + /* Read data based on which channels are enabled in scan mask. */
> > + ret = st->read_ec_sensors_data(indio_dev,
> > + *indio_dev->active_scan_mask,
> > + (s16 *)st->samples);
> > + if (ret < 0)
> > + goto done;
> > +
> > + iio_push_to_buffers_with_timestamp(indio_dev, st->samples,
> > + iio_get_time_ns(indio_dev));
> > +
> > +done:
> > + /*
> > +* Tell the core we are done with this trigger and ready for the
> > +* next one.
> > +*/
> > + iio_trigger_notify_done(indio_dev->trig);
> > +
> > + mutex_unlock(>cmd_lock);
> > +
> > + return IRQ_HANDLED;
> > +}
> > +
> > +int cros_ec_sensors_core_register(struct platform_device *pdev,
> > + struct iio_dev *indio_dev)
> > +{
> > + int ret;
> > + struct device *dev = >dev;
> > +
> > + ret = devm_iio_triggered_buffer_setup(dev, indio_dev, NULL,
> > + cros_ec_sensors_capture, NULL);
> > + if (ret)
> > + return ret;
> > +
> > + return devm_iio_device_register(dev, indio_dev);
> > +}
> > +EXPORT_SYMBOL_GPL(cros_ec_sensors_core_register);
> > +
> > int cros_ec_motion_send_host_cmd(struct
Re: [PATCH v3 3/8] iio: common: cros_ec_sensors: move registration to core
On Tue, 18 Jun 2019 11:06:34 +0200
Fabien Lahoudere wrote:
> In order to simplify derivated drivers from cros_ec_sensors_core,
> a new core function is created to registered IIO stricture.
>
> Signed-off-by: Fabien Lahoudere
This one looks good to me.
I'll pick it up once the minor stuff in other patches is sorted.
Thanks,
Jonathan
> ---
> .../common/cros_ec_sensors/cros_ec_sensors.c | 9 +-
> .../cros_ec_sensors/cros_ec_sensors_core.c| 97 ---
> drivers/iio/light/cros_ec_light_prox.c| 7 +-
> drivers/iio/pressure/cros_ec_baro.c | 7 +-
> .../linux/iio/common/cros_ec_sensors_core.h | 16 ++-
> 5 files changed, 72 insertions(+), 64 deletions(-)
>
> diff --git a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c
> b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c
> index 897dc83a3355..c4bee9265246 100644
> --- a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c
> +++ b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c
> @@ -14,7 +14,7 @@
> #include
> #include
> #include
> -#include
> +
> #include
> #include
> #include
> @@ -233,12 +233,7 @@ static int cros_ec_sensors_probe(struct platform_device
> *pdev)
> else
> state->core.read_ec_sensors_data = cros_ec_sensors_read_cmd;
>
> - ret = devm_iio_triggered_buffer_setup(dev, indio_dev, NULL,
> - cros_ec_sensors_capture, NULL);
> - if (ret)
> - return ret;
> -
> - return devm_iio_device_register(dev, indio_dev);
> + return cros_ec_sensors_core_register(pdev, indio_dev);
> }
>
> static const struct platform_device_id cros_ec_sensors_ids[] = {
> diff --git a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c
> b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c
> index e5181e007dd7..3880849c5cca 100644
> --- a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c
> +++ b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c
> @@ -12,6 +12,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -95,6 +96,67 @@ int cros_ec_sensors_core_init(struct platform_device *pdev,
> }
> EXPORT_SYMBOL_GPL(cros_ec_sensors_core_init);
>
> +/**
> + * cros_ec_sensors_capture() - the trigger handler function
> + * @irq: the interrupt number.
> + * @p: a pointer to the poll function.
> + *
> + * On a trigger event occurring, if the pollfunc is attached then this
> + * handler is called as a threaded interrupt (and hence may sleep). It
> + * is responsible for grabbing data from the device and pushing it into
> + * the associated buffer.
> + *
> + * Return: IRQ_HANDLED
> + */
> +static irqreturn_t cros_ec_sensors_capture(int irq, void *p)
> +{
> + struct iio_poll_func *pf = p;
> + struct iio_dev *indio_dev = pf->indio_dev;
> + struct cros_ec_sensors_core_state *st = iio_priv(indio_dev);
> + int ret;
> +
> + mutex_lock(>cmd_lock);
> +
> + /* Clear capture data. */
> + memset(st->samples, 0, indio_dev->scan_bytes);
> +
> + /* Read data based on which channels are enabled in scan mask. */
> + ret = st->read_ec_sensors_data(indio_dev,
> +*indio_dev->active_scan_mask,
> +(s16 *)st->samples);
> + if (ret < 0)
> + goto done;
> +
> + iio_push_to_buffers_with_timestamp(indio_dev, st->samples,
> +iio_get_time_ns(indio_dev));
> +
> +done:
> + /*
> + * Tell the core we are done with this trigger and ready for the
> + * next one.
> + */
> + iio_trigger_notify_done(indio_dev->trig);
> +
> + mutex_unlock(>cmd_lock);
> +
> + return IRQ_HANDLED;
> +}
> +
> +int cros_ec_sensors_core_register(struct platform_device *pdev,
> + struct iio_dev *indio_dev)
> +{
> + int ret;
> + struct device *dev = >dev;
> +
> + ret = devm_iio_triggered_buffer_setup(dev, indio_dev, NULL,
> + cros_ec_sensors_capture, NULL);
> + if (ret)
> + return ret;
> +
> + return devm_iio_device_register(dev, indio_dev);
> +}
> +EXPORT_SYMBOL_GPL(cros_ec_sensors_core_register);
> +
> int cros_ec_motion_send_host_cmd(struct cros_ec_sensors_core_state *state,
>u16 opt_length)
> {
> @@ -380,41 +442,6 @@ int cros_ec_sensors_read_cmd(struct iio_dev *indio_dev,
> }
> EXPORT_SYMBOL_GPL(cros_ec_sensors_read_cmd);
>
> -irqreturn_t cros_ec_sensors_capture(int irq, void *p)
> -{
> - struct iio_poll_func *pf = p;
> - struct iio_dev *indio_dev = pf->indio_dev;
> - struct cros_ec_sensors_core_state *st = iio_priv(indio_dev);
> - int ret;
> -
> - mutex_lock(>cmd_lock);
> -
> - /* Clear capture data. */
> - memset(st->samples, 0, indio_dev->scan_bytes);
> -
> - /* Read data based on which channels are enabled in scan mask. */
> - ret =
[PATCH v3 3/8] iio: common: cros_ec_sensors: move registration to core
In order to simplify derivated drivers from cros_ec_sensors_core,
a new core function is created to registered IIO stricture.
Signed-off-by: Fabien Lahoudere
---
.../common/cros_ec_sensors/cros_ec_sensors.c | 9 +-
.../cros_ec_sensors/cros_ec_sensors_core.c| 97 ---
drivers/iio/light/cros_ec_light_prox.c| 7 +-
drivers/iio/pressure/cros_ec_baro.c | 7 +-
.../linux/iio/common/cros_ec_sensors_core.h | 16 ++-
5 files changed, 72 insertions(+), 64 deletions(-)
diff --git a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c
b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c
index 897dc83a3355..c4bee9265246 100644
--- a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c
+++ b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c
@@ -14,7 +14,7 @@
#include
#include
#include
-#include
+
#include
#include
#include
@@ -233,12 +233,7 @@ static int cros_ec_sensors_probe(struct platform_device
*pdev)
else
state->core.read_ec_sensors_data = cros_ec_sensors_read_cmd;
- ret = devm_iio_triggered_buffer_setup(dev, indio_dev, NULL,
- cros_ec_sensors_capture, NULL);
- if (ret)
- return ret;
-
- return devm_iio_device_register(dev, indio_dev);
+ return cros_ec_sensors_core_register(pdev, indio_dev);
}
static const struct platform_device_id cros_ec_sensors_ids[] = {
diff --git a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c
b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c
index e5181e007dd7..3880849c5cca 100644
--- a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c
+++ b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c
@@ -12,6 +12,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -95,6 +96,67 @@ int cros_ec_sensors_core_init(struct platform_device *pdev,
}
EXPORT_SYMBOL_GPL(cros_ec_sensors_core_init);
+/**
+ * cros_ec_sensors_capture() - the trigger handler function
+ * @irq: the interrupt number.
+ * @p: a pointer to the poll function.
+ *
+ * On a trigger event occurring, if the pollfunc is attached then this
+ * handler is called as a threaded interrupt (and hence may sleep). It
+ * is responsible for grabbing data from the device and pushing it into
+ * the associated buffer.
+ *
+ * Return: IRQ_HANDLED
+ */
+static irqreturn_t cros_ec_sensors_capture(int irq, void *p)
+{
+ struct iio_poll_func *pf = p;
+ struct iio_dev *indio_dev = pf->indio_dev;
+ struct cros_ec_sensors_core_state *st = iio_priv(indio_dev);
+ int ret;
+
+ mutex_lock(>cmd_lock);
+
+ /* Clear capture data. */
+ memset(st->samples, 0, indio_dev->scan_bytes);
+
+ /* Read data based on which channels are enabled in scan mask. */
+ ret = st->read_ec_sensors_data(indio_dev,
+ *indio_dev->active_scan_mask,
+ (s16 *)st->samples);
+ if (ret < 0)
+ goto done;
+
+ iio_push_to_buffers_with_timestamp(indio_dev, st->samples,
+ iio_get_time_ns(indio_dev));
+
+done:
+ /*
+* Tell the core we are done with this trigger and ready for the
+* next one.
+*/
+ iio_trigger_notify_done(indio_dev->trig);
+
+ mutex_unlock(>cmd_lock);
+
+ return IRQ_HANDLED;
+}
+
+int cros_ec_sensors_core_register(struct platform_device *pdev,
+ struct iio_dev *indio_dev)
+{
+ int ret;
+ struct device *dev = >dev;
+
+ ret = devm_iio_triggered_buffer_setup(dev, indio_dev, NULL,
+ cros_ec_sensors_capture, NULL);
+ if (ret)
+ return ret;
+
+ return devm_iio_device_register(dev, indio_dev);
+}
+EXPORT_SYMBOL_GPL(cros_ec_sensors_core_register);
+
int cros_ec_motion_send_host_cmd(struct cros_ec_sensors_core_state *state,
u16 opt_length)
{
@@ -380,41 +442,6 @@ int cros_ec_sensors_read_cmd(struct iio_dev *indio_dev,
}
EXPORT_SYMBOL_GPL(cros_ec_sensors_read_cmd);
-irqreturn_t cros_ec_sensors_capture(int irq, void *p)
-{
- struct iio_poll_func *pf = p;
- struct iio_dev *indio_dev = pf->indio_dev;
- struct cros_ec_sensors_core_state *st = iio_priv(indio_dev);
- int ret;
-
- mutex_lock(>cmd_lock);
-
- /* Clear capture data. */
- memset(st->samples, 0, indio_dev->scan_bytes);
-
- /* Read data based on which channels are enabled in scan mask. */
- ret = st->read_ec_sensors_data(indio_dev,
- *(indio_dev->active_scan_mask),
- (s16 *)st->samples);
- if (ret < 0)
- goto done;
-
- iio_push_to_buffers_with_timestamp(indio_dev, st->samples,
- iio_get_time_ns(indio_dev));
-
-done:
-
