Re: [PATCH] I2C: Explicitly include correct DT includes
Hi Rob, On Fri, Jul 14, 2023 at 11:46:16AM -0600, Rob Herring wrote: > The DT of_device.h and of_platform.h date back to the separate > of_platform_bus_type before it as merged into the regular platform bus. > As part of that merge prepping Arm DT support 13 years ago, they > "temporarily" include each other. They also include platform_device.h > and of.h. As a result, there's a pretty much random mix of those include > files used throughout the tree. In order to detangle these headers and > replace the implicit includes with struct declarations, users need to > explicitly include the correct includes. > > Signed-off-by: Rob Herring I went through all of the changes and they look fine to me. Reviewed-by: Andi Shyti Thanks, Andi
Re: [PATCH v2 03/10] i2c: pasemi: Don't let i2c adapters declare I2C_CLASS_SPD support if they support I2C_CLASS_HWMON
Hi Heiner, On Fri, Nov 24, 2023 at 11:16:12AM +0100, Heiner Kallweit wrote: > After removal of the legacy eeprom driver the only remaining I2C > client device driver supporting I2C_CLASS_SPD is jc42. Because this > driver also supports I2C_CLASS_HWMON, adapters don't have to > declare support for I2C_CLASS_SPD if they support I2C_CLASS_HWMON. > It's one step towards getting rid of I2C_CLASS_SPD mid-term. > > Series was created supported by Coccinelle and its splitpatch. > > Signed-off-by: Heiner Kallweit Acked-by: Andi Shyti Thanks, Andi
Re: [PATCH] i2c: cpm: Fix data type
Hi Christophe, On Tue, Dec 05, 2023 at 07:16:53PM +0100, Christophe Leroy wrote: > sparse reports an error on some data that gets converted from be32. > > That's because that data is typed u32 instead of __be32. > > Fix it. the reason for this sparse error is that the data variables is then parsed with a be32_to_cpup() and I think that's not necessary. I think the real fix here is to not use be32_to_cpup(). Andi
Re: [PATCH v2] i2c: cpm: Remove linux,i2c-index conversion from be32
Hi Christophe, On Wed, Dec 06, 2023 at 11:24:03PM +0100, Christophe Leroy wrote: > sparse reports an error on some data that gets converted from be32. > > That's because that data is typed u32 instead of __be32. > > The type is correct, the be32_to_cpu() conversion is not. > > Remove the conversion. > > Reported-by: kernel test robot > Closes: > https://lore.kernel.org/oe-kbuild-all/202312042210.ql4da8av-...@intel.com/ > Signed-off-by: Christophe Leroy > --- > v2: Use u32 directly, remove be32_to_cpu(). > --- > drivers/i2c/busses/i2c-cpm.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/i2c/busses/i2c-cpm.c b/drivers/i2c/busses/i2c-cpm.c > index 9a664abf734d..771d60bc8d71 100644 > --- a/drivers/i2c/busses/i2c-cpm.c > +++ b/drivers/i2c/busses/i2c-cpm.c > @@ -658,7 +658,7 @@ static int cpm_i2c_probe(struct platform_device *ofdev) > /* register new adapter to i2c module... */ > > data = of_get_property(ofdev->dev.of_node, "linux,i2c-index", &len); > - cpm->adap.nr = (data && len == 4) ? be32_to_cpup(data) : -1; > + cpm->adap.nr = (data && len == 4) ? *data : -1; thanks! Reviewed-by: Andi Shyti Andi
Re: [PATCH] i2c: pasemi: split driver into two separate modules
Hi On Mon, 12 Feb 2024 12:19:04 +0100, Arnd Bergmann wrote: > On powerpc, it is possible to compile test both the new apple (arm) and > old pasemi (powerpc) drivers for the i2c hardware at the same time, > which leads to a warning about linking the same object file twice: > > scripts/Makefile.build:244: drivers/i2c/busses/Makefile: i2c-pasemi-core.o is > added to multiple modules: i2c-apple i2c-pasemi > > Rework the driver to have an explicit helper module, letting Kbuild > take care of whether this should be built-in or a loadable driver. > > [...] Applied to i2c/i2c-host-fixes on git://git.kernel.org/pub/scm/linux/kernel/git/andi.shyti/linux.git Thank you, Andi Patches applied === [1/1] i2c: pasemi: split driver into two separate modules commit: 3fab8a74c71a4ba32b2fa1dca7340f9107ff8dfc
Re: [PATCH 00/64] i2c: reword i2c_algorithm according to newest specification
Hi Wolfram, On Fri, Mar 22, 2024 at 02:24:53PM +0100, Wolfram Sang wrote: > Okay, we need to begin somewhere... > > Start changing the wording of the I2C main header wrt. the newest I2C > v7, SMBus 3.2, I3C specifications and replace "master/slave" with more > appropriate terms. This first step renames the members of struct > i2c_algorithm. Once all in-tree users are converted, the anonymous union > will go away again. All this work will also pave the way for finally > seperating the monolithic header into more fine-grained headers like > "i2c/clients.h" etc. So, this is not a simple renaming-excercise but > also a chance to update the I2C core to recent Linux standards. yes, very good! It's clearly stated in all three documentations that Target replaces Slave and Controller replaces Master (i3c is at the 1.1.1 version). > My motivation is to improve the I2C core API, in general. My motivation > is not to clean each and every driver. I think this is impossible > because register names based on official documentation will need to stay > as they are. But the Linux-internal names should be updated IMO. Also because some drivers have been written based on previous specifications where master/slave was used. > That being said, I worked on 62 drivers in this series beyond plain > renames inside 'struct i2c_algorithm' because the fruits were so > low-hanging. Before this series, 112 files in the 'busses/' directory > contained 'master' and/or 'slave'. After the series, only 57. Why not? > > Next step is updating the drivers outside the 'i2c'-folder regarding > 'struct i2c_algorithm' so we can remove the anonymous union ASAP. To be > able to work on this with minimal dependencies, I'd like to apply this > series between -rc1 and -rc2. > > I hope this will work for you guys. The changes are really minimal. If > you are not comfortable with changes to your driver or need more time to > review, please NACK the patch and I will drop the patch and/or address > the issues separeately. > > @Andi: are you okay with this approach? It means you'd need to merge > -rc2 into your for-next branch. Or rebase if all fails. I think it's a good plan, I'll try to support you with it. > Speaking of Andi, thanks a lot to him taking care of the controller > drivers these days. His work really gives me the freedom to work on I2C > core issues again. Thank you, Wolfram! Andi
Re: [PATCH 00/64] i2c: reword i2c_algorithm according to newest specification
Hi Wolfram, > > @Andi: are you okay with this approach? It means you'd need to merge > > -rc2 into your for-next branch. Or rebase if all fails. > > I think it's a good plan, I'll try to support you with it. Do you feel more comfortable if I take the patches as soon as they are reviewd? So far I have tagged patch 1-4 and I can already merge 2,3,4 as long as you merge patch 1. Andi
Re: [PATCH 14/64] i2c: cpm: reword according to newest specification
Hi Wolfram, ... > @@ -570,7 +570,7 @@ static int cpm_i2c_setup(struct cpm_i2c *cpm) > out_8(&cpm->i2c_reg->i2brg, brg); > > out_8(&cpm->i2c_reg->i2mod, 0x00); > - out_8(&cpm->i2c_reg->i2com, I2COM_MASTER); /* Master mode */ > + out_8(&cpm->i2c_reg->i2com, I2COM_MASTER); /* Host mode */ I2COM_MASTER might be coming from the datasheet. Reviewed-by: Andi Shyti Thanks,
Re: [PATCH 14/64] i2c: cpm: reword according to newest specification
> > > out_8(&cpm->i2c_reg->i2mod, 0x00); > > > - out_8(&cpm->i2c_reg->i2com, I2COM_MASTER); /* Master mode */ > > > + out_8(&cpm->i2c_reg->i2com, I2COM_MASTER); /* Host mode */ > > > > I2COM_MASTER might be coming from the datasheet. > > Maybe we can just drop the comment? The value we write is pretty > self-explaining. indeed. Andi
Re: [PATCH] i2c: mpc: Removal of of_node_put with __free for auto cleanup
Hi Chris, On Tue, Apr 16, 2024 at 03:59:13AM +, Chris Packham wrote: > On 16/04/24 08:54, Andi Shyti wrote: > >>/* Enable I2C interrupts for mpc5121 */ > >> - node_ctrl = of_find_compatible_node(NULL, NULL, > >> - "fsl,mpc5121-i2c-ctrl"); > >> + struct device_node *node_ctrl __free(device_node) = > > How have you tested this? > > I'm not sure I know anyone that still has a mpc5121. Maybe someone on > linuxppc-dev? that's why I was asking, this would be the first driver in i2c using Rob's __free(device_node). > I did try to take the patch for a spin on my T2081RDB but I'm having > some userland issues on it for some reason (unrelated to this change). > The kernel boot does discover a few peripherals hanging of the I2C > interface but I'm not in a position to offer up a Tested-by and I've run > out of time to debug why my board is unhappy. Thanks for giving it a try. Andi
Re: [PATCH] i2c: mpc: Removal of of_node_put with __free for auto cleanup
On Tue, Apr 16, 2024 at 04:07:48PM +0200, Andi Shyti wrote: > On Tue, Apr 16, 2024 at 03:59:13AM +, Chris Packham wrote: > > On 16/04/24 08:54, Andi Shyti wrote: > > >> /* Enable I2C interrupts for mpc5121 */ > > >> -node_ctrl = of_find_compatible_node(NULL, NULL, > > >> -"fsl,mpc5121-i2c-ctrl"); > > >> +struct device_node *node_ctrl __free(device_node) = > > > How have you tested this? > > > > I'm not sure I know anyone that still has a mpc5121. Maybe someone on > > linuxppc-dev? > > that's why I was asking, this would be the first driver in i2c > using Rob's __free(device_node). Jonathan's, of course :-) Andi
Re: [PATCH v1 1/1] treewide: Align match_string() with sysfs_match_string()
Hi Andy,
On Sun, Jun 02, 2024 at 06:57:12PM +0300, Andy Shevchenko wrote:
> Make two APIs look similar. Hence convert match_string() to be
> a 2-argument macro. In order to avoid unneeded churn, convert
> all users as well. There is no functional change intended.
>
> Signed-off-by: Andy Shevchenko
nice patch, I checked some (maybe most) of your changes. There
are a few unrelated changes which I don't mind, but there are two
errors where the error value changes from ENODEV to EINVAL.
Find the comments through the line.
...
> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> index 1b7e82a0ad2e..b6f52f44625f 100644
> --- a/drivers/cpufreq/amd-pstate.c
> +++ b/drivers/cpufreq/amd-pstate.c
> @@ -1117,9 +1117,9 @@ static ssize_t store_energy_performance_preference(
> if (ret != 1)
> return -EINVAL;
>
> - ret = match_string(energy_perf_strings, -1, str_preference);
> + ret = __match_string(energy_perf_strings, -1, str_preference);
> if (ret < 0)
> - return -EINVAL;
> + return ret;
a bit of unrelated changes here, but I guess no one will complain :-)
>
> mutex_lock(&amd_pstate_limits_lock);
> ret = amd_pstate_set_energy_pref_index(cpudata, ret);
...
> diff --git a/drivers/mmc/host/sdhci-xenon-phy.c
> b/drivers/mmc/host/sdhci-xenon-phy.c
> index cc9d28b75eb9..1865e26ae736 100644
> --- a/drivers/mmc/host/sdhci-xenon-phy.c
> +++ b/drivers/mmc/host/sdhci-xenon-phy.c
> @@ -135,15 +135,14 @@ struct xenon_emmc_phy_regs {
> u32 logic_timing_val;
> };
>
> -static const char * const phy_types[] = {
> - "emmc 5.0 phy",
> - "emmc 5.1 phy"
> -};
> -
> enum xenon_phy_type_enum {
> EMMC_5_0_PHY,
> EMMC_5_1_PHY,
> - NR_PHY_TYPES
> +};
> +
> +static const char * const phy_types[] = {
> + [EMMC_5_0_PHY] = "emmc 5.0 phy",
> + [EMMC_5_1_PHY] = "emmc 5.1 phy",
> };
Another unrelated cleanup, but I don't complain
> enum soc_pad_ctrl_type {
...
> - tablet_found = match_string(tablet_chassis_types,
> - ARRAY_SIZE(tablet_chassis_types),
> - chassis_type) >= 0;
> - if (!tablet_found)
> - return -ENODEV;
> + ret = match_string(tablet_chassis_types, chassis_type);
> + if (ret < 0)
> + return ret;
This is a logical change though, because we are changing from
-ENODEV to -EINVAL. Even if it might look the right thing, but
still, it's a logical change.
>
> ret = hp_wmi_perform_query(HPWMI_SYSTEM_DEVICE_MODE, HPWMI_READ,
> system_device_mode,
> zero_if_sup(system_device_mode),
> @@ -490,9 +487,7 @@ static bool is_omen_thermal_profile(void)
> if (!board_name)
> return false;
>
> - return match_string(omen_thermal_profile_boards,
> - ARRAY_SIZE(omen_thermal_profile_boards),
> - board_name) >= 0;
> + return match_string(omen_thermal_profile_boards, board_name) >= 0;
> }
>
> static int omen_get_thermal_policy_version(void)
...
> diff --git a/drivers/thermal/intel/int340x_thermal/processor_thermal_rfim.c
> b/drivers/thermal/intel/int340x_thermal/processor_thermal_rfim.c
> index e56db75a94fb..dbd176b0fb1f 100644
> --- a/drivers/thermal/intel/int340x_thermal/processor_thermal_rfim.c
> +++ b/drivers/thermal/intel/int340x_thermal/processor_thermal_rfim.c
> @@ -111,7 +111,7 @@ static ssize_t suffix##_show(struct device *dev,\
> match_strs = (const char **)fivr_strings;\
> mmio_regs = tgl_fivr_mmio_regs;\
> } \
> - ret = match_string(match_strs, -1, attr->attr.name);\
> + ret = __match_string(match_strs, -1, attr->attr.name);\
> if (ret < 0)\
> return ret;\
> reg_val = readl((void __iomem *) (proc_priv->mmio_base +
> mmio_regs[ret].offset));\
> @@ -145,7 +145,7 @@ static ssize_t suffix##_store(struct device *dev,\
> mmio_regs = tgl_fivr_mmio_regs;\
> } \
> \
> - ret = match_string(match_strs, -1, attr->attr.name);\
> + ret = __match_string(match_strs, -1, attr->attr.name);\
> if (ret < 0)\
> return ret;\
> if (mmio_regs[ret].read_only)\
> diff --git a/drivers/thermal/intel/int340x_thermal/processor_thermal_wt_req.c
> b/drivers/thermal/intel/int340x_thermal/processor_thermal_wt_req.c
> index f298e7442662..57f456befb34 100644
> --- a/drivers/thermal/intel/int340x_thermal/processor_thermal_wt_req.c
> +++ b/drivers/thermal/intel/int340x_thermal/processor_thermal_wt_req.c
> @@ -50,7 +50,7 @@ static ssize_t workload_type_store(struct device *dev,
> if (ret != 1)
> return -EINVAL;
>
> - ret = match_string(workload_types, -1, str_preference);
> + ret = __match_string(workload_types, -1, str_preference);
We could even thing of a "match_string_terminated" (or a better
name), but maybe it's too much?
> if (ret < 0)
>
Re: [Patch v4 10/10] i2x: pnx: Use threaded irq to fix warning from del_timer_sync()
Hi Piotr, On Thu, Jun 20, 2024 at 07:56:41PM GMT, Piotr Wojtaszczyk wrote: > When del_timer_sync() is called in an interrupt context it throws a warning > because of potential deadlock. Threaded irq handler fixes the potential > problem. > > Signed-off-by: Piotr Wojtaszczyk did you run into a lockdep splat? Anything against using del_timer(), instead? Have you tried? Thanks, Andi
Re: [Patch v4 10/10] i2x: pnx: Use threaded irq to fix warning from del_timer_sync()
Hi Piotr, On Fri, Jun 21, 2024 at 02:08:03PM GMT, Piotr Wojtaszczyk wrote: > On Fri, Jun 21, 2024 at 12:57 AM Andi Shyti wrote: > > On Thu, Jun 20, 2024 at 07:56:41PM GMT, Piotr Wojtaszczyk wrote: > > > When del_timer_sync() is called in an interrupt context it throws a > > > warning > > > because of potential deadlock. Threaded irq handler fixes the potential > > > problem. > > > > > > Signed-off-by: Piotr Wojtaszczyk > > > > did you run into a lockdep splat? > > > > Anything against using del_timer(), instead? Have you tried? > > I didn't get a lockdep splat but console was flooded with warnings from > https://github.com/torvalds/linux/blob/v6.10-rc4/kernel/time/timer.c#L1655 > In the linux kernel v5.15 I didn't see these warnings. > > I'm not a maintainer of the driver and I didn't do any research on > what kind of impact > would have using del_timer() instad. Maybe Vladimir Zapolskiy will know that. Your patch is definitely correct, no doubt about that. And I don't have anything aginast changing irq handlers to threaded handlers. But I would be careful at doing that depending on the use of the controller and for accepting such change I would need an ack from someone who knows the device. Vladimir, perhaps? There are cases where using threaded handlers are not totally right, for example when the controller is used at early boot for power management handling. I don't think it's the case for this driver, but I can't be 100% sure. If you were able to see the flood of WARN_ON's, would be interesting to know how it behaves with del_timer(). Mind giving it a test? Thanks, Andi
Re: [PATCH v2 35/60] i2c: opal: reword according to newest specification
Hi Wolfram, On Sat, Jul 06, 2024 at 01:20:35PM GMT, Wolfram Sang wrote: > Change the wording of this driver wrt. the newest I2C v7 and SMBus 3.2 > specifications and replace "master/slave" with more appropriate terms. > > Signed-off-by: Wolfram Sang Reviewed-by: Andi Shyti Thanks, Andi
Re: [PATCH v2 37/60] i2c: pasemi: reword according to newest specification
Hi Wolfram, On Sat, Jul 06, 2024 at 01:20:37PM GMT, Wolfram Sang wrote: > Change the wording of this driver wrt. the newest I2C v7 and SMBus 3.2 > specifications and replace "master/slave" with more appropriate terms. > > Signed-off-by: Wolfram Sang Reviewed-by: Andi Shyti Thanks, Andi
Re: [PATCH v2 39/60] i2c: powermac: reword according to newest specification
Hi Wolfram, > -static int i2c_powermac_master_xfer( struct i2c_adapter *adap, > - struct i2c_msg *msgs, > - int num) > +static int i2c_powermac_xfer(struct i2c_adapter *adap, > + struct i2c_msg *msgs, > + int num) and we get a nice free cleanup here :-) Reviewed-by: Andi Shyti Thanks, Andi
Re: [PATCH v2 00/60] i2c: reword first drivers according to newest specification
Hi Wolfram, pushed in i2c/i2c-host. Thanks for this big work, at the end it turned out quite nice and I'm happy of the outcome! Thanks Andi On Sat, Jul 06, 2024 at 01:20:00PM GMT, Wolfram Sang wrote: > Start changing the wording of the I2C main header wrt. the newest I2C > v7 and SMBus 3.2 specifications and replace "master/slave" with more > appropriate terms. This first step renames the members of struct > i2c_algorithm. Once all in-tree users are converted, the anonymous union > will go away again. All this work will also pave the way for finally > seperating the monolithic header into more fine-grained headers like > "i2c/clients.h" etc. So, this is not a simple renaming-excercise but > also a chance to update the I2C core to recent Linux standards. > > Changes since v1: > > * changed wording according to the terminology we agreed on and defined > upstream. That means consistent use of "controller/target", and no > more "host/client". I added "local/remote target" where necessary. > * added tags which I kept despite some changes in wording. The approach > and code changes (if necessary) did not change. > * rebased to Andi's for-next branch > * this series only contains patches which convert the drivers fully. If > all goes well, no more updates for them are needed. The previous > series converted all users of "master_xfer". But to avoid tons of > incremental patches to one driver, I will incrementally improve i2c.h > and see which drivers can be fully converted step-by-step. > * do not mention I3C specs in commit messages, not really relevant here > > Please note that I am not super strict with the 80 char limit. And, as > agreed, I did not convert occasions where old terminology is used in > register names or bits etc. or in function names outside of the I2C > realm. > > The outcome is that before this series 115 drivers use old terminology, > after this only 54. Hooray. > > And a comment to all janitors: Do not convert I2C drivers outside of > drivers/i2c yet. Let us first gain experience here and present the > well-tested results of what we figured out to other maintainers then. > This ensures they have to deal with way less patch revisions. > > Thanks and happy hacking! > > > Wolfram Sang (60): > i2c: reword i2c_algorithm according to newest specification > i2c: ali15x3: reword according to newest specification > i2c: altera: reword according to newest specification > i2c: au1550: reword according to newest specification > i2c: bcm-kona: reword according to newest specification > i2c: bcm2835: reword according to newest specification > i2c: brcmstb: reword according to newest specification > i2c: cht-wc: reword according to newest specification > i2c: cp2615: reword according to newest specification > i2c: cros-ec-tunnel: reword according to newest specification > i2c: davinci: reword according to newest specification > i2c: digicolor: reword according to newest specification > i2c: diolan-u2c: reword according to newest specification > i2c: dln2: reword according to newest specification > i2c: fsi: reword according to newest specification > i2c: gpio: reword according to newest specification > i2c: highlander: reword according to newest specification > i2c: hisi: reword according to newest specification > i2c: hix5hd2: reword according to newest specification > i2c: i801: reword according to newest specification > i2c: ibm_iic: reword according to newest specification > i2c: iop3xx: reword according to newest specification > i2c: isch: reword according to newest specification > i2c: jz4780: reword according to newest specification > i2c: kempld: reword according to newest specification > i2c: ljca: reword according to newest specification > i2c: lpc2k: reword according to newest specification > i2c: ls2x: reword according to newest specification > i2c: mlxcpld: reword according to newest specification > i2c: mpc: reword according to newest specification > i2c: mt7621: reword according to newest specification > i2c: mv64xxx: reword according to newest specification > i2c: ocores: reword according to newest specification > i2c: octeon: reword according to newest specification > i2c: opal: reword according to newest specification > i2c: owl: reword according to newest specification > i2c: pasemi: reword according to newest specification > i2c: piix4: reword according to newest specification > i2c: powermac: reword according to newest specification > i2c: pxa-pci: reword according to newest specification > i2c: riic: reword according to newest specification > i2c: rk3x: reword according to newest specification > i2c: robotfuzz-osif: reword according to newest specification > i2c: rzv2m: reword according to newest specification > i2c: sis5595: reword according to newest specification > i2c: sprd: reword according to newest specification > i2c: stm32f4: reword according to newest speci
Re: [PATCH] of/irq: handle irq_of_parse_and_map() errors
On Tue, Sep 03, 2024 at 06:56:41PM GMT, Christophe Leroy wrote:
> Le 30/08/2024 à 16:21, Ma Ke a écrit :
> > Zero and negative number is not a valid IRQ for in-kernel code and the
> > irq_of_parse_and_map() function returns zero on error. So this check for
> > valid IRQs should only accept values > 0.
>
> unsigned int irq_of_parse_and_map(struct device_node *node, int index);
>
> I can't see how an 'unsigned int' can be negative.
hehe... correct... even though looks like we are walking on a
slackline, relying too much that no one in the future, inside
irq_of_parse_and_map() (and various callers), someone will try to
fit an -ENOSOMETHING into the unsigned int.
I wouldn't mind something like this[*] to ensure I can sleep
soundly.
Thanks for the review,
Andi
[*]
diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index cea8f6874b1fb..df44a8ffa6843 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -954,6 +954,8 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec
*fwspec)
out:
mutex_unlock(&domain->root->mutex);
+ BUG_ON(virq < 0);
+
return virq;
}
EXPORT_SYMBOL_GPL(irq_create_fwspec_mapping);
> Christophe
>
> >
> > Cc: sta...@vger.kernel.org
> > Fixes: f7578496a671 ("of/irq: Use irq_of_parse_and_map()")
> > Signed-off-by: Ma Ke
> > ---
> > drivers/i2c/busses/i2c-cpm.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/i2c/busses/i2c-cpm.c b/drivers/i2c/busses/i2c-cpm.c
> > index 4794ec066eb0..41e3c95c0ef7 100644
> > --- a/drivers/i2c/busses/i2c-cpm.c
> > +++ b/drivers/i2c/busses/i2c-cpm.c
> > @@ -435,7 +435,7 @@ static int cpm_i2c_setup(struct cpm_i2c *cpm)
> > init_waitqueue_head(&cpm->i2c_wait);
> > cpm->irq = irq_of_parse_and_map(ofdev->dev.of_node, 0);
> > - if (!cpm->irq)
> > + if (cpm->irq <= 0)
> > return -EINVAL;
> > /* Install interrupt handler. */
Re: [PATCH 1/4] i2c: pasemi: Add registers bits and switch to BIT()
Hi Sven, On Sat, Feb 22, 2025 at 01:38:33PM +, Sven Peter via B4 Relay wrote: > From: Sven Peter > > Add the missing register bits to the defines and also switch > those to use the BIT macro which is much more readable than > using hardcoded masks > > Co-developed-by: Hector Martin > Signed-off-by: Hector Martin > Signed-off-by: Sven Peter Just this patch merged to i2c/i2c-host. Patch 3 and 4 look good, just some comments on patch 2. Thanks, Andi
Re: [PATCH 2/4] i2c: pasemi: Improve error recovery
Hi Sven,
On Sat, Feb 22, 2025 at 01:38:34PM +, Sven Peter via B4 Relay wrote:
> The hardware (supposedly) has a 25ms timeout for clock stretching
> and the driver uses 100ms which should be plenty.
Can we add this lines as a comment to the define you are adding?
> The error
> reocvery itself is however lacking.
...
> -static void pasemi_smb_clear(struct pasemi_smbus *smbus)
> +static int pasemi_smb_clear(struct pasemi_smbus *smbus)
> {
> unsigned int status;
> + int timeout = TRANSFER_TIMEOUT_MS;
>
> status = reg_read(smbus, REG_SMSTA);
> +
> + /* First wait for the bus to go idle */
> + while ((status & (SMSTA_XIP | SMSTA_JAM)) && timeout--) {
> + msleep(1);
Please, use usleep_range for 1 millisecond timeout.
> + status = reg_read(smbus, REG_SMSTA);
> + }
You could use here readx_poll_timeout() here.
> +
> + if (timeout < 0) {
> + dev_warn(smbus->dev, "Bus is still stuck (status 0x%08x)\n",
> status);
if it's an error, please use an error.
> + return -EIO;
> + }
> +
> + /* If any badness happened or there is data in the FIFOs, reset the
> FIFOs */
> + if ((status & (SMSTA_MRNE | SMSTA_JMD | SMSTA_MTO | SMSTA_TOM |
> SMSTA_MTN | SMSTA_MTA)) ||
> + !(status & SMSTA_MTE))
Please, fixe the alignment here.
> + pasemi_reset(smbus);
> +
> + /* Clear the flags */
> reg_write(smbus, REG_SMSTA, status);
> +
> + return 0;
> }
>
> static int pasemi_smb_waitready(struct pasemi_smbus *smbus)
> {
> - int timeout = 100;
> + int timeout = TRANSFER_TIMEOUT_MS;
> unsigned int status;
>
> if (smbus->use_irq) {
> reinit_completion(&smbus->irq_completion);
> - reg_write(smbus, REG_IMASK, SMSTA_XEN | SMSTA_MTN);
> - wait_for_completion_timeout(&smbus->irq_completion,
> msecs_to_jiffies(100));
> + /* XEN should be set when a transaction terminates, whether due
> to error or not */
> + reg_write(smbus, REG_IMASK, SMSTA_XEN);
> + wait_for_completion_timeout(&smbus->irq_completion,
> msecs_to_jiffies(timeout));
what happens if the timeout expires?
> reg_write(smbus, REG_IMASK, 0);
> status = reg_read(smbus, REG_SMSTA);
> } else {
...
> struct pasemi_smbus *smbus = adapter->algo_data;
> int ret, i;
>
> - pasemi_smb_clear(smbus);
> + if (pasemi_smb_clear(smbus))
> + return -EIO;
Can we use
ret = ...
if (ret)
return ret;
This way we return whatever comes from pasemi_smb_clear().
>
> ret = 0;
This way we can remove this line, as well.
Andi
Re: [PATCH v3 0/4] Apple/PASemi i2c error recovery fixes
Hi Sven, > Hector Martin (3): > i2c: pasemi: Enable the unjam machine > i2c: pasemi: Improve error recovery > i2c: pasemi: Log bus reset causes > > Sven Peter (1): > i2c: pasemi: Improve timeout handling merged to i2c/i2c-host. Thanks, Andi
Re: [PATCH v2 3/6] i2c: pasemi: Improve timeout handling
Hi Sven,
> static int pasemi_smb_waitready(struct pasemi_smbus *smbus)
> {
> int timeout = 100;
> + int ret;
can you please declare this "ret" inside the if() statement
below?
> unsigned int status;
>
> if (smbus->use_irq) {
> reinit_completion(&smbus->irq_completion);
> reg_write(smbus, REG_IMASK, SMSTA_XEN | SMSTA_MTN);
> - wait_for_completion_timeout(&smbus->irq_completion,
> msecs_to_jiffies(100));
> + ret = wait_for_completion_timeout(&smbus->irq_completion,
> msecs_to_jiffies(100));
> reg_write(smbus, REG_IMASK, 0);
> status = reg_read(smbus, REG_SMSTA);
> +
> + if (ret < 0) {
> + dev_err(smbus->dev,
> + "Completion wait failed with %d, status
> 0x%08x\n",
> + ret, status);
> + return ret;
> + } else if (ret == 0) {
> + dev_warn(smbus->dev, "Timeout, status 0x%08x\n",
> status);
> + return -ETIME;
> + }
> } else {
> status = reg_read(smbus, REG_SMSTA);
> while (!(status & SMSTA_XEN) && timeout--) {
> msleep(1);
> status = reg_read(smbus, REG_SMSTA);
> }
> +
> + if (timeout < 0) {
> + dev_warn(smbus->dev, "Timeout, status 0x%08x\n",
> status);
this is an error and it's treated as an error, can we please
print dev_err()?
Andi
> + return -ETIME;
> + }
> }
Re: [PATCH v2 6/6] i2c: pasemi: Log bus reset causes
Hi Sven,
...
> static int pasemi_smb_clear(struct pasemi_smbus *smbus)
> {
> - unsigned int status;
> + unsigned int status, xfstatus;
Let's declare the variables in the innermost scope where they are
used.
Andi
> int ret;
>
> /* First wait for the bus to go idle */
> @@ -98,7 +99,9 @@ static int pasemi_smb_clear(struct pasemi_smbus *smbus)
>USEC_PER_MSEC, USEC_PER_MSEC *
> TRANSFER_TIMEOUT_MS);
>
> if (ret < 0) {
> - dev_err(smbus->dev, "Bus is still stuck (status 0x%08x)\n",
> status);
> + xfstatus = reg_read(smbus, REG_XFSTA);
> + dev_err(smbus->dev, "Bus is still stuck (status 0x%08x xfstatus
> 0x%08x)\n",
> + status, xfstatus);
> return -EIO;
> }
>
>
> --
> 2.34.1
>
Re: [PATCH v2 4/6] i2c: pasemi: Improve error recovery
Hi Sven, Hector,
...
> +/*
> + * The hardware (supposedly) has a 25ms timeout for clock stretching, thus
> + * use 100ms here which should be plenty.
> + */
> +#define TRANSFER_TIMEOUT_MS 100
Please use the PASEMI prefix here. TRANSFER_TIMEOUT_MS it's not a
naming belonging to this driver.
100ms looks a bit too much to me, but if you say it works, then
it works.
> +
...
> unsigned int status;
>
> if (smbus->use_irq) {
> reinit_completion(&smbus->irq_completion);
> - reg_write(smbus, REG_IMASK, SMSTA_XEN | SMSTA_MTN);
> - ret = wait_for_completion_timeout(&smbus->irq_completion,
> msecs_to_jiffies(100));
> + /* XEN should be set when a transaction terminates, whether due
> to error or not */
> + reg_write(smbus, REG_IMASK, SMSTA_XEN);
> + ret = wait_for_completion_timeout(&smbus->irq_completion,
> + msecs_to_jiffies(timeout));
> reg_write(smbus, REG_IMASK, 0);
> status = reg_read(smbus, REG_SMSTA);
>
> @@ -123,9 +150,35 @@ static int pasemi_smb_waitready(struct pasemi_smbus
> *smbus)
> }
> }
>
> + /* Controller timeout? */
> + if (status & SMSTA_TOM) {
> + dev_warn(smbus->dev, "Controller timeout, status 0x%08x\n",
> status);
> + return -EIO;
as before, these warnings are treated as errors. Can we just
print error?
The rest looks good.
Andi
> + }
> +
> + /* Peripheral timeout? */
> + if (status & SMSTA_MTO) {
> + dev_warn(smbus->dev, "Peripheral timeout, status 0x%08x\n",
> status);
> + return -ETIME;
> + }
...
Re: [PATCH v2 0/6] Apple/PASemi i2c error recovery fixes
Hi Sven, > Hector Martin (3): > i2c: pasemi: Improve error recovery > i2c: pasemi: Enable the unjam machine > i2c: pasemi: Log bus reset causes > > Sven Peter (3): > i2c: pasemi: Use correct bits.h include > i2c: pasemi: Sort includes alphabetically I applied in i2c/i2c-host the two patches above, so that you can start from patch 3 as there were a few little notes. I didn't see necessary the Fixes tag in the bits.h patch so that I removed it. Thanks, Andi
Re: [PATCH 1/3] i2c: powermac: convert of_node usage to fwnode
Hi Wolfram, On Tue, May 20, 2025 at 11:15:21AM +0200, Wolfram Sang wrote: > > > I took this patch in i2c/i2c-host. Please let me know if you want > > me to take also the others. > > To avoid the dependency with your PR, is it okay if you drop it and I > take this patch via my tree? yes, sure! I will take it out. Thanks, Andi
Re: [PATCH 1/3] i2c: powermac: convert of_node usage to fwnode
Hi Wolfram, On Mon, May 19, 2025 at 01:13:12PM +0200, Wolfram Sang wrote: > 'of_node' in i2c_boardinfo is deprecated in favor of 'fwnode'. The I2C > core handles them equally, so simply convert this driver to fwnode. > > Signed-off-by: Wolfram Sang I took this patch in i2c/i2c-host. Please let me know if you want me to take also the others. Thanks, Andi
