From: Guenter Roeck <groeck@google.com>
To: Jonathan Cameron <jic23@kernel.org>
Cc: Alexandru Ardelean <alexandru.ardelean@analog.com>,
linux-iio@vger.kernel.org,
linux-kernel <linux-kernel@vger.kernel.org>,
Benson Leung <bleung@chromium.org>,
Gwendal Grignou <gwendal@chromium.org>,
Enric Balletbo i Serra <enric.balletbo@collabora.com>,
Guenter Roeck <groeck@chromium.org>
Subject: Re: [PATCH] iio: cros_ec: unify hw fifo attributes into the core file
Date: Fri, 25 Sep 2020 08:26:23 -0700 [thread overview]
Message-ID: <CABXOdTdr52ikhoSEMqqFakO42hZzpvtH=rxkAYWQsExrnWodFA@mail.gmail.com> (raw)
In-Reply-To: <20200925133301.4789c47a@archlinux>
On Fri, Sep 25, 2020 at 5:33 AM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Wed, 23 Sep 2020 16:03:39 +0300
> Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:
>
> > The intent here is to minimize the use of iio_buffer_set_attrs(). Since we
> > are planning to add support for multiple IIO buffers per IIO device, the
> > issue has to do with:
> > 1. Accessing 'indio_dev->buffer' directly (as is done with
> > 'iio_buffer_set_attrs(indio_dev->buffer, <attrs>)').
> > 2. The way that the buffer attributes would get handled or expanded when
> > there are more buffers per IIO device. Current a sysfs kobj_type expands
> > into a 'device' object that expands into an 'iio_dev' object.
> > We will need to change this, so that the sysfs attributes for IIO
> > buffers expand into IIO buffers at some point.
> >
> > Right now, the current IIO framework works fine for the
> > '1 IIO device == 1 IIO buffer' case (that is now).
> >
> > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> Looks good to me, but I'll need a cros_ec ack for this one.
>
I like it.
Reviewed-by: Guenter Roeck <groeck@chromium.org>
Guenter
> thanks
>
> Jonathan
>
> > ---
> > drivers/iio/accel/cros_ec_accel_legacy.c | 2 +-
> > .../iio/common/cros_ec_sensors/cros_ec_lid_angle.c | 3 ++-
> > drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c | 5 ++---
> > .../iio/common/cros_ec_sensors/cros_ec_sensors_core.c | 11 ++++++++---
> > drivers/iio/light/cros_ec_light_prox.c | 5 ++---
> > drivers/iio/pressure/cros_ec_baro.c | 5 ++---
> > include/linux/iio/common/cros_ec_sensors_core.h | 4 ++--
> > 7 files changed, 19 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/iio/accel/cros_ec_accel_legacy.c b/drivers/iio/accel/cros_ec_accel_legacy.c
> > index b6f3471b62dc..8f1232c38e0d 100644
> > --- a/drivers/iio/accel/cros_ec_accel_legacy.c
> > +++ b/drivers/iio/accel/cros_ec_accel_legacy.c
> > @@ -215,7 +215,7 @@ static int cros_ec_accel_legacy_probe(struct platform_device *pdev)
> > return -ENOMEM;
> >
> > ret = cros_ec_sensors_core_init(pdev, indio_dev, true,
> > - cros_ec_sensors_capture, NULL);
> > + cros_ec_sensors_capture, NULL, false);
> > if (ret)
> > return ret;
> >
> > diff --git a/drivers/iio/common/cros_ec_sensors/cros_ec_lid_angle.c b/drivers/iio/common/cros_ec_sensors/cros_ec_lid_angle.c
> > index af801e203623..752f59037715 100644
> > --- a/drivers/iio/common/cros_ec_sensors/cros_ec_lid_angle.c
> > +++ b/drivers/iio/common/cros_ec_sensors/cros_ec_lid_angle.c
> > @@ -97,7 +97,8 @@ static int cros_ec_lid_angle_probe(struct platform_device *pdev)
> > if (!indio_dev)
> > return -ENOMEM;
> >
> > - ret = cros_ec_sensors_core_init(pdev, indio_dev, false, NULL, NULL);
> > + ret = cros_ec_sensors_core_init(pdev, indio_dev, false, NULL,
> > + NULL, false);
> > if (ret)
> > return ret;
> >
> > 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 130ab8ce0269..57038ca48d93 100644
> > --- a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c
> > +++ b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c
> > @@ -236,12 +236,11 @@ static int cros_ec_sensors_probe(struct platform_device *pdev)
> >
> > ret = cros_ec_sensors_core_init(pdev, indio_dev, true,
> > cros_ec_sensors_capture,
> > - cros_ec_sensors_push_data);
> > + cros_ec_sensors_push_data,
> > + true);
> > if (ret)
> > return ret;
> >
> > - iio_buffer_set_attrs(indio_dev->buffer, cros_ec_sensor_fifo_attributes);
> > -
> > indio_dev->info = &ec_sensors_info;
> > state = iio_priv(indio_dev);
> > for (channel = state->channels, i = CROS_EC_SENSOR_X;
> > 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 ea480c1d4349..0de800d41978 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
> > @@ -174,12 +174,11 @@ static ssize_t hwfifo_watermark_max_show(struct device *dev,
> >
> > static IIO_DEVICE_ATTR_RO(hwfifo_watermark_max, 0);
> >
> > -const struct attribute *cros_ec_sensor_fifo_attributes[] = {
> > +static const struct attribute *cros_ec_sensor_fifo_attributes[] = {
> > &iio_dev_attr_hwfifo_timeout.dev_attr.attr,
> > &iio_dev_attr_hwfifo_watermark_max.dev_attr.attr,
> > NULL,
> > };
> > -EXPORT_SYMBOL_GPL(cros_ec_sensor_fifo_attributes);
> >
> > int cros_ec_sensors_push_data(struct iio_dev *indio_dev,
> > s16 *data,
> > @@ -238,6 +237,7 @@ static void cros_ec_sensors_core_clean(void *arg)
> > * for backward compatibility.
> > * @push_data: function to call when cros_ec_sensorhub receives
> > * a sample for that sensor.
> > + * @has_hw_fifo: Set true if this device has/uses a HW FIFO
> > *
> > * Return: 0 on success, -errno on failure.
> > */
> > @@ -245,7 +245,8 @@ int cros_ec_sensors_core_init(struct platform_device *pdev,
> > struct iio_dev *indio_dev,
> > bool physical_device,
> > cros_ec_sensors_capture_t trigger_capture,
> > - cros_ec_sensorhub_push_data_cb_t push_data)
> > + cros_ec_sensorhub_push_data_cb_t push_data,
> > + bool has_hw_fifo)
> > {
> > struct device *dev = &pdev->dev;
> > struct cros_ec_sensors_core_state *state = iio_priv(indio_dev);
> > @@ -358,6 +359,10 @@ int cros_ec_sensors_core_init(struct platform_device *pdev,
> > NULL);
> > if (ret)
> > return ret;
> > +
> > + if (has_hw_fifo)
> > + iio_buffer_set_attrs(indio_dev->buffer,
> > + cros_ec_sensor_fifo_attributes);
> > }
> > }
> >
> > diff --git a/drivers/iio/light/cros_ec_light_prox.c b/drivers/iio/light/cros_ec_light_prox.c
> > index fed79ba27fda..75d6b5fcf2cc 100644
> > --- a/drivers/iio/light/cros_ec_light_prox.c
> > +++ b/drivers/iio/light/cros_ec_light_prox.c
> > @@ -182,12 +182,11 @@ static int cros_ec_light_prox_probe(struct platform_device *pdev)
> >
> > ret = cros_ec_sensors_core_init(pdev, indio_dev, true,
> > cros_ec_sensors_capture,
> > - cros_ec_sensors_push_data);
> > + cros_ec_sensors_push_data,
> > + true);
> > if (ret)
> > return ret;
> >
> > - iio_buffer_set_attrs(indio_dev->buffer, cros_ec_sensor_fifo_attributes);
> > -
> > indio_dev->info = &cros_ec_light_prox_info;
> > state = iio_priv(indio_dev);
> > state->core.type = state->core.resp->info.type;
> > diff --git a/drivers/iio/pressure/cros_ec_baro.c b/drivers/iio/pressure/cros_ec_baro.c
> > index f0938b6fbba0..aa043cb9ac42 100644
> > --- a/drivers/iio/pressure/cros_ec_baro.c
> > +++ b/drivers/iio/pressure/cros_ec_baro.c
> > @@ -139,12 +139,11 @@ static int cros_ec_baro_probe(struct platform_device *pdev)
> >
> > ret = cros_ec_sensors_core_init(pdev, indio_dev, true,
> > cros_ec_sensors_capture,
> > - cros_ec_sensors_push_data);
> > + cros_ec_sensors_push_data,
> > + true);
> > if (ret)
> > return ret;
> >
> > - iio_buffer_set_attrs(indio_dev->buffer, cros_ec_sensor_fifo_attributes);
> > -
> > indio_dev->info = &cros_ec_baro_info;
> > state = iio_priv(indio_dev);
> > state->core.type = state->core.resp->info.type;
> > diff --git a/include/linux/iio/common/cros_ec_sensors_core.h b/include/linux/iio/common/cros_ec_sensors_core.h
> > index caa8bb279a34..c9b80be82440 100644
> > --- a/include/linux/iio/common/cros_ec_sensors_core.h
> > +++ b/include/linux/iio/common/cros_ec_sensors_core.h
> > @@ -96,7 +96,8 @@ struct platform_device;
> > int cros_ec_sensors_core_init(struct platform_device *pdev,
> > struct iio_dev *indio_dev, bool physical_device,
> > cros_ec_sensors_capture_t trigger_capture,
> > - cros_ec_sensorhub_push_data_cb_t push_data);
> > + cros_ec_sensorhub_push_data_cb_t push_data,
> > + bool has_hw_fifo);
> >
> > irqreturn_t cros_ec_sensors_capture(int irq, void *p);
> > int cros_ec_sensors_push_data(struct iio_dev *indio_dev,
> > @@ -125,6 +126,5 @@ extern const struct dev_pm_ops cros_ec_sensors_pm_ops;
> >
> > /* List of extended channel specification for all sensors. */
> > extern const struct iio_chan_spec_ext_info cros_ec_sensors_ext_info[];
> > -extern const struct attribute *cros_ec_sensor_fifo_attributes[];
> >
> > #endif /* __CROS_EC_SENSORS_CORE_H */
>
next prev parent reply other threads:[~2020-09-25 15:26 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-09-23 13:03 [PATCH] iio: cros_ec: unify hw fifo attributes into the core file Alexandru Ardelean
2020-09-25 12:33 ` Jonathan Cameron
2020-09-25 15:26 ` Guenter Roeck [this message]
2020-09-26 15:04 ` Jonathan Cameron
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CABXOdTdr52ikhoSEMqqFakO42hZzpvtH=rxkAYWQsExrnWodFA@mail.gmail.com' \
--to=groeck@google.com \
--cc=alexandru.ardelean@analog.com \
--cc=bleung@chromium.org \
--cc=enric.balletbo@collabora.com \
--cc=groeck@chromium.org \
--cc=gwendal@chromium.org \
--cc=jic23@kernel.org \
--cc=linux-iio@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).