hwmon: mc13783-adc: Refactor source to indicate various mc13xxx chips
Subsystems (3)
PATCH 1/2 hwmon: mc13783-adc: Refactor source to indicate various mc13xxx chips
This patch renames structures, variables and functions to indicate support for various MC13xxx chips.
--- drivers/hwmon/mc13783-adc.c | 135 +++++++++++++++++++++----------------------- 1 file changed, 63 insertions(+), 72 deletions(-)
drivers/hwmon/mc13783-adc.c +63 -72
@@ -28,30 +28,28 @@#include <linux/init.h> #include <linux/err.h> -#define DRIVER_NAME "mc13783-adc" -/* platform device id driver data */ #define MC13783_ADC_16CHANS 1 -#define MC13783_ADC_BPDIV2 2 +#define MC13892_ADC_BPDIV2 2 -struct mc13783_adc_priv { +struct mc13xxx_adc_priv {struct mc13xxx *mc13xxx;struct device *hwmon_dev;char name[10 ]; }; -static ssize_t mc13783_adc_show_name (struct device *dev,struct device_attribute +static ssize_t mc13xxx_adc_show_name (struct device *dev,struct device_attribute *devattr,char *buf) { -struct mc13783_adc_priv *priv =dev_get_drvdata (dev); +struct mc13xxx_adc_priv *priv =dev_get_drvdata (dev);return sprintf (buf,"%s\n" , priv->name); } -static int mc13783_adc_read (struct device *dev, +static int mc13xxx_adc_read (struct device *dev,struct device_attribute *devattr,unsigned int *val) { -struct mc13783_adc_priv *priv =dev_get_drvdata (dev); +struct mc13xxx_adc_priv *priv =dev_get_drvdata (dev);struct sensor_device_attribute *attr =to_sensor_dev_attr (devattr);unsigned int channel = attr->index;unsigned int sample[4 ];
@@ -70,18 +68,18 @@ static int mc13783_adc_read(struct device *dev,return 0 ; } -static ssize_t mc13783_adc_read_bp (struct device *dev, +static ssize_t mc13xxx_adc_read_bp (struct device *dev,struct device_attribute *devattr,char *buf) {unsigned val;struct platform_device *pdev =to_platform_device (dev);kernel_ulong_t driver_data =platform_get_device_id (pdev)->driver_data; -int ret =mc13783_adc_read (dev, devattr, &val); +int ret =mc13xxx_adc_read (dev, devattr, &val);if (ret)return ret; -if (driver_data &MC13783_ADC_BPDIV2 ) +if (driver_data &MC13892_ADC_BPDIV2 ) val =DIV_ROUND_CLOSEST (val *9 ,2 );else /*
@@ -93,11 +91,11 @@ static ssize_t mc13783_adc_read_bp(struct device *dev,return sprintf (buf,"%u\n" , val); } -static ssize_t mc13783_adc_read_gp (struct device *dev, +static ssize_t mc13xxx_adc_read_gp (struct device *dev,struct device_attribute *devattr,char *buf) {unsigned val; -int ret =mc13783_adc_read (dev, devattr, &val); +int ret =mc13xxx_adc_read (dev, devattr, &val);if (ret)return ret;
@@ -111,21 +109,21 @@ static ssize_t mc13783_adc_read_gp(struct device *dev,return sprintf (buf,"%u\n" , val); } -static DEVICE_ATTR(name, S_IRUGO, mc13783_adc_show_name, NULL); -static SENSOR_DEVICE_ATTR(in2_input, S_IRUGO, mc13783_adc_read_bp, NULL, 2); -static SENSOR_DEVICE_ATTR(in5_input, S_IRUGO, mc13783_adc_read_gp, NULL, 5); -static SENSOR_DEVICE_ATTR(in6_input, S_IRUGO, mc13783_adc_read_gp, NULL, 6); -static SENSOR_DEVICE_ATTR(in7_input, S_IRUGO, mc13783_adc_read_gp, NULL, 7); -static SENSOR_DEVICE_ATTR(in8_input, S_IRUGO, mc13783_adc_read_gp, NULL, 8); -static SENSOR_DEVICE_ATTR(in9_input, S_IRUGO, mc13783_adc_read_gp, NULL, 9); -static SENSOR_DEVICE_ATTR(in10_input, S_IRUGO, mc13783_adc_read_gp, NULL, 10); -static SENSOR_DEVICE_ATTR(in11_input, S_IRUGO, mc13783_adc_read_gp, NULL, 11); -static SENSOR_DEVICE_ATTR(in12_input, S_IRUGO, mc13783_adc_read_gp, NULL, 12); -static SENSOR_DEVICE_ATTR(in13_input, S_IRUGO, mc13783_adc_read_gp, NULL, 13); -static SENSOR_DEVICE_ATTR(in14_input, S_IRUGO, mc13783_adc_read_gp, NULL, 14); -static SENSOR_DEVICE_ATTR(in15_input, S_IRUGO, mc13783_adc_read_gp, NULL, 15); - -static struct attribute *mc13783_attr_base[] = { +static DEVICE_ATTR(name, S_IRUGO, mc13xxx_adc_show_name, NULL); +static SENSOR_DEVICE_ATTR(in2_input, S_IRUGO, mc13xxx_adc_read_bp, NULL, 2); +static SENSOR_DEVICE_ATTR(in5_input, S_IRUGO, mc13xxx_adc_read_gp, NULL, 5); +static SENSOR_DEVICE_ATTR(in6_input, S_IRUGO, mc13xxx_adc_read_gp, NULL, 6); +static SENSOR_DEVICE_ATTR(in7_input, S_IRUGO, mc13xxx_adc_read_gp, NULL, 7); +static SENSOR_DEVICE_ATTR(in8_input, S_IRUGO, mc13xxx_adc_read_gp, NULL, 8); +static SENSOR_DEVICE_ATTR(in9_input, S_IRUGO, mc13xxx_adc_read_gp, NULL, 9); +static SENSOR_DEVICE_ATTR(in10_input, S_IRUGO, mc13xxx_adc_read_gp, NULL, 10); +static SENSOR_DEVICE_ATTR(in11_input, S_IRUGO, mc13xxx_adc_read_gp, NULL, 11); +static SENSOR_DEVICE_ATTR(in12_input, S_IRUGO, mc13xxx_adc_read_gp, NULL, 12); +static SENSOR_DEVICE_ATTR(in13_input, S_IRUGO, mc13xxx_adc_read_gp, NULL, 13); +static SENSOR_DEVICE_ATTR(in14_input, S_IRUGO, mc13xxx_adc_read_gp, NULL, 14); +static SENSOR_DEVICE_ATTR(in15_input, S_IRUGO, mc13xxx_adc_read_gp, NULL, 15); + +static struct attribute *mc13xxx_attr_base[] = { &dev_attr_name.attr, &sensor_dev_attr_in2_input.dev_attr.attr, &sensor_dev_attr_in5_input.dev_attr.attr,
@@ -134,12 +132,12 @@ static struct attribute *mc13783_attr_base[] = {NULL }; -static const struct attribute_group mc13783_group_base = { - .attrs = mc13783_attr_base, +static const struct attribute_group mc13xxx_group_base = { + .attrs = mc13xxx_attr_base, };/* these are only used if MC13783_ADC_16CHANS is provided in driver data */ -static struct attribute *mc13783_attr_16chans[] = { +static struct attribute *mc13xxx_attr_16chans[] = { &sensor_dev_attr_in8_input.dev_attr.attr, &sensor_dev_attr_in9_input.dev_attr.attr, &sensor_dev_attr_in10_input.dev_attr.attr,
@@ -147,12 +145,12 @@ static struct attribute *mc13783_attr_16chans[] = {NULL }; -static const struct attribute_group mc13783_group_16chans = { - .attrs = mc13783_attr_16chans, +static const struct attribute_group mc13xxx_group_16chans = { + .attrs = mc13xxx_attr_16chans, };/* last four channels may be occupied by the touchscreen */ -static struct attribute *mc13783_attr_ts[] = { +static struct attribute *mc13xxx_attr_ts[] = { &sensor_dev_attr_in12_input.dev_attr.attr, &sensor_dev_attr_in13_input.dev_attr.attr, &sensor_dev_attr_in14_input.dev_attr.attr,
@@ -160,21 +158,21 @@ static struct attribute *mc13783_attr_ts[] = { NULL }; -static const struct attribute_group mc13783_group_ts = { - .attrs = mc13783_attr_ts, +static const struct attribute_group mc13xxx_group_ts = { + .attrs = mc13xxx_attr_ts, }; -static int mc13783_adc_use_touchscreen (struct platform_device *pdev) +static int mc13xxx_adc_use_touchscreen (struct platform_device *pdev) { -struct mc13783_adc_priv *priv =platform_get_drvdata (pdev); +struct mc13xxx_adc_priv *priv =platform_get_drvdata (pdev);unsigned flags =mc13xxx_get_flags (priv->mc13xxx);return flags &MC13XXX_USE_TOUCHSCREEN ; } -static int __init mc13783_adc_probe (struct platform_device *pdev) +static int __init mc13xxx_adc_probe (struct platform_device *pdev) { -struct mc13783_adc_priv *priv; +struct mc13xxx_adc_priv *priv;int ret;const struct platform_device_id *id =platform_get_device_id (pdev);char *dash;
@@ -192,19 +190,19 @@ static int __init mc13783_adc_probe(struct platform_device *pdev)platform_set_drvdata (pdev, priv);/* Register sysfs hooks */ - ret =sysfs_create_group (&pdev->dev.kobj, &mc13783_group_base); + ret =sysfs_create_group (&pdev->dev.kobj, &mc13xxx_group_base);if (ret)return ret;if (id->driver_data &MC13783_ADC_16CHANS ) { ret =sysfs_create_group (&pdev->dev.kobj, - &mc13783_group_16chans); + &mc13xxx_group_16chans);if (ret) goto out_err_create_16chans; } -if (!mc13783_adc_use_touchscreen (pdev)) { - ret =sysfs_create_group (&pdev->dev.kobj, &mc13783_group_ts); +if (!mc13xxx_adc_use_touchscreen (pdev)) { + ret =sysfs_create_group (&pdev->dev.kobj, &mc13xxx_group_ts);if (ret) goto out_err_create_ts; }
@@ -221,60 +219,53 @@ static int __init mc13783_adc_probe(struct platform_device *pdev) out_err_register: -if (!mc13783_adc_use_touchscreen (pdev)) -sysfs_remove_group (&pdev->dev.kobj, &mc13783_group_ts); +if (!mc13xxx_adc_use_touchscreen (pdev)) +sysfs_remove_group (&pdev->dev.kobj, &mc13xxx_group_ts); out_err_create_ts:if (id->driver_data &MC13783_ADC_16CHANS ) -sysfs_remove_group (&pdev->dev.kobj, &mc13783_group_16chans); +sysfs_remove_group (&pdev->dev.kobj, &mc13xxx_group_16chans); out_err_create_16chans: -sysfs_remove_group (&pdev->dev.kobj, &mc13783_group_base); +sysfs_remove_group (&pdev->dev.kobj, &mc13xxx_group_base);return ret; } -static int mc13783_adc_remove (struct platform_device *pdev) +static int mc13xxx_adc_remove (struct platform_device *pdev) { -struct mc13783_adc_priv *priv =platform_get_drvdata (pdev); +struct mc13xxx_adc_priv *priv =platform_get_drvdata (pdev);kernel_ulong_t driver_data =platform_get_device_id (pdev)->driver_data;hwmon_device_unregister (priv->hwmon_dev); -if (!mc13783_adc_use_touchscreen (pdev)) -sysfs_remove_group (&pdev->dev.kobj, &mc13783_group_ts); +if (!mc13xxx_adc_use_touchscreen (pdev)) +sysfs_remove_group (&pdev->dev.kobj, &mc13xxx_group_ts);if (driver_data &MC13783_ADC_16CHANS ) -sysfs_remove_group (&pdev->dev.kobj, &mc13783_group_16chans); +sysfs_remove_group (&pdev->dev.kobj, &mc13xxx_group_16chans); -sysfs_remove_group (&pdev->dev.kobj, &mc13783_group_base); +sysfs_remove_group (&pdev->dev.kobj, &mc13xxx_group_base);return 0 ; } -static const struct platform_device_id mc13783_adc_idtable[] = { - { - .name ="mc13783-adc" , - .driver_data =MC13783_ADC_16CHANS , - }, { - .name ="mc13892-adc" , - .driver_data =MC13783_ADC_BPDIV2 , - }, { -/* sentinel */ - } +static const struct platform_device_id mc13xxx_adc_idtable[] = { + {"mc13783-adc" ,MC13783_ADC_16CHANS , }, + {"mc13892-adc" ,MC13892_ADC_BPDIV2 , }, + { } }; -MODULE_DEVICE_TABLE (platform, mc13783_adc_idtable); +MODULE_DEVICE_TABLE (platform, mc13xxx_adc_idtable); -static struct platform_driver mc13783_adc_driver = { - .remove = mc13783_adc_remove, +static struct platform_driver mc13xxx_adc_driver = { .driver = { .owner =THIS_MODULE , - .name =DRIVER_NAME , + .name ="mc13xxx-adc" , }, - .id_table = mc13783_adc_idtable, + .remove = mc13xxx_adc_remove, + .id_table = mc13xxx_adc_idtable, }; +module_platform_driver_probe (mc13xxx_adc_driver, mc13xxx_adc_probe); -module_platform_driver_probe (mc13783_adc_driver, mc13783_adc_probe); - -MODULE_DESCRIPTION ("MC13783 ADC driver" ); +MODULE_DESCRIPTION ("MC13XXX ADC driver" );MODULE_AUTHOR ("Luotao Fu <[email protected]>" );MODULE_LICENSE ("GPL" );
Signature · 1 line
1.8.1.5
PATCH 2/2 hwmon: mc13783-adc: Rename unit to indicate various MC13xxx chips support
---
arch/arm/configs/imx_v4_v5_defconfig | 2 +-
drivers/hwmon/Kconfig | 10 +++++-----
drivers/hwmon/Makefile | 2 +-
drivers/hwmon/{mc13783-adc.c => mc13xxx-adc.c} | 0
4 files changed, 7 insertions(+), 7 deletions(-)
rename drivers/hwmon/{mc13783-adc.c => mc13xxx-adc.c} (100%)arch/arm/configs/imx_v4_v5_defconfig +1 -1
@@ -114,7 +114,7 @@ CONFIG_W1=y CONFIG_W1_MASTER_MXC=y CONFIG_W1_SLAVE_THERM=y CONFIG_HWMON=m -CONFIG_SENSORS_MC13783_ADC=m +CONFIG_SENSORS_MC13XXX_ADC=m CONFIG_WATCHDOG=y CONFIG_IMX2_WDT=y CONFIG_MFD_MC13XXX_SPI=y
drivers/hwmon/Kconfig +5 -5
@@ -1526,11 +1526,11 @@ config SENSORS_APPLESMC Say Y here if you have an applicable laptop and want to experience the awesome power of applesmc. -config SENSORS_MC13783_ADC - tristate "Freescale MC13783/MC13892 ADC" - depends on MFD_MC13XXX - help - Support for the A/D converter on MC13783 and MC13892 PMIC. +config SENSORS_MC13XXX_ADC + tristate "Freescale MC13783/MC13892 ADC" + depends on MFD_MC13XXX + help + Support for the A/D converter on MC13783 and MC13892 PMIC. if ACPI
drivers/hwmon/Makefile +1 -1
@@ -106,7 +106,7 @@ obj-$(CONFIG_SENSORS_MAX6639) += max6639.o obj-$(CONFIG_SENSORS_MAX6642) += max6642.o obj-$(CONFIG_SENSORS_MAX6650) += max6650.o obj-$(CONFIG_SENSORS_MAX6697) += max6697.o -obj-$(CONFIG_SENSORS_MC13783_ADC)+= mc13783-adc.o +obj-$(CONFIG_SENSORS_MC13XXX_ADC)+= mc13xxx-adc.o obj-$(CONFIG_SENSORS_MCP3021) += mcp3021.o obj-$(CONFIG_SENSORS_NCT6775) += nct6775.o obj-$(CONFIG_SENSORS_NTC_THERMISTOR) += ntc_thermistor.o
drivers/hwmon/mc13xxx-adc.c renamed from drivers/hwmon/mc13783-adc.c +0 -0
Signature · 1 line
1.8.1.5
PATCH 1/2 hwmon: mc13783-adc: Refactor source to indicate various mc13xxx chips
In reply to Alexander ShiyanQuoted 7 lines · Alexander Shiyan
This patch renames structures, variables and functions to indicate support for various MC13xxx chips.
Signed-off-by Alexander Shiyan <[email protected]>--- drivers/hwmon/mc13783-adc.c | 135 +++++++++++++++++++++----------------------- 1 file changed, 63 insertions(+), 72 deletions(-)
Nack. This change serves absolutely no purpose, all it's doing it make everyone's life harder. Just don't do that, thanks.
Signature · 1 line
Jean Delvare
Quoted 11 lines · Alexander Shiyan
Quoted 7 lines
This patch renames structures, variables and functions to indicate support for various MC13xxx chips.
Signed-off-by Alexander Shiyan <[email protected]>--- drivers/hwmon/mc13783-adc.c | 135 +++++++++++++++++++++----------------------- 1 file changed, 63 insertions(+), 72 deletions(-)Nack. This change serves absolutely no purpose, all it's doing it make everyone's life harder. Just don't do that, thanks.
I do not understand stubbornness here.
The driver works for different chips, it should be reflected in the code.
---
PATCH 1/2 hwmon: mc13783-adc: Refactor source to indicate various mc13xxx chips
Quoted 9 lines · Alexander Shiyan
Quoted 9 lines · Alexander Shiyan
Quoted 7 lines
This patch renames structures, variables and functions to indicate support for various MC13xxx chips.
Signed-off-by Alexander Shiyan <[email protected]>--- drivers/hwmon/mc13783-adc.c | 135 +++++++++++++++++++++----------------------- 1 file changed, 63 insertions(+), 72 deletions(-)
Quoted 5 lines
Quoted 2 lines
Nack. This change serves absolutely no purpose, all it's doing it make everyone's life harder. Just don't do that, thanks.
I do not understand stubbornness here.
The driver works for different chips, it should be reflected in the code.
Let's look at it from another angle. What problem are you trying to solve? I see absolutely no problem with the current function and file names. Almost all Linux kernel drivers support more chips than their name says.
On the other hand, changing all function names makes backporting fixes harder, and changing Kconfig symbol names makes upgrading to the new kernel version harder for everyone.
So the cost of your proposal far outweighs the benefits.
Signature · 1 line
Jean Delvare
Quoted 20 lines
Quoted 9 lines
Quoted 9 lines · Alexander Shiyan
Quoted 7 lines
This patch renames structures, variables and functions to indicate support for various MC13xxx chips.
Signed-off-by Alexander Shiyan <[email protected]>--- drivers/hwmon/mc13783-adc.c | 135 +++++++++++++++++++++----------------------- 1 file changed, 63 insertions(+), 72 deletions(-)Quoted 5 lines
Quoted 2 lines
Nack. This change serves absolutely no purpose, all it's doing it make everyone's life harder. Just don't do that, thanks.
I do not understand stubbornness here.
The driver works for different chips, it should be reflected in the code.Let's look at it from another angle. What problem are you trying to solve? I see absolutely no problem with the current function and file names. Almost all Linux kernel drivers support more chips than their name says.
Quoted 5 lines
On the other hand, changing all function names makes backporting fixes harder, and changing
Kconfigsymbol names makes upgrading to the new kernel version harder for everyone.So the cost of your proposal far outweighs the benefits.
Well, let's leave the kconfig symbol as is. But about the rest, that mc13783_* symbols in the log (debug) will say a developer who does not know about mc13892 compatibility with mc13783? I still believe that this should be reflected. Thanks.
---
PATCH 1/2 hwmon: mc13783-adc: Refactor source to indicate various mc13xxx chips
Quoted 3 lines · Alexander Shiyan
Well, let's leave the kconfig symbol as is. But about the rest, that mc13783_* symbols in the log (debug) will say a developer who does not know about mc13892 compatibility with mc13783? I still believe that this should be reflected.
If the log messages use dev_printk, they will use the correct device name already, if not, that can be fixed.
I agree with Jean: we don't just rename things unless there is a significant benefit, which I don't see here. There is also the risk that we need to support a different chip named mc13xxx in the future that is different enough to require a new driver and then the naming is even more screwed up.
Arnd
Quoted 5 lines · Alexander Shiyan
Quoted 3 lines
Well, let's leave the kconfig symbol as is. But about the rest, that mc13783_* symbols in the log (debug) will say a developer who does not know about mc13892 compatibility with mc13783? I still believe that this should be reflected.
Quoted 7 lines
If the log messages use
dev_printk, they will use the correct device name already, if not, that can be fixed.I agree with Jean: we don't just rename things unless there is a significant benefit, which I don't see here. There is also the risk that we need to support a different chip named mc13xxx in the future that is different enough to require a new driver and then the naming is even more screwed up.
It is a pity that the revision and updating of the code of this driver died. Well, lets drop thesepatches.
Thanks.
---
PATCH 1/2 hwmon: mc13783-adc: Refactor source to indicate various mc13xxx chips
In reply to Alexander ShiyanQuoted 9 lines · Alexander Shiyan
Quoted 9 lines
Let's look at it from another angle. What problem are you trying to solve? I see absolutely no problem with the current function and file names. Almost all Linux kernel drivers support more chips than their name says.
On the other hand, changing all function names makes backporting fixes harder, and changing
Kconfigsymbol names makes upgrading to the new kernel version harder for everyone.
Quoted 5 lines
Quoted 1 line
So the cost of your proposal far outweighs the benefits.
Well, let's leave the kconfig symbol as is. But about the rest, that mc13783_* symbols in the log (debug) will say a developer who does not know about mc13892 compatibility with mc13783? I still believe that this should be reflected.
You're splitting hairs here. mc13783_* symbols in the log mean this comes from a driver named mc13783, period. A developer drawing conclusions beyond that needs to be educated. Which device the driver is driving can be retried from the log itself, or from sysfs. Or the hardware board datasheet. "mc13xxx" would tell no more, BTW.
Also note that the name mc13xxx is wrong as well, as the mc13xxx-i2c and mc13xxx-spi drivers handle the MC34708 chip too. So you'd have to name all these drivers and functions mcxxxxx* to cover all the supported chip. Which makes no sense at all, because so many x's are confusing, and because the mask then matches many devices the drivers do _not_ handle.
So please forget about this and move on to something else. There are many many code cleanups and improvements, and bug fixes, all way more important than this. So I'm sure you can find better ways to spend your time, and mine.
Thanks,
Signature · 1 line
Jean Delvare
?????, 5 ???? 2013, 13:45 +02:00 ?? Jean Delvare <[email protected]>:
Quoted 29 lines · Alexander Shiyan
Quoted 9 lines
Quoted 9 lines
Let's look at it from another angle. What problem are you trying to solve? I see absolutely no problem with the current function and file names. Almost all Linux kernel drivers support more chips than their name says.
On the other hand, changing all function names makes backporting fixes harder, and changing
Kconfigsymbol names makes upgrading to the new kernel version harder for everyone.Quoted 5 lines
Quoted 1 line
So the cost of your proposal far outweighs the benefits.
Well, let's leave the kconfig symbol as is. But about the rest, that mc13783_* symbols in the log (debug) will say a developer who does not know about mc13892 compatibility with mc13783? I still believe that this should be reflected.
You're splitting hairs here. mc13783_* symbols in the log mean this comes from a driver named mc13783, period. A developer drawing conclusions beyond that needs to be educated. Which device the driver is driving can be retried from the log itself, or from sysfs. Or the hardware board datasheet. "mc13xxx" would tell no more, BTW.
Also note that the name mc13xxx is wrong as well, as the mc13xxx-i2c and mc13xxx-spi drivers handle the MC34708 chip too. So you'd have to name all these drivers and functions mcxxxxx* to cover all the supported chip. Which makes no sense at all, because so many x's are confusing, and because the mask then matches many devices the drivers do _not_ handle.
Quoted 4 lines
So please forget about this and move on to something else. There are many many code cleanups and improvements, and bug fixes, all way more important than this. So I'm sure you can find better ways to spend your time, and mine.
Agree, we have a lot of places in the kernel to be cleared.
For the same driver MC13XXX, at least we need to remove a useless symbol MFD_M13783 which is selected automatically if we select MFD_MC13XXX, mc13xxx_lock/unlock functions in the driver has long been not needed because we use regmap etc.
But to start in my opinion you need with just such trifles as rename etc... But even at this stage of obstacles have come up against ...
Well, by and large it does not change code but rather a grooming code, so I can easily give it up. But, in my opinion, the purity and clarity of the code has never harmed.
OK, lets drop these patches.
Thanks.
---