| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <8caaa1f8-ebbe-50ab-a655-d5c487c9a751@microchip.com>
Date: Wed, 17 Jun 2020 13:25:43 +0000
From: <Eugen.Hristev@...rochip.com>
To: <jic23@...nel.org>, <alexandru.ardelean@...log.com>
CC: <linux-iio@...r.kernel.org>,
<linux-arm-kernel@...ts.infradead.org>,
<linux-kernel@...r.kernel.org>, <alexandre.belloni@...tlin.com>,
<Nicolas.Ferre@...rochip.com>, <Ludovic.Desroches@...rochip.com>
Subject: Re: [PATCH] iio: at91-sama5d2_adc: remove usage of iio_priv_to_dev()
helper
On 31.05.2020 17:39, Jonathan Cameron wrote:
> On Mon, 25 May 2020 13:53:41 +0300
> Alexandru Ardelean <alexandru.ardelean@...log.com> 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 ?)
Eugen
>
> Poke me if no action in a few weeks.
>
> Thanks,
>
> Jonathan
>
>>
>> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@...log.com>
>> ---
>> 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))
>
Powered by blists - more mailing lists