Hi,
Thanks for the comments.
On Fri, 2012-04-13 at 12:37 +0200, Thomas Gleixner wrote:
> On Thu, 12 Apr 2012, Vivien Didelot wrote:
> > +/**
> > + * struct ts5500_sbc - TS-5500 SBC main structure
> > + * @lock: Read/Write mutex.
>
> What's the point of this mutex ?
>
> AFAICT, it's only ever used in the init path which is serialized by
> itself.
You're right, I'll remove it.
>
> > + * @board_id: Board name.
>
> name in an integer ?
>
> > + * @sram: Check SRAM option.
> > + * @rs485: Check RS-485 option.
> > + * @adc: Check Analog/Digital converter option.
> > + * @ereset: Check External Reset option.
> > + * @itr: Check Industrial Temperature Range option.
> > + * @jumpers: States of jumpers 1-7.
> > + */
> > +struct ts5500_sbc {
> > + struct mutex lock;
> > + int board_id;
> > + bool sram;
> > + bool rs485;
> > + bool adc;
> > + bool ereset;
> > + bool itr;
> > + u8 jumpers;
> > +};
>
>
> > +/**
> > + * ts5500_bios_signature() - find board signature in BIOS shadow RAM.
> > + */
> > +static int __init ts5500_bios_signature(void)
> > +{
> > + void __iomem *bios = ioremap(0xF0000, 0x10000);
> > + int i, ret = 0;
>
> ioremaps can fail.
>
> > + for (i = 0; i < ARRAY_SIZE(signatures); i++)
> > + if (check_signature(bios + signatures[i].offset,
> + signatures[i].string,
> > + strlen(signatures[i].string)))
> > + goto found;
> > + else
> > + pr_notice("Technologic Systems BIOS signature "
> > + "'%s' not found at offset %zd\n",
> > + signatures[i].string, signatures[i].offset);
> > + ret = -ENODEV;
> > +found:
> > + iounmap(bios);
>
> Uurg, this is convoluted. What's wrong with doing:
>
> int ret = -ENODEV;
>
> for (....) {
> if (check_signature()) {
> ret = 0;
> break;
> }
> }
>
> That way the code becomes readable and we really do not need a
> printout when the kernel is configured for multiple platforms and
> runs on a !TS board. Also you would print out that nonsense if your
> signature array has more than one entry for each non matching
> one. Pretty pointless.
Got it.
>
> > + tmp = inb(TS5500_PRODUCT_CODE_ADDR);
> > + if (tmp != TS5500_PRODUCT_CODE) {
> > + pr_err("This platform is not a TS-5500 (found ID 0x%x)\n", tmp);
> > + ret = -ENODEV;
> > + goto cleanup;
> > + }
> > + sbc->board_id = tmp;
>
> So we store a constant value in a data structure and the sole purpose
> is to display that constant value in sysfs file. Interesting feature.
I plan to add support for a few variants of this board which differ at
this register level.
>
> > +static struct attribute *ts5500_attributes[] = {
> > + &dev_attr_id.attr,
> > + &dev_attr_sram.attr,
> > + &dev_attr_rs485.attr,
> > + &dev_attr_adc.attr,
> > + &dev_attr_ereset.attr,
> > + &dev_attr_itr.attr,
> > + &dev_attr_jp1.attr,
> > + &dev_attr_jp2.attr,
> > + &dev_attr_jp3.attr,
> > + &dev_attr_jp4.attr,
> > + &dev_attr_jp5.attr,
> > + &dev_attr_jp6.attr,
>
> So you create 12 sysfs entries to export boolean features and a
> constant value. I don't care much, but this looks like massive
> overkill.
Ok, I'll go for a simple "settings" sysfs attribute.
>
> > +/* A/D Converter platform device */
> > +
> > +static int ts5500_adc_convert(u8 ctrl, u16 *raw)
> > +{
> > + u8 lsb, msb;
> > +
> > + /* Start convertion (ensure the 3 MSB are set to 0) */
> > + outb(ctrl & 0x1F, TS5500_ADC_CONV_INIT_LSB_ADDR);
> > +
> > + udelay(TS5500_ADC_CONV_DELAY);
> > + if (inb(TS5500_ADC_CONV_BUSY_ADDR) & TS5500_ADC_CONV_BUSY)
> > + return -EBUSY;
>
> Shouldn't you check the busy bit _BEFORE_ writing into the converter?
>
> Also initiating the conversion and then bailing out if it did not
> complete in some micro seconds is kinda strange. What's wrong with
> that hardware? And how does it ever recover?
The manufacturer has CPLD logic driving the actual A/D converter. The
documentation says they guarantee it must complete within x microseconds
otherwise you have to re-initiate a conversion.
I'll add a proper comment explaining this.
>
> > +static void ts5500_adc_release(struct device *dev)
> > +{
> > + /* noop */
>
> Very helpful comment.
>
> > +}
> > +
> > +static struct platform_device ts5500_adc_pdev = {
> > + .name = "max197",
> > + .id = -1,
> > + .dev = {
> > + .platform_data = &ts5500_adc_pdata,
> > + .release = ts5500_adc_release,
>
> What's the point of this empty release function ? The device is never
> released.
Ok.
Thanks,
Vivien
_______________________________________________
lm-sensors mailing list
lm-sensors@xxxxxxxxxxxxxx
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
