Re: [PATCH 03/11] libata: add probeinit component operation to ata_drive_probe_reset()

[Jeff Garzik <jgarzik@pobox.com>]

Tejun wrote:
> Jeff Garzik wrote:
> 
>> Tejun Heo wrote:
>>
>>> This patch adds probeinit component operation to
>>> ata_drive_probe_reset().  If present, this new operation is called
>>> before performing any reset.  The operations's roll is to prepare @ap
>>> for following probe-reset operations.
>>>
>>> Signed-off-by: Tejun Heo <htejun@gmail.com>
>>
>>
>> [...]
>>
>>>  extern int ata_drive_probe_reset(struct ata_port *ap,
>>> +            ata_probeinit_fn_t probeinit,
>>>              ata_reset_fn_t softreset, ata_reset_fn_t hardreset,
>>>              ata_postreset_fn_t postreset, unsigned int *classes);
>>
>>
>>
>> Applied patches 3-4, although I dislike that ata_drive_probe_reset() 
>> is growing a ton of function pointer arguments.  Please consider a 
>> better approach when you have some free time.  Perhaps these need to 
>> be added to ata_port_operations?  Perhaps another ata_reset_operations 
>> struct? What do you think?
>>
> 
> I thought about adding the component operations to ata_port_operations, 
> but those callbacks would only be used if ->probe_reset uses 
> ata_drive_probe_reset() and layering ends up weird.  BTW, the same thing 
> is true for ->bmdma_* callbacks and a few more, I think.
> 
> So, I'm a little bit unconformatble with clamming multi levels of 
> operations into ata_port_operations or adding ata_reset_operations to 
> ata_port.  So, the wrap-with-drive-function thing was my compromise, 
> which isn't very pretty but keeps the functionality.

Fair enough.


> A problem with clamming multi-level callbacks into one structure is that 
> it's not clear which callbacks should be implemented and with core code 
> constantly changing, the requirements also changes along.  Changing API 
> is actually good thing but in this case it's difficult to know what end 
> effects changes have on low-level drivers.  (remember ->dev_select 
> breakage last year?)
> 
> So, IMHO we should not add more layered operations to top-level.  It 
> would be nice if we can come up with some simple way to separate out 
> layered callbacks.  Do you agree with this line of thought?

Indeed, one thought I had was having a cleaner upper-level qc_issue API, 
and separating the BMDMA hooks out somehow into a separate layer, 
outside ata_port_operations.

	Jeff