Received: by 2002:ab2:6991:0:b0:1f7:f6c3:9cb1 with SMTP id v17csp539740lqo; Wed, 8 May 2024 07:32:48 -0700 (PDT) X-Forwarded-Encrypted: i=3; AJvYcCUqPzR8HooIMti76X9Sdr/aU+zUKVpslc/uyxd16+enwfnSuny/l1naSE/uiMQwBBMhY2wJUKyyqPCdHl0skkZcgDZhnjzBbRd71S004g== X-Google-Smtp-Source: AGHT+IH8c8+MMjK6zaiuzvG1AIkaipP357Gnz7Aj4NNNywXmxWeJU3WPzskxRUDVIek4PQTWDLZO X-Received: by 2002:a05:6214:5093:b0:6a0:5446:dbc1 with SMTP id 6a1803df08f44-6a15143c60emr30997956d6.2.1715178768296; Wed, 08 May 2024 07:32:48 -0700 (PDT) ARC-Seal: i=2; a=rsa-sha256; t=1715178768; cv=pass; d=google.com; s=arc-20160816; b=oIuSvYoYmjLXuKOQeka0lbx0YW/qBnwr81MuqJiRVyCXx2AivGexbl73k2fOrmwpOR cAFpvoXp+gltrcGWUyIpmgldOsAQ5MKkTuPZ4nKiWKLO0UFWC/wiAb5FDkaAoXoRj0oA aAxTs6Cmq6GpfGkSJjdqb+6GKO2D+lADI/uZ/kFbK4hM5j26VfRuzVniKD/HAlHxmwi/ x2PIyOif/1gEBHO1yhaJBt47U7HSZ6gy+bzLS1jiwVgWtEI24oJxu7EMZIU5BPnSzd90 Cu89VgBCY2rc7e8CVxN7KmxRUbz9ee11O7X9CTsphlhCBWbhStsRQ28qto6lG/dKCAf3 ytiQ== ARC-Message-Signature: i=2; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:list-unsubscribe :list-subscribe:list-id:precedence:date:message-id:dkim-signature; bh=ysuS9s45cXIq/QGNikc88+ChWM3Bs7tDYJ9bNDy61j0=; fh=leAgrBfJQOT/Hh4LEVhU/Wg3uwxH8W5iErpY6wzwcmM=; b=HEtiJpZJB53mcjRQgcYzWSNMmksDYz8MjFzvyQjixQ+I7VOV3IzqTLFlSCVppk0XHP YuDIJuZdR2yrBBVLXEfS5gMz0TS1eKi+eadnyYqj7U9k7IoEC0xbztkOlgAym2B0nZKu rpfjPyn3/evfjujgAyJ9nkM1zAINQalQ2Y2u9fq7lmXcn5AB95VrIcyQRwRpTBf4KjLj 8S1rW5lBlgHMcMwqXgHxtzMc4nuuZqpXKaWJ3aC/8BoXunQdv3kKkVWzAm45rfKggQbg x3T2P7Guq2NN1WIkck6hvrZ8dqiC2TZiOLce6wgORIAu6SR0bj02kRxH5HK38xQ5mG1E s/iQ==; dara=google.com ARC-Authentication-Results: i=2; mx.google.com; dkim=pass header.i=@gmail.com header.s=20230601 header.b=JbziYtq8; arc=pass (i=1 spf=pass spfdomain=gmail.com dkim=pass dkdomain=gmail.com dmarc=pass fromdomain=gmail.com); spf=pass (google.com: domain of linux-kernel+bounces-173403-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) smtp.mailfrom="linux-kernel+bounces-173403-linux.lists.archive=gmail.com@vger.kernel.org"; dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Return-Path: Received: from ny.mirrors.kernel.org (ny.mirrors.kernel.org. [2604:1380:45d1:ec00::1]) by mx.google.com with ESMTPS id gy12-20020a056214242c00b0069b69c9e9absi13782762qvb.414.2024.05.08.07.32.48 for (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 08 May 2024 07:32:48 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel+bounces-173403-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1; Authentication-Results: mx.google.com; dkim=pass header.i=@gmail.com header.s=20230601 header.b=JbziYtq8; arc=pass (i=1 spf=pass spfdomain=gmail.com dkim=pass dkdomain=gmail.com dmarc=pass fromdomain=gmail.com); spf=pass (google.com: domain of linux-kernel+bounces-173403-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) smtp.mailfrom="linux-kernel+bounces-173403-linux.lists.archive=gmail.com@vger.kernel.org"; dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Received: from smtp.subspace.kernel.org (wormhole.subspace.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ny.mirrors.kernel.org (Postfix) with ESMTPS id F2FBF1C232BE for ; Wed, 8 May 2024 14:32:47 +0000 (UTC) Received: from localhost.localdomain (localhost.localdomain [127.0.0.1]) by smtp.subspace.kernel.org (Postfix) with ESMTP id CBBAB86254; Wed, 8 May 2024 14:32:40 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="JbziYtq8" Received: from mail-wm1-f51.google.com (mail-wm1-f51.google.com [209.85.128.51]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 202142575A; Wed, 8 May 2024 14:32:37 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.128.51 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1715178759; cv=none; b=rZiltHY6MgmfEUwiZLmTpMBVnNRI+9wMvMz2YwfNvQNF625fz4RycxhtdQ7n7aI4+dwrw8Y5IuvicBR0ir5ra4BY22VLG/WbklLIHA4rB6623WCQpOI0DVNniBOCRmBkQjGQlV6E5aKLzqU5zQ5E/6Kh6zTem50Jw5FxmoBEL8E= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1715178759; c=relaxed/simple; bh=oj63ouKpfDlDfTL/RNh6p+xuT5dFT21ACWDeoMAXWQM=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=ekgPpLAii9RxDNGGh7OSjlvR84aKi8hTvkA3aeVGd9ZWRUzE/3AX9f7GzCC9GP7Hp4ALSKhRRzyK/oW/QMIgjlVmdOV15JCfbOoB6gLU2A/1XfZSJov8KpXMLmq9ZyrEka8Cjm2kTQhdusyD+recSHAuDXULy8OkWCPjpfrnO7o= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com; spf=pass smtp.mailfrom=gmail.com; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b=JbziYtq8; arc=none smtp.client-ip=209.85.128.51 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=gmail.com Received: by mail-wm1-f51.google.com with SMTP id 5b1f17b1804b1-41b79451145so33225605e9.3; Wed, 08 May 2024 07:32:37 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1715178756; x=1715783556; darn=vger.kernel.org; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :from:to:cc:subject:date:message-id:reply-to; bh=ysuS9s45cXIq/QGNikc88+ChWM3Bs7tDYJ9bNDy61j0=; b=JbziYtq8ge0m6uq/5KqpOHIAX9j4Vllx/hXsI/oX3//ya8RBcZjEJhVmUAeeL3VtH6 E9YrCogqaY7U/xeLtfgtsp1VuqUVI2lwKa1j+FO6SME9e+2Ebiuh9YyYJEluF34OyFC3 BuJ9JLoN7lt5o1iSAtFC1aXkWIywm/PmgDe8VKY3k0h8R9N/Mr5aLVvekjfBwHB5d3c1 JukBZcc5htYD/Dr5Ul8IHqKb2uUbHEY7ey801VVu/PObkB/qVXiBjqa9R/21Na+HmjWx RxJ7E7SUHQ3ttymWbA9vNAfchbAUXPqbDypGjBRhWqmlmoxVzjoeYITuxx/OxTGprVsQ mNTA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1715178756; x=1715783556; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=ysuS9s45cXIq/QGNikc88+ChWM3Bs7tDYJ9bNDy61j0=; b=tNWuU7YnAeJastSWXtbQLbN9rjFfeL+ercO/NlxKBMehjgt6CEEOzLEUi9y9iLBw1f S4n4TP+wYfXL6UacudDVikO1UshXlgOsm43BeL9dGhXPIZ2CzIliWj3whoyOgckdfkBT xZ5U3ZMrK+rjJePDOK9FihC8LWnQzkIwgLsssgByT/FNwqvr/q+tCAzey110m7pKYjcF 9Mi5oaH6/pdfaEu2eo0kqmjMZYIzUEzk2ZRszllfN645peN90ShlGZfK0fYeZPSzjAPl Y3bItESWKFcmFvIOlxbLctgvM6xtiE3+UnVaocxXDxTmab7JBn/P8FKnCMJ3ybr+1Lm0 LxzQ== X-Forwarded-Encrypted: i=1; AJvYcCUj9ienH0Eg1QtJoY7EIyszrnPOv/ci0WxfaP7gYNIqJrzIsAzqvhDuDjS0XE13P5bg76MZPpi/yoB8ew+V7jgWU4vj4NWr09OZfOH+g+NFtBb442A17xjhbzgQyP50IUZB9oqYNw== X-Gm-Message-State: AOJu0YzvFGQqTeC2YuRfmxFn5cdER+rNcZNmXHJGtNbc9xs/qTw+wPSZ 3S7Q1MhwWNNv/gILIzeaMSxg/KydQrJpHmMn6FWTsneOuOVkRLTA X-Received: by 2002:adf:e6c6:0:b0:34c:a509:6138 with SMTP id ffacd0b85a97d-34fca623484mr1976806f8f.49.1715178756097; Wed, 08 May 2024 07:32:36 -0700 (PDT) Received: from [10.76.84.112] ([5.2.194.157]) by smtp.gmail.com with ESMTPSA id cm11-20020a5d5f4b000000b0034dd27adb2fsm15500861wrb.107.2024.05.08.07.32.35 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 08 May 2024 07:32:35 -0700 (PDT) Message-ID: <9f0a8fdb-dd34-4a53-948d-d4ed0410de6f@gmail.com> Date: Wed, 8 May 2024 17:32:34 +0300 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH 7/7] drivers: iio: imu: Add support for adis1657x family To: Jonathan Cameron Cc: linux-kernel@vger.kernel.org, linux-iio@vger.kernel.org, devicetree@vger.kernel.org, conor+dt@kernel.org, krzysztof.kozlowski+dt@linaro.org, robh@kernel.org, nuno.sa@analog.com References: <20240426135339.185602-1-ramona.bolboaca13@gmail.com> <20240426135339.185602-8-ramona.bolboaca13@gmail.com> <20240428154523.17b27fa8@jic23-huawei> Content-Language: en-US From: Ramona Gradinariu In-Reply-To: <20240428154523.17b27fa8@jic23-huawei> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Hello Jonathan, Some explanations from my side. >> @@ -437,6 +467,130 @@ static int adis16475_set_filter(struct adis16475 *st, const u32 filter) >> return 0; >> } >> >> +static ssize_t adis16475_get_fifo_enabled(struct device *dev, >> + struct device_attribute *attr, >> + char *buf) >> +{ >> + struct iio_dev *indio_dev = dev_to_iio_dev(dev); >> + struct adis16475 *st = iio_priv(indio_dev); >> + int ret; >> + u16 val; >> + >> + ret = adis_read_reg_16(&st->adis, ADIS16475_REG_FIFO_CTRL, &val); >> + if (ret) >> + return ret; >> + val = FIELD_GET(ADIS16475_FIFO_EN_MASK, val); >> + >> + return sysfs_emit(buf, "%d\n", val); > As below, might as well put the FIELD_GET() in the sysfs_emit rather than > writing the local parameter. In all instances where I did, I did it to avoid casting. v2 inlines the values and the cast is needed to avoid compilation errors. > >> + if (adis->data->burst_max_len) >> + burst_max_length = adis->data->burst_max_len; >> + else >> + burst_max_length = burst_length; >> + >> + tx = adis->buffer + burst_max_length; >> + tx[0] = ADIS_READ_REG(burst_req); >> + >> + if (burst_req) > If !burst_req does the rest of this do anything at all? > If so flip the logic as > if (!burst_req) > return adis16475_push_single_sample(pf); > > the rest... > return spi_sync(adis->spi, &adis->msg); The update is needed even if burst_req is false. The adis message has to be updated based on the burst request value, which is then used either in adis16475_push_single_sample or in spi_sync call. > > >> + return spi_sync(adis->spi, &adis->msg); >> + >> + return adis16475_push_single_sample(pf); >> +} >> + >> +/* >> + * This handler is meant to be used for devices which support burst readings >> + * from FIFO (namely devices from adis1657x family). >> + * In order to pop the FIFO the 0x68 0x00 FIFO pop burst request has to be sent. >> + * If the previous device command was not a FIFO pop burst request, the FIFO pop >> + * burst request will simply pop the FIFO without returning valid data. >> + * For the nth consecutive burst request, the >> + * device will send the data popped with the (n-1)th consecutive burst request. >> + * In order to read the data which was popped previously, without popping the FIFO, >> + * the 0x00 0x00 burst request has to be sent. >> + * If after a 0x68 0x00 FIFO pop burst request, there is any other device access >> + * different from a 0x68 0x00 or a 0x00 0x00 burst request, the FIFO data popped >> + * previously will be lost. >> + */ >> +static irqreturn_t adis16475_trigger_handler_with_fifo(int irq, void *p) >> { >> struct iio_poll_func *pf = p; >> struct iio_dev *indio_dev = pf->indio_dev; >> + struct adis16475 *st = iio_priv(indio_dev); >> + struct adis *adis = &st->adis; >> + int ret; >> + u16 fifo_cnt, i; >> >> - adis16475_push_single_sample(pf); >> + adis_dev_lock(&st->adis); >> + >> + ret = __adis_read_reg_16(adis, ADIS16475_REG_FIFO_CNT, &fifo_cnt); >> + if (ret || fifo_cnt < 2) >> + goto unlock; > I would break these conditions and add a comment on why fifo_cnt < 2 is > a reason to just return 0; Updated this in v2, and actually fifo_cnt can also be 1 so the code simply verifies if !fifo_cnt. > >> + >> + if (fifo_cnt > st->fifo_watermark) >> + fifo_cnt = st->fifo_watermark; > fifo_cnt = min(fifo_cnt, st->fifo_watermark); > > This confuses me though as normally overreading after a fifo watermark is > both safe and the right thing to do (as reduces chance of overflow etc). > If we need to clamp to the watermark for some reason, add a comment. Removed this in v2. >> >> ret = __adis_write_reg_16(&st->adis, >> ADIS16475_REG_UP_SCALE, >> @@ -1467,7 +1888,23 @@ static int adis16475_config_irq_pin(struct adis16475 *st) >> */ >> usleep_range(250, 260); >> >> - return 0; >> + /* >> + * If the device has FIFO support, configure the watermark polarity >> + * pin as well. > The pin is for polarity or the polarity is for the watermark signalling on that > pin? I'm not seeing a datasheet yet for these parts so I couldn't check. The device has a watermark pin, which can be used as a trigger source. In this case we set the polarity for the watermark pin. > >> + */ >> + if (st->info->flags & ADIS16475_HAS_FIFO) { >> + val = ADIS16475_WM_POL(polarity); >> + ret = adis_update_bits(&st->adis, ADIS16475_REG_FIFO_CTRL, >> + ADIS16475_WM_POL_MASK, val); >> + if (ret) >> + return ret; >> + >> + /* Enable watermark interrupt pin. */ >> + val = ADIS16475_WM_EN(1); >> + ret = adis_update_bits(&st->adis, ADIS16475_REG_FIFO_CTRL, ADIS16475_WM_EN_MASK, val); Best Regards, Ramona