Re: [PATCH 3/4] iio: light: al3010: Implement regmap support

[Jonathan Cameron <jic23@kernel.org>]

On Sun, 9 Mar 2025 22:11:57 +0100
David Heidelberg <david@ixit.cz> wrote:

> On 09/03/2025 17:45, Jonathan Cameron wrote:
> > On Sat, 08 Mar 2025 21:01:00 +0100
> > David Heidelberg via B4 Relay <devnull+david.ixit.cz@kernel.org> wrote:
> >   
> >> From: David Heidelberg <david@ixit.cz>
> >>
> >> Modernize and make driver a bit cleaner.
> >>
> >> Incorporate most of the feedback given on new AL3000A.  
> > Hi David,
> > 
> > Why does regmap bring benefits here?  This seems to be a like for like
> > change (no use of additional helpers / caching etc) so I'm not immediately
> > seeing the advantage.  
> 
> As I mentioned in the summary, the change is smaller binary size and 
> being in sync with al3000a. Since al3xxx series drivers are pretty close 
> to each other, I believe for future maintenance having them in pair, 
> where it's possible is beneficial.

Description should be here (briefly) as this is what ends up in
the git change log.

> > 
> > Various comments inline. Main one is this is doing several not particularly
> > closely related changes that belong in separate patches.  
> 
> I'm aware I should likely address the changes in smaller chunks, but as 
> I get this patch tested, it's fairly small patch, so I would believe 
> it's still bearable size of the change? If not, I'll split changes into 
> separate patches.

Separate patches still preferred even when the overall changeset
is not large.  It allows for crisper description and easy review.

...

> >> @@ -119,21 +118,21 @@ static int al3010_read_raw(struct iio_dev *indio_dev,
> >>   		 * - low byte of output is stored at AL3010_REG_DATA_LOW
> >>   		 * - high byte of output is stored at AL3010_REG_DATA_LOW + 1
> >>   		 */
> >> -		ret = i2c_smbus_read_word_data(data->client,
> >> -					       AL3010_REG_DATA_LOW);
> >> -		if (ret < 0)
> >> +		ret = regmap_read(data->regmap, AL3010_REG_DATA_LOW, &value);
> >> +		if (ret)
> >>   			return ret;
> >> -		*val = ret;
> >> +
> >> +		*val = value;
> >> +
> >>   		return IIO_VAL_INT;
> >>   	case IIO_CHAN_INFO_SCALE:
> >> -		ret = i2c_smbus_read_byte_data(data->client,
> >> -					       AL3010_REG_CONFIG);
> >> -		if (ret < 0)
> >> +		ret = regmap_read(data->regmap, AL3010_REG_CONFIG, &value);
> >> +		if (ret)
> >>   			return ret;
> >>   
> >> -		ret = FIELD_GET(AL3010_GAIN_MASK, ret);
> >> -		*val = al3010_scales[ret][0];
> >> -		*val2 = al3010_scales[ret][1];
> >> +		value = FIELD_GET(AL3010_GAIN_MASK, value);  
> > I'm never a big fan of conflating use of one variable for the register value
> > (where value is a reasonable name) and the field extract from it where
> > it's not really. scale_idx or something like that would make more sense for
> > this second case.  
> 
> I originally had name gain for this one, but decided to go with generic 
> value to cover all cases. If you want, I can go back to custom name per 
> case.

Please do.

> >   
> >> +		*val = al3010_scales[value][0];
> >> +		*val2 = al3010_scales[value][1];
> >>   
> >>   		return IIO_VAL_INT_PLUS_MICRO;
> >>   	}
> >> @@ -145,7 +144,7 @@ static int al3010_write_raw(struct iio_dev *indio_dev,
> >>   			    int val2, long mask)
> >>   {
> >>   	struct al3010_data *data = iio_priv(indio_dev);
> >> -	int i;
> >> +	unsigned int i;  
> > 
> > Looks like an unrelated change.  Possibly even one that isn't worth making.  
> 
> Well, I was at editing and as i is used within array id, it cannot be 
> signed integer, second it's also compared against u8, so IMHO it make 
> sense here.

I wouldn't bother, but if you do want to, it's an unrelated change
so separate patch.

Jonathan