diff mbox series

iio: proximity: hx9023s: Added firmware file parsing functionality

Message ID 20241210-hx9023s-firmware-20241209-v1-1-8a736691b106@gmail.com (mailing list archive)
State Changes Requested
Headers show
Series iio: proximity: hx9023s: Added firmware file parsing functionality | expand

Commit Message

Yasin Lee Dec. 10, 2024, 4:24 a.m. UTC 
Configuration information is now prioritized from the firmware file.
If the firmware file is missing or fails to parse, the driver falls
back to using the default configuration list for writing the settings.

Signed-off-by: Yasin Lee <yasin.lee.x@gmail.com>
---
 drivers/iio/proximity/hx9023s.c | 96 ++++++++++++++++++++++++++++++++++++++---
 1 file changed, 89 insertions(+), 7 deletions(-)


---
base-commit: 8d4d26450d71289a35ff9e847675fd9c718798b8
change-id: 20241209-hx9023s-firmware-20241209-47411e8cda0b

Best regards,

Comments

Jonathan Cameron Dec. 15, 2024, 12:25 p.m. UTC | #1 
On Tue, 10 Dec 2024 12:24:03 +0800
Yasin Lee <yasin.lee.x@gmail.com> wrote:

> Configuration information is now prioritized from the firmware file.
> If the firmware file is missing or fails to parse, the driver falls
> back to using the default configuration list for writing the settings.
> 
> Signed-off-by: Yasin Lee <yasin.lee.x@gmail.com>

Ok.  I guess just reading a load of register values isn't hugely different
to more complex firmware loads.

Comments inline - in particular please take the time to tidy up
formatting of your code before posting.

Also why is this +CC linux-hardening, Kees and Gustavo?

They have enough stuff to do without getting patches that seem to have nothing
to do with hardening!

Jonathan

