Re: [PATCH v11 3/9] libata: identify and init ZPODD devices

[Tejun Heo <tj@kernel.org>]

Hello, Aaron.

On Tue, Jan 08, 2013 at 05:07:46PM +0800, Aaron Lu wrote:
> >> +struct zpodd {
> >> +	bool slot:1;
> >> +	bool drawer:1;
> >> +
> >> +	struct ata_device *dev;
> >> +};
> > 
> > Field names are usually indented.  It would be nice to have a comment
> 
> checkscript.sh doesn't seem like this if I put a tab around the ':'
> 
> ERROR: spaces prohibited around that ':' (ctx:VxW)
> #222: FILE: include/uapi/linux/cdrom.h:915:
> +	__u8 reserved1:		2;
>  	              ^
> 
> Which style should I follow?

struct zpodd {
	bool			slot:1;
	bool			drawer:1;

	struct ata_device	*dev;
};

> > explaining synchronization.  Bitfields w/ their implicit RMW ops tend
> > to make people wonder about what the access rules are.
> 
> The slot and drawer bit field is assigned only once during ata probe
> time in EH context, and accessed later in PM's callback context.
> Not sure what access rule should I describe...

/* init during probe, RO afterwards */ should do but I'd prefer if you
stay away from bitfields altogether.  There are cases where bitfields
are okay but when you're working across multiple locking domains, it
usually is a bad idea because the code which accesses those fields
look completely independent while still being able to interact with
each other.  They look properly synchronized until you realized they
live on the same machine word.

> >> +/*
> >> + * Per the spec, only slot type and drawer type ODD can be supported
> >> + *
> >> + * Return 0 for slot type, 1 for drawer, -ENODEV for other types or on error.
> >> + */
> > 
> > Maybe bool odd_has_drawer() is better?
> 
> There are other types of ODD other than slot and drawer, and both slot
> and drawer type ODDs can be supported for ZPODD. So a bool can't convey
> such information :-)

Then please make it a proper ATA enum and use it in struct zpodd too.

Thanks!

-- 
tejun