On Thu, 18 Jun 2020 13:34:13 +0000
"Ardelean, Alexandru" [off-list ref] wrote:

On Thu, 2020-06-18 at 12:47 +0000, Eugen.Hristev@microchip.com wrote:

quoted

[External]

On 17.06.2020 17:02, Ardelean, Alexandru wrote:  

quoted

On Wed, 2020-06-17 at 13:25 +0000, Eugen.Hristev@microchip.com wrote:  

quoted

On 31.05.2020 17:39, Jonathan Cameron wrote:
  

quoted

On Mon, 25 May 2020 13:53:41 +0300
Alexandru Ardelean [off-list ref] wrote:
  

quoted

We may want to get rid of the iio_priv_to_dev() helper. The
reason is that
we will hide some of the members of the iio_dev structure (to
prevent
drivers from accessing them directly), and that will also mean
hiding the
implementation of the iio_priv_to_dev() helper inside the IIO
core.

Hiding the implementation of iio_priv_to_dev() implies that some
fast-
paths
may not be fast anymore, so a general idea is to try to get rid
of the
iio_priv_to_dev() altogether.
The iio_priv() helper won't be affected by the rework, as the
iio_dev
struct will keep a reference to the private information.

For this driver, not using iio_priv_to_dev(), means reworking
some paths
to
pass the iio device and using iio_priv() to access the private
information,
and also keeping a reference to the iio device for some quirky
paths.

One [quirky] path is the at91_adc_workq_handler() which requires
the IIO
device & the state struct to push to buffers.
Since this requires the back-ref to the IIO device, the
at91_adc_touch_pos() also uses it. This simplifies the patch a
bit. The
information required in this function is mostly for debugging
purposes.
Replacing it with a reference to the IIO device would have been a
slightly
bigger change, which may not be worth it (for just the debugging
purpose
and given that we need the back-ref to the IIO device anyway).  

That workq is indeed quirky.  This looks fine to me in general.
I'll
want an appropriate ack from the at91 side of things if possible so
let's leave this on the list for a while longer.  

Hi,

I am available to test this patch,
Can you tell me on which branch to apply it. On 5.8-rc1 it fails for
me
(or maybe it needs rebasing ?)
  

Hmm, weird.
I rebased the patches on Jonathan's iio/testing.
It seemed to work.
https://urldefense.com/v3/__https://github.com/commodo/linux/commits/iio-priv-to-dev__;!!A3Ni8CS0y2Y!oEVHsA6gf9ydBvAAjlhRV5QO1bMTZN2U_OXbor0gei7mWk14m64rilJ2WTAXvtWmGaisXQ$ 

As for which branch to test/apply. Not sure.
Maybe Jonathan's iio/testing as base?
Looks like it's based on 5.8.
  

Tested-by: Eugen Hristev <redacted>

I have tested the major features of the driver (including the resistive 
touch) and it looks to be working fine.

I did not fully understand the quirkyness of the workq . Something you 
want to change to that ?  

Umm, not really.
I did not follow that code too in-depth.
And I would like not to.
Mostly to avoid scaling myself in too many directions.

There may be a slightly better approach to it, but ¯\_(ツ)_/¯

I'm assuming no one minds me taking this as is.

Applied to the togreg branch of iio.git first pushed out as testing
for the autobuilders to play with it.

Thanks,

Jonathan

quoted

quoted

quoted

Eugen
  

quoted

Poke me if no action in a few weeks.

Thanks,

Jonathan
  

quoted

Signed-off-by: Alexandru Ardelean <redacted>
---
   drivers/iio/adc/at91-sama5d2_adc.c | 30 +++++++++++++++++-----
--------
   1 file changed, 17 insertions(+), 13 deletions(-)

diff --git a/drivers/iio/adc/at91-sama5d2_adc.c

b/drivers/iio/adc/at91-
sama5d2_adc.c
index 9abbbdcc7420..7bce18444430 100644

--- a/drivers/iio/adc/at91-sama5d2_adc.c
+++ b/drivers/iio/adc/at91-sama5d2_adc.c

