| Message ID | 20240726-topic-fs4480_check-v2-1-901ca449db15@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] usb: typec: fsa4480: Check if the chip is really there | expand |
On Fri, Jul 26, 2024 at 01:43:30PM +0200, Konrad Dybcio wrote: > From: Konrad Dybcio <konrad.dybcio@linaro.org> > > Currently, the driver will happily register the switch/mux devices, and > so long as the i2c master doesn't complain, the user would never know > there's something wrong. > > Add a device id check (based on [1]) and return -ENODEV if the read > fails or returns nonsense. > > Checking the value on a Qualcomm SM6115P-based Lenovo Tab P11 tablet, > the ID mentioned in the datasheet does indeed show up: > fsa4480 1-0042: Found FSA4480 v1.1 (Vendor ID = 0) > > [1] https://www.onsemi.com/pdf/datasheet/fsa4480-d.pdf > > Fixes: 1dc246320c6b ("usb: typec: mux: Add On Semi fsa4480 driver") > Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> > Signed-off-by: Konrad Dybcio <konradybcio@kernel.org> You can't sign off on a patch twice, that makes no sense, sorry. greg k-h
On 26.07.2024 3:12 PM, Greg Kroah-Hartman wrote: > On Fri, Jul 26, 2024 at 01:43:30PM +0200, Konrad Dybcio wrote: >> From: Konrad Dybcio <konrad.dybcio@linaro.org> >> >> Currently, the driver will happily register the switch/mux devices, and >> so long as the i2c master doesn't complain, the user would never know >> there's something wrong. >> >> Add a device id check (based on [1]) and return -ENODEV if the read >> fails or returns nonsense. >> >> Checking the value on a Qualcomm SM6115P-based Lenovo Tab P11 tablet, >> the ID mentioned in the datasheet does indeed show up: >> fsa4480 1-0042: Found FSA4480 v1.1 (Vendor ID = 0) >> >> [1] https://www.onsemi.com/pdf/datasheet/fsa4480-d.pdf >> >> Fixes: 1dc246320c6b ("usb: typec: mux: Add On Semi fsa4480 driver") >> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> >> Signed-off-by: Konrad Dybcio <konradybcio@kernel.org> > You can't sign off on a patch twice, that makes no sense, sorry. I'm losing access to the @linaro.org email and want to preserve the authorship there (as this patch was developed during work hours). Then, the author's email doesn't match the sender's email, so I'm expected to sign off with the sender's one. Should I assume that the maintainer trusts me to be the same person? Konrad
On Fri, Jul 26, 2024 at 03:52:22PM GMT, Konrad Dybcio wrote: > > > On 26.07.2024 3:12 PM, Greg Kroah-Hartman wrote: > > On Fri, Jul 26, 2024 at 01:43:30PM +0200, Konrad Dybcio wrote: > >> From: Konrad Dybcio <konrad.dybcio@linaro.org> > >> > >> Currently, the driver will happily register the switch/mux devices, and > >> so long as the i2c master doesn't complain, the user would never know > >> there's something wrong. > >> > >> Add a device id check (based on [1]) and return -ENODEV if the read > >> fails or returns nonsense. > >> > >> Checking the value on a Qualcomm SM6115P-based Lenovo Tab P11 tablet, > >> the ID mentioned in the datasheet does indeed show up: > >> fsa4480 1-0042: Found FSA4480 v1.1 (Vendor ID = 0) > >> > >> [1] https://www.onsemi.com/pdf/datasheet/fsa4480-d.pdf > >> > >> Fixes: 1dc246320c6b ("usb: typec: mux: Add On Semi fsa4480 driver") > >> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> > >> Signed-off-by: Konrad Dybcio <konradybcio@kernel.org> > > You can't sign off on a patch twice, that makes no sense, sorry. > > I'm losing access to the @linaro.org email and want to preserve the > authorship there (as this patch was developed during work hours). > > Then, the author's email doesn't match the sender's email, so I'm > expected to sign off with the sender's one. > The author is Linaro and as such the first s-o-b is correct/required. > Should I assume that the maintainer trusts me to be the same person? > I think in many cases you can assume that, but I find it reasonable that you certify the origin of the patch anew, even though you happen to be the same physical person. Regards, Bjorn
On Fri, Jul 26, 2024 at 03:52:22PM +0200, Konrad Dybcio wrote: > > > On 26.07.2024 3:12 PM, Greg Kroah-Hartman wrote: > > On Fri, Jul 26, 2024 at 01:43:30PM +0200, Konrad Dybcio wrote: > >> From: Konrad Dybcio <konrad.dybcio@linaro.org> > >> > >> Currently, the driver will happily register the switch/mux devices, and > >> so long as the i2c master doesn't complain, the user would never know > >> there's something wrong. > >> > >> Add a device id check (based on [1]) and return -ENODEV if the read > >> fails or returns nonsense. > >> > >> Checking the value on a Qualcomm SM6115P-based Lenovo Tab P11 tablet, > >> the ID mentioned in the datasheet does indeed show up: > >> fsa4480 1-0042: Found FSA4480 v1.1 (Vendor ID = 0) > >> > >> [1] https://www.onsemi.com/pdf/datasheet/fsa4480-d.pdf > >> > >> Fixes: 1dc246320c6b ("usb: typec: mux: Add On Semi fsa4480 driver") > >> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> > >> Signed-off-by: Konrad Dybcio <konradybcio@kernel.org> > > You can't sign off on a patch twice, that makes no sense, sorry. > > I'm losing access to the @linaro.org email and want to preserve the > authorship there (as this patch was developed during work hours). > > Then, the author's email doesn't match the sender's email, so I'm > expected to sign off with the sender's one. > > Should I assume that the maintainer trusts me to be the same person? Yes I do :)
On Fri, Jul 26, 2024 at 01:43:30PM GMT, Konrad Dybcio wrote: > From: Konrad Dybcio <konrad.dybcio@linaro.org> > > Currently, the driver will happily register the switch/mux devices, and > so long as the i2c master doesn't complain, the user would never know > there's something wrong. > > Add a device id check (based on [1]) and return -ENODEV if the read > fails or returns nonsense. > > Checking the value on a Qualcomm SM6115P-based Lenovo Tab P11 tablet, > the ID mentioned in the datasheet does indeed show up: > fsa4480 1-0042: Found FSA4480 v1.1 (Vendor ID = 0) So wonderful to have 0 Vendor ID (initially assumed that you are showing it as an example of an error). But yes, the datasheet has 0 there. > > [1] https://www.onsemi.com/pdf/datasheet/fsa4480-d.pdf > > Fixes: 1dc246320c6b ("usb: typec: mux: Add On Semi fsa4480 driver") > Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> > Signed-off-by: Konrad Dybcio <konradybcio@kernel.org> > --- > Changes in v2: > - Prepend the new defines with FSA4480_ to make them more obvious > - Link to v1: https://lore.kernel.org/r/20240212-topic-fs4480_check-v1-1-d9969e4d6f9a@linaro.org > --- > drivers/usb/typec/mux/fsa4480.c | 14 ++++++++++++++ > 1 file changed, 14 insertions(+) With the S-o-B tags fixed: Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
diff --git a/drivers/usb/typec/mux/fsa4480.c b/drivers/usb/typec/mux/fsa4480.c index cb7cdf90cb0a..cd235339834b 100644 --- a/drivers/usb/typec/mux/fsa4480.c +++ b/drivers/usb/typec/mux/fsa4480.c @@ -13,6 +13,10 @@ #include <linux/usb/typec_dp.h> #include <linux/usb/typec_mux.h> +#define FSA4480_DEVICE_ID 0x00 + #define FSA4480_DEVICE_ID_VENDOR_ID GENMASK(7, 6) + #define FSA4480_DEVICE_ID_VERSION_ID GENMASK(5, 3) + #define FSA4480_DEVICE_ID_REV_ID GENMASK(2, 0) #define FSA4480_SWITCH_ENABLE 0x04 #define FSA4480_SWITCH_SELECT 0x05 #define FSA4480_SWITCH_STATUS1 0x07 @@ -251,6 +255,7 @@ static int fsa4480_probe(struct i2c_client *client) struct typec_switch_desc sw_desc = { }; struct typec_mux_desc mux_desc = { }; struct fsa4480 *fsa; + int val = 0; int ret; fsa = devm_kzalloc(dev, sizeof(*fsa), GFP_KERNEL); @@ -268,6 +273,15 @@ static int fsa4480_probe(struct i2c_client *client) if (IS_ERR(fsa->regmap)) return dev_err_probe(dev, PTR_ERR(fsa->regmap), "failed to initialize regmap\n"); + ret = regmap_read(fsa->regmap, FSA4480_DEVICE_ID, &val); + if (ret || !val) + return dev_err_probe(dev, -ENODEV, "FSA4480 not found\n"); + + dev_dbg(dev, "Found FSA4480 v%lu.%lu (Vendor ID = %lu)\n", + FIELD_GET(FSA4480_DEVICE_ID_VERSION_ID, val), + FIELD_GET(FSA4480_DEVICE_ID_REV_ID, val), + FIELD_GET(FSA4480_DEVICE_ID_VENDOR_ID, val)); + /* Safe mode */ fsa->cur_enable = FSA4480_ENABLE_DEVICE | FSA4480_ENABLE_USB; fsa->mode = TYPEC_STATE_SAFE;