Re: [PATCH RFC] support sata odd zero power

[Tejun Heo <htejun@gmail.com>]

Hello,

On 06/28/2010 10:43 AM, su henry wrote:
>> Can you please align fields?  Also, single struct for the whole
>> system?  I haven't read the spec but I don't think anybody would be
>> defining things like this system-wide.  Right?
>>
> 
> My original though is to put this structure to cdrom_device_info, but
> this structure should be firstly used by acpi_walk_namespace in
> sr_init, and cdrom_device_info is allocated in sr_probe, that's why I
> define a separate structure for the zero power odd.

What prevents you from walking the acpi device tree from sr_probe()?
Or even if you need to walk it from sr_init(), you still need to store
the result and associate it with a specific cdrom device.  It is
something which is specific to single device.  You can't use single
global data structure for all devices like this.

>> bool?
> 
> The return value of sr_test_unit_ready is int.

I would still go for bool but it's a peripheral issue.

>> This thing definitely belongs to struct scsi_cd.  What are the
>> synchronization rules here?  Also, what happens if async notification
>> is in use?  How does the timer get activated then?
> 
> This is a problem, any suggestions? Especially when the system goes to
> S3/S4 state.

Associate with specific device and using timer should work.

>>> +     default:
>>> +             /*tray/drawer/pop-up type*/
>>> +             /* 3a/01(asc/ascq) means MEDIUM NOT PRESENT - TRAY CLOSED */
>>> +             if (isvalid && (sshdr->asc  == 0x3a &&
>>> +                             sshdr->ascq == 0x01)) {
>>> +
>>> +                     /* Eject the tray for a tray type drive*/
>>> +                     if (sr_zpodd_device.flags & SR_ZPODD_NEED_EJECT) {
>>> +                             sr_tray_move(cdi, 1);
>>> +                             sr_zpodd_device.flags &= ~SR_ZPODD_NEED_EJECT;
>>> +                     } else if (!(sr_zpodd_device.flags &
>>> +                                     SR_ZPODD_NO_DISK)) {
>>> +                             sr_zpodd_device.flags |= SR_ZPODD_NO_DISK;
>>> +                             sr_zpodd_device.last_jiffies = jiffies;
>>> +                     } else if (time_after(jiffies,
>>> +                                     sr_zpodd_device.last_jiffies
>>> +                                     + SR_NO_MEDIA_TIMEOUT)) {
>>> +                             acpi_bus_set_power(sr_zpodd_device.handle,
>>> +                                                     ACPI_STATE_D3);
>>> +                     }
>>> +             } else
>>> +                     sr_zpodd_device.flags &= ~SR_ZPODD_NO_DISK;
>>> +             break;
>>> +     }
>>
>> It would probably be better to separate decision making and updating
>> states.
> 
> When user pressing the button on a tray type drive,  driver should set
> a flag used to eject the tray in sr_media_change, if we use that
> states here, we should a one more state "NEED_EJECT" for drive status.

Oh, I meant the code.  ie. instead of

	switch (device type) {
	type a:
		something something
		break;
	default:
		something something slightly differently
		break;
	}

do something like the following,

	switch (device type) {
	type a:
		determine what to do
		break;
	default:
		determine what to do slightly differently;
		break;
	}
	do what has been determined;

Thanks.

-- 
tejun