Hi Guenter,
On Thu, Sep 29, 2011 at 09:38:23AM -0700, Guenter Roeck wrote:
> On Tue, 2011-09-27 at 17:16 -0400, Uwe Kleine-König wrote:
> > [...]
> > diff --git a/drivers/hwmon/mc13783-adc.c b/drivers/hwmon/mc13783-adc.c
> > index ef65ab5..0d796f7 100644
> > --- a/drivers/hwmon/mc13783-adc.c
> > +++ b/drivers/hwmon/mc13783-adc.c
> > @@ -1,5 +1,5 @@
> > /*
> > - * Driver for the Freescale Semiconductor MC13783 adc.
> > + * Driver for the adc on Freescale Semiconductor MC13783 and MC13892 PMICs.
> > *
> > * Copyright 2004-2007 Freescale Semiconductor, Inc. All Rights Reserved.
> > * Copyright (C) 2009 Sascha Hauer, Pengutronix
> > @@ -18,7 +18,7 @@
> > * Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
> > */
> >
> > -#include <linux/mfd/mc13783.h>
> > +#include <linux/mfd/mc13xxx.h>
> > #include <linux/platform_device.h>
> > #include <linux/hwmon-sysfs.h>
> > #include <linux/kernel.h>
> > @@ -28,7 +28,11 @@
> > #include <linux/init.h>
> > #include <linux/err.h>
> >
> > -#define MC13783_ADC_NAME "mc13783-adc"
> > +#define DRIVER_NAME "mc13783-adc"
> > +
> > +/* platform device id driver data */
> > +#define MC13783_ADC_16CHANS 1
> > +#define MC13783_ADC_BPDIV2 2
> >
> I am all for using functional bitmaps if it serves a purpose. However,
> that is not the case here, and there is no indication that or if there
> will ever be a chip supporting more than one of the features in the bit
> map. With a bitmap like this, we get all the pain and none of the
> benefits of having a bitmap.
What exactly do you consider a pain here? I really like it that way and
want to keep it.
> So please use an enum instead to distinguish chips, similar to other
> hwmon drivers. Something like
>
> enum chips { mc13783, mc13892 };
>
In my eyes this would be a step back. I've seen quite some places where
an if() used to decide based on a name (mostly cpu_is_mxyz()) and adding
support for a new cpu has to touch all these if()s.
> > struct mc13783_adc_priv {
> > struct mc13xxx *mc13xxx;
>
> It might actually make sense to store the chip id in mc13783_adc_priv to
> simplify access to it.
>
> > @@ -38,7 +42,13 @@ struct mc13783_adc_priv {
> > static ssize_t mc13783_adc_show_name(struct device *dev, struct device_attribute
> > *devattr, char *buf)
> > {
> > - return sprintf(buf, "mc13783_adc\n");
> > + struct platform_device *pdev = to_platform_device(dev);
> > + ssize_t ret = sprintf(buf, "%s\n", pdev->name);
> > +
> > + if (ret > 7 && buf[7] == '-')
> > + buf[7] = '_';
> > +
> This is clumsy, and would get really ugly if there is ever a chip
> supported by this driver which doesn't have the '-' at the same
> position.
Right. But this can still be done when that new chip comes down the
road. And today this is IMHO good enough...
> You could store the name with '_' in platform_device_id, or have a const
> char *name in mc13783_adc_priv and point it to the correct/expected
> string. No need to do a runtime string correction.
... and doesn't spend memory to save several nearly identical strings.
> > [...]
> > +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 */
> > + }
> > +};
> > +MODULE_DEVICE_TABLE(platform, mc13783_adc_idtable);
> > +
> > static struct platform_driver mc13783_adc_driver = {
> > - .remove = __devexit_p(mc13783_adc_remove),
> > + .remove = __devexit_p(mc13783_adc_remove),
> > .driver = {
> > .owner = THIS_MODULE,
> > - .name = MC13783_ADC_NAME,
> > + .name = DRIVER_NAME,
> > },
> > + .id_table = mc13783_adc_idtable,
> > };
> >
> > static int __init mc13783_adc_init(void)
> > @@ -243,4 +299,3 @@ module_exit(mc13783_adc_exit);
> > MODULE_DESCRIPTION("MC13783 ADC driver");
> > MODULE_AUTHOR("Luotao Fu <l.fu@xxxxxxxxxxxxxx>");
> > MODULE_LICENSE("GPL");
> > -MODULE_ALIAS("platform:" MC13783_ADC_NAME);
>
> Just for clarification - is the alias no longer needed because the name
> matches the one in mc13783_adc_driver ?
It is superseeded by the MODULE_DEVICE_TABLE above.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
_______________________________________________
lm-sensors mailing list
lm-sensors@xxxxxxxxxxxxxx
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
