Re: [PATCH RFC] support sata odd zero power

su henry
2010-06-28T10:42:47Z

Hi tejun,

Thanks for the good suggestion, please see my comments inline.

On Mon, Jun 28, 2010 at 5:04 PM, Tejun Heo <htejun@gmail.com> wrote:
> 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.
>

In order to make sure we walk the name space one time only.

Because when the host starts the power supply to ODD, the driver will
start from sr_probe. I think it unnecessary to walk the acpi name
space when driver probes the device, so I walk the acpi name space
from sr_init.

>>> 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.
>

Considering the S3/S4 state, if we add a new timer for this, we should
also add the suspend/resume callbacks for the driver, and modify the
timer timeout(mod_timer) in the callback function.

>>>> +     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 for the advice.

> Thanks.
>
> --
> tejun
>
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html