Message ID | 20240121051735.32246-1-subhajit.ghosh@tweaklogic.com |
---|---|
Headers |
Return-Path: <linux-kernel+bounces-31966-ouuuleilei=gmail.com@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a05:7301:2bc4:b0:101:a8e8:374 with SMTP id hx4csp2387636dyb; Sun, 21 Jan 2024 22:02:53 -0800 (PST) X-Google-Smtp-Source: AGHT+IGNwFSO6nEovJseV9FQq7PiX1Zt0Wyw9/u5XqeHOL3BvyIwTq2opPnikx9F29i2Gbb2yjoA X-Received: by 2002:a17:907:7858:b0:a23:690e:48bf with SMTP id lb24-20020a170907785800b00a23690e48bfmr5132101ejc.12.1705903373325; Sun, 21 Jan 2024 22:02:53 -0800 (PST) ARC-Seal: i=2; a=rsa-sha256; t=1705903373; cv=pass; d=google.com; s=arc-20160816; b=fiI/KgP5qwVkVIvup0yLEp35Lit9fzDtveTqVVpsfSYwQDDMzgiAjTnHc7S354wqF+ +Jsfm8uDNyZPNKC0XrUtr6I0XzatftiyAobwS61Co4x60eQnp8asg9k/qazCPvVLE7gJ 05O7XD96yMvlaL6z7XmIFGEmEtTqAIO0dTKDMk5W8hT6BB16cYBsRijYRXOh7WKk7pBZ xJI1/etRa8QtFDFKd4yD0tIzD0Gb3DKZYeXWUBzKw5k4fHTRWHEuL6YQaODcg3aiZTL2 ohyDIOlOXsztVVmMjsKbWy/FNA6QSgt35Z401EVsTmbpjIH16A5nr3WoerGPilcgv13o XmCw== ARC-Message-Signature: i=2; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=content-transfer-encoding:mime-version:list-unsubscribe :list-subscribe:list-id:precedence:message-id:date:subject:cc:to :from:dkim-signature; bh=otpPQ8q+RYv1jY1aPkVNIU5dIBW1JVgH90JU2MjCsBQ=; fh=UmVHA2HXJwJd6uESM8v9C81mqF3XC2ufORF7x4MKwek=; b=d0Vm/dRm3TbsSBVsD0PYhE/TpN/qaS76WyA3/eeZ+EjBkriMV07Lvqm7ITctUj0ERq VnUCvvJox6krcYzbGIRaN8HvP+J0ZikTuH1rHl0pOZM3iU2CQ6BjYJriDMbEyfl/Axj6 +bEJw7FW+l5EXltCplR4djbP2HvWOa4kEIC2PgwH5db/0NKI3fyAfFly3aQKS7Ap1H6D +UnKh0D2kBCHb8H7WbzIY/qyHxpzm45HPYGb/XNAKJCAr26E9I7XCsDu4qLobfd8oeHA EX68lGj6x2tihsi7dWY5f39Vj+BlS1/8YkNLjoQl3p6IOAZ873TM7KrKBBxdEJdsJv93 nHXg== ARC-Authentication-Results: i=2; mx.google.com; dkim=pass header.i=@tweaklogic.com header.s=google header.b=J2WFLoj3; arc=pass (i=1 spf=pass spfdomain=tweaklogic.com dkim=pass dkdomain=tweaklogic.com); spf=pass (google.com: domain of linux-kernel+bounces-31966-ouuuleilei=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) smtp.mailfrom="linux-kernel+bounces-31966-ouuuleilei=gmail.com@vger.kernel.org" Received: from am.mirrors.kernel.org (am.mirrors.kernel.org. [2604:1380:4601:e00::3]) by mx.google.com with ESMTPS id f24-20020a170906391800b00a2693a66d02si10360210eje.251.2024.01.21.22.02.53 for <ouuuleilei@gmail.com> (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sun, 21 Jan 2024 22:02:53 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel+bounces-31966-ouuuleilei=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3; Authentication-Results: mx.google.com; dkim=pass header.i=@tweaklogic.com header.s=google header.b=J2WFLoj3; arc=pass (i=1 spf=pass spfdomain=tweaklogic.com dkim=pass dkdomain=tweaklogic.com); spf=pass (google.com: domain of linux-kernel+bounces-31966-ouuuleilei=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) smtp.mailfrom="linux-kernel+bounces-31966-ouuuleilei=gmail.com@vger.kernel.org" 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 am.mirrors.kernel.org (Postfix) with ESMTPS id 548481F219A0 for <ouuuleilei@gmail.com>; Sun, 21 Jan 2024 05:18:14 +0000 (UTC) Received: from localhost.localdomain (localhost.localdomain [127.0.0.1]) by smtp.subspace.kernel.org (Postfix) with ESMTP id 4E075364A6; Sun, 21 Jan 2024 05:17:53 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=tweaklogic.com header.i=@tweaklogic.com header.b="J2WFLoj3" Received: from mail-ot1-f43.google.com (mail-ot1-f43.google.com [209.85.210.43]) (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 5BF7022063 for <linux-kernel@vger.kernel.org>; Sun, 21 Jan 2024 05:17:48 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.210.43 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1705814270; cv=none; b=fgARi2vftl4zjZDlQ5ymkcoIh7jdqD8SslUeooTKqTmW8rKkvsrO2/P4lqMZe4vKCC1/8coB/bKwid/7k4JqP/OuyCJDRDj/XbyuL5G+6TWur+cPm/RZQBjqB7ugSNocCJflYH5qPjWlguqJkrXsAU+7gUR9ZvEaQZYZB9FKSZ4= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1705814270; c=relaxed/simple; bh=ipSzNcsozIyMfm06SP86zDpf6H5Wpu46NsfyrOWklmw=; h=From:To:Cc:Subject:Date:Message-Id:MIME-Version; b=SjTiVPINr4vYjMdL+Vs07SbrE2AxqmW8HM4WDBXFhZlsSqSe6wUXEx+FHOhS37MGWfHGFbU6KGaYoxst9IizOgwvdW/yN2EFWQAM3N6fhkj/Rd2N99Eemc7i1JKkM8ST9diDzefGs/tXsczaYcQTUdTnD7KkQeEv6PLZjq5m9G4= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=tweaklogic.com; spf=pass smtp.mailfrom=tweaklogic.com; dkim=pass (2048-bit key) header.d=tweaklogic.com header.i=@tweaklogic.com header.b=J2WFLoj3; arc=none smtp.client-ip=209.85.210.43 Authentication-Results: smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=tweaklogic.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=tweaklogic.com Received: by mail-ot1-f43.google.com with SMTP id 46e09a7af769-6dde290d09eso1520765a34.2 for <linux-kernel@vger.kernel.org>; Sat, 20 Jan 2024 21:17:48 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=tweaklogic.com; s=google; t=1705814267; x=1706419067; darn=vger.kernel.org; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:from:to:cc:subject:date:message-id:reply-to; bh=otpPQ8q+RYv1jY1aPkVNIU5dIBW1JVgH90JU2MjCsBQ=; b=J2WFLoj33LFLj0z7qmBk1JYhANnvrYTuHLAQp+c3ZowlSq542Hd8XbPduc/bA1qqqq /a3Y7S6d6fTfpZ4J6flWiGdt1eSab3rIl9UoZguTsJ0TeJ8G7gYeExopnXZvyDsP1/un sboQWcENthv1NUeFrHLqKATHyXQHrxDoJEKoEGlKG1tiLdNn0bXYZfcuEbK+XsyJhvqF ScRCuqGK6mCqNkR9AK1I9eLwb48v6Moo0YKan8ri31y7pO0QR4U8pm+NVMdNEvLFL2EZ ZYbZtSBWqlnc8eP/mfssFSMnROEBXpXV2NZ3JEcsIK3m31k4NbYGLs0Pbu1upx636MbG xo0A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1705814267; x=1706419067; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=otpPQ8q+RYv1jY1aPkVNIU5dIBW1JVgH90JU2MjCsBQ=; b=tfeUVNgjXIucsp7mx4zwsTjcLOvmYUPdXwKblcuFM8aj3KPo/GTvLQcNlMGXpZCGxj e/7l/ookBVg8Azpew01PFS1WZzTMuiXGV8Zh4rqnLQwxm3JHEOl7KHU1FJk2Z1olxK0w gSuVgJsMlnjNnGP1EevLTQ6JmxcfOFis064UnMqU+rZguF5dg8sBEb/dtjcf5wEGgyj5 mSUox42wmA4YegzcfHErnwCkXniYizPDrrAtnjEC8Fi7oWZVMTA98ulWrnqU95mIZR/s TU/90y3KWhb1ViRnMJmPcU4a/6DHrR4DALtJafgYHASZ/rt33zuY75kgvuymMcroaPHS U/HA== X-Gm-Message-State: AOJu0YxO2YEVuqGlZnSzIT10wHS901fX/FAXCzRxBSKocAsqkVSRa+bl XlUJp0D0OumxaOp6n7kXCw1nFB8gPiKxK33IzEt8sAluc95hjcaz0/gmwFzumCk= X-Received: by 2002:a9d:64ca:0:b0:6d9:d144:c9a9 with SMTP id n10-20020a9d64ca000000b006d9d144c9a9mr2574521otl.22.1705814267257; Sat, 20 Jan 2024 21:17:47 -0800 (PST) Received: from localhost.localdomain (2403-580d-82f4-0-3fa1-f9ce-6074-3bab.ip6.aussiebb.net. [2403:580d:82f4:0:3fa1:f9ce:6074:3bab]) by smtp.gmail.com with ESMTPSA id f6-20020a056a000b0600b006d96d034befsm7547196pfu.30.2024.01.20.21.17.41 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sat, 20 Jan 2024 21:17:46 -0800 (PST) From: Subhajit Ghosh <subhajit.ghosh@tweaklogic.com> To: Jonathan Cameron <jic23@kernel.org>, Lars-Peter Clausen <lars@metafoo.de>, Rob Herring <robh+dt@kernel.org>, Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>, Conor Dooley <conor+dt@kernel.org>, Matti Vaittinen <mazziesaccount@gmail.com>, Andy Shevchenko <andriy.shevchenko@linux.intel.com>, Marek Vasut <marex@denx.de>, Anshul Dalal <anshulusr@gmail.com>, Javier Carrasco <javier.carrasco.cruz@gmail.com> Cc: Subhajit Ghosh <subhajit.ghosh@tweaklogic.com>, Matt Ranostay <matt@ranostay.sg>, Stefan Windfeldt-Prytz <stefan.windfeldt-prytz@axis.com>, linux-iio@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org Subject: [PATCH v5 0/3] Support for Avago APDS9306 Ambient Light Sensor Date: Sun, 21 Jan 2024 15:47:31 +1030 Message-Id: <20240121051735.32246-1-subhajit.ghosh@tweaklogic.com> X-Mailer: git-send-email 2.34.1 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: <linux-kernel.vger.kernel.org> List-Subscribe: <mailto:linux-kernel+subscribe@vger.kernel.org> List-Unsubscribe: <mailto:linux-kernel+unsubscribe@vger.kernel.org> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1788769335513957539 X-GMAIL-MSGID: 1788769335513957539 |
Series |
Support for Avago APDS9306 Ambient Light Sensor
|
|
Message
Subhajit Ghosh
Jan. 21, 2024, 5:17 a.m. UTC
Support for Avago APDS9306 Ambient Light Sensor Driver support for Avago (Broadcom) APDS9306 Ambient Light Sensor. It has two channels - ALS and CLEAR. The ALS (Ambient Light Sensor) channel approximates the response of the human-eye providing direct read out where the output count is proportional to ambient light levels. It is internally temperature compensated and rejects 50Hz and 60Hz flicker caused by artificial light sources. Hardware interrupt configuration is optional. It is a low power device with 20 bit resolution and has configurable adaptive interrupt mode and interrupt persistence mode. The device also features inbuilt hardware gain, multiple integration time selection options and sampling frequency selection options. This driver also uses the IIO GTS (Gain Time Scale) Helpers Namespace for Scales, Gains and Integration time implementation. Link: https://docs.broadcom.com/doc/AV02-4755EN Following features are supported: - I2C interface - 2 channels - als and clear - Raw data for als and clear channels - Up to 20 bit resolution - 20 bit data register for each channel - Common Configurable items for both channels - Integration Time - Scale - Interrupt (event) interface - High and Low threshold interrupts for each channel - Selection of interrupt channels - als or clear - Selection of interrupt mode - threshold or adaptive - Level selection for adaptive threshold interrupts - Persistence (Period) level selection for interrupts root@stm32mp1:~# tree -I 'dev|name|of_node|power|subsystem|uevent' \ /sys/bus/iio/devices/iio:device0/ |-- events | |-- in_illuminance_thresh_either_en | |-- in_intensity_clear_thresh_either_en | |-- thresh_adaptive_either_en | |-- thresh_adaptive_either_value | |-- thresh_adaptive_either_values_available | |-- thresh_either_period | |-- thresh_either_period_available | |-- thresh_falling_value | `-- thresh_rising_value |-- in_illuminance_raw |-- in_illuminance_scale |-- in_illuminance_scale_available |-- in_intensity_clear_raw |-- integration_time |-- integration_time_available |-- sampling_frequency |-- sampling_frequency_available `-- waiting_for_supplier 1 directory, 18 files v2 -> v5: - Bumped up the version: RFC->v0->v1->v2->v3 (Earlier scheme) v1->v2->v3->v4->v5 (Scheme after review) (Current scheme) Link: https://lore.kernel.org/all/20231028143631.2545f93e@jic23-huawei/ - Added separate patch to merge schemas for APDS9300 and APDS9906. Added APDS9306 support on top of that. Link: https://lore.kernel.org/lkml/4e785d2e-d310-4592-a75a-13549938dcef@linaro.org/ Link: https://lore.kernel.org/lkml/20231028142944.7e210eb6@jic23-huawei/ - Removed scale attribute for Intensity channel: Link: https://lore.kernel.org/all/20231204095108.22f89718@jic23-huawei/ - Dropped caching of hardware gain, repeat rate and integration time and updated code as per earlier reviews. Link: https://lore.kernel.org/lkml/20231028142944.7e210eb6@jic23-huawei/ - Added descriptive commit messages - Fixed wrongly formatted commit messages - Added changelog in right positions - Link to v2: https://lore.kernel.org/lkml/20231027074545.6055-3-subhajit.ghosh@tweaklogic.com/ v2 -> v5 Bindings: - Removed 'required' for Interrupts and 'oneOf' for compatibility strings as per below reviews: Link: https://lore.kernel.org/lkml/20231028142944.7e210eb6@jic23-huawei/ Link: https://lore.kernel.org/lkml/22e9e5e9-d26a-46e9-8986-5062bbfd72ec@linaro.org/ - Implemented changes as per previous reviews: Link: https://lore.kernel.org/lkml/20231028142944.7e210eb6@jic23-huawei/ Link: https://lore.kernel.org/lkml/22e9e5e9-d26a-46e9-8986-5062bbfd72ec@linaro.org/ Subhajit Ghosh (3): dt-bindings: iio: light: Squash APDS9300 and APDS9960 schemas dt-bindings: iio: light: Avago APDS9306 iio: light: Add support for APDS9306 Light Sensor .../bindings/iio/light/avago,apds9300.yaml | 20 +- .../bindings/iio/light/avago,apds9960.yaml | 44 - drivers/iio/light/Kconfig | 12 + drivers/iio/light/Makefile | 1 + drivers/iio/light/apds9306.c | 1315 +++++++++++++++++ 5 files changed, 1343 insertions(+), 49 deletions(-) delete mode 100644 Documentation/devicetree/bindings/iio/light/avago,apds9960.yaml create mode 100644 drivers/iio/light/apds9306.c base-commit: 9d1694dc91ce7b80bc96d6d8eaf1a1eca668d847
Comments
On 22/1/24 01:53, Jonathan Cameron wrote: > On Sun, 21 Jan 2024 15:47:34 +1030 > Subhajit Ghosh <subhajit.ghosh@tweaklogic.com> wrote: > >> Driver support for Avago (Broadcom) APDS9306 Ambient Light Sensor. >> It has two channels - ALS and CLEAR. The ALS (Ambient Light Sensor) >> channel approximates the response of the human-eye providing direct >> read out where the output count is proportional to ambient light levels. >> It is internally temperature compensated and rejects 50Hz and 60Hz flicker >> caused by artificial light sources. Hardware interrupt configuration is >> optional. It is a low power device with 20 bit resolution and has >> configurable adaptive interrupt mode and interrupt persistence mode. >> The device also features inbuilt hardware gain, multiple integration time >> selection options and sampling frequency selection options. >> >> This driver also uses the IIO GTS (Gain Time Scale) Helpers Namespace for >> Scales, Gains and Integration time implementation. >> >> Signed-off-by: Subhajit Ghosh <subhajit.ghosh@tweaklogic.com> >> --- >> v2 -> v5: > > Why did you jump to v5? Some internal or private reviews perhaps? > Better for those tracking on the list if you just used v3. Wish I had someone to review my code before sending it to kernel community! I do this in my own time. v5 was suggested by you. Now I understand that Suggested-by: tag has to be used :) https://lore.kernel.org/all/20231028143631.2545f93e@jic23-huawei/ > > >> - Removed scale attribute for Intensity channel: >> Link: https://lore.kernel.org/all/20231204095108.22f89718@jic23-huawei/ >> >> - Dropped caching of hardware gain, repeat rate and integration time and >> updated code as per earlier reviews. >> Link: https://lore.kernel.org/lkml/20231028142944.7e210eb6@jic23-huawei/ > > ... > > A few, mostly very minor comments inline to add to Christophe's review. > > Thanks, > > Jonathan > >> diff --git a/drivers/iio/light/apds9306.c b/drivers/iio/light/apds9306.c >> new file mode 100644 >> index 000000000000..8ed5899050ed >> --- /dev/null >> +++ b/drivers/iio/light/apds9306.c >> @@ -0,0 +1,1315 @@ >> +// SPDX-License-Identifier: GPL-2.0-or-later >> +/* >> + * APDS-9306/APDS-9306-065 Ambient Light Sensor >> + * I2C Address: 0x52 >> + * Datasheet: https://docs.broadcom.com/doc/AV02-4755EN >> + * >> + * Copyright (C) 2023 Subhajit Ghosh <subhajit.ghosh@tweaklogic.com> > > Given you are still changing it, feel free to include 2024! I sincerely hope that I don't have to update it to 2025! > >> + */ > ... >> +static const int apds9306_repeat_rate_freq[][2] = { >> + {40, 0}, >> + {20, 0}, >> + {10, 0}, >> + {5, 0}, >> + {2, 0}, >> + {1, 0}, >> + {0, 500000}, > Prefer > { 40, 0 }, > etc and whilst I don't really like forcing alignment like this, if you > are going to do it be consistent. The last 50000 is one space too far to the > left I think. > > >> +}; >> +static_assert(ARRAY_SIZE(apds9306_repeat_rate_freq) == >> + APDS9306_NUM_REPEAT_RATES); >> + >> +static const int apds9306_repeat_rate_period[] = { >> + 25000, 50000, 100000, 200000, 500000, 1000000, 2000000, >> +}; >> +static_assert(ARRAY_SIZE(apds9306_repeat_rate_period) == >> + APDS9306_NUM_REPEAT_RATES); >> + >> +/** >> + * struct apds9306_data - apds9306 private data and registers definitions >> + * >> + * @dev: Pointer to the device structure >> + * @gts: IIO Gain Time Scale structure >> + * @mutex: Lock for protecting register access, adc reads and power > > ADC. I guess the double comment is to keep checkpatch happy? > > Just ignore it being dumb as you have a comment up here and put all the info > here rather than splitting it up like this. > >> + * @regmap: Regmap structure pointer >> + * @regfield_sw_reset: Reg: MAIN_CTRL, Field: SW_Reset >> + * @regfield_en: Reg: MAIN_CTRL, Field: ALS_EN >> + * @regfield_intg_time: Reg: ALS_MEAS_RATE, Field: ALS Resolution/Bit Width >> + * @regfield_repeat_rate: Reg: ALS_MEAS_RATE, Field: ALS Measurement Rate >> + * @regfield_gain: Reg: ALS_GAIN, Field: ALS Gain Range >> + * @regfield_int_src: Reg: INT_CFG, Field: ALS Interrupt Source >> + * @regfield_int_thresh_var_en: Reg: INT_CFG, Field: ALS Var Interrupt Mode >> + * @regfield_int_en: Reg: INT_CFG, Field: ALS Interrupt Enable >> + * @regfield_int_persist_val: Reg: INT_PERSISTENCE, Field: ALS_PERSIST >> + * @regfield_int_thresh_var_val: Reg: ALS_THRSH_VAR, Field: ALS_THRES_VAR >> + * @nlux_per_count: nano lux per ADC count for a particular model >> + * @read_data_available: Flag set by IRQ handler for ADC data available >> + */ >> +struct apds9306_data { >> + struct device *dev; >> + struct iio_gts gts; >> + /* >> + * Protects device settings changes where some calculations are required >> + * before or after setting or getting the raw settings values from regmap >> + * writes or reads respectively. >> + */ >> + struct mutex mutex; >> + >> + struct regmap *regmap; >> + struct regmap_field *regfield_sw_reset; >> + struct regmap_field *regfield_en; >> + struct regmap_field *regfield_intg_time; >> + struct regmap_field *regfield_repeat_rate; >> + struct regmap_field *regfield_gain; >> + struct regmap_field *regfield_int_src; >> + struct regmap_field *regfield_int_thresh_var_en; >> + struct regmap_field *regfield_int_en; >> + struct regmap_field *regfield_int_persist_val; >> + struct regmap_field *regfield_int_thresh_var_val; >> + >> + int nlux_per_count; >> + int read_data_available; >> +}; > >> + >> +static struct iio_event_spec apds9306_event_spec_als[] = { >> + { >> + .type = IIO_EV_TYPE_THRESH, >> + .dir = IIO_EV_DIR_RISING, >> + .mask_shared_by_all = BIT(IIO_EV_INFO_VALUE), >> + }, { >> + .type = IIO_EV_TYPE_THRESH, >> + .dir = IIO_EV_DIR_FALLING, >> + .mask_shared_by_all = BIT(IIO_EV_INFO_VALUE), >> + }, { >> + .type = IIO_EV_TYPE_THRESH, >> + .mask_shared_by_all = BIT(IIO_EV_INFO_PERIOD), >> + }, { >> + .type = IIO_EV_TYPE_THRESH_ADAPTIVE, >> + .mask_shared_by_all = BIT(IIO_EV_INFO_VALUE) | >> + BIT(IIO_EV_INFO_ENABLE), >> + }, { >> + .mask_separate = BIT(IIO_EV_INFO_ENABLE), > > What's the intent of this final entry? > The type will default to IIO_EV_TYPE_THRESH anyway but if that > the intent you should specify it. There isn't an 'obvious' > default for type in the same way there sort of is for dir > (as it's either direction). Understood, let me experiment and see the ABI difference, if any and get back to you. > >> + }, >> +}; > >> + > >> + >> +static int apds9306_runtime_power_on(struct device *dev) >> +{ >> + int ret; >> + >> + ret = pm_runtime_resume_and_get(dev); >> + if (ret < 0) >> + dev_err(dev, "runtime resume failed: %d\n", ret); >> + >> + return ret; >> +} >> + >> +static int apds9306_runtime_power_off(struct device *dev) >> +{ >> + pm_runtime_mark_last_busy(dev); >> + pm_runtime_put_autosuspend(dev); >> + >> + return 0; >> +} > > I'm not entirely convinced these two wrappers are worthwhile given they > aren't that common used and locally obscure what is going on when > it could be apparent at the few call sites. The above was suggested by Andy. https://lore.kernel.org/linux-devicetree/ZTuuUl0PBklbVjb9@smile.fi.intel.com/ Apologies for my ignorance - "it could be apparent at the few call sites" - I did not understand the above statement. Can you please elaborate? > > > >> +static int apds9306_read_event_config(struct iio_dev *indio_dev, >> + const struct iio_chan_spec *chan, >> + enum iio_event_type type, >> + enum iio_event_direction dir) >> +{ >> + struct apds9306_data *data = iio_priv(indio_dev); >> + int int_en, int_ch, ret; >> + >> + guard(mutex)(&data->mutex); >> + >> + switch (type) { >> + case IIO_EV_TYPE_THRESH: >> + ret = regmap_field_read(data->regfield_int_src, &int_ch); > > int_ch is a not particularly informative name. > > event_ch_is_light perhaps? > >> + if (ret) >> + return ret; >> + ret = regmap_field_read(data->regfield_int_en, &int_en); >> + if (ret) >> + return ret; >> + if (chan->type == IIO_LIGHT) >> + return int_en & int_ch; >> + else if (chan->type == IIO_INTENSITY) >> + return int_en & !int_ch; >> + return -EINVAL; >> + case IIO_EV_TYPE_THRESH_ADAPTIVE: >> + ret = regmap_field_read(data->regfield_int_thresh_var_en, &int_en); >> + if (ret) >> + return ret; >> + return int_en; >> + default: >> + return -EINVAL; >> + } >> +} >> + >> +static int apds9306_write_event_config(struct iio_dev *indio_dev, >> + const struct iio_chan_spec *chan, >> + enum iio_event_type type, >> + enum iio_event_direction dir, >> + int state) >> +{ >> + struct apds9306_data *data = iio_priv(indio_dev); >> + int ret, val; >> + >> + state = !!state; >> + >> + guard(mutex)(&data->mutex); >> + >> + switch (type) { >> + case IIO_EV_TYPE_THRESH: >> + /* >> + * If interrupt is enabled, the channel is set before enabling >> + * the interrupt. In case of disable, no need to switch >> + * channels. In case of different channel is selected while >> + * interrupt in on, just change the channel. >> + */ >> + if (state) { >> + if (chan->type == IIO_LIGHT) >> + val = 1; >> + else if (chan->type == IIO_INTENSITY) >> + val = 0; >> + else >> + return -EINVAL; > > Blank line here and similar. > >> + ret = regmap_field_write(data->regfield_int_src, val); >> + if (ret) >> + return ret; >> + } >> + ret = regmap_field_read(data->regfield_int_en, &val); >> + if (ret) >> + return ret; >> + if (val == state) >> + return 0; > > Blank line. Basically add one whenever a block of related code ends. > >> + ret = regmap_field_write(data->regfield_int_en, state); >> + if (ret) >> + return ret; >> + if (state) >> + return apds9306_runtime_power_on(data->dev); >> + return apds9306_runtime_power_off(data->dev); >> + case IIO_EV_TYPE_THRESH_ADAPTIVE: >> + return regmap_field_write(data->regfield_int_thresh_var_en, state); >> + default: >> + return -EINVAL; >> + } >> +} >> > > .. > >> +static void apds9306_powerdown(void *ptr) >> +{ >> + struct apds9306_data *data = (struct apds9306_data *)ptr; >> + int ret; >> + >> + ret = regmap_field_write(data->regfield_int_thresh_var_en, 0); >> + if (ret) >> + return; > > blank line here ideally. > >> + ret = regmap_field_write(data->regfield_int_en, 0); >> + if (ret) >> + return; >> + >> + apds9306_power_state(data, false); >> +} > > ... > >> + >> +static int apds9306_probe(struct i2c_client *client) >> +{ >> + struct device *dev = &client->dev; >> + struct apds9306_data *data; >> + struct iio_dev *indio_dev; >> + int ret; >> + >> + indio_dev = devm_iio_device_alloc(dev, sizeof(*data)); >> + if (!indio_dev) >> + return -ENOMEM; >> + >> + data = iio_priv(indio_dev); >> + >> + mutex_init(&data->mutex); >> + >> + data->regmap = devm_regmap_init_i2c(client, &apds9306_regmap); >> + if (IS_ERR(data->regmap)) >> + return dev_err_probe(dev, PTR_ERR(data->regmap), >> + "regmap initialization failed\n"); >> + >> + data->dev = dev; >> + i2c_set_clientdata(client, indio_dev); >> + >> + ret = apds9306_regfield_init(data); >> + if (ret) >> + return dev_err_probe(dev, ret, "regfield initialization failed\n"); >> + >> + ret = devm_regulator_get_enable(dev, "vdd"); >> + if (ret) >> + return dev_err_probe(dev, ret, "Failed to enable regulator\n"); >> + >> + indio_dev->name = "apds9306"; >> + indio_dev->modes = INDIO_DIRECT_MODE; >> + if (client->irq) { >> + indio_dev->info = &apds9306_info; >> + indio_dev->channels = apds9306_channels_with_events; >> + indio_dev->num_channels = >> + ARRAY_SIZE(apds9306_channels_with_events); >> + ret = devm_request_threaded_irq(dev, client->irq, NULL, >> + apds9306_irq_handler, IRQF_ONESHOT, >> + "apds9306_event", indio_dev); >> + if (ret) >> + return dev_err_probe(dev, ret, "failed to assign interrupt.\n"); >> + } else { >> + indio_dev->info = &apds9306_info_no_events; >> + indio_dev->channels = apds9306_channels_without_events; >> + indio_dev->num_channels = ARRAY_SIZE(apds9306_channels_without_events); >> + } >> + >> + ret = apds9306_pm_init(data); >> + if (ret) >> + return dev_err_probe(dev, ret, "failed pm init\n"); >> + >> + ret = apds9306_device_init(data); >> + if (ret) >> + return dev_err_probe(dev, ret, "failed to init device\n"); >> + >> + ret = devm_add_action_or_reset(dev, apds9306_powerdown, data); >> + if (ret) >> + return dev_err_probe(dev, ret, "failed to add action or reset\n"); >> + >> + ret = devm_iio_device_register(dev, indio_dev); >> + if (ret) >> + return dev_err_probe(dev, ret, "failed iio device registration\n"); >> + >> + pm_runtime_put_autosuspend(dev); > > Where is the matching get? I don't recall any of the pm functions > leaving us with the reference count raised except for the where it is > called out in the function name. > I blindly copy pasted your below suggestion. https://lore.kernel.org/all/20231028162025.4259f1cc@jic23-huawei/ "this lot of runtime pm stuff isn't initializing the device, so I don't see it as making sense in here. I'd push it out to the caller with the power up before init and the autosuspend etc after. I'll note that I'd expect to see a a pm_runtime_put_autosuspend() at the end of probe to put device to sleep soon after loading." > The runtime pm reference counters are protected against underflowing so this > probably just has no impact. Still good to only have it if necessary and if > you do need the power to be on until this point, force it to do so by > an appropriate pm_runtime_get(). I will use a pm_runtime_get() in the apds9306_pm_init() function above. > > >> + >> + return 0; >> +} > Thank you for your review. Regards, Subhajit Ghosh
On 21/1/24 19:52, Christophe JAILLET wrote: > Le 21/01/2024 à 06:17, Subhajit Ghosh a écrit : >> Driver support for Avago (Broadcom) APDS9306 Ambient Light Sensor. >> It has two channels - ALS and CLEAR. The ALS (Ambient Light Sensor) >> channel approximates the response of the human-eye providing direct >> read out where the output count is proportional to ambient light levels. >> It is internally temperature compensated and rejects 50Hz and 60Hz flicker >> caused by artificial light sources. Hardware interrupt configuration is >> optional. It is a low power device with 20 bit resolution and has >> configurable adaptive interrupt mode and interrupt persistence mode. >> The device also features inbuilt hardware gain, multiple integration time >> selection options and sampling frequency selection options. >> >> This driver also uses the IIO GTS (Gain Time Scale) Helpers Namespace for >> Scales, Gains and Integration time implementation. >> >> Signed-off-by: Subhajit Ghosh <subhajit.ghosh-ojZBjWEdjYKukZHgTAicrQ@public.gmane.org> >> --- > > Hi, > > a few nits and a few real comment/question below. > > Just my 2c. > > CJ > ... >> + >> +static int apds9306_read_event(struct iio_dev *indio_dev, >> + const struct iio_chan_spec *chan, >> + enum iio_event_type type, >> + enum iio_event_direction dir, >> + enum iio_event_info info, >> + int *val, int *val2) >> +{ >> + struct apds9306_data *data = iio_priv(indio_dev); >> + int ret; > > Other functions below that look really similar have a: > guard(mutex)(&data->mutex); > > Is it needed here? You are right, don't think so. Regmap lock is being used. I will review the locking mechanism. Acknowledging all other comments. Thanks for the review. Regards, Subhajit Ghosh
Hi Jonathan, >> >>> + >>> +static struct iio_event_spec apds9306_event_spec_als[] = { >>> + { >>> + .type = IIO_EV_TYPE_THRESH, >>> + .dir = IIO_EV_DIR_RISING, >>> + .mask_shared_by_all = BIT(IIO_EV_INFO_VALUE), >>> + }, { >>> + .type = IIO_EV_TYPE_THRESH, >>> + .dir = IIO_EV_DIR_FALLING, >>> + .mask_shared_by_all = BIT(IIO_EV_INFO_VALUE), >>> + }, { >>> + .type = IIO_EV_TYPE_THRESH, >>> + .mask_shared_by_all = BIT(IIO_EV_INFO_PERIOD), >>> + }, { >>> + .type = IIO_EV_TYPE_THRESH_ADAPTIVE, >>> + .mask_shared_by_all = BIT(IIO_EV_INFO_VALUE) | >>> + BIT(IIO_EV_INFO_ENABLE), >>> + }, { >>> + .mask_separate = BIT(IIO_EV_INFO_ENABLE), >> >> What's the intent of this final entry? >> The type will default to IIO_EV_TYPE_THRESH anyway but if that >> the intent you should specify it. There isn't an 'obvious' >> default for type in the same way there sort of is for dir >> (as it's either direction). > Understood, let me experiment and see the ABI difference, if any and get back to you. > This device has two channels - ALS and CLEAR. One interrupt enable option and one Channel selection option (Clear or ALS). According to our previous discussions: https://lore.kernel.org/all/20230415183543.6d5e3392@jic23-huawei/ the event_spec was updated to have two interrupt enable attributes - one for CLEAR and one for ALS. (Intensity channel and Illuminance channel) If I remove the final entry I am getting only one enable option (intensity channel): /sys/bus/iio/devices/iio:device0/ |-- events | |-- in_intensity_clear_thresh_either_en | |-- thresh_adaptive_either_en | |-- thresh_adaptive_either_value | |-- thresh_adaptive_either_values_available | |-- thresh_either_period | |-- thresh_either_period_available | |-- thresh_falling_value | `-- thresh_rising_value The last entry gives be the following event attributes, enable attributes for both intensity and illuminance channels: /sys/bus/iio/devices/iio:device0/ |-- events | |-- in_illuminance_thresh_either_en | |-- in_intensity_clear_thresh_either_en | |-- thresh_adaptive_either_en | |-- thresh_adaptive_either_value | |-- thresh_adaptive_either_values_available | |-- thresh_either_period | |-- thresh_either_period_available | |-- thresh_falling_value | `-- thresh_rising_value Please let me know if this sounds ok to you. Regards, Subhajit Ghosh
On Sun, 4 Feb 2024 21:53:55 +1030 Subhajit Ghosh <subhajit.ghosh@tweaklogic.com> wrote: > Hi Jonathan, > >> > >>> + > >>> +static struct iio_event_spec apds9306_event_spec_als[] = { > >>> + { > >>> + .type = IIO_EV_TYPE_THRESH, > >>> + .dir = IIO_EV_DIR_RISING, > >>> + .mask_shared_by_all = BIT(IIO_EV_INFO_VALUE), > >>> + }, { > >>> + .type = IIO_EV_TYPE_THRESH, > >>> + .dir = IIO_EV_DIR_FALLING, > >>> + .mask_shared_by_all = BIT(IIO_EV_INFO_VALUE), > >>> + }, { > >>> + .type = IIO_EV_TYPE_THRESH, > >>> + .mask_shared_by_all = BIT(IIO_EV_INFO_PERIOD), > >>> + }, { > >>> + .type = IIO_EV_TYPE_THRESH_ADAPTIVE, > >>> + .mask_shared_by_all = BIT(IIO_EV_INFO_VALUE) | > >>> + BIT(IIO_EV_INFO_ENABLE), > >>> + }, { > >>> + .mask_separate = BIT(IIO_EV_INFO_ENABLE), > >> > >> What's the intent of this final entry? > >> The type will default to IIO_EV_TYPE_THRESH anyway but if that > >> the intent you should specify it. There isn't an 'obvious' > >> default for type in the same way there sort of is for dir > >> (as it's either direction). > > Understood, let me experiment and see the ABI difference, if any and get back to you. > > > This device has two channels - ALS and CLEAR. One interrupt enable option and > one Channel selection option (Clear or ALS). According to our previous discussions: > https://lore.kernel.org/all/20230415183543.6d5e3392@jic23-huawei/ > the event_spec was updated to have two interrupt enable attributes - one for CLEAR and > one for ALS. (Intensity channel and Illuminance channel) > > If I remove the final entry I am getting only one enable option (intensity channel): > /sys/bus/iio/devices/iio:device0/ > |-- events > | |-- in_intensity_clear_thresh_either_en > | |-- thresh_adaptive_either_en > | |-- thresh_adaptive_either_value > | |-- thresh_adaptive_either_values_available > | |-- thresh_either_period > | |-- thresh_either_period_available > | |-- thresh_falling_value > | `-- thresh_rising_value > > The last entry gives be the following event attributes, enable attributes for both > intensity and illuminance channels: > /sys/bus/iio/devices/iio:device0/ > |-- events > | |-- in_illuminance_thresh_either_en > | |-- in_intensity_clear_thresh_either_en > | |-- thresh_adaptive_either_en > | |-- thresh_adaptive_either_value > | |-- thresh_adaptive_either_values_available > | |-- thresh_either_period > | |-- thresh_either_period_available > | |-- thresh_falling_value > | `-- thresh_rising_value > > Please let me know if this sounds ok to you. Looks like coincidence of enum values being 0. It's really { .type = IIO_EV_TYPE_THRESH, /* Value 0 */ .dir = IIO_EV_DIR_EITHER, /* value 0 */ .mask_separate = BIT(IIO_EV_INFO_ENABLE), Dropping 'defaults' for these things is fine if they are the obvious default or other parameters mean they aren't used, but that isn't the case here so please be explicit for all the values that are used. You can put this final mask a few lines earlier as the other fields match anyway. { .type = IIO_EV_TYPE_THRESH, .dir = IIO_EV_DIR_EITHER, .mask_shared_by_all = BIT(IIO_EV_INFO_PERIOD), .mask_separate = BIT(IIO_EV_INFO_ENABLE), },.. > > Regards, > Subhajit Ghosh