Re: [PATCH 3.13.1 2/9] rsi: OS dependent functions

[Jahnavi Meher <jahnavi.meher@redpinesignals.com>]

Will change to build both SDIO and USB modules
simultaneously, as suggested by Dan.

Regards, 
Jahnavi
      

> On Thu, 2014-01-30 at 21:24 +0530, Jahnavi wrote:
>
>> +rsi_common-y := rsi_core.o rsi_osd_ops.o
>> +ifeq ($(CONFIG_RSI_USB), y)
>> +	rsi_common-y += rsi_usb.o
>> +	ccflags-y =-DUSE_USB_INTF
>> +else
>> +	rsi_common-y += rsi_sdio.o
>> +	ccflags-y =-DUSE_SDIO_INTF
>> +endif
> That can't be right - in the Kconfig you declared that it's valid to
> select both SDIO and USB at the same time. You can also use the syntax
> you used below:
>
>> +obj-$(CONFIG_RSI_91x) := rsi_common.o
> for ccflags to clean this up. But you should get rid of the
> -DUSE_XYZ_INTF anyway and use CONFIG_RSI_USB in the code.
>
>> +static int rsi_proc_version_read(struct seq_file *seq, void *data)
> /proc? really? definitely staging material from here on ...
>
>> +/**
>> + * This function is used to put the current execution in a queue
>> + * and reschedules itself for execution on "timeout" or when a
>> + * wakeup is generated.
>> + *
>> + * @param  event Pointer to the event structure.
>> + * @param  timeout Timeout value in msecs.
>> + * @return status: 0 on success, -1 on failure.
>> + */
>> +int rsi_wait_event(struct rsi_event *event, unsigned int timeout)
>> +{
>> +	int status = 0;
>> +
>> +	if (!timeout)
>> +		status = wait_event_interruptible(event->event_queue,
>> +				(atomic_read(&event->event_condition) == 0));
>> +	else
>> +		status = wait_event_interruptible_timeout(event->event_queue,
>> +				(atomic_read(&event->event_condition) == 0),
>> +				timeout);
>> +	return status;
>> +}
> That's probably better inlined.
>
> Along with the rest of the event and thread functionality - doesn't
> really help readability to obscure things behind layers of "OS
> abstractions"...
>
>> +void rsi_print(int zone, unsigned char *vdata, int len)
>> +{
>> +	unsigned short ii;
>> +
>> +	if (!(zone & rsi_zone_enabled))
>> +		return;
>> +
>> +	if (!vdata)
>> +		return;
>> +
>> +	for (ii = 0; ii < len; ii++) {
>> +		if (!(ii % 16))
>> +			pr_info("\n");
>> +		pr_info("%02x ", (vdata[ii]));
>> +	}
>> +	pr_info("\n");
>> +}
> Yeah, umm, see above.
>
>> +	unsigned int bbp_lmac_clk_reg_val:16;
> 16 bit bitfields? Is there anything wrong with u16 (aka unsigned short)
> on your compiler version?
>
> johannes
>
>
>