Re: [PATCH] iio: at91-sama5d2_adc: remove usage of iio_priv_to_dev() helper

From: Eugen.Hristev
Date: Thu Jun 18 2020 - 08:47:59 EST

Next message: zhaowuyun: "[PATCH] f2fs-tools: set cold flag for non-dir node"
Previous message: Vivek Goyal: "Re: [PATCH 2/3] kvm: Add capability to be able to report async pf error to guest"
In reply to: Ardelean, Alexandru: "Re: [PATCH] iio: at91-sama5d2_adc: remove usage of iio_priv_to_dev() helper"
Next in thread: Ardelean, Alexandru: "Re: [PATCH] iio: at91-sama5d2_adc: remove usage of iio_priv_to_dev() helper"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On 17.06.2020 17:02, Ardelean, Alexandru wrote:
> On Wed, 2020-06-17 at 13:25 +0000, Eugen.Hristev@xxxxxxxxxxxxx wrote:
>> On 31.05.2020 17:39, Jonathan Cameron wrote:
>>
>>> On Mon, 25 May 2020 13:53:41 +0300
>>> Alexandru Ardelean <alexandru.ardelean@xxxxxxxxxx> wrote:
>>>
>>>> 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://github.com/commodo/linux/commits/iio-priv-to-dev
>
> 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 <eugen.hristev@xxxxxxxxxxxxx>

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 ?

>
>> Eugen
>>
>>> Poke me if no action in a few weeks.
>>>
>>> Thanks,
>>>
>>> Jonathan
>>>
>>>> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@xxxxxxxxxx>
>>>> ---
>>>> 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_HWORDS];
>>>> /*
>>>> * 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->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))

Next message: zhaowuyun: "[PATCH] f2fs-tools: set cold flag for non-dir node"
Previous message: Vivek Goyal: "Re: [PATCH 2/3] kvm: Add capability to be able to report async pf error to guest"
In reply to: Ardelean, Alexandru: "Re: [PATCH] iio: at91-sama5d2_adc: remove usage of iio_priv_to_dev() helper"
Next in thread: Ardelean, Alexandru: "Re: [PATCH] iio: at91-sama5d2_adc: remove usage of iio_priv_to_dev() helper"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]