| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <49dfbb2e-5caf-42c0-8917-baf59b6533dd@ixit.cz>
Date: Sun, 9 Mar 2025 22:11:57 +0100
From: David Heidelberg <david@...t.cz>
To: Jonathan Cameron <jic23@...nel.org>,
David Heidelberg via B4 Relay <devnull+david.ixit.cz@...nel.org>
Cc: Lars-Peter Clausen <lars@...afoo.de>,
Svyatoslav Ryhel <clamor95@...il.com>,
Robert Eckelmann <longnoserob@...il.com>, linux-iio@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/4] iio: light: al3010: Implement regmap support
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@...nel.org> wrote:
>
>> From: David Heidelberg <david@...t.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.
>
> 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.
>
> Jonathan
>
>>
>> Signed-off-by: David Heidelberg <david@...t.cz>
>> ---
>> drivers/iio/light/al3010.c | 95 ++++++++++++++++++++++++++--------------------
>> 1 file changed, 53 insertions(+), 42 deletions(-)
>>
>> diff --git a/drivers/iio/light/al3010.c b/drivers/iio/light/al3010.c
>> index 7cbb8b203300907a88f4a0ab87da89cabdd087f3..f6ed7246864a777fdb7d3861b74f5834e8af4105 100644
>> --- a/drivers/iio/light/al3010.c
>> +++ b/drivers/iio/light/al3010.c
>> @@ -4,7 +4,7 @@
>> *
>> * Copyright (c) 2014, Intel Corporation.
>> * Copyright (c) 2016, Dyna-Image Corp.
>> - * Copyright (c) 2020, David Heidelberg, Michał Mirosław, Dmitry Osipenko
>> + * Copyright (c) 2020 - 2025, David Heidelberg, Michał Mirosław, Dmitry Osipenko
>
> This implies all 3 of you were involved in this update. If that's not
> the case perhaps just add a new copyright line for this change.
>
>> *
>> * IIO driver for AL3010 (7-bit I2C slave address 0x1C).
>> *
>
>>
>> static const struct iio_chan_spec al3010_channels[] = {
>> @@ -69,40 +76,32 @@ static const struct attribute_group al3010_attribute_group = {
>> .attrs = al3010_attributes,
>> };
>>
>> -static int al3010_set_pwr(struct i2c_client *client, bool pwr)
>> +static int al3010_set_pwr_on(struct al3010_data *data)
>> {
>> - u8 val = pwr ? AL3010_CONFIG_ENABLE : AL3010_CONFIG_DISABLE;
>> - return i2c_smbus_write_byte_data(client, AL3010_REG_SYSTEM, val);
>> + return regmap_write(data->regmap, AL3010_REG_SYSTEM, AL3010_CONFIG_ENABLE);
> Splitting this write into the on and off cases is a change that is
> not closely related to regmap change, so probably wants to be in a separate
> patch.
Sure, I can do.
>
>> }
>>
>> static void al3010_set_pwr_off(void *_data)
>> {
>> struct al3010_data *data = _data;
>> + struct device *dev = regmap_get_device(data->regmap);
>> + int ret;
>>
>> - al3010_set_pwr(data->client, false);
>> + ret = regmap_write(data->regmap, AL3010_REG_SYSTEM, AL3010_CONFIG_DISABLE);
>> + if (ret)
>> + dev_err(dev, "failed to write system register\n");
>> }
>>
>> static int al3010_init(struct al3010_data *data)
>> {
>> int ret;
>>
>> - ret = al3010_set_pwr(data->client, true);
>> - if (ret < 0)
>> - return ret;
>> -
>> - ret = devm_add_action_or_reset(&data->client->dev,
>> - al3010_set_pwr_off,
>> - data);
>
> As below. Not obvious to me why we'd want to move this.
>
>> - if (ret < 0)
>> - return ret;
>> -
>> - ret = i2c_smbus_write_byte_data(data->client, AL3010_REG_CONFIG,
>> - FIELD_PREP(AL3010_GAIN_MASK,
>> - AL3XXX_RANGE_3));
>> - if (ret < 0)
>> + ret = al3010_set_pwr_on(data);
>> + if (ret)
>> return ret;
>>
>> - return 0;
>> + return regmap_write(data->regmap, AL3010_REG_CONFIG,
>> + FIELD_PREP(AL3010_GAIN_MASK, AL3XXX_RANGE_3));
>> }
>>
>> static int al3010_read_raw(struct iio_dev *indio_dev,
>> @@ -110,7 +109,7 @@ static int al3010_read_raw(struct iio_dev *indio_dev,
>> int *val2, long mask)
>> {
>> struct al3010_data *data = iio_priv(indio_dev);
>> - int ret;
>> + int ret, value;
>>
>> switch (mask) {
>> case IIO_CHAN_INFO_RAW:
>> @@ -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.
>
>> + *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.
>
>>
>> switch (mask) {
>> case IIO_CHAN_INFO_SCALE:
>> @@ -154,9 +153,8 @@ static int al3010_write_raw(struct iio_dev *indio_dev,
>> val2 != al3010_scales[i][1])
>> continue;
>>
>> - return i2c_smbus_write_byte_data(data->client,
>> - AL3010_REG_CONFIG,
>> - FIELD_PREP(AL3010_GAIN_MASK, i));
>> + return regmap_write(data->regmap, AL3010_REG_CONFIG,
>> + FIELD_PREP(AL3010_GAIN_MASK, i));
>> }
>> break;
>> }
>> @@ -172,16 +170,20 @@ static const struct iio_info al3010_info = {
>> static int al3010_probe(struct i2c_client *client)
>> {
>> struct al3010_data *data;
>> + struct device *dev = &client->dev;
>
> This is confusing two things. I'd prefer a precursor patch that
> adds the local variable followed by one that adds the regmap stuff.
Sure, I can split it into additional patch.
>
>> struct iio_dev *indio_dev;
>> int ret;
>>
>> - indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
>> + indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
>> if (!indio_dev)
>> return -ENOMEM;
>>
>> data = iio_priv(indio_dev);
>> i2c_set_clientdata(client, indio_dev);
>> - data->client = client;
>> + data->regmap = devm_regmap_init_i2c(client, &al3010_regmap_config);
>> + if (IS_ERR(data->regmap))
>> + return dev_err_probe(dev, PTR_ERR(data->regmap),
>> + "cannot allocate regmap\n");
>>
>> indio_dev->info = &al3010_info;
>> indio_dev->name = AL3010_DRV_NAME;
>> @@ -191,21 +193,30 @@ static int al3010_probe(struct i2c_client *client)
>>
>> ret = al3010_init(data);
>> if (ret < 0) {
>> - dev_err(&client->dev, "al3010 chip init failed\n");
>> + dev_err(dev, "failed to init ALS\n");
>> return ret;
>> }
>>
>> - return devm_iio_device_register(&client->dev, indio_dev);
>> + ret = devm_add_action_or_reset(dev, al3010_set_pwr_off, data);
>
> Moving this out here doesn't look like a change related to regmap.
> Generally I'd prefer that stayed next to where the power on is as this
> is not obviously undoing the al3010_init() given naming etc.
Sure, I'll move it back.
Thank you for the feedback,
>
>> + if (ret < 0)
>> + return ret;
>> +
>> + return devm_iio_device_register(dev, indio_dev);
>> }
--
David Heidelberg
Powered by blists - more mailing lists