From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-12.7 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id B0FFAC432BE for ; Mon, 2 Aug 2021 14:22:59 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 9CFDB60EC0 for ; Mon, 2 Aug 2021 14:22:59 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S236306AbhHBOXH (ORCPT ); Mon, 2 Aug 2021 10:23:07 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:45706 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S236175AbhHBOW5 (ORCPT ); Mon, 2 Aug 2021 10:22:57 -0400 Received: from mail-qt1-x836.google.com (mail-qt1-x836.google.com [IPv6:2607:f8b0:4864:20::836]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 45B8AC07720F; Mon, 2 Aug 2021 07:10:49 -0700 (PDT) Received: by mail-qt1-x836.google.com with SMTP id d2so11687823qto.6; Mon, 02 Aug 2021 07:10:49 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:content-transfer-encoding:date:message-id:from:to:cc :subject:references:in-reply-to; bh=2X+kvVIkTXeU3G/rutGby/mSqVSEzt5UnQ7WKMI/d2I=; b=Xv2UKFyUvbz+5h8eFQvVKZN36IG/7D4mrKmUQCy73IgqhpOtwtYtLZl3PP07PCRFg4 ctwlRebDWPg4r2+TxBOIibZjguq+pWw7AwqgQpu/egFKCK3udg0R4WYHG3pQb9lTbxuP Rs4IbvJivNxX248IjAESaoh4nioL23y8kdZRW3Fsm2sTUvGnWqwKrZO5l0YZB85rzg0s GoqYu/dQEGpGUgQMIayKaZWLLfgAkHG5nL7LKLBuPjvjwB7eEs8F9Gm4xtcFKplygcZN le9O4r061wb4KEHMoiGefmluIhrkgXvJ/UT/TqXYUsEwbg935XPFMIIr+M39IKTWGmhB 6R/Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:content-transfer-encoding:date :message-id:from:to:cc:subject:references:in-reply-to; bh=2X+kvVIkTXeU3G/rutGby/mSqVSEzt5UnQ7WKMI/d2I=; b=IhngyNvYyT0b0BBM1MmeNGw1QTVGhECSxEt+6PN1PXT/Wh0Yo+FIjNK7yRzVyWWvx3 TDIrH+WNFIM1+6EEI9M3CDpM70XCmz66fkrKj487AXd829Pfiwsr08GqAszxvVmrkINs ktuaE1vnAqGkeBkhT/MfxV0tJ3iPFRNXrKpB6jEKKGkLO/FcyqHsSY1t8fqxA+2hchc1 IuT+GfpQ/q+kyyZmMfeXKr5vgEIFcoD/vMDWXZsHdkYKroBYVPyPq4qGtuWvKg0Abpz1 MpVDJTg60niDi2RCcaex6djChunCczN1Jlx0I2DNKjgMyESvhweE3MSBmrVkdyjrKfUF dRYg== X-Gm-Message-State: AOAM5305Xn6QDXKm8alNqM0jx39A8/zz/NSBrx41zNgqV8yCyAfCClqe fmCvkN/6r/1kBbH/LMOFUmc= X-Google-Smtp-Source: ABdhPJz9VTtHXR8ZvRkE9nbzG8r6HlFBgzDOgT4z+BCx8yj0ke68eM7zetabzXbvrZ7pIsG23m423Q== X-Received: by 2002:a05:622a:2cc:: with SMTP id a12mr14339747qtx.115.1627913448421; Mon, 02 Aug 2021 07:10:48 -0700 (PDT) Received: from localhost (198-48-202-89.cpe.pppoe.ca. [198.48.202.89]) by smtp.gmail.com with ESMTPSA id m19sm4617639qtx.84.2021.08.02.07.10.47 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 02 Aug 2021 07:10:47 -0700 (PDT) Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=UTF-8 Date: Mon, 02 Aug 2021 10:10:46 -0400 Message-Id: From: "Liam Beguin" To: "Jonathan Cameron" Cc: "Jonathan Cameron" , , , , , , , , Subject: Re: [PATCH v4 2/5] iio: adc: ad7949: fix spi messages on non 14-bit controllers References: <20210727232906.980769-1-liambeguin@gmail.com> <20210727232906.980769-3-liambeguin@gmail.com> <20210731152921.2fcb53ab@jic23-huawei> <20210731155228.5cf77479@jic23-huawei> <20210802112013.00000ff4@Huawei.com> In-Reply-To: <20210802112013.00000ff4@Huawei.com> Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon Aug 2, 2021 at 6:20 AM EDT, Jonathan Cameron wrote: > On Sun, 01 Aug 2021 19:01:10 -0400 > "Liam Beguin" wrote: > > > On Sat Jul 31, 2021 at 10:52 AM EDT, Jonathan Cameron wrote: > > > On Sat, 31 Jul 2021 15:29:21 +0100 > > > Jonathan Cameron wrote: > > > =20 > > > > On Tue, 27 Jul 2021 19:29:03 -0400 > > > > Liam Beguin wrote: > > > > =20 > > > > > From: Liam Beguin > > > > >=20 > > > > > This driver supports devices with 14-bit and 16-bit sample sizes. > > > > > This is not always handled properly by spi controllers and can fa= il. To > > > > > work around this limitation, pad samples to 16-bit and split the = sample > > > > > into two 8-bit messages in the event that only 8-bit messages are > > > > > supported by the controller. > > > > >=20 > > > > > Signed-off-by: Liam Beguin > > > > > --- > > > > > drivers/iio/adc/ad7949.c | 62 ++++++++++++++++++++++++++++++++++= ------ > > > > > 1 file changed, 54 insertions(+), 8 deletions(-) > > > > >=20 > > > > > diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c > > > > > index 0b549b8bd7a9..f1702c54c8be 100644 > > > > > --- a/drivers/iio/adc/ad7949.c > > > > > +++ b/drivers/iio/adc/ad7949.c > > > > > @@ -67,6 +67,7 @@ static const struct ad7949_adc_spec ad7949_adc_= spec[] =3D { > > > > > * @indio_dev: reference to iio structure > > > > > * @spi: reference to spi structure > > > > > * @resolution: resolution of the chip > > > > > + * @bits_per_word: number of bits per SPI word > > > > > * @cfg: copy of the configuration register > > > > > * @current_channel: current channel in use > > > > > * @buffer: buffer to send / receive data to / from device > > > > > @@ -77,6 +78,7 @@ struct ad7949_adc_chip { > > > > > struct iio_dev *indio_dev; > > > > > struct spi_device *spi; > > > > > u8 resolution; > > > > > + u8 bits_per_word; > > > > > u16 cfg; > > > > > unsigned int current_channel; > > > > > u16 buffer ____cacheline_aligned; > > > > > @@ -86,19 +88,34 @@ static int ad7949_spi_write_cfg(struct ad7949= _adc_chip *ad7949_adc, u16 val, > > > > > u16 mask) > > > > > { > > > > > int ret; > > > > > - int bits_per_word =3D ad7949_adc->resolution; > > > > > - int shift =3D bits_per_word - AD7949_CFG_REG_SIZE_BITS; =20 > > > >=20 > > > > The define for this was removed in patch 1. I'll fix that up whils= t applying by > > > > keeping it until this patch. Please check build passes on intermed= iate points > > > > during a patch series as otherwise we may break bisectability and t= hat's really > > > > annoying if you are bisecting! > > > >=20 > > > > Jonathan > > > > =20 > > > > > struct spi_message msg; > > > > > struct spi_transfer tx[] =3D { > > > > > { > > > > > .tx_buf =3D &ad7949_adc->buffer, > > > > > .len =3D 2, > > > > > - .bits_per_word =3D bits_per_word, > > > > > + .bits_per_word =3D ad7949_adc->bits_per_word, > > > > > }, > > > > > }; > > > > > =20 > > > > > + ad7949_adc->buffer =3D 0; > > > > > ad7949_adc->cfg =3D (val & mask) | (ad7949_adc->cfg & ~mask); > > > > > - ad7949_adc->buffer =3D ad7949_adc->cfg << shift; > > > > > + > > > > > + switch (ad7949_adc->bits_per_word) { > > > > > + case 16: > > > > > + ad7949_adc->buffer =3D ad7949_adc->cfg << 2; > > > > > + break; > > > > > + case 14: > > > > > + ad7949_adc->buffer =3D ad7949_adc->cfg; > > > > > + break; > > > > > + case 8: > > > > > + /* Here, type is big endian as it must be sent in two transfer= s */ > > > > > + ad7949_adc->buffer =3D (u16)cpu_to_be16(ad7949_adc->cfg << 2);= =20 > > > > > > Gah, I wasn't thinking clearly when I suggested this. Sparse warns on > > > the > > > endian conversion > > > > > > One option is to resort to ignoring the fact we know it's aligned and > > > use the put_unaligned_be16() and get_unaligned_be16 calls which spars= e > > > seems to be > > > happy with. Alternative would be to just have a be16 buffer after the > > > existing > > > one in the iio_priv structure. Then you will have to change the vario= us > > > users > > > of iio_priv()->buffer to point to the new value if we are doing 8 bit > > > transfers. > > > > > > Whilst more invasive, this second option is the one I'd suggest. =20 > >=20 > > Understood, I'll go with your suggestion. > >=20 > > Out of curiosity, other that being more explicit, is there another > > we'd rather not use {get,put}_unaligned_be16()? > > We know it is aligned (as u16) so on a big endian platform that happens > to not handle unaligned accesses we will now be doing work which > wouldn't > be needed if there was a get_aligned_be16() that was more relaxed about > types than cpu_to_be16() etc. > > So basically it looks odd and we will will be hiding that we are > smashing > data of potentially different ordering into the same structure field. Got it! Thanks for the explanation. Liam > > >=20 > > > Note that there will be no need to add an __cacheline_aligned marking= to > > > this > > > new element because it will be in a cachline that is only used for DM= A > > > simply being > > > after the other buffer element which is force to start on a new > > > cacheline. =20 > >=20 > > Noted, Thanks for taking the time to explaining this. > >=20 > > Liam > >=20 > > > > > > Jonathan > > > =20 > > > > > + break; > > > > > + default: > > > > > + dev_err(&ad7949_adc->indio_dev->dev, "unsupported BPW\n"); > > > > > + return -EINVAL; > > > > > + } > > > > > + > > > > > spi_message_init_with_transfers(&msg, tx, 1); > > > > > ret =3D spi_sync(ad7949_adc->spi, &msg); > > > > > =20 > > > > > @@ -115,14 +132,12 @@ static int ad7949_spi_read_channel(struct a= d7949_adc_chip *ad7949_adc, int *val, > > > > > { > > > > > int ret; > > > > > int i; > > > > > - int bits_per_word =3D ad7949_adc->resolution; > > > > > - int mask =3D GENMASK(ad7949_adc->resolution - 1, 0); > > > > > struct spi_message msg; > > > > > struct spi_transfer tx[] =3D { > > > > > { > > > > > .rx_buf =3D &ad7949_adc->buffer, > > > > > .len =3D 2, > > > > > - .bits_per_word =3D bits_per_word, > > > > > + .bits_per_word =3D ad7949_adc->bits_per_word, > > > > > }, > > > > > }; > > > > > =20 > > > > > @@ -157,7 +172,25 @@ static int ad7949_spi_read_channel(struct ad= 7949_adc_chip *ad7949_adc, int *val, > > > > > =20 > > > > > ad7949_adc->current_channel =3D channel; > > > > > =20 > > > > > - *val =3D ad7949_adc->buffer & mask; > > > > > + switch (ad7949_adc->bits_per_word) { > > > > > + case 16: > > > > > + *val =3D ad7949_adc->buffer; > > > > > + /* Shift-out padding bits */ > > > > > + *val >>=3D 16 - ad7949_adc->resolution; > > > > > + break; > > > > > + case 14: > > > > > + *val =3D ad7949_adc->buffer & GENMASK(13, 0); > > > > > + break; > > > > > + case 8: > > > > > + /* Here, type is big endian as data was sent in two transfers = */ > > > > > + *val =3D be16_to_cpu(ad7949_adc->buffer); > > > > > + /* Shift-out padding bits */ > > > > > + *val >>=3D 16 - ad7949_adc->resolution; > > > > > + break; > > > > > + default: > > > > > + dev_err(&ad7949_adc->indio_dev->dev, "unsupported BPW\n"); > > > > > + return -EINVAL; > > > > > + } > > > > > =20 > > > > > return 0; > > > > > } > > > > > @@ -265,6 +298,7 @@ static int ad7949_spi_init(struct ad7949_adc_= chip *ad7949_adc) > > > > > =20 > > > > > static int ad7949_spi_probe(struct spi_device *spi) > > > > > { > > > > > + u32 spi_ctrl_mask =3D spi->controller->bits_per_word_mask; > > > > > struct device *dev =3D &spi->dev; > > > > > const struct ad7949_adc_spec *spec; > > > > > struct ad7949_adc_chip *ad7949_adc; > > > > > @@ -291,6 +325,18 @@ static int ad7949_spi_probe(struct spi_devic= e *spi) > > > > > indio_dev->num_channels =3D spec->num_channels; > > > > > ad7949_adc->resolution =3D spec->resolution; > > > > > =20 > > > > > + /* Set SPI bits per word */ > > > > > + if (spi_ctrl_mask & SPI_BPW_MASK(ad7949_adc->resolution)) { > > > > > + ad7949_adc->bits_per_word =3D ad7949_adc->resolution; > > > > > + } else if (spi_ctrl_mask =3D=3D SPI_BPW_MASK(16)) { > > > > > + ad7949_adc->bits_per_word =3D 16; > > > > > + } else if (spi_ctrl_mask =3D=3D SPI_BPW_MASK(8)) { > > > > > + ad7949_adc->bits_per_word =3D 8; > > > > > + } else { > > > > > + dev_err(dev, "unable to find common BPW with spi controller\n"= ); > > > > > + return -EINVAL; > > > > > + } > > > > > + > > > > > ad7949_adc->vref =3D devm_regulator_get(dev, "vref"); > > > > > if (IS_ERR(ad7949_adc->vref)) { > > > > > dev_err(dev, "fail to request regulator\n"); =20 > > > > =20 > >=20