> ---
>  drivers/iio/proximity/hx9023s.c | 96 ++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 89 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/iio/proximity/hx9023s.c b/drivers/iio/proximity/hx9023s.c
> index 4021feb7a7ac..6cb1b688bfa9 100644
> --- a/drivers/iio/proximity/hx9023s.c
> +++ b/drivers/iio/proximity/hx9023s.c
> @@ -14,6 +14,7 @@
>  #include <linux/cleanup.h>
>  #include <linux/device.h>
>  #include <linux/errno.h>
> +#include <linux/firmware.h>
>  #include <linux/i2c.h>
>  #include <linux/interrupt.h>
>  #include <linux/irqreturn.h>
> @@ -100,6 +101,17 @@
>  #define HX9023S_INTERRUPT_MASK GENMASK(9, 0)
>  #define HX9023S_PROX_DEBOUNCE_MASK GENMASK(3, 0)
>  
> +#define FW_VER_OFFSET 2
> +#define FW_REG_CNT_OFFSET 3
> +#define FW_DATA_OFFSET 16
> +
> +struct hx9023s_bin {
> +	u16 reg_count;
> +	u16 fw_size;
> +	u8 fw_ver;
> +	u8 data[] __counted_by(fw_size);
> +};
> +
>  struct hx9023s_ch_data {
>  	s16 raw; /* Raw Data*/
>  	s16 lp; /* Low Pass Filter Data*/
> @@ -998,6 +1010,80 @@ static int hx9023s_id_check(struct iio_dev *indio_dev)
>  	return 0;
>  }
>  
> +static int hx9023s_bin_load(struct hx9023s_data *data,
> +								struct hx9023s_bin *bin)
> +{
> +	u8 *cfg_start = bin->data + FW_DATA_OFFSET;
> +	u8 addr, val;
> +	u16 i;
> +	int ret;
> +
> +	for (i = 0; i < bin->reg_count; i++) {
> +		addr = cfg_start[i * 2];
> +		val = cfg_start[i * 2 + 1];
> +		ret = regmap_write(data->regmap, addr, val);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	return ret;
return 0;
Can't get here with anything else.

> +}
> +
> +static int hx9023s_send_cfg(const struct firmware *fw,
> +								struct hx9023s_data *data)


Fix all your indenting to match kernel style.

> +{
> +	if (!fw)

You can't get to this call below with out fw so drop this check.

> +		return -EINVAL;
> +
> +	struct hx9023s_bin *bin __free(kfree) =
> +		kzalloc(fw->size + sizeof(*bin), GFP_KERNEL);
> +	if (!bin)
> +		return -ENOMEM;
> +
> +	memcpy(bin->data, fw->data, fw->size);
> +	release_firmware(fw);
> +
> +	bin->fw_size = fw->size;
> +	bin->fw_ver = bin->data[FW_VER_OFFSET];
> +	bin->reg_count = get_unaligned_le16(bin->data + FW_REG_CNT_OFFSET);
> +
> +	return hx9023s_bin_load(data, bin);
> +}
> +
> +static void hx9023s_cfg_update(const struct firmware *fw, void *context)
> +{
> +	struct hx9023s_data *data = context;
> +	struct device *dev = regmap_get_device(data->regmap);
> +	int ret;
> +
> +	if (!fw || !fw->data) {
> +		dev_warn(dev, "No firmware\n");
> +		goto no_fw;
> +	}
> +
> +	ret = hx9023s_send_cfg(fw, data);
> +	if (ret)
If this fails, we want to notify the user.  The firmware loaded
but we were unable to use it for some reasons.

> +		goto no_fw;
> +
> +	ret = regcache_sync(data->regmap);
> +	if (ret)
> +		dev_err(dev, "regcache sync failed\n");
> +
> +	return;
> +
> +no_fw:
> +	ret = regmap_multi_reg_write(data->regmap, hx9023s_reg_init_list,
> +								ARRAY_SIZE(hx9023s_reg_init_list));
> +	if (ret) {
> +		dev_err(dev, "Error loading default configuration\n");
> +		return;
> +	}
> +
> +	ret = regcache_sync(data->regmap);
> +	if (ret)
> +		dev_err(dev, "regcache sync failed\n");
> +}
> +
>  static int hx9023s_probe(struct i2c_client *client)
>  {
>  	struct device *dev = &client->dev;
> @@ -1036,18 +1122,14 @@ static int hx9023s_probe(struct i2c_client *client)
>  	indio_dev->modes = INDIO_DIRECT_MODE;
>  	i2c_set_clientdata(client, indio_dev);
>  
> -	ret = regmap_multi_reg_write(data->regmap, hx9023s_reg_init_list,
> -				     ARRAY_SIZE(hx9023s_reg_init_list));
> -	if (ret)
> -		return dev_err_probe(dev, ret, "device init failed\n");
> -
>  	ret = hx9023s_ch_cfg(data);
>  	if (ret)
>  		return dev_err_probe(dev, ret, "channel config failed\n");
>  
> -	ret = regcache_sync(data->regmap);

> +	ret = request_firmware_nowait(THIS_MODULE, true, "hx9023s.bin",
> +						dev, GFP_KERNEL, data, hx9023s_cfg_update);

Indent dev under T of the THIS_MODULE


>  	if (ret)
> -		return dev_err_probe(dev, ret, "regcache sync failed\n");
> +		return dev_err_probe(dev, ret, "reg config failed\n");
>  
>  	if (client->irq) {
>  		ret = devm_request_threaded_irq(dev, client->irq,
> 
> ---
> base-commit: 8d4d26450d71289a35ff9e847675fd9c718798b8
> change-id: 20241209-hx9023s-firmware-20241209-47411e8cda0b
> 
> Best regards,
Yasin Lee Dec. 16, 2024, 8:18 a.m. UTC | #2 
On 12/15/24 20:25, Jonathan Cameron wrote:
> On Tue, 10 Dec 2024 12:24:03 +0800
> Yasin Lee <yasin.lee.x@gmail.com> wrote:
>
>> Configuration information is now prioritized from the firmware file.
>> If the firmware file is missing or fails to parse, the driver falls
>> back to using the default configuration list for writing the settings.
>>
>> Signed-off-by: Yasin Lee <yasin.lee.x@gmail.com>
> Ok.  I guess just reading a load of register values isn't hugely different
> to more complex firmware loads.
>
> Comments inline - in particular please take the time to tidy up
> formatting of your code before posting.
>
> Also why is this +CC linux-hardening, Kees and Gustavo?
>
> They have enough stuff to do without getting patches that seem to have nothing
> to do with hardening!
>
> Jonathan
>
Hi Jonathan,

Thank you for your detailed feedback on my patch. I’ve carefully 
reviewed and addressed all the issues you highlighted:
1. Simplified the return value handling in hx9023s_bin_load to directly 
return 0 where applicable.
2. Corrected all indentation and alignment issues to conform to the 
Linux kernel coding standards.
3. Improved log messages to clearly inform users of specific reasons for 
firmware load failure.
4. Removed irrelevant recipients, such as linux-hardening, from the CC list.

Best regards,
Yasin Lee

>> ---
>>   drivers/iio/proximity/hx9023s.c | 96 ++++++++++++++++++++++++++++++++++++++---
>>   1 file changed, 89 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/iio/proximity/hx9023s.c b/drivers/iio/proximity/hx9023s.c
>> index 4021feb7a7ac..6cb1b688bfa9 100644
>> --- a/drivers/iio/proximity/hx9023s.c
>> +++ b/drivers/iio/proximity/hx9023s.c
>> @@ -14,6 +14,7 @@
>>   #include <linux/cleanup.h>
>>   #include <linux/device.h>
>>   #include <linux/errno.h>
>> +#include <linux/firmware.h>
>>   #include <linux/i2c.h>
>>   #include <linux/interrupt.h>
>>   #include <linux/irqreturn.h>
>> @@ -100,6 +101,17 @@
>>   #define HX9023S_INTERRUPT_MASK GENMASK(9, 0)
>>   #define HX9023S_PROX_DEBOUNCE_MASK GENMASK(3, 0)
>>   
>> +#define FW_VER_OFFSET 2
>> +#define FW_REG_CNT_OFFSET 3
>> +#define FW_DATA_OFFSET 16
>> +
>> +struct hx9023s_bin {
>> +	u16 reg_count;
>> +	u16 fw_size;
>> +	u8 fw_ver;
>> +	u8 data[] __counted_by(fw_size);
>> +};
>> +
>>   struct hx9023s_ch_data {
>>   	s16 raw; /* Raw Data*/
>>   	s16 lp; /* Low Pass Filter Data*/
>> @@ -998,6 +1010,80 @@ static int hx9023s_id_check(struct iio_dev *indio_dev)
>>   	return 0;
>>   }
>>   
>> +static int hx9023s_bin_load(struct hx9023s_data *data,
>> +								struct hx9023s_bin *bin)
>> +{
>> +	u8 *cfg_start = bin->data + FW_DATA_OFFSET;
>> +	u8 addr, val;
>> +	u16 i;
>> +	int ret;
>> +
>> +	for (i = 0; i < bin->reg_count; i++) {
>> +		addr = cfg_start[i * 2];
>> +		val = cfg_start[i * 2 + 1];
>> +		ret = regmap_write(data->regmap, addr, val);
>> +		if (ret < 0)
>> +			return ret;
>> +	}
>> +
>> +	return ret;
> return 0;
> Can't get here with anything else.
>
>> +}
>> +
>> +static int hx9023s_send_cfg(const struct firmware *fw,
>> +								struct hx9023s_data *data)
>
> Fix all your indenting to match kernel style.
>
>> +{
>> +	if (!fw)
> You can't get to this call below with out fw so drop this check.
>
>> +		return -EINVAL;
>> +
>> +	struct hx9023s_bin *bin __free(kfree) =
>> +		kzalloc(fw->size + sizeof(*bin), GFP_KERNEL);
>> +	if (!bin)
>> +		return -ENOMEM;
>> +
>> +	memcpy(bin->data, fw->data, fw->size);
>> +	release_firmware(fw);
>> +
>> +	bin->fw_size = fw->size;
>> +	bin->fw_ver = bin->data[FW_VER_OFFSET];
>> +	bin->reg_count = get_unaligned_le16(bin->data + FW_REG_CNT_OFFSET);
>> +
>> +	return hx9023s_bin_load(data, bin);
>> +}
>> +
>> +static void hx9023s_cfg_update(const struct firmware *fw, void *context)
>> +{
>> +	struct hx9023s_data *data = context;
>> +	struct device *dev = regmap_get_device(data->regmap);
>> +	int ret;
>> +
>> +	if (!fw || !fw->data) {
>> +		dev_warn(dev, "No firmware\n");
>> +		goto no_fw;
>> +	}
>> +
>> +	ret = hx9023s_send_cfg(fw, data);
>> +	if (ret)
> If this fails, we want to notify the user.  The firmware loaded
> but we were unable to use it for some reasons.
>
>> +		goto no_fw;
>> +
>> +	ret = regcache_sync(data->regmap);
>> +	if (ret)
>> +		dev_err(dev, "regcache sync failed\n");
>> +
>> +	return;
>> +
>> +no_fw:
>> +	ret = regmap_multi_reg_write(data->regmap, hx9023s_reg_init_list,
>> +								ARRAY_SIZE(hx9023s_reg_init_list));
>> +	if (ret) {
>> +		dev_err(dev, "Error loading default configuration\n");
>> +		return;
>> +	}
>> +
>> +	ret = regcache_sync(data->regmap);
>> +	if (ret)
>> +		dev_err(dev, "regcache sync failed\n");
>> +}
>> +
>>   static int hx9023s_probe(struct i2c_client *client)
>>   {
>>   	struct device *dev = &client->dev;
>> @@ -1036,18 +1122,14 @@ static int hx9023s_probe(struct i2c_client *client)
>>   	indio_dev->modes = INDIO_DIRECT_MODE;
>>   	i2c_set_clientdata(client, indio_dev);
>>   
>> -	ret = regmap_multi_reg_write(data->regmap, hx9023s_reg_init_list,
>> -				     ARRAY_SIZE(hx9023s_reg_init_list));
>> -	if (ret)
>> -		return dev_err_probe(dev, ret, "device init failed\n");
>> -
>>   	ret = hx9023s_ch_cfg(data);
>>   	if (ret)
>>   		return dev_err_probe(dev, ret, "channel config failed\n");
>>   
>> -	ret = regcache_sync(data->regmap);
>> +	ret = request_firmware_nowait(THIS_MODULE, true, "hx9023s.bin",
>> +						dev, GFP_KERNEL, data, hx9023s_cfg_update);
> Indent dev under T of the THIS_MODULE
>
>
>>   	if (ret)
>> -		return dev_err_probe(dev, ret, "regcache sync failed\n");
>> +		return dev_err_probe(dev, ret, "reg config failed\n");
>>   
>>   	if (client->irq) {
>>   		ret = devm_request_threaded_irq(dev, client->irq,
>>
>> ---
>> base-commit: 8d4d26450d71289a35ff9e847675fd9c718798b8
>> change-id: 20241209-hx9023s-firmware-20241209-47411e8cda0b
>>
>> Best regards,
diff mbox series

Patch

diff --git a/drivers/iio/proximity/hx9023s.c b/drivers/iio/proximity/hx9023s.c
index 4021feb7a7ac..6cb1b688bfa9 100644
--- a/drivers/iio/proximity/hx9023s.c
+++ b/drivers/iio/proximity/hx9023s.c
@@ -14,6 +14,7 @@ 
 #include <linux/cleanup.h>
 #include <linux/device.h>
 #include <linux/errno.h>
+#include <linux/firmware.h>
 #include <linux/i2c.h>
 #include <linux/interrupt.h>
 #include <linux/irqreturn.h>
@@ -100,6 +101,17 @@ 
 #define HX9023S_INTERRUPT_MASK GENMASK(9, 0)
 #define HX9023S_PROX_DEBOUNCE_MASK GENMASK(3, 0)
 
+#define FW_VER_OFFSET 2
+#define FW_REG_CNT_OFFSET 3
+#define FW_DATA_OFFSET 16
+
+struct hx9023s_bin {
+	u16 reg_count;
+	u16 fw_size;
+	u8 fw_ver;
+	u8 data[] __counted_by(fw_size);
+};
+
 struct hx9023s_ch_data {
 	s16 raw; /* Raw Data*/
 	s16 lp; /* Low Pass Filter Data*/
@@ -998,6 +1010,80 @@  static int hx9023s_id_check(struct iio_dev *indio_dev)
 	return 0;
 }
 
+static int hx9023s_bin_load(struct hx9023s_data *data,
+								struct hx9023s_bin *bin)
+{
+	u8 *cfg_start = bin->data + FW_DATA_OFFSET;
+	u8 addr, val;
+	u16 i;
+	int ret;
+
+	for (i = 0; i < bin->reg_count; i++) {
+		addr = cfg_start[i * 2];
+		val = cfg_start[i * 2 + 1];
+		ret = regmap_write(data->regmap, addr, val);
+		if (ret < 0)
+			return ret;
+	}
+
+	return ret;
+}
+
+static int hx9023s_send_cfg(const struct firmware *fw,
+								struct hx9023s_data *data)
+{
+	if (!fw)
+		return -EINVAL;
+
+	struct hx9023s_bin *bin __free(kfree) =
+		kzalloc(fw->size + sizeof(*bin), GFP_KERNEL);
+	if (!bin)
+		return -ENOMEM;
+
+	memcpy(bin->data, fw->data, fw->size);
+	release_firmware(fw);
+
+	bin->fw_size = fw->size;
+	bin->fw_ver = bin->data[FW_VER_OFFSET];
+	bin->reg_count = get_unaligned_le16(bin->data + FW_REG_CNT_OFFSET);
+
+	return hx9023s_bin_load(data, bin);
+}
+
+static void hx9023s_cfg_update(const struct firmware *fw, void *context)
+{
+	struct hx9023s_data *data = context;
+	struct device *dev = regmap_get_device(data->regmap);
+	int ret;
+
+	if (!fw || !fw->data) {
+		dev_warn(dev, "No firmware\n");
+		goto no_fw;
+	}
+
+	ret = hx9023s_send_cfg(fw, data);
+	if (ret)
+		goto no_fw;
+
+	ret = regcache_sync(data->regmap);
+	if (ret)
+		dev_err(dev, "regcache sync failed\n");
+
+	return;
+
+no_fw:
+	ret = regmap_multi_reg_write(data->regmap, hx9023s_reg_init_list,
+								ARRAY_SIZE(hx9023s_reg_init_list));
+	if (ret) {
+		dev_err(dev, "Error loading default configuration\n");
+		return;
+	}
+
+	ret = regcache_sync(data->regmap);
+	if (ret)
+		dev_err(dev, "regcache sync failed\n");
+}
+
 static int hx9023s_probe(struct i2c_client *client)
 {
 	struct device *dev = &client->dev;
@@ -1036,18 +1122,14 @@  static int hx9023s_probe(struct i2c_client *client)
 	indio_dev->modes = INDIO_DIRECT_MODE;
 	i2c_set_clientdata(client, indio_dev);
 
-	ret = regmap_multi_reg_write(data->regmap, hx9023s_reg_init_list,
-				     ARRAY_SIZE(hx9023s_reg_init_list));
-	if (ret)
-		return dev_err_probe(dev, ret, "device init failed\n");
-
 	ret = hx9023s_ch_cfg(data);
 	if (ret)
 		return dev_err_probe(dev, ret, "channel config failed\n");
 
-	ret = regcache_sync(data->regmap);
+	ret = request_firmware_nowait(THIS_MODULE, true, "hx9023s.bin",
+						dev, GFP_KERNEL, data, hx9023s_cfg_update);
 	if (ret)
-		return dev_err_probe(dev, ret, "regcache sync failed\n");
+		return dev_err_probe(dev, ret, "reg config failed\n");
 
 	if (client->irq) {
 		ret = devm_request_threaded_irq(dev, client->irq,