linux-kernel - Re: [PATCH v2 2/2] iio: adc: max14001: New driver

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 6 Jun 2023 11:35:50 +0100
From:   Jonathan Cameron <Jonathan.Cameron@...wei.com>
To:     "Paller, Kim Seer" <KimSeer.Paller@...log.com>
CC:     Jonathan Cameron <jic23@...nel.org>,
        "lars@...afoo.de" <lars@...afoo.de>,
        "krzysztof.kozlowski@...aro.org" <krzysztof.kozlowski@...aro.org>,
        "broonie@...nel.org" <broonie@...nel.org>,
        "lgirdwood@...il.com" <lgirdwood@...il.com>,
        "linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 2/2] iio: adc: max14001: New driver



> >   
> > > +	struct mutex		lock;
> > > +	struct regmap		*regmap;
> > > +	int			vref_mv;
> > > +	/*
> > > +	 * DMA (thus cache coherency maintenance) requires the
> > > +	 * transfer buffers to live in their own cache lines.  
> > 
> > You are looking at an old kernel I guess - we fixed all of these - and
> > introduced IIO_DMA_MINALIGN for __aligned(IIO_DMA_MINALIGN) to
> > make it easier to fix any such problems in future.
> > 
> > Upshot is that ___cacheline_aligned aligns to the l1 cacheline length.
> > Some fun systems (such as the big servers I use in my dayjob) have higher
> > cacheline sizes for their larger / further from CPU caches.
> > One group of SoCs out there is known to both do non coherent DMA and
> > have a larger line size for the bit relevant to that than ___cacheline_aligned
> > gives you. So on that rare platform this is currently broken.  
> 
> It's good to know. Given this information, is there anything specific that I 
> need to change in the code or implementation related to 
> the ___cacheline_aligned part?

Replace it with __aligned(IIO_DMA_MINALIGN) as has hopefully now been done
in all upstream drivers.

> >   

> > > +}
> > > +
> > > +static int max14001_reg_update(struct max14001_state *st,
> > > +				unsigned int reg_addr,
> > > +				unsigned int mask,
> > > +				unsigned int val)
> > > +{
> > > +	int ret;
> > > +
> > > +	/* Enable SPI Registers Write */
> > > +	ret = max14001_write(st, MAX14001_WEN,  
> > MAX14001_WRITE_WEN);
> > 
> > Mixing regmap and non regmap rather defeats the point of having a standard
> > interface.  Use regmap_read and regmap_write throughout or not at all.  
> 
> I found it difficult to implement the regmap interface due to the timing diagram 
> requirements. The chip select needs to be changed between transfers, which, 
> as far as I know, does not work with regmap. Perhaps, I will consider sticking 
> to the non-regmap approach.

That may be sensible if there are odd requirements or just call regmap_write()
which will call your max14001_write() anyway and opencode the timing
requirements etc by multiple remap calls.  Obviously benefits of regmap reduced
though so may not be worth bothering unless it is worth using the caching or similar.

Jonathan