Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   "Paller, Kim Seer" <KimSeer.Paller@analog.com>
To:     "andy.shevchenko@gmail.com" <andy.shevchenko@gmail.com>
CC:     "jic23@kernel.org" <jic23@kernel.org>,
        "lars@metafoo.de" <lars@metafoo.de>,
        "krzysztof.kozlowski@linaro.org" <krzysztof.kozlowski@linaro.org>,
        "broonie@kernel.org" <broonie@kernel.org>,
        "lgirdwood@gmail.com" <lgirdwood@gmail.com>,
        "linux-iio@vger.kernel.org" <linux-iio@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: RE: [PATCH v2 2/2] iio: adc: max14001: New driver
Thread-Topic: [PATCH v2 2/2] iio: adc: max14001: New driver
Thread-Index: AQHZl67Y+VuDoLqrgE+gZ2yoOVHpUq99NPAA///oTSA=
Date:   Tue, 6 Jun 2023 03:39:43 +0000
Message-ID: <a38229ec97934d2fa0c5ab85c8edd325@analog.com>
References: <20230605130755.92642-1-kimseer.paller@analog.com>
 <20230605130755.92642-3-kimseer.paller@analog.com>
 <ZH6CZJCBTXrsUErm@surfacebook>
In-Reply-To: <ZH6CZJCBTXrsUErm@surfacebook>
Accept-Language: en-US
Content-Language: en-US
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 8BIT
MIME-Version: 1.0
Precedence: bulk


> -----Original Message-----
> From: andy.shevchenko@gmail.com <andy.shevchenko@gmail.com>
> Sent: Tuesday, June 6, 2023 8:49 AM
> To: Paller, Kim Seer <KimSeer.Paller@analog.com>
> Cc: jic23@kernel.org; lars@metafoo.de; krzysztof.kozlowski@linaro.org;
> broonie@kernel.org; lgirdwood@gmail.com; linux-iio@vger.kernel.org; linux-
> kernel@vger.kernel.org
> Subject: Re: [PATCH v2 2/2] iio: adc: max14001: New driver
> 
> [External]
> 
> Mon, Jun 05, 2023 at 09:07:55PM +0800, Kim Seer Paller kirjoitti:
> > The MAX14001 is configurable, isolated 10-bit ADCs for multi-range
> > binary inputs.
> 
> First question, why don't you use regmap SPI?

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. Additionally, the regmap_config 
parameters, specifically reg_bits and val_bits, cannot be set to specific values. 
This limitation makes it challenging to accommodate specific configurations 
such as using 5 bits for register address and 10 bits for data.

> 
> ...
> 
> > +static int max14001_read(void *context, unsigned int reg_addr,
> > +					unsigned int *data)
> 
> Strange indentation.
> 
> > +{
> > +	struct max14001_state *st = context;
> > +	u16 tx = 0;
> 
> Redundant assignment.
> 
> > +	int ret;
> > +
> > +	struct spi_transfer xfers[] = {
> > +		{
> > +			.tx_buf = &st->spi_tx_buffer,
> > +			.len = 2,
> > +			.cs_change = 1,
> > +		}, {
> > +			.rx_buf = &st->spi_rx_buffer,
> > +			.len = 2,
> > +		},
> > +	};
> > +
> > +	tx = FIELD_PREP(MAX14001_ADDR_MASK, reg_addr);
> 
> > +	st->spi_tx_buffer = bitrev16(cpu_to_be16(tx));
> 
> This is strange. Why this monsteur construct with bitrev16() is used?
> 
> According to the datasheet, the bit stream is LE16, where 10 LSB is data,
> 5 MSB is address and bit 11 is R/W.

I discovered that following the instructions in the application note made 
the implementation much easier. 
https://www.analog.com/media/en/technical-documentation/user-guides/guide-to-programming-the-max14001max14002-isolated-adcs--maxim-integrated.pdf

To ensure compatibility with the device, I changed the format to big 
endian and reversed the data before sending it to the device. 
This is why I used the "bitrev" function. I'm happy to report that 
this approach worked perfectly.

> 
> > +	ret = spi_sync_transfer(st->spi, xfers, ARRAY_SIZE(xfers));
> > +	if (ret)
> > +		return ret;
> 
> > +	*data = bitrev16(be16_to_cpu(st->spi_rx_buffer)) &
> > +MAX14001_DATA_MASK;
> 
> Ditto.
> 
> > +	return 0;
> > +}
> 
> > +static int max14001_write(void *context, unsigned int reg_addr,
> > +					unsigned int data)
> > +{
> > +	struct max14001_state *st = context;
> > +	u16 tx = 0;
> 
> Redundant assignment.
> 
> > +	tx = FIELD_PREP(MAX14001_ADDR_MASK, reg_addr);
> > +	tx |= FIELD_PREP(MAX14001_SET_WRITE_BIT, 1);
> > +	tx |= FIELD_PREP(MAX14001_DATA_MASK, data);
> > +
> > +	st->spi_tx_buffer = bitrev16(cpu_to_be16(tx));
> > +
> > +	return spi_write(st->spi, &st->spi_tx_buffer, 2);
> 
> sizeof() ?
> 
> > +}
> 
> ...
> 
> > +			return dev_err_probe(&spi->dev, PTR_ERR(vref),
> > +					     "Failed to get vref regulator");
> 
> With
> 
> 	struct device *dev = &spi->dev;
> 
> this and other calls in this function can be made neater.
> 
> --
> With Best Regards,
> Andy Shevchenko
>