Received-SPF: pass (google.com: domain of linux-kernel+bounces-52901-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Received-SPF: Pass (protection.outlook.com: domain of topic.nl designates
 20.93.157.195 as permitted sender) receiver=protection.outlook.com;
 client-ip=20.93.157.195; helo=westeu11-emailsignatures-cloud.codetwo.com;
 pr=C
Authentication-Results-Original: dkim=none (message not signed)
 header.d=none;dmarc=none action=none header.from=topic.nl;
Message-ID: <7d7ea4e4-fcae-4966-b194-e1d328751b6b@topic.nl>
Date: Mon, 5 Feb 2024 16:25:19 +0100
User-Agent: Mozilla Thunderbird
From: Mike Looijmans <mike.looijmans@topic.nl>
Subject: Re: [PATCH v2 2/2] iio: adc: ti-ads1298: Add driver
Content-Language: en-US, nl
To: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
CC: devicetree@vger.kernel.org, linux-iio@vger.kernel.org,
 Jonathan Cameron <jic23@kernel.org>, Lars-Peter Clausen <lars@metafoo.de>,
 Liam Beguin <liambeguin@gmail.com>, Liam Girdwood <lgirdwood@gmail.com>,
 Maksim Kiselev <bigunclemax@gmail.com>,
 Marcus Folkesson <marcus.folkesson@gmail.com>,
 Marius Cristea <marius.cristea@microchip.com>,
 Mark Brown <broonie@kernel.org>, Niklas Schnelle <schnelle@linux.ibm.com>,
 Okan Sahin <okan.sahin@analog.com>, linux-kernel@vger.kernel.org
References: <ZcDo6QvoE_e5s_f1@smile.fi.intel.com>
Organization: Topic
In-Reply-To: <ZcDo6QvoE_e5s_f1@smile.fi.intel.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: quoted-printable
Precedence: bulk
MIME-Version: 1.0
X-Microsoft-Antispam-Message-Info-Original:
 VQSowW7k64VJsULxJxT3Pb5JnexvdaCK/pnVhRng3LZStTpOjW+ZpGSYP+uaGp+CYPG8IUyhH+J6NkVd3bvF1jFOOnU4bHoEvik2NDIdSLKDSDQUsR8HfKXwFDNDK07GPlIgtOeRrFw9uv2CwZUPd7gdH3QRExHR9AQL8Bp8GpUataioJEvFZS1Q9tLg2IIvQzMLEJ4KBQ4nXe9yKZCzKTyN8psR6hxAiaSLpICGN14KdU9YOizX/KI5hgs37PVQUSIywoFI6vZJQwqiTIZ2ULCp8pn4tsvDIR2GV5ejlU/lLurr+JahKcNvh5wxnxBnccpIH2GVYo9vZ0o4g6dvo3j+sgTHq2ePffr6k960+4Zw9Dq3wGWPfZNsLcSeFrLxiPbKpI66afryli25Gi3BAp75/UoxfJUvhQJ/k2A0VUM7sYyAgHObhSmhewaUR9qcuY9AxzSsIfWYzS86G4smmuiSDJiJIXdx7At9GmaovWJ0gkIHyF/VfFoFmO2bg6J4G3EGVDh+tbEGuiNVtfFIrffdYQMzADzrMm12BCleRlZGLsaSVqs5aQ9R3YeH7s0FF9gVb1Dfi2a+1sQGtQEsKstxC4QOU88aolSDtWret1iBwsCsHiwci9/P9j6C5furEM7x/OJQqCYT65d6ZZ1TcVLrXiuuUTRDXXOJ5MwSYOc=
X-Forefront-Antispam-Report-Untrusted:
 CIP:255.255.255.255;CTRY:;LANG:en;SCL:1;SRV:;IPV:NLI;SFV:NSPM;H:AS8PR04MB8644.eurprd04.prod.outlook.com;PTR:;CAT:NONE;SFS:(13230031)(366004)(136003)(396003)(39840400004)(376002)(346002)(230922051799003)(186009)(451199024)(64100799003)(1800799012)(31686004)(15974865002)(36756003)(86362001)(6512007)(53546011)(36916002)(6486002)(478600001)(38350700005)(52116002)(41300700001)(31696002)(26005)(83380400001)(2616005)(66476007)(5660300002)(6506007)(7416002)(316002)(6916009)(66946007)(44832011)(66556008)(4326008)(2906002)(8676002)(54906003)(8936002)(38100700002)(43740500002)(45980500001);DIR:OUT;SFP:1102;
X-MS-Exchange-Transport-CrossTenantHeadersStamped: AM9PR04MB8210
X-CodeTwo-MessageID: 73e4eb7e-9491-485c-9cd0-3c6f3792f17b.20240205152524@westeu11-emailsignatures-cloud.codetwo.com
X-CodeTwoProcessed: true
X-EOPAttributedMessage: 0
X-MS-Exchange-Transport-CrossTenantHeadersStripped:
 AMS0EPF000001AE.eurprd05.prod.outlook.com
X-MS-PublicTrafficType: Email
X-MS-Office365-Filtering-Correlation-Id-Prvs:
	8da3142d-07b9-4a0d-1612-08dc265ea9e1
X-Microsoft-Antispam: BCL:0;
X-Microsoft-Antispam-Message-Info:
	KE47q2OImjwUM2eP3Vo9wLI6IlohQeNaXVaHCwnVkbXDS+qWum41OIDV0clBHK5FmWucC66uLHJoeEziHAoiRqUBCcWWMK0hWy1YL6U79s1Cd19KvyYAWx64DJnr68rbOkF8ADjNc3PcvTNKxhVornDT5TvfD2H2rnQyZA4Tos3Qk3miJM+1rX9OgtD4zHwJdbNQ83PUmvPwodH1XTHMIkArogUDVpHq3d0DFj9JXHoDimKwe7gHlbTSjKbKqBarp3PBr6ikPvYN4drb21+XqswIlPf5DWloZEs0P5oH727dYiIglypKddO2Deuehx8NfO9MZ8CnkwT9/c2ISuHoJutfEhnFB/DUdJTuXrXEPuXTB8EX4bchX8YHp7G+s6mFEOliSqAhoNahOsQggdh3jeB3PLKp55PR6ow2HeQjKGXYBks6h/6SwqzbJewcRR5/K5mt70Hly17zCevHnfm9YB/mwoB4RNUFU6xnDybyCTxQco4jscNY6KM3FBzK/r1qqDrJcGfVjPwj0KHQfBRk1V9C8C/OWjcxVzUmRxyTHdNDtMvTUWM2da2dYj9P8oIf28VNat1vD/XDlUE/bh72q+mvL1M6FtsbsKPd216TbjFDBQkEGiH+X4FyGnQfb5OCrFXhaZnqTXdkA9zQ209Z1pla87of0sWMIX2SxxsbvjesREMeTjKbWsmVjru1YIkXC6UGc/XeBzrAmnniYhrzZNytt/BizurDy2Ggt7TodbGw1xlOXnQfIf3o7aY92E/5
X-Forefront-Antispam-Report:
	CIP:20.93.157.195;CTRY:NL;LANG:en;SCL:1;SRV:;IPV:NLI;SFV:NSPM;H:westeu11-emailsignatures-cloud.codetwo.com;PTR:westeu11-emailsignatures-cloud.codetwo.com;CAT:NONE;SFS:(13230031)(4636009)(376002)(346002)(136003)(39840400004)(396003)(230922051799003)(186009)(1800799012)(82310400011)(64100799003)(451199024)(46966006)(36840700001)(86362001)(31696002)(336012)(36860700001)(53546011)(2616005)(36756003)(6512007)(83380400001)(356005)(7596003)(478600001)(47076005)(6486002)(31686004)(7636003)(26005)(40480700001)(6506007)(36916002)(15974865002)(4326008)(316002)(6916009)(54906003)(70586007)(70206006)(5660300002)(41300700001)(44832011)(7416002)(2906002)(8936002)(8676002)(43740500002)(18886075002);DIR:OUT;SFP:1102;
X-OriginatorOrg: topic.nl
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 05 Feb 2024 15:25:25.9396
 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: dbaabe1d-b93e-4345-5087-08dc265ead8e
X-MS-Exchange-CrossTenant-Id: 449607a5-3517-482d-8d16-41dd868cbda3
X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=449607a5-3517-482d-8d16-41dd868cbda3;Ip=[20.93.157.195];Helo=[westeu11-emailsignatures-cloud.codetwo.com]
X-MS-Exchange-CrossTenant-AuthSource:
	AMS0EPF000001AE.eurprd05.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Anonymous
X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem
X-MS-Exchange-Transport-CrossTenantHeadersStamped: AM8PR04MB7987

Assume "yes" to all suggestions not mentioned below.

I'll send out a v3 later.


On 05-02-2024 14:55, Andy Shevchenko wrote:
> On Fri, Feb 02, 2024 at 04:28:07PM +0200, Andy Shevchenko wrote:
>> On Fri, Feb 02, 2024 at 11:59:01AM +0100, Mike Looijmans wrote:
> Hit "Send" by a mistake, here is the full review.
>
> ...
>
> Why this can't be the part of drivers/iio/adc/ti-ads124s08.c?
> Seems to me the command list is the same, registers are different though.
> Broadly the Q is have you checked other existing drivers if they can
> be used as a base. If not, perhaps a word in the cover letter is good to =
have
> (sorry if I asked this already).
>
> ...

The ads124s08 command list is the same, but that's about all that's similar=
.

>>> +	struct spi_transfer rdata_xfer;
>>> +	struct spi_message rdata_msg;
> Do you use this outside of the ->probe()? I just ask since I removed some
> context already...

Yes, probe() initializes the structs, they're used in the interrupt=20
routines.


>
>>> +	spinlock_t irq_busy_lock; /* Handshake between SPI and DRDY irqs */
>>> +	int rdata_xfer_busy;
>>> +
>>> +	/* Temporary storage for demuxing data after SPI transfer */
>>> +	u32 bounce_buffer[ADS1298_MAX_CHANNELS];
>>> +
>>> +	/* For synchronous SPI exchanges (read/write registers) */
>>> +	u8 cmd_buffer[ADS1298_SPI_CMD_BUFFER_SIZE] __aligned(IIO_DMA_MINALIGN=
);
>>> +
>>> +	/* Buffer used for incoming SPI data */
>>> +	u8 rx_buffer[ADS1298_SPI_RDATA_BUFFER_SIZE];
> Cacheline aligned?
> I see the cmd_buffer, but shouldn't this be also aligned?

I understood from Jonathan that that wasn't needed... "My" SPI=20
controller is rather dumb and doesn't even have DMA.

Will take a closer look though.

>>> +	else
>>> +		rate =3D ADS1298_CLK_RATE_HZ;
> Dead code (here and elsewhere). You probably wanted _optional clk APIs
> in the probe.

Yes "optional" was intended.

> ...
>
>>> +static int ads1298_reg_write(void *context, unsigned int reg, unsigned=
 int val)
>>> +{
>>> +	struct ads1298_private *priv =3D context;
>>> +	struct spi_transfer reg_write_xfer =3D {
>>> +		.tx_buf =3D priv->cmd_buffer,
>>> +		.rx_buf =3D priv->cmd_buffer,
>>> +		.len =3D 3,
>>> +		.speed_hz =3D ADS1298_SPI_BUS_SPEED_SLOW,
>>> +		.delay =3D {
>>> +			.value =3D 2,
>>> +			.unit =3D SPI_DELAY_UNIT_USECS,
>>> +		},
>>> +	};
>>> +
>>> +	priv->cmd_buffer[0] =3D ADS1298_CMD_WREG | reg;
>>> +	priv->cmd_buffer[1] =3D 0x0;
>>> +	priv->cmd_buffer[2] =3D val;
> Sounds to me like put_unaligned_be16().

Added comment to explain the first zero is the "number of registers to=20
read/write minus one".

> ...
>
>>> +	unsigned long flags;
>>> +
>>> +	/* Notify we're no longer waiting for the SPI transfer to complete */
>>> +	spin_lock_irqsave(&priv->irq_busy_lock, flags);
>>> +	priv->rdata_xfer_busy =3D 0;
>>> +	spin_unlock_irqrestore(&priv->irq_busy_lock, flags);
> Use cleanup.h?
>
> ...

Will dive into it, is new to me... Looks like C++ "scoped_xxx" at a=20
first glance.


>
>>> +static int ads1298_update_scan_mode(struct iio_dev *indio_dev,
>>> +				    const unsigned long *scan_mask)
>>> +{
>>> +	struct ads1298_private *priv =3D iio_priv(indio_dev);
>>> +	unsigned int val;
>>> +	int ret;
>>> +	int i;
>>> +
>>> +	/* Make the interrupt routines start with a clean slate */
>>> +	ads1298_rdata_unmark_busy(priv);
>>> +
>>> +	/* Power down channels that aren't in use */
> This comment does not describe why you need to write to _all_ channels.

Yeah, need to configure them all. Most common is that the cached value=20
is already okay and no actual write register will happen.

>
>>> +	for (i =3D 0; i < ADS1298_MAX_CHANNELS; i++) {
>>> +		val =3D test_bit(i, scan_mask) ? 0 : ADS1298_MASK_CH_PD;
> With above in mind, this perhaps needs to be one of for_each_set_bit(scan=
_mask) /
> for_each_clear_bit(scan_mask).

Probably more efficient to access the device registers in a natural order.

> ...
>
>>> +	.cache_type =3D REGCACHE_RBTREE,
> Why not MAPPLE TREE?

Reading the description this driver isn't a good candidate - the map=20
isn't sparse and the hardware can do bulk read/write (though the driver=20
doesn't use it).


..


--=20
Mike Looijmans
System Expert

TOPIC Embedded Products B.V.
Materiaalweg 4, 5681 RJ Best
The Netherlands

T: +31 (0) 499 33 69 69
E: mike.looijmans@topic.nl
W: www.topic.nl