Re: [PATCH v6 2/3] x86/platform: TS-5500 basic platform support

[Vivien Didelot <vivien.didelot@savoirfairelinux.com>]

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