On Sun, Jan 06, 2013 at 10:48:23AM +0800, Aaron Lu wrote:
> diff --git a/drivers/ata/libata-acpi.c b/drivers/ata/libata-acpi.c
> index ef01ac0..5aa7322 100644
> --- a/drivers/ata/libata-acpi.c
> +++ b/drivers/ata/libata-acpi.c
> @@ -1063,6 +1063,8 @@ void ata_acpi_bind(struct ata_device *dev)
>
> void ata_acpi_unbind(struct ata_device *dev)
> {
> + if (zpodd_dev_enabled(dev))
> + zpodd_exit(dev);
> ata_acpi_remove_pm_notifier(dev);
> ata_acpi_unregister_power_resource(dev);
> }
Wouldn't it make more sense to invoke zpodd_exit() from
ata_scsi_remove_dev() which is approximate counterpart of
dev_configure?
> +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
explaining synchronization. Bitfields w/ their implicit RMW ops tend
to make people wonder about what the access rules are.
> +static int run_atapi_cmd(struct ata_device *dev, const char *cdb,
> + unsigned short cdb_len, char *buf, unsigned int buf_len)
> +{
> + struct ata_taskfile tf = {0};
No need for 0. { } is enough and more generic.
> +
> + tf.flags |= ATA_TFLAG_ISADDR | ATA_TFLAG_DEVICE;
> + tf.command = ATA_CMD_PACKET;
> +
> + if (buf) {
> + tf.protocol = ATAPI_PROT_PIO;
> + tf.lbam = buf_len;
> + } else {
> + tf.protocol = ATAPI_PROT_NODATA;
> + }
> +
> + return ata_exec_internal(dev, &tf, cdb,
> + buf ? DMA_FROM_DEVICE : DMA_NONE, buf, buf_len, 0);
> +}
So, the function name is a bit of misnomer given that ATAPI commands
are not limited to PIO or DMA_FROM_DEVICE. Also, this function ends
up being used twice - once w/ read buffer and once w/o. Do we really
want this function? It's not like exec_internal is difficult to use.
> +/*
> + * 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?
> +static int check_loading_mechanism(struct ata_device *dev)
> +{
> + char buf[16];
> + unsigned int ret;
> + struct rm_feature_desc *desc = (void *)(buf + 8);
> +
> + char cdb[] = { GPCMD_GET_CONFIGURATION,
> + 2, /* only 1 feature descriptor requested */
> + 0, 3, /* 3, removable medium feature */
> + 0, 0, 0,/* reserved */
> + 0, sizeof(buf),
> + 0, 0, 0,
> + };
> +
> + ret = run_atapi_cmd(dev, cdb, sizeof(cdb), buf, sizeof(buf));
> + if (ret)
> + return -ENODEV;
> +
> + if (be16_to_cpu(desc->feature_code) != 3)
> + return -ENODEV;
> +
> + if (desc->mech_type == 0 && desc->load == 0 && desc->eject == 1)
> + return 0; /* slot */
> + else if (desc->mech_type == 1 && desc->load == 0 && desc->eject == 1)
> + return 1; /* drawer */
> + else
> + return -ENODEV;
> +}
> +
> +static bool odd_can_poweroff(struct ata_device *ata_dev)
> +{
> + acpi_handle handle;
> + acpi_status status;
> + struct acpi_device *acpi_dev;
> +
> + handle = ata_dev_acpi_handle(ata_dev);
> + if (!handle)
> + return false;
> +
> + status = acpi_bus_get_device(handle, &acpi_dev);
> + if (ACPI_FAILURE(status))
> + return false;
> +
> + return acpi_device_can_poweroff(acpi_dev);
> +}
> +
> +void zpodd_init(struct ata_device *dev)
> +{
> + int ret;
> + struct zpodd *zpodd;
> +
> + if (dev->zpodd)
> + return;
> +
> + if (!odd_can_poweroff(dev))
> + return;
> +
> + if ((ret = check_loading_mechanism(dev)) == -ENODEV)
> + return;
> +
> + zpodd = kzalloc(sizeof(struct zpodd), GFP_KERNEL);
> + if (!zpodd)
> + return;
> +
> + if (ret)
> + zpodd->drawer = true;
> + else
> + zpodd->slot = true;
> +
> + zpodd->dev = dev;
> + dev->zpodd = zpodd;
> +}
> +
> +void zpodd_exit(struct ata_device *dev)
> +{
> + kfree(dev->zpodd);
> + dev->zpodd = NULL;
> +}
> diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h
> index 7148a58..8cb4372 100644
> --- a/drivers/ata/libata.h
> +++ b/drivers/ata/libata.h
> @@ -230,4 +230,18 @@ static inline void ata_sff_exit(void)
> { }
> #endif /* CONFIG_ATA_SFF */
>
> +/* libata-zpodd.c */
> +#ifdef CONFIG_SATA_ZPODD
> +void zpodd_init(struct ata_device *dev);
> +void zpodd_exit(struct ata_device *dev);
> +static inline bool zpodd_dev_enabled(struct ata_device *dev)
> +{
> + return dev->zpodd ? true : false;
return dev->zpodd or return dev->zpodd != NULL?
Other than the above nits, looks okay to me.
Thanks.
--
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