@@ -402,6 +402,7 @@ struct at91_adc_state {
        wait_queue_head_t               wq_data_available;
        struct at91_adc_dma             dma_st;
        struct at91_adc_touch           touch_st;
+     struct iio_dev                  *indio_dev;
        u16                             buffer[AT91_BUFFER_MAX_HW

ORDS];
        /*
         * lock to prevent concurrent 'single conversion'
requests through

@@ -642,13 +643,13 @@ static u16 at91_adc_touch_pos(struct

at91_adc_state
*st, int reg)
        /* first half of register is the x or y, second half is
the scale
*/
        val = at91_adc_readl(st, reg);
        if (!val)
-             dev_dbg(&iio_priv_to_dev(st)->dev, "pos is 0\n");
+             dev_dbg(&st->indio_dev->dev, "pos is 0\n");

        pos = val & AT91_SAMA5D2_XYZ_MASK;
        result = (pos << AT91_SAMA5D2_MAX_POS_BITS) - pos;
        scale = (val >> 16) & AT91_SAMA5D2_XYZ_MASK;
        if (scale == 0) {
-             dev_err(&iio_priv_to_dev(st)->dev, "scale is 0\n");
+             dev_err(&st->indio_dev->dev, "scale is 0\n");
                return 0;
        }
        result /= scale;

@@ -1204,9 +1205,9 @@ static unsigned

at91_adc_startup_time(unsigned
startup_time_min,
        return i;
   }

-static void at91_adc_setup_samp_freq(struct at91_adc_state *st,
unsigned
freq)
+static void at91_adc_setup_samp_freq(struct iio_dev *indio_dev,
unsigned
freq)
   {
-     struct iio_dev *indio_dev = iio_priv_to_dev(st);
+     struct at91_adc_state *st = iio_priv(indio_dev);
        unsigned f_per, prescal, startup, mr;

        f_per = clk_get_rate(st->per_clk);

@@ -1275,9 +1276,9 @@ static void

at91_adc_pen_detect_interrupt(struct
at91_adc_state *st)
        st->touch_st.touching = true;
   }

-static void at91_adc_no_pen_detect_interrupt(struct
at91_adc_state *st)
+static void at91_adc_no_pen_detect_interrupt(struct iio_dev
*indio_dev)
   {
-     struct iio_dev *indio_dev = iio_priv_to_dev(st);
+     struct at91_adc_state *st = iio_priv(indio_dev);

        at91_adc_writel(st, AT91_SAMA5D2_TRGR,
                        AT91_SAMA5D2_TRGR_TRGMOD_NO_TRIGGER);

@@ -1297,7 +1298,7 @@ static void at91_adc_workq_handler(struct

work_struct *workq)
                                        struct at91_adc_touch,
workq);
        struct at91_adc_state *st = container_of(touch_st,
                                        struct at91_adc_state,
touch_st);
-     struct iio_dev *indio_dev = iio_priv_to_dev(st);
+     struct iio_dev *indio_dev = st->indio_dev;

        iio_push_to_buffers(indio_dev, st->buffer);
   }

@@ -1318,7 +1319,7 @@ static irqreturn_t at91_adc_interrupt(int

irq, void
*private)
                at91_adc_pen_detect_interrupt(st);
        } else if ((status & AT91_SAMA5D2_IER_NOPEN)) {
                /* nopen detected IRQ */
-             at91_adc_no_pen_detect_interrupt(st);
+             at91_adc_no_pen_detect_interrupt(indio);
        } else if ((status & AT91_SAMA5D2_ISR_PENS) &&
                   ((status & rdy_mask) == rdy_mask)) {
                /* periodic trigger IRQ - during pen sense */

@@ -1486,7 +1487,7 @@ static int at91_adc_write_raw(struct

iio_dev
*indio_dev,
                    val > st->soc_info.max_sample_rate)
                        return -EINVAL;

-             at91_adc_setup_samp_freq(st, val);
+             at91_adc_setup_samp_freq(indio_dev, val);
                return 0;
        default:
                return -EINVAL;

@@ -1624,8 +1625,10 @@ static int

at91_adc_update_scan_mode(struct iio_dev
*indio_dev,
        return 0;
   }

-static void at91_adc_hw_init(struct at91_adc_state *st)
+static void at91_adc_hw_init(struct iio_dev *indio_dev)
   {
+     struct at91_adc_state *st = iio_priv(indio_dev);
+
        at91_adc_writel(st, AT91_SAMA5D2_CR,
AT91_SAMA5D2_CR_SWRST);
        at91_adc_writel(st, AT91_SAMA5D2_IDR, 0xffffffff);
        /*

@@ -1635,7 +1638,7 @@ static void at91_adc_hw_init(struct

at91_adc_state
*st)
        at91_adc_writel(st, AT91_SAMA5D2_MR,
                        AT91_SAMA5D2_MR_TRANSFER(2) |
AT91_SAMA5D2_MR_ANACH);

-     at91_adc_setup_samp_freq(st, st->soc_info.min_sample_rate);
+     at91_adc_setup_samp_freq(indio_dev, st-  

quoted

soc_info.min_sample_rate);  

        /* configure extended mode register */
        at91_adc_config_emr(st);

@@ -1718,6 +1721,7 @@ static int at91_adc_probe(struct

platform_device
*pdev)
        indio_dev->num_channels = ARRAY_SIZE(at91_adc_channels);

        st = iio_priv(indio_dev);
+     st->indio_dev = indio_dev;

        bitmap_set(&st->touch_st.channels_bitmask,
                   AT91_SAMA5D2_TOUCH_X_CHAN_IDX, 1);

@@ -1829,7 +1833,7 @@ static int at91_adc_probe(struct

platform_device
*pdev)
                goto vref_disable;
        }

-     at91_adc_hw_init(st);
+     at91_adc_hw_init(indio_dev);

        ret = clk_prepare_enable(st->per_clk);
        if (ret)

@@ -1945,7 +1949,7 @@ static __maybe_unused int

at91_adc_resume(struct
device *dev)
        if (ret)
                goto vref_disable_resume;

-     at91_adc_hw_init(st);
+     at91_adc_hw_init(indio_dev);

        /* reconfiguring trigger hardware state */
        if (!iio_buffer_enabled(indio_dev))  

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel