Hi
>-----Original Message-----
>From: Barnabás Pőcze <pobrn@xxxxxxxxxxxxxx>
>Sent: Sunday, February 14, 2021 5:43 AM
>To: Nitin Joshi <nitjoshi@xxxxxxxxx>
>Cc: hdegoede@xxxxxxxxxx; ibm-acpi-devel@xxxxxxxxxxxxxxxxxxxxx; platform-
>driver-x86@xxxxxxxxxxxxxxx; Nitin Joshi1 <njoshi1@xxxxxxxxxx>; Mark RH
>Pearson <markpearson@xxxxxxxxxx>
>Subject: [External] Re: [PATCH 2/2] platorm/x86: thinkpad_acpi: sysfs
>interface to interface to get wwan antenna type
>
>Hi
>
>
>2021. február 12., péntek 6:58 keltezéssel, Nitin Joshi írta:
>
>> [...]
>> +/* sysfs wwan antenna type entry */
>> +static ssize_t wwan_antenna_type_show(struct device *dev,
>> + struct device_attribute *attr,
>> + char *buf)
>> +{
>> + switch (wwan_antennatype) {
>> + case 1:
>> + return sysfs_emit(buf, "type a\n");
>> + case 2:
>> + return sysfs_emit(buf, "type b\n");
>> + default:
>> + return sysfs_emit(buf, "unknown type\n");
>
>I feel like returning -ENODATA would be more appropriate here. Or maybe
>you could choose not to create the attribute if the antenna type is unknown.
>And I'm not sure if the "type" prefix is necessary. I believe the name of the
>attribute 'wwan_antenna_type'
Ack . I will check it.
Regarding prefix, it's not so necessary but let me keep "type" prefix.
>already implies that the content will describe a type. Furthermore, I think you
>could omit the `has_antennatype` variable altogether, storing only
>`wwan_antennatype` is enough.
Ack . I will check it
>
>
>> + }
>> +}
>> +
>> static ssize_t wlan_tx_strength_reduce_store(struct device *dev,
>> struct device_attribute *attr,
>> const char *buf, size_t count)
>> @@ -10076,24 +10114,29 @@ static ssize_t
>wlan_tx_strength_reduce_store(struct device *dev,
>> }
>>
>> sysfs_notify(&tpacpi_pdev->dev.kobj, NULL,
>> "wlan_tx_strength_reduce");
>> +
>
>If you want the empty line here, I think you should place it in the previous
>patch.
Ack . I will remove it.
>
>
>> return count;
>> }
>> static DEVICE_ATTR_RW(wlan_tx_strength_reduce);
>> +static DEVICE_ATTR_RO(wwan_antenna_type);
>>
>> static int tpacpi_dprc_init(struct ibm_init_struct *iibm) {
>> - int wlantx_err, err;
>> + int wlantx_err, wwanantenna_err, err;
>>
>> wlantx_err = get_wlan_state(&has_wlantxreduce, &wlan_txreduce);
>> + wwanantenna_err = get_wwan_antenna(&has_antennatype,
>&wwan_antennatype);
>> /*
>> * If support isn't available (ENODEV) for both devices then quit, but
>> * don't return an error.
>> */
>> - if (wlantx_err == -ENODEV)
>> + if ((wlantx_err == -ENODEV) && (wwanantenna_err == -ENODEV))
>> return 0;
>> /* Otherwise, if there was an error return it */
>> if (wlantx_err && (wlantx_err != -ENODEV))
>> return wlantx_err;
>> + if (wwanantenna_err && (wwanantenna_err != -ENODEV))
>> + return wwanantenna_err;
>>
>> if (has_wlantxreduce) {
>> err = sysfs_create_file(&tpacpi_pdev->dev.kobj,
>> @@ -10101,6 +10144,12 @@ static int tpacpi_dprc_init(struct
>ibm_init_struct *iibm)
>> if (err)
>> return err;
>> }
>> +
>> + if (has_antennatype) {
>> + err = sysfs_create_file(&tpacpi_pdev->dev.kobj,
>&dev_attr_wwan_antenna_type.attr);
>> + if (err)
>> + return err;
>> + }
>> return 0;
>> }
>> [...]
>
>
>Regards,
>Barnabás Pőcze
Thanks & regards,
Nitin Joshi
